From dc0fda8199c11e6671627a1fa7dbe7dd87a688c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Tue, 28 Sep 2021 07:55:23 +0200 Subject: [PATCH 1/3] Consistently reject proxy requests when run behind web server --- src/SapiHandler.php | 11 ++++++++++- tests/RouteHandlerTest.php | 38 ++++++++++++++++++++++++++++++++++++ tests/SapiHandlerTest.php | 40 ++++++++++++++++++++++++++++++++++++++ tests/acceptance.sh | 4 ++++ 4 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/SapiHandler.php b/src/SapiHandler.php index a841e0f..56ea048 100644 --- a/src/SapiHandler.php +++ b/src/SapiHandler.php @@ -42,11 +42,17 @@ public function requestFromGlobals(): ServerRequestInterface } // @codeCoverageIgnoreEnd + $target = ($_SERVER['REQUEST_URI'] ?? '/'); + $url = $target; + if (($target[0] ?? '/') === '/') { + $url = ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . $url; + } + $body = file_get_contents('php://input'); $request = new ServerRequest( $_SERVER['REQUEST_METHOD'] ?? 'GET', - ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . ($_SERVER['REQUEST_URI'] ?? '/'), + $url, $headers, $body, substr($_SERVER['SERVER_PROTOCOL'] ?? 'http/1.1', 5), @@ -55,6 +61,9 @@ public function requestFromGlobals(): ServerRequestInterface if ($host === null) { $request = $request->withoutHeader('Host'); } + if (isset($target[0]) && $target[0] !== '/') { + $request = $request->withRequestTarget($target); + } $request = $request->withParsedBody($_POST); // Content-Length / Content-Type are special <3 diff --git a/tests/RouteHandlerTest.php b/tests/RouteHandlerTest.php index 76ee0e0..7780f52 100644 --- a/tests/RouteHandlerTest.php +++ b/tests/RouteHandlerTest.php @@ -6,6 +6,8 @@ use FrameworkX\MiddlewareHandler; use FrameworkX\RouteHandler; use PHPUnit\Framework\TestCase; +use Psr\Http\Message\ResponseInterface; +use React\Http\Message\ServerRequest; class RouteHandlerTest extends TestCase { @@ -41,4 +43,40 @@ public function testMapRouteWithMiddlewareAndControllerAddsRouteWithMiddlewareHa $handler->map(['GET'], '/', $middleware, $controller); } + + public function testHandleRequestWithProxyRequestReturnsResponseWithMessageThatProxyRequestsAreNotAllowed() + { + $request = new ServerRequest('GET', 'http://example.com/'); + $request = $request->withRequestTarget('http://example.com/'); + + $handler = new RouteHandler(); + $response = $handler($request); + + /** @var ResponseInterface $response */ + $this->assertInstanceOf(ResponseInterface::class, $response); + $this->assertEquals(400, $response->getStatusCode()); + $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type')); + $this->assertStringMatchesFormat("\n%a\n", (string) $response->getBody()); + + $this->assertStringContainsString("Error 400: Proxy Requests Not Allowed\n", (string) $response->getBody()); + $this->assertStringContainsString("

Please check your settings and retry.

\n", (string) $response->getBody()); + } + + public function testHandleRequestWithConnectProxyRequestReturnsResponseWithMessageThatProxyRequestsAreNotAllowed() + { + $request = new ServerRequest('CONNECT', 'example.com:80'); + $request = $request->withRequestTarget('example.com:80'); + + $handler = new RouteHandler(); + $response = $handler($request); + + /** @var ResponseInterface $response */ + $this->assertInstanceOf(ResponseInterface::class, $response); + $this->assertEquals(400, $response->getStatusCode()); + $this->assertEquals('text/html; charset=utf-8', $response->getHeaderLine('Content-Type')); + $this->assertStringMatchesFormat("\n%a\n", (string) $response->getBody()); + + $this->assertStringContainsString("Error 400: Proxy Requests Not Allowed\n", (string) $response->getBody()); + $this->assertStringContainsString("

Please check your settings and retry.

