diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 5139faf04..ad44687f2 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -18,9 +18,6 @@ use SilverStripe\ORM\ArrayLib; * The intention is that a single HTTPRequest object can be passed from one object to another, each object calling * match() to get the information that they need out of the URL. This is generally handled by * {@link RequestHandler::handleRequest()}. - * - * @todo Accept X_HTTP_METHOD_OVERRIDE http header and $_REQUEST['_method'] to override request types (useful for - * webclients not supporting PUT and DELETE) */ class HTTPRequest implements ArrayAccess { @@ -156,7 +153,7 @@ class HTTPRequest implements ArrayAccess */ public function __construct($httpMethod, $url, $getVars = [], $postVars = [], $body = null) { - $this->httpMethod = strtoupper(self::detect_method($httpMethod, $postVars)); + $this->httpMethod = strtoupper($httpMethod); $this->setUrl($url); $this->getVars = (array) $getVars; $this->postVars = (array) $postVars; @@ -853,6 +850,21 @@ class HTTPRequest implements ArrayAccess return $this->httpMethod; } + /** + * Explicitly set the HTTP method for this request. + * @param string $method + * @return $this + */ + public function setHttpMethod($method) + { + if (!self::isValidHttpMethod($method)) { + user_error('HTTPRequest::setHttpMethod: Invalid HTTP method', E_USER_ERROR); + } + + $this->httpMethod = strtoupper($method); + return $this; + } + /** * Return the URL scheme (e.g. "http" or "https"). * Equivalent to PSR-7 getUri()->getScheme() @@ -878,25 +890,28 @@ class HTTPRequest implements ArrayAccess } /** - * Gets the "real" HTTP method for a request. - * - * Used to work around browser limitations of form - * submissions to GET and POST, by overriding the HTTP method - * with a POST parameter called "_method" for PUT, DELETE, HEAD. - * Using GET for the "_method" override is not supported, - * as GET should never carry out state changes. - * Alternatively you can use a custom HTTP header 'X-HTTP-Method-Override' - * to override the original method. - * The '_method' POST parameter overrules the custom HTTP header. + * @param string $method + * @return bool + */ + private static function isValidHttpMethod($method) + { + return in_array(strtoupper($method), ['GET','POST','PUT','DELETE','HEAD']); + } + + /** + * Gets the "real" HTTP method for a request. This method is no longer used to mitigate the risk of web cache + * poisoning. * + * @see https://www.silverstripe.org/download/security-releases/CVE-2019-19326 * @param string $origMethod Original HTTP method from the browser request * @param array $postVars * @return string HTTP method (all uppercase) + * @deprecated 4.4.7 */ public static function detect_method($origMethod, $postVars) { if (isset($postVars['_method'])) { - if (!in_array(strtoupper($postVars['_method']), ['GET','POST','PUT','DELETE','HEAD'])) { + if (!self::isValidHttpMethod($postVars['_method'])) { user_error('HTTPRequest::detect_method(): Invalid "_method" parameter', E_USER_ERROR); } return strtoupper($postVars['_method']); diff --git a/src/Control/HTTPRequestBuilder.php b/src/Control/HTTPRequestBuilder.php index 3ddef06d6..443ae38c6 100644 --- a/src/Control/HTTPRequestBuilder.php +++ b/src/Control/HTTPRequestBuilder.php @@ -135,16 +135,6 @@ class HTTPRequestBuilder */ public static function cleanEnvironment(array $variables) { - // IIS will sometimes generate this. - if (!empty($variables['_SERVER']['HTTP_X_ORIGINAL_URL'])) { - $variables['_SERVER']['REQUEST_URI'] = $variables['_SERVER']['HTTP_X_ORIGINAL_URL']; - } - - // Override REQUEST_METHOD - if (isset($variables['_SERVER']['X-HTTP-Method-Override'])) { - $variables['_SERVER']['REQUEST_METHOD'] = $variables['_SERVER']['X-HTTP-Method-Override']; - } - // Merge $_FILES into $_POST $variables['_POST'] = array_merge((array)$variables['_POST'], (array)$variables['_FILES']); diff --git a/tests/php/Control/HTTPRequestTest.php b/tests/php/Control/HTTPRequestTest.php index 4a56b8f04..51ea76fa3 100644 --- a/tests/php/Control/HTTPRequestTest.php +++ b/tests/php/Control/HTTPRequestTest.php @@ -98,9 +98,15 @@ class HTTPRequestTest extends SapphireTest [], ['_method' => 'DELETE'] ); + $this->assertTrue( + $request->isPOST(), + '_method override is no longer honored' + ); + + $this->assertFalse( $request->isDELETE(), - 'POST with valid method override to DELETE' + 'DELETE _method override is not honored' ); $request = new HTTPRequest( @@ -109,9 +115,9 @@ class HTTPRequestTest extends SapphireTest [], ['_method' => 'put'] ); - $this->assertTrue( + $this->assertFalse( $request->isPUT(), - 'POST with valid method override to PUT' + 'PUT _method override is not honored' ); $request = new HTTPRequest( @@ -120,31 +126,78 @@ class HTTPRequestTest extends SapphireTest [], ['_method' => 'head'] ); - $this->assertTrue( + $this->assertFalse( $request->isHEAD(), - 'POST with valid method override to HEAD ' + 'HEAD _method override is not honored' ); + } - $request = new HTTPRequest( - 'POST', - 'admin/crm', - [], - ['_method' => 'head'] - ); - $this->assertTrue( - $request->isHEAD(), - 'POST with valid method override to HEAD' - ); + public function detectMethodDataProvider() + { + return [ + 'Plain POST request' => ['POST', [], 'POST'], + 'Plain GET request' => ['GET', [], 'GET'], + 'Plain DELETE request' => ['DELETE', [], 'DELETE'], + 'Plain PUT request' => ['PUT', [], 'PUT'], + 'Plain HEAD request' => ['HEAD', [], 'HEAD'], - $request = new HTTPRequest( - 'POST', - 'admin/crm', - ['_method' => 'head'] - ); - $this->assertTrue( - $request->isPOST(), - 'POST with invalid method override by GET parameters to HEAD' - ); + 'Request with GET method override' => ['POST', ['_method' => 'GET'], 'GET'], + 'Request with HEAD method override' => ['POST', ['_method' => 'HEAD'], 'HEAD'], + 'Request with DELETE method override' => ['POST', ['_method' => 'DELETE'], 'DELETE'], + 'Request with PUT method override' => ['POST', ['_method' => 'PUT'], 'PUT'], + 'Request with POST method override' => ['POST', ['_method' => 'POST'], 'POST'], + + 'Request with mixed case method override' => ['POST', ['_method' => 'gEt'], 'GET'] + ]; + } + + /** + * @dataProvider detectMethodDataProvider + */ + public function testDetectMethod($realMethod, $post, $expected) + { + $actual = HTTPRequest::detect_method($realMethod, $post); + $this->assertEquals($expected, $actual); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testBadDetectMethod() + { + HTTPRequest::detect_method('POST', ['_method' => 'Boom']); + } + + public function setHttpMethodDataProvider() + { + return [ + 'POST request' => ['POST','POST'], + 'GET request' => ['GET', 'GET'], + 'DELETE request' => ['DELETE', 'DELETE'], + 'PUT request' => ['PUT', 'PUT'], + 'HEAD request' => ['HEAD', 'HEAD'], + 'Mixed case POST' => ['gEt', 'GET'], + ]; + } + + /** + * @dataProvider setHttpMethodDataProvider + */ + public function testSetHttpMethod($method, $expected) + { + $request = new HTTPRequest('GET', '/hello'); + $returnedRequest = $request->setHttpMethod($method); + $this->assertEquals($expected, $request->httpMethod()); + $this->assertEquals($request, $returnedRequest); + } + + /** + * @expectedException PHPUnit_Framework_Error + */ + public function testBadSetHttpMethod() + { + $request = new HTTPRequest('GET', '/hello'); + $request->setHttpMethod('boom'); } public function testRequestVars()