Merge pull request #780 from 1ed/command-escaping

Fix command escaping
phpro · Jul 9, 2020 · 8457d88 · 8457d88
2 parents 936e2b3 + 2237f98
commit 8457d88
Show file tree

Hide file tree

Showing 14 changed files with 39 additions and 183 deletions.
diff --git a/doc/parameters.md b/doc/parameters.md
@@ -50,20 +50,24 @@ This parameter will allow you to customize git hooks templates. For now, those p
 
 -  `VAGRANT_HOST_DIR` : specifies the vagrant location on your host machine relative to the git working folder (_default_ `.`)
 -  `VAGRANT_PROJECT_DIR` : specifies the project dir location **inside** the vagrant box (_default_ `/var/www`)
--  `EXEC_GRUMPHP_COMMAND` : specifies the name of the command that will execute the grumphp script (_default_ `exec`)
-
-Examples: 
-
-```yaml
-grumphp:
-    git_hook_variables:
-        EXEC_GRUMPHP_COMMAND: '/usr/local/bin/php72'
-        EXEC_GRUMPHP_COMMAND: 'lando php'
-        EXEC_GRUMPHP_COMMAND: 'fin exec php'
-        EXEC_GRUMPHP_COMMAND: 'php -c /custom/config.ini'
-        EXEC_GRUMPHP_COMMAND: 'docker-compose run --rm --no-deps php'
-        EXEC_GRUMPHP_COMMAND: 'docker run --rm -it -v $(pwd):/grumphp -w /grumphp webdevops/php:alpine'
-```
+-  `EXEC_GRUMPHP_COMMAND` : specifies the command that will execute the grumphp script (_default_ `exec`)
+
+    It can be a string or an array. If you provide an array, its every element will be escaped by symfony/process.
+    If you provide a string, it will be used as-is, without escaping and if it contains special characters you may
+    need to escape them.   
+
+    Examples: 
+
+    ```yaml
+    grumphp:
+        git_hook_variables:
+            EXEC_GRUMPHP_COMMAND: '/usr/local/bin/php72'
+            EXEC_GRUMPHP_COMMAND: 'lando php'
+            EXEC_GRUMPHP_COMMAND: 'fin exec php'
+            EXEC_GRUMPHP_COMMAND: ['php', '-c /custom/config.ini']
+            EXEC_GRUMPHP_COMMAND: ['docker-compose', 'run', '--rm', '--no-deps', 'php']
+            EXEC_GRUMPHP_COMMAND: 'docker run --rm -it -v $(pwd):/grumphp -w /grumphp webdevops/php:alpine'
+    ```
 
 **stop_on_failure**
 

diff --git a/spec/Formatter/ESLintFormatterSpec.php b/spec/Formatter/ESLintFormatterSpec.php
@@ -6,7 +6,6 @@
 use GrumPHP\Formatter\ProcessFormatterInterface;
 use PhpSpec\ObjectBehavior;
 use Symfony\Component\Process\Process;
