From 5932089388ff5dae1ea09602ebfe67680ba9664b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Tue, 18 Apr 2017 15:27:42 +0200 Subject: [PATCH 1/4] Use socket address for URI if Host header is missing --- README.md | 6 ++ src/Server.php | 10 +++ tests/FunctionalServerTest.php | 146 +++++++++++++++++++++++++++++++++ tests/ServerTest.php | 26 ++++++ 4 files changed, 188 insertions(+) diff --git a/README.md b/README.md index 74cf2254..988baec7 100644 --- a/README.md +++ b/README.md @@ -286,6 +286,12 @@ URI which provides you access to individiual URI components. Note that (depending on the given `request-target`) certain URI components may or may not be present, for example the `getPath(): string` method will return an empty string for requests in `asterisk-form` or `authority-form`. +Its `getHost(): string` method will return the host as determined by the +effective request URI, which defaults to the local socket address if a HTTP/1.0 +client did not specify one (i.e. no `Host` header). +Its `getScheme(): string` method will return `http` or `https` depending +on whether the request was made over a secure TLS connection to the target host. + You can use `getMethod(): string` and `getRequestTarget(): string` to check this is an accepted request and may want to reject other requests with an appropriate error code, such as `400` (Bad Request) or `405` (Method Not diff --git a/src/Server.php b/src/Server.php index f901bdec..b47b7900 100644 --- a/src/Server.php +++ b/src/Server.php @@ -198,6 +198,16 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface } } + // set URI components from socket address if not already filled via Host header + if ($request->getUri()->getHost() === '') { + $parts = parse_url('tcp://' . $conn->getLocalAddress()); + + $request = $request->withUri( + $request->getUri()->withScheme('http')->withHost($parts['host'])->withPort($parts['port']), + true + ); + } + $contentLength = 0; $stream = new CloseProtectionStream($conn); if ($request->getMethod() === 'CONNECT') { diff --git a/tests/FunctionalServerTest.php b/tests/FunctionalServerTest.php index 4c7ed6ff..f34f2bc3 100644 --- a/tests/FunctionalServerTest.php +++ b/tests/FunctionalServerTest.php @@ -39,6 +39,54 @@ public function testPlainHttpOnRandomPort() $socket->close(); } + public function testPlainHttpOnRandomPortWithoutHostHeaderUsesSocketUri() + { + $loop = Factory::create(); + $socket = new Socket(0, $loop); + $connector = new Connector($loop); + + $server = new Server($socket, function (RequestInterface $request) { + return new Response(200, array(), (string)$request->getUri()); + }); + + $result = $connector->connect($socket->getAddress())->then(function (ConnectionInterface $conn) { + $conn->write("GET / HTTP/1.0\r\n\r\n"); + + return BufferedSink::createPromise($conn); + }); + + $response = Block\await($result, $loop, 1.0); + + $this->assertContains("HTTP/1.0 200 OK", $response); + $this->assertContains('http://' . $socket->getAddress() . '/', $response); + + $socket->close(); + } + + public function testPlainHttpOnRandomPortWithOtherHostHeaderTakesPrecedence() + { + $loop = Factory::create(); + $socket = new Socket(0, $loop); + $connector = new Connector($loop); + + $server = new Server($socket, function (RequestInterface $request) { + return new Response(200, array(), (string)$request->getUri()); + }); + + $result = $connector->connect($socket->getAddress())->then(function (ConnectionInterface $conn) { + $conn->write("GET / HTTP/1.0\r\nHost: localhost:1000\r\n\r\n"); + + return BufferedSink::createPromise($conn); + }); + + $response = Block\await($result, $loop, 1.0); + + $this->assertContains("HTTP/1.0 200 OK", $response); + $this->assertContains('http://localhost:1000/', $response); + + $socket->close(); + } + public function testSecureHttpsOnRandomPort() { if (!function_exists('stream_socket_enable_crypto')) { @@ -72,6 +120,39 @@ public function testSecureHttpsOnRandomPort() $socket->close(); } + public function testSecureHttpsOnRandomPortWithoutHostHeaderUsesSocketUri() + { + if (!function_exists('stream_socket_enable_crypto')) { + $this->markTestSkipped('Not supported on your platform (outdated HHVM?)'); + } + + $loop = Factory::create(); + $socket = new Socket(0, $loop); + $socket = new SecureServer($socket, $loop, array( + 'local_cert' => __DIR__ . '/../examples/localhost.pem' + )); + $connector = new Connector($loop, array( + 'tls' => array('verify_peer' => false) + )); + + $server = new Server($socket, function (RequestInterface $request) { + return new Response(200, array(), (string)$request->getUri()); + }); + + $result = $connector->connect('tls://' . $socket->getAddress())->then(function (ConnectionInterface $conn) { + $conn->write("GET / HTTP/1.0\r\n\r\n"); + + return BufferedSink::createPromise($conn); + }); + + $response = Block\await($result, $loop, 1.0); + + $this->assertContains("HTTP/1.0 200 OK", $response); + $this->assertContains('https://' . $socket->getAddress() . '/', $response); + + $socket->close(); + } + public function testPlainHttpOnStandardPortReturnsUriWithNoPort() { $loop = Factory::create(); @@ -100,6 +181,34 @@ public function testPlainHttpOnStandardPortReturnsUriWithNoPort() $socket->close(); } + public function testPlainHttpOnStandardPortWithoutHostHeaderReturnsUriWithNoPort() + { + $loop = Factory::create(); + try { + $socket = new Socket(80, $loop); + } catch (\RuntimeException $e) { + $this->markTestSkipped('Listening on port 80 failed (root and unused?)'); + } + $connector = new Connector($loop); + + $server = new Server($socket, function (RequestInterface $request) { + return new Response(200, array(), (string)$request->getUri()); + }); + + $result = $connector->connect($socket->getAddress())->then(function (ConnectionInterface $conn) { + $conn->write("GET / HTTP/1.0\r\n\r\n"); + + return BufferedSink::createPromise($conn); + }); + + $response = Block\await($result, $loop, 1.0); + + $this->assertContains("HTTP/1.0 200 OK", $response); + $this->assertContains('http://127.0.0.1/', $response); + + $socket->close(); + } + public function testSecureHttpsOnStandardPortReturnsUriWithNoPort() { if (!function_exists('stream_socket_enable_crypto')) { @@ -137,6 +246,43 @@ public function testSecureHttpsOnStandardPortReturnsUriWithNoPort() $socket->close(); } + public function testSecureHttpsOnStandardPortWithoutHostHeaderUsesSocketUri() + { + if (!function_exists('stream_socket_enable_crypto')) { + $this->markTestSkipped('Not supported on your platform (outdated HHVM?)'); + } + + $loop = Factory::create(); + try { + $socket = new Socket(443, $loop); + } catch (\RuntimeException $e) { + $this->markTestSkipped('Listening on port 443 failed (root and unused?)'); + } + $socket = new SecureServer($socket, $loop, array( + 'local_cert' => __DIR__ . '/../examples/localhost.pem' + )); + $connector = new Connector($loop, array( + 'tls' => array('verify_peer' => false) + )); + + $server = new Server($socket, function (RequestInterface $request) { + return new Response(200, array(), (string)$request->getUri()); + }); + + $result = $connector->connect('tls://' . $socket->getAddress())->then(function (ConnectionInterface $conn) { + $conn->write("GET / HTTP/1.0\r\n\r\n"); + + return BufferedSink::createPromise($conn); + }); + + $response = Block\await($result, $loop, 1.0); + + $this->assertContains("HTTP/1.0 200 OK", $response); + $this->assertContains('https://127.0.0.1/', $response); + + $socket->close(); + } + public function testPlainHttpOnHttpsStandardPortReturnsUriWithPort() { $loop = Factory::create(); diff --git a/tests/ServerTest.php b/tests/ServerTest.php index aa4b8f89..bd4f3a7b 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -267,6 +267,32 @@ public function testRequestNonConnectWithAuthorityRequestTargetWillReject() $this->connection->emit('data', array($data)); } + public function testRequestWithoutHostEventUsesSocketAddress() + { + $requestAssertion = null; + + $server = new Server($this->socket, function (ServerRequestInterface $request) use (&$requestAssertion) { + $requestAssertion = $request; + return new Response(); + }); + + $this->socket->emit('connection', array($this->connection)); + + $this->connection + ->expects($this->once()) + ->method('getLocalAddress') + ->willReturn('127.0.0.1:80'); + + $data = "GET /test HTTP/1.0\r\n\r\n"; + $this->connection->emit('data', array($data)); + + $this->assertInstanceOf('RingCentral\Psr7\Request', $requestAssertion); + $this->assertSame('GET', $requestAssertion->getMethod()); + $this->assertSame('/test', $requestAssertion->getRequestTarget()); + $this->assertEquals('http://127.0.0.1/test', $requestAssertion->getUri()); + $this->assertSame('/test', $requestAssertion->getUri()->getPath()); + } + public function testRequestAbsoluteEvent() { $requestAssertion = null; From 7d42e92a8a7cdb01302a6ab458dbf425912b2410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 21 Apr 2017 09:19:42 +0200 Subject: [PATCH 2/4] Simplify request validation by moving logic to RequestHeaderParser --- src/RequestHeaderParser.php | 24 +++++++++++++++++++++- src/Server.php | 27 +------------------------ tests/RequestHeaderParserTest.php | 33 +++++++++++++++++++++++++++++++ tests/ServerTest.php | 11 +++++++++++ 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/RequestHeaderParser.php b/src/RequestHeaderParser.php index 97fa7a00..1566476a 100644 --- a/src/RequestHeaderParser.php +++ b/src/RequestHeaderParser.php @@ -30,7 +30,7 @@ public function feed($data) } if ($currentHeaderSize > $this->maxSize) { - $this->emit('error', array(new \OverflowException("Maximum header size of {$this->maxSize} exceeded."), $this)); + $this->emit('error', array(new \OverflowException("Maximum header size of {$this->maxSize} exceeded.", 431), $this)); $this->removeAllListeners(); return; } @@ -112,6 +112,28 @@ private function parseRequest($data) } } + // only support HTTP/1.1 and HTTP/1.0 requests + if ($request->getProtocolVersion() !== '1.1' && $request->getProtocolVersion() !== '1.0') { + throw new \InvalidArgumentException('Received request with invalid protocol version', 505); + } + + // HTTP/1.1 requests MUST include a valid host header (host and optional port) + // https://tools.ietf.org/html/rfc7230#section-5.4 + if ($request->getProtocolVersion() === '1.1') { + $parts = parse_url('http://' . $request->getHeaderLine('Host')); + + // make sure value contains valid host component (IP or hostname) + if (!$parts || !isset($parts['scheme'], $parts['host'])) { + $parts = false; + } + + // make sure value does not contain any other URI component + unset($parts['scheme'], $parts['host'], $parts['port']); + if ($parts === false || $parts) { + throw new \InvalidArgumentException('Invalid Host header for HTTP/1.1 request'); + } + } + return array($request, $bodyBuffer); } } diff --git a/src/Server.php b/src/Server.php index b47b7900..e1aa05a5 100644 --- a/src/Server.php +++ b/src/Server.php @@ -165,7 +165,7 @@ public function handleConnection(ConnectionInterface $conn) $that->writeError( $conn, - ($e instanceof \OverflowException) ? 431 : 400 + $e->getCode() !== 0 ? $e->getCode() : 400 ); }); } @@ -173,31 +173,6 @@ public function handleConnection(ConnectionInterface $conn) /** @internal */ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface $request) { - // only support HTTP/1.1 and HTTP/1.0 requests - if ($request->getProtocolVersion() !== '1.1' && $request->getProtocolVersion() !== '1.0') { - $this->emit('error', array(new \InvalidArgumentException('Received request with invalid protocol version'))); - $request = $request->withProtocolVersion('1.1'); - return $this->writeError($conn, 505, $request); - } - - // HTTP/1.1 requests MUST include a valid host header (host and optional port) - // https://tools.ietf.org/html/rfc7230#section-5.4 - if ($request->getProtocolVersion() === '1.1') { - $parts = parse_url('http://' . $request->getHeaderLine('Host')); - - // make sure value contains valid host component (IP or hostname) - if (!$parts || !isset($parts['scheme'], $parts['host'])) { - $parts = false; - } - - // make sure value does not contain any other URI component - unset($parts['scheme'], $parts['host'], $parts['port']); - if ($parts === false || $parts) { - $this->emit('error', array(new \InvalidArgumentException('Invalid Host header for HTTP/1.1 request'))); - return $this->writeError($conn, 400, $request); - } - } - // set URI components from socket address if not already filled via Host header if ($request->getUri()->getHost() === '') { $parts = parse_url('tcp://' . $conn->getLocalAddress()); diff --git a/tests/RequestHeaderParserTest.php b/tests/RequestHeaderParserTest.php index f0422361..9f1f7d75 100644 --- a/tests/RequestHeaderParserTest.php +++ b/tests/RequestHeaderParserTest.php @@ -199,6 +199,39 @@ public function testInvalidAbsoluteFormWithFragmentEmitsError() $this->assertSame('Invalid absolute-form request-target', $error->getMessage()); } + public function testInvalidHostHeaderForHttp11() + { + $error = null; + + $parser = new RequestHeaderParser(); + $parser->on('headers', $this->expectCallableNever()); + $parser->on('error', function ($message) use (&$error) { + $error = $message; + }); + + $parser->feed("GET / HTTP/1.1\r\nHost: a/b/c\r\n\r\n"); + + $this->assertInstanceOf('InvalidArgumentException', $error); + $this->assertSame('Invalid Host header for HTTP/1.1 request', $error->getMessage()); + } + + public function testInvalidHttpVersion() + { + $error = null; + + $parser = new RequestHeaderParser(); + $parser->on('headers', $this->expectCallableNever()); + $parser->on('error', function ($message) use (&$error) { + $error = $message; + }); + + $parser->feed("GET / HTTP/1.2\r\n\r\n"); + + $this->assertInstanceOf('InvalidArgumentException', $error); + $this->assertSame(505, $error->getCode()); + $this->assertSame('Received request with invalid protocol version', $error->getMessage()); + } + private function createGetRequest() { $data = "GET / HTTP/1.1\r\n"; diff --git a/tests/ServerTest.php b/tests/ServerTest.php index bd4f3a7b..af53cc02 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -256,6 +256,17 @@ public function testRequestConnectAuthorityFormNonMatchingHostWillBePassedAsIs() $this->assertSame('example.com', $requestAssertion->getHeaderLine('host')); } + public function testRequestConnectOriginFormRequestTargetWillReject() + { + $server = new Server($this->socket, $this->expectCallableNever()); + $server->on('error', $this->expectCallableOnce()); + + $this->socket->emit('connection', array($this->connection)); + + $data = "CONNECT / HTTP/1.1\r\nHost: example.com\r\n\r\n"; + $this->connection->emit('data', array($data)); + } + public function testRequestNonConnectWithAuthorityRequestTargetWillReject() { $server = new Server($this->socket, $this->expectCallableNever()); From e6b12d6a0043f453af8c3976467f9c4c855927ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 21 Apr 2017 12:57:46 +0200 Subject: [PATCH 3/4] Sanitize Host header value across all requests --- README.md | 5 ++- src/RequestHeaderParser.php | 22 +++++++--- src/Server.php | 33 ++++++++------- tests/RequestHeaderParserTest.php | 22 ++++++++-- tests/ServerTest.php | 67 +++++-------------------------- 5 files changed, 68 insertions(+), 81 deletions(-) diff --git a/README.md b/README.md index 988baec7..d8155817 100644 --- a/README.md +++ b/README.md @@ -292,6 +292,9 @@ client did not specify one (i.e. no `Host` header). Its `getScheme(): string` method will return `http` or `https` depending on whether the request was made over a secure TLS connection to the target host. +The `Host` header value will be sanitized to match this host component plus the +port component only if it is non-standard for this URI scheme. + You can use `getMethod(): string` and `getRequestTarget(): string` to check this is an accepted request and may want to reject other requests with an appropriate error code, such as `400` (Bad Request) or `405` (Method Not @@ -300,7 +303,7 @@ Allowed). > The `CONNECT` method is useful in a tunneling setup (HTTPS proxy) and not something most HTTP servers would want to care about. Note that if you want to handle this method, the client MAY send a different - request-target than the `Host` header field (such as removing default ports) + request-target than the `Host` header value (such as removing default ports) and the request-target MUST take precendence when forwarding. The HTTP specs define an opaque "tunneling mode" for this method and make no use of the message body. diff --git a/src/RequestHeaderParser.php b/src/RequestHeaderParser.php index 1566476a..91c8d2cd 100644 --- a/src/RequestHeaderParser.php +++ b/src/RequestHeaderParser.php @@ -55,6 +55,8 @@ private function parseRequest($data) { list($headers, $bodyBuffer) = explode("\r\n\r\n", $data, 2); + // parser does not support asterisk-form and authority-form + // remember original target and temporarily replace and re-apply below $originalTarget = null; if (strpos($headers, 'OPTIONS * ') === 0) { $originalTarget = '*'; @@ -75,7 +77,7 @@ private function parseRequest($data) $request = g7\parse_request($headers); // create new obj implementing ServerRequestInterface by preserving all - // previous properties and restoring original request target-target + // previous properties and restoring original request-target $target = $request->getRequestTarget(); $request = new ServerRequest( $request->getMethod(), @@ -95,9 +97,18 @@ private function parseRequest($data) ); } + // re-apply actual request target from above if ($originalTarget !== null) { + $uri = $request->getUri()->withPath(''); + + // re-apply host and port from request-target if given + $parts = parse_url('tcp://' . $originalTarget); + if (isset($parts['host'], $parts['port'])) { + $uri = $uri->withHost($parts['host'])->withPort($parts['port']); + } + $request = $request->withUri( - $request->getUri()->withPath(''), + $uri, true )->withRequestTarget($originalTarget); } @@ -117,9 +128,8 @@ private function parseRequest($data) throw new \InvalidArgumentException('Received request with invalid protocol version', 505); } - // HTTP/1.1 requests MUST include a valid host header (host and optional port) - // https://tools.ietf.org/html/rfc7230#section-5.4 - if ($request->getProtocolVersion() === '1.1') { + // Optional Host header value MUST be valid (host and optional port) + if ($request->hasHeader('Host')) { $parts = parse_url('http://' . $request->getHeaderLine('Host')); // make sure value contains valid host component (IP or hostname) @@ -130,7 +140,7 @@ private function parseRequest($data) // make sure value does not contain any other URI component unset($parts['scheme'], $parts['host'], $parts['port']); if ($parts === false || $parts) { - throw new \InvalidArgumentException('Invalid Host header for HTTP/1.1 request'); + throw new \InvalidArgumentException('Invalid Host header value'); } } diff --git a/src/Server.php b/src/Server.php index e1aa05a5..c0212c7c 100644 --- a/src/Server.php +++ b/src/Server.php @@ -183,6 +183,24 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface ); } + // Update request URI to "https" scheme if the connection is encrypted + if ($this->isConnectionEncrypted($conn)) { + // The request URI may omit default ports here, so try to parse port + // from Host header field (if possible) + $port = $request->getUri()->getPort(); + if ($port === null) { + $port = parse_url('tcp://' . $request->getHeaderLine('Host'), PHP_URL_PORT); // @codeCoverageIgnore + } + + $request = $request->withUri( + $request->getUri()->withScheme('https')->withPort($port), + true + ); + } + + // always sanitize Host header because it contains critical routing information + $request = $request->withHeader('Host', $request->getUri()->withUserInfo('')->getAuthority()); + $contentLength = 0; $stream = new CloseProtectionStream($conn); if ($request->getMethod() === 'CONNECT') { @@ -240,21 +258,6 @@ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface '[]' ); - // Update request URI to "https" scheme if the connection is encrypted - if ($this->isConnectionEncrypted($conn)) { - // The request URI may omit default ports here, so try to parse port - // from Host header field (if possible) - $port = $request->getUri()->getPort(); - if ($port === null) { - $port = parse_url('tcp://' . $request->getHeaderLine('Host'), PHP_URL_PORT); - } - - $request = $request->withUri( - $request->getUri()->withScheme('https')->withPort($port), - true - ); - } - $callback = $this->callback; $promise = new Promise(function ($resolve, $reject) use ($callback, $request) { $resolve($callback($request)); diff --git a/tests/RequestHeaderParserTest.php b/tests/RequestHeaderParserTest.php index 9f1f7d75..9ecefddf 100644 --- a/tests/RequestHeaderParserTest.php +++ b/tests/RequestHeaderParserTest.php @@ -199,7 +199,7 @@ public function testInvalidAbsoluteFormWithFragmentEmitsError() $this->assertSame('Invalid absolute-form request-target', $error->getMessage()); } - public function testInvalidHostHeaderForHttp11() + public function testInvalidHeaderContainsFullUri() { $error = null; @@ -209,10 +209,26 @@ public function testInvalidHostHeaderForHttp11() $error = $message; }); - $parser->feed("GET / HTTP/1.1\r\nHost: a/b/c\r\n\r\n"); + $parser->feed("GET / HTTP/1.1\r\nHost: http://user:pass@host/\r\n\r\n"); $this->assertInstanceOf('InvalidArgumentException', $error); - $this->assertSame('Invalid Host header for HTTP/1.1 request', $error->getMessage()); + $this->assertSame('Invalid Host header value', $error->getMessage()); + } + + public function testInvalidAbsoluteFormWithHostHeaderEmpty() + { + $error = null; + + $parser = new RequestHeaderParser(); + $parser->on('headers', $this->expectCallableNever()); + $parser->on('error', function ($message) use (&$error) { + $error = $message; + }); + + $parser->feed("GET http://example.com/ HTTP/1.1\r\nHost: \r\n\r\n"); + + $this->assertInstanceOf('InvalidArgumentException', $error); + $this->assertSame('Invalid Host header value', $error->getMessage()); } public function testInvalidHttpVersion() diff --git a/tests/ServerTest.php b/tests/ServerTest.php index af53cc02..ee435dc7 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -88,7 +88,7 @@ public function testRequestEvent() $this->assertSame('/', $requestAssertion->getRequestTarget()); $this->assertSame('/', $requestAssertion->getUri()->getPath()); $this->assertSame('http://example.com/', (string)$requestAssertion->getUri()); - $this->assertSame('example.com:80', $requestAssertion->getHeaderLine('Host')); + $this->assertSame('example.com', $requestAssertion->getHeaderLine('Host')); $this->assertSame('127.0.0.1', $requestAssertion->remoteAddress); } @@ -155,7 +155,7 @@ public function testRequestGetWithHostAndDefaultPortWillBeIgnored() $this->assertSame('/', $requestAssertion->getUri()->getPath()); $this->assertSame('http://example.com/', (string)$requestAssertion->getUri()); $this->assertSame(null, $requestAssertion->getUri()->getPort()); - $this->assertSame('example.com:80', $requestAssertion->getHeaderLine('Host')); + $this->assertSame('example.com', $requestAssertion->getHeaderLine('Host')); } public function testRequestOptionsAsterisk() @@ -209,7 +209,7 @@ public function testRequestConnectAuthorityForm() $this->assertSame('', $requestAssertion->getUri()->getPath()); $this->assertSame('http://example.com:443', (string)$requestAssertion->getUri()); $this->assertSame(443, $requestAssertion->getUri()->getPort()); - $this->assertSame('example.com:443', $requestAssertion->getHeaderLine('host')); + $this->assertSame('example.com:443', $requestAssertion->getHeaderLine('Host')); } public function testRequestConnectAuthorityFormWithDefaultPortWillBeIgnored() @@ -231,10 +231,10 @@ public function testRequestConnectAuthorityFormWithDefaultPortWillBeIgnored() $this->assertSame('', $requestAssertion->getUri()->getPath()); $this->assertSame('http://example.com', (string)$requestAssertion->getUri()); $this->assertSame(null, $requestAssertion->getUri()->getPort()); - $this->assertSame('example.com:80', $requestAssertion->getHeaderLine('Host')); + $this->assertSame('example.com', $requestAssertion->getHeaderLine('Host')); } - public function testRequestConnectAuthorityFormNonMatchingHostWillBePassedAsIs() + public function testRequestConnectAuthorityFormNonMatchingHostWillBeOverwritten() { $requestAssertion = null; $server = new Server($this->socket, function (ServerRequestInterface $request) use (&$requestAssertion) { @@ -244,7 +244,7 @@ public function testRequestConnectAuthorityFormNonMatchingHostWillBePassedAsIs() $this->socket->emit('connection', array($this->connection)); - $data = "CONNECT example.com:80 HTTP/1.1\r\nHost: example.com\r\n\r\n"; + $data = "CONNECT example.com:80 HTTP/1.1\r\nHost: other.example.org\r\n\r\n"; $this->connection->emit('data', array($data)); $this->assertInstanceOf('RingCentral\Psr7\Request', $requestAssertion); @@ -253,7 +253,7 @@ public function testRequestConnectAuthorityFormNonMatchingHostWillBePassedAsIs() $this->assertSame('', $requestAssertion->getUri()->getPath()); $this->assertSame('http://example.com', (string)$requestAssertion->getUri()); $this->assertSame(null, $requestAssertion->getUri()->getPort()); - $this->assertSame('example.com', $requestAssertion->getHeaderLine('host')); + $this->assertSame('example.com', $requestAssertion->getHeaderLine('Host')); } public function testRequestConnectOriginFormRequestTargetWillReject() @@ -349,7 +349,7 @@ public function testRequestAbsoluteAddsMissingHostEvent() $this->assertSame('example.com:8080', $requestAssertion->getHeaderLine('Host')); } - public function testRequestAbsoluteNonMatchingHostWillBePassedAsIs() + public function testRequestAbsoluteNonMatchingHostWillBeOverwritten() { $requestAssertion = null; @@ -368,7 +368,7 @@ public function testRequestAbsoluteNonMatchingHostWillBePassedAsIs() $this->assertSame('http://example.com/test', $requestAssertion->getRequestTarget()); $this->assertEquals('http://example.com/test', $requestAssertion->getUri()); $this->assertSame('/test', $requestAssertion->getUri()->getPath()); - $this->assertSame('other.example.org', $requestAssertion->getHeaderLine('Host')); + $this->assertSame('example.com', $requestAssertion->getHeaderLine('Host')); } public function testRequestOptionsAsteriskEvent() @@ -1002,39 +1002,7 @@ public function testChunkedIsMixedUpperAndLowerCase() $this->connection->emit('data', array($data)); } - public function testRequestHttp11WithoutHostWillEmitErrorAndSendErrorResponse() - { - $error = null; - $server = new Server($this->socket, $this->expectCallableNever()); - $server->on('error', function ($message) use (&$error) { - $error = $message; - }); - - $buffer = ''; - - $this->connection - ->expects($this->any()) - ->method('write') - ->will( - $this->returnCallback( - function ($data) use (&$buffer) { - $buffer .= $data; - } - ) - ); - - $this->socket->emit('connection', array($this->connection)); - - $data = "GET / HTTP/1.1\r\n\r\n"; - $this->connection->emit('data', array($data)); - - $this->assertInstanceOf('InvalidArgumentException', $error); - - $this->assertContains("HTTP/1.1 400 Bad Request\r\n", $buffer); - $this->assertContains("\r\n\r\nError 400: Bad Request", $buffer); - } - - public function testRequestHttp11WithMalformedHostWillEmitErrorAndSendErrorResponse() + public function testRequestWithMalformedHostWillEmitErrorAndSendErrorResponse() { $error = null; $server = new Server($this->socket, $this->expectCallableNever()); @@ -1066,7 +1034,7 @@ function ($data) use (&$buffer) { $this->assertContains("\r\n\r\nError 400: Bad Request", $buffer); } - public function testRequestHttp11WithInvalidHostUriComponentsWillEmitErrorAndSendErrorResponse() + public function testRequestWithInvalidHostUriComponentsWillEmitErrorAndSendErrorResponse() { $error = null; $server = new Server($this->socket, $this->expectCallableNever()); @@ -1098,19 +1066,6 @@ function ($data) use (&$buffer) { $this->assertContains("\r\n\r\nError 400: Bad Request", $buffer); } - public function testRequestHttp10WithoutHostEmitsRequestWithNoError() - { - $server = new Server($this->socket, function (ServerRequestInterface $request) { - return \React\Promise\resolve(new Response()); - }); - $server->on('error', $this->expectCallableNever()); - - $this->socket->emit('connection', array($this->connection)); - - $data = "GET / HTTP/1.0\r\n\r\n"; - $this->connection->emit('data', array($data)); - } - public function testWontEmitFurtherDataWhenContentLengthIsReached() { $dataEvent = $this->expectCallableOnceWith('hello'); From a60e20265473ca4492e68c5ad4fe97c2e6258227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sat, 22 Apr 2017 00:41:16 +0200 Subject: [PATCH 4/4] Move complete URI handling to RequestHeaderParser --- src/RequestHeaderParser.php | 64 ++++++++++++++++++++++++------- src/Server.php | 33 ++-------------- tests/RequestHeaderParserTest.php | 36 +++++++++++++++-- tests/ServerTest.php | 4 +- 4 files changed, 89 insertions(+), 48 deletions(-) diff --git a/src/RequestHeaderParser.php b/src/RequestHeaderParser.php index 91c8d2cd..cb25dcab 100644 --- a/src/RequestHeaderParser.php +++ b/src/RequestHeaderParser.php @@ -17,6 +17,13 @@ class RequestHeaderParser extends EventEmitter private $buffer = ''; private $maxSize = 4096; + private $uri; + + public function __construct($localSocketUri = '') + { + $this->uri = $localSocketUri; + } + public function feed($data) { $this->buffer .= $data; @@ -88,15 +95,6 @@ private function parseRequest($data) ); $request = $request->withRequestTarget($target); - // Do not assume this is HTTPS when this happens to be port 443 - // detecting HTTPS is left up to the socket layer (TLS detection) - if ($request->getUri()->getScheme() === 'https') { - $request = $request->withUri( - $request->getUri()->withScheme('http')->withPort(443), - true - ); - } - // re-apply actual request target from above if ($originalTarget !== null) { $uri = $request->getUri()->withPath(''); @@ -113,6 +111,11 @@ private function parseRequest($data) )->withRequestTarget($originalTarget); } + // only support HTTP/1.1 and HTTP/1.0 requests + if ($request->getProtocolVersion() !== '1.1' && $request->getProtocolVersion() !== '1.0') { + throw new \InvalidArgumentException('Received request with invalid protocol version', 505); + } + // ensure absolute-form request-target contains a valid URI if (strpos($request->getRequestTarget(), '://') !== false) { $parts = parse_url($request->getRequestTarget()); @@ -123,11 +126,6 @@ private function parseRequest($data) } } - // only support HTTP/1.1 and HTTP/1.0 requests - if ($request->getProtocolVersion() !== '1.1' && $request->getProtocolVersion() !== '1.0') { - throw new \InvalidArgumentException('Received request with invalid protocol version', 505); - } - // Optional Host header value MUST be valid (host and optional port) if ($request->hasHeader('Host')) { $parts = parse_url('http://' . $request->getHeaderLine('Host')); @@ -144,6 +142,44 @@ private function parseRequest($data) } } + // set URI components from socket address if not already filled via Host header + if ($request->getUri()->getHost() === '') { + $parts = parse_url($this->uri); + + $request = $request->withUri( + $request->getUri()->withScheme('http')->withHost($parts['host'])->withPort($parts['port']), + true + ); + } + + // Do not assume this is HTTPS when this happens to be port 443 + // detecting HTTPS is left up to the socket layer (TLS detection) + if ($request->getUri()->getScheme() === 'https') { + $request = $request->withUri( + $request->getUri()->withScheme('http')->withPort(443), + true + ); + } + + // Update request URI to "https" scheme if the connection is encrypted + $parts = parse_url($this->uri); + if (isset($parts['scheme']) && $parts['scheme'] === 'https') { + // The request URI may omit default ports here, so try to parse port + // from Host header field (if possible) + $port = $request->getUri()->getPort(); + if ($port === null) { + $port = parse_url('tcp://' . $request->getHeaderLine('Host'), PHP_URL_PORT); // @codeCoverageIgnore + } + + $request = $request->withUri( + $request->getUri()->withScheme('https')->withPort($port), + true + ); + } + + // always sanitize Host header because it contains critical routing information + $request = $request->withUri($request->getUri()->withUserInfo('u')->withUserInfo('')); + return array($request, $bodyBuffer); } } diff --git a/src/Server.php b/src/Server.php index c0212c7c..fa5c91fe 100644 --- a/src/Server.php +++ b/src/Server.php @@ -145,7 +145,10 @@ public function __construct(SocketServerInterface $io, $callback) public function handleConnection(ConnectionInterface $conn) { $that = $this; - $parser = new RequestHeaderParser(); + $parser = new RequestHeaderParser( + ($this->isConnectionEncrypted($conn) ? 'https://' : 'http://') . $conn->getLocalAddress() + ); + $listener = array($parser, 'feed'); $parser->on('headers', function (RequestInterface $request, $bodyBuffer) use ($conn, $listener, $parser, $that) { // parsing request completed => stop feeding parser @@ -173,34 +176,6 @@ public function handleConnection(ConnectionInterface $conn) /** @internal */ public function handleRequest(ConnectionInterface $conn, ServerRequestInterface $request) { - // set URI components from socket address if not already filled via Host header - if ($request->getUri()->getHost() === '') { - $parts = parse_url('tcp://' . $conn->getLocalAddress()); - - $request = $request->withUri( - $request->getUri()->withScheme('http')->withHost($parts['host'])->withPort($parts['port']), - true - ); - } - - // Update request URI to "https" scheme if the connection is encrypted - if ($this->isConnectionEncrypted($conn)) { - // The request URI may omit default ports here, so try to parse port - // from Host header field (if possible) - $port = $request->getUri()->getPort(); - if ($port === null) { - $port = parse_url('tcp://' . $request->getHeaderLine('Host'), PHP_URL_PORT); // @codeCoverageIgnore - } - - $request = $request->withUri( - $request->getUri()->withScheme('https')->withPort($port), - true - ); - } - - // always sanitize Host header because it contains critical routing information - $request = $request->withHeader('Host', $request->getUri()->withUserInfo('')->getAuthority()); - $contentLength = 0; $stream = new CloseProtectionStream($conn); if ($request->getMethod() === 'CONNECT') { diff --git a/tests/RequestHeaderParserTest.php b/tests/RequestHeaderParserTest.php index 9ecefddf..be725294 100644 --- a/tests/RequestHeaderParserTest.php +++ b/tests/RequestHeaderParserTest.php @@ -49,7 +49,7 @@ public function testHeadersEventShouldReturnRequestAndBodyBuffer() $this->assertSame('GET', $request->getMethod()); $this->assertEquals('http://example.com/', $request->getUri()); $this->assertSame('1.1', $request->getProtocolVersion()); - $this->assertSame(array('Host' => array('example.com:80'), 'Connection' => array('close')), $request->getHeaders()); + $this->assertSame(array('Host' => array('example.com'), 'Connection' => array('close')), $request->getHeaders()); $this->assertSame('RANDOM DATA', $bodyBuffer); } @@ -87,13 +87,43 @@ public function testHeadersEventShouldParsePathAndQueryString() $this->assertEquals('http://example.com/foo?bar=baz', $request->getUri()); $this->assertSame('1.1', $request->getProtocolVersion()); $headers = array( - 'Host' => array('example.com:80'), + 'Host' => array('example.com'), 'User-Agent' => array('react/alpha'), 'Connection' => array('close'), ); $this->assertSame($headers, $request->getHeaders()); } + public function testHeaderEventWithShouldApplyDefaultAddressFromConstructor() + { + $request = null; + + $parser = new RequestHeaderParser('http://127.1.1.1:8000'); + $parser->on('headers', function ($parsedRequest) use (&$request) { + $request = $parsedRequest; + }); + + $parser->feed("GET /foo HTTP/1.0\r\n\r\n"); + + $this->assertEquals('http://127.1.1.1:8000/foo', $request->getUri()); + $this->assertEquals('127.1.1.1:8000', $request->getHeaderLine('Host')); + } + + public function testHeaderEventViaHttpsShouldApplySchemeFromConstructor() + { + $request = null; + + $parser = new RequestHeaderParser('https://127.1.1.1:8000'); + $parser->on('headers', function ($parsedRequest) use (&$request) { + $request = $parsedRequest; + }); + + $parser->feed("GET /foo HTTP/1.0\r\nHost: example.com\r\n\r\n"); + + $this->assertEquals('https://example.com/foo', $request->getUri()); + $this->assertEquals('example.com', $request->getHeaderLine('Host')); + } + public function testHeaderOverflowShouldEmitError() { $error = null; @@ -137,7 +167,7 @@ public function testHeaderOverflowShouldNotEmitErrorWhenDataExceedsMaxHeaderSize $parser->feed($data); $headers = array( - 'Host' => array('example.com:80'), + 'Host' => array('example.com'), 'User-Agent' => array('react/alpha'), 'Connection' => array('close'), ); diff --git a/tests/ServerTest.php b/tests/ServerTest.php index ee435dc7..25c01810 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -287,13 +287,13 @@ public function testRequestWithoutHostEventUsesSocketAddress() return new Response(); }); - $this->socket->emit('connection', array($this->connection)); - $this->connection ->expects($this->once()) ->method('getLocalAddress') ->willReturn('127.0.0.1:80'); + $this->socket->emit('connection', array($this->connection)); + $data = "GET /test HTTP/1.0\r\n\r\n"; $this->connection->emit('data', array($data));