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';