\n", (string) $response->getBody()); + } } diff --git a/tests/SapiHandlerTest.php b/tests/SapiHandlerTest.php index 2c247dd..7d45972 100644 --- a/tests/SapiHandlerTest.php +++ b/tests/SapiHandlerTest.php @@ -79,6 +79,46 @@ public function testRequestFromGlobalsWithGetRequestOverHttps() $this->assertEquals('localhost', $request->getHeaderLine('Host')); } + /** + * @backupGlobals enabled + */ + public function testRequestFromGlobalsWithGetProxy() + { + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['REQUEST_URI'] = 'http://example.com/'; + $_SERVER['SERVER_PROTOCOL'] = 'http/1.1'; + $_SERVER['HTTP_HOST'] = 'example.com'; + + $sapi = new SapiHandler(); + $request = $sapi->requestFromGlobals(); + + $this->assertEquals('GET', $request->getMethod()); + $this->assertEquals('http://example.com/', (string) $request->getUri()); + $this->assertEquals('http://example.com/', $request->getRequestTarget()); + $this->assertEquals('1.1', $request->getProtocolVersion()); + $this->assertEquals('example.com', $request->getHeaderLine('Host')); + } + + /** + * @backupGlobals enabled + */ + public function testRequestFromGlobalsWithConnectProxy() + { + $_SERVER['REQUEST_METHOD'] = 'CONNECT'; + $_SERVER['REQUEST_URI'] = 'example.com:443'; + $_SERVER['SERVER_PROTOCOL'] = 'http/1.1'; + $_SERVER['HTTP_HOST'] = 'example.com:443'; + + $sapi = new SapiHandler(); + $request = $sapi->requestFromGlobals(); + + $this->assertEquals('CONNECT', $request->getMethod()); + $this->assertEquals('example.com:443', (string) $request->getUri()); + $this->assertEquals('example.com:443', $request->getRequestTarget()); + $this->assertEquals('1.1', $request->getProtocolVersion()); + $this->assertEquals('example.com:443', $request->getHeaderLine('Host')); + } + public function testSendResponseSendsEmptyResponseWithNoHeadersAndEmptyBodyAndAssignsNoContentTypeAndEmptyContentLength() { if (headers_sent() || !function_exists('xdebug_get_headers')) { diff --git a/tests/acceptance.sh b/tests/acceptance.sh index 0097a8b..fa0d991 100755 --- a/tests/acceptance.sh +++ b/tests/acceptance.sh @@ -1,6 +1,7 @@ #!/bin/bash base=${1:-http://localhost:8080} +baseWithPort=$(php -r 'echo parse_url($argv[1],PHP_URL_PORT) ? $argv[1] : $argv[1] . ":80";' "$base") n=0 match() { @@ -106,4 +107,7 @@ out=$(curl -v $base/headers -H 'Content-Type;' 2>&1); skipif "Server: Apac out=$(curl -v $base/headers -H 'DNT: 1' 2>&1); skipif "Server: nginx" && match "HTTP/.* 200" && match "\"DNT\"" && notmatch "\"Dnt\"" # skip nginx which doesn't report original case (DNT->Dnt) out=$(curl -v $base/headers -H 'V: a' -H 'V: b' 2>&1); skipif "Server: nginx" && skipif -v "Server:" && match "HTTP/.* 200" && match "\"V\": \"a, b\"" # skip nginx (last only) and PHP webserver (first only) +out=$(curl -v --proxy $baseWithPort $base/debug 2>&1); skipif "Server: nginx" && match "HTTP/.* 400" # skip nginx (continues like direct request) +out=$(curl -v --proxy $baseWithPort -p $base/debug 2>&1); skipif "CONNECT aborted" && match "HTTP/.* 400" # skip PHP development server (rejects as "Malformed HTTP request") + echo "OK ($n)" From 723f3ff02d8033482dd7860740551133e01262be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Mon, 27 Sep 2021 11:01:42 +0200 Subject: [PATCH 2/3] Consistently handle `OPTIONS *` request and match empty route --- examples/index.php | 5 +++++ src/SapiHandler.php | 4 ++-- tests/AppTest.php | 29 +++++++++++++++++++++++++++++ tests/RouteHandlerTest.php | 28 ++++++++++++++++++++++++++++ tests/SapiHandlerTest.php | 21 ++++++++++++++++++++- tests/acceptance.sh | 1 + 6 files changed, 85 insertions(+), 3 deletions(-) diff --git a/examples/index.php b/examples/index.php index c5561e6..7256554 100644 --- a/examples/index.php +++ b/examples/index.php @@ -136,4 +136,9 @@ yield null; }); +// OPTIONS * +$app->options('', function () { + return new React\Http\Message\Response(200); +}); + $app->run(); diff --git a/src/SapiHandler.php b/src/SapiHandler.php index 56ea048..c22caad 100644 --- a/src/SapiHandler.php +++ b/src/SapiHandler.php @@ -44,8 +44,8 @@ public function requestFromGlobals(): ServerRequestInterface $target = ($_SERVER['REQUEST_URI'] ?? '/'); $url = $target; - if (($target[0] ?? '/') === '/') { - $url = ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . $url; + if (($target[0] ?? '/') === '/' || $target === '*') { + $url = ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . ($target === '*' ? '' : $target); } $body = file_get_contents('php://input'); diff --git a/tests/AppTest.php b/tests/AppTest.php index 727b1d1..12a71a3 100644 --- a/tests/AppTest.php +++ b/tests/AppTest.php @@ -524,6 +524,35 @@ public function testHandleRequestWithMatchingRouteReturnsResponseFromMatchingRou $this->assertEquals("OK\n", (string) $response->getBody()); } + public function testHandleRequestWithOptionsAsteriskRequestReturnsResponseFromMatchingEmptyRouteHandler() + { + $app = $this->createAppWithoutLogger(); + + $app->options('', function () { + return new Response( + 200, + [ + 'Content-Type' => 'text/html' + ], + "OK\n" + ); + }); + + $request = new ServerRequest('OPTIONS', 'http://localhost'); + $request = $request->withRequestTarget('*'); + + // $response = $app->handleRequest($request); + $ref = new ReflectionMethod($app, 'handleRequest'); + $ref->setAccessible(true); + $response = $ref->invoke($app, $request); + + /** @var ResponseInterface $response */ + $this->assertInstanceOf(ResponseInterface::class, $response); + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals('text/html', $response->getHeaderLine('Content-Type')); + $this->assertEquals("OK\n", (string) $response->getBody()); + } + public function testHandleRequestWithMatchingRouteReturnsPromiseWhichFulfillsWithResponseWhenHandlerReturnsPromiseWhichFulfillsWithResponse() { $app = $this->createAppWithoutLogger(); diff --git a/tests/RouteHandlerTest.php b/tests/RouteHandlerTest.php index 7780f52..8a76495 100644 --- a/tests/RouteHandlerTest.php +++ b/tests/RouteHandlerTest.php @@ -7,6 +7,7 @@ use FrameworkX\RouteHandler; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ResponseInterface; +use React\Http\Message\Response; use React\Http\Message\ServerRequest; class RouteHandlerTest extends TestCase @@ -79,4 +80,31 @@ public function testHandleRequestWithConnectProxyRequestReturnsResponseWithMessa $this->assertStringContainsString("Error 400: Proxy Requests Not Allowed\n", (string) $response->getBody()); $this->assertStringContainsString("

