[CVE-2019-19326] Stop honouring X-HTTP-Method-Override header, X-Original-Url header and _method POST variable. Add SS_HTTPRequest::setHttpMethod()

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.
+     * @param string $method
-     *
+     * @return bool
-     * Used to work around browser limitations of form
+     */
-     * submissions to GET and POST, by overriding the HTTP method
+    private static function isValidHttpMethod($method)
-     * with a POST parameter called "_method" for PUT, DELETE, HEAD.
+    {
-     * Using GET for the "_method" override is not supported,
+        return in_array(strtoupper($method), ['GET','POST','PUT','DELETE','HEAD']);
-     * 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.
+     * 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']);

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

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(
+    public function detectMethodDataProvider()
-            'POST',
+    {
-            'admin/crm',
+        return [
-            [],
+            'Plain POST request' => ['POST', [], 'POST'],
-            ['_method' => 'head']
+            'Plain GET request' => ['GET', [], 'GET'],
-        );
+            'Plain DELETE request' => ['DELETE', [], 'DELETE'],
-        $this->assertTrue(
+            'Plain PUT request' => ['PUT', [], 'PUT'],
-            $request->isHEAD(),
+            'Plain HEAD request' => ['HEAD', [], 'HEAD'],
-            'POST with valid method override to HEAD'
-        );
 
-        $request = new HTTPRequest(
+            'Request with GET method override' => ['POST', ['_method' => 'GET'], 'GET'],
-            'POST',
+            'Request with HEAD method override' => ['POST', ['_method' => 'HEAD'], 'HEAD'],
-            'admin/crm',
+            'Request with DELETE method override' => ['POST', ['_method' => 'DELETE'], 'DELETE'],
-            ['_method' => 'head']
+            'Request with PUT method override' => ['POST', ['_method' => 'PUT'], 'PUT'],
-        );
+            'Request with POST method override' => ['POST', ['_method' => 'POST'], 'POST'],
-        $this->assertTrue(
+
-            $request->isPOST(),
+            'Request with mixed case method override' => ['POST', ['_method' => 'gEt'], 'GET']
-            'POST with invalid method override by GET parameters to HEAD'
+        ];
-        );
+    }
+
+    /**
+     * @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()