-use GrumPHP\Process\ProcessUtils;
 
 class ESLintFormatterSpec extends ObjectBehavior
 {

diff --git a/spec/Formatter/PhpCsFixerFormatterSpec.php b/spec/Formatter/PhpCsFixerFormatterSpec.php
@@ -6,7 +6,6 @@
 use GrumPHP\Formatter\ProcessFormatterInterface;
 use PhpSpec\ObjectBehavior;
 use Symfony\Component\Process\Process;
-use GrumPHP\Process\ProcessUtils;
 
 class PhpCsFixerFormatterSpec extends ObjectBehavior
 {
@@ -78,17 +77,6 @@ function it_should_be_possible_to_reset_the_counter(Process $process)
         $this->format($process)->shouldBe('1) name1');
     }
 
-    function it_formats_suggestions(Process $process)
-    {
-        $dryRun = ProcessUtils::escapeArgument('--dry-run');
-        $formatJson = ProcessUtils::escapeArgument('--format=json');
-
-        $command = sprintf('phpcsfixer %s %s .', $dryRun, $formatJson);
-
-        $process->getCommandLine()->willReturn($command);
-        $this->formatSuggestion($process)->shouldReturn('phpcsfixer .');
-    }
-
     function it_formats_the_error_message()
     {
         $this->formatErrorMessage(['message1'], ['message2'])->shouldBeString();

diff --git a/spec/Process/ProcessBuilderSpec.php b/spec/Process/ProcessBuilderSpec.php
@@ -9,8 +9,8 @@
 use GrumPHP\Process\ProcessBuilder;
 use PhpSpec\Exception\Example\FailureException;
 use PhpSpec\ObjectBehavior;
+use Prophecy\Argument;
 use Symfony\Component\Process\Process;
-use GrumPHP\Process\ProcessUtils;
 
 class ProcessBuilderSpec extends ObjectBehavior
 {
@@ -74,7 +74,7 @@ function it_outputs_the_command_when_run_very_very_verbose(IOInterface $io)
         $io->isVeryVerbose()->willReturn(true);
 
         $command = '/usr/bin/grumphp';
-        $io->write([PHP_EOL . 'Command: ' . ProcessUtils::escapeArgument($command)], true)->shouldBeCalled();
+        $io->write(Argument::withEveryEntry(Argument::containingString($command)), true)->shouldBeCalled();
 
         $arguments = new ProcessArgumentsCollection([$command]);
         $this->buildProcess($arguments);

diff --git a/spec/Process/ProcessUtilsSpec.php b/spec/Process/ProcessUtilsSpec.php
diff --git a/src/Composer/DevelopmentIntegrator.php b/src/Composer/DevelopmentIntegrator.php
@@ -7,9 +7,9 @@
 use Composer\Script\Event;
 use GrumPHP\Collection\ProcessArgumentsCollection;
 use GrumPHP\Process\ProcessFactory;
-use GrumPHP\Process\ProcessUtils;
 use GrumPHP\Util\Filesystem;
 use Symfony\Component\Process\Process;
+use Symfony\Component\Process\ProcessBuilder;
 
 class DevelopmentIntegrator
 {
@@ -55,11 +55,10 @@ public static function integrate(Event $event): void
     private static function fixInternalComposerProcessVersion(
         ProcessArgumentsCollection $commandlineArgs
     ): Process {
-        $process = ProcessFactory::fromArguments($commandlineArgs);
-        if (\is_string($process->getCommandLine())) {
-            return $process;
+        if (class_exists(ProcessBuilder::class, true)) {
+            return ProcessBuilder::create($commandlineArgs->getValues())->getProcess();
         }
 
-        return new Process(ProcessUtils::escapeArguments($commandlineArgs->getValues()));
+        return ProcessFactory::fromArguments($commandlineArgs);
     }
 }
diff --git a/src/Configuration/Configuration.php b/src/Configuration/Configuration.php
@@ -28,7 +28,7 @@ public function getConfigTreeBuilder(): TreeBuilder
         $gitHookVariables->addDefaultsIfNotSet();
         $gitHookVariables->children()->scalarNode('VAGRANT_HOST_DIR')->defaultValue('.');
         $gitHookVariables->children()->scalarNode('VAGRANT_PROJECT_DIR')->defaultValue('/var/www');
-        $gitHookVariables->children()->scalarNode('EXEC_GRUMPHP_COMMAND')->defaultValue('exec');
+        $gitHookVariables->children()->variableNode('EXEC_GRUMPHP_COMMAND')->defaultValue('exec');
 
         $rootNode->children()->scalarNode('additional_info')->defaultNull();
 

diff --git a/src/Console/Command/Git/InitCommand.php b/src/Console/Command/Git/InitCommand.php
@@ -6,13 +6,13 @@
 
 use GrumPHP\Configuration\Model\HooksConfig;
 use GrumPHP\Process\ProcessBuilder;
-use GrumPHP\Process\ProcessUtils;
 use GrumPHP\Util\Filesystem;
 use GrumPHP\Util\Paths;
 use RuntimeException;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Process\Process;
 
 /**
  * This command is responsible for enabling all the configured hooks.
@@ -133,7 +133,8 @@ protected function parseHookBody(string $hook, string $templateFile): string
         ];
 
         foreach ($this->hooksConfig->getVariables() as $key => $value) {
-            $replacements[sprintf('$(%s)', $key)] = ProcessUtils::escapeArgumentsFromString($value);
+            $process = is_array($value) ? new Process($value) : Process::fromShellCommandline($value);
+            $replacements[sprintf('$(%s)', $key)] = $process->getCommandLine();
         }
 
         return str_replace(array_keys($replacements), array_values($replacements), $content);

diff --git a/src/Formatter/PhpCsFixerFormatter.php b/src/Formatter/PhpCsFixerFormatter.php
@@ -5,7 +5,6 @@
 namespace GrumPHP\Formatter;
 
 use Symfony\Component\Process\Process;
-use GrumPHP\Process\ProcessUtils;
 
 class PhpCsFixerFormatter implements ProcessFormatterInterface
 {
@@ -36,16 +35,6 @@ public function format(Process $process): string
         return $this->formatJsonResponse($json);
     }
 
-    public function formatSuggestion(Process $process): string
-    {
-        $pattern = '%s ';
-
-        $dryrun = sprintf($pattern, ProcessUtils::escapeArgument('--dry-run'));
-        $formatJson = sprintf($pattern, ProcessUtils::escapeArgument('--format=json'));
-
-        return str_replace([$dryrun, $formatJson], '', $process->getCommandLine());
-    }
-
     public function formatErrorMessage(array $messages, array $suggestions): string
     {
         return sprintf(

diff --git a/src/Process/ProcessUtils.php b/src/Process/ProcessUtils.php
diff --git a/src/Task/PhpCsFixer.php b/src/Task/PhpCsFixer.php
@@ -102,7 +102,12 @@ public function run(ContextInterface $context): TaskResultInterface
 
         if (!$process->isSuccessful()) {
             $messages = [$this->formatter->format($process)];
-            $fixerCommand = $this->formatter->formatSuggestion($process);
+
+            $arguments->removeElement('--format=json');
+            $arguments->removeElement('--dry-run');
+
+            $process = $this->processBuilder->buildProcess($arguments);
+            $fixerCommand = $process->getCommandLine();
             $errorMessage = $this->formatter->formatErrorMessage($messages, [$fixerCommand]);
 
             return new FixableTaskResult(

diff --git a/test/E2E/AbstractE2ETestCase.php b/test/E2E/AbstractE2ETestCase.php
@@ -95,7 +95,7 @@ protected function initializeComposer(string $path): string
     {
         $process = new Process(
             [
-                $this->executableFinder->find('composer'),
+                realpath($this->executableFinder->find('composer', 'composer')),
                 'init',
                 '--name=grumphp/testsuite'.$this->hash,
                 '--type=library',
@@ -290,7 +290,7 @@ protected function installComposer(string $path, array $arguments = [])
         $process = new Process(
             array_merge(
                 [
-                    $this->executableFinder->find('composer'),
+                    realpath($this->executableFinder->find('composer', 'composer')),
                     'install',
                     '--optimize-autoloader',
                     '--no-interaction',

diff --git a/test/E2E/GitHookParametersTest.php b/test/E2E/GitHookParametersTest.php
@@ -41,14 +41,14 @@ function it_can_specify_hook_exec_command_with_additional_arguments()
         $this->mergeGrumphpConfig($grumphpFile, [
             'grumphp' => [
                 'git_hook_variables' => [
-                    'EXEC_GRUMPHP_COMMAND' => $php.' -d date.timezone=Europe/Brussels',
+                    'EXEC_GRUMPHP_COMMAND' => [$php, '-d date.timezone=Europe/Brussels'],
                 ],
             ],
         ]);
 
         $this->installComposer($this->rootDir);
 
-        $hookPattern = '{[\'"]?'.preg_quote($php, '{').'[\'"]? [\'"]?-d[\'"]? [\'"]?date\.timezone=Europe/Brussels[\'"]?}';
+        $hookPattern = sprintf('{[\'"]%s[\'"] [\'"]-d date\.timezone=Europe/Brussels[\'"]}', preg_quote($php));
         $this->ensureHooksExist($this->rootDir, $hookPattern);
 
         $this->enableValidatePathsTask($grumphpFile, $this->rootDir);

diff --git a/test/Unit/Task/PhpCsFixerTest.php b/test/Unit/Task/PhpCsFixerTest.php
@@ -76,8 +76,7 @@ function () {
 
                 $this->formatter->resetCounter()->shouldBeCalled();
                 $this->formatter->format($process)->willReturn($message = 'message');
-                $this->formatter->formatSuggestion($process)->willReturn($suggestion = 'suggestion');
-                $this->formatter->formatErrorMessage([$message], [$suggestion])->willReturn('nope');
+                $this->formatter->formatErrorMessage([$message], ['mocked-cli'])->willReturn('nope');
             },
             'nope',
             FixableTaskResult::class