[SS-2018-007] Add CSRF protection (#2)

.travis.yml

@@ -23,13 +23,11 @@ before_script:
   - phpenv config-rm xdebug.ini
 
 # Install composer dependencies
-  - export PATH=~/.composer/vendor/bin:$PATH
   - composer validate
   - composer install --prefer-dist
   - composer require --prefer-dist --no-update silverstripe/recipe-core:4.2.x-dev silverstripe/versioned:1.2.x-dev silverstripe/assets:1.2.x-dev --prefer-dist
   - composer update
   - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.1.x-dev --prefer-dist; fi
-  - if [[ $PHPCS_TEST ]]; then composer global require squizlabs/php_codesniffer:^3 --prefer-dist --no-interaction --no-progress --no-suggest -o; fi
 
 script:
   - if [[ $PHPUNIT_TEST ]]; then vendor/bin/phpunit; fi

README.md

@@ -65,9 +65,11 @@ composer require silverstripe/graphql
    - [HTTP basic authentication](#http-basic-authentication)
      - [In GraphiQL](#in-graphiql)
    - [Defining your own authenticators](#defining-your-own-authenticators)
+ - [CSRF tokens (required for mutations)](#csrf-tokens-required-for-mutations)
  - [Cross-Origin Resource Sharing (CORS)](#cross-origin-resource-sharing-cors)
    - [Sample Custom CORS Config](#sample-custom-cors-config)
  - [Schema introspection](#schema-introspection)
+ - [Strict HTTP Method Checking](#strict-http-method-checking)
  - [TODO](#todo)
 
 
@@ -1973,7 +1975,30 @@ SilverStripe\GraphQL\Auth\Handler:
     - class: SilverStripe\GraphQL\Auth\BasicAuthAuthenticator
       priority: 10
 ```
+## CSRF tokens (required for mutations)
 
+Even if your graphql endpoints are behind authentication, it is still possible for unauthorised
+users to access that endpoint through a [CSRF exploitation](https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)). This involves
+forcing an already authenticated user to access an HTTP resource unknowingly (e.g. through a fake image), thereby hijacking the user's
+session.
+
+In the absence of a token-based authentication system, like OAuth, the best countermeasure to this
+is the use of a CSRF token for any requests that destroy or mutate data.
+
+By default, this module comes with a `CSRFMiddleware` implementation that forces all mutations to check 
+for the presence of a CSRF token in the request. That token must be applied to a header named` X-CSRF-TOKEN`.
+
+In SilverStripe, CSRF tokens are most commonly stored in the session as `SecurityID`, or accessed through
+the `SecurityToken` API, using `SecurityToken::inst()->getValue()`.
+
+Queries do not require CSRF tokens.
+
+```yaml
+  SilverStripe\GraphQL\Manager:
+    properties:
+      Middlewares:
+        CSRFMiddleware: false
+```
 ## Cross-Origin Resource Sharing (CORS)
 
 By default [CORS](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS) is disabled in the GraphQL Server. This can be easily enabled via YAML:
@@ -2114,6 +2139,20 @@ SilverStripe\GraphQL\Controller:
     Max-Age:  600  # 600 seconds = 10 minutes.
 ```
 
+## Strict HTTP Method Checking
+
+According to GraphQL best practices, mutations should be done over `POST`, while queries have the option
+to use either `GET` or `POST`. By default, this module enforces the `POST` request method for all mutations.
+
+To disable that requirement, you can remove the `HTTPMethodMiddleware` from your `Manager` implementation.
+
+```yaml
+  SilverStripe\GraphQL\Manager:
+    properties:
+      Middlewares:
+        HTTPMethodMiddleware: false
+```
+
 ## TODO
 
  * Permission checks

_config/config.yml

@@ -7,6 +7,15 @@ SilverStripe\Core\Injector\Injector:
     class: SilverStripe\GraphQL\Scaffolding\Util\StringTypeParser
   SilverStripe\GraphQL\Scaffolding\Interfaces\TypeParserInterface.array:
     class: SilverStripe\GraphQL\Scaffolding\Util\ArrayTypeParser
+  SilverStripe\GraphQL\Middleware\QueryMiddleware.csrf:
+    class: SilverStripe\GraphQL\Middleware\CSRFMiddleware
+  SilverStripe\GraphQL\Middleware\QueryMiddleware.httpMethod:
+    class: SilverStripe\GraphQL\Middleware\HTTPMethodMiddleware
+  SilverStripe\GraphQL\Manager:
+    properties:
+      Middlewares:
+        CSRFMiddleware: '%$SilverStripe\GraphQL\Middleware\QueryMiddleware.csrf'
+        HTTPMethodMiddleware: '%$SilverStripe\GraphQL\Middleware\QueryMiddleware.httpMethod'
 
 # Assign each DBField subclass with an associated internal type
 SilverStripe\ORM\FieldType\DBField:

composer.json

@@ -8,7 +8,9 @@
         "webonyx/graphql-php": "~0.8.0"
     },
     "require-dev": {
-        "phpunit/PHPUnit": "^5.7"
+        "phpunit/PHPUnit": "^5.7",
+        "phpunit/PHPUnit": "^5.7",
+        "squizlabs/php_codesniffer": "^3.0"
     },
     "extra": [],
     "autoload": {
@@ -25,8 +27,8 @@
         "process-timeout": 600
     },
     "scripts": {
-        "lint": "phpcs src/ tests/",
-        "lint-clean": "phpcbf src/ tests/"
+        "lint": "vendor/bin/phpcs src/ tests/",
+        "lint-clean": "vendor/bin/phpcbf src/ tests/"
     },
     "minimum-stability": "dev",
     "prefer-stable": true,

src/Controller.php

@@ -14,6 +14,7 @@
 use SilverStripe\GraphQL\Scaffolding\StaticSchema;
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Permission;
+use SilverStripe\Security\SecurityToken;
 use SilverStripe\Versioned\Versioned;
 
 /**
@@ -68,7 +69,6 @@ public function index(HTTPRequest $request)
         if ($stage && in_array($stage, [Versioned::DRAFT, Versioned::LIVE])) {
             Versioned::set_stage($stage);
         }
-
         // Check for a possible CORS preflight request and handle if necessary
         // Refer issue 66:  https://github.com/silverstripe/silverstripe-graphql/issues/66
         if ($request->httpMethod() === 'OPTIONS') {
@@ -78,6 +78,14 @@ public function index(HTTPRequest $request)
         // Main query handling
         try {
             $manager = $this->getManager();
+            $manager->addContext('token', $this->getToken());
+            $method = null;
+            if ($request->isGET()) {
+                $method = 'GET';
+            } elseif ($request->isPOST()) {
+                $method = 'POST';
+            }
+            $manager->addContext('httpMethod', $method);
 
             // Check and validate user for this request
             $member = $this->getRequestUser($request);
@@ -165,6 +173,14 @@ public function getAuthHandler()
         return new Handler;
     }
 
+    /**
+     * @return string
+     */
+    public function getToken()
+    {
+        return $this->getRequest()->getHeader('X-CSRF-TOKEN');
+    }
+
     /**
      * Process the CORS config options and add the appropriate headers to the response.
      *

src/Manager.php

@@ -64,12 +64,16 @@ class Manager
      */
     protected $member;
 
-
     /**
      * @var QueryMiddleware[]
      */
     protected $middlewares = [];
 
+    /**
+     * @var array
+     */
+    protected $extraContext = [];
+
     /**
      * @return QueryMiddleware[]
      */
@@ -112,8 +116,10 @@ protected function callMiddleware(Schema $schema, $query, $context, $params, cal
     {
         // Reverse middlewares
         $next = $last;
+        // Filter out any middlewares that are set to `false`, e.g. via config
+        $middlewares = array_reverse(array_filter($this->getMiddlewares()));
         /** @var QueryMiddleware $middleware */
-        foreach (array_reverse($this->getMiddlewares()) as $middleware) {
+        foreach ($middlewares as $middleware) {
             $next = function ($schema, $query, $context, $params) use ($middleware, $next) {
                 return $middleware->process($schema, $query, $context, $params, $next);
             };
@@ -434,12 +440,41 @@ public function getMember()
      * @return array
      */
     protected function getContext()
+    {
+        return array_merge(
+            $this->getContextDefaults(),
+            $this->extraContext
+        );
+    }
+
+    /**
+     * @return array
+     */
+    protected function getContextDefaults()
     {
         return [
-            'currentUser' => $this->getMember()
+            'currentUser' => $this->getMember(),
         ];
     }
 
+    /**
+     * @param string $key
+     * @param any $value
+     * @return $this
+     */
+    public function addContext($key, $value)
+    {
+        if (!is_string($key)) {
+            throw new InvalidArgumentException(sprintf(
+                'Context key must be a string. Got %s',
+                gettype($key)
+            ));
+        }
+        $this->extraContext[$key] = $value;
+
+        return $this;
+    }
+
     /**
      * Serialise a Graphql result object for output
      *

src/Middleware/CSRFMiddleware.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace SilverStripe\GraphQL\Middleware;
+
+use GraphQL\Schema;
+use SilverStripe\GraphQL\Middleware\QueryMiddleware;
+use SilverStripe\Security\SecurityToken;
+use Exception;
+
+class CSRFMiddleware implements QueryMiddleware
+{
+    public function process(Schema $schema, $query, $context, $params, callable $next)
+    {
+        if (preg_match('/^\s*mutation/', $query)) {
+            if (empty($context['token'])) {
+                throw new Exception('Mutations must provide a CSRF token in the X-CSRF-TOKEN header');
+            }
+            $token = $context['token'];
+
+            if (!SecurityToken::inst()->check($token)) {
+                throw new Exception('Invalid CSRF token');
+            }
+        }
+
+        return $next($schema, $query, $context, $params);
+    }
+}

src/Middleware/HTTPMethodMiddleware.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace SilverStripe\GraphQL\Middleware;
+
+use GraphQL\Schema;
+use SilverStripe\GraphQL\Middleware\QueryMiddleware;
+use Exception;
+
+class HTTPMethodMiddleware implements QueryMiddleware
+{
+    public function process(Schema $schema, $query, $context, $params, callable $next)
+    {
+        $isGET = false;
+        $isPOST = false;
+        if (isset($context['httpMethod'])) {
+            $isGET = $context['httpMethod'] === 'GET';
+            $isPOST = $context['httpMethod'] === 'POST';
+        }
+
+        if (!$isGET && !$isPOST) {
+            throw new Exception('Request method must be POST or GET');
+        }
+
+        if (preg_match('/^\s*mutation/', $query)) {
+            if (!$isPOST) {
+                throw new Exception('Mutations must use the POST request method');
+            }
+        }
+
+        return $next($schema, $query, $context, $params);
+    }
+}