Fix flight.allow_method_override not disabling method override (GHSA-vr9m-jx6f-hhj6)

Assign Request::$allowMethodOverride on the class before any Request is built, so the constructor does not cache an overridden verb while the static still defaults to true. When override is disabled and an override header or _method field is present, recompute the cached request method so early Flight::request() access cannot keep a spoofed DELETE/PUT/PATCH. Add regression tests for the Engine start path, early request construction, _method, and intentional manual method assignment without override input.
1 week ago · d8fe78449c
parent ee03274498
commit d8fe78449c
3 changed files with 238 additions and 2 deletions
--- a/flight/Engine.php
+++ b/flight/Engine.php
@ -227,8 +227,24 @@ class Engine
            // which causes a lot of problems. This will be removed
            // in v4
            $self->response()->v2_output_buffering = $this->get('flight.v2.output_buffering');
-            // Propagate method override setting to Request
-            $self->request()::$allowMethodOverride = (bool) $self->get('flight.allow_method_override');
+
+            // Propagate method override setting to Request.
+            // Assign the static on the class first — do not call request() before this,
+            // or Request's constructor caches the method while the flag is still the default (true).
+            Request::$allowMethodOverride = (bool) $self->get('flight.allow_method_override');
+
+            // If a Request was already built earlier (common: apps touch request() before start)
+            // while override was still enabled, refresh the cached verb when override is off
+            // and an override input is present. When no override input exists, leave any
+            // intentional manual method assignment alone.
+            if (Request::$allowMethodOverride === false) {
+                $hasOverrideInput = Request::getVar('HTTP_X_HTTP_METHOD_OVERRIDE') !== ''
+                    || isset($_REQUEST['_method']);
+                if ($hasOverrideInput === true) {
+                    $request = $self->request();
+                    $request->method = Request::getMethod();
+                }
+            }
        });

        $this->initialized = true;
--- a/tests/EngineTest.php
+++ b/tests/EngineTest.php
@ -23,11 +23,18 @@ class EngineTest extends TestCase
    public function setUp(): void
    {
        $_SERVER = [];
+        $_REQUEST = [];
+        $_GET = [];
+        $_POST = [];
+        // Static flag leaks across tests (and across Engine instances).
+        Request::$allowMethodOverride = true;
    }

    public function tearDown(): void
    {
        $_SERVER = [];
+        $_REQUEST = [];
+        Request::$allowMethodOverride = true;
    }

    public function testInitBeforeStart(): void
@ -1207,4 +1214,170 @@ class EngineTest extends TestCase
        $this->expectExceptionMessage("/path/to/nowhere cannot be found.");
        $engine->download('/path/to/nowhere');
    }
+
+    /**
+     * Regression for GHSA method-override opt-out: setting the flag to false must
+     * prevent X-HTTP-Method-Override from selecting a different route.
+     *
+     * The original mitigation assigned Request::$allowMethodOverride via
+     * $self->request()::$allowMethodOverride, which built Request (and cached the
+     * overridden verb) before the static was set.
+     */
+    public function testAllowMethodOverrideFalseBlocksHeaderOverrideOnStart(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['REQUEST_URI'] = '/test';
+        $_SERVER['SCRIPT_NAME'] = '/index.php';
+        $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] = 'DELETE';
+        $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
+        $_SERVER['SERVER_NAME'] = 'localhost';
+        $_SERVER['HTTP_HOST'] = 'localhost';
+
+        $engine = new Engine();
+        $engine->set('flight.allow_method_override', false);
+
+        $hit = null;
+        $engine->route('GET /test', function () use (&$hit) {
+            $hit = 'get';
+            echo 'get';
+        });
+        $engine->route('DELETE /test', function () use (&$hit) {
+            $hit = 'delete';
+            echo 'delete';
+        });
+
+        $this->expectOutputString('get');
+        $engine->start();
+
+        $this->assertSame('get', $hit);
+        $this->assertFalse(Request::$allowMethodOverride);
+        $this->assertSame('GET', $engine->request()->method);
+    }
+
+    public function testAllowMethodOverrideFalseBlocksPostMethodFieldOnStart(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_SERVER['REQUEST_URI'] = '/test';
+        $_SERVER['SCRIPT_NAME'] = '/index.php';
+        $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
+        $_SERVER['SERVER_NAME'] = 'localhost';
+        $_SERVER['HTTP_HOST'] = 'localhost';
+        $_REQUEST['_method'] = 'PUT';
+
+        $engine = new Engine();
+        $engine->set('flight.allow_method_override', false);
+
+        $hit = null;
+        $engine->route('POST /test', function () use (&$hit) {
+            $hit = 'post';
+            echo 'post';
+        });
+        $engine->route('PUT /test', function () use (&$hit) {
+            $hit = 'put';
+            echo 'put';
+        });
+
+        $this->expectOutputString('post');
+        $engine->start();
+
+        $this->assertSame('post', $hit);
+        $this->assertSame('POST', $engine->request()->method);
+    }
+
+    public function testAllowMethodOverrideTrueStillHonorsHeaderOnStart(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['REQUEST_URI'] = '/test';
+        $_SERVER['SCRIPT_NAME'] = '/index.php';
+        $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] = 'DELETE';
+        $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
+        $_SERVER['SERVER_NAME'] = 'localhost';
+        $_SERVER['HTTP_HOST'] = 'localhost';
+
+        $engine = new Engine();
+        // default is true; set explicitly for clarity
+        $engine->set('flight.allow_method_override', true);
+
+        $hit = null;
+        $engine->route('GET /test', function () use (&$hit) {
+            $hit = 'get';
+            echo 'get';
+        });
+        $engine->route('DELETE /test', function () use (&$hit) {
+            $hit = 'delete';
+            echo 'delete';
+        });
+
+        $this->expectOutputString('delete');
+        $engine->start();
+
+        $this->assertSame('delete', $hit);
+        $this->assertTrue(Request::$allowMethodOverride);
+        $this->assertSame('DELETE', $engine->request()->method);
+    }
+
+    /**
+     * App code often touches request() before start() (set url, inspect headers, etc.).
+     * The flag must still win even when Request was constructed early under the default.
+     */
+    public function testAllowMethodOverrideFalseRefreshesMethodIfRequestBuiltEarly(): void
+    {
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['REQUEST_URI'] = '/test';
+        $_SERVER['SCRIPT_NAME'] = '/index.php';
+        $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] = 'DELETE';
+        $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
+        $_SERVER['SERVER_NAME'] = 'localhost';
+        $_SERVER['HTTP_HOST'] = 'localhost';
+
+        $engine = new Engine();
+        $engine->set('flight.allow_method_override', false);
+
+        // Construct Request while static is still the default (true)
+        $this->assertTrue(Request::$allowMethodOverride);
+        $early = $engine->request();
+        $this->assertSame('DELETE', $early->method, 'pre-start construction still sees default override=on');
+
+        $hit = null;
+        $engine->route('GET /test', function () use (&$hit) {
+            $hit = 'get';
+            echo 'get';
+        });
+        $engine->route('DELETE /test', function () use (&$hit) {
+            $hit = 'delete';
+            echo 'delete';
+        });
+
+        $this->expectOutputString('get');
+        $engine->start();
+
+        $this->assertSame('get', $hit);
+        $this->assertFalse(Request::$allowMethodOverride);
+        $this->assertSame('GET', $engine->request()->method);
+        $this->assertSame($early, $engine->request(), 'same Request instance is refreshed, not replaced');
+    }
+
+    public function testAllowMethodOverrideFalseDoesNotClobberManualMethodWithoutOverrideInput(): void
+    {
+        // No X-HTTP-Method-Override / _method — manual method assignment must survive start().
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['REQUEST_URI'] = '/someRoute';
+        $_SERVER['SCRIPT_NAME'] = '/index.php';
+        $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
+        $_SERVER['SERVER_NAME'] = 'localhost';
+        $_SERVER['HTTP_HOST'] = 'localhost';
+
+        $engine = new Engine();
+        $engine->set('flight.allow_method_override', false);
+        $engine->route('GET /someRoute', function () {
+            echo 'i ran';
+        }, true);
+        $engine->request()->method = 'HEAD';
+        $engine->request()->url = '/someRoute';
+
+        $this->expectOutputString('');
+        $engine->start();
+
+        $this->assertSame('HEAD', $engine->request()->method);
+    }
 }
--- a/tests/RequestTest.php
+++ b/tests/RequestTest.php
@ -31,11 +31,15 @@ class RequestTest extends TestCase
        $_COOKIE = [];
        $_FILES = [];

+        // Static flag leaks across tests; always restore the framework default.
+        Request::$allowMethodOverride = true;
+
        $this->request = new Request();
    }

    protected function tearDown(): void
    {
+        Request::$allowMethodOverride = true;
        unset($_REQUEST);
        unset($_SERVER);
    }
@ -127,6 +131,49 @@ class RequestTest extends TestCase
        $this->assertEquals('PUT', $request->method);
    }

+    public function testMethodOverrideDisabledIgnoresHeader(): void
+    {
+        Request::$allowMethodOverride = false;
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] = 'DELETE';
+
+        $request = new Request();
+
+        $this->assertSame('GET', $request->method);
+        $this->assertSame('GET', Request::getMethod());
+    }
+
+    public function testMethodOverrideDisabledIgnoresPostField(): void
+    {
+        Request::$allowMethodOverride = false;
+        $_SERVER['REQUEST_METHOD'] = 'POST';
+        $_REQUEST['_method'] = 'PUT';
+
+        $request = new Request();
+
+        $this->assertSame('POST', $request->method);
+        $this->assertSame('POST', Request::getMethod());
+    }
+
+    public function testMethodOverrideFlagMustBeSetBeforeConstruction(): void
+    {
+        // Documents the caching behavior: flipping the static after construct
+        // does not rewrite the already-cached $request->method.
+        $_SERVER['REQUEST_METHOD'] = 'GET';
+        $_SERVER['HTTP_X_HTTP_METHOD_OVERRIDE'] = 'DELETE';
+
+        Request::$allowMethodOverride = true;
+        $request = new Request();
+        $this->assertSame('DELETE', $request->method);
+
+        Request::$allowMethodOverride = false;
+        $this->assertSame('DELETE', $request->method, 'cached method is not auto-refreshed');
+        $this->assertSame('GET', Request::getMethod(), 'getMethod() respects the new flag');
+
+        $request->method = Request::getMethod();
+        $this->assertSame('GET', $request->method);
+    }
+
    public function testHttps(): void
    {
        $_SERVER['HTTPS'] = 'on';