Skip to content

Commit f0e1ebb

Browse files
jrfnljrfnl
committed
Merge branch '3.x' into 4.x
2 parents b90bca2 + ddbf849 commit f0e1ebb

5 files changed

Lines changed: 214 additions & 3 deletions

File tree

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ vendor/
1212
/phpunitlte9.xml
1313
/phpunit-lte9.xml
1414
/tests/Standards/sniffStnd.xml
15+
/tests/Core/Reports/GitBlame/$*
16+
/tests/Core/Reports/GitBlame/-*
17+
/tests/Core/Reports/GitBlame/newfile.txt
1518
/tests/Core/Util/ExitCode/Fixtures/ExitCodeTest/*.fixed
1619
/tests/Core/Util/ExitCode/Fixtures/ExitCodeTest/phpcs.cache
1720
/tests/EndToEndBash/Fixtures/*.fixed

‎src/Reports/Gitblame.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ protected function getBlameContent(string $filename)
7171
$cwd = getcwd();
7272

7373
chdir(dirname($filename));
74-
$command = 'git blame --date=short "' . basename($filename) . '" 2>&1';
74+
$command = 'git blame --date=short -- ' . escapeshellarg(basename($filename)) . ' 2>&1';
7575
$handle = popen($command, 'r');
7676
if ($handle === false) {
7777
$error = 'ERROR: Could not execute "' . $command . '"' . PHP_EOL . PHP_EOL;

‎src/Reports/Hgblame.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ protected function getBlameContent(string $filename)
9090
throw new DeepExitException($error, ExitCode::PROCESS_ERROR);
9191
}
9292

93-
$command = 'hg blame -u -d -v "' . $filename . '" 2>&1';
93+
$command = 'hg blame -u -d -v ' . escapeshellarg($filename) . ' 2>&1';
9494
$handle = popen($command, 'r');
9595
if ($handle === false) {
9696
$error = 'ERROR: Could not execute "' . $command . '"' . PHP_EOL . PHP_EOL;

‎src/Reports/Svnblame.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ protected function getAuthor(string $line)
5454
*/
5555
protected function getBlameContent(string $filename)
5656
{
57-
$command = 'svn blame "' . $filename . '" 2>&1';
57+
$command = 'svn blame ' . escapeshellarg($filename) . ' 2>&1';
5858
$handle = popen($command, 'r');
5959
if ($handle === false) {
6060
$error = 'ERROR: Could not execute "' . $command . '"' . PHP_EOL . PHP_EOL;

‎tests/Core/Reports/GitBlame/GitblameEscapesFilenameTest.php‎

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
<?php
2+
/**
3+
* Tests a security fix for the Gitblame report generation.
4+
*
5+
* @copyright 2026 PHPCSStandards and contributors
6+
* @license https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/HEAD/licence.txt BSD Licence
7+
*/
8+
9+
namespace PHP_CodeSniffer\Tests\Core\Reports\Gitblame;
10+
11+
use PHP_CodeSniffer\Ruleset;
12+
use PHP_CodeSniffer\Runner;
13+
use PHP_CodeSniffer\Tests\ConfigDouble;
14+
use PHPUnit\Framework\TestCase;
15+
use ReflectionMethod;
16+
17+
/**
18+
* Tests a security fix for the Gitblame report generation.
19+
*
20+
* @coversNothing
21+
*
22+
* @group Windows
23+
*/
24+
final class GitblameEscapesFilenameTest extends TestCase
25+
{
26+
27+
/**
28+
* Name of the file which, if the filename of the temporary file is escaped correctly, should *not* be created.
29+
*
30+
* @var string
31+
*/
32+
private const DO_NOT_CREATE_FILE = 'newfile.txt';
33+
34+
/**
35+
* List of files to clean up after running the tests in this file.
36+
*
37+
* @var string[]
38+
*/
39+
private static $temporaryFiles = [];
40+
41+
42+
/**
43+
* Skip these tests when in CBF mode.
44+
*
45+
* @return void
46+
*/
47+
protected function setUp(): void
48+
{
49+
if (PHP_CODESNIFFER_CBF === true) {
50+
$this->markTestSkipped('This test needs CS mode to run');
51+
}
52+
}
53+
54+
55+
/**
56+
* Clean up temporary file(s).
57+
*
58+
* @return void
59+
*/
60+
public static function tearDownAfterClass(): void
61+
{
62+
foreach (self::$temporaryFiles as $file) {
63+
@unlink($file);
64+
}
65+
66+
@unlink(__DIR__ . '/' . self::DO_NOT_CREATE_FILE);
67+
}
68+
69+
70+
/**
71+
* Test that arbitrary shell commands injected in a filename do not get executed.
72+
*
73+
* @return void
74+
*/
75+
public function testFilenameEscaping()
76+
{
77+
$untrustedFileName = __DIR__ . '/$(touch ' . self::DO_NOT_CREATE_FILE . ').php';
78+
$this->createTemporaryFile($untrustedFileName);
79+
80+
$errorMessage = sprintf('File %s should not exist at the start of the test', self::DO_NOT_CREATE_FILE);
81+
$this->verifyFileDoesNotExist(__DIR__ . '/' . self::DO_NOT_CREATE_FILE, $errorMessage);
82+
83+
// While we do not care about the report output for this particular test,
84+
// we do need to be sure that the correct report ran.
85+
// This regex checks that the output complies with the expected format for a 'Gitblame' reports.
86+
$this->expectOutputRegex('`\bUnknown\s+\([0-9\.]+\)\s+\(100\)\s+3\b`');
87+
88+
$args = [
89+
'--standard=PSR12',
90+
'--basepath=' . __DIR__,
91+
'--report=Gitblame',
92+
'--report-width=80',
93+
$untrustedFileName,
94+
];
95+
$this->generateReport($args);
96+
97+
$errorMessage = sprintf('File %s should not exist at the end of the test', self::DO_NOT_CREATE_FILE);
98+
$this->verifyFileDoesNotExist(__DIR__ . '/' . self::DO_NOT_CREATE_FILE, $errorMessage);
99+
}
100+
101+
102+
/**
103+
* Test that file names are not interpreted as parameters/CLI flags to the external command 'git'.
104+
*
105+
* @param string $fileName Name for a temporary file to create to run the test with.
106+
*
107+
* @dataProvider dataFilenameParameterInjection
108+
*
109+
* @return void
110+
*/
111+
public function testFilenameParameterInjection($fileName)
112+
{
113+
$this->createTemporaryFile($fileName);
114+
115+
// This line of the report is different when 'git' interprets the
116+
// filename as a parameter.
117+
$this->expectOutputRegex('`\bUnknown\s+\(100\)\s+\(100\)\s+3\b`');
118+
119+
$args = [
120+
'--standard=PSR12',
121+
'--basepath=' . __DIR__,
122+
'--report=Gitblame',
123+
'--report-width=80',
124+
$fileName,
125+
];
126+
$this->generateReport($args);
127+
}
128+
129+
130+
/**
131+
* Data provider.
132+
*
133+
* @return array<string, array<string>>
134+
*/
135+
public static function dataFilenameParameterInjection()
136+
{
137+
return [
138+
'long-option' => [__DIR__ . '/--invalid-parameter-injection.php'],
139+
140+
// At time of writing, there is no valid '-V' option to git(1).
141+
'short-option' => [__DIR__ . '/-V.php'],
142+
];
143+
}
144+
145+
146+
/**
147+
* Create a temporary file with the supplied name.
148+
*
149+
* The created file MUST contain at least one error when run against the PSR12 ruleset,
150+
* as otherwise the report code will not be reached.
151+
*
152+
* @param string $fileName Name of the file to create.
153+
*
154+
* @return void
155+
*/
156+
private function createTemporaryFile($fileName)
157+
{
158+
file_put_contents($fileName, "<?php\n\$x=1 ;\n");
159+
$this->assertFileExists($fileName, 'Failed to write temporary test file');
160+
161+
// Remember that the file was created for clean up later.
162+
self::$temporaryFiles[] = $fileName;
163+
}
164+
165+
166+
/**
167+
* Helper function to run PHPCS and create the report with the provided CLI arguments.
168+
*
169+
* @param array<string> $args CLI arguments to pass to the PHPCS run.
170+
*
171+
* @return void
172+
*/
173+
private function generateReport($args)
174+
{
175+
$runner = new Runner();
176+
$runner->config = new ConfigDouble($args);
177+
$runner->ruleset = new Ruleset($runner->config);
178+
179+
$reflMethod = new ReflectionMethod($runner, 'run');
180+
(PHP_VERSION_ID < 80100) && $reflMethod->setAccessible(true);
181+
$reflMethod->invoke($runner);
182+
183+
$result = ($runner->reporter->totalErrors + $runner->reporter->totalWarnings);
184+
$this->assertGreaterThan(0, $result, 'File scanned did not contain any errors. Report code would not be triggered');
185+
186+
$runner->reporter->printReports();
187+
}
188+
189+
190+
/**
191+
* Helper function for PHPUnit cross-version compatible checking whether a file does *not* exist.
192+
*
193+
* @param string $fileName Name of the file to check.
194+
* @param string $errorMessage Message to display if the file unexpectedly would be found.
195+
*
196+
* @return void
197+
*/
198+
public function verifyFileDoesNotExist($fileName, $errorMessage): void
199+
{
200+
if (method_exists($this, 'assertFileDoesNotExist') === true) {
201+
// PHPUnit 9.1.0+.
202+
$this->assertFileDoesNotExist($fileName, $errorMessage);
203+
} else {
204+
// PHPUnit < 9.1.0.
205+
$this->assertFileNotExists($fileName, $errorMessage);
206+
}
207+
}
208+
}

0 commit comments

Comments
 (0)