diff --git a/claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md b/claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md index 2c83d7ec8..eaec4eeb8 100644 --- a/claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md +++ b/claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md @@ -649,6 +649,9 @@ relevant item only if a spike contradicts an assumption. IdP anyway (they expire on their own ≤1 h timer; the hub-side `sub_denylist` deferred to future work is the right closure for that window, same as it is for stolen-token-without-clear). + *(2026-08-03 update: shipped as bd-jkih1ql7 — the revocation + ledger now gates the Bearer path; see + `2026-08-03-bearer-revocation-and-mcp-auth-followups.md` F1.)* - **Best-effort: revocation failure does NOT block local cleanup.** Network errors, 5xx, expired-tokens-returning-200/400-with- `invalid_token` — the local delete proceeds regardless. The @@ -992,6 +995,10 @@ relevant item only if a spike contradicts an assumption. hub for up to ≤1 h (ID) / indefinitely (refresh, until user revokes grant). Closing the ID-token window still requires the hub-side `sub_denylist` deferred from v1. + *(2026-08-03 update: the ID-token window is now closed for + hub-side events — bans and logout-everywhere gate the Bearer + path (bd-jkih1ql7). The refresh-token residual stands; see + `2026-08-03-bearer-revocation-and-mcp-auth-followups.md` F1.)* - **Unchanged:** brand-confusion residual. If an attacker already has code execution on the victim's machine, they can drive a real loopback flow under our `client_id` and capture @@ -1188,4 +1195,10 @@ unchecked above): it. Revisit if we move to self-hosted OIDC. - **`sub_denylist` on the hub side** to close the ≤1 h stolen-ID-token window. Already noted as future work in the existing device-flow - plan; cross-listed here. + plan; cross-listed here. **Done (2026-08-03, bd-jkih1ql7):** rather + than a separate denylist, the existing revocation ledger (bans + + logout-everywhere `not_before` floors) is enforced on the Bearer + path — 403 `user_banned` / 401 `bearer_revoked`, anchored at the + token's `iat`, failing closed when `iat` is absent. Refresh-token + theft remains a Google-side revocation matter. See + `2026-08-03-bearer-revocation-and-mcp-auth-followups.md` F1. diff --git a/claude-notes/plans/2026-08-03-bearer-revocation-and-mcp-auth-followups.md b/claude-notes/plans/2026-08-03-bearer-revocation-and-mcp-auth-followups.md new file mode 100644 index 000000000..e01a2916c --- /dev/null +++ b/claude-notes/plans/2026-08-03-bearer-revocation-and-mcp-auth-followups.md @@ -0,0 +1,395 @@ +# Auth review follow-ups: Bearer revocation parity, MCP reconnect auth classification, /auth/me exp semantics + +**Status:** implemented — all three findings landed 2026-08-03; strands and +epic closed. **Date:** 2026-08-03. +**Epic:** `bd-rk55baiz`. **Child strands:** F1 `bd-jkih1ql7` · F2 `bd-l3b1brn8` · +F3 `bd-aw8f3sp8` (all closed). +**Branches:** integration `feature/auth-review-followups` (not yet pushed); +topic branches `braid/bd-jkih1ql7-bearer-revocation-ledger`, +`braid/bd-l3b1brn8-mcp-reconnect-auth-classification`, +`braid/bd-aw8f3sp8-auth-me-exp-discriminator`, each merged `--no-ff` per the +worktrees convention. + +## Overview + +An auth review surfaced three real gaps. This plan fixes them: + +1. **F1 (security, hub):** the Bearer path never consults the revocation + ledger. Bans and `logout-everywhere` only affect session cookies — in a + no-allowlist public deployment, a banned user keeps full MCP access + indefinitely, and a stolen Google ID token survives `logout-everywhere` + for up to ~1 h. Already noted as future work ("`sub_denylist`") in + `2026-05-28-hub-mcp-loopback-pkce.md`; the ledger shipped since (C5), so + the fix is now a wiring job, not a new store. +2. **F2 (robustness, MCP client):** a 401/403 on the WS upgrade — or a + terminal refresh failure (`invalid_grant` → `ReauthRequired`) — is + indistinguishable from a network blip: + `NodeWebSocketClientAdapter.openSocket` swallows `getBearer` failures and + the retry loop spins silently forever. The SPA got an evidence-based probe + for exactly this shape (`bd-3o8zmz46`, `useAuthProbe`); MCP has no + equivalent, so a revoked grant mid-session presents as an immortal, silent + "offline". +3. **F3 (semantics, hub + SPA):** `AuthMeResponse.exp` means "sliding session + expiry" on the cookie path but "fixed Google token expiry" on the Bearer + path, with nothing in the response distinguishing them + (`server.rs:1268-1288`). Latent trap for any future Bearer caller; also + the SPA still carries the dead pre-sliding `DEFAULT_SESSION_MS = 1 h` + fallback (`useAuth.ts:42`), which would mis-schedule (~168× too often) if + it ever fired. + +Sequencing: **F1 → F2** (F2's end-to-end "banned mid-session" case and its +403 handling exercise F1's new hub behavior). **F3 is independent** and can +land any time. F1 is Rust-only; F2 is TS-only (plus e2e); F3 touches both, +lightly. + +## F1 — enforce the revocation ledger on the Bearer path (`bd-jkih1ql7`) + +### Design + +- **Where:** the Bearer *credential* path only — i.e. the path reached from + `authenticate_credential`'s `Credential::Bearer` arm + (`context.rs:823-840`). **Not** the mint-time validation path: + `auth_callback`/`auth_session` validate an incoming Google token through + the same `authenticate_claims` machinery, and they must keep their existing + semantics (bans already gate mint explicitly; the `not_before` floor is + handled there by the `min_auth_time` clamp so same-second re-login works — + a raw `iat < not_before` check at mint would break exactly that). + Pick the seam at implementation under one hard constraint: **`auth_ok` + must not be emitted before the ledger check passes** (today it is emitted + at `context.rs:663-670`; a check bolted on after the call would leave an + `auth_ok` + deny pair in the audit log). Note a bearer-specific *wrapper* + cannot satisfy this as-is — `auth_ok` is emitted inside + `authenticate_claims_for_kind`, which the mint callers share — so the + practical seam is an explicit enforce/skip parameter on + `authenticate_claims_for_kind` (the ledger is already on `self` via + `revocations()`), with the check inserted between the allowlist check + (`:641-661`) and the `auth_ok` event. Mint callers + (`authenticate_claims`) pass skip. +- **Checks**, mirroring `authenticate_session` (`context.rs:749-781`), using + `RevocationLedger::check(sub, anchor)` (`revocation.rs:136-143`) with the + Google token's `iat` as the anchor: + - banned `sub` → **403**, audit `detail = "user_banned"`, + `credential_kind = "bearer"`; + - `iat < not_before[sub]` → **401**, audit `detail = "bearer_revoked"` + (deliberately not `session_revoked` — it isn't a session); + - **missing `iat` fails closed**: `OidcClaims.iat` is `Option` + (`auth.rs:210` — required by OIDC and always sent by Google, but the + type admits absence). Anchor with `claims.iat.unwrap_or(0)` so any + `not_before` entry kills an `iat`-less token (the ban check is + `iat`-independent anyway). +- **Honest scope (document it, don't oversell):** the `not_before` check + kills outstanding *ID tokens* issued before a `logout-everywhere` — closing + the documented ≤1 h stolen-token window. It does **not** kill a stolen + *refresh token*: a refresh grant mints a fresh `iat` that passes. The + hub-side lever for a hostile identity is the **ban**; Google-side + revocation (`authenticate_clear` / RFC 7009) remains the refresh-token + lever. A legitimate MCP client caught by `logout-everywhere` self-heals on + its next refresh (fresh `iat` ≥ `not_before`) — same "immediate re-login + works" semantics as the browser. +- **Unchanged:** WS validate-once (a ban still doesn't sever a live socket; + restart remains the operator remedy — `server.rs:1584-1590`), allowlist + checks, azp/iat validation, the dual-credential 400, and all mint paths. + `q2 provide-hub` (`quarto-hub-provider`) inherits enforcement automatically + since it presents the same Bearer. + +### Work items (TDD) + +- [x] Tests first (extended `auth_bearer.rs` with a `revocation_setup()` + fixture — pre-written `revocations.json`, per-sub `not_before` floors + anchored to the fixture instant; `support.rs` gained + `TestHubBuilder::not_before_subs` and `ClaimsBuilder::no_iat`; + observed 5/6 failing pre-fix, the self-heal 200 already passing as + expected): + - banned `sub` + otherwise-valid Google Bearer → 403 on `/health` **and** + on the WS upgrade; audit shows `user_banned` / `credential_kind=bearer` + (`bearer_banned_sub_returns_403`, `ws_upgrade_with_banned_bearer_returns_403`); + - `not_before` floor then a Bearer whose `iat` predates it → 401, audit + `bearer_revoked` (`bearer_with_iat_before_not_before_returns_401`); + - a Bearer with **no `iat` claim** while a `not_before` entry exists → + 401 (`bearer_without_iat_fails_closed_when_not_before_exists`); + - a Bearer minted *after* the revocation instant → 200 + (`bearer_minted_after_revocation_authenticates`); + - mint regression (`bearer_revocation_does_not_leak_into_mint_path`): + the same credential that 401s as a Bearer still mints via + `POST /auth/session` — the shared-machinery mint path; `/auth/callback` + is Google-provider-only (sealed login-state cookie) and uses the same + `authenticate_claims` + `min_auth_time` clamp. Both deny-tests also + pin the audit ordering (no `auth_ok` for a denied sub); + - session-path regression: all 448 quarto-hub tests green, including + `ban_gates_verify_and_mint` and + `logout_everywhere_kills_prior_tokens_and_relogin_works`. +- [x] Implemented: `RevocationEnforcement { Enforce, Skip }` parameter on + `authenticate_claims_for_kind`; ledger check inserted between the + allowlist check and the `auth_ok` emission, anchored at + `claims.iat.unwrap_or(0)`; the Bearer dispatch arm passes `Enforce`, + `authenticate_claims` (both mint callers) passes `Skip`. +- [x] Docs: `ts-packages/quarto-hub-mcp/README.md` residual-window + paragraph rewritten (window closed for hub-side events; + refresh-token caveat stated); `dev-docs/quarto-hub/session-auth-operations.md` + updated in three spots (model paragraph, revocation section, + audit-detail list gains `bearer_revoked`); all three `sub_denylist` + notes in `2026-05-28-hub-mcp-loopback-pkce.md` annotated. +- [x] `cargo nextest run --workspace`: 10863 passed, 0 failed. + `cargo xtask verify --skip-hub-build`: pass (see session log). +- [x] E2E per policy: `scripts/hub-bearer-revocation-e2e.mjs` (committed, + sibling of `hub-sliding-sessions-e2e.mjs`) — mock IdP + real + `target/debug/hub`; baseline 200/101 for three subs; stopped-hub + write of `revocations.json` (ban sub A, `not_before` floor for + sub B); restart; observed: banned A → 403 on `/health` **and** the + WS upgrade (fresh token too), B pre-floor token → 401 on both, + B fresh-iat token → 200 (self-heal), untouched C → 200/101. + Invocation: `cargo build --bin hub && node scripts/hub-bearer-revocation-e2e.mjs` + → `ALL CHECKS PASSED` (12/12, 2026-08-03). Additionally the + full-stack MCP e2e (`ts-packages/quarto-hub-mcp/src/e2e-auth.test.ts`: + real hub binary + real keyring + loopback PKCE + Bearer WS) passes + against the F1-patched hub. + +## F2 — MCP-side auth classification on reconnect (`bd-l3b1brn8`) + +### Design + +Split evidence from policy, mirroring the SPA's `bd-3o8zmz46` invariant +(*only a reachable server's definitive 401/403 changes auth state; network +errors never do*): + +- **Adapter reports evidence** (`quarto-sync-client`): + - Widen the factory/`WebSocketLike` seam (`NodeWebSocketClientAdapter.ts:50-73`) + so the default `ws` factory can surface a non-101 upgrade status. Note: + `ws` exposes this via the **EventEmitter-only `'unexpected-response'` + event** — it is not reachable through `addEventListener`, so the default + factory must attach it natively and translate it into the seam (an + optional capability; test fakes without it keep working). With no + `'unexpected-response'` listener, `ws` folds the status into a generic + `'error'` — which is exactly the current information loss. + **Verified in ws@8 source** (`websocket.js`: `!websocket.emit('unexpected-response', …) && abortHandshake(…)`): + when a listener **is** attached, ws skips `abortHandshake` entirely — + no `'error'` or `'close'` fires for that socket and the underlying + HTTP request is left open. The factory's handler must therefore abort + the handshake itself (destroy the request, drain the response) after + capturing the status, or every failed attempt leaks a connection. + Retry continuity then rests on the adapter's `retryIntervalId` + interval — cleared only in `onOpen`, re-created by `connect()` after a + live-socket close — not on `'close'` from the failed socket. Pin both + with tests: no request leak per failed attempt, and retry survives the + mid-session sequence open → hub closes socket → reconnect gets 403 + via `'unexpected-response'`. + - New optional `onAuthRejected(evidence)` on the adapter options / + `SyncClientAuthOptions` (`types.ts:204-212`), fired on definitive + evidence only: `{ kind: 'upgrade-status', status: 401 | 403 }` or + `{ kind: 'token-refresh-terminal' }` (a `ReauthRequired` thrown by + `getBearer` — today swallowed at `openSocket`, `:184-190`). Debounced to + one report per failure episode — an episode ends at the next successful + open (`peer-candidate`), which resets the debounce; plain network + close/error never fires it. **Classification is by + `error.name === 'ReauthRequired'`**: sync-client cannot import the + class (the dependency direction is hub-mcp → sync-client), and + `refresh-manager.ts:81-88` already stamps + `override readonly name = 'ReauthRequired'`. Document that as the + cross-package contract. `TokenRefreshError` (structured non-`invalid_grant` + IdP failures — transient or config, per `buildTokenRefreshMessage`) must + **not** be treated as terminal. Plumbing: `buildWsAdapter` + (`client.ts:130-152`) currently forwards only `getBearer` + + `retryInterval` to the adapter — forward the new callback alongside. + - On `token-refresh-terminal`, stop the retry loop (today it spins forever + calling a `getBearer` that will throw every time). On upgrade-status + evidence, keep retrying — policy below may fix the token and the next + attempt succeeds. +- **Connection manager owns policy** (`quarto-hub-mcp`, + `connection-manager.ts`): coalesce concurrent reports — multiple project + adapters share one manager and will all fire after a hub-wide event, so at + most one forceRefresh+reprobe cycle runs at a time. On + `upgrade-status: 401` → one `forceRefresh()` + + `probeAuth` (reusing the `:484-501` pattern; that code is pre-connect — + the mid-session handler is a new method reusing the same pieces); if the + probe then passes, do nothing (the + adapter's retry picks up the fresh token via `getBearer`). If it still + 401s → `rm.invalidate()` + disconnect the project handle + set a + `reauth-required` state so the **next tool call returns the existing + `ReauthRequired` message immediately** instead of hanging into the 15 s + peer timeout. On `403` → terminal "your account is not allowed on this hub + (banned or not allowlisted)" state; **keyring kept** (credentials are + valid; identity is denied — re-auth won't help, so don't wipe). Also map a + 403 from the *initial connect probe* (today: `Unexpected status 403`, + `:511-513`) to the same clear message. +- Surface through the existing `SyncClientCallbacks.onError` seam + stderr; + no new MCP protocol surface. +- **Bundle-safety constraints:** `ws` must stay out of browser bundles (the + lazy import in `client.ts:130-152` is the guard — don't disturb it); + hub-client bundles `quarto-sync-client` from source, so + `npm run build:all` must pass. + +### Work items (TDD) + +- [x] Tests first, sync-client (7 new specs in + `NodeWebSocketClientAdapter.test.ts`, 4 observed failing pre-fix, 3 + pinning must-stay behavior): upgrade-401 fires `onAuthRejected` exactly + once per episode with reset on peer handshake; mid-session 403 with no + close event (the ws unexpected-response shape) keeps the interval retry + alive; network close/error fires nothing and keeps retrying; + `ReauthRequired`-named `getBearer` failure fires `token-refresh-terminal` + and stops the retry loop; `TokenRefreshError`-named failures stay + transient; a factory without the status capability degrades to today's + behavior; plus a REAL-`ws` spec against a raw net server that answers + 403 and keeps the connection open — pins both the status surfacing and + the no-connection-leak invariant. +- [x] Tests first, hub-mcp (10 new specs in `connection-manager.test.ts`, + all observed failing pre-fix): wiring; 401 → one forceRefresh+reprobe + → silent recovery; persistent 401 → invalidate + reauth-required + + next call rejects `ReauthRequired` with zero network (scripted fetch + exhausted); re-auth self-heal; 403 evidence → keyring intact + + `HubAccessDeniedError` on the re-probe; recheck-403 maps to the same + denial; token-refresh-terminal skips the pointless refresh; concurrent + reports coalesce to one cycle; transient refresh failure is + state-neutral; initial-probe 403 gets the clear message. +- [x] Implemented. Adapter: factory seam gains optional `onUpgradeStatus` + capability; the default `ws` factory attaches the EventEmitter-only + `'unexpected-response'` natively and aborts the handshake itself + (drain + destroy request + destroy the captured TCP socket — verified + empirically that a no-op handler leaks and that ws skips + `abortHandshake` when a listener exists); `AuthRejectionEvidence` + reported via `onAuthRejected`, episode-debounced, reset at + peer-candidate; ReauthRequired-by-name classification; `authTerminal` + stops the loop. Found+fixed adjacent hazard: `disconnect()` during a + CONNECTING real socket removed listeners then `close()`d, turning + ws's "closed before established" error event into an + uncaughtException — a swallow-only error listener now guards it. + Manager: `handleAuthRejected` (coalesced), `enterReauthRequired` / + `enterDenied` / `dropDeadProjects`, `gateAuthState` fail-fast with + store-presence self-heal, `HubAccessDeniedError` replacing + `Unexpected status 403`, wiring via `buildAuthOptions`. +- [x] `e2e-auth.test.ts` extended (real hub binary + mock IdP + real + keyring): grant revoked mid-session (IdP `invalid_grant`, 45 s TTL + forces refresh) → next tool call returns the ReauthRequired message in + <10 s and the keyring is wiped; ban `test-subject-1` mid-session + (stopped-hub `revocations.json` write + restart, F1's enforcement) → + WS reconnect 403 → adapter evidence → stderr terminal message → next + tool call answers with the banned/allowlist message in <10 s, keyring + kept. `TEST_REFRESH_TOKEN` exported from `test-idp.ts` for the + revocation hook. +- [x] Verification: sync-client vitest 137/137; hub-mcp vitest 246 passed / + 3 platform-skipped; `cd hub-client && npm run build:all` ✓ and + `npm run test:ci` 130/130 (sync-client bundles from source; the lazy + `ws` import untouched); `cargo xtask build-hub-mcp-bundle && cargo + build --bin q2` → `q2 mcp --launcher-info` shows the fresh embed + (bundle-hash 0fa70f4f3a9bbdd4, gitCommit = HEAD). +- [x] E2E through the real binaries: `npx vitest run src/e2e-auth.test.ts` + (ts-packages/quarto-hub-mcp) → 3/3 passed (2026-08-03), with channel B + driving the real `q2 mcp` launcher (embed fresh; no fallback notice). + Observed outputs: `create_project` after revocation → "…credentials + have expired or were revoked. Ask me to authenticate again." ; + post-ban stderr → "[hub-mcp] Your account is not allowed on this + Quarto Hub…" ; post-ban `read_file` → the same denial message. + +## F3 — discriminate `/auth/me` `exp` (`bd-aw8f3sp8`) + +### Design + +- Add a discriminator to `AuthMeResponse` (`server.rs`, struct at `:1208`, + handler at `:1268-1288`): `credential: "session" | "bearer"` (naming: match the + `AuthenticatedUser` variants; final name at implementation). Document + `exp` as *the expiry of the presented credential* — sliding for sessions, + fixed for Bearer. Additive; no field removed, Bearer keeps returning `exp`. +- SPA: extend `authService.AuthState` with the field; **remove the dead + `DEFAULT_SESSION_MS = 1 h` fallback** in `useAuth.ts:42` — when `exp` is + absent, schedule *no* expiry re-check (the mount check, visibility-change + re-check, hourly `useSessionKeepAlive`, and disconnected `useAuthProbe` + all remain; a sliding-session hub always reports `exp` — the server field + is a non-optional `i64` — so the fallback is unreachable today and + 168×-too-frequent if it ever weren't). The tests currently pinning the + 1 h fallback are `useAuth.test.tsx:186` and `:204` + (`vi.advanceTimersByTime(3600 * 1000 + 2000)`). + +### Work items (TDD) + +- [x] Tests first, hub integration (extended the two existing `/auth/me` + tests in `session_auth.rs`, both observed failing pre-fix): + `auth_me_returns_sliding_exp_from_session` now asserts + `credential == "session"`; `auth_me_supports_bearer` asserts + `credential == "bearer"` alongside the token's own `exp`. +- [x] Tests first, hub-client (both observed failing pre-fix): + `authService.test.ts` gains a `fetchAuthMe` mapping test + (`exp` → `expiresAt` ms + `credential` passthrough); + `useAuth.test.tsx`'s two 1 h-fallback-pinned tests rewritten to + schedule from an explicit server `expiresAt` (behavior coverage + kept), plus a new spec: absent `exp` → zero re-checks across a + simulated week (the retired fallback would have fired ~168×). +- [x] Implemented: `AuthMeResponse.credential: &'static str` + (`"session"`/`"bearer"`, mirroring the `AuthenticatedUser` + variants) with `exp` re-documented as the presented credential's + expiry; SPA `AuthState`/`AuthMeResponse` gain + `credential?: AuthCredentialKind`, `DEFAULT_SESSION_MS` deleted, + and the expiry-re-check effect skips scheduling when `expiresAt` + is absent (mount/visibility/keep-alive/probe checks unchanged). +- [x] Full `cargo xtask verify`: pass (see session log). E2E through the + real hub binary (scratchpad `auth-me-credential-e2e.mjs`, mock IdP): + Bearer `/auth/me` → 200, `credential:"bearer"`, `exp ≈ now+600 s` + (the token's own); `/auth/session`-minted cookie → 200, + `credential:"session"`, `exp ≈ now+7 d` (sliding). All checks passed + (2026-08-03). hub-client changelog committed via the two-commit + workflow. + +## Verification (whole epic) + +Per CLAUDE.md: TDD per item (fail → implement → pass), full +`cargo nextest run --workspace` after each finding lands, `cargo xtask +verify` at the epic tip (full, not `--skip-hub-build` — F2/F3 touch +ts-packages/hub-client), and end-to-end through the real binaries with +invocation + observed output recorded in this plan before any strand closes. + +**Done (2026-08-03):** workspace suite green after each finding (10863 +tests); full `cargo xtask verify` passed on the F3 branch, whose tree +equals the epic tip modulo the changelog commit (itself gated by +`npm run test:wasm`, 130/130) and merge commits. One flake encountered +during the first F3 verify — +`admin_collect_lifecycle::collect_lifecycle_quarantine_restore_purge`, +the known case-insensitive-filesystem id-collision class — recorded as a +sighting on `bd-ce1mv6xv`; passed in isolation and in the full re-run. +E2E evidence per finding is recorded in each Work-items section above. + +## Risks + +- **F1 audit-ordering trap:** the ledger check must precede the `auth_ok` + emission inside the claims path — a post-hoc check in the dispatch arm + would log allow-then-deny for the same request. +- **F1 mint-path leakage:** `authenticate_claims` is shared by the mint + endpoints; an unconditional in-place check would break same-second + re-login after `logout-everywhere` (the `min_auth_time` clamp exists + precisely for that). The mint regression test pins this. +- **F2 upstream variance:** `'unexpected-response'` is a `ws`-specific + EventEmitter event; keep it an optional capability of the factory seam so + fakes and any future transport swap degrade to today's behavior, never + crash. +- **F2 false terminals:** wiping the keyring or stopping retries on + non-definitive evidence would strand offline users — the evidence rule + (definitive 401/403 or terminal refresh error only) is the guard; network + errors must remain state-neutral. +- **F1×F2 clock-granularity race (accepted, not mitigated):** the Bearer + anchor is IdP-clock `iat` against hub-clock `not_before = now + 1` — + the session path's mint clamp has no Bearer equivalent (the hub doesn't + mint the token). A refresh landing in the same second as the revocation + (or with the IdP clock behind the hub's by skew) still 401s, and F2's + single forceRefresh+reprobe would then surface a spurious + `ReauthRequired`. Window is ≤1 s plus NTP-level skew; consequence is one + re-run of `authenticate`. Documented here so nobody "fixes" it with a + leeway that re-opens the revocation window. +- **F3 back-compat:** additive field only; do not change `exp`'s presence on + either path. + +## References + +- Review context: `2026-07-06-hub-server-minted-sliding-sessions.md` (the + sliding-session design + "Bearer unchanged" coexistence), + `2026-05-28-hub-mcp-loopback-pkce.md` (MCP auth; `sub_denylist` + future-work note F1 discharges), `2026-06-10-ws-auth-expiry-handling.md` + (the SPA evidence-based probe F2 mirrors), + `2026-06-11-q2-mcp-hub-auth.md` (q2 mcp launcher / bundle rebuild chain). +- Key files: `crates/quarto-hub/src/context.rs` (566-672 Bearer claims path, + 749-781 session revocation handling, 823-840 dispatch), + `crates/quarto-hub/src/revocation.rs` (130-143 ledger API), + `crates/quarto-hub/src/server.rs` (1268-1288 auth_me, 1584-1590 WS + validate-once), + `ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.ts` + (50-73 seam, 181-221 openSocket/onClose), + `ts-packages/quarto-hub-mcp/src/connection-manager.ts` (448-538 probe + + policy), `hub-client/src/hooks/useAuth.ts`, + `hub-client/src/services/authService.ts`. diff --git a/crates/quarto-hub/src/context.rs b/crates/quarto-hub/src/context.rs index 32db11ca0..4d7d73891 100644 --- a/crates/quarto-hub/src/context.rs +++ b/crates/quarto-hub/src/context.rs @@ -553,20 +553,30 @@ impl HubContext { /// mint-time validation in `auth_callback`/`auth_session`, whose /// input is a fresh Google credential from the request body — never /// the cookie (that path is [`Self::authenticate_session`]). + /// + /// Mint-time semantics: the revocation ledger is **skipped** here + /// (see [`RevocationEnforcement::Skip`]) — bans gate mint + /// explicitly in the handlers, and the `min_auth_time` clamp + /// handles `not_before` floors so same-second re-login after a + /// logout-everywhere keeps working. pub async fn authenticate_claims( &self, token: Option<&str>, ) -> std::result::Result { - self.authenticate_claims_for_kind(token, "unknown").await + self.authenticate_claims_for_kind(token, "unknown", RevocationEnforcement::Skip) + .await } /// Variant of [`authenticate_claims`] that records the /// `credential_kind` (`"bearer"` / `"unknown"`) on every audit - /// event, as required by Phase 2 of the device-flow plan. + /// event, as required by Phase 2 of the device-flow plan, and lets + /// the caller pick the revocation-ledger posture (request + /// credentials enforce; mint-time validation skips). pub async fn authenticate_claims_for_kind( &self, token: Option<&str>, credential_kind: &'static str, + revocation: RevocationEnforcement, ) -> std::result::Result { let auth_config = self.auth_config().ok_or_else(|| { tracing::event!( @@ -660,6 +670,46 @@ impl HubContext { return Err(status); } + // Revocation ledger on the request-credential path (bd-jkih1ql7): + // bans and logout-everywhere floors must bite Bearer credentials + // too. Anchored at the token's `iat`; a missing `iat` (the type + // admits it, OIDC requires it) fails closed against any + // `not_before` entry. Must run before the `auth_ok` emission — + // an allow-then-deny pair for one request would corrupt the + // audit log. + if revocation == RevocationEnforcement::Enforce { + let anchor = token_data.claims.iat.unwrap_or(0); + match self.revocations.check(&token_data.claims.sub, anchor).await { + RevocationStatus::Banned => { + tracing::event!( + target: "quarto_hub::audit", + tracing::Level::INFO, + action = "auth_fail", + outcome = "deny", + credential_kind = credential_kind, + sub = %token_data.claims.sub, + detail = "user_banned", + ); + return Err(StatusCode::FORBIDDEN); + } + RevocationStatus::Revoked => { + // Deliberately not `session_revoked` — it isn't a + // session; the dead thing is the Bearer token itself. + tracing::event!( + target: "quarto_hub::audit", + tracing::Level::INFO, + action = "auth_fail", + outcome = "deny", + credential_kind = credential_kind, + sub = %token_data.claims.sub, + detail = "bearer_revoked", + ); + return Err(StatusCode::UNAUTHORIZED); + } + RevocationStatus::Ok => {} + } + } + tracing::event!( target: "quarto_hub::audit", tracing::Level::INFO, @@ -833,6 +883,7 @@ impl HubContext { .authenticate_claims_for_kind( Some(token), crate::server::CredentialKind::Bearer.label(), + RevocationEnforcement::Enforce, ) .await .map(AuthenticatedUser::Google), @@ -840,6 +891,22 @@ impl HubContext { } } +/// Whether [`HubContext::authenticate_claims_for_kind`] enforces the +/// revocation ledger (bans + `not_before` floors) on the validated +/// claims. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RevocationEnforcement { + /// Request-credential path (the Bearer arm of + /// [`HubContext::authenticate_credential`]): a banned `sub` is 403, + /// an `iat` below the user's `not_before` floor is 401. + Enforce, + /// Mint-time validation (`auth_callback` / `auth_session`): bans + /// gate mint explicitly in the handlers, and the `min_auth_time` + /// clamp handles the floor — a raw `iat` check here would break + /// same-second re-login after logout-everywhere. + Skip, +} + /// A successfully validated request credential, tagged with the path /// that verified it. #[derive(Debug, Clone)] diff --git a/crates/quarto-hub/src/server.rs b/crates/quarto-hub/src/server.rs index dd059705e..1dedc9131 100644 --- a/crates/quarto-hub/src/server.rs +++ b/crates/quarto-hub/src/server.rs @@ -1209,9 +1209,18 @@ struct AuthMeResponse { email: String, name: Option, picture: Option, - /// Token expiry (epoch seconds) so the client can schedule silent - /// refresh from the real expiry instead of assuming a fixed lifetime. + /// Expiry (epoch seconds) **of the presented credential** — the + /// semantics depend on `credential`: a *sliding* session expiry on + /// the cookie path (authenticated activity extends it; the SPA + /// schedules its expiry re-check from it), but the Google token's + /// *fixed* expiry on the Bearer path (nothing slides; the client + /// refreshes at the IdP). `credential` is the discriminator + /// (bd-aw8f3sp8). exp: i64, + /// Which verification path authenticated this request: `"session"` + /// (hub-minted cookie) or `"bearer"` (Google ID token). Mirrors the + /// [`crate::context::AuthenticatedUser`] variants. + credential: &'static str, } /// Query parameters for GET /auth/actor. @@ -1276,12 +1285,14 @@ async fn auth_me( name: v.claims.name, picture: v.claims.picture, exp: v.claims.exp, + credential: "session", }, crate::context::AuthenticatedUser::Google(claims) => AuthMeResponse { email: claims.email, name: claims.name, picture: claims.picture, exp: claims.exp, + credential: "bearer", }, }; Ok(Json(response)) diff --git a/crates/quarto-hub/tests/integration/auth_bearer.rs b/crates/quarto-hub/tests/integration/auth_bearer.rs index de79a6e86..27527fcc7 100644 --- a/crates/quarto-hub/tests/integration/auth_bearer.rs +++ b/crates/quarto-hub/tests/integration/auth_bearer.rs @@ -755,6 +755,220 @@ async fn dual_credential_400_wins_over_csrf_and_origin() { assert_eq!(resp.status(), 400); } +// ── Revocation ledger on the Bearer path (bd-jkih1ql7) ─────────── +// +// Bans and logout-everywhere `not_before` floors must bite Bearer +// credentials too, not just session cookies — otherwise a banned user +// keeps full MCP access and a stolen Google ID token survives +// logout-everywhere for its remaining lifetime. The anchor is the +// Google token's `iat`; a missing `iat` fails closed. Plan: +// claude-notes/plans/2026-08-03-bearer-revocation-and-mcp-auth-followups.md (F1). + +/// Hub with pre-written revocation events (the stopped-hub operator +/// procedure): one banned sub, plus per-test `not_before` floors +/// anchored to the fixture's start instant (returned as third element +/// so tests mint `iat`s relative to it, immune to test-order timing). +async fn revocation_setup() -> &'static (MockOidcProvider, TestHub, i64) { + static SETUP: tokio::sync::OnceCell<(MockOidcProvider, TestHub, i64)> = + tokio::sync::OnceCell::const_new(); + SETUP + .get_or_init(|| async { + install_tracing_once(); + let now = chrono::Utc::now().timestamp(); + let provider = MockOidcProvider::start().await; + let hub = TestHubBuilder::new() + .banned_subs(&["banned-bearer-sub"]) + .not_before_subs(&[ + ("revoked-bearer-sub", now - 100), + ("no-iat-bearer-sub", now - 100), + ("self-heal-bearer-sub", now - 100), + // Future floor = the shape a live logout-everywhere + // writes (now + 1); lets the mint-path test present + // a token whose iat provably predates the floor. + ("mint-clamp-bearer-sub", now + 1), + ]) + .start(&provider) + .await; + (provider, hub, now) + }) + .await +} + +#[tokio::test] +async fn bearer_banned_sub_returns_403() { + let (provider, hub, now) = revocation_setup().await; + let token = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("banned-bearer-sub") + .iat(now - 5) + .to_value(), + ); + let resp = hub.get_health().bearer_auth(&token).send().await.unwrap(); + assert_eq!( + resp.status(), + 403, + "banned sub must be refused on the Bearer path" + ); + + let events = snapshot_events(); + assert!( + events.iter().any(|e| { + e.fields.get("action").map(|s| s.as_str()) == Some("auth_fail") + && e.fields.get("credential_kind").map(|s| s.as_str()) == Some("bearer") + && e.fields.get("sub").map(|s| s.as_str()) == Some("banned-bearer-sub") + && e.fields.get("detail").map(|s| s.as_str()) == Some("user_banned") + }), + "expected auth_fail with detail=user_banned, credential_kind=bearer" + ); + // Ordering pin: the deny must happen before the auth_ok emission — + // an allow-then-deny pair for one request would corrupt the audit log. + assert!( + !events.iter().any(|e| { + e.fields.get("action").map(|s| s.as_str()) == Some("auth_ok") + && e.fields.get("sub").map(|s| s.as_str()) == Some("banned-bearer-sub") + }), + "a denied Bearer must not leave an auth_ok event in the audit log" + ); +} + +#[tokio::test] +async fn ws_upgrade_with_banned_bearer_returns_403() { + let (provider, hub, now) = revocation_setup().await; + let token = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("banned-bearer-sub") + .iat(now - 5) + .to_value(), + ); + let resp = hub.ws_upgrade().bearer_auth(&token).send().await.unwrap(); + assert_eq!( + resp.status(), + 403, + "banned sub must be refused on the WS upgrade too" + ); +} + +#[tokio::test] +async fn bearer_with_iat_before_not_before_returns_401() { + let (provider, hub, now) = revocation_setup().await; + let token = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("revoked-bearer-sub") + .iat(now - 200) // predates the now-100 floor + .to_value(), + ); + let resp = hub.get_health().bearer_auth(&token).send().await.unwrap(); + assert_eq!( + resp.status(), + 401, + "a Bearer minted before the not_before floor must be refused" + ); + + let events = snapshot_events(); + assert!( + events.iter().any(|e| { + e.fields.get("action").map(|s| s.as_str()) == Some("auth_fail") + && e.fields.get("credential_kind").map(|s| s.as_str()) == Some("bearer") + && e.fields.get("sub").map(|s| s.as_str()) == Some("revoked-bearer-sub") + && e.fields.get("detail").map(|s| s.as_str()) == Some("bearer_revoked") + }), + "expected auth_fail with detail=bearer_revoked (not session_revoked — it isn't a session)" + ); + assert!( + !events.iter().any(|e| { + e.fields.get("action").map(|s| s.as_str()) == Some("auth_ok") + && e.fields.get("sub").map(|s| s.as_str()) == Some("revoked-bearer-sub") + }), + "a denied Bearer must not leave an auth_ok event in the audit log" + ); +} + +#[tokio::test] +async fn bearer_without_iat_fails_closed_when_not_before_exists() { + let (provider, hub, _now) = revocation_setup().await; + // OIDC requires iat and Google always sends it, but OidcClaims.iat + // is Option — an iat-less token must anchor at 0 and die + // against any not_before entry rather than sail past the check. + let token = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("no-iat-bearer-sub") + .no_iat() + .to_value(), + ); + let resp = hub.get_health().bearer_auth(&token).send().await.unwrap(); + assert_eq!( + resp.status(), + 401, + "an iat-less Bearer must fail closed against a not_before entry" + ); +} + +#[tokio::test] +async fn bearer_minted_after_revocation_authenticates() { + let (provider, hub, now) = revocation_setup().await; + // The self-heal path: a legitimate MCP client caught by + // logout-everywhere refreshes, gets a fresh iat ≥ not_before, and + // is back in — same "immediate re-login works" semantics as the + // browser. + let token = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("self-heal-bearer-sub") + .iat(now - 5) // after the now-100 floor + .to_value(), + ); + let resp = hub.get_health().bearer_auth(&token).send().await.unwrap(); + assert_eq!( + resp.status(), + 200, + "a Bearer minted after the revocation instant must authenticate; body: {:?}", + resp.text().await + ); +} + +#[tokio::test] +async fn bearer_revocation_does_not_leak_into_mint_path() { + let (provider, hub, now) = revocation_setup().await; + // A token whose iat predates the not_before floor: refused as a + // request credential… + let google = provider.sign( + &ClaimsBuilder::from_provider(provider) + .sub("mint-clamp-bearer-sub") + .iat(now - 5) // predates the now+1 floor + .to_value(), + ); + let r = hub.get_health().bearer_auth(&google).send().await.unwrap(); + assert_eq!( + r.status(), + 401, + "pre-revocation iat must be refused as a request credential" + ); + + // …but the same credential still mints a session: the mint path's + // min_auth_time clamp (not a raw iat check) is what handles the + // floor there, so same-second re-login keeps working. + let r = hub + .client + .post(hub.url("/auth/session")) + .header("x-requested-with", "XMLHttpRequest") + .json(&serde_json::json!({ "credential": google })) + .send() + .await + .unwrap(); + assert_eq!( + r.status(), + 200, + "mint must not enforce the Bearer iat floor (min_auth_time clamp)" + ); + let (cookie, _) = TestHub::set_auth_cookie(&r).expect("fresh session cookie"); + let r = hub + .get_health() + .header("cookie", format!("quarto_hub_token={cookie}")) + .send() + .await + .unwrap(); + assert_eq!(r.status(), 200, "post-clamp session authenticates"); +} + // ── Unauthenticated endpoints unaffected ───────────────────────── #[tokio::test] diff --git a/crates/quarto-hub/tests/integration/session_auth.rs b/crates/quarto-hub/tests/integration/session_auth.rs index 3a3fc058e..4b65f58f7 100644 --- a/crates/quarto-hub/tests/integration/session_auth.rs +++ b/crates/quarto-hub/tests/integration/session_auth.rs @@ -1049,6 +1049,10 @@ async fn auth_me_returns_sliding_exp_from_session() { let body: serde_json::Value = resp.json().await.unwrap(); assert_eq!(body["email"], "user@posit.co"); assert_eq!(body["name"], "Session Test User"); + assert_eq!( + body["credential"], "session", + "cookie path must be discriminated as credential=session (bd-aw8f3sp8)" + ); let exp = body["exp"].as_i64().unwrap(); let idle = SessionLifetimes::default().idle_secs; assert!( @@ -1075,6 +1079,11 @@ async fn auth_me_supports_bearer() { let body: serde_json::Value = resp.json().await.unwrap(); assert_eq!(body["email"], "user@posit.co"); assert_eq!(body["exp"], exp, "Bearer path reports the Google exp"); + assert_eq!( + body["credential"], "bearer", + "Bearer path must be discriminated as credential=bearer — its exp \ + is the token's fixed expiry, not a sliding session (bd-aw8f3sp8)" + ); } #[tokio::test] diff --git a/crates/quarto-hub/tests/integration/support.rs b/crates/quarto-hub/tests/integration/support.rs index 7693d273d..b08e06d7d 100644 --- a/crates/quarto-hub/tests/integration/support.rs +++ b/crates/quarto-hub/tests/integration/support.rs @@ -282,6 +282,13 @@ impl ClaimsBuilder { self.iat = Some(iat); self } + /// Omit the `iat` claim entirely — OIDC requires it and Google + /// always sends it, but `OidcClaims.iat` is `Option`, so the + /// fail-closed revocation anchor needs this shape covered. + pub fn no_iat(mut self) -> Self { + self.iat = None; + self + } pub fn exp(mut self, exp: i64) -> Self { self.exp = exp; self @@ -367,6 +374,7 @@ pub struct TestHubBuilder { google_provider: bool, auth_disabled: bool, banned_subs: Vec, + not_before_subs: Vec<(String, i64)>, } impl Default for TestHubBuilder { @@ -385,6 +393,7 @@ impl TestHubBuilder { google_provider: false, auth_disabled: false, banned_subs: Vec::new(), + not_before_subs: Vec::new(), } } @@ -439,6 +448,13 @@ impl TestHubBuilder { self } + /// Pre-write `revocations.json` `not_before` entries (the on-disk + /// shape a `logout-everywhere` leaves behind) before the hub starts. + pub fn not_before_subs(mut self, entries: &[(&str, i64)]) -> Self { + self.not_before_subs = entries.iter().map(|(s, t)| (s.to_string(), *t)).collect(); + self + } + pub async fn start(self, provider: &MockOidcProvider) -> TestHub { // Auth config carries TWO audiences: SPA primary + MCP additional. // Construct directly (bypassing AuthConfig::new) so we can use the @@ -483,12 +499,17 @@ impl TestHubBuilder { std::fs::write(temp.path().join("hub.json"), config.to_string()).unwrap(); } - // Pre-write revocations.json with ban entries (the documented - // stopped-hub operator procedure). - if !self.banned_subs.is_empty() { + // Pre-write revocations.json with ban / not_before entries (the + // documented stopped-hub operator procedure). + if !self.banned_subs.is_empty() || !self.not_before_subs.is_empty() { + let not_before: serde_json::Map = self + .not_before_subs + .iter() + .map(|(sub, ts)| (sub.clone(), serde_json::json!(ts))) + .collect(); let revocations = serde_json::json!({ "version": 1, - "not_before": {}, + "not_before": not_before, "banned": self.banned_subs, }); std::fs::write( diff --git a/dev-docs/quarto-hub/session-auth-operations.md b/dev-docs/quarto-hub/session-auth-operations.md index 11c01bb05..5541804d0 100644 --- a/dev-docs/quarto-hub/session-auth-operations.md +++ b/dev-docs/quarto-hub/session-auth-operations.md @@ -13,9 +13,11 @@ cookie (~400 bytes) — named `__Host-quarto_hub_token` under TLS, or HTTP activity re-issues the cookie (at most ~1/hour), up to an **idle timeout** (default 7 days) and an **absolute lifetime cap** (default 30 days, anchored at login — re-issue can never extend past it). The -MCP Bearer path (`Authorization: Bearer `) is -unchanged. Legacy Google-JWT cookies are rejected (one-time re-login -at the cutover deploy). +MCP Bearer path (`Authorization: Bearer `) keeps its +validate-per-request model, and since `bd-jkih1ql7` it is subject to +the same revocation ledger as sessions (bans, logout-everywhere — see +"Revoking users"). Legacy Google-JWT cookies are rejected (one-time +re-login at the cutover deploy). ## Configuration @@ -185,7 +187,13 @@ records only revocation events: - **Self-service:** `POST /auth/logout-everywhere` (browser session + CSRF header) kills the calling user's entire token family across - devices. Immediate re-login works. + devices — session cookies **and** outstanding Google ID tokens on + the Bearer/MCP path (any token whose `iat` predates the event is + refused with 401 `bearer_revoked`). Immediate re-login works, and an + MCP client self-heals on its next token refresh (fresh `iat`). What + it does **not** kill is a stolen Google *refresh token* — a refresh + mints a fresh, passing `iat`; the levers there are a hub **ban** or + Google-side revocation (the MCP `authenticate_clear` tool / RFC 7009). - **Operator ban:** with the **hub stopped** (or restarting right after), add the user's Google `sub` to the `banned` array: @@ -193,11 +201,13 @@ records only revocation events: { "version": 1, "not_before": {}, "banned": ["1234567890"] } ``` - A ban rejects every session **and refuses new logins** for that - `sub`; it never expires until removed. Never hand-edit while the hub - runs — the hub's own atomic persist can overwrite a live edit. The - restart also severs the banned user's live WebSocket (expiry and - revocation otherwise bite on reconnect, not on open sockets). + A ban rejects every session, **every Bearer/MCP request** (403 + `user_banned`), **and refuses new logins** for that `sub`; it never + expires until removed. Never hand-edit while the hub runs — the + hub's own atomic persist can overwrite a live edit. The restart also + severs the banned user's live WebSocket (expiry and revocation + otherwise bite on reconnect, not on open sockets — true for the + Bearer path too, which validates once at upgrade). - Allowlist removal (`--allowed-emails`/`--allowed-domains`) also bites on the user's next request — but remove-then-re-add is **not** a revocation: unexpired tokens resume working. Use @@ -250,6 +260,7 @@ Auth events on target `quarto_hub::audit` carry legacy Google-JWT cookie, or **two instances that each auto-generated their own secret** — see below), `session_expired`, `session_absolute_cap`, `session_tampered`, `session_revoked`, +`bearer_revoked` (a Google ID token predating a logout-everywhere), `user_banned`, `user_not_allowlisted`, `conflicting_credentials`, `login_state_stale_client` and `login_state_missing` (the two cookie-absent login classes — see "Login nonce" above for their opposite diff --git a/hub-client/changelog.md b/hub-client/changelog.md index d27a80a79..1be1adea4 100644 --- a/hub-client/changelog.md +++ b/hub-client/changelog.md @@ -23,6 +23,10 @@ WASM rebuild is needed for a changelog-only edit. --> +### 2026-08-03 + +- [`bd8f5206`](https://github.com/quarto-dev/q2/commits/bd8f5206): The session-expiry re-check now schedules only from the server-reported expiry; `/auth/me` gained a `credential` field distinguishing sliding sessions from fixed-expiry Bearer tokens, and the obsolete 1-hour fallback lifetime was removed. + ### 2026-07-30 - [`9e65fcdc`](https://github.com/quarto-dev/q2/commits/9e65fcdc): `quartoDebug.openServerInspector()` embeds the standalone Automerge debugger next to the editor, pre-pointed at the current project, so the sync server's view of a document can be compared against the editor's own. diff --git a/hub-client/quarto-hub-sandboxed-preview/package-lock.json b/hub-client/quarto-hub-sandboxed-preview/package-lock.json index 1efad6c86..1e65c7f98 100644 --- a/hub-client/quarto-hub-sandboxed-preview/package-lock.json +++ b/hub-client/quarto-hub-sandboxed-preview/package-lock.json @@ -53,7 +53,6 @@ "integrity": "sha512-RgHBCvtjbOK2gXSNBNIkNoEc9qoVEtau3hj8gEqKQuL3HZAibKarWFEI3Lfm6EYKkLalOh8eSrj9b+ch9H/VBA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/code-frame": "^7.29.7", "@babel/generator": "^7.29.7", @@ -1226,7 +1225,6 @@ "integrity": "sha512-3ek6mwJL5/VBewBcY4S66cqlCtK3qi4WIq37Z0m/NHw1hjhI7274Mx1qz/+ggSzyBCOEf7eHjBN6INjPAWYfYw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@types/prop-types": "*", "csstype": "^3.2.2" @@ -1309,7 +1307,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "baseline-browser-mapping": "^2.10.12", "caniuse-lite": "^1.0.30001782", @@ -1657,7 +1654,6 @@ "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -1699,7 +1695,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", "integrity": "sha512-wS+hAgJShR0KhEvPJArfuPVN1+Hz1t0Y6n5jLrGQbkb4urgPE/0Rve+1kMB1v/oWgHgm4WIcV+i7F2pTVj+2iQ==", "license": "MIT", - "peer": true, "dependencies": { "loose-envify": "^1.1.0" }, @@ -1885,7 +1880,6 @@ "integrity": "sha512-NTKlcQjlAK7MlQoyb6LgaqHc8sso/pVyUJYWMws3jg21uTJw/LddqIFPcPqP6PzpgbIcZyKI85sFE4HBrQDA8A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.4.4", diff --git a/hub-client/src/hooks/useAuth.test.tsx b/hub-client/src/hooks/useAuth.test.tsx index 207e9ac1a..5a54e862f 100644 --- a/hub-client/src/hooks/useAuth.test.tsx +++ b/hub-client/src/hooks/useAuth.test.tsx @@ -173,15 +173,16 @@ describe('useAuth', () => { }); it('clears auth and flags sessionExpired on a definitive 401 at expiry', async () => { - const user = { email: 'a@b.com', name: 'A', picture: null }; + const user = { email: 'a@b.com', name: 'A', picture: null, expiresAt: Date.now() + 3600 * 1000 }; + const freshUser = { email: 'a@b.com', name: 'A', picture: null, expiresAt: Date.now() + 3600 * 1000 }; mockFetchAuthMe .mockResolvedValueOnce(user) // mount .mockResolvedValueOnce(null); // expiry re-check → 401 const { result } = renderHook(() => useAuth(), { wrapper }); - await vi.waitFor(() => expect(result.current.auth).toEqual(user)); + await vi.waitFor(() => expect(result.current.auth).toEqual(freshUser)); - // Default 1 h lifetime (no server exp) → re-check just after +1 h. + // The re-check is scheduled from the server-reported exp (+1 h). await act(async () => { vi.advanceTimersByTime(3600 * 1000 + 2000); }); @@ -191,7 +192,7 @@ describe('useAuth', () => { }); it('keeps auth when the server confirms a still-valid cookie at expiry', async () => { - const user = { email: 'a@b.com', name: 'A', picture: null }; + const user = { email: 'a@b.com', name: 'A', picture: null, expiresAt: Date.now() + 3600 * 1000 }; const freshUser = { email: 'a@b.com', name: 'Still Valid', picture: null }; mockFetchAuthMe .mockResolvedValueOnce(user) // mount @@ -208,6 +209,26 @@ describe('useAuth', () => { expect(result.current.sessionExpired).toBe(false); }); + it('schedules no expiry re-check when the server reports no exp (bd-aw8f3sp8)', async () => { + // A sliding-session hub always reports exp (the field is + // non-optional server-side); when it is genuinely absent there is + // nothing sane to schedule from. The retired 1 h fallback would + // have re-probed ~168× over the week simulated here. + const user = { email: 'a@b.com', name: 'A', picture: null }; // no expiresAt + mockFetchAuthMe.mockResolvedValueOnce(user); // mount — and nothing more + + const { result } = renderHook(() => useAuth(), { wrapper }); + await vi.waitFor(() => expect(result.current.auth).toEqual(user)); + + await act(async () => { + vi.advanceTimersByTime(7 * 24 * 3600 * 1000); + }); + + expect(mockFetchAuthMe).toHaveBeenCalledTimes(1); // the mount check only + expect(result.current.auth).toEqual(user); + expect(result.current.sessionExpired).toBe(false); + }); + it('reschedules from a slid exp and logs out only at the new expiry', async () => { const start = Date.now(); const user = { email: 'a@b.com', name: 'A', picture: null, expiresAt: start + 20 * 60 * 1000 }; diff --git a/hub-client/src/hooks/useAuth.ts b/hub-client/src/hooks/useAuth.ts index 2c1a8d6a9..023b935ee 100644 --- a/hub-client/src/hooks/useAuth.ts +++ b/hub-client/src/hooks/useAuth.ts @@ -22,9 +22,13 @@ * rarely-reached hard boundaries. * * Expiry tracking: /auth/me reports the session's current `exp` - * (`AuthState.expiresAt`, ms epoch — sliding, typically days out; - * falls back to +1 h for older servers). An expiry-time re-check runs - * against the reported `exp`. + * (`AuthState.expiresAt`, ms epoch — sliding, typically days out). An + * expiry-time re-check runs against the reported `exp`. When the server + * reports no `exp` (only conceivable on a pre-sliding hub), no expiry + * re-check is scheduled — the mount check, visibility-change re-check, + * hourly keep-alive, and the disconnected-state auth probe still cover + * session-end detection. (The old 1 h fallback here would have probed + * ~168× too often against a sliding session; bd-aw8f3sp8.) * * Evidence-based logout (bd-3o8zmz46): auth is only cleared when a reachable * server definitively rejects us (401/403). Network errors — refocus checks, @@ -38,9 +42,6 @@ import { useAuthProvider } from '../auth/AuthProvider'; import type { AuthState } from '../services/authService'; import { fetchAuthMe, logout as serverLogout } from '../services/authService'; -/** Assumed session lifetime when the server doesn't report `exp` (1 hour). */ -const DEFAULT_SESSION_MS = 3600 * 1000; - /** Re-check interval when an expiry-time verdict couldn't be reached. */ const EXPIRY_RECHECK_MS = 60 * 1000; @@ -127,11 +128,14 @@ export function useAuth() { }; }, [auth, applyAuth, expireSession]); - // Schedule an expiry-time server re-check from the session's real expiry. + // Schedule an expiry-time server re-check from the session's real + // expiry. No reported exp → nothing to schedule from (the other + // checks — mount, visibility, keep-alive, disconnected probe — still + // run); guessing a lifetime here mis-scheduled badly (bd-aw8f3sp8). useEffect(() => { - if (!auth) return; + if (!auth || auth.expiresAt === undefined) return; - const expiresAt = auth.expiresAt ?? Date.now() + DEFAULT_SESSION_MS; + const expiresAt = auth.expiresAt; // Expiry-time re-check. Only a definitive 401/403 clears the session; // network errors reschedule (logout on evidence, not on schedule). diff --git a/hub-client/src/services/authService.test.ts b/hub-client/src/services/authService.test.ts index c626e43ef..985dac180 100644 --- a/hub-client/src/services/authService.test.ts +++ b/hub-client/src/services/authService.test.ts @@ -79,6 +79,25 @@ describe('authService', () => { }); }); + it('maps exp and the credential discriminator through (bd-aw8f3sp8)', async () => { + const now = Math.floor(Date.now() / 1000); + vi.mocked(fetch).mockResolvedValue({ + ok: true, + status: 200, + json: () => Promise.resolve({ + email: 'a@b.com', + name: 'A', + picture: null, + exp: now + 600, + credential: 'session', + }), + } as Response); + + const result = await fetchAuthMe(); + expect(result?.expiresAt).toBe((now + 600) * 1000); + expect(result?.credential).toBe('session'); + }); + it('returns null on 401', async () => { vi.mocked(fetch).mockResolvedValue({ ok: false, diff --git a/hub-client/src/services/authService.ts b/hub-client/src/services/authService.ts index edbf82ca3..6514feda0 100644 --- a/hub-client/src/services/authService.ts +++ b/hub-client/src/services/authService.ts @@ -11,18 +11,29 @@ import { hubPath } from '../utils/routing'; +/** Which verification path authenticated the request (bd-aw8f3sp8). */ +export type AuthCredentialKind = 'session' | 'bearer'; + /** User info returned by GET /auth/me. */ export interface AuthState { email: string; name: string | null; picture: string | null; /** - * Session expiry in ms-epoch (from the server's `exp`). **Sliding**: - * the hub re-issues the session cookie on authenticated activity, so - * this moves forward over time (typically days out). Absent on older + * Expiry in ms-epoch of the **presented credential** (from the + * server's `exp`). Semantics depend on `credential`: **sliding** for + * a session cookie (the hub re-issues it on authenticated activity, + * so this moves forward over time — typically days out), but the + * token's **fixed** expiry on the Bearer path. Absent on older * servers. */ expiresAt?: number; + /** + * Discriminator for `expiresAt` semantics: `'session'` (hub-minted + * cookie, sliding) or `'bearer'` (Google ID token, fixed). Absent on + * older servers. + */ + credential?: AuthCredentialKind; } /** Raw JSON shape from GET /auth/me (snake_case). */ @@ -30,8 +41,10 @@ interface AuthMeResponse { email: string; name: string | null; picture: string | null; - /** Token expiry in epoch seconds. */ + /** Expiry of the presented credential, epoch seconds. */ exp?: number; + /** `'session'` | `'bearer'` discriminator (bd-aw8f3sp8). */ + credential?: AuthCredentialKind; } /** Fetch user info from the server. Returns null on 401 (not authenticated). */ @@ -45,6 +58,7 @@ export async function fetchAuthMe(): Promise { name: data.name, picture: data.picture, expiresAt: data.exp ? data.exp * 1000 : undefined, + credential: data.credential, }; } diff --git a/scripts/hub-bearer-revocation-e2e.mjs b/scripts/hub-bearer-revocation-e2e.mjs new file mode 100644 index 000000000..743636249 --- /dev/null +++ b/scripts/hub-bearer-revocation-e2e.mjs @@ -0,0 +1,165 @@ +// End-to-end verification for revocation-ledger enforcement on the +// Bearer path (F1, bd-jkih1ql7). Drives the REAL `hub` binary over +// HTTP with a standalone mock OIDC IdP (this script). Node built-ins +// only. Plan: +// claude-notes/plans/2026-08-03-bearer-revocation-and-mcp-auth-followups.md +// +// Steps: +// 1. baseline: Google Bearer for subs A/B/C -> /health 200, /ws 101 +// (A also shows the WS upgrade shape an MCP client uses). +// 2. stop the hub; apply the documented stopped-hub operator +// procedure to revocations.json: ban A, write a not_before floor +// for B; restart. +// 3. banned A -> 403 on /health AND on the WS upgrade, with any +// token (even a fresh one — bans are iat-independent). +// 4. B's pre-floor token (iat < not_before) -> 401; B's post-floor +// token (fresh iat) -> 200 (the MCP self-heal-on-refresh path). +// 5. untouched C still works -> 200 / 101. +// +// Prereq: cargo build --bin hub +import { generateKeyPairSync, sign, randomUUID } from 'node:crypto'; +import { createServer } from 'node:http'; +import http from 'node:http'; +import { spawn } from 'node:child_process'; +import { mkdtempSync, writeFileSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const CLIENT_ID = 'mcp.e2e.test'; +const HUB_PORT = 3996; +const HUB = `http://127.0.0.1:${HUB_PORT}`; + +const b64u = (buf) => Buffer.from(buf).toString('base64url'); +const now = () => Math.floor(Date.now() / 1000); + +// ── mock IdP ──────────────────────────────────────────────────────── +const { publicKey, privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048 }); +const jwk = { ...publicKey.export({ format: 'jwk' }), alg: 'RS256', use: 'sig', kid: 'e2e-kid-1' }; + +function googleToken({ sub, iat = now() - 5, expIn = 600 }) { + const header = b64u(JSON.stringify({ alg: 'RS256', typ: 'JWT', kid: 'e2e-kid-1' })); + const payload = b64u(JSON.stringify({ + iss: issuer, sub, aud: CLIENT_ID, email: 'e2e@posit.co', email_verified: true, + name: 'E2E User', iat, exp: now() + expIn, + })); + const sig = sign('sha256', Buffer.from(`${header}.${payload}`), privateKey); + return `${header}.${payload}.${b64u(sig)}`; +} + +const idp = createServer((req, res) => { + res.setHeader('content-type', 'application/json'); + if (req.url === '/.well-known/openid-configuration') { + res.end(JSON.stringify({ issuer, jwks_uri: `${issuer}/jwks.json` })); + } else if (req.url === '/jwks.json') { + res.end(JSON.stringify({ keys: [jwk] })); + } else { + res.statusCode = 404; res.end('{}'); + } +}); +await new Promise((r) => idp.listen(0, '127.0.0.1', r)); +const issuer = `http://127.0.0.1:${idp.address().port}`; +console.log(`[idp] serving discovery+jwks at ${issuer}`); + +// ── the real hub binary ───────────────────────────────────────────── +const dataDir = mkdtempSync(join(tmpdir(), 'hub-bearer-revocation-e2e-')); +const hubArgs = [ + '--data-dir', dataDir, '--port', String(HUB_PORT), + '--oidc-client-id', CLIENT_ID, '--oidc-issuer', issuer, + '--allow-insecure-auth', +]; + +let hub; +async function startHub() { + console.log(`[hub] target/debug/hub ${hubArgs.join(' ')}`); + hub = spawn('target/debug/hub', hubArgs, { stdio: ['ignore', 'pipe', 'pipe'] }); + hub.stderr.on('data', () => {}); + hub.stdout.on('data', () => {}); + for (let i = 0; ; i++) { + try { await fetch(`${HUB}/auth/me`); break; } + catch { if (i > 50) throw new Error('hub did not start'); await new Promise((r) => setTimeout(r, 200)); } + } + console.log('[hub] up'); +} +async function stopHub() { + const exited = new Promise((r) => hub.once('exit', r)); + hub.kill(); + await exited; + console.log('[hub] stopped'); +} +process.on('exit', () => hub?.kill()); + +// ── helpers ───────────────────────────────────────────────────────── +let failures = 0; +const check = (label, cond, detail) => { + console.log(`${cond ? 'PASS' : 'FAIL'} ${label}${detail ? ` — ${detail}` : ''}`); + if (!cond) failures++; +}; +const health = (token) => + fetch(`${HUB}/health`, { headers: { authorization: `Bearer ${token}` } }); +const wsUpgrade = (token) => new Promise((resolve) => { + const req = http.request(`${HUB}/ws`, { + headers: { + connection: 'Upgrade', upgrade: 'websocket', 'sec-websocket-version': '13', + 'sec-websocket-key': 'dGVzdHNvY2tleS0xMjM0NTY3OA==', + host: `127.0.0.1:${HUB_PORT}`, + authorization: `Bearer ${token}`, + }, + }); + req.on('upgrade', (res) => { resolve(res.statusCode); req.destroy(); }); + req.on('response', (res) => resolve(res.statusCode)); + req.on('error', () => resolve(-1)); + req.end(); +}); + +const T = now(); +const subA = `banned-${randomUUID()}`; +const subB = `revoked-${randomUUID()}`; +const subC = `untouched-${randomUUID()}`; +// Explicit iats so the checks are deterministic no matter how long the +// restart takes: B's old token predates the floor, its fresh one follows it. +const tokenA = googleToken({ sub: subA, iat: T - 5 }); +const tokenBOld = googleToken({ sub: subB, iat: T - 100 }); +const tokenC = googleToken({ sub: subC, iat: T - 5 }); +const NOT_BEFORE_B = T - 50; + +// ── 1. baseline: everyone authenticates ───────────────────────────── +await startHub(); +check('baseline: A /health -> 200', (await health(tokenA)).status === 200); +check('baseline: B /health -> 200', (await health(tokenBOld)).status === 200); +check('baseline: C /health -> 200', (await health(tokenC)).status === 200); +check('baseline: A /ws -> 101', (await wsUpgrade(tokenA)) === 101); + +// ── 2. stopped-hub operator procedure ─────────────────────────────── +await stopHub(); +const revPath = join(dataDir, 'revocations.json'); +writeFileSync(revPath, JSON.stringify({ + version: 1, + not_before: { [subB]: NOT_BEFORE_B }, + banned: [subA], +})); +console.log(`[operator] wrote ${revPath}: ${readFileSync(revPath, 'utf8')}`); +await startHub(); + +// ── 3. banned sub: 403 on probe and WS, iat-independent ──────────── +check('banned A /health -> 403', (await health(tokenA)).status === 403); +check('banned A /ws -> 403', (await wsUpgrade(tokenA)) === 403); +const tokenAFresh = googleToken({ sub: subA }); +check('banned A with a FRESH token -> still 403 (bans are iat-independent)', + (await health(tokenAFresh)).status === 403); + +// ── 4. not_before floor: old iat dies, fresh iat self-heals ──────── +check('B pre-floor token (iat < not_before) /health -> 401', + (await health(tokenBOld)).status === 401); +check('B pre-floor token /ws -> 401', (await wsUpgrade(tokenBOld)) === 401); +const tokenBFresh = googleToken({ sub: subB, iat: now() - 5 }); +check('B post-floor token (fresh iat) -> 200 (self-heal on refresh)', + (await health(tokenBFresh)).status === 200); + +// ── 5. untouched identity unaffected ──────────────────────────────── +check('untouched C /health -> 200', (await health(tokenC)).status === 200); +check('untouched C /ws -> 101', (await wsUpgrade(tokenC)) === 101); + +await stopHub(); +idp.close(); +console.log(failures === 0 ? '\nALL CHECKS PASSED' : `\n${failures} CHECK(S) FAILED`); +process.exit(failures === 0 ? 0 : 1); diff --git a/ts-packages/quarto-hub-mcp/README.md b/ts-packages/quarto-hub-mcp/README.md index cd15f2c28..52084ab86 100644 --- a/ts-packages/quarto-hub-mcp/README.md +++ b/ts-packages/quarto-hub-mcp/README.md @@ -240,13 +240,16 @@ To revoke manually, or to be sure when the revoke step failed: The agent's next action surfaces `ReauthRequired` with a message asking you to re-authenticate. -> **ID-token residual validity.** A stolen ID token authenticates to -> the hub for up to **≤1 hour** after revocation, because JWTs are -> self-contained and the hub does not consult Google on each -> request. Closing this window requires a hub-side denylist — not in -> v1. If you have evidence of an active compromise (e.g. a leaked -> machine), ask your hub operator to roll the audience allowlist or -> rotate the OAuth client. +> **ID-token residual validity — closed for hub-side events.** The +> hub enforces its revocation ledger on the Bearer path (bd-jkih1ql7): +> a **ban** denies the identity immediately (403), and a +> **logout-everywhere** kills every outstanding ID token whose `iat` +> predates it (401). What the ledger cannot kill is a stolen **refresh +> token** — a refresh mints a fresh `iat` that passes the floor — so +> Google-side revocation (`authenticate_clear`, or the manual steps +> above) remains the lever for a compromised grant, and a hub **ban** +> remains the operator lever for a hostile identity. A legitimate +> client caught by logout-everywhere self-heals on its next refresh. ## Why both env vars must come from the operator diff --git a/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts b/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts index 90e1f38fb..8a855c62a 100644 --- a/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts +++ b/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts @@ -18,6 +18,7 @@ import type { SyncClient, SyncClientCallbacks } from '@quarto/quarto-sync-client import { AuthRequiredError, ConnectionManager, + HubAccessDeniedError, InsecureTransportError, isLoopbackHost, } from './connection-manager.js'; @@ -702,6 +703,269 @@ describe('waitForChange (long-poll)', () => { }); }); +// --------------------------------------------------------------------------- +// Mid-session auth rejection (bd-l3b1brn8) +// --------------------------------------------------------------------------- +// +// The sync adapter reports definitive evidence (upgrade 401/403, +// terminal refresh failure) through `onAuthRejected`; the manager owns +// policy: one forceRefresh+reprobe cycle on 401 (coalesced across +// projects), invalidate + reauth-required on persistent 401, a +// keyring-preserving terminal "not allowed" state on 403. Network +// errors never reach this path (the adapter never reports them). + +/** The upgrade-401 evidence shape the adapter reports. */ +const evidence401 = { kind: 'upgrade-status', status: 401 } as const; +const evidence403 = { kind: 'upgrade-status', status: 403 } as const; + +describe('mid-session auth rejection', () => { + it('wires onAuthRejected into the sync-client auth options', async () => { + const auth = seededAuth(); + const fetchSpy = scriptedFetch([200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + const passed = sync.connectCalls[0]!.auth as { + onAuthRejected?: (evidence: unknown) => void; + }; + expect(typeof passed.onAuthRejected).toBe('function'); + }); + + it('recovers silently when one forceRefresh+reprobe fixes a 401', async () => { + const auth = seededAuth(); + auth.forceRefresh.mockResolvedValue('fresh-token'); + // initial probe 200, recheck probe 200. + const fetchSpy = scriptedFetch([200, 200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + await mgr.handleAuthRejected(evidence401); + + expect(auth.forceRefresh).toHaveBeenCalledOnce(); + expect(fetchSpy.calls).toHaveLength(2); + expect(fetchSpy.calls[1]!.headers.Authorization).toBe('Bearer fresh-token'); + // Recovery is invisible: no invalidate, project handle intact — the + // adapter's own retry picks the fresh token up via getBearer. + expect(auth.invalidate).not.toHaveBeenCalled(); + expect(mgr.get('idx-1')).toBeDefined(); + }); + + it('invalidates and enters reauth-required on persistent 401; the next call fails fast with ReauthRequired', async () => { + const auth = seededAuth(); + auth.forceRefresh.mockResolvedValue('still-bad-token'); + // initial probe 200, recheck probe 401 — and NOTHING more scripted: + // the post-state connect must fail without touching the network. + const fetchSpy = scriptedFetch([200, 401]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + await mgr.handleAuthRejected(evidence401); + + expect(auth.invalidate).toHaveBeenCalledOnce(); + expect(await auth.store.read()).toBeNull(); + // The dead project handle was disconnected and dropped… + expect(mgr.get('idx-1')).toBeUndefined(); + // …and the next tool call fails fast with the reauth message instead + // of hanging into the peer timeout (no probe: fetch is exhausted). + await expect(mgr.connect('idx-1')).rejects.toBeInstanceOf(ReauthRequired); + }); + + it('self-heals from reauth-required after the user re-authenticates', async () => { + const auth = seededAuth(); + auth.forceRefresh.mockResolvedValue('still-bad-token'); + const fetchSpy = scriptedFetch([200, 401, 200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + await mgr.handleAuthRejected(evidence401); + await expect(mgr.connect('idx-1')).rejects.toBeInstanceOf(ReauthRequired); + + // The user runs `authenticate`: fresh credentials land in the store. + await auth.store.write({ + idToken: 'post-reauth-token', + refreshToken: 'post-reauth-refresh', + idTokenExpiresAt: new Date(Date.now() + 3600_000), + scopes: ['openid', 'email', 'profile'], + }); + auth.getValid.mockResolvedValue('post-reauth-token'); + + // The gate clears and a normal probed connect runs (the third 200). + await mgr.connect('idx-1'); + expect(mgr.get('idx-1')).toBeDefined(); + }); + + it('treats 403 evidence as terminal denial: keyring kept, clear message on the next call', async () => { + const auth = seededAuth(); + // initial probe 200; the next connect's re-probe answers 403. + const fetchSpy = scriptedFetch([200, 403]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + await mgr.handleAuthRejected(evidence403); + + // Identity denial, not credential failure: no refresh, no wipe. + expect(auth.forceRefresh).not.toHaveBeenCalled(); + expect(auth.invalidate).not.toHaveBeenCalled(); + expect(await auth.store.read()).not.toBeNull(); + expect(mgr.get('idx-1')).toBeUndefined(); + + // Next call re-probes (fast HTTP, no peer timeout) and surfaces the + // clear denial message; a later operator un-ban would self-heal here. + const err = await mgr.connect('idx-1').catch((e: unknown) => e); + expect(err).toBeInstanceOf(HubAccessDeniedError); + expect(String((err as Error).message)).toMatch(/banned|allowlist/i); + }); + + it('maps a 403 on the recheck probe after 401 evidence to the same denial state', async () => { + const auth = seededAuth(); + auth.forceRefresh.mockResolvedValue('fresh-token'); + // initial 200; recheck probe 403 (banned mid-session while a refresh + // was in flight); next connect re-probes 403. + const fetchSpy = scriptedFetch([200, 403, 403]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + await mgr.handleAuthRejected(evidence401); + + expect(auth.invalidate).not.toHaveBeenCalled(); + expect(await auth.store.read()).not.toBeNull(); + expect(mgr.get('idx-1')).toBeUndefined(); + await expect(mgr.connect('idx-1')).rejects.toBeInstanceOf(HubAccessDeniedError); + }); + + it('enters reauth-required directly on token-refresh-terminal evidence', async () => { + const auth = seededAuth(); + const fetchSpy = scriptedFetch([200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + // The refresh manager already invalidated the grant when it threw + // ReauthRequired from getBearer; simulate that store state. + await auth.store.clear(); + await mgr.handleAuthRejected({ kind: 'token-refresh-terminal' }); + + expect(auth.forceRefresh).not.toHaveBeenCalled(); // pointless, skip it + expect(mgr.get('idx-1')).toBeUndefined(); + await expect(mgr.connect('idx-1')).rejects.toBeInstanceOf(ReauthRequired); + }); + + it('coalesces concurrent reports into one forceRefresh+reprobe cycle', async () => { + const auth = seededAuth(); + auth.forceRefresh.mockResolvedValue('fresh-token'); + // Exactly one recheck response scripted: a second cycle would throw + // "ran out of responses". + const fetchSpy = scriptedFetch([200, 200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + // Two project adapters both fire after a hub-wide event. + await Promise.all([ + mgr.handleAuthRejected(evidence401), + mgr.handleAuthRejected(evidence401), + ]); + + expect(auth.forceRefresh).toHaveBeenCalledOnce(); + expect(fetchSpy.calls).toHaveLength(2); + }); + + it('leaves state untouched when the recheck refresh fails transiently (TokenRefreshError)', async () => { + const auth = seededAuth(); + const transient = new Error('IdP hiccup'); + transient.name = 'TokenRefreshError'; + auth.forceRefresh.mockRejectedValue(transient); + const fetchSpy = scriptedFetch([200]); // no recheck probe happens + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + await mgr.connect('idx-1'); + + await mgr.handleAuthRejected(evidence401); + + expect(auth.invalidate).not.toHaveBeenCalled(); + expect(mgr.get('idx-1')).toBeDefined(); // nothing torn down + expect(await auth.store.read()).not.toBeNull(); + }); + + it('surfaces the clear denial message on an initial-probe 403 (not "Unexpected status")', async () => { + const auth = seededAuth(); + const fetchSpy = scriptedFetch([403]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + credentialStore: auth.store, + refreshManager: auth.refresh, + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + + const err = await mgr.connect('idx-1').catch((e: unknown) => e); + expect(err).toBeInstanceOf(HubAccessDeniedError); + expect(String((err as Error).message)).not.toMatch(/unexpected status/i); + expect(String((err as Error).message)).toMatch(/banned|allowlist/i); + expect(sync.connectCalls).toHaveLength(0); + }); +}); + // --------------------------------------------------------------------------- // Redaction invariants // --------------------------------------------------------------------------- diff --git a/ts-packages/quarto-hub-mcp/src/connection-manager.ts b/ts-packages/quarto-hub-mcp/src/connection-manager.ts index 9ef98cbad..4d353fc90 100644 --- a/ts-packages/quarto-hub-mcp/src/connection-manager.ts +++ b/ts-packages/quarto-hub-mcp/src/connection-manager.ts @@ -27,6 +27,7 @@ import { createHash } from 'node:crypto'; import { createSyncClient, + type AuthRejectionEvidence, type DisconnectOptions, type SyncClient, type SyncClientCallbacks, @@ -69,6 +70,24 @@ export class InsecureTransportError extends Error { } } +/** + * The hub returned 403 for our (valid) credentials: the identity is + * denied — banned or not in the allowlist. Distinct from a 401 in that + * re-authenticating with the same account cannot help, so the keyring + * is deliberately left intact (bd-l3b1brn8). + */ +export class HubAccessDeniedError extends Error { + override readonly name = 'HubAccessDeniedError'; + constructor( + message: string = 'Your account is not allowed on this Quarto Hub ' + + '(it may be banned or not in the allowlist). Re-authenticating ' + + 'with the same account will not help — contact the hub operator, ' + + 'or run authenticate_clear and sign in as a different account.', + ) { + super(message); + } +} + export interface ConnectionManagerDeps { readonly serverUrl: string; readonly credentialStore?: CredentialStore; @@ -197,6 +216,15 @@ export class ConnectionManager { // proactively and the WS handshake is the backstop if the hub later // rejects the token. private authConfirmed = false; + // Set when a mid-session rejection ended in a wiped grant: the next + // tool call must fail fast with ReauthRequired instead of hanging + // into the peer timeout. Cleared once fresh credentials appear in + // the store (the user ran `authenticate`). (bd-l3b1brn8) + private reauthRequired = false; + // Coalesces concurrent onAuthRejected reports: every project adapter + // fires after a hub-wide event, but at most one forceRefresh+reprobe + // cycle runs at a time. + private authRecheckInflight: Promise | undefined; constructor(deps: ConnectionManagerDeps | string) { // Backwards-compat: the prior signature was `new ConnectionManager(url)`. @@ -230,6 +258,7 @@ export class ConnectionManager { * we've already connected. */ async connect(indexDocId: string): Promise { + await this.gateAuthState(); const existing = this.projects.get(indexDocId); if (existing) return existing; @@ -336,6 +365,7 @@ export class ConnectionManager { async createProject( files: Array<{ path: string; content: string }>, ): Promise<{ indexDocId: string; files: Array<{ path: string; docId: string }> }> { + await this.gateAuthState(); const auth = await this.resolveAuthForConnect(); const tempFiles = new Map(); @@ -475,7 +505,7 @@ export class ConnectionManager { // on the happy path that's a cached, network-free call. if (this.authConfirmed) { await rm.getValidIdToken(); - return { getBearer: () => rm.getValidIdToken() }; + return this.buildAuthOptions(rm); } // Pull a valid id_token (refreshes proactively within the skew). @@ -506,13 +536,165 @@ export class ConnectionManager { // probe, and hand the sync client a getter so each attach + retry // sees a freshly-refreshed token. this.authConfirmed = true; - return { getBearer: () => rm.getValidIdToken() }; + return this.buildAuthOptions(rm); + } + if (status === 403) { + // Valid credentials, denied identity (banned / not allowlisted). + // Deliberately NOT invalidate(): re-auth with the same account + // cannot help, so keep the keyring intact. + throw new HubAccessDeniedError(); } throw new Error( `Unexpected status ${status} from hub auth probe at ${this.probePath}`, ); } + /** + * The auth options handed to the sync client: a fresh-token getter + * for every attach/retry, plus the evidence channel the adapter uses + * to report definitive mid-session auth rejections (bd-l3b1brn8). + */ + private buildAuthOptions(rm: RefreshManager): { + getBearer: () => Promise; + onAuthRejected: (evidence: AuthRejectionEvidence) => void; + } { + return { + getBearer: () => rm.getValidIdToken(), + onAuthRejected: (evidence) => { + void this.handleAuthRejected(evidence); + }, + }; + } + + /** + * Fail fast when a prior mid-session rejection wiped the grant: the + * next tool call gets the ReauthRequired message immediately instead + * of hanging into the peer timeout. Fresh credentials in the store + * (the user ran `authenticate`) clear the gate. + */ + private async gateAuthState(): Promise { + if (!this.reauthRequired) return; + const bundle = this.credentialStore + ? await this.credentialStore.read() + : null; + if (bundle !== null) { + this.reauthRequired = false; + return; + } + throw new ReauthRequired(); + } + + /** + * Policy for the adapter's definitive auth-rejection evidence. + * Coalesced: concurrent reports from multiple project adapters run at + * most one cycle. Outcomes: + * + * - `token-refresh-terminal` → the refresh manager already wiped the + * grant when it threw; enter reauth-required. + * - upgrade 401 → one forceRefresh + reprobe. 200 = recovered + * (silent; the adapter's retry picks the fresh token up via + * getBearer). 401 again = invalidate + reauth-required. 403 = + * denial. Transient refresh/probe failures change nothing. + * - upgrade 403 → denial: keyring kept (identity denied, credentials + * fine), projects dropped; the next connect re-probes and surfaces + * {@link HubAccessDeniedError} — which also self-heals if the + * operator lifts the ban. + */ + async handleAuthRejected(evidence: AuthRejectionEvidence): Promise { + if (this.authRecheckInflight) return this.authRecheckInflight; + const run = this.classifyAuthRejection(evidence).finally(() => { + this.authRecheckInflight = undefined; + }); + this.authRecheckInflight = run; + return run; + } + + private async classifyAuthRejection( + evidence: AuthRejectionEvidence, + ): Promise { + const rm = this.refreshManager; + if (!rm) return; // no Bearer wired — nothing to decide + + if (evidence.kind === 'token-refresh-terminal') { + await this.enterReauthRequired(); + return; + } + if (evidence.status === 403) { + await this.enterDenied(); + return; + } + + // 401: possibly just a token the proactive refresh missed. One + // forceRefresh + reprobe decides; recovery is invisible to the user. + let token: string; + try { + token = await rm.forceRefresh(); + } catch (err) { + if ((err as { name?: string } | null)?.name === 'ReauthRequired') { + // invalid_grant: the refresh manager wiped the grant already. + await this.enterReauthRequired(); + return; + } + // TokenRefreshError / network: transient — never change auth + // state on non-definitive evidence; the adapter keeps retrying. + return; + } + let status: number; + try { + status = await this.probeAuth(token); + } catch { + return; // network error: state-neutral + } + if (status === 401) { + // Freshly-refreshed token still rejected — same terminal shape as + // the pre-connect persistent-401 path. + await rm.invalidate(); + await this.enterReauthRequired(); + } else if (status === 403) { + await this.enterDenied(); + } + // 200: recovered. The adapter's retry loop re-pulls via getBearer. + } + + private async enterReauthRequired(): Promise { + this.reauthRequired = true; + this.authConfirmed = false; + console.error( + '[hub-mcp] Quarto Hub rejected our credentials mid-session; ' + + 're-authentication is required. Ask me to run `authenticate`.', + ); + await this.dropDeadProjects(); + } + + private async enterDenied(): Promise { + this.authConfirmed = false; // force a fresh probe on the next connect + console.error(`[hub-mcp] ${new HubAccessDeniedError().message}`); + await this.dropDeadProjects(); + } + + /** + * Disconnect and drop every project handle after a terminal auth + * event — their sockets are dead or doomed, and a dropped handle + * makes the next tool call re-enter `connect()` where the fast, + * clearly-messaged failure paths live. + */ + private async dropDeadProjects(): Promise { + const entries = Array.from(this.projects.entries()); + this.projects.clear(); + await Promise.all( + entries.map(async ([indexDocId, s]) => { + try { + await s.client.disconnect(); + } catch { + console.error( + `[hub-mcp] error disconnecting project ${indexDocId} after ` + + 'an auth rejection (ignored)', + ); + } + }), + ); + } + /** * Performs an HTTP GET against the configured probe path with the * given Bearer (if any). Returns the HTTP status code; throws on diff --git a/ts-packages/quarto-hub-mcp/src/e2e-auth.test.ts b/ts-packages/quarto-hub-mcp/src/e2e-auth.test.ts index 108d1db56..e161248a1 100644 --- a/ts-packages/quarto-hub-mcp/src/e2e-auth.test.ts +++ b/ts-packages/quarto-hub-mcp/src/e2e-auth.test.ts @@ -45,7 +45,7 @@ import { fileURLToPath } from 'node:url'; import { AsyncEntry } from '@napi-rs/keyring'; import { McpTestClient } from './mcp-test-client.js'; -import { startTestIdp, type TestIdp } from './test-idp.js'; +import { startTestIdp, TEST_REFRESH_TOKEN, type TestIdp } from './test-idp.js'; const pkgRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const repoRoot = path.resolve(pkgRoot, '..', '..'); @@ -152,11 +152,30 @@ describe.runIf(runSuite)('auth e2e (real hub + keyring + loopback)', () => { let idp: TestIdp; let hub: ChildProcess; let hubUrl: string; + let hubPort: number; let tmpDir: string; let serverEnv: NodeJS.ProcessEnv; let keyringAccount: string; let projectId: string; + function spawnHub(): ChildProcess { + return spawn( + hubBin, + [ + '--data-dir', path.join(tmpDir, 'hub-data'), + '-P', String(hubPort), + '-H', '127.0.0.1', + '--oidc-client-id', CLIENT_ID, + '--oidc-issuer', idp.issuer, + '--allowed-emails', EMAIL, + '--allow-insecure-auth', + // Hub-side auth audit trail when debugging (DEBUG_MCP=1). + ...(process.env['DEBUG_MCP'] ? ['-vv'] : []), + ], + { stdio: ['ignore', 'ignore', process.env['DEBUG_MCP'] ? 'inherit' : 'ignore'] }, + ); + } + beforeAll(async () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'q2-e2e-auth-')); const shimDir = path.join(tmpDir, 'bin'); @@ -176,23 +195,9 @@ describe.runIf(runSuite)('auth e2e (real hub + keyring + loopback)', () => { // (ephemeral port), but clear anyway in case of port reuse. await new AsyncEntry(KEYRING_SERVICE, keyringAccount).deletePassword().catch(() => {}); - const hubPort = await freePort(); + hubPort = await freePort(); hubUrl = `ws://127.0.0.1:${hubPort}/ws`; - hub = spawn( - hubBin, - [ - '--data-dir', path.join(tmpDir, 'hub-data'), - '-P', String(hubPort), - '-H', '127.0.0.1', - '--oidc-client-id', CLIENT_ID, - '--oidc-issuer', idp.issuer, - '--allowed-emails', EMAIL, - '--allow-insecure-auth', - // Hub-side auth audit trail when debugging (DEBUG_MCP=1). - ...(process.env['DEBUG_MCP'] ? ['-vv'] : []), - ], - { stdio: ['ignore', 'ignore', process.env['DEBUG_MCP'] ? 'inherit' : 'ignore'] }, - ); + hub = spawnHub(); await waitForHealth(`http://127.0.0.1:${hubPort}/health`); serverEnv = { @@ -288,4 +293,109 @@ describe.runIf(runSuite)('auth e2e (real hub + keyring + loopback)', () => { await b.stop(); } }, 90000); + + /** Play the browser's role in the loopback+PKCE flow for `client`. */ + async function authenticateViaLoopback(client: McpTestClient): Promise { + const authPromise = client.callTool('authenticate', {}); + const urlLine = await client.waitForStderr(/open this URL to sign in: /, 15000); + const authUrl = urlLine.slice(urlLine.indexOf('http')); + const browserResp = await fetch(authUrl); + expect(browserResp.ok).toBe(true); + const result = await authPromise; + expect(result.content[0]!.text).toContain(EMAIL); + } + + // ── mid-session auth rejection (bd-l3b1brn8) ───────────────────── + + it('reports ReauthRequired promptly when the grant is revoked mid-session', async () => { + // The main test's authenticate_clear left the (never-rotated) + // refresh token in the IdP's revoked list — reset so this test's + // own session works before its revocation moment. + idp.counters.revokedTokens.length = 0; + + const c = new McpTestClient(); + try { + await c.start(['--server', hubUrl], { env: serverEnv }); + await authenticateViaLoopback(c); + + // The session works. + const connected = await c.callTool('connect_project', { project: projectId }); + expect(connected.content[0]!.text).toContain('auth-e2e.qmd'); + + // The user revokes the grant at the IdP (the real-world analog is + // myaccount.google.com → Remove Access): every further refresh + // grant answers invalid_grant. The 45s TTL sits inside the 60s + // early-refresh window, so the next auth-touching tool call MUST + // refresh — and must fail terminally, not spin. + idp.counters.revokedTokens.push(TEST_REFRESH_TOKEN); + + const started = Date.now(); + const result = await c.callTool('create_project', { + files: [{ path: 'after-revocation.qmd', content: 'x\n' }], + }); + const elapsed = Date.now() - started; + const resultText = result.content[0]!.text; + expect(resultText).toMatch(/expired or were revoked/i); + expect(resultText).toMatch(/authenticate/i); + // Fail-fast is the point: nowhere near the 15 s peer timeout. + expect(elapsed).toBeLessThan(10_000); + + // The dead grant was wiped so a fresh `authenticate` starts clean. + expect( + await new AsyncEntry(KEYRING_SERVICE, keyringAccount).getPassword(), + ).toBeNull(); + } finally { + idp.counters.revokedTokens.length = 0; + await c.stop(); + } + }, 90000); + + it('surfaces the terminal denial message when the sub is banned mid-session (F1 bd-jkih1ql7 + F2)', async () => { + idp.counters.revokedTokens.length = 0; + + const c = new McpTestClient(); + try { + await c.start(['--server', hubUrl], { env: serverEnv }); + await authenticateViaLoopback(c); + const connected = await c.callTool('connect_project', { project: projectId }); + expect(connected.content[0]!.text).toContain('auth-e2e.qmd'); + + // Operator bans the sub — the documented stopped-hub procedure. + const exited = new Promise((r) => hub.once('exit', r)); + hub.kill(); + await exited; + fs.writeFileSync( + path.join(tmpDir, 'hub-data', 'revocations.json'), + JSON.stringify({ version: 1, not_before: {}, banned: ['test-subject-1'] }), + ); + hub = spawnHub(); + await waitForHealth(`http://127.0.0.1:${hubPort}/health`); + + // The dropped socket reconnects (≤5 s retry); the upgrade gets the + // hub's 403 (F1's Bearer-path ledger enforcement); the adapter + // reports it as evidence; the manager enters the terminal denial + // state — announced on stderr. + await c.waitForStderr(/not allowed on this Quarto Hub/i, 30000); + + // The next tool call fails fast with the same clear message… + const started = Date.now(); + const result = await c.callTool('read_file', { + project: projectId, + path: 'auth-e2e.qmd', + }); + expect(result.content[0]!.text).toMatch(/banned|allowlist/i); + expect(Date.now() - started).toBeLessThan(10_000); + + // …and the keyring is kept: identity denial, not credential + // failure — re-auth with the same account cannot help. + expect( + await new AsyncEntry(KEYRING_SERVICE, keyringAccount).getPassword(), + ).toBeTruthy(); + + await c.callTool('authenticate_clear', {}).catch(() => {}); + } finally { + idp.counters.revokedTokens.length = 0; + await c.stop(); + } + }, 120000); }); diff --git a/ts-packages/quarto-hub-mcp/src/test-idp.ts b/ts-packages/quarto-hub-mcp/src/test-idp.ts index 02f610d05..02dfda222 100644 --- a/ts-packages/quarto-hub-mcp/src/test-idp.ts +++ b/ts-packages/quarto-hub-mcp/src/test-idp.ts @@ -26,6 +26,14 @@ export interface TestIdpOptions { idTokenTtlSecs?: number; } +/** + * The single, never-rotated refresh token the IdP hands out + * (Google-style). Exported so tests can simulate a mid-session grant + * revocation by pushing it into `counters.revokedTokens` — every + * subsequent refresh grant then answers `invalid_grant`. + */ +export const TEST_REFRESH_TOKEN = 'rt-test-refresh-token'; + export interface TestIdp { issuer: string; counters: { @@ -51,7 +59,7 @@ export async function startTestIdp(opts: TestIdpOptions): Promise { }; // code -> the PKCE challenge it was issued against const pendingCodes = new Map(); - const REFRESH_TOKEN = 'rt-test-refresh-token'; + const REFRESH_TOKEN = TEST_REFRESH_TOKEN; let issuer = ''; // assigned after listen() const b64url = (input: Buffer | string): string => diff --git a/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.test.ts b/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.test.ts index a41ff3df8..8ea2200ef 100644 --- a/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.test.ts +++ b/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.test.ts @@ -9,6 +9,9 @@ */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as net from 'node:net'; +import type { AddressInfo } from 'node:net'; +import { cbor } from '@automerge/automerge-repo/slim'; import type { PeerId } from '@automerge/automerge-repo/slim'; import { @@ -234,3 +237,308 @@ describe('NodeWebSocketClientAdapter', () => { expect(calls).toHaveLength(0); // no socket was constructed }); }); + +// --------------------------------------------------------------------------- +// Auth-rejection evidence (bd-l3b1brn8) +// --------------------------------------------------------------------------- +// +// The adapter reports *definitive* auth evidence — a 401/403 upgrade +// status surfaced by the factory's optional capability, or a terminal +// refresh failure (`ReauthRequired`-named error from getBearer) — via +// `onAuthRejected`, debounced to one report per failure episode (an +// episode ends at the next successful peer handshake). Network errors +// never fire it. Policy (refresh, invalidate, user messaging) lives in +// hub-mcp's connection manager, not here. + +/** Fake-socket factory whose calls expose the upgrade-status capability. */ +interface StatusFactoryCall extends FactoryCall { + options: { + headers: Record; + onUpgradeStatus?: (status: number) => void; + }; +} + +function makeStatusFactory(sockets: WebSocketLike[]): { + factory: WebSocketFactory; + calls: StatusFactoryCall[]; +} { + const queue = [...sockets]; + const calls: StatusFactoryCall[] = []; + const factory: WebSocketFactory = (url, protocols, options) => { + calls.push({ url, protocols, options } as StatusFactoryCall); + return queue.shift() ?? makeFakeSocket(); + }; + return { factory, calls }; +} + +/** Encoded server `peer` message — completes the sync handshake. */ +function peerMessageEvent(): { data: Uint8Array } { + const cborApi = cbor as { encode(value: unknown): Uint8Array }; + return { + data: cborApi.encode({ + type: 'peer', + senderId: 'server-peer', + peerMetadata: {}, + }), + }; +} + +describe('NodeWebSocketClientAdapter auth-rejection evidence', () => { + it('reports an upgrade 401 once per failure episode, resetting on a successful handshake', async () => { + const s1 = makeFakeSocket(); + const s2 = makeFakeSocket(); + const s3 = makeFakeSocket(); + const s4 = makeFakeSocket(); + const { factory, calls } = makeStatusFactory([s1, s2, s3, s4]); + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer: async () => 'tok', + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + expect(calls).toHaveLength(1); + expect(calls[0]!.options.onUpgradeStatus).toBeDefined(); + + // Attempt 1 fails the upgrade with 401 (ws fires no close/error when + // the unexpected-response capability is consumed). + calls[0]!.options.onUpgradeStatus!(401); + expect(onAuthRejected).toHaveBeenCalledTimes(1); + expect(onAuthRejected).toHaveBeenCalledWith({ + kind: 'upgrade-status', + status: 401, + }); + + // Retry interval fires attempt 2 — also 401. Same episode: no new report. + await vi.advanceTimersByTimeAsync(1000); + expect(calls).toHaveLength(2); + calls[1]!.options.onUpgradeStatus!(401); + expect(onAuthRejected).toHaveBeenCalledTimes(1); + + // Attempt 3 succeeds: open + server `peer` message ends the episode. + await vi.advanceTimersByTimeAsync(1000); + expect(calls).toHaveLength(3); + s3.emit('open', {}); + s3.emit('message', peerMessageEvent()); + + // Hub closes the live socket; the one-shot reconnect gets 401 again — + // a NEW episode, so a second report fires. + s3.emit('close', {}); + await vi.advanceTimersByTimeAsync(1000); + expect(calls).toHaveLength(4); + calls[3]!.options.onUpgradeStatus!(401); + expect(onAuthRejected).toHaveBeenCalledTimes(2); + }); + + it('keeps retrying after a mid-session 403 that fires no close event (ws unexpected-response shape)', async () => { + const s1 = makeFakeSocket(); + const s2 = makeFakeSocket(); + const s3 = makeFakeSocket(); + const { factory, calls } = makeStatusFactory([s1, s2, s3]); + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer: async () => 'tok', + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + // Live session: open + peer. + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + s1.emit('open', {}); // clears the retry interval + s1.emit('message', peerMessageEvent()); + + // Hub closes the socket (e.g. restart after a ban). The one-shot + // reconnect's upgrade is refused 403 — and per ws semantics with an + // unexpected-response listener, NO close/error fires on s2. + s1.emit('close', {}); + await vi.advanceTimersByTimeAsync(1000); + expect(calls).toHaveLength(2); + calls[1]!.options.onUpgradeStatus!(403); + expect(onAuthRejected).toHaveBeenCalledTimes(1); + expect(onAuthRejected).toHaveBeenCalledWith({ + kind: 'upgrade-status', + status: 403, + }); + + // Retry continuity rests on the interval connect() re-created for the + // reconnect — a further attempt must still happen with a fresh token. + await vi.advanceTimersByTimeAsync(1000); + expect(calls.length).toBeGreaterThanOrEqual(3); + }); + + it('never reports on plain network close/error and keeps retrying', async () => { + const s1 = makeFakeSocket(); + const s2 = makeFakeSocket(); + const { factory, calls } = makeStatusFactory([s1, s2]); + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer: async () => 'tok', + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + s1.emit('error', { error: { code: 'ECONNREFUSED', message: 'refused' } }); + s1.emit('close', {}); + + await vi.advanceTimersByTimeAsync(1000); + expect(calls.length).toBeGreaterThanOrEqual(2); // still retrying + expect(onAuthRejected).not.toHaveBeenCalled(); + }); + + it('reports token-refresh-terminal and stops the retry loop when getBearer throws a ReauthRequired-named error', async () => { + const { factory, calls } = makeStatusFactory([]); + const reauth = new Error('credentials revoked'); + reauth.name = 'ReauthRequired'; + const getBearer = vi.fn().mockRejectedValue(reauth); + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer, + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + expect(onAuthRejected).toHaveBeenCalledTimes(1); + expect(onAuthRejected).toHaveBeenCalledWith({ kind: 'token-refresh-terminal' }); + expect(calls).toHaveLength(0); // no socket constructed + + // The retry loop is stopped: no further getBearer calls, ever. + const before = getBearer.mock.calls.length; + await vi.advanceTimersByTimeAsync(10_000); + expect(getBearer.mock.calls.length).toBe(before); + }); + + it('treats TokenRefreshError-named failures as transient: no report, retry continues', async () => { + const { factory } = makeStatusFactory([]); + const transient = new Error('IdP hiccup'); + transient.name = 'TokenRefreshError'; + const getBearer = vi.fn().mockRejectedValue(transient); + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer, + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + await vi.advanceTimersByTimeAsync(2000); + + expect(getBearer.mock.calls.length).toBeGreaterThanOrEqual(3); + expect(onAuthRejected).not.toHaveBeenCalled(); + }); + + it('degrades to today’s behavior with a factory that lacks the status capability', async () => { + const socket = makeFakeSocket(); + const { factory, calls } = makeFactory(socket); // ignores onUpgradeStatus + const onAuthRejected = vi.fn(); + + const adapter = new NodeWebSocketClientAdapter('wss://hub.example.com/ws', { + getBearer: async () => 'tok', + webSocketFactory: factory, + retryInterval: 1000, + onAuthRejected, + }); + + adapter.connect(peerId); + await vi.advanceTimersByTimeAsync(0); + expect(calls).toHaveLength(1); + // A failed upgrade folds into a generic error/close, exactly as today. + socket.emit('error', { error: { code: 'ECONNRESET', message: 'reset' } }); + socket.emit('close', {}); + await vi.advanceTimersByTimeAsync(1000); + + expect(onAuthRejected).not.toHaveBeenCalled(); // no evidence, no report + }); +}); + +// --------------------------------------------------------------------------- +// Default `ws` factory: unexpected-response must not leak connections +// --------------------------------------------------------------------------- +// +// With an 'unexpected-response' listener attached, ws@8 skips its own +// abortHandshake — no error/close fires and the HTTP request stays open +// unless the listener aborts it. These specs run the REAL `ws` package +// against a local HTTP server that answers upgrades with 403 and keeps +// the TCP connection open (keep-alive), so a leak is observable as a +// lingering server-side socket. + +describe('default ws factory unexpected-response handling (real sockets)', () => { + beforeEach(() => { + vi.useRealTimers(); + }); + + it('surfaces the 403, aborts each failed handshake, and leaks no connections', async () => { + // Raw net server (not http.Server): after an 'upgrade' handoff the + // http server never reads the socket again, so a client FIN would + // sit unobserved and 'close' would not fire — a net server reads + // the bytes itself and sees every close. It answers any request + // with 403 and does NOT close: a keep-alive server leaves closing + // to the client — exactly where the leak would appear. + const live = new Set(); + const server = net.createServer((socket) => { + live.add(socket); + socket.on('close', () => live.delete(socket)); + socket.on('error', () => undefined); + socket.on('data', () => { + socket.write('HTTP/1.1 403 Forbidden\r\nContent-Length: 0\r\n\r\n'); + }); + }); + await new Promise((r) => server.listen(0, '127.0.0.1', r)); + const port = (server.address() as AddressInfo).port; + + const getBearer = vi.fn().mockResolvedValue('tok'); + const onAuthRejected = vi.fn(); + const adapter = new NodeWebSocketClientAdapter(`ws://127.0.0.1:${port}/ws`, { + getBearer, + retryInterval: 50, // real default factory: webSocketFactory omitted + onAuthRejected, + }); + + try { + adapter.connect(peerId); + // Let several retry attempts run. + await vi.waitFor( + () => { + expect(getBearer.mock.calls.length).toBeGreaterThanOrEqual(3); + }, + { timeout: 5000 }, + ); + + expect(onAuthRejected).toHaveBeenCalledTimes(1); + expect(onAuthRejected).toHaveBeenCalledWith({ + kind: 'upgrade-status', + status: 403, + }); + + adapter.disconnect(); + // Every failed attempt must have aborted its handshake: the server + // sees each connection close. + await vi.waitFor( + () => { + expect(live.size).toBe(0); + }, + { timeout: 5000 }, + ); + } finally { + adapter.disconnect(); + await new Promise((r) => server.close(() => r())); + } + }); +}); diff --git a/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.ts b/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.ts index 8aacb8d24..11bf225ab 100644 --- a/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.ts +++ b/ts-packages/quarto-sync-client/src/NodeWebSocketClientAdapter.ts @@ -30,6 +30,7 @@ import type { // browser `WebSocket`. This adapter is Node-only by design. import WebSocket from 'ws'; import { syncLog } from './log.js'; +import type { AuthRejectionEvidence } from './types.js'; const ProtocolV1 = '1'; const READY_TIMEOUT_MS = 1000; @@ -65,11 +66,20 @@ export interface WebSocketLike { /** * Test seam: callers can inject a WebSocket factory so unit tests don't * require a real server. Defaults to the `ws` constructor. + * + * `onUpgradeStatus` is an **optional capability**: a factory that can + * observe the HTTP status of a failed (non-101) upgrade invokes it with + * that status. Factories without the capability (test fakes, future + * transports) simply ignore it and degrade to today's behavior — the + * failure folds into a generic error/close. */ export type WebSocketFactory = ( url: string, protocols: readonly string[], - options: { readonly headers: Record }, + options: { + readonly headers: Record; + readonly onUpgradeStatus?: (status: number) => void; + }, ) => WebSocketLike; export interface NodeWebSocketClientAdapterOptions { @@ -83,6 +93,13 @@ export interface NodeWebSocketClientAdapterOptions { readonly retryInterval?: number; /** Test hook — defaults to the `ws` constructor. */ readonly webSocketFactory?: WebSocketFactory; + /** + * Fired on definitive auth-rejection evidence only (see + * {@link AuthRejectionEvidence}), debounced to one report per failure + * episode; the episode ends at the next successful peer handshake. + * Plain network close/error never fires it. + */ + readonly onAuthRejected?: (evidence: AuthRejectionEvidence) => void; } /** @@ -98,8 +115,36 @@ export function redactAuthorization(s: string): string { ); } -const defaultWebSocketFactory: WebSocketFactory = (url, protocols, options) => - new WebSocket(url, [...protocols], options) as unknown as WebSocketLike; +const defaultWebSocketFactory: WebSocketFactory = (url, protocols, options) => { + const socket = new WebSocket(url, [...protocols], { + headers: options.headers, + }); + if (options.onUpgradeStatus) { + // 'unexpected-response' is EventEmitter-only (not reachable through + // addEventListener), so it must be attached natively here. Attaching + // it changes ws's behavior: abortHandshake is SKIPPED — no + // error/close fires for this socket and the underlying HTTP request + // stays open — so this handler must abort the handshake itself + // (drain the response, destroy the request) or every failed attempt + // leaks a connection. Retry continuity rests on the adapter's + // interval, not on a close event from this socket. + socket.on('unexpected-response', (req, res) => { + try { + options.onUpgradeStatus?.(res.statusCode ?? 0); + } finally { + // Capture the TCP socket before touching the request: once the + // response has completed, req.destroy() no longer closes the + // underlying socket (the ws#1869 caveat its own abortHandshake + // works around the same way). + const tcp = res.socket ?? req.socket; + res.resume(); + req.destroy(); + if (tcp && !tcp.destroyed) tcp.destroy(); + } + }); + } + return socket as unknown as WebSocketLike; +}; const cborApi = cbor as { encode(value: unknown): Uint8Array; @@ -111,11 +156,20 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { readonly retryInterval: number; private readonly getBearer: () => Promise; private readonly wsFactory: WebSocketFactory; + private readonly onAuthRejected?: (evidence: AuthRejectionEvidence) => void; private socket: WebSocketLike | undefined; private retryIntervalId: ReturnType | undefined; /** Set by disconnect(); makes any later connect() a permanent no-op. */ private stopped = false; + /** + * Set on a terminal refresh failure (ReauthRequired from getBearer): + * every future attempt would throw the same way, so the retry loop + * stops. Policy (re-auth, discarding this adapter) lives upstream. + */ + private authTerminal = false; + /** Episode debounce for onAuthRejected; reset on peer handshake. */ + private authRejectionReported = false; private ready = false; private readyResolver?: () => void; private readonly readyPromise: Promise; @@ -127,6 +181,7 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { this.retryInterval = opts.retryInterval ?? 5000; this.getBearer = opts.getBearer; this.wsFactory = opts.webSocketFactory ?? defaultWebSocketFactory; + this.onAuthRejected = opts.onAuthRejected; this.readyPromise = new Promise((resolve) => { this.readyResolver = resolve; }); @@ -153,7 +208,9 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { // without this gate a discarded adapter resurrects itself and // retries a dead endpoint forever. Mirrors // StoppableWebSocketClientAdapter (the browser-side fix). - if (this.stopped) return; + // authTerminal gates the same way: a terminal refresh failure + // means every attempt would fail identically (bd-l3b1brn8). + if (this.stopped || this.authTerminal) return; if (!this.socket || !this.peerId) { this.peerId = peerId; this.peerMetadata = peerMetadata ?? {}; @@ -182,15 +239,29 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { let token: string; try { token = await this.getBearer(); - } catch { - // Token-fetch failure: leave the socket unset. The retry loop - // will try again, and the underlying refresh manager (if any) - // surfaces the error to its own caller. + } catch (err) { + // Classification is by error NAME, not instanceof: sync-client + // cannot import hub-mcp's ReauthRequired class (the dependency + // direction is hub-mcp → sync-client); the refresh manager stamps + // `name = 'ReauthRequired'` as the cross-package contract. A + // ReauthRequired is terminal — every retry would throw the same + // way — so report it and stop the loop. Anything else + // (TokenRefreshError, network) is transient: leave the socket + // unset and let the retry loop try again, as before. + if ((err as { name?: string } | null)?.name === 'ReauthRequired') { + this.authTerminal = true; + if (this.retryIntervalId) { + clearInterval(this.retryIntervalId); + this.retryIntervalId = undefined; + } + this.reportAuthRejection({ kind: 'token-refresh-terminal' }); + } return; } const socket = this.wsFactory(this.url, [], { headers: { Authorization: `Bearer ${token}` }, + onUpgradeStatus: this.onUpgradeStatus, }); socket.binaryType = 'arraybuffer'; socket.addEventListener('open', this.onOpen); @@ -213,6 +284,7 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { if (this.remotePeerId) { this.emit('peer-disconnected', { peerId: this.remotePeerId }); } + if (this.authTerminal) return; // reconnecting would just re-throw if (this.retryInterval > 0 && !this.retryIntervalId && this.peerId) { const peerId = this.peerId; const peerMetadata = this.peerMetadata; @@ -243,6 +315,30 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { ); }; + /** + * Upgrade-status capability callback, handed to the factory on every + * attempt. Only a definitive 401/403 is auth evidence; anything else + * (proxy 502s, redirects) stays generic and the retry loop handles it + * exactly as today. Retrying continues on upgrade evidence — policy + * upstream may fix the token so a later attempt succeeds. + */ + private readonly onUpgradeStatus = (status: number): void => { + if (status === 401 || status === 403) { + this.reportAuthRejection({ kind: 'upgrade-status', status }); + } + }; + + /** One report per failure episode; reset by the next peer handshake. */ + private reportAuthRejection(evidence: AuthRejectionEvidence): void { + if (this.authRejectionReported) return; + this.authRejectionReported = true; + try { + this.onAuthRejected?.(evidence); + } catch { + // Observer errors must never break the transport (bd-xzspx4r9). + } + } + private removeListeners(socket: WebSocketLike): void { socket.removeEventListener('open', this.onOpen); socket.removeEventListener('close', this.onClose); @@ -266,6 +362,12 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { this.stopped = true; if (this.socket) { this.removeListeners(this.socket); + // close() on a still-CONNECTING `ws` socket emits an error event + // ("closed before the connection was established"); with our + // listeners just removed, an unhandled 'error' would crash the + // process. Keep a swallow-only handler attached for the socket's + // remaining lifetime. + this.socket.addEventListener('error', () => undefined); this.socket.close(); } if (this.retryIntervalId) { @@ -300,6 +402,9 @@ export class NodeWebSocketClientAdapter extends NetworkAdapter { private peerCandidate(remotePeerId: PeerId, peerMetadata: PeerMetadata): void { this.forceReady(); + // A successful handshake ends any auth-failure episode: the next + // definitive rejection is new evidence and reports again. + this.authRejectionReported = false; this.remotePeerId = remotePeerId; this.emit('peer-candidate', { peerId: remotePeerId, peerMetadata }); } diff --git a/ts-packages/quarto-sync-client/src/client.ts b/ts-packages/quarto-sync-client/src/client.ts index 0b598cfb8..19527db91 100644 --- a/ts-packages/quarto-sync-client/src/client.ts +++ b/ts-packages/quarto-sync-client/src/client.ts @@ -147,6 +147,7 @@ async function buildWsAdapter( return new mod.NodeWebSocketClientAdapter(url, { getBearer: auth.getBearer, retryInterval: retryIntervalMs, + onAuthRejected: auth.onAuthRejected, }) as unknown as NetworkAdapter; } diff --git a/ts-packages/quarto-sync-client/src/index.ts b/ts-packages/quarto-sync-client/src/index.ts index 0b89ce901..ce670ae25 100644 --- a/ts-packages/quarto-sync-client/src/index.ts +++ b/ts-packages/quarto-sync-client/src/index.ts @@ -29,6 +29,7 @@ export { // Export sync client types export type { AnnotatedFileEntry, + AuthRejectionEvidence, Patch, EditorContentChange, TextFilePayload, diff --git a/ts-packages/quarto-sync-client/src/types.ts b/ts-packages/quarto-sync-client/src/types.ts index 75a04a39b..cf460c267 100644 --- a/ts-packages/quarto-sync-client/src/types.ts +++ b/ts-packages/quarto-sync-client/src/types.ts @@ -200,6 +200,25 @@ export interface ASTOptions { // Auth Options // ============================================================================ +/** + * Definitive auth-rejection evidence observed by the Node WebSocket + * adapter (bd-l3b1brn8). Only two shapes qualify: + * + * - `upgrade-status`: a reachable hub refused the WS upgrade with a + * definitive 401/403 (surfaced through the factory's optional + * upgrade-status capability); + * - `token-refresh-terminal`: `getBearer` threw an error whose + * `name === 'ReauthRequired'` — the cross-package contract with + * hub-mcp's refresh manager (the class itself cannot be imported + * here; the dependency direction is hub-mcp → sync-client). + * + * Network errors are never evidence: they leave auth state unchanged + * and the retry loop running (the SPA's bd-3o8zmz46 invariant). + */ +export type AuthRejectionEvidence = + | { kind: 'upgrade-status'; status: 401 | 403 } + | { kind: 'token-refresh-terminal' }; + /** * Bearer-auth options for the sync client's WebSocket upgrade. * @@ -210,6 +229,13 @@ export interface ASTOptions { */ export interface SyncClientAuthOptions { getBearer: () => Promise; + /** + * Fired on definitive auth-rejection evidence only, debounced to one + * report per failure episode (an episode ends at the next successful + * peer handshake). Policy — refreshing, invalidating credentials, + * user messaging — belongs to the caller; the adapter only reports. + */ + onAuthRejected?: (evidence: AuthRejectionEvidence) => void; } /**