From 47ced8460ab090f84d8e11ca5765cca3a898eb75 Mon Sep 17 00:00:00 2001 From: Toon Verwerft Date: Fri, 2 Oct 2026 15:41:52 +0200 Subject: [PATCH] Prevent file paths from being parsed as tool options Tasks pass repository files as separate arguments to external tools, so a file named like "--config=x.php" was read as an option. Paths starting with "-" are now prefixed with "./". A "--" separator is not used because not every tool supports it and several tasks append options such as "--fix" after the file list. git_blacklist now runs with --literal-pathspecs, so a file name starting with ":" is no longer read as pathspec magic. --- .../ProcessArgumentsCollectionSpec.php | 29 ++++++++ src/Collection/ProcessArgumentsCollection.php | 15 +++- src/Task/Git/Blacklist.php | 2 + test/E2E/AbstractE2ETestCase.php | 32 +++++++++ test/E2E/TasksTest.php | 57 +++++++++++++++ test/Unit/Task/Git/BlacklistTest.php | 3 + .../e2e/tasks/ValidateArgvPathsTask.php | 70 +++++++++++++++++++ .../e2e/tasks/validate-argv-paths.php | 17 +++++ 8 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/e2e/tasks/ValidateArgvPathsTask.php create mode 100644 test/fixtures/e2e/tasks/validate-argv-paths.php diff --git a/spec/Collection/ProcessArgumentsCollectionSpec.php b/spec/Collection/ProcessArgumentsCollectionSpec.php index fa9d1f2bb..e3846997d 100644 --- a/spec/Collection/ProcessArgumentsCollectionSpec.php +++ b/spec/Collection/ProcessArgumentsCollectionSpec.php @@ -111,6 +111,35 @@ function it_should_be_able_to_add_files() ]); } + function it_should_prevent_files_from_being_parsed_as_options() + { + $files = new FilesCollection([ + new SplFileInfo('--config=file1.php'), + new SplFileInfo('-c.php'), + new SplFileInfo('-dir/file2.php'), + new SplFileInfo('dir/-file3.php'), + ]); + $this->addFiles($files); + + $this->getValues()->shouldBe([ + './--config=file1.php', + './-c.php', + './-dir/file2.php', + 'dir/-file3.php', + ]); + } + + function it_should_prevent_comma_separated_files_from_being_parsed_as_options() + { + $files = new FilesCollection([ + new SplFileInfo('--config=file1.php'), + new SplFileInfo('file2.php') + ]); + $this->addCommaSeparatedFiles($files); + + $this->getValues()->shouldBe(['./--config=file1.php,file2.php']); + } + function it_should_be_able_to_add_comma_separated_files() { $files = new FilesCollection([ diff --git a/src/Collection/ProcessArgumentsCollection.php b/src/Collection/ProcessArgumentsCollection.php index 3a0f19db5..dafd28e53 100644 --- a/src/Collection/ProcessArgumentsCollection.php +++ b/src/Collection/ProcessArgumentsCollection.php @@ -99,7 +99,7 @@ public function addFiles(FilesCollection $files): void public function addFile(\SplFileInfo $file): void { - $this->add($file->getPathname()); + $this->add(self::escapeFilePath($file)); } public function addCommaSeparatedFiles(FilesCollection $files): void @@ -107,12 +107,23 @@ public function addCommaSeparatedFiles(FilesCollection $files): void $paths = []; foreach ($files as $file) { - $paths[] = $file->getPathname(); + $paths[] = self::escapeFilePath($file); } $this->add(implode(',', $paths)); } + /** + * A repository file named like "--config=evil.php" would otherwise be parsed as an option by the tool. + * Prefixing it with "./" keeps it a path, without relying on every tool supporting the "--" separator. + */ + private static function escapeFilePath(\SplFileInfo $file): string + { + $path = $file->getPathname(); + + return str_starts_with($path, '-') ? './'.$path : $path; + } + public function addArgumentWithCommaSeparatedFiles(string $argument, FilesCollection $files): void { $paths = []; diff --git a/src/Task/Git/Blacklist.php b/src/Task/Git/Blacklist.php index f30793f8a..edfbcb5ee 100644 --- a/src/Task/Git/Blacklist.php +++ b/src/Task/Git/Blacklist.php @@ -83,6 +83,8 @@ public function run(ContextInterface $context): TaskResultInterface } $arguments = $this->processBuilder->createArgumentsForCommand('git'); + // Without this, a committed file named like ":!other.php" is read as pathspec magic and excludes files. + $arguments->add('--literal-pathspecs'); $arguments->add('grep'); $arguments->add('--cached'); $arguments->add('-n'); diff --git a/test/E2E/AbstractE2ETestCase.php b/test/E2E/AbstractE2ETestCase.php index ac7d25c97..8aec36233 100644 --- a/test/E2E/AbstractE2ETestCase.php +++ b/test/E2E/AbstractE2ETestCase.php @@ -347,6 +347,38 @@ protected function enableDummyTask(string $grumphpFile, string $projectDir, arra ]); } + protected function enableValidateArgvPathsTask(string $grumphpFile, string $projectDir) + { + $e2eDir = $this->ensureGrumphpE2eTasksDir($projectDir); + $this->dumpFile( + $e2eDir.'/ValidateArgvPathsTask.php', + file_get_contents(TEST_BASE_PATH.'/fixtures/e2e/tasks/ValidateArgvPathsTask.php') + ); + $this->dumpFile( + $e2eDir.'/validate-argv-paths.php', + file_get_contents(TEST_BASE_PATH.'/fixtures/e2e/tasks/validate-argv-paths.php') + ); + + $this->mergeGrumphpConfig($grumphpFile, [ + 'grumphp' => [ + 'tasks' => [ + 'validateArgvPaths' => [], + ], + ], + 'services' => [ + 'GrumPHPE2E\\ValidateArgvPathsTask' => [ + 'arguments' => ['@process_builder'], + 'tags' => [ + [ + 'name' => 'grumphp.task', + 'task' => 'validateArgvPaths' + ], + ] + ] + ], + ]); + } + protected function installComposer(string $path, array $arguments = []) { $process = new Process( diff --git a/test/E2E/TasksTest.php b/test/E2E/TasksTest.php index 90c7fbcbd..1a1f8313e 100644 --- a/test/E2E/TasksTest.php +++ b/test/E2E/TasksTest.php @@ -48,4 +48,61 @@ function it_can_resolve_task_config_With_env_vars() $this->commitAll(); $this->runGrumphp($this->rootDir); } + + #[Test] + function it_passes_option_like_file_names_to_external_commands_as_paths() + { + $this->initializeGitInRootDir(); + $this->initializeComposer($this->rootDir); + $grumphpFile = $this->initializeGrumphpConfig($this->rootDir); + $this->installComposer($this->rootDir); + $this->ensureHooksExist(); + + $this->enableValidateArgvPathsTask($grumphpFile, $this->rootDir); + $this->dumpOptionLikeFiles(); + + $this->commitAll(); + $this->runGrumphp($this->rootDir); + } + + #[Test] + function it_finds_blacklisted_keywords_in_option_like_file_names() + { + $this->initializeGitInRootDir(); + $this->initializeComposer($this->rootDir); + $grumphpFile = $this->initializeGrumphpConfig($this->rootDir); + $this->installComposer($this->rootDir); + $this->ensureHooksExist(); + + $this->mergeGrumphpConfig($grumphpFile, [ + 'grumphp' => [ + 'tasks' => [ + 'git_blacklist' => [ + 'keywords' => ['blacklisted_keyword'], + ], + ], + ], + ]); + $this->dumpOptionLikeFiles(); + $this->dumpFile($this->rootDir.'/-dash.php', 'commitAll(); + } catch (\RuntimeException $e) { + $this->assertStringContainsString('You have blacklisted keywords in your commit', $e->getMessage()); + $this->assertStringContainsString('-dash.php', $e->getMessage()); + + return; + } + + $this->fail('Expected git_blacklist to find the keyword in -dash.php.'); + } + + private function dumpOptionLikeFiles(): void + { + $this->dumpFile($this->rootDir.'/--option-like=value.php', 'dumpFile($this->rootDir.'/-dash.php', 'mkdir($this->rootDir.'/-dir'); + $this->dumpFile($this->rootDir.'/-dir/file.php', 'config = new EmptyTaskConfig(); + $this->processBuilder = $processBuilder; + } + + public function getConfig(): TaskConfigInterface + { + return $this->config; + } + + public function withConfig(TaskConfigInterface $config): TaskInterface + { + $new = clone $this; + $new->config = $config; + + return $new; + } + + public static function getConfigurableOptions(): ConfigOptionsResolver + { + return ConfigOptionsResolver::fromOptionsResolver(new OptionsResolver()); + } + + public function canRunInContext(ContextInterface $context): bool + { + return true; + } + + public function run(ContextInterface $context): TaskResultInterface + { + $arguments = $this->processBuilder->createArgumentsForCommand('php'); + $arguments->add(__DIR__.'/validate-argv-paths.php'); + $arguments->addFiles($context->getFiles()); + + $process = $this->processBuilder->buildProcess($arguments); + $process->run(); + + if (!$process->isSuccessful()) { + return TaskResult::createFailed($this, $context, $process->getOutput().$process->getErrorOutput()); + } + + return TaskResult::createPassed($this, $context); + } +} diff --git a/test/fixtures/e2e/tasks/validate-argv-paths.php b/test/fixtures/e2e/tasks/validate-argv-paths.php new file mode 100644 index 000000000..97d3d7507 --- /dev/null +++ b/test/fixtures/e2e/tasks/validate-argv-paths.php @@ -0,0 +1,17 @@ +