Repository navigation
Keep request credentials out of frontend router data - #7360
Conversation
|
Merging this PR will degrade performance by 11.16%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_hydration_metadata[200] |
1.7 ms | 1.9 ms | -11.16% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/session-cookie-privacy (d45cef9) with main (749dd55)
Footnotes
-
18 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
27416d7 to
33cad32
Compare
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I tested this PR in a small app against main (the PR's parent commit).
I sent raw websocket handshakes with custom header lists:
- one
Cookiefield - an upper-case
COOKIEfield - two
cookiefields, as HTTP/2 can send CookieandCOOKIEtogetherAuthorizationandCf-Access-Jwt-Assertion- two
X-Forwarded-Forfields
Then I searched the hydrate payload for the secret values. On main, every cookie case leaks. With this PR, no cookie value reaches the frontend in any case. Server code still reads the cookie.
In Chrome, State.router.headers.cookie renders "", and raw_headers has no cookie. An rx.Cookie set from the UI stays after a reload. The console shows no errors. The deprecation warning prints at compile time.
I also compared the cookie parsers of Starlette, Django, Werkzeug, Node cookie and universal-cookie. rx.Cookie takes the first value of a duplicate cookie name, as Node does. That follows the order in RFC 6265 §5.4, and this PR does not change it.
My findings are in the inline comments.
Preserve server-side cookie access while omitting Cookie from serialized router headers and raw_headers. Deprecate frontend router cookie reads in favor of rx.Cookie. Tests: 411 related unit tests passed; regression tests reproduced the leak before the fix. Ruff, formatting, and targeted pyright passed. Refs: ENG-12909
Apply the deprecated empty-string fallback to indexed cookie access. Strip server-only router metadata from outbound events, and omit standard authorization and Cloudflare/OAuth2 Proxy token headers from frontend header serialization. Preserve backend access and document missing raw header keys. Regression tests reproduced indexed access failures, HttpOnly cookie exposure in on-load events, and authorization header exposure in dev and production WebSocket frames before the fixes.
0b887ec to
7a31119
Compare
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Stop copying state.router_data onto generated on-load events. Backend chains already inherit the originating view through EventContext, and frontend actions do not need request routing metadata. Restore the original StateUpdate serializer to remove event filtering and cloning from serialization. Replace the serializer-level regression with a producer-level test that failed before this change. State/event processor tests (458), dev/prod browser tests (10), framework type checks, and changed-file hooks pass.
Comments Outside DiffThese findings could not be posted inline.
|
|
Addressing the latest outside-diff finding about explicit An application can still explicitly attach server data to an outgoing |
The State.router.headers.cookie deprecation added in #7360 shipped in the 0.10 alpha line, so the warning should name 0.10.0 rather than 0.9.13. Claude-Session: https://claude.ai/code/session_01XGbR9efAcf7RNMMqU7JLY8 Co-authored-by: Claude <noreply@anthropic.com>
One up-to-date, parameterized copy of each app, driver, probe and run script from the eight passes, in four areas with a README each (findings re-verified, run commands, expected result on 0.10.0): - hydration: browser storage and hydration, sync=True cross-tab storms and convergence, router data / credential exposure (#7360). - class_state: the state class model, set_default / copy-at-definition, add_var and auto-setter collisions, state-store compatibility. - enterprise: OIDC cross-tab logout, auth suites and matrix + MCP, AG Grid, demo smoke, reflex-azure-auth. - upgrade: installs, in-place upgrades of reflex-examples apps, third-party packages, events suite, CLI lifecycle, upgrade guide. Upstream apps, enterprise demos and the enterprise wheel are fetched or supplied at run time, never committed. Representative fixtures of every area were re-run on reflex 0.10.0 (+ reflex-enterprise 0.9.7) and match the 0.10.0a5 records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SjCrbdguMkfe4bhbaREiwo
One up-to-date, parameterized copy of each app, driver, probe and run script from the eight passes, in four areas with a README each (findings re-verified, run commands, expected result on 0.10.0): - hydration: browser storage and hydration, sync=True cross-tab storms and convergence, router data / credential exposure (#7360). - class_state: the state class model, set_default / copy-at-definition, add_var and auto-setter collisions, state-store compatibility. - enterprise: OIDC cross-tab logout, auth suites and matrix + MCP, AG Grid, demo smoke, reflex-azure-auth. - upgrade: installs, in-place upgrades of reflex-examples apps, third-party packages, events suite, CLI lifecycle, upgrade guide. Upstream apps, enterprise demos and the enterprise wheel are fetched or supplied at run time, never committed. Representative fixtures of every area were re-run on reflex 0.10.0 (+ reflex-enterprise 0.9.7) and match the 0.10.0a5 records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SjCrbdguMkfe4bhbaREiwo
WebSocket request cookies, including HttpOnly credentials, were copied into frontend router state. Frontend
on_loadevents could also return the raw server router metadata, exposing cookies even after the header serializer filtered them.Keep request credentials on the server in both paths: omit
Cookie,Authorization,Proxy-Authorization, Cloudflare Access assertions, OAuth2 Proxy access-token headers, AWS ALB OIDC tokens, and Google IAP assertions from serialized router headers, and stop copyingstate.router_dataonto generated on-load events. Backend chains inherit the originating view throughEventContext. This is a one-line source fix inreflex/state.py;StateUpdateserialization performs no added event filtering or cloning. Explicitly constructed application events retain anyrouter_datasupplied by the application.Backend handlers and server-side state persistence retain the original headers. Frontend
.headers.cookieand literal.headers["cookie"]access warn and render an empty string. Filteredraw_headerskeys are absent; the docs explain indexed access and.get(..., "")fallbacks.Validation on current main (
749dd55b3):pyright reflex testspassed. Workspace pyright targeting Linux passed, and the remaining pre-commit hooks passed. The macOS workspace-wide pyright hook reports six pre-existing CPU-affinity typing errors in benchmark files.The review's duplicate-header parsing report reproduces on main and is tracked separately in #7521, as the reviewer permits. This PR preserves the existing backend decoding behavior.
Covers ENG-12909. First layer of the token-protected endpoints project.
Next: #7361 → #7362 → #7363 → #7364. Enterprise companion: reflex-dev/reflex-enterprise#240.
Related to ENG-12909, ENG-9088.