Repository navigation
Prevent oversized client storage hydration payloads - #7551
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
 [Medium impact] The PR appears safe to merge; no new actionable issue was found. SummaryRejects oversized hydration CONNECT packets before sending them, using the server’s announced limit.
Reviews (20) · Last reviewed commit: "Merge branch 'main' into fix/7486-locals..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merging this PR will not alter performance1 warning
Comparing 
|
FarhanAliRaza
left a comment
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I tested this PR in a small Reflex app. The app has an rx.LocalStorage var, hydration and on_load markers, a counter, and a second page. I ran the same app on main (the PR base) as a baseline. I put values in localStorage from the browser and reloaded. A WebSocket wrapper counted the sockets opened and the bytes sent.
The reported fault is fixed. With 1.2M ASCII characters in storage, main opened 264 sockets and sent 315 MB in 8 seconds. With this PR, the client sent 0 bytes and recorded an error. The reconnect path also works: after storage grew past the limit while connected, a backend reload caused one failed reconnect and no storm.
I also checked behavior near the limit. A 900k ASCII value still hydrates, and events work. A 600k é value hydrates on main but is refused with this PR. See the first inline comment.
The two new JS tests fail on the base template and pass on this PR. test_state_js_template.py and test_build.py pass.
The requested changes are in the inline comments.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
FarhanAliRaza
left a comment
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I think this is a partial solution of the reconnect storm. We know payload limit if user sets it if user does not set it and payload limit is set at ngnix level this solution is useless there.
|
Thanks, you're right that the build-time Reflex limit alone cannot account for a lower, independently configured reverse-proxy limit. I pushed b206c7d with two changes: the browser still preflights against Reflex's configured limit, and an Engine.IO polling CONNECT POST that returns HTTP 413 now produces the hydration error and stops retries. The environment and release-note guidance also say to configure the effective limit no higher than the proxy's request limit and keep the frontend-build/backend values aligned. A WebSocket proxy that closes the connection without exposing a size-specific status cannot be distinguished reliably from a transient network failure, so treating every such disconnect as permanent would break normal reconnect behavior. Does your Nginx case use polling and return HTTP 413, or is there a specific WebSocket close/error signal we should handle? The targeted runtime suite passes (26 tests). |
… sending it Measure the CONNECT frame socket.io-client sends, namespace included, against the maxPayload the Engine.IO handshake announces. An oversized hydrate is reported once with its size and the limit, and the default connection toast shows it at once under its own id.
df9333c to
485347c
Compare
…n it Check the CONNECT packet inside the encoder, so it is encoded once and the namespace comes from socket.io-client. A missing handshake limit skips the check instead of refusing every hydrate. ConnectionModal now opens on the single hydration size error too.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
state.js now wraps io.encoder.encode, which socket.io-client always provides.
The opt-in modal shows the generic connection text and cannot be closed, so it does not fit a size error that never clears. The default toaster and ConnectionBanner report the error.
Comments Outside DiffThese findings could not be posted inline.
|
Port upstream's EventNamespace changes to the moved namespace modules: the trusted proxy hops for the client IP (reflex-dev#7614) apply in build_static_router_data, and the cancellation of a disconnected socket's page-load chains (reflex-dev#7415) runs in BaseEventNamespace.handle_disconnect, so both transports get them. The branch already decoded non-UTF-8 header values (reflex-dev#7602). Their tests move from tests/units/test_app.py to tests/units/test_event_namespace.py, and the transport benchmark's app double gains the event processor attribute the disconnect reads. Port the oversized hydrate check of reflex-dev#7551 to the Socket.IO transport only: state.js wraps the socket.io encoder where it creates the Socket.IO socket, since the plain WebSocket has no engine.io encoder. Its state.js tests run over Socket.IO, whose fake socket now encodes CONNECT packets, and the reconnect back-off of reflex-dev#7605 stays in place for both transports. uv.lock is upstream's, relocked for the branch's dependencies.
The plain WebSocket sends its hydrate with the connect frame, before any handshake could announce the server's message limit. A frame over the limit closed the socket with 1009, which the client reported as a generic connection failure and retried with back-off forever. The server now names the limit in that close's reason, and the transport reports such a close as the hydration_payload_too_large error reflex-dev#7551 added for Socket.IO, with the frame's size and the limit when the reason gives it. A reconnect over a limit an earlier handshake announced is refused before it is sent. retryConnect treats the error as final for both transports: it records it once, schedules no retry and keeps wait_connect set, so reconnect() does not rebuild the hydrate, and the toast and banner keyed on the error code show it.
- Take the engine.io server as a required argument and read its buffer limit, instead of a separate max_size and an optional eio. - Drain with a single more_body loop, read Content-Length through starlette Headers, and drop the ValueError and broad exception guards (servers validate Content-Length; engine.io logs handler errors). - Import engineio only for type checking. - Extract the handshake and oversized POST test helpers, and add a test that drives an oversized POST through the App's event endpoint. - Describe the remaining user-facing case in the news fragment: reflex-dev#7551 stops oversized hydrate packets in the client, so the stall now comes from oversized events.
…nt cannot stall the page (#7621) * Create +drain-oversized-polling-post.bugfix.md * Update app.py * Update test_app.py * Update app.py * Update test_app.py * Answer oversized polling POSTs from the drain middleware After the drain, answer the 400 from _DrainOversizedBodyMiddleware itself and close the engine.io session with socket.close(wait=False) instead of delegating to engine.io. engine.io rejects by disconnecting the session, whose close(wait=True) blocks in queue.join() until a poll GET consumes the CLOSE packet; the engine.io client races the CONNECT POST against the first poll GET, so with no poll parked the POST response was held forever and later polls got 'Session is disconnected'. The CLOSE packet still reaches a parked poll, but a missing poll no longer holds the response. Add a no-parked-poll regression test under asyncio.wait_for; it times out on the unpatched middleware and passes with the fix. * Correct eio docstring, assert 400 body in no-poll test * Simplify oversized POST drain middleware and test its mount - Take the engine.io server as a required argument and read its buffer limit, instead of a separate max_size and an optional eio. - Drain with a single more_body loop, read Content-Length through starlette Headers, and drop the ValueError and broad exception guards (servers validate Content-Length; engine.io logs handler errors). - Import engineio only for type checking. - Extract the handshake and oversized POST test helpers, and add a test that drives an oversized POST through the App's event endpoint. - Describe the remaining user-facing case in the news fragment: #7551 stops oversized hydrate packets in the client, so the stall now comes from oversized events. * Decode the drained POST's query string as Latin-1 engine.io treats a query string that is not UTF-8 as empty. A strict decode raised after the 400 was sent, so the session was not closed. Also word the news fragment as 'can reconnect': the client's reconnect policy controls the timing. --------- Co-authored-by: Farhan <www.mfarvirus@gmail.com>
Summary
Follow-up to #7486 and #7605. #7605 stops the reconnect loop: a CONNECT that the server or a proxy drops is retried with backoff and shows a generic connection error. This PR gives a clear error when browser storage makes the hydrate larger than the server's message limit, and does not send it.
/_eventnamespace and the Engine.IO message type, as the server receives it.maxPayloadfrom the Engine.IO handshake. The frontend build does not need the limit.Hydration payload (1000004 characters) exceeds the server limit (1000000 characters). Reduce browser storage data and reload.The socket disconnects, and later events do not rebuild the hydrate.ConnectionToastershows this error at once, under its own toast id, until the user closes it.ConnectionBannershows it too.ClientErrorType.HYDRATION_PAYLOAD_TOO_LARGEis the shared error code.A proxy limit is not known to the client. That case keeps the backoff from #7605.
Tests
tests/units/compiler/state_js.test.mjs: the check measures the namespaced frame (exactly at the limit is sent, one over is refused), for both transports. It also covers: the limit comes from the handshake, code points vs. bytes, one encode per CONNECT, other packets are unchanged, no rebuild on later events or on reconnect.tests/units/components/core/test_banner.py: the toaster shows the error before the two-error threshold.tests/integration/tests_playwright/test_hydration_storage.py:test_hydration_over_server_limit_is_reported_once(1.1 MB ofLocalStorageopens one socket, shows the error, and hydrates after the storage is cleared). It fails onmain. The fix(state): back off when the connection closes before connecting #7605 backoff test now covers only the proxy case.