Fix command escaping #780

1ed · 2020-06-27T14:44:55Z

Q	A
Branch	master
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Documented?	no
Fixed tickets	#779, #733

The first commit what I need, the others are optional. I removed PhpCsFixerFormatter::formatSuggestion() which was public and not internal so I consider it as a BC break.

veewee

Hello @1ed,

Thanks for the PR! This part of the tool is haunting us for some time, so I want to be carefull in 'how' we fix the issue.
That is why I added quite a lot of remarks and questions to the PR. Can you take a look at them?

veewee · 2020-06-29T13:51:20Z

src/Composer/DevelopmentIntegrator.php

@@ -60,6 +59,6 @@ private static function fixInternalComposerProcessVersion(
            return $process;
        }

-        return new Process(ProcessUtils::escapeArguments($commandlineArgs->getValues()));
+        return new Process($commandlineArgs->getValues());


Sadly, composer 1 and 2 comes shipped with a hardcoded symfony/process v2.8 dependency:
https://github.com/composer/composer/blob/master/composer.lock#L949-L950

The development integrator runs as a composer script in a php context which contains the composer dependencies rather than with the application's dependencies.
This means that this code won't work since you now pass an array instead of a string.

We could add a method ProcessArgumentsCollection::asCommandLineString() or something similar instead.

veewee · 2020-06-29T13:54:35Z

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');


I think it is better to make it an arrayNode but to also accept string input. If it is a string input, the config is normalized into an array. (That should be possible with the symfony configuration component)
This change also needs to be added to the documentation.

veewee · 2020-06-29T13:59:18Z

spec/Process/ProcessBuilderSpec.php

@@ -74,7 +73,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([PHP_EOL . sprintf('Command: %1$s%2$s%1$s', '\\' === DIRECTORY_SEPARATOR ? '"' : "'", $command)], true)->shouldBeCalled();


maybe instead of adding this directory separator logic : pass it through
Process::fromShellCommandline($command)->getCommandLine()
Instead?

Thay way it will always work the way the loaded symfony/process wants it to be formatted?

Process::fromShellCommandline not escapes the command. I added a check if the call to $io->write contains the command string, maybe that's enough

veewee · 2020-06-29T14:10:13Z

src/Console/Command/Git/InitCommand.php

@@ -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);


Wouldn't it be better to add a configuration normalizer that converts a string into an array?
That way we can always assume we have an array, even if the grumphp.yaml contains a string.

I am not sure how this solves your issue though: it still escapes the configured CLI argument.
If you are on windows, it escapes with double quotes, so it might work.
However, if you are on linux: it escapes with single quotes, meaning it won't work.

Not sure if it's a good idea, but maybe we shouldn't escape the variables and leave that up to the one configurating grumphp? That way you can do something like:

grumphp: git_hook_variables: EXEC_GRUMPHP_COMMAND: ['docker-composer --rm --no-deps -u "$(id -u):$(id -g)"', "'php'" ]

Which will result in:

docker-composer --rm --no-deps -u "$(id -u):$(id -g)" 'php'

Meaning you can choose if you want to escape or not from the config file.
However: it means we won't escape by default anymore....

(Assuming it is a developer tool, this won't be a big security issue IMO.)

In this way it's up to the developer if the command will be escaped or not. If someone uses a string it won't be escaped automatically, but if someone uses an array it will be escaped by the process component.
Won't doing automatic escaping would be the other option, but I think it's more convenient this way. It's up to the user to decide.

If it would be normalized to an array how could we decide if it needs escaping or not?

In my case I can use EXEC_GRUMPHP_COMMAND: 'docker-compose run --rm --no-deps -u $(id -u):$(id -g) php'

sounds good!

veewee · 2020-06-29T14:13:17Z

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')),


Can you tell me what the issue is exactly?
Why are you defaulting to the string 'composer' if the file cannot be found?
I try to avoid realpath as much as possible in this package, since it already caused a lot of issues.

I have ...:vendor/bin:... in the $PATH so the executable finder finds that first and tries to run that with a relative path.

@janvernieuwe : Does this fix the issue you had while running paratest as well? It might be related

veewee · 2020-06-29T14:15:57Z

src/Formatter/PhpCsFixerFormatter.php

@@ -36,16 +35,6 @@ public function format(Process $process): string
        return $this->formatJsonResponse($json);
    }

