Skip to content

Commit

Permalink
Fix AdminGroupSchema CSRF protection
Browse files Browse the repository at this point in the history
Doing `super().__init__(validator=username_validator)` overrides the
Colander schema's global validator function, with the result that the
base `CSRFSchema` class's `validator()` method never gets called and
there is no CSRF protection.

Replace this with a subclass `validator()` method that calls the
superclass's method first.
  • Loading branch information
seanh committed Feb 7, 2025
1 parent a3a53e9 commit 3afebfd
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
6 changes: 5 additions & 1 deletion h/schemas/forms/admin/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def group_organization_select_widget(_node, kwargs):

class AdminGroupSchema(CSRFSchema):
def __init__(self, *args):
super().__init__(validator=username_validator, *args) # noqa: B026
super().__init__(*args)

group_type = colander.SchemaNode(
colander.String(),
Expand Down Expand Up @@ -217,3 +217,7 @@ def __init__(self, *args):
widget=SequenceWidget(add_subitem_text_template=_("Add member")),
missing=None,
)

def validator(self, node, value):
super().validator(node, value)
username_validator(node, value)
13 changes: 13 additions & 0 deletions tests/unit/h/schemas/forms/admin/group_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import colander
import pytest
from pyramid.exceptions import BadCSRFToken

from h.models.group import (
GROUP_DESCRIPTION_MAX_LENGTH,
Expand All @@ -17,6 +18,18 @@ class TestAdminGroupSchema:
def test_it_allows_with_valid_data(self, group_data, bound_schema):
bound_schema.deserialize(group_data)

def test_it_raises_if_csrf_token_missing(self, group_data, bound_schema):
del bound_schema.bindings["request"].headers["X-CSRF-Token"]

with pytest.raises(BadCSRFToken):
bound_schema.deserialize(group_data)

def test_it_raises_if_csrf_token_wrong(self, group_data, bound_schema):
bound_schema.bindings["request"].headers["X-CSRF-Token"] = "foobar"

with pytest.raises(BadCSRFToken):
bound_schema.deserialize(group_data)

def test_it_raises_if_name_too_short(self, group_data, bound_schema):
too_short_name = "a" * (GROUP_NAME_MIN_LENGTH - 1)
group_data["name"] = too_short_name
Expand Down

0 comments on commit 3afebfd

Please sign in to comment.