Visitar URL original
Strip optional whitespace when splitting headers on `,` by MaxFreedomPollard · Pull Request #1526 · wemake-services/django-modern-rest · GitHub
Skip to content

Strip optional whitespace when splitting headers on , - #1526

Merged
sobolevn merged 1 commit into
wemake-services:masterfrom
MaxFreedomPollard:fix/split-commas-whitespace
Sep 15, 2026
Merged

sobolevn merged 1 commit into
wemake-services:masterfrom
MaxFreedomPollard:fix/split-commas-whitespace

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

I have made things!

One bug in dmr/internal/django.py, in parse_headers: a header listed in __dmr_split_commas__ is split on , and the whitespace that follows the comma is kept as part of the next value.

A client that writes a list header the usual way gets a different result from one that writes it compactly. X-Tag: 1,2 parses as ['1', '2'], but X-Tag: 1, 2 parses as ['1', ' 2']. On a msgspec model with tags: list[int] that is a 400:

Expected `int`, got `str` - at `$.parsed_headers.X-tag[1]`

On pydantic the int case is coerced and passes, so the failure is quieter there: with tags: list[str] the second value is ' 2', a leading space nobody asked for.

RFC 9110, section 5.6.1 defines a list header as #element => [ element ] *( OWS "," OWS [ element ] ). The optional whitespace around the separator is part of the grammar, not part of the member values, so a, b and a,b are the same list and a recipient has to accept both. Section 5.3 also lets an intermediary combine repeated field lines with a comma and optional whitespace, so a value that started as two separate X-Tag lines can reach the application with the spaces in it. Django's own single-line join, in django/core/handlers/asgi.py and in wsgiref, uses a bare ,, which is why the documented example in docs/examples/components/headers_split.py works today and the bug only shows up for clients or proxies that write the spaced form. Django's own HttpRequest.accepted_types does the same token.strip() on the comma-split Accept header (django/http/request.py, line 100 in 5.2.17).

The fix strips each part in parse_headers. The split_commas branch of convert_multi_value_dict is deliberately left alone: that path splits a submitted form value for the Body component, where a comma is ordinary user data and a space next to it is data too, and there is no header grammar to appeal to. Section 5.6.1.2 of the RFC also asks recipients to ignore empty list members, so first,,second would become ['first', 'second']; I left that alone, since it changes the list length a model sees and deserves its own discussion, and the new test pins the current ('first,', ['first', '']) result so that choice is explicit. This is a single str.strip per part on a path that only runs when a model actually declares __dmr_split_commas__, and benchmarks/apps/dmr.py declares no __dmr_split_commas__ on its HeadersModel, so the CodSpeed suite never enters this branch.

Verification on macOS, Python 3.11.15, Django 5.2.17:

python -m pytest tests/test_unit -o addopts='' -q
2406 passed, 5 skipped in 27.10s

python -m ruff check --exit-non-zero-on-fix   All checks passed!
python -m ruff format --check                 823 files already formatted
python -m flake8 .                            no output
python -m slotscheck -v -m dmr                All OK!
import-linter lint                            Contracts: 9 kept, 0 broken.
python -m mypy dmr                            Success: no issues found in 201 source files

I skipped tests/test_integration/test_throttling locally because it needs a Redis container.

The new tests are test_tags_split_commas_whitespace in tests/test_unit/test_components/test_headers.py and the new tests/test_unit/test_internal/test_django/test_parse_headers.py. On master 6 of them fail, all on the spaced and tabbed inputs; the compact first,second cases pass there as before.

Whitespace in a split header value is arguably the user's problem, since __dmr_split_commas__ is documented as a blunt tool. I put the changelog line under Bugfixes because the two spellings of the same list giving two different results looks like a bug rather than a choice; move it under Breaking changes if you would rather have it there.

AI Policy

  • I have read and agree to the AI Policy, removed any "Co-Authored-By" lines attributing coding agents, and manually reviewed the final result

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made

Related issues

No existing issue. I found this while reading the headers component docs and trying the example with a hand written curl -H "X-Tag: 1, 2".

`parse_headers` split a `__dmr_split_commas__` header on `,` and kept
whatever came next, so `X-Tag: 1, 2` was parsed as `['1', ' 2']` while
`X-Tag: 1,2` was parsed as `['1', '2']`. On a `list[int]` field that is a
`400` with `msgspec`, and on a `list[str]` field it is a value with a
leading space on both plugins.

RFC 9110, section 5.6.1 defines a list header as
`#element => [ element ] *( OWS "," OWS [ element ] )`, so the whitespace
around the separator belongs to the grammar and not to the values, and a
recipient has to accept both forms. Each part is now stripped.

The `split_commas` branch of `convert_multi_value_dict` is left alone on
purpose: it splits a submitted form value, where a comma is ordinary data
and a space next to it is data too.
@codspeed

codspeed Bot commented Sep 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 18 untouched benchmarks


Comparing MaxFreedomPollard:fix/split-commas-whitespace (0a618a0) with master (eefe988)

Open in CodSpeed

@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (eefe988) to head (0a618a0).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1526   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          501       502    +1     
  Lines        23710     23726   +16     
  Branches       731       731           
=========================================
+ Hits         23710     23726   +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sobolevn
sobolevn merged commit a104bce into wemake-services:master Sep 15, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants