Skip to content

Prevent shell injection and RCE risk in CI pipelines by passing commands as argument arrays to Symfony Process instead of shell strings.#23

Merged
gustavofreze merged 2 commits intomainfrom
feature/develop
Apr 21, 2026
Merged

Prevent shell injection and RCE risk in CI pipelines by passing commands as argument arrays to Symfony Process instead of shell strings.#23
gustavofreze merged 2 commits intomainfrom
feature/develop

Conversation

@gustavofreze
Copy link
Copy Markdown
Member

No description provided.

…commands as argument arrays to Symfony Process instead of shell strings.
Copilot AI review requested due to automatic review settings April 21, 2026 21:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens Docker command execution against shell injection/RCE by switching from shell command strings to Symfony Process argument arrays, and updates supporting domain logic and unit tests accordingly.

Changes:

  • Replaced string-based Docker command rendering with Command::toArguments() and executed via new Process([...]).
  • Updated Docker command builders and error rendering to work with argument arrays and safe command display.
  • Expanded/adjusted unit tests for edge cases (timeouts, port parsing, reaper behavior) and refreshed project tooling/docs config.

Reviewed changes

Copilot reviewed 54 out of 56 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/bootstrap.php Removed custom PHPUnit bootstrap (previously enabled bypass-finals).
phpunit.xml Switched PHPUnit bootstrap to vendor/autoload.php.
tests/Unit/Waits/ContainerWaitForTimeTest.php Adjusted test double usage for wait-after timing test.
tests/Unit/Waits/ContainerWaitForDependencyTest.php Added edge-case coverage for timeout/attempt behavior.
tests/Unit/MySQLDockerContainerTest.php Updated command assertions for new argument-based rendering; updated shutdown hook test double usage.
tests/Unit/Mocks/ShutdownHookMock.php Added a concrete shutdown hook test double to track registrations.
tests/Unit/Mocks/CommandWithTimeoutMock.php Updated mock command to return argument arrays.
tests/Unit/Mocks/CommandMock.php Updated mock command to return argument arrays.
tests/Unit/Mocks/ClientMock.php Updated mock client to record commands based on argument arrays.
tests/Unit/Internal/Containers/Overrides/file_exists_inside_docker.php Fixed brace formatting in override helper.
tests/Unit/Internal/Containers/ContainerReaperTest.php Added tests for reaper startup when running inside Docker and handling whitespace list output.
tests/Unit/Internal/Containers/Address/PortsTest.php Added tests to ensure port arrays drop falsy values and reindex.
tests/Unit/Internal/Commands/DockerCommandsTest.php Updated tests to assert toArguments() output for Docker commands.
tests/Unit/GenericDockerContainerTest.php Added coverage for whitespace list output, command rendering in exceptions, and port-binding edge cases; updated shutdown hook tests.
tests/Unit/FlywayDockerContainerTest.php Updated env var assertions to match new argument-based command rendering.
src/Waits/ContainerWaitForDependency.php Reworked retry loop to be attempt/budget-based.
src/Waits/Conditions/MySQL/MySQLReady.php Changed readiness probe to use env MYSQL_PWD=... mysqladmin ping form.
src/Internal/Exceptions/DockerCommandExecutionFailed.php Rendered failed commands using shell-escaped argument display.
src/Internal/Containers/ShutdownHook.php Made shutdown hook inheritable to support concrete test doubles.
src/Internal/Containers/Models/Name.php Avoided empty() to prevent treating '0' as empty.
src/Internal/Containers/HostEnvironment.php Minor formatting change around hostname casting.
src/Internal/Containers/Environment/EnvironmentVariables.php Stored env vars as an array to avoid repeated Collection->toArray() calls.
src/Internal/Containers/Definitions/VolumeMapping.php Emitted volume mapping as argument arrays.
src/Internal/Containers/Definitions/PortMapping.php Emitted port mapping as argument arrays.
src/Internal/Containers/Definitions/EnvironmentVariable.php Emitted env vars as argument arrays (no shell quoting).
src/Internal/Containers/Definitions/CopyInstruction.php Emitted docker cp args as arrays.
src/Internal/Containers/ContainerLookup.php Hardened JSON decode handling for inspect payloads.
src/Internal/Containers/ContainerInspection.php Adjusted host port extraction to rely on downstream filtering/reindexing.
src/Internal/Containers/Address/Ports.php Switched Ports internals to arrays; filtered/reindexed port lists.
src/Internal/Commands/DockerStop.php Implemented toArguments() including --time.
src/Internal/Commands/DockerRun.php Rebuilt docker run construction as argument arrays.
src/Internal/Commands/DockerRemove.php Implemented toArguments() for docker rm.
src/Internal/Commands/DockerReaper.php Implemented toArguments() and moved shell script construction into helper.
src/Internal/Commands/DockerPull.php Implemented toArguments() for docker pull.
src/Internal/Commands/DockerNetworkPrune.php Implemented toArguments() for network prune.
src/Internal/Commands/DockerNetworkCreate.php Implemented toArguments() for network create.
src/Internal/Commands/DockerNetworkConnect.php Implemented toArguments() for network connect.
src/Internal/Commands/DockerList.php Implemented toArguments() for docker ps filtering.
src/Internal/Commands/DockerInspect.php Implemented toArguments() for docker inspect.
src/Internal/Commands/DockerExecute.php Implemented toArguments() for docker exec.
src/Internal/Commands/DockerCopy.php Implemented toArguments() for docker cp.
src/Internal/Commands/Command.php Updated command contract docs to be argument-array based.
src/Internal/Client/DockerClient.php Switched process execution to new Process($command->toArguments()).
composer.json Updated dependencies/dev tooling and normalized composer scripts.
README.md Updated overview wording to reflect broader feature set.
Makefile Formatting cleanup; added normalize/outdated targets; updated help filters.
.gitattributes Added LF normalization and export-ignore settings.
.editorconfig Added repo-wide formatting settings.
.claude/rules/php-library-testing.md Refreshed testing rules and structure text.
.claude/rules/php-library-modeling.md Added/updated modeling rules document.
.claude/rules/php-library-documentation.md Updated documentation rules (FAQ/examples guidance).
.claude/rules/php-library-code-style.md Added/updated code-style rules document.
.claude/rules/github-workflows.md Added workflow conventions document.
.claude/CLAUDE.md Updated project instructions and post-change validation guidance.
Comments suppressed due to low confidence (2)

