Rebuild process execution safely#19
Merged
Merged
Conversation
WendellAdriel
force-pushed
the
feat/rebuild-process-exec
branch
from
12cfdfb to
46dbddd
Compare
WendellAdriel
force-pushed
the
feat/rebuild-process-exec
branch
from
46dbddd to
d05fa86
Compare
| { | ||
| $target = array_shift($tokens); | ||
|
|
||
| return new self(is_string($target) ? $target : '', $tokens); |
Collaborator
There was a problem hiding this comment.
This will ultimately throw an exception if you pass the blank string... is that intentional?
Member
Author
There was a problem hiding this comment.
Yes, that's intentional!
Collaborator
There was a problem hiding this comment.
Is that just to keep the exception centralized? Doing it this way as opposed to throwing it here?
Member
Author
There was a problem hiding this comment.
Yeah, just to centralize!
Collaborator
|
@WendellAdriel just a couple of comments here, not much blocking but give it a look before I merge and let me know what you think. |
Help and version are provided by Symfony Console out of the box, so the custom commands are gone. The check, format, and test commands are removed too, along with the now-dead Console::exec() and ConsoleException they relied on.
The find-autoloader, load-laravel-bootstrap, and alias-classes options are now VALUE_NEGATABLE with real boolean defaults, so the command reads true booleans and the default lives in one place. The file fallback coerces its parsed value before handing it to Symfony, keeping `--find-autoloader=false` working.
Introduce Cpx\Process\ProcessRunner as the single proc_open() boundary, executing argv arrays with inherited stdio or captured output and returning child exit codes. Add an architecture test that forbids proc_open() outside the runner and bans shell-exec helpers.
Replace the shell-string Cpx\Composer wrapper with Cpx\Composer\ComposerRunner, which builds composer invocations as argv tokens (including --working-dir) and surfaces stderr on failure.
Drop the legacy Cpx\Console string parser/executor in favor of an immutable Cpx\Input\PackageInvocation built from raw argv tokens, preserving --, repeated options, and shell metacharacters as literal tokens.
Replace the generic Utils class with Cpx\Support\Arr and Cpx\Support\Filesystem for the array mapping and recursive directory deletion that are still needed.
Move Metadata and PackageMetadata under Cpx\Cache and the PHP execution helpers under Cpx\Runtime. Replace Metadata's string mode flag with intent-named recordRun() and recordUpdate() methods.
Move Package, PackageAlias, PackageAliases, and PackageCommandRunner into Cpx\Packages and execute resolved binaries as argv arrays that return child exit codes. Reject path-traversal version constraints during parsing.
Update the Symfony commands, application wiring, and sandbox bootstrap to build PackageInvocations, call ProcessRunner and ComposerRunner with argv arrays, and propagate child process exit codes.
- Run subprocesses over reopened stdio so exit codes survive any test harness - Stream Composer output live instead of buffering and discarding it - Thread console output through Package and drop the printColor buffering - Validate package targets against Composer's official name grammar
WendellAdriel
force-pushed
the
feat/rebuild-process-exec
branch
from
05cbb09 to
77d67d4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The original cpx built its commands as shell strings, which let crafted package names slip into a shell, swallowed child exit codes so a failing tool could still make cpx exit zero, and mangled forwarded arguments. None of that is safe to wrap other binaries with or to run in CI.
What changed
This rebuilds the execution path around argv arrays instead of shell strings. Package binaries and Composer both run through a small ProcessRunner over reopened stdio, so child exit codes propagate all the way back to cpx and arguments reach the target untouched (including everything after
--). A PackageInvocation value object carries the forwarded tokens verbatim, package targets are validated against Composer's official name grammar before they ever touch the filesystem, and cache cleanup uses filesystem APIs rather thanrm -rf. Composer output now streams live instead of being buffered and thrown away.The supporting classes are also regrouped into focused Cache, Composer, Input, Packages, Process, Runtime, and Support namespaces.
Why not symfony/process
cpx is a package runner, so the child process needs the real terminal: interactive prompts, REPLs like tinker, colored output, and a passthrough TTY. symfony/process is built to capture or pipe output, not to hand the child the inherited descriptors, so it gets in the way of true passthrough. Raw proc_open with the inherited stdio is the simplest thing that preserves it.
Note
The generated install scaffold still sets
allow-plugins: true, which the cache/install hardening work should revisit.