Skip to content

Commit

Permalink
Strictly forbid pseudo headers in messages
Browse files Browse the repository at this point in the history
  • Loading branch information
@trowski
trowski committed Aug 20, 2019
1 parent b199fdb commit 4d91b47
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 22 deletions.
20 changes: 11 additions & 9 deletions src/Driver/Http2Driver.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,12 +467,7 @@ private function dispatchInternalRequest(Request $request, int $streamId, PsrUri
$path .= "?" . $query;
}

$headers = \array_merge([
":authority" => [$uri->getAuthority()],
":scheme" => [$uri->getScheme()],
":path" => [$path],
":method" => ["GET"],
], \array_intersect_key($request->getHeaders(), self::PUSH_PROMISE_INTERSECT), $headers);
$headers = \array_intersect_key($request->getHeaders(), self::PUSH_PROMISE_INTERSECT);

$id = $this->localStreamId += 2; // Server initiated stream IDs must be even.
$this->remoteStreamId = \max($id, $this->remoteStreamId);
Expand All @@ -485,6 +480,13 @@ private function dispatchInternalRequest(Request $request, int $streamId, PsrUri
Http2Stream::RESERVED | Http2Stream::REMOTE_CLOSED
);

$headers = \array_merge([
":authority" => [$uri->getAuthority()],
":scheme" => [$uri->getScheme()],
":path" => [$path],
":method" => ["GET"],
], $headers);

$headers = \pack("N", $id) . $this->table->encode($headers);

if (\strlen($headers) >= $this->maxFrameSize) {
Expand Down Expand Up @@ -1245,7 +1247,7 @@ private function parser(string $settings = null): \Generator
throw new Http2StreamException("Shutting down", $id, self::REFUSED_STREAM);
}