src/Internal/Commands/DockerReaper.php:53

  • buildScript() interpolates $testRunnerHostname into a sh -c script without shell-quoting. If the hostname contains shell metacharacters, it becomes command injection inside the reaper container (which has the Docker socket mounted). Please shell-escape/quote interpolated values (e.g., escapeshellarg) or avoid sh -c by passing structured arguments.
                'while docker inspect %s >/dev/null 2>&1; do sleep 2; done;',
                'docker rm -fv %s 2>/dev/null;',
                'docker network prune -f --filter label=%s 2>/dev/null'
            ]),
            $this->testRunnerHostname,

.claude/rules/php-library-testing.md:74

  • Rule 12 forbids named arguments on PHPUnit assertions, but the earlier example in this same document uses named arguments in assertEquals(expected: ..., actual: ...). Either update the example to positional arguments or relax/remove rule 12 so the guidance is internally consistent.

Comment thread src/Internal/Containers/ShutdownHook.php
Comment thread .gitattributes Outdated
Comment thread src/Waits/ContainerWaitForDependency.php
Comment thread .gitattributes Outdated
…commands as argument arrays to Symfony Process instead of shell strings.
@gustavofreze gustavofreze merged commit 019c1e9 into main Apr 21, 2026
5 checks passed
@gustavofreze gustavofreze deleted the feature/develop branch April 21, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants