Skip to content

Commit 04cb936

Browse files
WyriHaximusWyriHaximus
authored
Merge pull request #523 from clue-labs/http-validate
Validate outgoing HTTP message headers and reject invalid messages
2 parents 6212f3e + 27d2e74 commit 04cb936

File tree

5 files changed

+183
-12
lines changed

5 files changed

+183
-12
lines changed

‎src/Io/AbstractMessage.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ abstract class AbstractMessage implements MessageInterface
1919
* @internal
2020
* @var string
2121
*/
22-
const REGEX_HEADERS = '/^([^()<>@,;:\\\"\/\[\]?={}\x01-\x20\x7F]++):[\x20\x09]*+((?:[\x20\x09]*+[\x21-\x7E\x80-\xFF]++)*+)[\x20\x09]*+[\r]?+\n/m';
22+
const REGEX_HEADERS = '/^([^()<>@,;:\\\"\/\[\]?={}\x00-\x20\x7F]++):[\x20\x09]*+((?:[\x20\x09]*+[\x21-\x7E\x80-\xFF]++)*+)[\x20\x09]*+[\r]?+\n/m';
2323

2424
/** @var array<string,string[]> */
2525
private $headers = array();

‎src/Io/ClientRequestStream.php‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,33 @@ private function writeHead()
5656
{
5757
$this->state = self::STATE_WRITING_HEAD;
5858

59-
$request = $this->request;
59+
$expected = 0;
60+
$headers = "{$this->request->getMethod()} {$this->request->getRequestTarget()} HTTP/{$this->request->getProtocolVersion()}\r\n";
61+
foreach ($this->request->getHeaders() as $name => $values) {
62+
if (\strpos($name, ':') !== false) {
63+
$expected = -1;
64+
break;
65+
}
66+
foreach ($values as $value) {
67+
$headers .= "$name: $value\r\n";
68+
++$expected;
69+
}
70+
}
71+
72+
/** @var array $m legacy PHP 5.3 only */
73+
if (!\preg_match('#^\S+ \S+ HTTP/1\.[01]\r\n#m', $headers) || \substr_count($headers, "\n") !== ($expected + 1) || (\PHP_VERSION_ID >= 50400 ? \preg_match_all(AbstractMessage::REGEX_HEADERS, $headers) : \preg_match_all(AbstractMessage::REGEX_HEADERS, $headers, $m)) !== $expected) {
74+
$this->closeError(new \InvalidArgumentException('Unable to send request with invalid request headers'));
75+
return;
76+
}
77+
6078
$connectionRef = &$this->connection;
6179
$stateRef = &$this->state;
6280
$pendingWrites = &$this->pendingWrites;
6381
$that = $this;
6482

6583
$promise = $this->connectionManager->connect($this->request->getUri());
6684
$promise->then(
67-
function (ConnectionInterface $connection) use ($request, &$connectionRef, &$stateRef, &$pendingWrites, $that) {
85+
function (ConnectionInterface $connection) use ($headers, &$connectionRef, &$stateRef, &$pendingWrites, $that) {
6886
$connectionRef = $connection;
6987
assert($connectionRef instanceof ConnectionInterface);
7088

@@ -74,14 +92,6 @@ function (ConnectionInterface $connection) use ($request, &$connectionRef, &$sta
7492
$connection->on('error', array($that, 'handleError'));
7593
$connection->on('close', array($that, 'close'));
7694

77-
assert($request instanceof RequestInterface);
78-
$headers = "{$request->getMethod()} {$request->getRequestTarget()} HTTP/{$request->getProtocolVersion()}\r\n";
79-
foreach ($request->getHeaders() as $name => $values) {
80-
foreach ($values as $value) {
81-
$headers .= "$name: $value\r\n";
82-
}
83-
}
84-
8595
$more = $connection->write($headers . "\r\n" . $pendingWrites);
8696

8797
assert($stateRef === ClientRequestStream::STATE_WRITING_HEAD);

‎src/Io/StreamingServer.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,13 +333,26 @@ public function handleResponse(ConnectionInterface $connection, ServerRequestInt
333333
}
334334

335335
// build HTTP response header by appending status line and header fields
336+
$expected = 0;
336337
$headers = "HTTP/" . $version . " " . $code . " " . $response->getReasonPhrase() . "\r\n";
337338
foreach ($response->getHeaders() as $name => $values) {
339+
if (\strpos($name, ':') !== false) {
340+
$expected = -1;
341+
break;
342+
}
338343
foreach ($values as $value) {
339344
$headers .= $name . ": " . $value . "\r\n";
345+
++$expected;
340346
}
341347
}
342348

349+
/** @var array $m legacy PHP 5.3 only */
350+
if ($code < 100 || $code > 999 || \substr_count($headers, "\n") !== ($expected + 1) || (\PHP_VERSION_ID >= 50400 ? \preg_match_all(AbstractMessage::REGEX_HEADERS, $headers) : \preg_match_all(AbstractMessage::REGEX_HEADERS, $headers, $m)) !== $expected) {
351+
$this->emit('error', array(new \InvalidArgumentException('Unable to send response with invalid response headers')));
352+
$this->writeError($connection, Response::STATUS_INTERNAL_SERVER_ERROR, $request);
353+
return;
354+
}
355+
343356
// response to HEAD and 1xx, 204 and 304 responses MUST NOT include a body
344357
// exclude status 101 (Switching Protocols) here for Upgrade request handling above
345358
if ($method === 'HEAD' || ($code >= 100 && $code < 200 && $code !== Response::STATUS_SWITCHING_PROTOCOLS) || $code === Response::STATUS_NO_CONTENT || $code === Response::STATUS_NOT_MODIFIED) {

‎tests/Io/ClientRequestStreamTest.php‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace React\Tests\Http\Io;
44

5+
use Psr\Http\Message\RequestInterface;
56
use Psr\Http\Message\ResponseInterface;
67
use React\Http\Io\ClientRequestStream;
78
use React\Http\Message\Request;
@@ -100,6 +101,67 @@ public function requestShouldEmitErrorIfConnectionEmitsError()
100101
$request->handleError(new \Exception('test'));
101102
}
102103

104+
public static function provideInvalidRequest()
105+
{
106+
$request = new Request('GET' , "http://localhost/");
107+
108+
return array(
109+
array(
110+
$request->withMethod("INVA\r\nLID", '')
111+
),
112+
array(
113+
$request->withRequestTarget('/inva lid')
114+
),
115+
array(
116+
$request->withHeader('Invalid', "Yes\r\n")
117+
),
118+
array(
119+
$request->withHeader('Invalid', "Yes\n")
120+
),
121+
array(
122+
$request->withHeader('Invalid', "Yes\r")
123+
),
124+
array(
125+
$request->withHeader("Inva\r\nlid", 'Yes')
126+
),
127+
array(
128+
$request->withHeader("Inva\nlid", 'Yes')
129+
),
130+
array(
131+
$request->withHeader("Inva\rlid", 'Yes')
132+
),
133+
array(
134+
$request->withHeader('Inva Lid', 'Yes')
135+
),
136+
array(
137+
$request->withHeader('Inva:Lid', 'Yes')
138+
),
139+
array(
140+
$request->withHeader('Invalid', "Val\0ue")
141+
),
142+
array(
143+
$request->withHeader("Inva\0lid", 'Yes')
144+
)
145+
);
146+
}
147+
148+
/**
149+
* @dataProvider provideInvalidRequest
150+
* @param RequestInterface $request
151+
*/
152+
public function testStreamShouldEmitErrorBeforeCreatingConnectionWhenRequestIsInvalid(RequestInterface $request)
153+
{
154+
$connectionManager = $this->getMockBuilder('React\Http\Io\ClientConnectionManager')->disableOriginalConstructor()->getMock();
155+
$connectionManager->expects($this->never())->method('connect');
156+
157+
$stream = new ClientRequestStream($connectionManager, $request);
158+
159+
$stream->on('error', $this->expectCallableOnceWith($this->isInstanceOf('InvalidArgumentException')));
160+
$stream->on('close', $this->expectCallableOnce());
161+
162+
$stream->end();
163+
}
164+
103165
/** @test */
104166
public function requestShouldEmitErrorIfRequestParserThrowsException()
105167
{

‎tests/Io/StreamingServerTest.php‎

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace React\Tests\Http\Io;
44

5+
use Psr\Http\Message\ResponseInterface;
56
use Psr\Http\Message\ServerRequestInterface;
67
use React\EventLoop\Loop;
78
use React\Http\Io\StreamingServer;
@@ -2511,7 +2512,7 @@ function ($data) use (&$buffer) {
25112512
public function testInvalidCallbackFunctionLeadsToException()
25122513
{
25132514
$this->setExpectedException('InvalidArgumentException');
2514-
$server = new StreamingServer(Loop::get(), 'invalid');
2515+
new StreamingServer(Loop::get(), 'invalid');
25152516
}
25162517

25172518
public function testResponseBodyStreamWillStreamDataWithChunkedTransferEncoding()
@@ -2926,6 +2927,91 @@ function ($data) use (&$buffer) {
29262927
$this->assertInstanceOf('RuntimeException', $exception);
29272928
}
29282929

2930+
public static function provideInvalidResponse()
2931+
{
2932+
$response = new Response(200, array(), '', '1.1', 'OK');
2933+
2934+
return array(
2935+
array(
2936+
$response->withStatus(99, 'OK')
2937+
),
2938+
array(
2939+
$response->withStatus(1000, 'OK')
2940+
),
2941+
array(
2942+
$response->withStatus(200, "Invald\r\nReason: Yes")
2943+
),
2944+
array(
2945+
$response->withHeader('Invalid', "Yes\r\n")
2946+
),
2947+
array(
2948+
$response->withHeader('Invalid', "Yes\n")
2949+
),
2950+
array(
2951+
$response->withHeader('Invalid', "Yes\r")
2952+
),
2953+
array(
2954+
$response->withHeader("Inva\r\nlid", 'Yes')
2955+
),
2956+
array(
2957+
$response->withHeader("Inva\nlid", 'Yes')
2958+
),
2959+
array(
2960+
$response->withHeader("Inva\rlid", 'Yes')
2961+
),
2962+
array(
2963+
$response->withHeader('Inva Lid', 'Yes')
2964+
),
2965+
array(
2966+
$response->withHeader('Inva:Lid', 'Yes')
2967+
),
2968+
array(
2969+
$response->withHeader('Invalid', "Val\0ue")
2970+
),
2971+
array(
2972+
$response->withHeader("Inva\0lid", 'Yes')
2973+
)
2974+
);
2975+
}
2976+
2977+
/**
2978+
* @dataProvider provideInvalidResponse
2979+
* @param ResponseInterface $response
2980+
*/
2981+
public function testInvalidResponseObjectWillResultInErrorMessage(ResponseInterface $response)
2982+
{
2983+
$server = new StreamingServer(Loop::get(), function (ServerRequestInterface $request) use ($response) {
2984+
return $response;
2985+
});
2986+
2987+
$exception = null;
2988+
$server->on('error', function (\Exception $ex) use (&$exception) {
2989+
$exception = $ex;
2990+
});
2991+
2992+
$buffer = '';
2993+
$this->connection
2994+
->expects($this->any())
2995+
->method('write')
2996+
->will(
2997+
$this->returnCallback(
2998+
function ($data) use (&$buffer) {
2999+
$buffer .= $data;
3000+
}
3001+
)
3002+
);
3003+
3004+
$server->listen($this->socket);
3005+
$this->socket->emit('connection', array($this->connection));
3006+
3007+
$data = $this->createGetRequest();
3008+
3009+
$this->connection->emit('data', array($data));
3010+
3011+
$this->assertContainsString("HTTP/1.1 500 Internal Server Error\r\n", $buffer);
3012+
$this->assertInstanceOf('InvalidArgumentException', $exception);
3013+
}
3014+
29293015
public function testRequestServerRequestParams()
29303016
{
29313017
$requestValidation = null;

0 commit comments

Comments
 (0)