Fix PHPStan issues and a couple minor bugs in audit functionality, refs

composer#10798
emahorvat52 · Jan 18, 2023 · b189d74 · b189d74
1 parent 9dc1b06
commit b189d74
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 35 deletions.
diff --git a/src/Composer/Command/AuditCommand.php b/src/Composer/Command/AuditCommand.php
@@ -9,7 +9,7 @@
 use Composer\Repository\InstalledRepository;
 use Composer\Repository\RepositoryInterface;
 use Composer\Util\Auditor;
-use Symfony\Component\Console\Input\InputOption;
+use Composer\Console\Input\InputOption;
 
 class AuditCommand extends BaseCommand
 {
@@ -47,7 +47,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
         }
 
         $auditor = new Auditor($httpDownloader);
-        return $auditor->audit($this->getIO(), $packages, $input->getOption('format'), false);
+        return $auditor->audit($this->getIO(), $packages, $this->getAuditFormat($input, 'format'), false);
     }
 
     /**

diff --git a/src/Composer/Command/BaseCommand.php b/src/Composer/Command/BaseCommand.php
@@ -25,6 +25,7 @@
 use Composer\Plugin\PreCommandRunEvent;
 use Composer\Package\Version\VersionParser;
 use Composer\Plugin\PluginEvents;
+use Composer\Util\Auditor;
 use Composer\Util\Platform;
 use Symfony\Component\Console\Completion\CompletionInput;
 use Symfony\Component\Console\Completion\CompletionSuggestions;
@@ -410,4 +411,23 @@ protected function getTerminalWidth()
 
         return $width;
     }
+
+    /**
+     * @internal
+     * @param 'format'|'audit-format' $optName
+     * @return Auditor::FORMAT_*
+     */
+    protected function getAuditFormat(InputInterface $input, string $optName = 'audit-format'): string
+    {
+        if (!$input->hasOption($optName)) {
+            throw new \LogicException('This should not be called on a Command which has no '.$optName.' option defined.');
+        }
+
+        $val = $input->getOption($optName);
+        if (!in_array($val, Auditor::FORMATS, true)) {
+            throw new \InvalidArgumentException('--'.$optName.' must be one of '.implode(', ', Auditor::FORMATS).'.');
+        }
+
+        return $val;
+    }
 }