Please check your settings and retry.

\n", (string) $response->getBody()); } + + public function testHandleRequestWithGetRequestReturnsResponseFromMatchingHandler() + { + $request = new ServerRequest('GET', 'http://example.com/'); + $response = new Response(200, [], ''); + + $handler = new RouteHandler(); + $handler->map(['GET'], '/', function () use ($response) { return $response; }); + + $ret = $handler($request); + + $this->assertSame($response, $ret); + } + + public function testHandleRequestWithOptionsAsteriskRequestReturnsResponseFromMatchingEmptyHandler() + { + $request = new ServerRequest('OPTIONS', 'http://example.com'); + $request = $request->withRequestTarget('*'); + $response = new Response(200, [], ''); + + $handler = new RouteHandler(); + $handler->map(['OPTIONS'], '', function () use ($response) { return $response; }); + + $ret = $handler($request); + + $this->assertSame($response, $ret); + } } diff --git a/tests/SapiHandlerTest.php b/tests/SapiHandlerTest.php index 7d45972..cf7879b 100644 --- a/tests/SapiHandlerTest.php +++ b/tests/SapiHandlerTest.php @@ -4,7 +4,6 @@ use FrameworkX\SapiHandler; use PHPUnit\Framework\TestCase; -use React\Http\Message\ServerRequest; use React\Http\Message\Response; use React\Stream\ThroughStream; @@ -79,6 +78,26 @@ public function testRequestFromGlobalsWithGetRequestOverHttps() $this->assertEquals('localhost', $request->getHeaderLine('Host')); } + /** + * @backupGlobals enabled + */ + public function testRequestFromGlobalsWithOptionsAsterisk() + { + $_SERVER['REQUEST_METHOD'] = 'OPTIONS'; + $_SERVER['REQUEST_URI'] = '*'; + $_SERVER['SERVER_PROTOCOL'] = 'http/1.1'; + $_SERVER['HTTP_HOST'] = 'localhost'; + + $sapi = new SapiHandler(); + $request = $sapi->requestFromGlobals(); + + $this->assertEquals('OPTIONS', $request->getMethod()); + $this->assertEquals('http://localhost', (string) $request->getUri()); + $this->assertEquals('*', $request->getRequestTarget()); + $this->assertEquals('1.1', $request->getProtocolVersion()); + $this->assertEquals('localhost', $request->getHeaderLine('Host')); + } + /** * @backupGlobals enabled */ diff --git a/tests/acceptance.sh b/tests/acceptance.sh index fa0d991..fa53416 100755 --- a/tests/acceptance.sh +++ b/tests/acceptance.sh @@ -95,6 +95,7 @@ out=$(curl -v $base/method -X PUT 2>&1); match "HTTP/.* 200" && match "PU out=$(curl -v $base/method -X PATCH 2>&1); match "HTTP/.* 200" && match "PATCH" out=$(curl -v $base/method -X DELETE 2>&1); match "HTTP/.* 200" && match "DELETE" out=$(curl -v $base/method -X OPTIONS 2>&1); match "HTTP/.* 200" && match "OPTIONS" +out=$(curl -v $base -X OPTIONS --request-target "*" 2>&1); skipif "Server: nginx" && match "HTTP/.* 200" # skip nginx (400) out=$(curl -v $base/headers -H 'Accept: text/html' 2>&1); match "HTTP/.* 200" && match "\"Accept\": \"text/html\"" out=$(curl -v $base/headers -d 'name=Alice' 2>&1); match "HTTP/.* 200" && match "\"Content-Type\": \"application/x-www-form-urlencoded\"" && match "\"Content-Length\": \"10\"" From fa3eba9bb035c9c62d1802545c863b48eb156fdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sat, 25 Sep 2021 14:56:39 +0200 Subject: [PATCH 3/3] Properly route requests that contain URLs in their path --- src/RouteHandler.php | 2 +- tests/RouteHandlerTest.php | 13 +++++++++++++ tests/acceptance.sh | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/RouteHandler.php b/src/RouteHandler.php index 6aff15b..155b9df 100644 --- a/src/RouteHandler.php +++ b/src/RouteHandler.php @@ -45,7 +45,7 @@ public function map(array $methods, string $route, callable $handler, callable . */ public function __invoke(ServerRequestInterface $request) { - if (\strpos($request->getRequestTarget(), '://') !== false || $request->getMethod() === 'CONNECT') { + if ($request->getRequestTarget()[0] !== '/' && $request->getRequestTarget() !== '*') { return $this->errorHandler->requestProxyUnsupported($request); } diff --git a/tests/RouteHandlerTest.php b/tests/RouteHandlerTest.php index 8a76495..cec4039 100644 --- a/tests/RouteHandlerTest.php +++ b/tests/RouteHandlerTest.php @@ -94,6 +94,19 @@ public function testHandleRequestWithGetRequestReturnsResponseFromMatchingHandle $this->assertSame($response, $ret); } + public function testHandleRequestWithGetRequestWithHttpUrlInPathReturnsResponseFromMatchingHandler() + { + $request = new ServerRequest('GET', 'http://example.com/http://localhost/'); + $response = new Response(200, [], ''); + + $handler = new RouteHandler(); + $handler->map(['GET'], '/http://localhost/', function () use ($response) { return $response; }); + + $ret = $handler($request); + + $this->assertSame($response, $ret); + } + public function testHandleRequestWithOptionsAsteriskRequestReturnsResponseFromMatchingEmptyHandler() { $request = new ServerRequest('OPTIONS', 'http://example.com'); diff --git a/tests/acceptance.sh b/tests/acceptance.sh index fa53416..4607a25 100755 --- a/tests/acceptance.sh +++ b/tests/acceptance.sh @@ -36,6 +36,7 @@ out=$(curl -v $base/uri/Wham%21 2>&1); match "HTTP/.* 200" && m out=$(curl -v $base/uri/AC%2FDC 2>&1); skipif "HTTP/.* 404" && match "HTTP/.* 200" && match "$base/uri/AC%2FDC" # skip Apache (404 unless `AllowEncodedSlashes NoDecode`) out=$(curl -v $base/uri/bin%00ary 2>&1); skipif "HTTP/.* 40[04]" && match "HTTP/.* 200" && match "$base/uri/bin%00ary" # skip nginx (400) and Apache (404) out=$(curl -v $base/uri/AC/DC 2>&1); match "HTTP/.* 200" && match "$base/uri/AC/DC" +out=$(curl -v $base/uri/http://example.com:8080/ 2>&1); match "HTTP/.* 200" && match "$base/uri/http://example.com:8080/" out=$(curl -v $base/uri? 2>&1); match "HTTP/.* 200" && match "$base/uri" # trailing "?" not reported for empty query string out=$(curl -v $base/uri?query 2>&1); match "HTTP/.* 200" && match "$base/uri?query" out=$(curl -v $base/uri?q=a 2>&1); match "HTTP/.* 200" && match "$base/uri?q=a"