Merge pull request #41 from qld-gov-au/develop

Develop to master
qld-gov-au · Oct 12, 2023 · e6ae5cc · e6ae5cc
2 parents bee9587 + f6f2ac3
commit e6ae5cc
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 9 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -6,10 +6,10 @@ on: [push]
 
 jobs:
   lint:
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions/setup-python@v2
+      - uses: actions/checkout@v3
+      - uses: actions/setup-python@v4
         with:
           python-version: '3.x'
       - name: Install requirements
@@ -25,12 +25,12 @@ jobs:
       fail-fast: true
 
     name: CKAN ${{ matrix.ckan-version }}
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-latest
     container:
       image: openknowledge/ckan-dev:${{ matrix.ckan-version }}
 
     steps:
-    - uses: actions/checkout@v2
+    - uses: actions/checkout@v3
 
     - name: Install requirements
       run: |

diff --git a/README.md b/README.md
@@ -59,6 +59,11 @@ replace the FriendlyForm plugin in `who.ini` with a token-aware version:
     use = ckanext.csrf_filter.token_protected_friendlyform:TokenProtectedFriendlyFormPlugin
     ```
 
+1. Optional: To set token cookie [SameSite attribute](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value), set ``ckanext.csrf_filter.same_site`` setting in your CKAN config file. By default, the SameSite attribute will be ``None``. Supported values:
+    * Strict
+    * Lax
+    * None
+
 1. Restart CKAN. Eg if you've deployed CKAN with Apache on Ubuntu:
 
     ```
@@ -91,6 +96,19 @@ Optional
     # Default 10 minutes.
     ckanext.csrf_filter.token_rotation_minutes = 10
 
+    # Exempts given regex matches from token checks.
+    # Default None.
+    # Must be in a parsable JSON list format.
+    # Strings must be double quoted.
+    # WARNING: this is a very powerful feature. Please make sure that your regex rules are strict.
+    ckanext.csrf_filter.exempt_rules = [
+        "^/do/not/check/this/path/.*",
+        "^/datatables/ajax/.*"
+      ]
+
+    # Cookie samesite attribute value. Defaults to None
+    ckanext.csrf_filter.same_site = Strict
+
 
 Testing
 =======

diff --git a/ckanext/csrf_filter/anti_csrf.py b/ckanext/csrf_filter/anti_csrf.py
@@ -11,6 +11,7 @@
 
 import hashlib
 import hmac
+import json
 from logging import getLogger
 import random
 import re
@@ -43,7 +44,6 @@
 ENCODED_TOKEN_VALIDATION_PATTERN = re.compile(
     r'^[0-9a-z]+![0-9]+/[0-9]+/[-_a-z0-9%+=]+$',
     re.IGNORECASE)
-API_URL = re.compile(r'^/+api/.*')
 LOGIN_URL = re.compile(r'^(/user)?/log(ged_)?in(_generic)?')
 CONFIRM_MODULE_PATTERN = r'data-module=["\']confirm-action["\']'
 CONFIRM_MODULE = re.compile(CONFIRM_MODULE_PATTERN)
@@ -64,9 +64,11 @@ def configure(config):
     """ Configure global values for the filter.
     """
     global secure_cookies
+    global same_site
     global secret_key
     global token_expiry_age
     global token_renewal_age
+    global exempt_rules
 
     site_url = urlparse(config.get('ckan.site_url', ''))
     if site_url.scheme == 'https':
@@ -75,6 +77,10 @@ def configure(config):
         LOG.warning("Site %s is not secure! CSRF tokens may be exposed!", site_url)
         secure_cookies = False
 
+    same_site = config.get('ckanext.csrf_filter.same_site', 'None')
+    # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value
+    assert same_site in ['Strict', 'Lax', 'None']
+
     key_fields = ['ckanext.csrf_filter.secret_key',
                   'beaker.session.secret',
                   'flask.secret_key']
@@ -90,6 +96,13 @@ def configure(config):
     token_expiry_age = 60 * config.get('ckanext.csrf_filter.token_expiry_minutes', 30)
     token_renewal_age = 60 * config.get('ckanext.csrf_filter.token_renewal_minutes', 10)
 
