fix: enforce per-guest isolation on public share channels (#973)#993
fix: enforce per-guest isolation on public share channels (#973)#993codeacme17 wants to merge 12 commits into
Conversation
) PR1 of the share-channel hardening pass. A public share link is opened by many anonymous visitors sharing the same owner + entity id, but get_task_for_share_context only authorized a task by owner + agent/workforce id — so any visitor holding a valid share JWT could address another visitor's task id and read/continue that conversation. - Mint a server-owned, high-entropy guest_id at POST /api/share/auth (both agent- and workforce-share paths) and sign it into the guest JWT. The endpoint never accepts a client-supplied guest_id — share has no secondary credential (unlike widget's embed ticket / widget key), so guest_id is the sole isolation credential and must be server-minted. - get_share_chat_user fails closed on tokens without a guest_id (those minted before this change) rather than falling back to the old no-isolation path. - get_task_for_share_context requires task.agent_config["guest_id"] to match the caller's, on top of the existing checks, in both branches (shared helper). Automatically covers the WS connect + per-message revalidation. - Stamp guest_id into the task at create time: agent-share assigns server keys last so a client value can't override; workforce-share threads it via extra_agent_config (survives the snapshot merge). - Frontend (share only): persist and reuse the guest JWT so guest_id is stable across reloads instead of re-authing each mount, and recover a returning visitor whose persisted taskId belongs to a different guest — catch the 4003/403 access denial, drop the stale taskId, start fresh. Widget behavior unchanged. Stored in agent_config JSON; no schema change / migration. Rate limiting and upload hardening remain for PR2/PR3.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements per-guest isolation for public share links to prevent anonymous visitors from accessing each other's tasks. On the backend, a server-minted, high-entropy guest_id is introduced to the guest JWT, and access checks are added to ensure a guest can only access tasks matching their guest_id. On the frontend, guest tokens are persisted and reused across reloads to maintain session continuity, with recovery logic to handle invalid or legacy sessions. The reviewer suggests strengthening the validation of the persisted authentication object retrieved from localStorage to prevent potential runtime errors from corrupted or legacy data.
Reject corrupt or cross-version localStorage entries in readPersistedShareAuth instead of type-asserting: require a non-empty string access_token and keep agent_id/workforce_id to their optional-number contract. Malformed blobs now fall back to a clean re-auth rather than flowing bad values downstream. Addresses Gemini review on xorbitsai#993.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements per-guest isolation for public share links by introducing a server-minted guest_id embedded in the guest JWT, ensuring visitors can only access their own tasks. Legacy tokens lacking this ID are rejected to fail closed. On the frontend, guest tokens are persisted and reused, with recovery flows to clear stale sessions upon authorization failures. Reviewer feedback recommends wrapping localStorage.removeItem calls in try/catch blocks to prevent crashes in restricted environments (such as private browsing) and refactoring test helpers to avoid nested database sessions that could cause SQLite writer-lock contention.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR closes a per-guest privacy hole on public share channels (agent-share and workforce-share). Previously many anonymous visitors shared one owner+agent/workforce identity, and tasks were authorized only by a guessable Task.id + owner + agent id — so any visitor could address and read another visitor's conversation. The fix mints a server-owned, high-entropy guest_id (secrets.token_urlsafe(32)) at share-auth time, signs it into the guest JWT, stamps it onto the task's agent_config at creation, and requires it to match on every subsequent access via a new shared helper _require_share_guest_owns_task. Legacy tokens minted before this change (no guest_id) now fail closed rather than being silently allowed through.
Design verdict — Sound
The backend isolation mechanism is architecturally solid, verified by tracing the actual code paths (not just the diff):
guest_idis server-minted only at both auth endpoints (share.py:78,share.py:121);ShareAuthRequestexposes no client-suppliedguest_id. It is bound into the HS256 JWT signature, so it cannot be forged or stripped without invalidating the token, andget_share_chat_userfails closed (401) on a missing/emptyguest_idwith no fallback to the old behavior.- Agent-share task creation copies the client's
agent_configfirst, then assignsauth_mode/share_agent_id/guest_idafter (public_chat_access.py:716-719) — server values always win. - Workforce-share task creation routes
guest_idthroughextra_agent_config;_merge_agent_config(workforce_runs.py:83) does{**extra_agent_config, **task_config}andbuild_workforce_task_confignever setsguest_id, so the server value survives the snapshot merge untouched. - All four consumption sites (WS connect, WS per-message revalidation, share file upload, public file preview/download) go through
_require_share_guest_owns_task. - Share vs. widget claim safety: both flows use a
"widget"-typed JWT internally, butauth_mode("share" vs "widget") is enforced on both the JWT-consumption side and the storedagent_config.auth_mode, so the two claim spaces cannot be confused. - Scope boundary holds: rate-limiting and storage/orphan-GC are explicitly deferred to a follow-up per #973, and nothing in this diff drifts into that territory.
The one real defect is a frontend recovery-UX bug (Finding 1), not a weakness in the isolation mechanism — isolation is never bypassed; an affected returning visitor is simply left stuck instead of gracefully recovered.
Findings (by severity)
Finding 1 — MAJOR — Returning-guest recovery never triggers for its main intended scenario
The connect-time WS handler (public_chat_access.py:826-834) wraps auth in a bare except Exception and always closes with code=4001, reason="Authentication required", discarding the real HTTPException.detail. The 4003-with-reason path exists only in the per-message revalidation loop, which never runs during the initial connect. On the frontend, only the 4003 handler sets connectionError (use-websocket.ts:186-191); the 4001 handler does not, and app-context-chat.tsx:1326-1336 wires no onError. The recovery useEffect (public-agent-chat-page.tsx:131-140) only fires when connectionError is in SHARE_ACCESS_DENIED_REASONS — which never happens on connect. Net effect: a returning visitor whose persisted taskId belongs to another guest (or a pre-#973 legacy task) hits a connect failure that never triggers the recovery flow the PR's own comment (lines 124-130) describes — the comment's claim that "the WS connect closes 4003" is factually wrong; it always closes 4001. Not a security hole; the recovery UX this PR built is simply defeated.
Fix: either have the connect-time handler distinguish auth-denial exceptions and close with 4003 + the real reason (mirroring the revalidation path), or have the frontend 4001 handler also set connectionError / trigger the recovery check.
Finding 2 — MINOR — Overly broad denial-reason string conflates "stale task" with "link genuinely disabled"
"Share link is unavailable" is emitted by many distinct backend causes (channel mismatch, missing workforce_run, unpublished agent/workforce, sharing disabled by owner, etc.) and is included in SHARE_ACCESS_DENIED_REASONS (public-agent-chat-page.tsx:58-62). When a genuinely disabled link triggers it mid-session, the recovery logic fires anyway: clears state, retries task-create, fails, forces re-auth, fails, lands on the terminal error screen. Confirmed not an infinite loop, but an unnecessary extra round-trip, and the comments mischaracterize a non-recoverable case as recoverable.
Fix: narrow SHARE_ACCESS_DENIED_REASONS to the guest-mismatch-specific string only (the backend already emits a distinct "Access denied for this guest"), i.e. drop "Share link is unavailable" from the recoverable set.
Finding 3 — MINOR — No test exercises the core tamper-proof claim for agent-share
None of the 6 tests in tests/web/api/test_share_guest_isolation.py posts a forged guest_id inside the agent_config body of POST /api/share/chat/task/create to assert the server-minted value wins. The surface is real — TaskCreateRequest.agent_config is client-controlled and create_share_chat_task relies on assignment order to override it — so a future reorder would regress silently. (The workforce-share path is not at risk from this vector, since it never reads request.agent_config into the merge.)
Fix: add a case posting agent_config={"guest_id": "<attacker-id>"} and assert the persisted task's guest_id equals the authenticated guest's minted id.
Finding 4 — MINOR — Isolation test assertions check type, not value
test_share_guest_isolation.py:161 and :194 assert isinstance(task.agent_config.get("guest_id"), str) — presence/type only, never comparing to the guest's minted id. Low risk (no realistic bug passes this and the surrounding 403/200 assertions simultaneously, since stamping and checking share the same field), but the test is weaker and less self-documenting than it could be.
Fix (optional, cheap): decode the JWT in the auth helper to extract the minted guest_id and assert equality directly.
Finding 5 — MINOR/NOTE — Concurrent-tab race mints two different guest_ids for one visitor
If a visitor opens two tabs to the same share link near-simultaneously (before the first tab persists auth), each independently calls /api/share/auth and mints a different guest_id, so the tabs behave as separate guests. No BroadcastChannel / storage listener / lock coordinates the share-auth path (the Web Locks mutex in api-wrapper.ts is scoped to logged-in token refresh, not imported here). This edge case is made consequential by this PR's own per-guest isolation but is explicitly within the accepted scope (anonymous, no cross-tab guarantee). Note only — not blocking.
Simplification opportunities
Lean already. net: -0 lines possible.
Re-review / prior bot status
One prior automated review exists (gemini-code-assist, COMMENTED, no blocking inline threads). Its localStorage-validation suggestion is partially addressed: the auth read/write (readPersistedShareAuth/persistShareAuth, ~lines 378-410) got try/catch wrapping, but three localStorage.removeItem calls in the same file (lines 121, 138, 202) remain unwrapped. Low risk in practice (removeItem rarely throws even in restricted storage contexts, unlike getItem/setItem) — a passing nit, not blocking.
Testing
Static review only. The test suite was not executed in this review environment (a fresh dependency install including heavy packages such as torch would be required, and no venv was available — a review-environment limitation, not a comment on the PR). The author reports the full tests/web/api suite passing and ruff/mypy/tsc/eslint clean; this was not independently verified here.
…bitsai#973) Wrap the three recovery-path localStorage.removeItem calls (cross-guest taskId reset, task-create 401/403 handler, onAuthInvalidated) in try/catch so a SecurityError in private mode / sandboxed iframes can't abort the session-reset that follows. Matches the existing try/catch style around persist/read of the share auth blob. Addresses Gemini review on xorbitsai#993.
get_share_chat_user treated any non-empty string guest_id as valid, so a whitespace-only value would pass the fail-closed check. Such an id can never match a server-minted token_urlsafe guest_id, so treat it as absent and reject the token loudly rather than admitting a guest that can own no task.
The connect-time handler wrapped auth in a bare except and always closed 4001, discarding the HTTPException.detail. The 4003 + reason path only existed in per-message revalidation, which never runs on initial connect, so a returning guest whose persisted task belongs to another guest (or a pre-xorbitsai#973 legacy task) hit a 4001 that never triggered the frontend recovery flow. Mirror the revalidation path: close 4003 with the real reason on HTTPException, 4001 otherwise. Also narrow SHARE_ACCESS_DENIED_REASONS to the guest-mismatch case: 'Share link is unavailable' is emitted by non-recoverable causes (owner disabled the link, unpublished, channel mismatch), so treating it as recoverable only forced a pointless clear + re-auth round-trip that still landed on the terminal error screen.
…sai#973) Add a case posting a forged guest_id in the agent-share create-task body and assert the persisted task carries the attacker's own minted id, not the forged victim id — locking the assignment-order override so a future reorder cannot regress it silently. Strengthen the two isolation tests to compare the stamped guest_id against this guest's minted id (decoded from the JWT) instead of merely asserting a string is present.
|
@rogercloud thanks for the thorough trace-based review — especially confirming the isolation mechanism by following the real code paths. Addressed the findings in Finding 1 (MAJOR) — fixed. You're right: the connect-time handler always closed Finding 2 (MINOR) — fixed. Dropped Finding 3 (MINOR) — fixed. Added Finding 4 (MINOR) — fixed. The two isolation tests now decode the guest JWT (new Finding 5 (NOTE) — acknowledged, not changed. Agreed it's within the accepted anonymous/no-cross-tab-guarantee scope for PR1. gemini Verification: full |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Re-review (round 2) — head cd7915f9 (was ae45073)
What changed since last round
Since my initial review the author addressed all 4 real findings (commits 91130d2a, b958e172), plus landed 1 unprompted hardening fix (c972efeb). I ran a fresh full pass on the current head — a blank-slate re-derivation plus independent verification of every claimed fix and of two newly-surfaced issues.
Design verdict: Sound (third independent confirmation)
The backend per-guest isolation mechanism remains architecturally solid and tamper-proof. This is now the third independent trace (initial review, drafted first re-review, this blank-slate pass) to reach the same conclusion — server-minted guest_id is authoritative, client-supplied values are ignored, and cross-guest access is denied at both the connect and per-message paths.
Status of original findings
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | MAJOR | WS connect always closed 4001, discarding the real denial reason → returning-guest recovery never fired | FIXED — verified (91130d2a); connect handler now closes 4003 with the real exc.detail, full recovery chain traced end-to-end |
| 2 | MINOR | Overly broad "Share link is unavailable" in SHARE_ACCESS_DENIED_REASONS caused needless round-trips |
FIXED — verified (91130d2a); string removed, all remaining raise-sites confirmed non-recoverable |
| 3 | MINOR | No test for a forged guest_id override in the agent-share task-create body |
FIXED — verified (b958e172); new test forges a victim id and asserts the server-minted id wins; full suite passes |
| 4 | MINOR | Isolation test assertions checked type only, not the minted guest_id value |
FIXED — verified (b958e172); tests now assert equality against the decoded minted id |
| 5 | NOTE | Concurrent-tab race mints two guest_ids for one visitor |
WAIVED — accepted PR1 scope (anonymous, no cross-tab guarantee); untouched, as expected |
Note on the unprompted whitespace-guest_id fix (c972efeb)
The change from not guest_id to not guest_id.strip() in get_share_chat_user is correct, tested, and zero-regression. Characterizing it honestly: this is defense-in-depth, not a fix for a reachable vulnerability. Reaching this check requires an already-validly-signed JWT, which requires JWT_SECRET_KEY itself to be compromised — at which point an attacker could forge any non-whitespace guest_id just as easily, so the check doesn't meaningfully raise the bar against a real attacker. Fine to keep (harmless and tested) — just not a closed exploit.
New finding this round
[MEDIUM · NEW] Expired persisted share-auth token has no recovery path.
This PR introduced persisting the guest JWT in localStorage so guest_id stays stable across reloads (a reasonable design choice). But readPersistedShareAuth validates only the persisted object's structure — it never checks the JWT exp claim (30-day TTL). A visitor returning after 30+ days with intact storage reuses the expired token; the backend raises ExpiredSignatureError → HTTPException(401, "Invalid share token"); the connect handler (via the Finding-1 fix) closes 4003 with that reason — but "Invalid share token" is not in SHARE_ACCESS_DENIED_REASONS, so the recovery effect never fires. The onAuthInvalidated path doesn't help either: it triggers only on a REST 401/403 from task/create, which a returning visitor resuming over the WS connect path never calls. Net effect: dead session, no automatic recovery — only a manual localStorage clear unsticks the visitor.
Severity MEDIUM: the mechanism is fully real with zero mitigation, but likelihood is narrow (requires a 30+ day return with intact storage, e.g. a bookmarked/embedded widget link). Newly introduced by this PR's own persistence feature — the scenario was structurally impossible before (mounts always re-authed fresh). See inline comment for the suggested fix.
Hypothesis checked and ruled out
For transparency: I also examined a hypothesized React effect-ordering race (recovery effect immediately re-clearing a freshly-created task because a stale connectionError lingers in the same commit). Tracing the actual state machine ruled it out — state.taskId cannot jump old→new in one commit; it must pass through an explicit null, and use-websocket.ts clears connectionError on exactly that null transition, before any new task can exist. Not a finding, no change needed.
Simplification opportunities (non-blocking)
- Still open (raised previously): the three
localStorage.removeItemtry/catch blocks inpublic-agent-chat-page.tsx(plus a 4th unwrapped one in the taskId-persistence sync effect) are duplicated structure — extract a singlesafeRemoveItem(key)helper. - New this round: two near-identical JWT-decode-to-
guest_idtest helpers now exist —_minted_guest_idintests/web/api/test_share_guest_isolation.pyand_share_guest_idintests/web/api/test_agents_management.py— same decode logic, differing only in headers-dict vs raw-token input. Consolidate into one shared helper.
Merge readiness
Nothing here blocks on security grounds. The security-critical isolation mechanism is solid, and its one major UX gap (Finding 1) is now closed. The remaining MEDIUM (expired-token recovery) is a real but narrow edge case: it could reasonably ship now with a fast-follow, or be closed pre-merge with a ~2-line change (adding one string to a Set). Approving — the MEDIUM item and the two simplification notes are non-blocking follow-ups.
| // lands on the terminal error. "Access denied for this guest" is the backend | ||
| // HTTPException.detail surfaced as event.reason on a 4003 close; "Access | ||
| // denied" is use-websocket.ts's fallback when a 4003 carries no reason. | ||
| const SHARE_ACCESS_DENIED_REASONS = new Set([ |
There was a problem hiding this comment.
[MEDIUM · NEW] Expired persisted token isn't recoverable.
Now that this PR persists the guest JWT in localStorage, a visitor returning after the 30-day TTL reuses an expired token. The backend converts ExpiredSignatureError into HTTPException(401, "Invalid share token"), and the connect handler closes 4003 with that reason — but "Invalid share token" isn't in this Set, so the recovery effect never clears the stale auth. The onAuthInvalidated path doesn't cover this either (it fires only on a REST 401/403 from task/create, which a WS-resuming visitor never hits). Result: dead session, recoverable only by manually clearing localStorage.
Two ways to fix:
- Simplest — add
"Invalid share token"to this Set, reusing the existing recovery effect. - Or check
expclient-side inreadPersistedShareAuthbefore reuse and treat an expired token the same as an absent one.
Non-blocking — narrow edge case (requires a 30+ day return with intact storage), but flagging since it has zero current mitigation.
3 participants
What
PR1 of the public-share-channel hardening pass (#973): per-guest isolation.
A public share link is opened by many anonymous visitors who all share the same owner + agent/workforce id.
get_task_for_share_contextauthorized a task solely byTask.id(a guessable sequential PK) + owner + agent/workforce id, with no per-guest claim — so any visitor holding a valid share JWT for the same shared entity could address another visitor's task id and read/continue that conversation. This affects both the agent-share and workforce-share paths; workforce sessions carry more private multi-turn content, so the blast radius is larger.How
Backend
guest_idatPOST /api/share/auth(both agent- and workforce-share paths) and sign it into the guest JWT. The endpoint never accepts a client-suppliedguest_id: share has no secondary credential (unlike widget's embed ticket / widget key), soguest_idis the sole isolation credential and must be server-minted.get_share_chat_userfails closed on tokens that carry noguest_id(those minted before this change) rather than falling back to the old no-isolation behavior.get_task_for_share_contextrequirestask.agent_config["guest_id"]to match the caller's, on top of the existing checks, in both branches (shared helper_require_share_guest_owns_task). This automatically covers the WS connect + per-message revalidation, sinceshare_chat_websocket_endpointroutes through the same function.guest_idinto the task at create time: the agent-share path assigns server keys last so a client-supplied value can't override; the workforce-share path threads it viaextra_agent_config(survives the snapshot merge, which never setsguest_id).Frontend (share only; widget behavior unchanged)
localStoragesoguest_idis stable across reloads, instead of re-authing on every mount (which would mint a new guest each time and make a returning visitor's own tasks fail the check).taskIdbelongs to a different guest (e.g. a pre-migration task): catch the4003/403access denial, drop the staletaskId, and start a fresh session.Migration / scope
guest_idlives in the existingagent_configJSON.guest_id), which is acceptable for anonymous public share (no accounts, no persistence promise).Tests
tests/web/api/test_share_guest_isolation.py: cross-guest isolation for both agent- and workforce-share, distinct server-minted ids per auth, and fail-closed rejection of legacy tokens without aguest_id.tests/web/apisuite passes; ruff / mypy / tsc / eslint clean.Part of #973.