Repository navigation
Conversation
github-actions
Bot
added
api
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const authTimer = setTimeout(() => { | ||
| if (socketId === undefined) { | ||
| statsd.increment('ws_auth_timeout'); | ||
| ws.close(AUTH_CLOSE_CODE, 'authentication timeout'); | ||
| } | ||
| }, AUTH_TIMEOUT_MS); |
There was a problem hiding this comment.
Suggestion: Because the upgrade is accepted before this timer and the limit is checked only after authentication, an unauthenticated client can bypass configured connection caps for 10 seconds.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Security
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-websocket/src/index.ts
**Line:** 829:834
**Comment:**
*Security: Because the upgrade is accepted before this timer and the limit is checked only after authentication, an unauthenticated client can bypass configured connection caps for 10 seconds.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed—the connection limits are only evaluated inside authenticate, so sockets that never send an auth frame are uncounted for the full 10s auth window (plus the close handshake), and anonymous clients can hold many upgraded sockets regardless of maxTotalConnections. Before this change an unauthenticated upgrade was rejected immediately. Should pending sockets count toward a total or separate pre-auth cap?
There was a problem hiding this comment.
Pending sockets should count toward maxTotalConnections; that setting is the global resource cap, while maxConnectionsPerChannel cannot be evaluated until authentication identifies a principal. Reserve one slot when accepting the connection and keep it reserved when the socket becomes authenticated; release it on close or timeout.
A separate, lower maxPendingAuthConnections limit could further protect against unauthenticated clients monopolizing the total slots, but it should supplement—not replace—the total cap. This closes the bypass while keeping the existing total-connection limit meaningful. Add tests for rejecting at capacity and releasing a pending slot after timeout/close.
There was a problem hiding this comment.
Valid. Upgraded-but-unauthenticated sockets now count toward maxTotalConnections (capacity is reserved at upgrade and released on auth, close, or auth timeout), and a new maxPendingConnections (MAX_PENDING_CONNECTIONS) can cap them separately. Over-limit upgrades are closed immediately with 1013. Tests in spec/auth.test.ts. Fixed in f273ec7.
There was a problem hiding this comment.
Same fix as above: pending sockets are counted and capped, see f273ec7.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b8eaf29.
The upgrade handler now reserves a pending-connection slot and rejects upgrades when configured connection limits or the pending-connection cap are reached. The reservation is released on close, authentication, or timeout.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 4089b91.
The upgrade handler now reserves a pending-connection slot and rejects upgrades when configured connection limits or the pending-connection cap are reached. The reservation is released on close, authentication, or timeout.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 0d4a54e.
The upgrade handler checks connection limits and the pending-connection cap before accepting a pending reservation. It increments pendingConnections and releases the reservation on close, timeout, or authentication.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 88ea23d.
The upgrade handler checks connection limits and the pending-connection cap before accepting a pending reservation. It increments pendingConnections and releases the reservation on close, timeout, or authentication.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 2d0d1a8.
The upgrade handler checks connection limits and the pending-connection cap before accepting a pending reservation. It increments pendingConnections and releases the reservation on close, timeout, or authentication.
If that's not right, unresolve this thread and CodeAnt will leave it open.
CodeAnt PR Risk: Medium Risk
Assessed commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45404 +/- ##
==========================================
+ Coverage 84.73% 84.74% +0.01%
==========================================
Files 3069 3070 +1
Lines 205781 205859 +78
Branches 48391 48415 +24
==========================================
+ Hits 174366 174459 +93
+ Misses 28122 28114 -8
+ Partials 3293 3286 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the latest review round in the follow-up commits. Connection state is now the single source of truth: realtime.ts exposes Stale connection results are now guarded by a per-attempt identity on top of the generation, so a ticket fetch, auth ack, refresh timer or close event from an earlier connection cannot affect a newer one. New tests cover a disconnect during a refresh, a reconnect or reconfigure while a ticket fetch is in flight, and a delayed Ticket endpoint errors other than 401/403, including 404 from an older web server during a rolling upgrade, 5xx and network errors, are treated as transient: the client backs off and retries and never enters UPDATING.md and the websocket README now state that after logout or session revocation an open socket stays authenticated until its current ticket expires, because the next refresh fails. |
There was a problem hiding this comment.
The connection-cap bypass is fixed and tested (f273ec7), good.
The other three threads are still open with no reply yet, and two of them look serious enough to block on. Verified the public-role one myself: getRealtimeTicket calls SupersetClient.get({ endpoint: TICKET_ENDPOINT }) with no custom unauthorizedHandler, and the client's default unauthorized handling does navigate to /login?next=... on a 401. So a deployment with PUBLIC_ROLE_LIKE = "Gamma" (a real, documented pattern for public dashboards, not a contrived config) would bounce anonymous visitors to the login screen on every page load once this ships. The embedded guest-token race looks just as real. asyncEvent.ts's init() fires the ticket request at module load, before setupGuestClient() has a chance to run, and enterUnauthorized() being terminal means the later guest token never gets a retry.
That's two ways this could break public/embedded dashboards in production, not edge cases. Holding off on approving until those get a response.
…band refresh Replace the host-scoped httponly JWT cookie with a short-lived ticket fetched from GET /api/v1/realtime/token/ and sent as the first frame of the socket. The client refreshes the ticket on the open socket at 75% of its lifetime, so an idle tab no longer loses its socket at cookie expiry and never tears down to re-authenticate. The websocket server answers handshake refusals with real HTTP statuses, holds new connections in a pending_auth state, and closes with 4401 (expired or invalid ticket) or 4403 (principal switch on refresh). The client reconnects immediately on 4401 and stops on 401/403 from the ticket endpoint (new unauthorized state). Removes WEBSOCKET_JWT_COOKIE_NAME/SECURE/SAMESITE/DOMAIN and the websocket server's jwtCookieName.
…ber, reconcile on rebind and unauthorized Count upgraded-but-unauthenticated sockets toward maxTotalConnections and add maxPendingConnections. Refuse the first auth frame when the Redis subscriber is unhealthy. Raise a rebind open event when the server acknowledges a changed tab id so waiters run one catch-up, and settle async waiters on the terminal unauthorized state. Correct the upgrade note and the README on connection lifetime.
…ale attempts asyncEvent settles a waiter registered while the realtime state is unauthorized or unhealthy. Each connection attempt gets an identity so a stale ticket fetch cannot open a socket. Document that an open socket stays authenticated until its ticket expires after revocation.
…after the guest token, bound auth acks - The ticket request passes ignoreUnauthorized, so a refused ticket (e.g. an anonymous visitor under PUBLIC_ROLE_LIKE) ends in the 'unauthorized' state instead of running the client's login redirect or guest handler. - The embedded page reconnects realtime once the guest client is configured, since the module-load attempt runs before a guest token exists. - The websocket server sends realtime.auth acks through the bounded send path, so replayed auth frames hit maxSocketBufferBytes like any other message.
4089b91 to
e76a1f9
Compare
|
Rebased onto master and addressed the open threads in e76a1f9: the ticket request no longer triggers the unauthorized handler (public-role redirect), the embedded page reconnects realtime once the guest client is configured, and auth acks go through the bounded send path. @rusackas both blocking cases you verified are covered, with tests. |
…sts, update SECURITY.md - The ticket endpoint sets Cache-Control: no-store, private. A guest authenticates by header, not cookie, so a shared cache must not store or replay a bearer ticket. - auth.test.ts closes both ends of each connection and waits for the pending reservations to be released before resetting server state, so a late close can't decrement the freshly reset count. - SECURITY.md describes connections as authorized by the most recent ticket presented on the socket, not by a JWT accepted at upgrade.
SUMMARY
An idle browser tab loses its realtime socket and never gets it back on its own. The websocket credential was an
httponlycookie re-minted only as a side effect of other HTTP requests, the client's keepalive was anchored to socket-open time rather than cookie-mint time, and the reconnect path never refreshed the cookie. After a websocket server restart, Redis blip, or laptop sleep, every retry failed with "JWT not present" until the user happened to make an HTTP request. The server also destroyed unauthenticated upgrades with no HTTP status, so the client could not tell an auth failure from a network failure.This replaces the cookie with short-lived connection tickets and in-band re-authentication. The client always connects with a credential it has just obtained, knows its lifetime, and refreshes it on the open socket without reconnecting.
Backend: new
GET /api/v1/realtime/token/(gated bycan_readonRealtime, 401 without a session, 403 without the permission) returning{"token": "<jwt>", "expires_in": <seconds>}using the existingget_realtime_principal()andmint_channel_token(), withCache-Control: no-store, privateso a shared cache can never store or replay a ticket (embedded guests authenticate by header, not cookie). The cookie path (register_ws_channel_cookie,_should_remint) and theWEBSOCKET_JWT_COOKIE_NAME/SECURE/SAMESITE/DOMAINconfig keys are removed;WEBSOCKET_JWT_SECRETandWEBSOCKET_JWT_EXPIRATION_SECONDSstay. The now-unusedWEBSOCKET_JWT_EXPIRATION_SECONDSbootstrap entry is dropped because the client readsexpires_infrom the server instead.Websocket server: the handshake keeps the Origin allowlist and subscriber-health check, no longer reads cookies, and answers refusals with real statuses (403 for origin, 503 for unhealthy). A new connection starts
pending_auth, registered on no channel, and is closed with4401if no validauthframe ({"type": "auth", "token": "...", "tab_id": "..."}) arrives within 10s. Pending sockets count towardmaxTotalConnections, and a newmaxPendingConnections(MAX_PENDING_CONNECTIONS) can cap them separately; an over-limit upgrade is closed with1013. If the Redis subscriber has become unhealthy by the time the firstauthframe arrives, the socket is closed with1012and nothing is registered. The first valid frame registers the principal and tab channels and repliesrealtime.authwithexpires_in. A later frame refreshes the expiry; naming a different channel closes the socket with4403, and a changedtab_idrebinds the tab channel. Expired sockets are closed with4401instead of terminated. Inbound frames are capped at 16 KiB, andrealtime.authacks go through the same bounded send path (maxSocketBufferBytes) as every other outbound message. ThejwtCookieNameoption and thecookiedependency are removed.Frontend (
realtime.ts): every connect (initial and reconnect) fetches a ticket, opens the socket, sendsauth, and reportsopenonly after therealtime.authacknowledgement (abandoned and retried after 10s). A 401 or 403 from the ticket endpoint enters a new terminalunauthorizedstate with no connect loop; the request passesignoreUnauthorized, so a refused ticket (e.g. an anonymous visitor underPUBLIC_ROLE_LIKE) never triggers the client's login redirect or guest handler. Other ticket errors (404 from an older web server during a rolling upgrade, 5xx, network) are transient and retried with backoff. A ticket fetch, ack, refresh timer or close event from a superseded connection attempt is ignored. The ticket is refreshed on the open socket at 0.75 xexpires_inwith backoff on transient failures; close4401on a previously authenticated socket reconnects immediately, other closes keep the exponential backoff.KEEPALIVE_FRACTION,COOKIE_REFRESH_ENDPOINT, and the keepalive teardown are removed, andtab_idmoved from the connect URL into theauthframe, so a tab-id reassignment no longer reconnects. Thekeepaliveopen reason is gone since refreshes never reopen the socket; when the server acknowledges a changed tab id on the open socket, the client emits a singlerebindopen event soasyncEventruns one status catch-up.realtime.tsexposesgetRealtimeState(), andasyncEvent.tssettles chart requests promptly while the state isunauthorizedorunhealthy, both for waiters pending at the transition and for requests registered afterwards. The embedded entry reconnects realtime once the guest client is configured, since the module-load attempt runs before a guest token exists.Security notes: the token is now readable by page JavaScript, which only lets the holder receive this principal's realtime nudges (opaque ids and task statuses); script in the page can already call the REST API as the user. Tokens never appear in URLs, so proxies do not log them. A refresh cannot switch principal on a live socket. Revocation stays bounded by the token lifetime: after logout the ticket endpoint returns 401, no refresh happens, and the server closes the socket at expiry.
SECURITY.mddescribes an open connection as authorized by the most recent ticket presented on it. The Origin allowlist stays as defense in depth. This also removes the requirement that the websocket server share a browser-visible host with the web application.Docs,
SECURITY.mdandUPDATING.mdare updated, including the removed config keys.UPDATING.mdasks operators to upgrade the web and websocket servers together and reload open tabs: tabs still running the old cookie-based client are refused by the new server and retry until reloaded. The websocket README notes that the ticket lifetime no longer bounds connection lifetime, since clients refresh on the open socket, so draining a replica needs explicit socket closes or pod termination. Thesuperset-websocket/utils/client-ws-appdev tool is updated to send anauthframe instead of setting a cookie. Operator follow-up: stop renderingWEBSOCKET_JWT_COOKIE_NAMEand passingJWT_COOKIE_NAMEto the websocket server.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A, no visual change.
TESTING INSTRUCTIONS
Unit tests:
pytest tests/unit_tests/websocket,npm testinsuperset-websocket(includesspec/auth.test.ts, which drives the realwsupgrade,auth, refresh,4401, and4403paths over in-memory duplex streams), andnpx jest src/middleware/realtime.test.ts src/middleware/asyncEvent.test.ts src/embedded/index.test.tsx src/views/CRUD/hooks.test.tsxinsuperset-frontend.Manual, with
WEBSOCKET_ENABLE = TrueandWEBSOCKET_JWT_EXPIRATION_SECONDS = 60:101.4403.PUBLIC_ROLE_LIKE = "Gamma", open a public dashboard while logged out. The page is not redirected to login, and no socket is opened.ADDITIONAL INFORMATION
WEBSOCKET_ENABLEis on)Fixes #45493