Skip to content

Commit d9a9ea4

Browse files
committed
refactor: combine host:port into a single escapeshellarg() and add ServeTest
Address review feedback from @michalsn on PR #10156: #10156 (review) Previously $host was escaped on its own, which made the development server banner display: http://'localhost':8080 Build a single shell-escaped $address = escapeshellarg($host . ':' . $port) for the passthru() call instead, and keep $host in its raw form for the display line. The shell command is still safe against the original `spark serve --host='$(id)'` style injection, while the user-facing output is no longer mangled by stray quotes. Also add tests/system/Commands/Server/ServeTest.php with three cases: - testServeRunsWithDefaultHostAndPort sanity check that the default host/port path is wrapped in single quotes inside the constructed command. - testServeEscapesShellMetacharactersInHost passes --host='$(id)' and asserts the literal substitution token stays inside escapeshellarg's single-quoted wrapper. - testServeEscapesEmbeddedSingleQuoteInHost passes --host="evil'host" and asserts the embedded single quote is escaped via the standard '\\'' sequence rather than breaking out of the argument. The tests intercept passthru() through a function declaration in the CodeIgniter\\Commands\\Server namespace so the real PHP development server is never spawned during test runs.
1 parent 142090a commit d9a9ea4

2 files changed

Lines changed: 105 additions & 2 deletions

File tree

system/Commands/Server/Serve.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,14 @@ public function run(array $params)
9292
{
9393
// Collect any user-supplied options and apply them.
9494
$php = escapeshellarg(CLI::getOption('php') ?? PHP_BINARY);
95-
$host = escapeshellarg(CLI::getOption('host') ?? 'localhost');
95+
$host = CLI::getOption('host') ?? 'localhost';
9696
$port = (int) (CLI::getOption('port') ?? 8080) + $this->portOffset;
9797

98+
// Build a single shell-escaped host:port argument so the resulting
99+
// command is safe regardless of what the user passed via --host,
100+
// while $host remains in its raw form for the display below.
101+
$address = escapeshellarg($host . ':' . $port);
102+
98103
// Get the party started.
99104
CLI::write('CodeIgniter development server started on http://' . $host . ':' . $port, 'green');
100105
CLI::write('Press Control-C to stop.');
@@ -108,7 +113,7 @@ public function run(array $params)
108113
// Call PHP's built-in webserver, making sure to set our
109114
// base path to the public folder, and to use the rewrite file
110115
// to ensure our environment is set and it simulates basic mod_rewrite.
111-
passthru($php . ' -S ' . $host . ':' . $port . ' -t ' . $docroot . ' ' . $rewrite, $status);
116+
passthru($php . ' -S ' . $address . ' -t ' . $docroot . ' ' . $rewrite, $status);
112117

113118
if ($status !== EXIT_SUCCESS && $this->portOffset < $this->tries) {
114119
$this->portOffset++;
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* This file is part of CodeIgniter 4 framework.
7+
*
8+
* (c) CodeIgniter Foundation <admin@codeigniter.com>
9+
*
10+
* For the full copyright and license information, please view
11+
* the LICENSE file that was distributed with this source code.
12+
*/
13+
14+
namespace CodeIgniter\Commands\Server;
15+
16+
use CodeIgniter\Test\CIUnitTestCase;
17+
use CodeIgniter\Test\StreamFilterTrait;
18+
use PHPUnit\Framework\Attributes\Group;
19+
20+
/**
21+
* Override the global passthru() within this namespace so the unit test
22+
* can capture the shell command that Serve::run() would otherwise execute.
23+
*
24+
* @internal
25+
*/
26+
function passthru(string $command, ?int &$result_code = null): void
27+
{
28+
ServeTest::$capturedCommand = $command;
29+
$result_code = 0;
30+
}
31+
32+
/**
33+
* @internal
34+
*/
35+
#[Group('Others')]
36+
final class ServeTest extends CIUnitTestCase
37+
{
38+
use StreamFilterTrait;
39+
40+
/**
41+
* Captured shell command from the namespaced passthru() stub above.
42+
*/
43+
public static string $capturedCommand = '';
44+
45+
protected function setUp(): void
46+
{
47+
parent::setUp();
48+
49+
self::$capturedCommand = '';
50+
$_SERVER['argv'] = ['spark', 'serve'];
51+
}
52+
53+
protected function tearDown(): void
54+
{
55+
// Restore default argv so other tests are not affected.
56+
$_SERVER['argv'] = ['spark'];
57+
58+
parent::tearDown();
59+
}
60+
61+
public function testServeRunsWithDefaultHostAndPort(): void
62+
{
63+
command('serve');
64+
65+
$this->assertStringContainsString(' -S ', self::$capturedCommand);
66+
$this->assertStringContainsString("'localhost:8080'", self::$capturedCommand);
67+
}
68+
69+
public function testServeEscapesShellMetacharactersInHost(): void
70+
{
71+
$_SERVER['argv'] = ['spark', 'serve', '--host', '$(id)'];
72+
73+
command('serve --host="$(id)"');
74+
75+
// The literal '$(id)' must remain inside single quotes so /bin/sh -c
76+
// never expands it into a sub-shell. escapeshellarg() wraps the
77+
// entire host:port pair in single quotes and escapes any embedded
78+
// single quote.
79+
$this->assertStringNotContainsString(' $(id) ', self::$capturedCommand);
80+
$this->assertMatchesRegularExpression(
81+
"/'[^']*\\\$\\(id\\)[^']*:[0-9]+'/",
82+
self::$capturedCommand,
83+
'host:port must be wrapped in single quotes by escapeshellarg()',
84+
);
85+
}
86+
87+
public function testServeEscapesEmbeddedSingleQuoteInHost(): void
88+
{
89+
$_SERVER['argv'] = ['spark', 'serve', '--host', "evil'host"];
90+
91+
command("serve --host=\"evil'host\"");
92+
93+
// escapeshellarg() turns a single quote into '\'' inside the wrapper,
94+
// so the dangerous quote can never break out of the argument.
95+
$this->assertStringNotContainsString(" evil'host:", self::$capturedCommand);
96+
$this->assertStringContainsString("'\\''", self::$capturedCommand);
97+
}
98+
}

0 commit comments

Comments
 (0)