diff --git a/src/Composer/Command/CreateProjectCommand.php b/src/Composer/Command/CreateProjectCommand.php
@@ -265,7 +265,7 @@ public function installProject(IOInterface $io, Config $config, InputInterface $
                 ->setClassMapAuthoritative($config->get('classmap-authoritative'))
                 ->setApcuAutoloader($config->get('apcu-autoloader'))
                 ->setAudit(!$input->getOption('no-audit'))
-                ->setAuditFormat($input->getOption('audit-format'));
+                ->setAuditFormat($this->getAuditFormat($input));
 
             if (!$composer->getLocker()->isLocked()) {
                 $installer->setUpdate(true);

diff --git a/src/Composer/Command/InstallCommand.php b/src/Composer/Command/InstallCommand.php
@@ -134,7 +134,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
             ->setApcuAutoloader($apcu, $apcuPrefix)
             ->setPlatformRequirementFilter($this->getPlatformRequirementFilter($input))
             ->setAudit(!$input->getOption('no-audit'))
-            ->setAuditFormat($input->getOption('audit-format'))
+            ->setAuditFormat($this->getAuditFormat($input))
         ;
 
         if ($input->getOption('no-plugins')) {

diff --git a/src/Composer/Command/RemoveCommand.php b/src/Composer/Command/RemoveCommand.php
@@ -286,7 +286,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
             ->setPlatformRequirementFilter($this->getPlatformRequirementFilter($input))
             ->setDryRun($dryRun)
             ->setAudit(!$input->getOption('no-audit'))
-            ->setAuditFormat($input->getOption('audit-format'))
+            ->setAuditFormat($this->getAuditFormat($input))
         ;
 
         // if no lock is present, we do not do a partial update as

diff --git a/src/Composer/Command/RequireCommand.php b/src/Composer/Command/RequireCommand.php
@@ -419,7 +419,7 @@ private function doUpdate(InputInterface $input, OutputInterface $output, IOInte
             ->setPreferStable($input->getOption('prefer-stable'))
             ->setPreferLowest($input->getOption('prefer-lowest'))
             ->setAudit(!$input->getOption('no-audit'))
-            ->setAuditFormat($input->getOption('audit-format'))
+            ->setAuditFormat($this->getAuditFormat($input))
         ;
 
         // if no lock is present, or the file is brand new, we do not do a
@@ -475,7 +475,7 @@ private function updateFileCleanly(JsonFile $json, array $new, string $requireKe
 
     protected function interact(InputInterface $input, OutputInterface $output): void
     {
-        
+
     }
 
     /**

diff --git a/src/Composer/Command/UpdateCommand.php b/src/Composer/Command/UpdateCommand.php
@@ -233,7 +233,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
             ->setPreferLowest($input->getOption('prefer-lowest'))
             ->setTemporaryConstraints($temporaryConstraints)
             ->setAudit(!$input->getOption('no-audit'))
-            ->setAuditFormat($input->getOption('audit-format'))
+            ->setAuditFormat($this->getAuditFormat($input))
         ;
 
         if ($input->getOption('no-plugins')) {

diff --git a/src/Composer/Installer.php b/src/Composer/Installer.php
@@ -167,8 +167,8 @@ class Installer
     protected $executeOperations = true;
     /** @var bool */
     protected $audit = true;
-    /** @var string */
-    protected $auditFormat = Auditor::FORMAT_TABLE;
+    /** @var Auditor::FORMAT_* */
+    protected $auditFormat = Auditor::FORMAT_SUMMARY;
 
     /** @var bool */
     protected $updateMirrors = false;
@@ -1460,7 +1460,7 @@ public function setAudit(bool $audit): self
     /**
      * What format should be used for audit output?
      *
-     * @param string $auditFormat
+     * @param Auditor::FORMAT_* $auditFormat
      * @return Installer
      */
     public function setAuditFormat(string $auditFormat): self

diff --git a/src/Composer/Util/Auditor.php b/src/Composer/Util/Auditor.php
@@ -53,7 +53,8 @@ public function audit(IOInterface $io, array $packages, string $format, bool $wa
         if (count($advisories) > 0) {
             $numAdvisories = $this->countAdvisories($advisories);
             $plurality = $numAdvisories === 1 ? 'y' : 'ies';
-            $io->writeError("<$errorOrWarn>Found $numAdvisories security vulnerability advisor$plurality:</$errorOrWarn>");
+            $punctuation = $format === 'summary' ? '.' : ':';
+            $io->writeError("<$errorOrWarn>Found $numAdvisories security vulnerability advisor{$plurality}{$punctuation}</$errorOrWarn>");
             $this->outputAdvisories($io, $advisories, $format);
             return 1;
         }
@@ -177,8 +178,9 @@ private function outputAdvisories(IOInterface $io, array $advisories, string $fo
             case self::FORMAT_SUMMARY:
                 // We've already output the number of advisories in audit()
                 $io->writeError('Run composer audit for a full list of advisories.');
+                return;
             default:
-                throw new InvalidArgumentException('Invalid format.');
+                throw new InvalidArgumentException('Invalid format "'.$format.'".');
         }
     }
 
@@ -203,7 +205,7 @@ private function outputAvisoriesTable(ConsoleIO $io, array $advisories): void
                     ])
                     ->addRow([
                         $package,
-                        $advisory['cve'] ?: 'NO CVE',
+                        $advisory['cve'] ?? 'NO CVE',
                         $advisory['title'],
                         $advisory['link'],
                         $advisory['affectedVersions'],
@@ -230,7 +232,7 @@ private function outputAdvisoriesPlain(IOInterface $io, array $advisories): void
                 if (!$firstAdvisory) {
                     $error[] = '--------';
                 }
-                $cve = $advisory['cve'] ?: 'NO CVE';
+                $cve = $advisory['cve'] ?? 'NO CVE';
                 $error[] = "Package: $package";
                 $error[] = "CVE: $cve";
                 $error[] = "Title: {$advisory['title']}";

diff --git a/tests/Composer/Test/Util/AuditorTest.php b/tests/Composer/Test/Util/AuditorTest.php
@@ -3,6 +3,8 @@
 namespace Composer\Test\Util;
 
 use Composer\IO\IOInterface;
+use Composer\IO\NullIO;
+use Composer\Json\JsonFile;
 use Composer\Package\Package;
 use Composer\Test\TestCase;
 use Composer\Util\Auditor;
@@ -13,17 +15,6 @@
 
 class AuditorTest extends TestCase
 {
-    /** @var IOInterface&\PHPUnit\Framework\MockObject\MockObject */
-    private $io;
-
-    protected function setUp(): void
-    {
-        $this->io = $this
-            ->getMockBuilder(IOInterface::class)
-            ->disableOriginalConstructor()
-            ->getMock();
-    }
-
     public function auditProvider()
     {
         return [
@@ -75,11 +66,14 @@ public function testAudit(array $data, int $expected, string $message): void
             $this->expectException(InvalidArgumentException::class);
         }
         $auditor = new Auditor($this->getHttpDownloader());
-        $result = $auditor->audit($this->io, $data['packages'], Auditor::FORMAT_PLAIN, $data['warningOnly']);
+        $result = $auditor->audit(new NullIO(), $data['packages'], Auditor::FORMAT_PLAIN, $data['warningOnly']);
         $this->assertSame($expected, $result, $message);
     }
 
-    public function advisoriesProvider()
+    /**
+     * @return mixed[]
+     */
+    public function advisoriesProvider(): array
     {
         $advisories = static::getMockAdvisories(null);
         return [
@@ -187,7 +181,7 @@ public function testGetAdvisories(array $data, array $expected, string $message)
         if (count($data['packages']) === 0 && $data['updatedSince'] === null) {
             $this->expectException(InvalidArgumentException::class);
         }
-        $auditor = new Auditor($this->getHttpDownloader(), Auditor::FORMAT_PLAIN);
+        $auditor = new Auditor($this->getHttpDownloader());
         $result = $auditor->getAdvisories($data['packages'], $data['updatedSince'], $data['filterByVersion']);
         $this->assertSame($expected, $result, $message);
     }
@@ -204,7 +198,7 @@ private function getHttpDownloader(): MockObject
             ->getMock();
 
         $callback = function(string $url, array $options) {
-            parse_str(parse_url($url, PHP_URL_QUERY) ?? '', $query);
+            parse_str((string) parse_url($url, PHP_URL_QUERY), $query);
             $updatedSince = null;
             if (isset($query['updatedSince'])) {
                 $updatedSince = $query['updatedSince'];
@@ -217,13 +211,13 @@ private function getHttpDownloader(): MockObject
                 parse_str($options['http']['content'], $body);
                 $packages = $body['packages'];
                 foreach ($advisories as $package => $data) {
-                    if (!in_array($package, $packages)) {
+                    if (!in_array($package, $packages, true)) {
                         unset($advisories[$package]);
                     }
                 }
             }
 
-            return new Response(['url' => 'https://packagist.org/api/security-advisories/'], 200, [], json_encode(['advisories' => $advisories]));
+            return new Response(['url' => 'https://packagist.org/api/security-advisories/'], 200, [], JsonFile::encode(['advisories' => $advisories]));
         };
 
         $httpDownloader
@@ -233,7 +227,10 @@ private function getHttpDownloader(): MockObject
         return $httpDownloader;
     }
 
-    public static function getMockAdvisories(?int $updatedSince)
+    /**
+     * @return array<mixed>
+     */
+    public static function getMockAdvisories(?int $updatedSince): array
     {
         $advisories = [
             'vendor1/package1' => [
@@ -326,8 +323,7 @@ public static function getMockAdvisories(?int $updatedSince)
             ],
         ];
 
-        // Intentionally allow updatedSince === 0 to include these advisories
-        if (!$updatedSince) {
+        if (0 === $updatedSince || null === $updatedSince) {
             $advisories['vendor1/package1'][] = [
                 'advisoryId' => 'ID5',
                 'packageName' => 'vendor1/package1',