if (!isset($pseudo[":method"], $pseudo[":path"], $pseudo[":scheme"])
if (!isset($pseudo[":method"], $pseudo[":path"], $pseudo[":scheme"], $pseudo[":authority"])
|| isset($headers["connection"])
|| $pseudo[":path"] === ''
|| (isset($headers["te"]) && \implode($headers["te"]) !== "trailers")
Expand All @@ -1255,8 +1257,8 @@ private function parser(string $settings = null): \Generator

$method = $pseudo[":method"];
$target = $pseudo[":path"];
$scheme = $pseudo[":scheme"] ?? ($this->client->isEncrypted() ? "https" : "http");
$host = $pseudo[":authority"] ?? "";
$scheme = $pseudo[":scheme"];
$host = $pseudo[":authority"];
$query = null;

if (!\preg_match("#^([A-Z\d\.\-]+|\[[\d:]+\])(?::([1-9]\d*))?$#i", $host, $matches)) {
Expand Down
8 changes: 8 additions & 0 deletions src/Request.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ public function setHeaders(array $headers): void
*/
public function setHeader(string $name, $value): void
{
if (($name[0] ?? ":") === ":") {
throw new \Error("Header name cannot be empty or start with a colon (:)");
}

parent::setHeader($name, $value);

if (\stripos($name, "cookie") === 0) {
Expand All @@ -186,6 +190,10 @@ public function setHeader(string $name, $value): void
*/
public function addHeader(string $name, $value): void
{
if (($name[0] ?? ":") === ":") {
throw new \Error("Header name cannot be empty or start with a colon (:)");
}

parent::addHeader($name, $value);

if (\stripos($name, "cookie") === 0) {
Expand Down
12 changes: 9 additions & 3 deletions src/Response.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@

final class Response extends Message
{


/** @var InputStream */
private $body;

Expand Down Expand Up @@ -144,6 +142,10 @@ public function setHeaders(array $headers): void
*/
public function setHeader(string $name, $value): void
{
if (($name[0] ?? ":") === ":") {
throw new \Error("Header name cannot be empty or start with a colon (:)");
}

parent::setHeader($name, $value);

if (\stripos($name, "set-cookie") === 0) {
Expand All @@ -161,6 +163,10 @@ public function setHeader(string $name, $value): void
*/
public function addHeader(string $name, $value): void
{
if (($name[0] ?? ":") === ":") {
throw new \Error("Header name cannot be empty or start with a colon (:)");
}

parent::addHeader($name, $value);

if (\stripos($name, "set-cookie") === 0) {
Expand Down Expand Up @@ -354,7 +360,7 @@ public function push(string $url, array $headers = []): void
{
\assert((function (array $headers) {
foreach ($headers as $name => $header) {
if ($name[0] === ":" || !\strncasecmp("host", $name, 4)) {
if (($name[0] ?? ":") === ":" || !\strncasecmp("host", $name, 4)) {
return false;
}
}
Expand Down
25 changes: 15 additions & 10 deletions test/Driver/Http2DriverTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Amp\Delayed;
use Amp\Emitter;
use Amp\Http\HPack;
use Amp\Http\Message;
use Amp\Http\Server\Driver\Client;
use Amp\Http\Server\Driver\Http2Driver;
use Amp\Http\Server\Driver\HttpDriver;
Expand Down Expand Up @@ -84,11 +85,17 @@ public function testSimpleCases(string $msg, array $expectations)
}
}

/** @var Request $request */
$this->assertInstanceOf(Request::class, $request);

/** @var \Amp\Http\Server\Request $request */
$body = Promise\wait($request->getBody()->buffer());
$trailers = Promise\wait($request->getTrailers());
$body = yield $request->getBody()->buffer();
$trailers = $request->getTrailers();

if ($trailers !== null) {
$trailers = yield $trailers->awaitMessage();
/** @var $trailers Message */
$this->assertInstanceOf(Message::class, $trailers);
}

$headers = $request->getHeaders();
foreach ($headers as $header => $value) {
Expand All @@ -105,7 +112,7 @@ public function testSimpleCases(string $msg, array $expectations)
$this->assertSame($expectations["port"] ?? 80, $request->getUri()->getPort() ?: $defaultPort, "uriPort mismatch");
$this->assertSame($expectations["host"], $request->getUri()->getHost(), "uriHost mismatch");
$this->assertSame($expectations["body"], $body, "body mismatch");
$this->assertSame($expectations["trailers"] ?? [], $trailers->getHeaders());
$this->assertSame($expectations["trailers"] ?? [], $trailers ? $trailers->getHeaders() : []);
}
}

Expand Down Expand Up @@ -164,7 +171,7 @@ public function provideSimpleCases(): array
":scheme" => ["http"],
":method" => ["GET"],
"te" => ["trailers"],
"trailers" => ["expires"],
"trailer" => ["expires"],
];

$msg = self::packFrame(\pack("N", 100), Http2Driver::WINDOW_UPDATE, Http2Driver::NOFLAG);
Expand All @@ -179,7 +186,7 @@ public function provideSimpleCases(): array
"method" => "GET",
"uri" => "/foo",
"host" => "localhost",
"headers" => ["trailers" => ["expires"]],
"headers" => ["te" => ["trailers"], "trailer" => ["expires"]],
"body" => "ab",
"trailers" => ["expires" => ["date"]],
];
Expand Down Expand Up @@ -579,7 +586,7 @@ function () {

$emitter->emit("{data}");

Promise\wait($writer); // Will throw if the writer is not complete.
yield $writer; // Will throw if the writer is not complete.
}

public function testPush()
Expand Down Expand Up @@ -616,9 +623,7 @@ function () {

$this->assertInstanceOf(Request::class, $requests[0]);

$writer = $driver->write($requests[0], $response);

Promise\wait($writer);
yield $driver->write($requests[0], $response);

$paths = ["/base", "/absolute/path", "/base/relative/path", "/base/path/with/query"];

Expand Down

0 comments on commit 4d91b47

Please sign in to comment.