+    exempt_rules = [re.compile(r'^/+api/.*')]
+    custom_exempt_rules = config.get('ckanext.csrf_filter.exempt_rules', None)
+    if custom_exempt_rules:
+        for rule in json.loads(custom_exempt_rules):
+            LOG.debug("Adding CSRF exclusion: %s", rule)
+            exempt_rules.append(re.compile(rule))
+
 
 # -------------
 # Token parsing
@@ -204,8 +217,10 @@ def _is_request_exempt(request):
     as are API calls (which should instead provide an API key).
     """
     request_helper = RequestHelper(request)
+    for rule in exempt_rules:
+        if rule.match(request_helper.get_path()):
+            return True
     return not is_logged_in(request) \
-        or API_URL.match(request_helper.get_path()) \
         or request_helper.get_method() in {'GET', 'HEAD', 'OPTIONS'}
 
 
@@ -311,7 +326,7 @@ def _get_digest(message):
 def _set_response_token_cookie(token, response):
     """ Add a generated token cookie to the HTTP response.
     """
-    response.set_cookie(TOKEN_FIELD_NAME, token, secure=secure_cookies, httponly=True)
+    response.set_cookie(TOKEN_FIELD_NAME, token, secure=secure_cookies, httponly=True, samesite=same_site)
 
 
 def create_response_token():

diff --git a/ckanext/csrf_filter/helpers.py b/ckanext/csrf_filter/helpers.py
@@ -0,0 +1,18 @@
+from markupsafe import Markup
+from ckanext.csrf_filter.anti_csrf import get_response_token, TOKEN_FIELD_NAME
+
+try:
+    from ckan.common import is_flask_request
+except ImportError:
+    def is_flask_request():
+        return True
+
+
+def csrf_token_field():
+    if is_flask_request():
+        from flask import Response
+        response = Response()
+    else:
+        from pylons import response
+    token = get_response_token(response)
+    return Markup('<input type="hidden" name="{}" value="{}"/>'.format(TOKEN_FIELD_NAME, token))
diff --git a/ckanext/csrf_filter/plugin.py b/ckanext/csrf_filter/plugin.py
@@ -3,11 +3,13 @@
 """
 
 from logging import getLogger
+from types import GeneratorType
 
 from ckan import plugins
 from ckan.plugins import implements, toolkit
 
 from ckanext.csrf_filter import anti_csrf
+import ckanext.csrf_filter.helpers as h
 from ckanext.csrf_filter.request_helpers import RequestHelper
 
 
@@ -39,14 +41,21 @@ class CSRFFilterPlugin(plugins.SingletonPlugin):
     """ Inject CSRF tokens into HTML responses,
     and validate them on applicable requests.
     """
+    implements(plugins.IConfigurer)
     implements(plugins.IConfigurable, inherit=True)
     implements(plugins.IAuthenticator, inherit=True)
+    implements(plugins.ITemplateHelpers)
     if not toolkit.check_ckan_version('2.9'):
         implements(plugins.IRoutes, inherit=True)
     if toolkit.check_ckan_version(min_version='2.8.0'):
         implements(plugins.IBlueprint, inherit=True)
         implements(plugins.IMiddleware, inherit=True)
 
+    # IConfigurer
+
+    def update_config(self, config_):
+        toolkit.add_template_directory(config_, 'templates')
+
     # IConfigurable
 
     def configure(self, config):
@@ -62,6 +71,11 @@ def login(self):
         request.get_environ()['__no_cache__'] = True
         return None
 
+    # ITemplateHelpers
+
+    def get_helpers(self):
+        return {'csrf_token_field': h.csrf_token_field}
+
     # IRoutes
 
     def after_map(self, route_map):
@@ -94,7 +108,16 @@ def check_csrf():
         @blueprint.after_app_request
         def set_csrf_token(response):
             """ Apply a CSRF token to all response bodies.
+
+            Exclude GeneratorType responses as they are data streams.
+            Modifying the data of the data stream breaks the streaming process.
+
+            If a user needs to stream templates, they should use the csrf_token_field
+            helper in their forms inside of their streamed templates.
             """
