getPhpcsOutputForGitBatch() and getPhpcsOutputForSvnBatch() build each temp path by concatenating the file name onto the batch temp directory:
// PhpcsChanged/ShellRunner.php
$tempPath = $tempDir . '/new/' . ltrim($fileName, '/');
ltrim() strips leading slashes but does nothing about .., and the file names arrive exactly as typed on the command line — bin/phpcs-changed puts the expanded argument list straight into $options['files'], prepareGitScanPlan()/prepareSvnScanPlan() pass those through to the scan plan, and nothing calls realpath() along the way.
So a relative argument containing .. places the temp copy outside the batch directory:
$ php -r 'echo "/tmp/phpcs-changed-abc" . "/new/" . ltrim("../../evil.php", "/");'
/tmp/phpcs-changed-abc/new/../../evil.php # resolves to /tmp/evil.php
Running e.g. cd sub && phpcs-changed --git-unstaged ../../other.php therefore:
- writes the file's contents outside the tree
cleanupTempDir() removes, leaving litter in the temp directory after every run, and
- can silently overwrite an existing file at that path.
This is not a privilege boundary — the user is scanning their own files as themselves — but the temp copy escaping its directory is a real correctness and hygiene bug. It was deliberately left out of #120 / #127, which only hardens how the batch directory itself is created.
Suggested fix
Derive the temp path from a normalized form of the file name rather than the raw argument: strip .. segments, or key the temp files by a hash/index and keep the temp-path-to-original-path mapping that $tempToOriginal already maintains. Either approach also removes the need for ltrim().
getPhpcsOutputForGitBatch()andgetPhpcsOutputForSvnBatch()build each temp path by concatenating the file name onto the batch temp directory:ltrim()strips leading slashes but does nothing about.., and the file names arrive exactly as typed on the command line —bin/phpcs-changedputs the expanded argument list straight into$options['files'],prepareGitScanPlan()/prepareSvnScanPlan()pass those through to the scan plan, and nothing callsrealpath()along the way.So a relative argument containing
..places the temp copy outside the batch directory:Running e.g.
cd sub && phpcs-changed --git-unstaged ../../other.phptherefore:cleanupTempDir()removes, leaving litter in the temp directory after every run, andThis is not a privilege boundary — the user is scanning their own files as themselves — but the temp copy escaping its directory is a real correctness and hygiene bug. It was deliberately left out of #120 / #127, which only hardens how the batch directory itself is created.
Suggested fix
Derive the temp path from a normalized form of the file name rather than the raw argument: strip
..segments, or key the temp files by a hash/index and keep the temp-path-to-original-path mapping that$tempToOriginalalready maintains. Either approach also removes the need forltrim().