diff --git a/CHANGELOG.md b/CHANGELOG.md index a5059eac..7762b3c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ Latest * [#190](https://github.com/cleverage/process-bundle/issues/190) Update quality stack: use Rector `withComposerBased()` sets (removed `SYMFONY_64` / `PHPUNIT_100` sets), declare used Symfony packages and PHPUnit range in composer.json, apply quality tools fixes * [#145](https://github.com/cleverage/process-bundle/issues/145) Add missing documentations: reference pages for every Task & Transformer, ConditionTrait & GenericTransformer, complete guides and cookbooks. Harmonize and fix existing documentation. +## Fixes +* [#192](https://github.com/cleverage/process-bundle/issues/192) Fix CommandRunnerTask: only pass the `options` option to `Process::setOptions()`, support string `commandline` through `Process::fromShellCommandline()`, validate option types. Update documentation, add tests. + * v5.0 ----- diff --git a/docs/reference/tasks/command_runner_task.md b/docs/reference/tasks/command_runner_task.md index 50a45399..b2749b94 100644 --- a/docs/reference/tasks/command_runner_task.md +++ b/docs/reference/tasks/command_runner_task.md @@ -27,11 +27,11 @@ Options | Code | Type | Required | Default | Description | |---------------|--------------------|:--------:|---------------------------|--------------------------------------------------------------------------------| -| `commandline` | `string\|array` | **X** | | Command to run, as an array of arguments (recommended) or a string | +| `commandline` | `string\|array` | **X** | | Command to run, as an array of arguments (recommended) or a string run by the shell (see Notes) | | `cwd` | `string\|null` | | Symfony project directory | Working directory of the command | | `env` | `array\|null` | | `null` | Environment variables of the command (`null` inherits the current environment) | | `timeout` | `int\|float\|null` | | `60` | Timeout in seconds (`null` disables it) | -| `options` | `mixed` | | `null` | Unused by the task itself, see Notes | +| `options` | `array\|null` | | `null` | Passed to `Process::setOptions()`: `blocking_pipes`, `create_process_group`, `create_new_console` | Examples -------- @@ -48,10 +48,22 @@ count_lines: outputs: [next_task] ``` +* Use shell features (pipes, environment variables) + +```yaml +# Task configuration level +count_errors: + service: '@CleverAge\ProcessBundle\Task\Process\CommandRunnerTask' + options: + commandline: 'grep "$PATTERN" | wc -l' + env: + PATTERN: 'error' + outputs: [next_task] +``` + Notes ----- -The task calls `Process::setOptions()` with **all** its resolved options (`commandline`, `cwd`, `env`, `timeout` and -`options`). Recent versions of `symfony/process` only accept `blocking_pipes`, `create_process_group` and -`create_new_console` there and throw a `LogicException` for any other key, which makes the task fail before the -command is started. +An array `commandline` is run with `new Process()`: each argument is escaped and no shell is involved. A string +`commandline` is run with `Process::fromShellCommandline()`: it is interpreted by the shell, so never build it from +untrusted input. diff --git a/src/Task/Process/CommandRunnerTask.php b/src/Task/Process/CommandRunnerTask.php index a988ff3e..7e0ade0a 100644 --- a/src/Task/Process/CommandRunnerTask.php +++ b/src/Task/Process/CommandRunnerTask.php @@ -32,14 +32,18 @@ public function __construct( public function execute(ProcessState $state): void { $options = $this->getOptions($state); - $process = new Process( - $options['commandline'], + $arguments = [ $options['cwd'], $options['env'], $state->getInput(), $options['timeout'], - ); - $process->setOptions($options); + ]; + $process = \is_array($options['commandline']) + ? new Process($options['commandline'], ...$arguments) + : Process::fromShellCommandline($options['commandline'], ...$arguments); + if (null !== $options['options']) { + $process->setOptions($options['options']); + } $process->mustRun(); $state->setOutput($process->getOutput()); } @@ -56,5 +60,9 @@ protected function configureOptions(OptionsResolver $resolver): void 'options' => null, ] ); + $resolver->setAllowedTypes('cwd', ['null', 'string']); + $resolver->setAllowedTypes('env', ['null', 'array']); + $resolver->setAllowedTypes('timeout', ['null', 'int', 'float']); + $resolver->setAllowedTypes('options', ['null', 'array']); } } diff --git a/tests/Task/Process/CommandRunnerTaskTest.php b/tests/Task/Process/CommandRunnerTaskTest.php new file mode 100644 index 00000000..30d047cf --- /dev/null +++ b/tests/Task/Process/CommandRunnerTaskTest.php @@ -0,0 +1,127 @@ +createState(['commandline' => ['echo', 'hello']]); + + $this->createTask()->execute($state); + + $this->assertSame("hello\n", $state->getOutput()); + } + + public function testExecuteWithStringCommandline(): void + { + $state = $this->createState(['commandline' => 'echo "hello world" | tr a-z A-Z']); + + $this->createTask()->execute($state); + + $this->assertSame("HELLO WORLD\n", $state->getOutput()); + } + + public function testInputIsPassedToCommand(): void + { + $state = $this->createState(['commandline' => ['cat']]); + $state->setInput('from input'); + + $this->createTask()->execute($state); + + $this->assertSame('from input', $state->getOutput()); + } + + public function testCwdAndEnvOptions(): void + { + $state = $this->createState([ + 'commandline' => 'echo "$FOO" && pwd', + 'cwd' => sys_get_temp_dir(), + 'env' => ['FOO' => 'bar'], + ]); + + $this->createTask()->execute($state); + + $this->assertSame('bar'.\PHP_EOL.realpath(sys_get_temp_dir()).\PHP_EOL, $state->getOutput()); + } + + public function testProcessOptionsArePassedToProcess(): void + { + $state = $this->createState([ + 'commandline' => ['echo', 'hello'], + 'options' => ['create_new_console' => false], + ]); + + $this->createTask()->execute($state); + + $this->assertSame("hello\n", $state->getOutput()); + } + + public function testInvalidProcessOptionsType(): void + { + $state = $this->createState([ + 'commandline' => ['echo', 'hello'], + 'options' => 'invalid', + ]); + + $this->expectException(InvalidOptionsException::class); + $this->createTask()->execute($state); + } + + public function testFailingCommandThrows(): void + { + $state = $this->createState(['commandline' => ['false']]); + + $this->expectException(ProcessFailedException::class); + $this->createTask()->execute($state); + } + + private function createTask(): CommandRunnerTask + { + $kernel = $this->createStub(KernelInterface::class); + $kernel->method('getProjectDir')->willReturn(sys_get_temp_dir()); + + return new CommandRunnerTask($kernel); + } + + private function createState(array $options): ProcessState + { + $processConfiguration = new ProcessConfiguration('test', []); + $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration)); + $state->setContextualOptionResolver(new ContextualOptionResolver()); + $state->setContext([]); + $state->setTaskConfiguration(new TaskConfiguration('command', CommandRunnerTask::class, $options)); + + return $state; + } +}