Don't allow group names with leading or trailing whitespace #8834
Conversation
h/schemas/api/group.py
Outdated
| # Strip leading and trailing whitespace from group names. | ||
| if isinstance(data.get("name"), str): | ||
| data["name"] = data["name"].strip() |
There was a problem hiding this comment.
Just trim leading and trailing whitespace from the group name before validating it. So this means that not only are whitespace-only group names invalid, but also names that're less than the minimum length after trimming leading and trailing whitespace. Also if a name is still long enough after trimming whitespace, the name that gets saved will be the whitespace-trimmed version not the name that was actually given.
The alternative would be to raise a validation error if the name starts or ends with whitespace.
| def test_it_raises_if_name_isnt_a_string(self, schema): | ||
| name = 42 | ||
|
|
||
| with pytest.raises( | ||
| ValidationError, match=f"name: {name} is not of type 'string'" | ||
| ): | ||
| schema.validate({"name": name}) |
There was a problem hiding this comment.
When calling string methods like .trim() in the validation() method, it'd be easy to introduce a crash when someone calls the API with a non-string name. Added this test to cover that.
| def test_it_validates_with_no_data(self, schema): | ||
| appstruct = schema.validate({}) | ||
|
|
||
| assert appstruct == {} |
There was a problem hiding this comment.
GroupAPISchema is the base schema that CreateGroupAPISchema and UpdateGroupAPISchema inherit from. CreateGroupAPISchema makes name required but UpdateGroupAPISchema doesn't. So in both GroupAPISchema and UpdateGroupAPISchema an empty dict is valid data. When adding manual code about name to validate() it would be easy to write code that crashes if there's no name in the given data, so adding this test to cover that case.
| @@ -40,6 +40,32 @@ def test_it_raises_if_name_too_long(self, schema): | |||
| with pytest.raises(ValidationError, match="name:.*is too long"): | |||
| schema.validate({"name": "o" * (GROUP_NAME_MAX_LENGTH + 1)}) | |||
|
|
|||
| def test_it_raises_if_name_isnt_a_string(self, schema): | |||
There was a problem hiding this comment.
There are tests for the sad path here, but what about the happy path? If I copy some text with leading/trailing whitespace and paste it into the name field, the group name should be trimmed, assuming it is neither too short nor too long.
seanh
force-pushed
the
trim-whitespace-from-group-names
branch
2 times, most recently
from
0cefbc7
to
ad4124f
Compare
seanh
force-pushed
the
trim-whitespace-from-group-names
branch
from
ad4124f
to
9a43c62
Compare
There was a problem hiding this comment.
I've realized there is a follow-up issue we need to resolve here which is to display the validation messages in an aria-live region so that they get announced.
seanh
changed the title
Added a note for this: https://github.com/orgs/hypothesis/projects/142/views/1?pane=issue&itemId=73034957 |
We can validate this in the frontend UI as well, but I don't think the backend API should be allowing whitespace-only group names and the like either.