feat(crit): least-privilege thread-scoped endpoints + WS gap closure - #24
Merged
Conversation
Least-privilege follow-up: replace the wrapper's owner-gated orchestration calls with POST /api/crit/turn + GET /api/crit/turn-status authorized by session subject === threadId, so the sidecar token drops from role:owner to role:client + subject:threadId. Documents the async-turnId constraint and the server-side baseline (Option A) correlation design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ints Add CritTurnRequest, CritTurnResponse, and CritTurnStatusResponse Effect schemas (plus inferred types and tests) used by the new thread-scoped crit server endpoints. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add narrow, subject-bound crit endpoints the sidecar wrapper uses instead of the broad owner-gated orchestration routes. Authorization requires session.subject === threadId (the subject binding is the capability; role is deliberately not checked). POST /api/crit/turn captures the baseline priorTurnId, builds a thread.turn.start command server-side, normalizes and dispatches it. GET /api/crit/turn-status resolves the started turn via the pure compute_turn_status race logic. Register both layers in makeRoutesLayer. Tests cover every compute_turn_status branch plus load-bearing security properties: a subject-bound token reaches only its own thread (200); a token for thread A is rejected (403) on both endpoints for thread B; missing/invalid token -> 401; and the client-role token stays refused by the owner-gated /api/orchestration/dispatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Change the per-sidecar bearer token from role:"owner" to
role:"client" bound to subject:threadId. The token is authorized
only by the dedicated /api/crit/{turn,turn-status} endpoints
(session.subject === threadId), so a leaked token can act on nothing
but its own thread, while owner-gated /api/orchestration/* endpoints
reject the client role and stay unreachable. Keep the 2h TTL and the
revoke-on-teardown scope finalizer unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>…ints Remove snapshot/baseline/stale-turn logic from the crit agent wrapper and drive the new thread-scoped server endpoints instead: POST /api/crit/turn to start the turn (returns the opaque priorTurnId) and poll GET /api/crit/turn-status until completed/error/terminal or timeout. Server now owns turn correlation, so the wrapper only formats crit stdin into the <review_comment> block text and relays status. Update tests to mock the two new endpoints and cover completed, pending->completed, and timeout paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- GET /api/crit/turn-status: decode the query threadId through the Effect error channel (Schema.decodeUnknownEffect) instead of the throwing ThreadId.make(...). A whitespace-only threadId (e.g. ?threadId=%20) trims to "" and previously threw a schema defect that Effect.catchTags does not catch, surfacing as a pre-auth 500. Now mapped to a clean 400. - Correct the least-privilege overclaim: the NOTE in crit-sidecar-manager.ts and the design doc asserted the sidecar token "can act on nothing but its own thread". That is false — the WebSocket RPC surface (POST /api/auth/ws-token + GET /ws) has no role/subject gate and still reaches orchestrationEngine.dispatch. Reworded both to state the HTTP orchestration endpoints are scoped while the WS path remains broadly reachable, flagged as a tracked follow-up. - Encode the residual WS reachability as an asserted-limitation test and add 400 coverage for whitespace-only / missing threadId in critHttp.test.ts. The load-bearing cross-thread (403) and owner-endpoint-unreachable tests remain and pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add "thread-scoped" to both SessionRole definitions so the crit sidecar session (role:"thread-scoped", subject:threadId) can be minted and the web can decode session state including it. Denied at broad surfaces separately; this only widens the union. bySessionPriority still orders owner first with thread-scoped/client as peers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…chments Add a centralized denyThreadScopedRealtime helper near the auth service and apply it at the three broad surfaces so a dedicated crit sidecar role cannot mint a realtime connection or read arbitrary attachments: - issueWebSocketToken (POST /api/auth/ws-token) -> 403 - authenticateWebSocketUpgrade (/ws) -> 403 (defense-in-depth for pre-minted tokens) - attachmentsRouteLayer (GET /api/attachments/*) -> 403 Widen the session role schemas (JWT claims, persistence record/row/create) to admit the thread-scoped literal so the role can actually be minted. Owner and ordinary client sessions are unaffected; both still mint ws-tokens and pass the upgrade, proven by regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Flip the crit sidecar session from role:"client" to the dedicated
least-privilege role:"thread-scoped" now that the broad surfaces deny it.
The token is bound to subject:threadId and can reach ONLY the
/api/crit/{turn,turn-status} endpoints for its own thread; it is denied at
POST /api/auth/ws-token, the /ws upgrade, GET /api/attachments/*, and the
owner-gated /api/orchestration/* endpoints. Keep the 2h TTL and the
revoke-on-teardown finalizer unchanged. Rewrite the NOTE/CAVEAT: the
confinement guarantee now HOLDS and is no longer a residual gap.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>The sidecar token is role:"thread-scoped"+subject:threadId. Update
critHttp.test.ts so the sidecar-equivalent tokens (cross-thread 403,
owner-endpoint refusal) are issued as "thread-scoped", and remove the
stale KNOWN-LIMITATION ws-token test that expected a 200 — the
thread-scoped->403-at-ws-token property is now asserted positively by
"POST /api/auth/ws-token rejects a thread-scoped session with 403".
Replace the spec's "Residual gap" WS caveat with an accurate
authorization model: accepted only by /api/crit/turn{,-status}
(subject===threadId), denied at /api/auth/ws-token, /ws and
/api/attachments/*; owner endpoints unreachable; owner/client sessions
unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>- AuthControlPlane.createPairingLink: narrow role to owner|client (pairing links are never thread-scoped) so it satisfies BootstrapCredentialRole. - critHttp.test.ts: brand the mock instanceId via ProviderInstanceId.make, and type with_app's run/token callbacks concretely (AuthControlPlane | HttpClient) instead of 'any' in the requirements channel. These were missed earlier because the per-package ./node_modules/.bin/tsgo path does not exist and silently exits 0; the real checker is 'bun run typecheck'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up hardening on top of the merged PR #22 (crit PR-review integration v1). Lands the deferred least-privilege work plus the WS escalation-gap closure it surfaced. Flag-gated OFF (
critReviewEnabled, default false); no behaviour change to existing flows.What changed
POST /api/crit/turn+GET /api/crit/turn-status, authorized bysession.subject === threadId(not owner role). The wrapper CLI no longer calls the broad owner-gated/api/orchestration/{dispatch,snapshot}; baseline/snapshot race logic moved server-side into a purecompute_turn_status.thread-scopedsession role — the sidecar token drops fromrole:"owner"→role:"thread-scoped" + subject:threadId(2h TTL + revoke-on-teardown kept).thread-scopedis denied (403) atPOST /api/auth/ws-token, the/wsupgrade, andGET /api/attachments/*via one centralizeddenyThreadScopedRealtimepredicate. Owner + ordinary client sessions (paired "Shared Devices") are unaffected and still use/ws.Verification
role:"client"still gets a ws-token.security-reviewerpass: no High/Medium. Two LOW follow-ups tracked (project-favicon/OTLP reachable by any authenticated session;buildTurnStartCommandUUID PlatformError not mapped).Not in scope (still deferred)
Real crit CLI flags, agent_cmd stdin format, per-platform binary bundling, live E2E — needs a buildable crit (Go unavailable). Design:
docs/superpowers/specs/2026-06-07-crit-dedicated-endpoint-design.md.🤖 Generated with Claude Code