From 142090aa4c812c72a5e90e51e0c14a9f3b11829f Mon Sep 17 00:00:00 2001 From: Feng Ning Date: Mon, 4 May 2026 19:42:47 +0800 Subject: [PATCH 1/2] security: escape $host in spark serve to prevent CLI command injection The `spark serve` command in system/Commands/Server/Serve.php passes $host (--host CLI option) directly into passthru() without escaping, while sibling arguments ($php, $docroot, $rewrite) on the same line are correctly escaped via escapeshellarg(). Same root pattern as recently-fixed CVE-2025-54418 (ImageMagickHandler _resize/_text command injection): user-controlled string concatenation into a shell command. Although impact is limited here (requires local CLI control over argv to influence --host), the inconsistency makes defense-in-depth and consistency with the surrounding code beneficial. PoC (illustrative, requires local CLI access): php spark serve --host='$(id > /tmp/pwn)' The $(...) substitution executes when passthru() invokes /bin/sh -c. Fix wraps $host with escapeshellarg(), identical to how $php / $docroot / $rewrite are handled on the same passthru() line. Backwards compatibility: escapeshellarg('localhost') returns 'localhost' (single-quoted), which remains a valid 'php -S host:port' argument. Existing functionality unaffected. --- system/Commands/Server/Serve.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Commands/Server/Serve.php b/system/Commands/Server/Serve.php index 594e4e587e27..c10056f0680c 100644 --- a/system/Commands/Server/Serve.php +++ b/system/Commands/Server/Serve.php @@ -92,7 +92,7 @@ public function run(array $params) { // Collect any user-supplied options and apply them. $php = escapeshellarg(CLI::getOption('php') ?? PHP_BINARY); - $host = CLI::getOption('host') ?? 'localhost'; + $host = escapeshellarg(CLI::getOption('host') ?? 'localhost'); $port = (int) (CLI::getOption('port') ?? 8080) + $this->portOffset; // Get the party started. From d9a9ea48d7e55d0ba1a810a03263b2e55590f491 Mon Sep 17 00:00:00 2001 From: Feng Ning Date: Tue, 5 May 2026 00:00:29 +0800 Subject: [PATCH 2/2] refactor: combine host:port into a single escapeshellarg() and add ServeTest Address review feedback from @michalsn on PR #10156: https://github.com/codeigniter4/CodeIgniter4/pull/10156#pullrequestreview-4220164953 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. --- system/Commands/Server/Serve.php | 9 +- tests/system/Commands/Server/ServeTest.php | 98 ++++++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 tests/system/Commands/Server/ServeTest.php diff --git a/system/Commands/Server/Serve.php b/system/Commands/Server/Serve.php index c10056f0680c..cd9f8ef01cff 100644 --- a/system/Commands/Server/Serve.php +++ b/system/Commands/Server/Serve.php @@ -92,9 +92,14 @@ public function run(array $params) { // Collect any user-supplied options and apply them. $php = escapeshellarg(CLI::getOption('php') ?? PHP_BINARY); - $host = escapeshellarg(CLI::getOption('host') ?? 'localhost'); + $host = CLI::getOption('host') ?? 'localhost'; $port = (int) (CLI::getOption('port') ?? 8080) + $this->portOffset; + // Build a single shell-escaped host:port argument so the resulting + // command is safe regardless of what the user passed via --host, + // while $host remains in its raw form for the display below. + $address = escapeshellarg($host . ':' . $port); + // Get the party started. CLI::write('CodeIgniter development server started on http://' . $host . ':' . $port, 'green'); CLI::write('Press Control-C to stop.'); @@ -108,7 +113,7 @@ public function run(array $params) // Call PHP's built-in webserver, making sure to set our // base path to the public folder, and to use the rewrite file // to ensure our environment is set and it simulates basic mod_rewrite. - passthru($php . ' -S ' . $host . ':' . $port . ' -t ' . $docroot . ' ' . $rewrite, $status); + passthru($php . ' -S ' . $address . ' -t ' . $docroot . ' ' . $rewrite, $status); if ($status !== EXIT_SUCCESS && $this->portOffset < $this->tries) { $this->portOffset++; diff --git a/tests/system/Commands/Server/ServeTest.php b/tests/system/Commands/Server/ServeTest.php new file mode 100644 index 000000000000..e8be33d31a27 --- /dev/null +++ b/tests/system/Commands/Server/ServeTest.php @@ -0,0 +1,98 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Commands\Server; + +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\StreamFilterTrait; +use PHPUnit\Framework\Attributes\Group; + +/** + * Override the global passthru() within this namespace so the unit test + * can capture the shell command that Serve::run() would otherwise execute. + * + * @internal + */ +function passthru(string $command, ?int &$result_code = null): void +{ + ServeTest::$capturedCommand = $command; + $result_code = 0; +} + +/** + * @internal + */ +#[Group('Others')] +final class ServeTest extends CIUnitTestCase +{ + use StreamFilterTrait; + + /** + * Captured shell command from the namespaced passthru() stub above. + */ + public static string $capturedCommand = ''; + + protected function setUp(): void + { + parent::setUp(); + + self::$capturedCommand = ''; + $_SERVER['argv'] = ['spark', 'serve']; + } + + protected function tearDown(): void + { + // Restore default argv so other tests are not affected. + $_SERVER['argv'] = ['spark']; + + parent::tearDown(); + } + + public function testServeRunsWithDefaultHostAndPort(): void + { + command('serve'); + + $this->assertStringContainsString(' -S ', self::$capturedCommand); + $this->assertStringContainsString("'localhost:8080'", self::$capturedCommand); + } + + public function testServeEscapesShellMetacharactersInHost(): void + { + $_SERVER['argv'] = ['spark', 'serve', '--host', '$(id)']; + + command('serve --host="$(id)"'); + + // The literal '$(id)' must remain inside single quotes so /bin/sh -c + // never expands it into a sub-shell. escapeshellarg() wraps the + // entire host:port pair in single quotes and escapes any embedded + // single quote. + $this->assertStringNotContainsString(' $(id) ', self::$capturedCommand); + $this->assertMatchesRegularExpression( + "/'[^']*\\\$\\(id\\)[^']*:[0-9]+'/", + self::$capturedCommand, + 'host:port must be wrapped in single quotes by escapeshellarg()', + ); + } + + public function testServeEscapesEmbeddedSingleQuoteInHost(): void + { + $_SERVER['argv'] = ['spark', 'serve', '--host', "evil'host"]; + + command("serve --host=\"evil'host\""); + + // escapeshellarg() turns a single quote into '\'' inside the wrapper, + // so the dangerous quote can never break out of the argument. + $this->assertStringNotContainsString(" evil'host:", self::$capturedCommand); + $this->assertStringContainsString("'\\''", self::$capturedCommand); + } +}