Skip to content

fix: combine repeated request headers instead of keeping the last - #7588

Merged
masenf merged 2 commits into
reflex-dev:mainfrom
ybai08:fix/7521-repeated-headers
Oct 19, 2026
Merged

masenf merged 2 commits into
reflex-dev:mainfrom
ybai08:fix/7521-repeated-headers

Conversation

@ybai08

@ybai08 ybai08 commented Oct 17, 2026 •

Copy link
Copy Markdown

Refs #7521. This is part 1 of the split proposed on the issue: steps 1 and 2 of the fix. It leaves raw_headers multi-value access (getlist, step 3) to a follow-up PR.

_decode_asgi_headers built a dict from the ASGI header pairs, so a repeated field kept only its last value. ASGI keeps repeats as separate pairs. The function now combines them in order:

  • repeated cookie fields are joined with "; " (RFC 9113 §8.2.3);
  • any other repeated field is joined with ", " (RFC 9110 §5.3).

It still returns the same dict[str, str], so RouteVar.HEADERS and everything that reads it keep their shape. With the combined values:

  • router.headers.cookie has every cookie;
  • router.session.client_ip comes from the first x-forwarded-for hop, because the existing .partition(",")[0] now sees first, second;
  • raw_headers[name] shows the combined value.

The credential filtering from #7360 works on the joined dict, so a repeated cookie is still dropped entirely from what reaches the frontend.

Tests

  • test_decode_asgi_headers: the old "later duplicates win" assertion now expects a=1; b=2. A new case interleaves two x-forwarded-for fields with another header and expects 10.0.0.1, 10.0.0.2, 10.0.0.3.
  • test_static_router_data_keeps_every_repeated_header_field (new) goes through EventNamespace._build_static_router_data with two cookie and two x-forwarded-for fields. It checks the joined headers, that CLIENT_IP is the first hop (203.0.113.7, not the proxy), and that HeaderData.from_router_data(...).cookie keeps both cookies.

Both fail on main with the issue's symptom ({'cookie': 'b=2'}, 'other=2'). With the change, all of tests/units/test_app.py passes (194).

I also ran uv run pytest tests/units --cov --no-cov-on-fail --cov-report=: 12014 passed, 26 skipped, total coverage 80.03%. ruff check, ruff format --check and pyright on the two changed files are clean (0 errors).

Performance

The function now runs a loop instead of a comprehension. I measured that with timeit on the benchmark's unique-header shape on macOS arm64. It's 1.10–1.14× slower: 0.27 vs 0.25 µs for 4 headers, 4.08 vs 3.57 µs for 64. It is only called from _build_static_router_data, once per connection, not per event.

Not in this PR

Step 3: keeping every field in raw_headers for getlist() and multi_items(). The open questions for it are on the issue.

View guided diff

_decode_asgi_headers built a dict from the ASGI header pairs, so a repeated
field kept only its last value: router.headers.cookie lost every cookie but
the last, and client_ip came from the last x-forwarded-for field instead of
the first hop. Join repeated cookie fields with "; " and every other repeated
field with ", ", in order.

Refs reflex-dev#7521
@ybai08
ybai08 requested a review from a team as a code owner October 17, 2026 02:53
@greptile-apps

greptile-apps Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

Repeated request headers now keep their values in order.

  • Router headers keep every repeated request header value.

Reviews (2) · Last reviewed commit: "perf: decode ASGI headers with the defau..." · Reviewed by Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread reflex/app.py
Comment thread reflex/app.py Outdated
@codspeed

codspeed Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 4.47%

⚠️ 18 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 147 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ test_route_argument_extraction 115.2 µs 120.5 µs -4.47%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ybai08:fix/7521-repeated-headers (0dfb96c) with main (75a08e1)

Open in CodSpeed

@masenf
masenf merged commit f96cef3 into reflex-dev:main Oct 19, 2026
143 of 144 checks passed
benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Oct 20, 2026
Port upstream's repeated-header combining (reflex-dev#7588) from the removed
reflex/app.py helper to _decode_asgi_headers in reflex/event_namespace.py,
which both transports share, keeping the branch's Latin-1 fallback. Its
tests move from tests/units/test_app.py to tests/units/test_event_namespace.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on deck PRs lined up to review / merge next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants