From d8fe78449cbd76d0d54560efaefd10c98f1235a4 Mon Sep 17 00:00:00 2001 From: n0nag0n Date: Mon, 10 Aug 2026 23:29:08 -0600 Subject: [PATCH] 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. --- flight/Engine.php | 20 ++++- tests/EngineTest.php | 173 ++++++++++++++++++++++++++++++++++++++++++ tests/RequestTest.php | 47 ++++++++++++ 3 files changed, 238 insertions(+), 2 deletions(-) diff --git a/flight/Engine.php b/flight/Engine.php index db00d5b..880decf 100644 --- 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; diff --git a/tests/EngineTest.php b/tests/EngineTest.php index e2d8471..7c8226e 100644 --- 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); + } } diff --git a/tests/RequestTest.php b/tests/RequestTest.php index acc6e18..7954228 100644 --- 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';