Enable CSRF protection globally by default #9334

seanh · 2025-02-07T18:08:42Z

Enable Pyramid's config.set_default_csrf_options(require_csrf=True) which causes it to require a valid CSRF token for all requests with a request method that is not one of GET, HEAD, OPTIONS or TRACE.

The CSRF token must be in a csrf_token POST parameter or an X-CSRF-Token header, and must match the CSRF token stored in the signed session cookie.

It also checks that the request's Referer (if any) is the current host.

See:

This is a safer default. The current implementation requires all views receiving form submissions to use a Colander schema that's a subclass of CSRFSchema. It's too easy to forget to add CSRF protection to a form if it doesn't use Colander (for example: perhaps there are no parameters to be validated) or if it has a schema that doesn't subclass CSRFSchema. Even if the view's schema does sublass CSRFSchema, if it wants to have a validate() method it must remember to call super().validate() or it'll disable CSRFSchema's CSRF protection.

This commit removes the CSRF protection code from CSRFSchema (that schema is now only used to serialize the CSRF tokens into the forms, but doesn't do any CSRF validation at deserialization time) and instead enables Pyramid's global CSRF protection option.

CSRF protection can be disabled for individual views by passing require_csrf=False to @view_config. This has been added to h's custom @api_config decorator so that CSRF protection is disabled for all API endpoints.

Copilot reviewed 5 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (9)

h/schemas/forms/accounts/forgot_password.py: Evaluated as low risk
h/schemas/forms/accounts/login.py: Evaluated as low risk
h/accounts/schemas.py: Evaluated as low risk
tests/unit/h/schemas/forms/admin/group_test.py: Evaluated as low risk
tests/unit/h/schemas/forms/accounts/login_test.py: Evaluated as low risk
tests/unit/h/schemas/base_test.py: Evaluated as low risk
h/schemas/forms/admin/group.py: Evaluated as low risk
h/views/api/auth.py: Evaluated as low risk
h/schemas/auth_client.py: Evaluated as low risk

seanh · 2025-02-07T18:16:04Z

CSRF protection can be disabled for individual views by passing require_csrf=False to @view_config. This has been added to h's custom @api_config decorator so that CSRF protection is disabled for all API endpoints.

Cookie-authenticated API requests made by h's own frontend code (for example on the new group management pages) will still require a CSRF token. That's implemented separately by the APICookiePolicy security policy. You can verify this by removing the CSRF token and trying to create a new group:

diff --git a/h/views/groups.py b/h/views/groups.py
index a3de5c0f8..aa793a666 100644
--- a/h/views/groups.py
+++ b/h/views/groups.py
@@ -53,7 +53,7 @@ class GroupCreateEditController:
             return {
                 "method": method,
                 "url": self.request.route_url(route_name, **kw),
-                "headers": {"X-CSRF-Token": csrf_token},
+                "headers": {},
             }

         js_config = {

seanh · 2025-02-07T18:22:05Z

h/accounts/schemas.py

@@ -201,7 +201,6 @@ class EmailChangeSchema(CSRFSchema):
    password = password_node(title=_("Confirm password"), hide_until_form_active=True)

    def validator(self, node, value):
-        super().validator(node, value)


We're no longer inheriting from a CSRFSchema superclass with a validate() method, so several of these super() calls had to be removed. It'll no longer be possible to accidentally disable CSRF protection by forgetting to call super() here.

seanh · 2025-02-07T18:26:09Z

h/schemas/base.py

@@ -21,23 +21,49 @@ class ValidationError(httpexceptions.HTTPBadRequest):

 class CSRFSchema(colander.Schema):
    """
-    A CSRFSchema backward-compatible with the one from the hem module.
+    Add a hidden CSRF token to forms when seralized using Deform.


It's still up to us to make sure that the CSRF token is included in form submissions, Pyramid doesn't do that automatically. In h's case the way that's done is via this Colander schema base class.

Colander schemas get serialized into HTML forms through the machinery of Deform.

There's probably a better way to do this by just putting the CSRF token field directly into our custom Deform templates and getting rid of this schema entirely.

There's probably a better way to do this by just putting the CSRF token field directly into our custom Deform templates and getting rid of this schema entirely.

Pushed a second commit to do this

seanh · 2025-02-07T18:29:03Z

h/views/api/auth.py

@@ -104,6 +104,7 @@ def post(self):
        request_param="response_mode=web_message",
        is_authenticated=True,
        renderer="h:templates/oauth/authorize_web_message.html.jinja2",
+        require_csrf=False,


FIXME: This is needed to get some functests passing. This line should be removed and the failing functests should be fixed to use a CSRF token. (This reveals a bug in the code: this view wasn't requiring CSRF when it should have been.)

Can you add a comment here to indicate that this is being added only for the tests, and should not be required in actual usage.

seanh · 2025-02-07T18:32:56Z

tests/functional/h/views/admin/permissions_test.py

-    GROUP_PAGES = (
-        ("POST", "/admin/groups/delete/{pubid}", 302),
-        ("GET", "/admin/groups/{pubid}", 200),
-    )


FIXME: The POST to /admin/groups/delete is now failing because the test doesn't include a CSRF token. This actually reveals a bug in the code: that endpoint wasn't doing CSRF verification. Now it is. Rather than deleting the test case this functest should be fixed to use a CSRF token.

seanh · 2025-02-07T18:35:01Z

tests/unit/h/accounts/schemas_test.py

-        assert result == dict(
-            valid_params, privacy_accepted=True, comms_opt_in=None, csrf_token=None
-        )


The test was expecting the Colander schema to include csrf_token: None in the deserialized data if there was no csrf_token in the input. I've changed it to omit csrf_token from the output in this case.

seanh · 2025-02-07T18:37:09Z

h/schemas/base.py

+        #    POST param.
+        # 2. Unittests for schemas often don't set a CSRF token and would fail
+        #    if this `missing` value wasn't here.
+        missing=colander.drop,


Note the change from missing=None to missing=colander.drop. This means that if there's no csrf_token in the input data there'll be no csrf_token in the output data (previously it was injecting "csrf_token": None into the output). As noted in the code comment this case should never happen in production (unless someone is making requests with X-CSRF-Token headers, but our pages don't do that), but it does happen in tests.

seanh · 2025-02-07T18:38:17Z

tests/unit/h/accounts/schemas_test.py

-    def test_it_is_invalid_if_csrf_token_missing(self, pyramid_request, schema):
-        del pyramid_request.headers["X-CSRF-Token"]
-
-        with pytest.raises(BadCSRFToken):
-            schema.deserialize({"email": "[email protected]", "password": "flibble"})
-
-    def test_it_is_invalid_if_csrf_token_wrong(self, pyramid_request, schema):
-        pyramid_request.headers["X-CSRF-Token"] = "WRONG"
-
-        with pytest.raises(BadCSRFToken):
-            schema.deserialize({"email": "[email protected]", "password": "flibble"})


The Colander schemas no longer do CSRF verification. Several unittests like this can now be removed. (In most cases developers had forgotten to add CSRF test cases for their schemas anyway.)

robertknight · 2025-02-12T10:05:21Z

Generating a developer token at http://localhost:5000/account/developer is failing for me on this branch. This change fixed it locally:

diff --git a/h/templates/accounts/developer.html.jinja2 b/h/templates/accounts/developer.html.jinja2
index 761a3c7ed..f24c0c93c 100644
--- a/h/templates/accounts/developer.html.jinja2
+++ b/h/templates/accounts/developer.html.jinja2
@@ -20,6 +20,7 @@
     {% endif %}
 
       <form method="post" class="js-disable-on-submit">
+        <input type="hidden" name="csrf_token" value="{{ get_csrf_token() }}">
         {% if token %}
           <button type="submit" class="btn"
                   title="{% trans %}Delete your API token and generate a new one{% endtrans%}">

Enable Pyramid's `config.set_default_csrf_options(require_csrf=True)` which causes it to require a valid CSRF token for all requests with a request method that is *not* one of `GET`, `HEAD`, `OPTIONS` or `TRACE`. The CSRF token must be in a csrf_token POST parameter or an X-CSRF-Token header, and must match the CSRF token stored in the signed session cookie. It also checks that the request's `Referer` (if any) is the current host. See: * https://docs.pylonsproject.org/projects/pyramid/en/latest/narr/security.html#checking-csrf-tokens-automatically * https://docs.pylonsproject.org/projects/pyramid/en/latest/api/config.html#pyramid.config.Configurator.set_default_csrf_options This is a safer default. The current implementation requires all views receiving form submissions to use a Colander schema that's a subclass of `CSRFSchema`. It's too easy to forget to add CSRF protection to a form if it doesn't use Colander (for example: perhaps there are no parameters to be validated) or if it has a schema that doesn't subclass `CSRFSchema`. Even if the view's schema *does* sublass `CSRFSchema`, if it wants to have a `validate()` method it must remember to call `super().validate()` or it'll disable `CSRFSchema`'s CSRF protection. This commit removes the CSRF protection code form `CSRFSchema` (that schema is now only used to *serialize* the CSRF tokens into the forms, but doesn't do any CSRF validation at *deserialization* time) and instead enables Pyramid's global CSRF protection option. CSRF protection can be disabled for individual views by passing `require_csrf=False` to `@view_config`. This has been added to h's custom `@api_config` decorator so that CSRF protection is disabled for all API endpoints.

There's no need for this base class. This commit just replaces it with a line in h/templates/deform/form.jinja2 that adds a CSRF token to every form.

seanh · 2025-02-12T10:08:49Z

h/app.py

@@ -30,6 +30,8 @@ def create_app(_global_config, **settings):  # pragma: no cover


 def includeme(config):  # pragma: no cover
+    config.set_default_csrf_options(require_csrf=True)


This is what we're doing instead of always subclassing a CSRFSchema with a validate(): we just ask Pyramid to do automatic CSRF verification on all requests with unsafe HTTP methods, this is opt-out rather than opt-in and our views and schemas don't need to be concerned with it at all.

seanh · 2025-02-12T10:11:17Z

h/form.py

+        context.setdefault(
+            "get_csrf_token", partial(get_csrf_token, context["request"])
+        )


Adding Pyramid's get_csrf_token() method to the Jinja2 environment that's used to render our custom Deform templates, so that the base template for forms can automatically add a CSRF token.

TODO: There are lots of templates throughout the codebase that do their own get_csrf_token() calls, it should be possible to remove these now. Actually they may always have been unnecessary? Not sure.

robertknight

I found a missing CSRF token in /h/templates/accounts/developer.html.jinja2 but otherwise this looks good.

robertknight · 2025-02-12T10:03:09Z

h/views/api/auth.py

@@ -104,6 +104,7 @@ def post(self):
        request_param="response_mode=web_message",
        is_authenticated=True,
        renderer="h:templates/oauth/authorize_web_message.html.jinja2",
+        require_csrf=False,


Can you add a comment here to indicate that this is being added only for the tests, and should not be required in actual usage.

seanh · 2025-02-12T10:12:03Z

h/form.py

@@ -75,7 +82,7 @@ def create_form(request, *args, **kwargs):
    default) will use the renderer configured in the :py:mod:`h.form` module.
    """
    env = request.registry[ENVIRONMENT_KEY]
-    renderer = Jinja2Renderer(env, {"feature": request.feature})
+    renderer = Jinja2Renderer(env, {"feature": request.feature, "request": request})


Passing request to the renderer so it can add it to the template context above.

seanh · 2025-02-12T10:16:49Z

h/templates/deform/form.jinja2

@@ -8,6 +8,7 @@
      class="form {{ field.css_class or '' }}
             {%- if field.use_inline_editing %} js-form {% endif %}">
  <input type="hidden" name="__formid__" value="{{ field.formid }}" />
+  <input type="hidden" name="csrf_token" value="{{ get_csrf_token() }}">


The base Deform template for all forms now includes the CSRF token so this doesn't have to be done by each individual template.

Forms are rendered by having Deform serialize Colander templates to HTML using these custom Deform templates. When all our Colander templates were subclasses of a CSRFSchema class with a csrf_token field this causes Deform to include a CSRF token field in the serialized HTML (but only if the schema being serialized remembered to subclass CSRFSchema). This replaces that, just automatically putting the CSRF token in every form regardless of schema.

seanh requested review from marcospri, robertknight and Copilot February 7, 2025 18:10

Copilot AI reviewed Feb 7, 2025

View reviewed changes

seanh force-pushed the refactor-csrf-protection branch from d6a7000 to 21e84de Compare February 7, 2025 18:14

seanh commented Feb 7, 2025

View reviewed changes

seanh force-pushed the refactor-csrf-protection branch from 21e84de to 78d79ed Compare February 7, 2025 18:23

seanh commented Feb 7, 2025

View reviewed changes

Base automatically changed from fix-AdminGroupSchema-CSRF-protection to main February 12, 2025 09:59

seanh added 2 commits February 12, 2025 10:06

Factor out CSRFSchema base class

d20457e

There's no need for this base class. This commit just replaces it with a line in h/templates/deform/form.jinja2 that adds a CSRF token to every form.

seanh force-pushed the refactor-csrf-protection branch from 2c9573c to d20457e Compare February 12, 2025 10:06

seanh commented Feb 12, 2025

View reviewed changes

robertknight reviewed Feb 12, 2025

View reviewed changes

seanh commented Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CSRF protection globally by default #9334

Enable CSRF protection globally by default #9334

seanh commented Feb 7, 2025 •

edited

Loading

seanh commented Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025

seanh Feb 7, 2025 •

edited

Loading

robertknight Feb 12, 2025

seanh Feb 7, 2025

seanh Feb 7, 2025

seanh Feb 7, 2025

seanh Feb 7, 2025

robertknight commented Feb 12, 2025 •

edited

Loading

seanh Feb 12, 2025

seanh Feb 12, 2025 •

edited

Loading

robertknight left a comment

robertknight Feb 12, 2025

seanh Feb 12, 2025

seanh Feb 12, 2025

		@@ -30,6 +30,8 @@ def create_app(_global_config, **settings): # pragma: no cover


		def includeme(config): # pragma: no cover
		config.set_default_csrf_options(require_csrf=True)

Enable CSRF protection globally by default #9334

Are you sure you want to change the base?

Enable CSRF protection globally by default #9334

Conversation

seanh commented Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

seanh commented Feb 7, 2025 • edited Loading

seanh Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

seanh Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

seanh Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Feb 7, 2025 •

edited

Loading

seanh commented Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025 •

edited

Loading

seanh Feb 7, 2025 •

edited

Loading

robertknight commented Feb 12, 2025 •

edited

Loading

seanh Feb 12, 2025 •

edited

Loading