+            if isinstance(getattr(response, 'response', None), GeneratorType):
+                return response
+
             response.direct_passthrough = False
             anti_csrf.apply_token(response)
             return response

diff --git a/ckanext/csrf_filter/templates/user/snippets/api_token_list.html b/ckanext/csrf_filter/templates/user/snippets/api_token_list.html
@@ -0,0 +1,15 @@
+{% ckan_extends %}
+
+{% block token_cell_actions %}
+<td>
+  {% set action = h.url_for("user.api_token_revoke", id=user['name'], jti=token['id']) %}
+  <form action="{{ action }}" method="POST">
+    {{ h.csrf_input() }}
+    <div class="btn-group">
+      <button type="submit" href="{{ action }}" class="btn btn-danger btn-sm" title="{{ _('Revoke') }}" data-module="confirm-action" data-module-with-data=true>
+        <i class="fa fa-times"></i>
+      </button>
+    </div>
+  </form>
+</td>
+{% endblock token_cell_actions %}
diff --git a/ckanext/csrf_filter/test_anti_csrf.py b/ckanext/csrf_filter/test_anti_csrf.py
@@ -7,6 +7,12 @@
 import six
 import unittest
 
+try:
+    from json import JSONDecodeError
+except ImportError:
+    # Python 2 JSON library raises ValueError instead
+    JSONDecodeError = ValueError
+
 from ckanext.csrf_filter import anti_csrf
 
 try:
@@ -70,7 +76,8 @@ def mock_objects(username=None):
     """
     anti_csrf.configure({
         'ckanext.csrf_filter.secret_key': 'secret',
-        'ckan.site_url': 'https://unit-test'})
+        'ckan.site_url': 'https://unit-test',
+        'ckanext.csrf_filter.exempt_rules': '["^/datatables/ajax/.*"]'})
     if username:
         anti_csrf._get_user = lambda: MockUser(username)
     else:
@@ -248,6 +255,41 @@ def _check_config(self, secret_key, secure_cookies=False,
         self.assertEqual(anti_csrf.token_expiry_age, token_expiry_age)
         self.assertEqual(anti_csrf.token_renewal_age, token_renewal_age)
 
+    def test_exempt_rules(self):
+        """ Tests that requests matching the exemption rules are not checked for tokens
+        """
+        mock_objects('unit-test')
+
+        # tests exempt rule skips token check
+        path = '/datatables/ajax/331fed84-2c8d-4d2e-b9ee-9ce8cbda3352'
+        request = MockRequest(method='', path=path, cookies={'auth_tkt': 'unit-test'})
+        print("Expecting check_csrf to pass for {}".format(path))
+        self.assertTrue(anti_csrf.check_csrf(request))
+
+        path = '/datatables/filtered-download/331fed84-2c8d-4d2e-b9ee-9ce8cbda3352'
+        request = MockRequest(method='', path=path, cookies={'auth_tkt': 'unit-test'})
+        print("Expecting check_csrf to fail for {}".format(path))
+        self.assertFalse(anti_csrf.check_csrf(request))
+
+        # test multiple regex rules
+        config = {'ckanext.csrf_filter.secret_key': 'secret_key'}
+        config['ckanext.csrf_filter.exempt_rules'] = '["^/datatables/ajax/.*", "/datatables/filtered-download/.*"]'
+        expected = [
+            '^/+api/.*',
+            '^/datatables/ajax/.*',
+            '/datatables/filtered-download/.*'
+        ]
+        anti_csrf.configure(config)
+        # Use custom matching since equivalent patterns won't necessarily
+        # compile to equal objects under all Python versions
+        self.assertEqual(len(anti_csrf.exempt_rules), len(expected))
+        for index in range(len(anti_csrf.exempt_rules)):
+            self.assertEquals(anti_csrf.exempt_rules[index].pattern, expected[index])
+
+        # test bad JSON string
+        config['ckanext.csrf_filter.exempt_rules'] = '^/datatables/ajax/.*", "/datatables/filtered-download/.*'
+        self.assertRaises(JSONDecodeError, anti_csrf.configure, config)
+
 
 if __name__ == '__main__':
     unittest.main()