fix(auth): enforce active-profile session IDs - #5198
starship-s wants to merge 6 commits into
Conversation
|
| Filename | Overview |
|---|---|
| api/routes.py | Adds five visibility helpers and inserts the shared preflight guard into all five HTTP verb dispatchers; stream endpoints get per-site stream_id checks; _handle_chat_start gains a precise foreign-session 404 path with a correct empty-placeholder retag carve-out; health endpoint strips session/stream/workspace fields from the run payload. |
| api/config.py | Adds STREAM_SESSION_OWNERS dict + lock and three thread-safe helpers (register/read/unregister) for synchronous pre-worker ownership recording; unregister_active_run now also clears the owner entry to prevent stale entries. |
| api/upload.py | Adds _session_visible_to_active_profile and _reject_invisible_session helpers; applies them in all three upload handlers (attachment, extract, workspace) before any write or workspace resolution occurs. |
| tests/test_session_active_profile_authorization.py | New 513-line regression suite covering all guarded paths: duplicate, file-read, chat-start (persisted foreign, visible-empty retag prevention), all three upload variants, stream-status (active, pre-worker registered, and same-profile), stream-cancel, SSE replay, session-new with prev_session_id, and the unknown-dead-stream fallback. |
| tests/test_run_lifecycle_health.py | Updated to verify that session_id, stream_id, and workspace are absent from the health endpoint run payload; also exercises register_stream_owner/unregister_active_run lifecycle to confirm STREAM_SESSION_OWNERS is cleared on teardown. |
| tests/test_profile_switch_1200.py | Adds _get_active_profile_name monkeypatch to the placeholder-retag test so the active profile matches the requested profile, satisfying the new retag precondition introduced by this PR. |
| tests/test_session_ops.py | Extends _get() helper to accept custom headers and passes a hermes_profile cookie to the status-endpoint test so the active profile matches the session's profile under the new preflight guard. |
| tests/test_run_journal_routes.py | One-line update to the source-text assertion that verifies _run_journal_live_snapshot is called with handler=handler after the signature change in this PR. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{HTTP Verb}
B --> |GET /api/*| C[_guard_request_session_visibility\nsession_id in query]
B --> |POST /api/*| D[_guard_request_session_visibility\nsession_id in body]
B --> |PATCH/PUT/DELETE| E[_guard_request_session_visibility\nsession_id in body]
C --> F{Exempt route?}
D --> F
E --> F
F --> |No - session_id present| G[_session_id_visible_to_request_profile]
F --> |Yes /api/session/import\n/api/session/import_cli\n/api/chat/start| H[Route handler\nwith inline check]
G --> |get_session metadata_only| I{Profile match?}
I --> |Yes / not found| J[Route handler proceeds]
I --> |No: foreign profile| K[404 Session not found]
H --> |/api/chat/start| L[_session_visible_to_active_profile]
L --> |Visible| M[Run session]
L --> |Foreign + empty + requested==active| N[Retag → Run]
L --> |Foreign otherwise| K
B --> |SSE stream endpoints| O[_stream_id_visible_to_request_profile]
O --> P[_stream_id_owner_session_id]
P --> |1. ACTIVE_RUNS\n2. STREAM_SESSION_OWNERS\n3. find_run_summary| Q{Owner found?}
Q --> |No owner| J
Q --> |Owner found| G
B --> |Multipart upload| R[Upload handler\nparse_multipart]
R --> S[get_session]
S --> T[_reject_invisible_session]
T --> |Visible| U[Write / Extract / Workspace]
T --> |Foreign| K
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Incoming Request] --> B{HTTP Verb}
B --> |GET /api/*| C[_guard_request_session_visibility\nsession_id in query]
B --> |POST /api/*| D[_guard_request_session_visibility\nsession_id in body]
B --> |PATCH/PUT/DELETE| E[_guard_request_session_visibility\nsession_id in body]
C --> F{Exempt route?}
D --> F
E --> F
F --> |No - session_id present| G[_session_id_visible_to_request_profile]
F --> |Yes /api/session/import\n/api/session/import_cli\n/api/chat/start| H[Route handler\nwith inline check]
G --> |get_session metadata_only| I{Profile match?}
I --> |Yes / not found| J[Route handler proceeds]
I --> |No: foreign profile| K[404 Session not found]
H --> |/api/chat/start| L[_session_visible_to_active_profile]
L --> |Visible| M[Run session]
L --> |Foreign + empty + requested==active| N[Retag → Run]
L --> |Foreign otherwise| K
B --> |SSE stream endpoints| O[_stream_id_visible_to_request_profile]
O --> P[_stream_id_owner_session_id]
P --> |1. ACTIVE_RUNS\n2. STREAM_SESSION_OWNERS\n3. find_run_summary| Q{Owner found?}
Q --> |No owner| J
Q --> |Owner found| G
B --> |Multipart upload| R[Upload handler\nparse_multipart]
R --> S[get_session]
S --> T[_reject_invisible_session]
T --> |Visible| U[Write / Extract / Workspace]
T --> |Foreign| K
Reviews (4): Last reviewed commit: "fix(auth): guard profile-owned stream ID..." | Re-trigger Greptile
SummaryI pulled the branch into a read-only worktree and read the full diff in Code referenceThe shared preflight short-circuits cleanly on the common path (no session id, unsafe id, or unknown id all pass through to let the existing handler 404), and only emits the uniform 404 when a known session belongs to another profile: # api/routes.py:515-528
def _session_id_visible_to_request_profile(handler, sid) -> bool:
if not isinstance(sid, str) or not sid:
return True
if not is_safe_session_id(sid):
return True
try:
session = get_session(sid, metadata_only=True)
except KeyError:
return True
if not _session_visible_to_active_profile(getattr(session, "profile", None), handler):
bad(handler, "Session not found", 404)
return False
return TrueTwo things I verified by reading the surrounding code rather than trusting the description:
One thing worth confirmingThe guard inspects only the VerificationThe new |
🔬 Gate certification — RED ⛔ (genuine hardening, but leaves a reproduced stream_id bypass)Certified head: What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Codex (reproduce) | SHIP ONLY WITH FIXES — CORE stream_id bypass + SILENT prev_session_id (both verified) |
| Opus (full review) | APPROVE — but did not cover the stream_id axis (flagged prev_session_id non-blocking) |
Full pytest suite (-p no:xdist, rebased) |
11092 passed, 0 failed (green — but doesn't test the stream_id axis) |
| PR's own tests (clean checkout) | 8 passed |
| My verification | confirmed /health:9304 leaks stream_id; stream endpoints authorize by stream_id only |
⛔ Blocking #1 (CORE) — foreign-profile stream_id endpoints bypass the new guard (Codex-reproduced, I confirmed)
The new guard only checks session_id (qs + body) at the dispatcher chokepoint. But /api/chat/stream, /api/chat/stream/status, and /api/chat/cancel authorize by stream_id — a parallel key the guard never inspects — and journal replay resolves the owning session via find_run_summary(stream_id) with no active-profile check. So a user in profile A can subscribe to / status / cancel / replay profile B's active run by its stream_id.
Practically exploitable (I confirmed): public unauthenticated /health builds runs[] from ACTIVE_RUNS and does item.setdefault("stream_id", stream_id) (api/routes.py:9304), with item = dict(raw) typically also carrying session_id. During a foreign run, an attacker polls /health, harvests the live stream_id, then hits the stream endpoints cross-profile. (My live /health showed empty runs[] only because no run was active; the code path populates raw IDs when one is.)
Fix-spec: add a stream-owner visibility guard before status/cancel/live-subscribe/replay — resolve stream_id → session_id from a synchronously-populated owner map + ACTIVE_RUNS/journal fallback, then 404 when _session_visible_to_active_profile fails. AND redact/remove raw stream_id/session_id from public /health runs[] (it's an unauthenticated endpoint).
⚠️ Blocking #2 (SILENT) — prev_session_id on /api/session/new (Codex + Opus + me)
/api/session/new calls commit_session_memory(prev_session_id, …) before the generic guard's session_id-only check applies, so a foreign prev_session_id can trigger lifecycle/memory side effects unguarded. Lower severity (I traced it: it only flushes a cached agent's memory — no foreign content is read back to the requester), but it's a real gap in the boundary the PR is establishing.
Fix-spec: _session_id_visible_to_request_profile(handler, prev_session_id) → 404 before commit_session_memory, or extend the generic guard to cover the prev_session_id alias.
✅ What's correct (the core design is right — keep it)
- ✅ Central-chokepoint enforcement for
session_id:_guard_request_session_visibilitywired into GET/POST/PATCH/DELETE/PUT dispatchers (covers qs + body session_id everywhere) — better than per-endpoint guards. Plus targeted_session_visible_to_active_profiledeep checks (chat/start, file read, etc.) and upload-path guards. - ✅ Deny-before-serve, 404-not-403 (no existence leak), fail-closed. Verified the 8 entry-point tests assert "before_X" ordering.
- ✅ Narrow, correct exemption (
/api/session/import,import_cli,/api/chat/start) — and chat/start has its own deep guard (tested: foreign persisted session → 404 before start_run; body-profile can't retag a visible-empty session). - ✅ Rebases cleanly onto current master; no same-profile regression (suite green).
Recommendation to the next agent
Bounce for the stream_id axis (Blocking #1), then re-gate — this is a strong hardening that's one axis short of complete. The session_id enforcement is correct and well-architected; the gap is that stream_id is a second authorization key the guard doesn't cover, made practically exploitable by the /health ID leak. Both fixes are well-defined (guard stream endpoints + redact /health; guard prev_session_id). Given this is a crown-jewel security boundary (cross-profile isolation, related to the parked #3999), recommend the contributor (or a follow-up) closes the stream_id axis before merge rather than shipping a partial boundary. I applied gate-fail; the stream_id bypass is an objective reproduced defect. Cert valid only at sha:d110605b7c93.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close. Cert valid only at sha:d110605b7c93; a new push invalidates it → re-gate.
4f4d983 to
29466eb
Compare
🔬 Gate re-check — RED ⛔ (re-push did not address the blocker)Re-checked head: The re-push since my gate-fail (
So the documentation correctly acknowledges the gap (it now says other-key routes must self-guard), but the Status: still RED at
The Gate-certifier layer. I did not re-run the full gate for this re-check because the reproduced CORE blocker is verifiably unchanged at the new head (rebase + docstring only). A push that actually guards the stream_id axis will get a fresh full gate. |
67417cf to
b0b72cf
Compare
|
Thanks for the gate read — pushed follow-up commit
Local verification passed: The branch was rebased on current |
🔬 Gate certification — RED ⛔ (main bypass FIXED; one residual public-leak path remains —
|
| Gate | Result |
|---|---|
| Codex (reproduce) | SHIP ONLY WITH FIXES — CORE: /health?deep=1 still leaks raw stream_id |
| Opus (full review) | COMMENT — no blocking; stream_id bypass + plain-/health leak + prev_session_id all fixed (didn't check deep=1) |
Full pytest suite (-p no:xdist, rebased) |
(running; the public-leak finding is the call, not the suite) |
| PR's 17 auth tests (incl. new stream-owner cases) | 17 passed |
| My verification | main bypass FIXED ✓ · plain /health redacted ✓ · deep=1 leak CONFIRMED ⛔ |
✅ Fixed (my prior CORE blocker — verified)
- Stream-endpoint guard wired:
_stream_id_visible_to_request_profileresolvesstream_id → owner_session_id(ACTIVE_RUNS → registered owner map → journal fallback) and 404s a foreign owner, applied at status/cancel/stream/replay (routes.py:2549/11020/11037/14414). New tests defend it:test_chat_stream_status_blocks_foreign_active_stream,_blocks_foreign_registered_stream_before_worker_start(the pre-worker race window),_keeps_same_profile_stream_visible,test_chat_cancel_blocks_foreign_owned_stream_before_cancel_call. So a leaked foreign stream_id can no longer be used cross-profile. - Plain
/healthredacted:_run_lifecycle_healthnowitem.pop("session_id")+item.pop("stream_id")before appending toruns[]. - session_id-axis guard + prev_session_id still hold (Opus confirmed).
⛔ Blocking (CORE, Codex-reproduced + I confirmed) — /health?deep=1 still leaks raw stream_id
_stream_runtime_diagnostics() (routes.py:9335) builds streams.append({"stream_id": str(stream_id), "subscriber_count": …}) — its own docstring says "exposes counts only" but it includes the raw stream_id. This is surfaced by /health?deep=1, which is public (no auth — api/auth.py:51-57, reached after check_auth() in server.py:381). So the prior exploit input (harvest a foreign active run's stream_id) is still publicly available.
Severity: reduced from the original (the new endpoint guard means a leaked stream_id can't be used to access/cancel a foreign stream — those 404), but it's still cross-profile information disclosure on a public endpoint (you learn another profile has an active run + its raw identifiers; Codex notes workspace values may also surface), and it's the same leak class the PR already fixed for plain /health. For a crit=5 isolation primitive, the public surface should be fully closed.
Fix-spec (Codex, correct + matches the PR's own /health fix): remove/redact the raw stream_id (and any session_id/workspace) from _stream_runtime_diagnostics() — keep counts/subscriber/buffer diagnostics only, or use non-reversible ordinal labels. Update tests/test_webui_runtime_diagnostics.py to assert raw IDs are absent from /health?deep=1.
Recommendation to the next agent
Bounce to close the /health?deep=1 leak (Codex's, trivial — same redaction the PR already applied to plain /health), then re-gate. This is a big step forward: the actual cross-profile stream access bypass is fixed and well-tested; the residual is a public info-disclosure of raw identifiers via the deep-health diagnostics. Given crit=5 + the maintainer's most-thorough-fix preference on security primitives, finish the redaction so no public endpoint leaks cross-profile run identifiers. Also routine (Opus): add the CHANGELOG security entry crediting @starship-s. I applied gate-fail; the public leak is reproduced. Cert valid only at sha:b0b72cfa783e.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close. Cert valid only at sha:b0b72cfa783e; a new push invalidates it → re-gate.
✅ Gate certification — GREEN (re-gated at converged head
|
Release v0.51.763 — enforce active profile on request-supplied session IDs (#5198, @starship-s)
|
Shipped in v0.51.763 — thank you @starship-s. 🔒 Merged via a maintainer rebase of your branch onto current One maintainer hardening commit was added on the way in (co-authored back to you): the full Codex regression gate found that the new Full authoritative gate before ship:
Solid, well-structured security fix — registering the stream owner synchronously before publishing the stream was exactly the right call to avoid the worker-startup race. Thanks for the careful work. |
…le-run prune Gate hardening for nesquena#5198: the new stream-owner map leaked entries on three paths that bypass the teardown finally — worker early-return when the stream was cancelled before startup (streaming.py, gateway_chat.py) and the direct ACTIVE_RUNS stale-zombie prune (routes.py). Each now calls unregister_stream_owner. Adds a regression test proving the early-return path no longer leaks. Co-authored-by: starship-s <starship-s@users.noreply.github.com>
The 409 stuck-orphan pattern (Codex nesquena#5345 / nesquena#5198) requires that ACTIVE_RUNS[stream_id] be released *before* the terminal SSE event goes out, so a client draining the queue immediately after the event does not see a false 409 with _diag.in_streams=true + in_active_runs=true + has_pending_user_message=true. Pre-existing fix covered the success path (put('done')) and the error path (put('apperror')). This commit extends the contract to every cancel exit (~9 sites in api/streaming.py) and to the gateway's _done/_apperror paths (api/gateway_chat.py). Plus it makes cancel_stream() actually fulfill its eager-release promise by unregistering ACTIVE_RUNS at cancel time rather than waiting for the worker's `finally` block (which only runs after agent.interrupt()'d blocking tool returns — often minutes for wedged providers). Three files touched: - api/gateway_chat.py (15 lines): unregister_active_run before done/apperror in both _run_gateway_chat_streaming exit paths. - api/streaming.py (137 lines): unregister_active_run before each cancel event emission (9 sites), plus the early done/apperror sites (already had it; this commit standardises the pattern with the cancel sites). cancel_stream() now unregisters ACTIVE_RUNS before returning. - tests/test_stale_stream_cleanup.py (519 lines, 6 new tests): - test_streaming_cancel_paths_unregister_active_run_before_event: pins the "every put('cancel') is preceded by unregister_active_run(stream_id)" contract. - test_cancel_stream_function_eagerly_unregisters_active_runs: pins cancel_stream() drops ACTIVE_RUNS before returning. - test_chat_start_proactive_cancel_when_blocking_stream_is_stale: pins /api/chat_start proactively cancels a past-ceiling worker. - test_chat_start_409_includes_v2_diag_with_age_and_grace: pins v2 diag schema (effective_started_at, orphan_grace_s, etc). - test_chat_start_dedupes_rapid_double_fire_within_2s: pins fresh-stream dedupe for SSE-reconnect / Enter+click races. - test_chat_start_does_not_dedupe_running_stream: pins the inverse — only dedupe on phase ∈ {None, "starting"}. Refs: ZKREQ nesquena#5345 / nesquena#5198 follow-up
Records the design + post-mortem for the 409 stuck-orphan pattern
work in this branch:
- 4 409 exit paths in _start_chat_stream_for_session
(session_active_stream_id / session_active_stream_id_locked /
active_runs / stale_cleanup_failed) and why they each fire.
- 3 false-positive scenarios the v2 diag must distinguish:
stuck-orphan worker (needs proactive cancel), legitimate in-flight
turn (user should wait), rapid double-fire (fresh dedupe).
- v2 diag payload (diag_version, effective_started_at, orphan_grace_s,
etc) the test_stale_stream_cleanup.py suite now pins.
References ZKREQ nesquena#5345 / nesquena#5198 and the fresh-dedupe follow-up. The
beaf099 commit (streaming + gateway + tests) implements this RFC.
Thinking Path
session_idand loaded or mutated that session directly.404, while duplicate/delete/file/upload/chat-start paths could reach the foreign sidecar first.session_idinputs, then keep explicit route-level handling only where ownership is created or retagged intentionally.session_idfrom form fields before the normal JSON-body preflight can run.POST /api/chat/startremains special: persisted foreign sessions now 404 before a run can start, while empty placeholders can only be retagged when the requested profile matches the server-selected active profile.What Changed
api/routes.pythat checks query/bodysession_idvalues against the active profile before normal API route dispatch.404 Session not foundresponse shape used byGET /api/sessionfor foreign-profile session references.chat/start, where the route has additional ownership or placeholder-retag rules.api/upload.pybefore attachment writes, archive extraction, or workspace resolution.Why It Matters
Verification
Targeted regression coverage:
./scripts/test.sh tests/test_session_active_profile_authorization.py tests/test_issue4067_import_cli_cross_profile_guard.py tests/test_sprint1.py::test_session_delete_nonexistentResult:
12 passedCI-failure reproduction sweep after follow-ups:
./scripts/test.sh tests/test_anchor_scene_persistence.py tests/test_extension_status_endpoint.py tests/test_issue2914_truncation_watermark.py tests/test_metadata_save_wipe_1558.py tests/test_session_ops.py::test_status_returns_profile_specific_hermes_home tests/test_session_truncate_keep_count_validation.py tests/test_watermark_advance_after_edit.py tests/test_profile_switch_1200.py::test_chat_start_retags_empty_session_to_request_profile tests/test_session_active_profile_authorization.py tests/test_issue4067_import_cli_cross_profile_guard.py tests/test_sprint1.py::test_session_delete_nonexistentResult:
132 passedStatic / hygiene checks:
./.venv/bin/ruff check tests/test_session_active_profile_authorization.pyResult:
All checks passed!./.venv/bin/ruff check --select E9,F821 api/routes.py api/upload.py tests/test_session_active_profile_authorization.py tests/test_session_ops.py tests/test_profile_switch_1200.pyResult:
All checks passed!git diff --checkResult: passed with no output.
python3 scripts/ruff_lint.py --diff origin/masterResult:
ruff_lint: no new violations on added/modified lines. OK.Risks / Follow-ups
session_idshould add explicit route-specific handling rather than bypassing this guard implicitly.all_profilesand import-specific checks.Model Used