From 611b215896bca622761c590d91b967afaff5c46c Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 22 Jun 2022 15:39:51 +0200 Subject: [PATCH] Fix PHPStan issues and a couple minor bugs in audit functionality, refs #10798 --- src/Composer/Command/AuditCommand.php | 4 +-- src/Composer/Command/BaseCommand.php | 20 +++++++++++ src/Composer/Command/CreateProjectCommand.php | 2 +- src/Composer/Command/InstallCommand.php | 2 +- src/Composer/Command/RemoveCommand.php | 2 +- src/Composer/Command/RequireCommand.php | 4 +-- src/Composer/Command/UpdateCommand.php | 2 +- src/Composer/Installer.php | 6 ++-- src/Composer/Util/Auditor.php | 10 +++--- tests/Composer/Test/Util/AuditorTest.php | 36 +++++++++---------- 10 files changed, 53 insertions(+), 35 deletions(-) diff --git a/src/Composer/Command/AuditCommand.php b/src/Composer/Command/AuditCommand.php index e9770eb1c..4c093c372 100644 --- a/src/Composer/Command/AuditCommand.php +++ b/src/Composer/Command/AuditCommand.php @@ -9,7 +9,7 @@ use Composer\Package\PackageInterface; 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 @@ EOT } $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 index 89c399624..afd2ae33b 100644 --- a/src/Composer/Command/BaseCommand.php +++ b/src/Composer/Command/BaseCommand.php @@ -25,6 +25,7 @@ use Composer\IO\NullIO; 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; @@ -405,4 +406,23 @@ abstract class BaseCommand extends Command 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 index 0a7657271..3c25b06b0 100644 --- a/src/Composer/Command/CreateProjectCommand.php +++ b/src/Composer/Command/CreateProjectCommand.php @@ -264,7 +264,7 @@ EOT ->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 index d707ecbb4..015c7362d 100644 --- a/src/Composer/Command/InstallCommand.php +++ b/src/Composer/Command/InstallCommand.php @@ -134,7 +134,7 @@ EOT ->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 index 5cb509b92..bfa3fe825 100644 --- a/src/Composer/Command/RemoveCommand.php +++ b/src/Composer/Command/RemoveCommand.php @@ -286,7 +286,7 @@ EOT ->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 index 9b2ae44c7..d2898fbba 100644 --- a/src/Composer/Command/RequireCommand.php +++ b/src/Composer/Command/RequireCommand.php @@ -419,7 +419,7 @@ EOT ->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 @@ EOT protected function interact(InputInterface $input, OutputInterface $output): void { - + } /** diff --git a/src/Composer/Command/UpdateCommand.php b/src/Composer/Command/UpdateCommand.php index 94092609e..7df4cc397 100644 --- a/src/Composer/Command/UpdateCommand.php +++ b/src/Composer/Command/UpdateCommand.php @@ -233,7 +233,7 @@ EOT ->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 index 36825de36..572f6870c 100644 --- 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 @@ class Installer /** * 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 index 7b3cb0715..3bb22dd25 100644 --- a/src/Composer/Util/Auditor.php +++ b/src/Composer/Util/Auditor.php @@ -53,7 +53,8 @@ class Auditor if (count($advisories) > 0) { $numAdvisories = $this->countAdvisories($advisories); $plurality = $numAdvisories === 1 ? 'y' : 'ies'; - $io->writeError("<$errorOrWarn>Found $numAdvisories security vulnerability advisor$plurality:"); + $punctuation = $format === 'summary' ? '.' : ':'; + $io->writeError("<$errorOrWarn>Found $numAdvisories security vulnerability advisor{$plurality}{$punctuation}"); $this->outputAdvisories($io, $advisories, $format); return 1; } @@ -177,8 +178,9 @@ class Auditor 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 @@ class Auditor ]) ->addRow([ $package, - $advisory['cve'] ?: 'NO CVE', + $advisory['cve'] ?? 'NO CVE', $advisory['title'], $advisory['link'], $advisory['affectedVersions'], @@ -230,7 +232,7 @@ class Auditor 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 index 8a64a56ee..ad4c06f65 100644 --- 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 @@ use PHPUnit\Framework\MockObject\MockObject; 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 @@ class AuditorTest extends TestCase $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 @@ class AuditorTest extends TestCase 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 @@ class AuditorTest extends TestCase ->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 @@ class AuditorTest extends TestCase 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 @@ class AuditorTest extends TestCase return $httpDownloader; } - public static function getMockAdvisories(?int $updatedSince) + /** + * @return array + */ + public static function getMockAdvisories(?int $updatedSince): array { $advisories = [ 'vendor1/package1' => [ @@ -326,8 +323,7 @@ class AuditorTest extends TestCase ], ]; - // Intentionally allow updatedSince === 0 to include these advisories - if (!$updatedSince) { + if (0 === $updatedSince || null === $updatedSince) { $advisories['vendor1/package1'][] = [ 'advisoryId' => 'ID5', 'packageName' => 'vendor1/package1',