Merge pull request #46 from clue/route

Consistently reject proxy requests and handle `OPTIONS *` requests also when run behind web server
clue · Sep 28, 2021 · 18a0158 · 18a0158
2 parents 1371907 + fa3eba9
commit 18a0158
Show file tree

Hide file tree

Showing 7 changed files with 190 additions and 3 deletions.
diff --git 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/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/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] ?? '/') === '/' || $target === '*') {
+            $url = ($_SERVER['HTTPS'] ?? null === 'on' ? 'https://' : 'http://') . ($host ?? 'localhost') . ($target === '*' ? '' : $target);
+        }
+
         $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/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
@@ -6,6 +6,9 @@
 use FrameworkX\MiddlewareHandler;
 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
 {
@@ -41,4 +44,80 @@ 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("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
+        $this->assertStringContainsString("<title>Error 400: Proxy Requests Not Allowed</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Please check your settings and retry.</p>\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("<!DOCTYPE html>\n<html>%a</html>\n", (string) $response->getBody());
+
+        $this->assertStringContainsString("<title>Error 400: Proxy Requests Not Allowed</title>\n", (string) $response->getBody());
+        $this->assertStringContainsString("<p>Please check your settings and retry.</p>\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 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');
+        $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
@@ -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,66 @@ 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
+     */
+    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
@@ -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() {
@@ -35,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"
@@ -94,6 +96,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\""
@@ -106,4 +109,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)"