-    public function formatSuggestion(Process $process): string


Agree we can get rid of this method. However, we should still suggest the fix command. (See further in CR)

veewee · 2020-06-29T14:16:38Z

test/E2E/GitHookParametersTest.php

                ],
            ],
        ]);

        $this->installComposer($this->rootDir);

-        $hookPattern = '{[\'"]?'.preg_quote($php, '{').'[\'"]? [\'"]?-d[\'"]? [\'"]?date\.timezone=Europe/Brussels[\'"]?}';
+        $hookPattern = sprintf('{[\'"]%s[\'"] [\'"]-d date\.timezone=Europe/Brussels[\'"]}', $php);


better to keep $php preg_quote-ed

veewee · 2020-06-29T14:21:54Z

src/Task/PhpCsFixer.php

@@ -102,7 +102,7 @@ public function run(ContextInterface $context): TaskResultInterface

        if (!$process->isSuccessful()) {
            $messages = [$this->formatter->format($process)];
-            $fixerCommand = $this->formatter->formatSuggestion($process);
+            $fixerCommand = $process->getCommandLine();


Instead of creating the suggestion in the formatter, we can add arguments to the $arguments variable as we do in the eslint task:
https://github.com/phpro/grumphp/blob/master/src/Task/ESLint.php#L92-L105

This way, we are still suggesting the fix command to the end user in case of a detected fixable item.

Fixed, thanks!

veewee

The code looks good and I like the way we can now configure the command! Thanks!

veewee · 2020-07-09T05:33:56Z

src/Console/Command/Git/InitCommand.php

@@ -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);


sounds good!

1ed force-pushed the command-escaping branch from 65a293a to 7e16383 Compare June 27, 2020 15:07

veewee requested changes Jun 29, 2020

View reviewed changes

veewee added bug enhancement labels Jun 29, 2020

1ed added 7 commits July 4, 2020 23:40

Fix escaping of EXEC_GRUMPHP_COMMAND and add array support

3c384bd

Remove ProcessUtils

5aed933

Fix e2e tests with relative composer path

0b8a4c1

Fix compatibility with composer's built-in older Process version

c74e42f

Restore the correct php-cs-fixer command suggestion

e2f0bf4

Escape variable in regex pattern

b0148c6

Document EXEC_GRUMPHP_COMMAND escaping

2ca14d4

1ed force-pushed the command-escaping branch from 7e16383 to 2ca14d4 Compare July 5, 2020 01:08

Check only for the command string in the output

2237f98

veewee approved these changes Jul 9, 2020

View reviewed changes

veewee added this to the 0.19.2 milestone Jul 9, 2020

veewee merged commit 8457d88 into phpro:master Jul 9, 2020

veewee mentioned this pull request Jul 9, 2020

Use double quotes instead of simple ones for git_hook_variables argument escaping #733

Closed

1ed deleted the command-escaping branch February 1, 2021 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix command escaping #780

Fix command escaping #780

1ed commented Jun 27, 2020 •

edited by veewee

Loading

veewee left a comment

veewee Jun 29, 2020

1ed Jul 5, 2020

veewee Jun 29, 2020

veewee Jun 29, 2020

1ed Jul 5, 2020

veewee Jun 29, 2020

1ed Jul 5, 2020

veewee Jul 9, 2020

veewee Jun 29, 2020

1ed Jul 5, 2020 •

edited

Loading

veewee Jul 9, 2020

veewee Jun 29, 2020

veewee Jun 29, 2020

1ed Jul 5, 2020

veewee Jun 29, 2020

1ed Jul 5, 2020

veewee left a comment •

edited

Loading

veewee Jul 9, 2020

Fix command escaping #780

Fix command escaping #780

Conversation

1ed commented Jun 27, 2020 • edited by veewee Loading

veewee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ed Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veewee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ed commented Jun 27, 2020 •

edited by veewee

Loading

1ed Jul 5, 2020 •

edited

Loading

veewee left a comment •

edited

Loading