fix(security): stop putting long-lived JWTs in SSE/WebSocket URL query strings (#745) - #800
Conversation
…eams with single-use tickets (#745) Streams (EventSource/WebSocket) cannot send an Authorization header, so the two allowlisted SSE routes and both WS routes accepted the raw 24h JWT as ?token=. Query strings land in proxy/access logs and APM traces, so a leaked line granted a day-long session takeover. Backend: new codeframe/auth/stream_tickets.py mints opaque single-use tickets (60s TTL, in-process store mirroring the rate limiter's singleton pattern); POST /auth/stream-ticket (require_auth + enforce_auth_rate_limit) mints one; get_current_user's SSE query fallback and authenticate_websocket now redeem ?ticket= and no longer accept ?token= anywhere. Frontend: fetchStreamTicket() + async withStreamTicket() replace withTokenParam; useEventSource takes an async buildUrl re-resolved on every connect AND retry (single-use tickets must never be replayed); all four stream URL sites (task stream, stress-test stream, session chat WS, terminal WS) mint a fresh ticket per connection attempt, with a bare-URL fallback for auth-disabled mode. Test fallout migrated: query-param, WS-auth, session-chat and terminal WS suites now exercise tickets, including expired/reused/unknown-ticket and 'old ?token= no longer authenticates' cases.
…-test stream on workspace change Cross-family review (codex) findings on #745: - useTerminalSocket retried with the same closed-over URL, replaying the already-consumed single-use ticket; migrated to the same {enabled, connectionKey, buildUrl} shape as useEventSource so every connect AND retry re-resolves a fresh ticketed URL. - useStressTestStream's connectionKey lost the workspace identity the old URL-based effect had; include workspacePath so switching workspaces while streaming reconnects instead of staying on the stale one.
A redeemed ticket acts as a full user session on the WS routes (terminal input / chat mutate state), so a read-only API key could previously escalate to WS access by minting a ticket (codex review P1). Read-only keys don't need tickets: header-capable clients hit the SSE routes with X-API-Key directly.
Direct membership check rejected admin-only API keys even though the scope hierarchy treats admin as implying write (codex review P2); use the shared has_scope() helper like other guards.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughReplaces long-lived JWT query-string authentication for SSE/WebSocket streams with short-lived, single-use stream tickets. Adds a backend ticket store, a POST /auth/stream-ticket endpoint, updated REST/WebSocket auth dependencies, and frontend hooks/components that fetch fresh tickets per connection attempt. ChangesBackend Stream Ticket System
Estimated code review effort: 4 (Complex) | ~75 minutes Frontend Stream Ticket Integration
Estimated code review effort: 4 (Complex) | ~70 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthAPI as "POST /auth/stream-ticket"
participant TicketStore
participant StreamEndpoint as "SSE/WS Endpoint"
Browser->>AuthAPI: POST with Bearer JWT
AuthAPI->>TicketStore: mint_ticket(user_id)
TicketStore-->>AuthAPI: opaque ticket
AuthAPI-->>Browser: { ticket, expires_in }
Browser->>StreamEndpoint: connect with ?ticket=...
StreamEndpoint->>TicketStore: redeem_ticket(ticket)
TicketStore-->>StreamEndpoint: user_id or error
StreamEndpoint-->>Browser: stream established or 401/close
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This is a well-executed security fix. Replacing long-lived JWTs in URL query strings with short-lived single-use tickets closes a real exposure window (proxy/access logs, browser history, APM traces) and the implementation is careful across the board. What the PR does Replaces Security strengths
One design point worth confirming In no-auth mode ( Implementation quality
The One minor note on The frontend Multi-worker limitation Documented in the module docstring. Mirrors the rate-limiter limitation. Acceptable for self-hosted single-worker deployments; the Redis upgrade path is the same one already documented for the rate limiter. Test coverage Comprehensive: unit tests for mint/redeem/expiry/single-use/sweep; endpoint tests for all scope combinations; SSE query-param regression confirming Small observation: Breaking change Old Summary Solid, well-tested security improvement with conservative scope, documented trade-offs, and a test suite that would catch backsliding to the old pattern. No blocking concerns. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
web-ui/src/__tests__/hooks/useAgentChat.test.ts (1)
149-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the epoch-guard's actual race-condition path.
The async conversions and new "fresh ticket per reconnect" test look correct. However, none of the tests exercise the specific race
connectEpochRefwas introduced to guard against:disconnect()(unmount or sessionId change) firing whilefetchStreamTicket()is still pending. Every existing test awaitsflushConnect()(i.e., lets the ticket resolve) before triggering unmount/rerender, so theif (connectEpochRef.current !== epoch) return;branch inuseAgentChat.tsis never actually hit by the suite.Consider adding a test with a manually-controlled ticket promise, e.g.:
it('bails out if disconnected while the ticket fetch is in flight', async () => { let resolveTicket: (v: string | null) => void; mockFetchTicket.mockReturnValue( new Promise((res) => { resolveTicket = res; }) ); const { unmount } = renderHook(({ id }) => useAgentChat(id), { initialProps: { id: 'session-1' }, }); unmount(); await act(async () => { resolveTicket('test-ticket'); await Promise.resolve(); }); expect(MockWebSocket.instances).toHaveLength(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/src/__tests__/hooks/useAgentChat.test.ts` around lines 149 - 264, The useAgentChat test suite is missing coverage for the connectEpochRef race guard when disconnect happens while fetchStreamTicket is still pending. Add a test in useAgentChat.test.ts that controls the ticket promise manually, starts useAgentChat with a sessionId, triggers unmount or rerender before the promise resolves, then resolves the ticket and asserts no WebSocket is created; reference useAgentChat, fetchStreamTicket, and the connectEpochRef cancellation branch to locate the path being exercised.web-ui/src/hooks/useAgentChat.ts (1)
127-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEpoch guard is correctly placed — but ticket-URL logic duplicates
withStreamTicket.The
connectEpochRefrace guard is sound:epochis captured before the onlyawait, and rechecked immediately after, before any WebSocket is created — no interleaving window exists. No functional issue here.However, lines 143-144 manually re-implement the same "append
ticket=query param if present" logic thatwithStreamTicket()(web-ui/src/lib/auth.ts) already provides, and which sibling hooks in this cohort (e.g.useTerminalSocket) use. This diverges from the intended shared pattern and risks drift if the ticket param name/encoding ever changes.♻️ Proposed refactor to reuse `withStreamTicket`
-import { fetchStreamTicket, verifyAuthAfterStreamFailure } from '`@/lib/api`'; +import { fetchStreamTicket, verifyAuthAfterStreamFailure } from '`@/lib/api`'; +import { withStreamTicket } from '`@/lib/auth`';const epoch = connectEpochRef.current; - const ticket = await fetchStreamTicket(); + const url = await withStreamTicket( + `${WS_BASE_URL}/ws/sessions/${sessionId}/chat`, + fetchStreamTicket + ); // disconnect() (unmount / session change) may have run while the ticket // request was in flight; bail rather than opening a stale connection. if (connectEpochRef.current !== epoch) return; - - const ticketParam = ticket ? `?ticket=${encodeURIComponent(ticket)}` : ''; - const url = `${WS_BASE_URL}/ws/sessions/${sessionId}/chat${ticketParam}`; const ws = new WebSocket(url);
withStreamTicketis a pure function (no side effects beyond invoking the passedfetchTicket), so the existing@/lib/apimock in the test file needs no changes for this refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/src/hooks/useAgentChat.ts` around lines 127 - 144, The ticket URL assembly in useAgentChat is duplicating the shared withStreamTicket helper instead of using the existing auth utility. Refactor the WebSocket connect path in useAgentChat to pass the base ws URL through withStreamTicket (the same pattern used by useTerminalSocket), so ticket query-string construction stays centralized and consistent with web-ui/src/lib/auth.ts.web-ui/src/hooks/useTerminalSocket.ts (1)
92-171: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
connect()against thrown/rejectedbuildUrl/WebSocketconstruction.
await buildUrlRef.current()andnew WebSocket(url)aren't wrapped in try/catch. Sinceconnect()is invoked viavoid connect(), an unexpected throw (a futurebuildUrlimplementation rejecting, or a malformedurlmakingnew WebSocket()throw synchronously) would leavestatusstuck at'connecting'indefinitely with an unhandled promise rejection, rather than surfacing'error'. Not currently reachable sincefetchStreamTicket/withStreamTicketnever throw, but worth hardening since this is a reusable, generic hook contract.🛡️ Proposed fix to harden `connect()` against unexpected errors
const connect = async () => { setStatus('connecting'); - const url = await buildUrlRef.current(); - if (cancelled) return; - if (!url) { - setStatus('error'); - return; - } - - const ws = new WebSocket(url); + let url: string | null; + try { + url = await buildUrlRef.current(); + } catch { + if (!cancelled) setStatus('error'); + return; + } + if (cancelled) return; + if (!url) { + setStatus('error'); + return; + } + + let ws: WebSocket; + try { + ws = new WebSocket(url); + } catch { + setStatus('error'); + return; + } ws.binaryType = 'arraybuffer'; wsRef.current = ws;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/src/hooks/useTerminalSocket.ts` around lines 92 - 171, `connect()` in `useTerminalSocket` needs error hardening around `buildUrlRef.current()` and `new WebSocket(url)`, since either can throw/reject and leave the hook stuck in `connecting`. Wrap the async URL resolution and socket construction in `try/catch` inside `connect`, and on any unexpected failure clear the in-flight attempt and set status to `error` (while still respecting the `cancelled` guard). Keep the retry/close flow in the existing `connect`, `close`, and `ws.onclose` paths unchanged except for routing thrown setup failures into the same error state.web-ui/src/components/sessions/AgentTerminal.tsx (1)
66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
withStreamTicketinstead of duplicating ticket-append logic.This manually reimplements the same
?ticket=<encoded>construction thatwithStreamTicket()(web-ui/src/lib/auth.ts) already provides and is used consistently byuseStressTestStream/useTaskStream.♻️ Proposed refactor to reuse `withStreamTicket`
+import { withStreamTicket } from '`@/lib/auth`'; + const buildUrl = useCallback(async (): Promise<string | null> => { - const ticket = await fetchStreamTicket(); - const ticketParam = ticket ? `?ticket=${encodeURIComponent(ticket)}` : ''; - return `${wsBase()}/ws/sessions/${sessionId}/terminal${ticketParam}`; + return withStreamTicket(`${wsBase()}/ws/sessions/${sessionId}/terminal`, fetchStreamTicket); }, [sessionId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/src/components/sessions/AgentTerminal.tsx` around lines 66 - 76, Reuse the existing withStreamTicket helper instead of manually building the ticket query string in AgentTerminal’s buildUrl callback. Update buildUrl to delegate ticket handling to withStreamTicket from web-ui/src/lib/auth.ts, using the same pattern as useStressTestStream and useTaskStream, and keep the fallback behavior for missing/failed tickets intact.web-ui/src/hooks/useEventSource.ts (1)
113-122: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding
buildUrl()against thrown rejections.
connect()awaitsbuildUrlRef.current()with no try/catch. If a futurebuildUrlimplementation throws instead of resolving (the type signature() => Promise<string | null>doesn't prevent this), the connection gets stuck in'connecting'forever with an unhandled promise rejection and no retry. Today's callers (withStreamTicket/fetchStreamTicket) always resolve, so this isn't currently reachable, but it's a hidden expectation on all futurebuildUrlimplementations.🛡️ Proposed defensive fix
const connect = async () => { setStatus('connecting'); - const url = await buildUrlRef.current(); - if (cancelled) return; - if (!url) { - setStatus('error'); - return; - } + let url: string | null; + try { + url = await buildUrlRef.current(); + } catch { + url = null; + } + if (cancelled) return; + if (!url) { + setStatus('error'); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/src/hooks/useEventSource.ts` around lines 113 - 122, The connect() flow in useEventSource currently awaits buildUrlRef.current() without handling thrown rejections, so a failing buildUrl can leave the hook stuck in 'connecting'. Update connect() to defensively wrap the buildUrlRef.current() await in try/catch, and on error setStatus('error') and exit/abort the connection path so future buildUrl implementations cannot leave the hook hanging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@codeframe/auth/dependencies.py`:
- Around line 198-227: Guard _authenticate_stream_ticket against failures from
_load_active_user just like authenticate_websocket does for auth-related
lookups. Wrap the await _load_active_user(user_id) call in a try/except for the
same operational/auth exceptions used elsewhere in codeframe.auth.dependencies,
and translate them into HTTPException responses instead of letting them bubble
up as 500s. Keep the existing TicketRedemptionError and user_id is None handling
intact.
---
Nitpick comments:
In `@web-ui/src/__tests__/hooks/useAgentChat.test.ts`:
- Around line 149-264: The useAgentChat test suite is missing coverage for the
connectEpochRef race guard when disconnect happens while fetchStreamTicket is
still pending. Add a test in useAgentChat.test.ts that controls the ticket
promise manually, starts useAgentChat with a sessionId, triggers unmount or
rerender before the promise resolves, then resolves the ticket and asserts no
WebSocket is created; reference useAgentChat, fetchStreamTicket, and the
connectEpochRef cancellation branch to locate the path being exercised.
In `@web-ui/src/components/sessions/AgentTerminal.tsx`:
- Around line 66-76: Reuse the existing withStreamTicket helper instead of
manually building the ticket query string in AgentTerminal’s buildUrl callback.
Update buildUrl to delegate ticket handling to withStreamTicket from
web-ui/src/lib/auth.ts, using the same pattern as useStressTestStream and
useTaskStream, and keep the fallback behavior for missing/failed tickets intact.
In `@web-ui/src/hooks/useAgentChat.ts`:
- Around line 127-144: The ticket URL assembly in useAgentChat is duplicating
the shared withStreamTicket helper instead of using the existing auth utility.
Refactor the WebSocket connect path in useAgentChat to pass the base ws URL
through withStreamTicket (the same pattern used by useTerminalSocket), so ticket
query-string construction stays centralized and consistent with
web-ui/src/lib/auth.ts.
In `@web-ui/src/hooks/useEventSource.ts`:
- Around line 113-122: The connect() flow in useEventSource currently awaits
buildUrlRef.current() without handling thrown rejections, so a failing buildUrl
can leave the hook stuck in 'connecting'. Update connect() to defensively wrap
the buildUrlRef.current() await in try/catch, and on error setStatus('error')
and exit/abort the connection path so future buildUrl implementations cannot
leave the hook hanging.
In `@web-ui/src/hooks/useTerminalSocket.ts`:
- Around line 92-171: `connect()` in `useTerminalSocket` needs error hardening
around `buildUrlRef.current()` and `new WebSocket(url)`, since either can
throw/reject and leave the hook stuck in `connecting`. Wrap the async URL
resolution and socket construction in `try/catch` inside `connect`, and on any
unexpected failure clear the in-flight attempt and set status to `error` (while
still respecting the `cancelled` guard). Keep the retry/close flow in the
existing `connect`, `close`, and `ws.onclose` paths unchanged except for routing
thrown setup failures into the same error state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3301f20c-a701-48db-a022-8b6b5c98b192
📒 Files selected for processing (26)
codeframe/auth/dependencies.pycodeframe/auth/router.pycodeframe/auth/stream_tickets.pytests/api/test_session_chat_ws.pytests/auth/test_query_param_token.pytests/auth/test_stream_ticket_endpoint.pytests/auth/test_stream_tickets.pytests/auth/test_websocket_auth.pytests/ui/test_terminal_ws.pytests/ui/test_v2_auth_enforcement.pyweb-ui/__tests__/hooks/useEventSource.test.tsweb-ui/src/__tests__/components/sessions/AgentTerminal.test.tsxweb-ui/src/__tests__/hooks/useAgentChat.test.tsweb-ui/src/__tests__/hooks/useStressTestStream.test.tsweb-ui/src/__tests__/hooks/useTaskStream.test.tsweb-ui/src/__tests__/hooks/useTerminalSocket.test.tsweb-ui/src/__tests__/lib/auth.test.tsweb-ui/src/__tests__/lib/fetchStreamTicket.test.tsweb-ui/src/components/sessions/AgentTerminal.tsxweb-ui/src/hooks/useAgentChat.tsweb-ui/src/hooks/useEventSource.tsweb-ui/src/hooks/useStressTestStream.tsweb-ui/src/hooks/useTaskStream.tsweb-ui/src/hooks/useTerminalSocket.tsweb-ui/src/lib/api.tsweb-ui/src/lib/auth.ts
…et path CodeRabbit (PR #800, Major): _authenticate_stream_ticket let a DB error during _load_active_user bubble out as a 500; the bearer path and authenticate_websocket both convert unexpected failures to a controlled auth failure. Mirror them.
Follow-up Review (Round 2)Building on my prior review and CodeRabbit's pass, here is a status snapshot on the open items. Resolved since prior reviewsCodeRabbit's actionable: Still open (CodeRabbit nitpicks — low severity, but worth a pass before merge)1. Manual ticket URL construction duplicates
const ticketParam = ticket ? `?ticket=${encodeURIComponent(ticket)}` : '';The shared const url = await withStreamTicket(baseUrl, fetchStreamTicket);The terminal and stress-test hooks use this pattern correctly; these two are the outliers. Not blocking, but the inconsistency is a copy-paste trap for future work. 2.
let url: string | null;
try {
url = await buildUrlRef.current();
} catch {
url = null;
}3. Missing epoch-guard race test in The SummaryAll three are minor quality/robustness items — none are security concerns and the core ticket flow is correct. Items 1 and 2 are quick wins (~5 lines each). Item 3 covers a subtle async invariant. Happy to approve once these are addressed, or if items 2 and 3 are tracked in a follow-up issue, that's fine given the overall quality of this PR. |
|
Follow-up review (second pass) My initial review found this to be a solid, well-tested security fix. This pass checks the CodeRabbit findings and any new commits since then. CodeRabbit Actionable: DB error → 500 in The code already has the correct guard around CodeRabbit Nitpicks: Outstanding 1. Duplicate ticket-append logic in Both still manually construct the query param instead of delegating to
2. Both 3. Missing epoch-guard race test in The My Prior Note (Still Stands)
Overall No blocking concerns. The core security goal is achieved, the critical CodeRabbit actionable was already handled, and the remaining items are all minor DRY/robustness nitpicks. Ready to merge if the open nitpicks are tracked as follow-up work. |
1 participant
Summary
Implements #745 (P1.18, security): SSE/WS streams no longer authenticate with the raw 24h JWT in
?token=(which leaks via proxy/access logs, APM traces, and browser history). Streams now use a short-lived (60s), single-use ticket minted by an authenticatedPOST /auth/stream-ticketand redeemed as?ticket=— one uniform mechanism for both SSE (EventSource can't send headers) and WebSocket.codeframe/auth/stream_tickets.py(opaquesecrets.token_urlsafe(32)tickets, 60s TTL, consumed on first redemption, thread-safe in-process store mirroring the rate-limiter singleton pattern).POST /auth/stream-ticketrequiresrequire_auth+ write scope (viahas_scope, so admin implies write) and the auth rate limit.get_current_user's allowlisted SSE query fallback andauthenticate_websocketnow redeem?ticket=;?token=is no longer accepted anywhere.fetchStreamTicket()+ asyncwithStreamTicket()replacewithTokenParam.useEventSourceanduseTerminalSocketmigrated to an{enabled, connectionKey, buildUrl}API whose asyncbuildUrlis re-resolved on the initial connect and every retry, so a consumed ticket is never replayed. All four stream sites (task SSE, stress-test SSE, session-chat WS, terminal WS) mint a fresh ticket per connection attempt, with a bare-URL fallback for auth-disabled dev mode.Acceptance Criteria
?token=<valid JWT>now 401s), WS auth tests, and a server-level enforcement test.Test Plan
useAgentChat(epoch guard)has_scope).ticket=→token=→ 5 tests failKnown Limitations / Intentionally Deferred
--workers > 1. Same accepted trade-off as the in-memory rate limiter (documented in the module docstring); Redis-backed store is the upgrade path.?token=<JWT>on the two SSE routes or the WS routes must switch to the ticket flow. The web UI (the only in-tree client) is migrated.X-API-Keydirectly.Implementation Notes
tasks/todo.md._QUERY_TOKEN_PATHSrenamed to_QUERY_TICKET_PATHS(same two SSE regexes — tickets redeem only on stream paths).Closes #745
Summary by CodeRabbit
New Features
Bug Fixes
Tests