diff --git a/.mcp.json b/.mcp.json index 9b6181a2..89278aac 100644 --- a/.mcp.json +++ b/.mcp.json @@ -4,9 +4,7 @@ "type": "stdio", "command": "node", "args": [ - "ts-packages/quarto-hub-mcp/dist/index.js", - "--server", - "wss://quarto-hub.com/ws" + "ts-packages/quarto-hub-mcp/dist/index.js" ] } } diff --git a/claude-notes/instructions/hub-mcp-operator-runbook.md b/claude-notes/instructions/hub-mcp-operator-runbook.md index b03e4074..50460df8 100644 --- a/claude-notes/instructions/hub-mcp-operator-runbook.md +++ b/claude-notes/instructions/hub-mcp-operator-runbook.md @@ -6,22 +6,22 @@ end users. Sits alongside the SPA OAuth registration in both clients live in the same Google Cloud project. Design context: -[`claude-notes/plans/2026-05-05-hub-mcp-device-flow-implementation.md`](../plans/2026-05-05-hub-mcp-device-flow-implementation.md). +[`claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md`](../plans/2026-05-28-hub-mcp-loopback-pkce.md). ## What you're registering `quarto-hub-mcp` authenticates to the hub via Google's OAuth 2.0 -device-authorization grant (RFC 8628). That requires a second Google -OAuth client of type **"TV and Limited Input devices"** in the same -Google Cloud project as the SPA's existing "Web application" client. -The hub accepts ID tokens from either audience. +Authorization Code grant with PKCE and a loopback redirect (RFC 8252). +That requires a second Google OAuth client of type **"Desktop app"** +in the same Google Cloud project as the SPA's existing "Web +application" client. The hub accepts ID tokens from either audience. ## Step 1 — register the OAuth client 1. Open on the project that already hosts the SPA OAuth client. 2. **Create Credentials → OAuth client ID**. -3. **Application type → "TV and Limited Input devices"**. +3. **Application type → "Desktop app"**. 4. Name it something the audit log will read clearly, e.g. `quarto-hub-mcp`. 5. Click **Create**. Copy the **client_id** and **client_secret** off @@ -31,10 +31,14 @@ Notes: - The OAuth consent screen, brand assets, and verified scopes are shared with the SPA client. No extra consent-screen submission. -- The **client_secret is mandatory** for this client type — Google - refuses `/token` calls without it. This is empirically confirmed - (2026-05-19 verification log in the implementation plan); not a - configuration choice the operator can opt out of. +- The Desktop-app client also issues a **client_secret**, and Google + requires it on the token exchange and the refresh-token grant; PKCE + is layered on top of the confidential-client flow, not a replacement + for the secret. Google documents the installed-app secret as "not + treated as a secret," but it must still be distributed and set in the + env. No bundled default ships in the npm package for v1 — operators + (including the canonical-hub operator) publish both values to end + users. ## Step 2 — configure the hub @@ -100,13 +104,21 @@ their first agent action triggers the documented flow: 1. The MCP server probes the hub. Hub returns 401 (no creds). 2. hub-mcp surfaces `AuthRequired`, prompting the agent to call the - `authenticate_start` MCP tool. -3. Tool response carries `https://www.google.com/device`, a - short-lived `user_code`, and a hint URL from Google's response. -4. User opens the URL, enters the code, approves consent. -5. Agent calls `authenticate_finish`. The bundle is persisted to the - user's OS keyring under service `dev.quarto.hub-mcp`, account - `https://accounts.google.com:`. + `authenticate` MCP tool. +3. The tool binds a `127.0.0.1` listener and opens the user's browser + to Google's sign-in page (also printing the URL for headless/SSH + users). +4. User signs in and approves consent; the redirect lands on the local + listener. +5. The tool exchanges the authorization code (PKCE verifier + + client_secret) and persists the bundle to the user's OS keyring + under service `dev.quarto.hub-mcp`, account + `https://accounts.google.com:`. On success the agent + sees `"Authenticated as ."` and retries the original action. + +Users on headless or remote machines forward the loopback port over +SSH — see the package README's "Headless / SSH sessions" section +(`quarto-hub-mcp --redirect-port N` + `ssh -L N:127.0.0.1:N`). Subsequent connects refresh the ID token automatically and reuse the keyring entry until the user revokes the grant. @@ -141,19 +153,20 @@ CI logs, etc.): end user picks up the new value the next time their MCP client restarts (env vars are read at process start). 3. Existing user keyring entries are **not** invalidated — they hold - ID + refresh tokens that were issued by the old secret but are - redeemable against the new one (Google authenticates the - *device_code* per flow, not the secret directly). No user-side - action required after rotation unless an existing refresh token - itself is also compromised, in which case ask affected users to - revoke the grant (see below). + ID + refresh tokens that were issued by the old secret but remain + redeemable against the new one (the refresh grant authenticates with + whatever secret is currently configured). No user-side action + required after rotation unless an existing refresh token itself is + also compromised, in which case ask affected users to revoke the + grant (see below). If a **user's** refresh token leaks (rather than the operator's -secret), the user revokes the grant at - → "Third-party apps with -account access" → the hub-mcp client → "Remove Access". Their next -agent action surfaces `ReauthRequired` and runs through the device -flow again with a new bundle. +secret), the quickest fix is `authenticate_clear`, which best-effort +revokes the refresh token at Google before clearing the local copy. +The manual equivalent is → +"Third-party apps with account access" → the hub-mcp client → "Remove +Access". Their next agent action surfaces `ReauthRequired` and runs +through the loopback sign-in again with a new bundle. ## Residual risk to communicate to operators @@ -162,12 +175,20 @@ flow again with a new bundle. consult Google on each request. Closing this window requires a hub-side `sub_denylist` — not in v1; tracked in the plan's "Future work" section. -- **Refresh tokens are not rotated** by Google for this client type - (empirically confirmed 2026-05-19, 3/3 refreshes omitted the - field). A stolen refresh token is an indefinite foothold until - the user revokes the grant. -- **Headless Linux without Secret Service / libsecret cannot run - hub-mcp.** The credential store refuses silent fallback to a +- **A stolen refresh token is an indefinite foothold** until the user + revokes the grant (via `authenticate_clear` or + myaccount.google.com). hub-mcp persists a `refresh_token` only when + Google returns one and keeps the prior value otherwise, so rotation + behaviour does not change the residual; whichever value is current is + the one to revoke. (The Desktop-app client's exact rotation behaviour + is pending confirmation by the loopback+PKCE plan's Spike A.) +- **The loopback redirect closes the remote no-malware phishing class** + that device flow enabled: tokens are delivered only to the user's own + `127.0.0.1` listener, and PKCE binds the code to a verifier held in + the hub-mcp process. An attacker needs local code execution to + capture tokens — at which point the keyring is already exposed. +- **Headless Linux without Secret Service / libsecret cannot persist + credentials.** The credential store refuses silent fallback to a plaintext file. Users on those hosts use the SPA cookie path. See the implementation plan's *Threat model* and *Residual risks 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 new file mode 100644 index 00000000..2c83d7ec --- /dev/null +++ b/claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md @@ -0,0 +1,1191 @@ +# hub-mcp: replace device flow with Authorization Code + PKCE + loopback + +## Amendment 2026-05-28 + +Two changes to the plan as originally drafted, both settled by the user: + +1. **Phase 0 spikes are no longer a hard gate.** Implementation may + begin before the spikes are run. The spikes are retained below as + *recommended validation* to perform during Phase 1 / before merge, + not as a blocker for starting work. In particular, Spike B's + host-RPC-lifetime risk is real — if a host turns out to enforce a + short `tools/call` deadline, the two-tool fallback still applies — + but we proceed with the single-tool design and the 5-minute + deadline as the working assumption. +2. **Both `CLIENT_ID` and `CLIENT_SECRET` are kept.** Google's + "Desktop app" client type issues a `client_secret` and requires it + on the token exchange and refresh-token grant; PKCE is layered *on + top* of the confidential-client exchange, it does not replace the + secret. The original draft's premise that loopback+PKCE makes this + a secret-free public-client flow does not hold for Google as the + IdP. Consequently: `QUARTO_HUB_MCP_CLIENT_SECRET` is **retained**, + `oauth.ClientSecretPost(...)` stays in `refresh-manager.ts`, and no + `oauth.None()` public-client refresh construct is introduced. + + The justification for the device-flow → loopback switch now rests + entirely on the **threat-model improvement** (loopback structurally + defeats the no-malware remote-phishing class that device flow + enables — see Phase 4), not on secret elimination. The + secret-distribution operational friction is unchanged from today; + that is no longer claimed as a benefit. + +Sections below have been edited to reflect both changes. Where the +original "no secret" framing survives in prose, read it through this +amendment. + +## Overview + +Replace the device-flow auth path in `quarto-hub-mcp` with +Authorization Code + PKCE using a loopback redirect (RFC 8252). +Single new `authenticate` MCP tool replaces the existing +`authenticate_start` / `authenticate_finish` pair; the existing +`authenticate_clear` tool is retained on the MCP surface as the +escape hatch when the hub rejects cached credentials, with one +behavioural addition — best-effort revocation of the stored refresh +token at Google's revocation endpoint before the local keyring +delete. Today's `handleClear` is documented as not touching +Google-side grants; this plan closes that gap so "clear" actually +means "the credential cannot be used anywhere," which is the +contract a user clearing-on-suspected-compromise needs. + +Both `QUARTO_HUB_MCP_CLIENT_ID` and `QUARTO_HUB_MCP_CLIENT_SECRET` +are retained (see Amendment 2026-05-28): Google's Desktop-app client +type requires the secret on the token exchange and refresh grant, so +PKCE is layered on top of the confidential-client flow rather than +replacing it. The change from device flow to loopback is justified by +the threat-model improvement alone (Phase 4), not by secret +elimination. + +Device flow is removed, not kept as a fallback. Headless users +without a browser fall back to either SSH port-forwarding the +loopback URL (`ssh -L`) or the existing SPA-cookie path from a +graphical session. + +For v1 the `client_id` is **not** bundled as a default in the npm +package — operators (including the canonical-hub operator) publish +it to end users the same way they publish the hub WebSocket URL. +Bundling is deferred as future work. + +## Why + +The current device-flow design (see +[`2026-05-05-hub-mcp-device-flow-implementation.md`](2026-05-05-hub-mcp-device-flow-implementation.md)) +assumes an operator-to-end-user relationship: the operator registers +an OAuth client at Google and distributes `client_id` + `client_secret` +to each user through a secret-management channel. That model is +correct for internal-developer hubs (the case the original plan +solved for) and is workable — but device flow has a structural +phishing weakness that loopback does not. That weakness, not the +secret, is the reason to switch. + +Two design alternatives were considered: + +1. **Keep device flow** (bundled or operator-distributed + `client_id` + `client_secret`). Works, but device flow uniquely + enables the no-malware remote-phishing class documented in Phase 4 + (Storm-2372-style attacks). The secret-distribution requirement is + the same under loopback, so it is not a differentiator either way. +2. **Authorization Code + PKCE + loopback** (this plan). Standard + pattern for installed native apps per RFC 8252. Used by `gcloud + auth login`, `aws sso login`, `gh auth login`, `firebase login`, + `terraform login`. With Google's Desktop-app client type the + `client_secret` is still sent on the token exchange and refresh + (Google requires it; PKCE is layered on top), so this is not a + secret-free public-client flow — but the loopback `redirect_uri` + collapses the remote-attack surface to a local-attack surface, + which is the win. See Phase 4 threat-model section. + +(2) is the smallest delta that gives a strictly stronger threat model +than today. + +**What this plan does not solve:** the original public-hub motivation +("end users with no operator relationship") is unchanged — end users +still need two values (`client_id` + `client_secret`) from the +canonical-hub operator. Loopback does not reduce that config burden; +it improves the threat model. Closing the config-burden gap requires +bundled defaults (deferred). + +### PKCE-on-device-flow was considered and rejected + +RFC 9700 §4.13 recommends PKCE for device flow; Google does not +support it on `/device/code`. Even if Google added support, PKCE +on device flow protects against `device_code` leakage *between +clients on the same device*, not against the no-malware remote +phishing threat that loopback structurally prevents (see Phase 4). +Loopback is the actually-stronger position. + +## Scope decisions + +These are settled. Reopen only with explicit user discussion. + +1. **Stay on Google as the IdP.** Same identity provider as today, + same hub audit story, smaller blast radius. +2. **Do NOT bundle the `client_id` / `client_secret` in the npm + package for v1.** Operators — including the canonical-hub operator + — publish both values to end users alongside the hub WebSocket + URL. End users set `QUARTO_HUB_MCP_CLIENT_ID` and + `QUARTO_HUB_MCP_CLIENT_SECRET`. Bundling is future work pending + operational signal on end-user friction. +3. **Replace device flow entirely.** Do not keep as a fallback. + Removes one auth path, one MCP tool pair, and one OAuth client + type (the "TV and Limited Input devices" type is swapped for the + "Desktop app" type). Both env vars (`QUARTO_HUB_MCP_CLIENT_ID` and + `QUARTO_HUB_MCP_CLIENT_SECRET`) are retained — Google's Desktop-app + client requires the secret on token exchange. Headless users + without a browser use SSH loopback port-forwarding or the SPA + cookie path; documented in Phase 3. +4. **Single `authenticate` MCP tool.** Replaces `authenticate_start` + + `authenticate_finish`. Returns when the loopback listener + fires (success), the user cancels (error), or the timeout + expires (error). `authenticate_clear` stays on the MCP surface + — it is independent of the device-flow / loopback choice and + remains the documented recovery action for stale cached + credentials — but its implementation gains best-effort Google- + side refresh-token revocation (see Phase 1 "authenticate_clear + revocation" item). The public contract widens from "delete the + local copy" to "render the credential unusable, locally and at + Google." +5. **Concurrency: rely on the MCP host's request serialisation, + do not add a single-flight guard.** MCP stdio servers are + single-client by design (the host launches `quarto-hub-mcp` + as a subprocess and is the sole peer), and host agent loops + issue `tools/call` requests serially — the next call doesn't + leave the host until the previous `CallToolResult` arrives. + Two `authenticate` invocations in flight at the same time is + therefore a host-bug case, not a normal-operation case. The + same logic applies to `authenticate_clear` arriving while + `authenticate` is in flight: under a well-behaved host it + cannot happen. We accept the narrow misbehaving-host failure + mode (two listeners on different kernel-picked ports, confusing + browser UX) rather than carry single-flight bookkeeping for an + event that shouldn't occur. This is *not* a security + concession — `state` and PKCE bind each flow independently, so + tokens from one flow cannot be redirected into another. See + the Phase 1 single-`authenticate` tool item for the documented + assumption and the residual UX note. + +## Phase 0 — recommended validation spikes (no longer blocking) + +**Amended 2026-05-28: these spikes no longer gate Phase 1.** +Implementation may proceed against the working assumptions baked into +Phase 1 (5-minute deadline, `ClientSecretPost` refresh, `prompt=consent` +on by default). The spikes remain valuable validation to run during +Phase 1 or before merge; if one surfaces a problem, the fallback shape +is spelled out below and the affected Phase 1 items are revised. They +are documented here as the two structural risks worth confirming +against real systems, not as a precondition for starting work. + +- [ ] **Spike A — Google "Desktop app" client type + loopback + PKCE + against a real hub audience allowlist.** Register a throwaway + Desktop-app client in a test-tier Google project, drive a full + auth-code+PKCE round-trip from a Node script against the + production discovery endpoint, exchange the code, decode the + ID token, and verify: + - the `aud` claim equals the new `client_id`, + - the hub (with the test `client_id` added to + `--additional-audiences`) accepts the resulting bearer token on + a real WebSocket connect, + - refresh-token rotation behaviour is unchanged from the Limited- + Input-Devices client (the persistence rule documented in + `refresh-manager.ts`'s top-of-file comment still holds), + - **second-run refresh-token return.** Run the full flow twice + with the **same** Google account and **without** + `prompt=consent` on the second authorization request. Record + whether the second `/token` response body contains a non-empty + `refresh_token`. This decides whether we can drop the + `prompt=consent` default for returning users (see Phase 1 + Authorization URL construction). If the second run returns no + `refresh_token`, the default stays on; do not drop it on a hunch. + - **refresh construct under the Desktop-app client.** Confirm the + existing `oauth.ClientSecretPost(clientSecret)` construct in + `refresh-manager.ts` still works for the Desktop-app client's + refresh-token grant (it should — Desktop-app is a confidential + client to Google, same as the LID client). Run an end-to-end + refresh against Google and confirm the wire payload **includes** + `client_secret`. (Superseded by Amendment 2026-05-28: the secret + is kept, so there is no `oauth.None()` public-client construct to + pin. This bullet is now a regression check that the existing + construct still applies, not a new-symbol spike.) + - **Fail condition:** Google rejects loopback for Desktop-app + clients, or `aud` does not match `client_id`, or the hub + rejects the token despite allowlisting. → fall back to bundled- + defaults (alternative 1 in the "Why" section) and re-open the + secret-distribution discussion. The threat model in Phase 4 + still applies; the operational story changes. + +- [ ] **Spike B — MCP host tool-call lifetime.** The whole design + assumes the host (Claude Code, Cursor) keeps a `tools/call` + RPC alive for up to 5 minutes while the user signs in. This + is **not** something we control — if the host enforces a + shorter deadline (e.g. 60 s) the listener and browser are + torn down mid-flow and the single-tool surface is unviable. + Test both hosts against a stub MCP server whose tool handler + sleeps for N seconds before responding, sweep N ∈ {30, 60, + 90, 120, 300}, and record the timeout floor each host + enforces. Run on: + - Claude Code (latest stable), + - Cursor (latest stable), + - the project's own `mcp-test-client.ts` for a sanity baseline. + - **Success condition:** every host under test holds the RPC for + ≥300 s without cancelling. Then set the `authenticate` + deadline to 5 min as planned. + - **Partial success (some host floor < 300 s but ≥ 60 s):** + shorten the `authenticate` deadline to fit the lowest floor + minus a 10 s safety margin, document the floor in the README, + and proceed with the single-tool design. + - **Fail condition (any host floor < 60 s):** the single-tool + design does not survive. Fall back to a two-tool shape — + `authenticate_begin` returns the listener URL and starts the + listener; `authenticate_await` polls the listener state. + Loopback + PKCE still applies; only the MCP surface changes. + Add this as a separate plan and pause this one. + +Record spike outcomes in this plan's verification log as they are +run. Per Amendment 2026-05-28 they no longer block the Phase 1 PR, +but a merged Phase 1 should still carry the Spike B host-deadline +result (it determines the final `authenticate` timeout constant) and +the manual end-to-end check. + +## Phase 1 — loopback+PKCE implementation + +### Pre-work: module reshuffle + +`auth/device-flow.ts` exports 16 symbols; before Phase 3 deletes the +file, every non-device-flow-specific symbol needs a new home so the +deletion really is a clean `git rm`. Full inventory: + +| symbol | disposition | new home | +| --- | --- | --- | +| `MissingCredentialsConfigError` | **rename + move** to `MissingOAuthConfigError` | `src/auth/oauth-config.ts` | +| `redactTokens` | move | `src/auth/redact.ts` | +| `DeviceFlowEnvConfig` (interface) | **rename + move** to `OAuthEnvConfig` | `src/auth/oauth-config.ts` | +| `loadDeviceFlowConfigFromEnv` | **rename + move** to `loadOAuthConfigFromEnv`; reads **both** `QUARTO_HUB_MCP_CLIENT_ID` and `QUARTO_HUB_MCP_CLIENT_SECRET` (unchanged behaviour — secret retained per Amendment 2026-05-28) | `src/auth/oauth-config.ts` | +| `discoverAuthorizationServer` | move | `src/auth/oauth-config.ts` | +| `_resetDiscoveryCache` (test hook) | move — must live with the cache it resets | `src/auth/oauth-config.ts` | +| `AuthorizationServerSpec` (interface) | **delete** with device-flow | — | +| `buildAuthorizationServer` | **delete** with device-flow (only `device-flow.test.ts` uses it) | — | +| `DeviceFlowError`, `DeviceFlowDeniedError`, `DeviceFlowExpiredError` | **delete** with device-flow | — | +| `DeviceFlowClientConfig`, `DeviceFlowRequestOptions` (interfaces) | **delete** with device-flow | — | +| `PollResult` (type) | **delete** with device-flow | — | +| `initiateDeviceFlow`, `pollDeviceFlowOnce` | **delete** with device-flow | — | + +Verified (`grep` over `src/`) that `buildAuthorizationServer` / +`AuthorizationServerSpec` have no callers outside +`device-flow.test.ts`, which itself is deleted in Phase 3 — so they +go with the file, not into `oauth-config.ts`. + +- [x] Create `src/auth/oauth-config.ts` containing + `loadOAuthConfigFromEnv`, `OAuthEnvConfig`, + `MissingOAuthConfigError`, `discoverAuthorizationServer`, + and `_resetDiscoveryCache`. +- [x] Create `src/auth/redact.ts` with `redactTokens` and its + `TOKEN_PATTERN` regex (small, no deps). +- [x] Update imports in `src/index.ts`, `src/tools.ts`, + `src/auth/auth-tools.ts`, and any other in-tree caller so they + point at the new modules. After this pre-work, + `device-flow.ts` exports *only* device-flow-specific symbols; + Phase 3 then `rm`s the file with no dangling references. + +### Implementation + +Per Amendment 2026-05-28 there is no spike pre-condition. Implement +against the working assumptions noted inline (5-minute deadline, +`ClientSecretPost` refresh, `prompt=consent` on). Run the Phase 0 +spikes as validation during this phase or before merge; revisit the +relevant item only if a spike contradicts an assumption. + +- [x] PKCE primitives. Reuse `oauth4webapi` (already a project dep): + `generateRandomCodeVerifier()`, `calculatePKCECodeChallenge()`, + `generateRandomState()`. Rationale: the rest of the auth code + already routes through `oauth4webapi`; rolling our own ~20-line + version with Node `crypto` would diverge from the established + pattern for no gain. Wrap in a thin `src/auth/pkce.ts` only if + a stable surface is needed for tests. +- [x] Local HTTP listener in `src/auth/loopback.ts`: + - Bind to the **literal** `127.0.0.1`, not `localhost`. `localhost` + resolves via DNS / `/etc/hosts` and has historically been + hijackable on some installed-app stacks; the IP literal is the + fix RFC 8252 §7.3 recommends. v1 is IPv4 only; if we ever see a + user on a v6-only loopback we'll add `::1` then. + - Port selection: default to `0` (kernel-picks) for the normal + desktop case; honour an explicit override from the + `--redirect-port ` CLI flag (see below) for SSH-tunnel + users. The chosen port flows into `redirect_uri` verbatim and + must match byte-for-byte at the token-exchange step. + - Single route `/callback` accepting `?code=&state=` or + `?error=&state=`; any other path → 404 (do not echo path). + - **Host-header allowlist (DNS-rebinding defence).** Before + parsing query params, reject (`400`, empty body, no listener + teardown) any request whose `Host` header is not exactly + `127.0.0.1:` for our bound port. The bind to the IP + literal closes outbound rebinding, but a malicious tab open in + the user's browser during the auth window can still POST/GET to + `http://attacker-controlled-name/callback?code=…` and — if that + name resolves (via cached A-record rebinding) to `127.0.0.1` + after the initial fetch — land on our listener. The fixed-host + check is the standard RFC 8252 §8.4 mitigation. Reject **before** + state validation so attackers can't probe for active flows by + timing the response. Add a Phase 1 test that hits the listener + with a forged `Host: evil.example` header and asserts a 400 with + the listener still bound. + - On hit: validate `state` in constant time (CSRF defence), serve + the success HTML response (see next item), then resolve the + promise and close the listener. + - 5-minute timeout, 1-shot listener (close on first valid hit, + error response from Google, or timeout). + - Clean shutdown on SIGINT/SIGTERM — the listener registers and + unregisters its own handlers so they don't outlive the flow. +- [x] Callback response page — security-spec'd, not free-form HTML: + - Status: `200 OK`; respond fully **before** closing the listener + so the browser doesn't show a connection-reset error. + - Body: single self-contained HTML doc with inline CSS, **no + JavaScript**, no external resources (no fonts, images, fetches). + Eliminates exfiltration and referer-leak vectors. + - Headers: `Content-Type: text/html; charset=utf-8`, + `Cache-Control: no-store`, `Referrer-Policy: no-referrer`, + `X-Content-Type-Options: nosniff`, + `Content-Security-Policy: default-src 'none'; style-src 'unsafe-inline'`. + - Copy: "Authenticated. You can close this tab." On error, + surface the error code from `?error=` (already attacker-known) + plus the same "you can close" guidance. +- [x] Browser opener — cross-platform, with manual-URL fallback: + - macOS: `open ` (pass the URL as a single argv element to + `spawn`, no shell). + - Linux: `xdg-open ` (same — single argv element, no shell). + - Windows: **`cmd.exe /c start "" ""`** — invoked via + `spawn('cmd.exe', ['/c', 'start', '', url], { windowsVerbatimArguments: false })`. + Two gotchas the literal `start ` form gets wrong: + 1. `cmd.exe` treats `&` as a statement separator, and OAuth + authorization URLs are dense with `&` (`response_type=code&client_id=…&redirect_uri=…&state=…`). + Quoting the URL prevents that interpretation. + 2. `start`'s first quoted argument is the *window title*, not the + URL. The empty `""` is a placeholder title so the quoted URL + is parsed as the target. Without it, `start "https://accounts.google.com/…"` + opens an empty CMD window titled with the URL and never + launches the browser. + A test fixture asserting the constructed argv vector matches the + expected `['/c', 'start', '', '']` form is the + cheapest way to keep this from regressing. + - Default to in-tree ~15-line helper using `child_process.spawn`; + revisit bundling the `open` npm package if corner cases warrant. + - **Failure handling does not change the control flow** — the + listener is bound *before* the browser is launched and the + `authenticate` tool blocks on it regardless of whether the + launch succeeded. On non-zero exit / spawn error the only + observable difference is the tool's eventual response text: + on success-after-fallback it appends a one-line note + ("Browser launch failed; you signed in manually."); on + timeout-after-fallback the error text includes the manual-paste + URL so the user can retry. We never return early on launch + failure — the user may still complete the flow by pasting the + URL into a browser themselves, and the listener has to be live + when that callback lands. + - **Always surface the URL up-front, not only on launcher + error.** Immediately after the listener binds and *before* + invoking the browser opener, surface the full authorization URL + via **two channels simultaneously** (not redacted — the URL is + public, and the listener port + PKCE verifier already gate + misuse): + 1. **MCP `notifications/progress` against the `tools/call` + progress token.** This is the primary, host-UI-visible + channel: hosts that implement the MCP progress UI (Claude + Code, Cursor) surface progress notifications inline with the + in-flight tool call, exactly where the user is already + looking. Construction: + - **Only fire if** the incoming `CallToolRequest` carried a + `_meta.progressToken` (per MCP spec, progress notifications + are *only* valid when the caller requested progress). If the + token is absent, skip silently — do not synthesise a token, + do not error. The MCP SDK exposes this on the request-handler + context; verify the accessor name against the SDK version in + `package.json` during Phase 1 (same accessor-audit note as + the `AbortSignal` wiring above — do them together). + - Send one notification at bind-time with the URL in the + `message` field, `progress: 0`, `total: 1`. Progress + notifications carry no semantic state beyond + message-of-the-moment for this flow; do not emit a stream of + them. + - On any settle (success / error / abort / timeout) do not + emit a closing progress notification — the `CallToolResult` + response is itself the terminal signal per MCP semantics. + 2. **stderr at INFO level.** Belt-and-braces for hosts that + don't surface MCP progress, for SSH-tunnel users tailing the + remote process, and for `mcp-test-client.ts`. Writing to + stderr at bind-time means any host whose UI tails the + subprocess's stderr always has an actionable link without + waiting out the deadline. + + Rationale for both: on many headless Linux / SSH-only machines + `xdg-open` exits 0 successfully but opens nothing visible to the + user, who would otherwise see a spinner for the full deadline + (up to 5 min) before the timeout-path response surfaces a URL. + MCP progress is the right channel by spec; stderr is the + universal fallback. The launcher-failure response-text fallback + above stays as-is — it is the third line of defence for users + who see neither. + + **Test additions for this dual-channel surfacing (added to the + Phase 1 test list):** + - Invoke `handleAuthenticate` with a `progressToken` in + `_meta`; assert exactly one `notifications/progress` is sent + at bind-time, with `progress: 0`, `total: 1`, and `message` + containing the full authorization URL. + - Invoke `handleAuthenticate` *without* a `progressToken`; + assert zero progress notifications are sent and the flow + otherwise proceeds normally (no synthesised token, no error). + - On settle (success, timeout, abort) assert no further + progress notifications fire — the `CallToolResult` is the + terminal signal. +- [x] Authorization URL construction: + - Endpoint: `https://accounts.google.com/o/oauth2/v2/auth` + - Params: `response_type=code`, `client_id`, + `redirect_uri=http://127.0.0.1:/callback`, + `scope=openid email profile`, `code_challenge`, + `code_challenge_method=S256`, + `state` from `oauth4webapi.generateRandomState()` (≥128 bits of + entropy, base64url-encoded; per `oauth4webapi`'s implementation + this draws from `crypto.getRandomValues`). + - **`access_type=offline` — required.** Google only issues a + `refresh_token` when this is set; the default (`online`) returns + an `id_token` but no `refresh_token`, which would silently break + every code path in `refresh-manager.ts` and force the user to + re-authenticate each time the ID token expires (~1 h). Add a + Phase 1 token-exchange test that asserts the response body + contains a non-empty `refresh_token` — this is the regression + guard for accidentally dropping the param. + - **`include_granted_scopes=true` — set.** Lets a future scope + addition piggy-back on the existing grant rather than forcing a + fresh consent screen. No cost today; pays off if we ever add a + scope. + - **`prompt=consent` — set by default.** Not a UX preference: for + Google's Desktop-app client, a *returning* user who has already + consented to the app is frequently issued a new `id_token` + *without* a `refresh_token` on subsequent authorizations even + with `access_type=offline`. The Phase 1 regression test only + catches dropped params on a fresh consent; without + `prompt=consent` we would silently break every returning user's + refresh path in production while every test passes. Spike A + measures this directly (see Phase 0 Spike A's "second-run + refresh-token return" item); if the spike confirms returning + users *do* receive a refresh_token without `prompt=consent`, we + may drop it for the UX win, but the default until then is on. +- [x] Token exchange at `https://oauth2.googleapis.com/token`: + - POST `grant_type=authorization_code`, `code`, `redirect_uri` + (**byte-exact** match of the value sent in the auth request), + `client_id`, `client_secret`, `code_verifier`. (Per Amendment + 2026-05-28 the `client_secret` is sent — Google's Desktop-app + client requires it on the token exchange; PKCE's `code_verifier` + is sent in addition, not instead.) + - Use `oauth.ClientSecretPost(clientSecret)` as the client-auth + construct, matching the existing refresh path, so the secret + travels in the request body the same way it does today. + - Returns the same JSON shape as the old device-flow token + response → existing keyring storage code reused unchanged. +- [x] Refresh-token handling: **unchanged** (Amendment 2026-05-28). + `src/auth/refresh-manager.ts` keeps calling + `oauth.ClientSecretPost(this.deps.config.clientSecret)` at + line ~127 — the Desktop-app client is confidential to Google + and the secret is required on the refresh grant, same as today. + `clientSecret` stays in `RefreshClientConfig` + (refresh-manager.ts:49) and in `AuthFlowConfig` + (auth-tools.ts:60). The refresh request stays + `grant_type=refresh_token` + `client_id` + `client_secret` + + `refresh_token`. (The original draft swapped this to a + public-client `oauth.None()` construct; that change is dropped.) +- [x] Update the `refresh-manager.ts` top-of-file doc-comment + (currently lines 19–26, the **"Refresh-token persistence rule"** + paragraph). It documents an empirical 2026-05-19 finding that + Google does not rotate refresh tokens for the **Limited-Input- + Devices** client type. That observation is no longer load- + bearing once we switch to the Desktop-app client. Replace it + with the rotation behaviour Spike A actually records (verbatim + from the Verification log) — one of: + - "Spike A 2026-MM-DD confirmed Desktop-app client does not rotate + refresh tokens; the defensive 'keep prior on missing field' rule + below still applies and is the correct fallback for any future + IdP change.", or + - "Spike A 2026-MM-DD confirmed Desktop-app client **does** rotate + refresh tokens on every grant; the defensive rule below is now + also the steady-state path and persists each new value." + + The defensive rule itself (persist when present, keep prior + when absent) stays as-is — it's correct under both behaviours. + Only the empirical paragraph rotates. Do not leave the LID- + client reference in place; a future reader will assume the + observation is current and reason about the wrong IdP client + type. +- [x] Single `authenticate` MCP tool: + - **Pre-flight short-circuits — match today's `handleStart` + behaviour at `auth/auth-tools.ts:196–220`, do not regress + either:** + 1. **Already-authenticated.** Call + `refreshManager.getValidIdToken()` first. If it returns, + respond `"Already authenticated as . No action + needed."` without binding a listener or opening a browser. + Only `ReauthRequired` falls through to the loopback path; + every other error propagates (network blip, malformed JWT, + etc. — same rationale as today). + 2. **Hub does not require auth.** If + `connectionManager.lastObservedAuthMode() === 'no-auth'`, + respond `"The configured hub does not require + authentication; no action needed."`. Only the positive + `'no-auth'` observation triggers — `'requires-auth'` and + `'unknown'` both fall through to the loopback flow, same as + today. + These run before any network or listener I/O. + - Opens browser, runs listener, exchanges code, stores tokens in + keyring on success. + - Returns `"Authenticated as ."` on success. + - Returns typed error on timeout, user cancel, listener failure, + or token-exchange failure. + - **Concurrency — none.** No `inflight` slot, no single-flight + guard, no `finishTail`-style serialisation. The MCP host + serialises its own `tools/call` requests (it awaits each + `CallToolResult` before issuing the next), so two concurrent + `authenticate` invocations cannot arise under a well-behaved + host. Code a single handler that binds a listener, opens a + browser, and returns — no shared state to protect. If a + misbehaving host *does* double-fire, both flows run + independently on different kernel-picked ports; PKCE and + `state` bind each flow's tokens to its own callback so this + is a UX defect, not a security one. Do not borrow the + `RefreshManager.forceRefresh` or `finishTail` patterns — they + were correct for their problems (genuine token-cache mutation + races; device-flow finish-call coalescing) and are not needed + here. Document the assumption in a single comment at the + top of `handleAuthenticate`: "MCP stdio hosts serialise + `tools/call`; this handler intentionally has no concurrency + guard. Two simultaneous calls is undefined-but-non-corrupting + behaviour." + - **MCP-host cancellation mid-flow.** Independent of + concurrency — a single in-flight `authenticate` can be + cancelled by the host (e.g. when the user clicks cancel on + the host's tool-progress UI) via `notifications/cancelled`. + Thread the `AbortSignal` exposed by the MCP SDK's + `CallToolRequest` handler context (`extra.signal` on + `@modelcontextprotocol/sdk` ≥1.x — verify the exact accessor + against the version actually in `package.json` during Phase + 1; do this at the same time as the progress-token accessor + audit) into both the loopback listener (which closes its + `http.Server` on abort) and the browser-opener subprocess + (which is killed on abort). Without this wiring, a host + cancel leaves the listener bound and the browser subprocess + alive until the deadline fires. Phase 1 test: start + `handleAuthenticate`, fire `abort()` on the signal, assert + the listener is closed, the browser-opener subprocess (if + still alive) is killed, the promise rejects with a typed + cancellation error, and a follow-up `handleAuthenticate` is + accepted normally (no stale state from the cancelled flow). + - **Timeout:** governed by Spike B's outcome. If every host holds + RPCs ≥300 s the deadline is 5 min (matches the device-flow + plan); otherwise it is the lowest host floor minus a 10 s + safety margin, per Spike B's "partial success" branch. The + chosen value lives in a single named constant in + `auth/loopback.ts` so README + tests reference one source. +- [x] **Logging policy for the new flow** — explicit because the + existing code is consistent and we shouldn't drift: + - Every log call site reachable from the loopback path + (`auth/loopback.ts`, `auth/oauth-config.ts`, the new + `authenticate` handler) funnels through `redactTokens` (now at + `auth/redact.ts` per the module reshuffle), same rule as the + five existing sites: `connection-manager.ts:185, 366`, + `tools.ts:405`, `index.ts:79, 86, 184`, + `auth/credential-store.ts:158, 174, 186`, + `auth/auth-tools.ts:283, 331, 335`. + - The authorization URL is **explicitly not** routed through + `redactTokens`. It is public by construction (browser address + bar, server access logs, referer headers absent only because + we set `Referrer-Policy: no-referrer` on *our* response). + Redacting it would defeat the stderr-at-bind-time mitigation + in the Browser opener item, which is the actionable URL + surface for headless machines. + - The kernel-picked port (when `--redirect-port` is omitted) is + logged to stderr at INFO level on bind so SSH-tunnel users + aren't forced to set `--redirect-port` just to learn the port. + - The `code_verifier`, the auth `code`, the `state` value, and + every token field are never logged in any branch, redacted or + otherwise — `redactTokens` handles the token shapes by + pattern, but `code_verifier` / `state` / `code` do not match + those patterns and must be filtered at the call site (i.e. + don't pass them in). Add a Phase 1 review-checklist note for + this; mechanical enforcement is out of scope. +- [x] Remove `authenticate_start` and `authenticate_finish` MCP tools. + Keep `authenticate_clear` on the MCP surface (see next item + for its one behavioural change). +- [x] **`authenticate_clear` revocation.** Today's `handleClear` + (`ts-packages/quarto-hub-mcp/src/auth/auth-tools.ts:272`) + clears the in-memory cache and the keyring entry; its tool + description (line 120) explicitly disclaims any Google-side + effect. The stored refresh token is long-lived at Google and + remains usable until the user goes + to `myaccount.google.com` and revokes the grant by hand. + That's the wrong default for an "escape hatch when the hub + rejects cached credentials" tool — if a user is clearing + because they suspect compromise, leaving a working refresh + token at Google is the worst possible outcome. Implementation: + - **Order: read → revoke → delete.** Read the refresh token from + the keyring first; if present, POST to the revocation endpoint + *before* the local delete. Reading first means a revoke-then- + delete crash leaves the next `authenticate_clear` call able to + retry the revoke. Deleting first would orphan the token. + - **Revocation endpoint.** Discover via the existing + `discoverAuthorizationServer` (after the Phase 1 module reshuffle + it lives in `auth/oauth-config.ts`) and use the + `revocation_endpoint` field from the OIDC discovery doc. For + Google this is `https://oauth2.googleapis.com/revoke` today; do + not hardcode — discovery is already a sunk cost in the + authenticate path. POST `token=` + + `token_type_hint=refresh_token` as `application/x-www-form- + urlencoded`. Google's revocation endpoint does not require client + authentication — POST the `token` alone (the token itself is the + capability being burned). Do not send `client_id` / + `client_secret` on the revoke even though they are configured for + the token-exchange path; the revoke endpoint neither needs nor + expects them. + - **Revoking the refresh token also invalidates any access tokens + derived from it** (per RFC 7009 §2.1 and Google's documented + behaviour). One revoke call suffices; do not separately revoke + the ID token — ID tokens are JWTs and can't be revoked at the + 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). + - **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 + revoke is an extra layer; if it fails, the user is no worse off + than today's behaviour. The response text distinguishes the + cases: + - All clean: `"Quarto Hub credentials cleared and revoked at + Google. Call authenticate to sign in again."` + - Local delete OK, revoke failed: `"Quarto Hub credentials + cleared locally. Google-side revocation failed (); revoke the grant at myaccount.google.com if you + need it gone server-side."` (Short reason = HTTP status + + `error` field if RFC-6749-shaped, or a redacted exception + message otherwise — `redactTokens` from `auth/redact.ts`.) + - Local delete failed: existing error path (line 282) is + already correct; mention whether revoke ran in the message so + the user knows the Google-side state. + - **No revoke when no refresh token present.** `authenticate_clear` + is idempotent today (`safe to call when no credentials are + present`) and that contract is preserved. If the keyring read + returns no entry, skip the revoke entirely and return the + existing "nothing to clear" response shape. + - **Logging.** The revoke POST and its response go through the + same `redactTokens` policy as every other log site (the refresh + token itself must never appear in logs, including in error + messages — the response-text contract above is the only place + a short reason surfaces, and that path is already filtered). + - **Tool-description text.** Rewrite + `AUTH_TOOL_DEFINITIONS[authenticate_clear].description` (lines + 115–122) to reflect the new contract: drop the "Does not touch + Google-side grants" disclaimer, replace with one sentence + naming the best-effort revoke and pointing at + `myaccount.google.com` only as the manual-recovery path if + revocation fails. This is in addition to the "discard any in- + progress device-flow state" → "discard any in-progress sign-in" + rewording already listed in the in-tree sweep item below. + - **Success-message text.** The current `handleClear` returns + `"Quarto Hub credentials cleared. Call authenticate_start to + authenticate again."` (line 287). Update to reference + `authenticate` and use one of the two response strings above. + Add to the in-tree sweep checklist for completeness. +- [x] Sweep in-tree references to the old tool names. Known sites + (verified by `grep` 2026-05-28): + - `src/tools.ts:46` — `connect_project`'s description text + embeds `"call authenticate_start to begin the device-flow"`; + rewrite to name `authenticate` and drop "device-flow". + - `src/tools.ts:386–393` — the dispatcher branch matches + `'authenticate_start' | 'authenticate_finish' | 'authenticate_clear'`; + collapse to `'authenticate' | 'authenticate_clear'`. + - `src/auth/auth-tools.ts` `AUTH_TOOL_DEFINITIONS` (lines ~80–130) + — replace the two old `Tool` entries with a single + `authenticate` entry; the `authenticate_clear` entry stays but + its description needs two changes (covered in detail by the + "`authenticate_clear` revocation" item above): "discard any + in-progress device-flow state" → "discard any in-progress + sign-in"; "Does not touch Google-side grants; revoke those at + myaccount.google.com if needed" → one sentence naming the + best-effort revoke and the manual-recovery URL only as a + fallback when revocation fails. + - `src/mcp-test-client.ts` — audit for hard-coded tool names used + by integration tests; update call sites. + - `src/connection-manager.ts` error messages — audit for any + "call `authenticate_start`" hints surfaced on `AuthRequired` / + `ReauthRequired` paths, rewrite to `authenticate`. + - `README.md` and `claude-notes/instructions/hub-mcp-operator-runbook.md` + are covered separately under Phase 2. +- [x] **Keep** `QUARTO_HUB_MCP_CLIENT_SECRET` (Amendment 2026-05-28). + `loadOAuthConfigFromEnv` still requires both `CLIENT_ID` and + `CLIENT_SECRET`; a partial config (one set, the other not) fails + with the existing `MissingOAuthConfigError` naming the missing + var. No removal, no migration-pointer error for the secret. +- [x] Leave `hasAuthEnv` in `src/index.ts` (currently + `CLIENT_ID || CLIENT_SECRET` at line ~99) **as-is** — both vars + still participate. The "either var present → attempt auth + bootstrap; neither present → run no-auth" behaviour is + unchanged, and `loadOAuthConfigFromEnv` remains the single point + that rejects a partial config. Preserve the "no-auth hubs still + work" path explicitly. +- [x] Add `--redirect-port ` CLI flag in `src/index.ts` + alongside `--server` / `--read-only`. Validates as a TCP + port (`1..=65535`), defaults to `0` (kernel-picks) when + absent, threads through to the loopback listener and into + `redirect_uri`. This is the bridge that makes the SSH + port-forwarding story in Phase 3 actually work — without a + known port, `ssh -L :127.0.0.1:` has nothing to + point at. Implementing it in Phase 1 keeps the Phase 3 + headless documentation accurate when it ships. +- [x] Existing tests that need to be torn down or rewritten before + new tests land — this is **not** a touch-up, the device-flow + assumptions are baked in deeply: + - `src/auth/auth-tools.test.ts` (841 lines) — the entire test + surface is `handleStart` / `handleFinish` / the coalesce window + / the `finishTail` mutex chain / `authorization_pending` / + `slow_down` polling. None of these concepts survive. Rewrite + the file end-to-end against the new `handleAuthenticate` + surface; keep only `handleClear` tests, which carry over with + one extension — the four new revocation cases listed in the + test additions further down. + - `src/auth/refresh-manager.test.ts` (524 lines) — **no change** + required (Amendment 2026-05-28). It asserts the refresh request + includes `client_secret` via `ClientSecretPost`, which remains + correct. The `FAKE_CLIENT_SECRET` fixture (line ~31) stays. + - `src/auth/device-flow.test.ts` (535 lines) — deleted wholesale + in Phase 3 alongside `device-flow.ts`. Anything still useful + (e.g. the `redactTokens` known-answer cases) gets lifted into a + new `src/auth/redact.test.ts` first. + - `src/hub-mcp.test.ts` (379 lines) and + `src/connection-manager.test.ts` (543 lines) — audit for + references to the old tool names and for any fixtures that set + both `CLIENT_ID` and `CLIENT_SECRET`. Adjust as needed; these + do not require a full rewrite. +- [x] New unit tests (match the project's existing mocking pattern + — `nock` is not in use here, do not introduce it): + - PKCE: known-answer tests against RFC 7636 §4.6 example values + (whether using the `oauth4webapi` helpers directly or through + our thin wrapper). + - Listener: spin up listener, fire a `fetch` at it, verify the + returned promise resolves with `code`; verify state mismatch + rejects with a typed error; verify the response includes the + spec'd security headers. + - Sequential reuse: call `handleAuthenticate` once and let it + settle (success, rejection, or abort); a follow-up call must + be accepted normally. This is the only concurrency-shaped + test — there is no single-flight guard to exercise, but we + do want to confirm the handler leaves no stale state behind + (listener closed, subprocess reaped, no resident timers) that + would interfere with the next call. The host-serialised model + means this is the realistic worst case. + - SIGINT mid-flight: listener tears down cleanly and the in-flight + promise rejects with a typed cancellation error. + - MCP-host cancellation mid-flight: invoke `handleAuthenticate` + with an `AbortSignal`, fire `abort()` while the listener is + waiting, assert the listener is closed, the browser-opener + subprocess (if still alive) is killed, the promise rejects + with a typed cancellation error, and a follow-up + `handleAuthenticate` is accepted normally. + - Token exchange: mock token endpoint with the existing test + pattern; verify the request body contains **both** `code_verifier` + and `client_secret` (Amendment 2026-05-28 — PKCE is layered on top + of the secret, not a replacement). + - Refresh path regression: `refresh-manager.test.ts` continues to + assert the refresh request **includes** `client_secret`. No + rewrite — the existing assertions stay correct. + - `--redirect-port` validation: parser accepts `1..=65535`, + rejects `0` (the kernel-pick default is reached by *omitting* + the flag, not by `--redirect-port 0` — keeps the "stable port" + intent unambiguous), rejects non-numeric, rejects out-of-range + (`-1`, `65536`, `99999`), rejects values inside the privileged + range (`<1024`) with an error message naming the typical free + range to use over SSH. Bonus assertion: the validated port + flows verbatim into the URL the listener logs to stderr at + bind-time (per the Logging policy item) and into + `redirect_uri`. + - DNS-rebinding `Host` check: send a request with `Host: + evil.example` to the bound listener and assert a 400 with no + listener teardown and no `state` validation having run + (rejection must precede state parsing so the response is + timing-uniform with respect to flow state). + - `authenticate_clear` revocation, four cases: + 1. **Refresh token present + revoke succeeds.** Mock the + discovery doc to point `revocation_endpoint` at a stub that + returns 200. Pre-seed the keyring with a refresh token. Call + `handleClear`. Assert: revoke POST hit the stub with + `token=` + `token_type_hint=refresh_token` in a + `application/x-www-form-urlencoded` body, *no* `client_id` + or `client_secret` on the wire, keyring entry is gone after + the call, response text matches the "cleared and revoked" + string. + 2. **Refresh token present + revoke fails (network/5xx).** Stub + returns 500 or rejects the connection. Assert: keyring entry + is still gone (best-effort revoke does not block local + delete), response text matches the "cleared locally, + revocation failed" string with a redacted short reason, no + refresh token leaks into the message. + 3. **No refresh token in keyring.** Empty keyring, call + `handleClear`. Assert: revoke endpoint is *not* hit (no POST + fired), response is the existing idempotent "nothing to + clear" shape, no error. + 4. **Local delete fails.** Force the keyring delete to throw. + Assert: response uses the existing failure path (line 282) + and the message names whether the revoke ran, so the user + knows the Google-side state. +- [ ] Manual end-to-end: + - Real Google "Desktop app" client (test-tier project, not the + canonical-hub client) + - Real browser + - Verify token lands in keyring with same schema as before + - Verify hub accepts the token (add test `client_id` to a dev + hub's audience allowlist) + - Verify the refresh path works after the initial sign-in (force + an ID-token expiry and confirm refresh succeeds using + `client_secret` via `ClientSecretPost`, unchanged from today). + - Document the invocation and observed output in this plan per + the end-to-end-verification policy in `CLAUDE.md` + +## Phase 2 — canonical Quarto Hub client + documentation + +- [ ] Quarto team registers the canonical "Quarto Hub MCP" Desktop-app + OAuth client at Google: + - Verified-publisher consent screen + - App name: "Quarto Hub MCP" (or similar) + - Icon + support URL pointing at canonical Quarto documentation +- [ ] Add the canonical `client_id` to the canonical hub's audience + allowlist (`--additional-audiences`). +- [ ] Document the canonical `client_id` **and** `client_secret` in + the canonical hub's end-user onboarding (quarto.org / handbook / + wherever the hub WebSocket URL is published). Per Amendment + 2026-05-28 the Desktop-app `client_secret` is required; + Google's own docs note it is "not treated as a secret" for + installed apps, but it must still be distributed and set in the + env — keep the existing secret-handling guidance. +- [x] README updates: + - **Keep** the `QUARTO_HUB_MCP_CLIENT_SECRET` row in Setup + (Amendment 2026-05-28). + - Replace the device-flow walkthrough with the loopback + walkthrough: agent calls `authenticate`, browser opens, user + signs in, returns to terminal with `"Authenticated as X."` + - Update credential-storage section (still keyring, same schema) + - Keep "Why both env vars must come from the operator." Update only + the client *type* (Desktop app, not TV/Limited-Input) and the + auth *flow* (loopback, not device flow); the audience-allowlist + and consent-screen-ownership rationale, and the two-value + distribution, are unchanged. + - Update the `authenticate_clear` description in the tools + reference to match the new contract: it now performs a + best-effort revoke at Google before the local delete, with the + manual `myaccount.google.com` step framed as the fallback when + revocation fails (network down, token already invalid) rather + than the only Google-side step the user is responsible for. +- [x] Operator-runbook updates + (`claude-notes/instructions/hub-mcp-operator-runbook.md`): + - Replace "TV and Limited Input devices" client-registration + steps with "Desktop app" steps + - **Keep** the secret-distribution section (Amendment 2026-05-28); + update it to reflect the Desktop-app client and the + "publish both `client_id` and `client_secret` to end users" + framing. Google's installed-app secret is low-sensitivity but + still distributed; the section stays. + - Note: the canonical-hub operator follows the same steps as + private operators (no bundled-default special case for v1) +- [x] Migration for existing users: their refresh tokens were issued + against the device-flow client. After upgrading and switching + `QUARTO_HUB_MCP_CLIENT_ID` (and `QUARTO_HUB_MCP_CLIENT_SECRET`) + to the new Desktop-app values, the credential + store keys by `(issuer, clientId)`, so the old entry is simply + invisible to the new lookup — `getValidIdToken` throws + `ReauthRequired` on first call and the normal sign-in prompt + fires. No detection code, no migration warning; the old + keyring entry becomes unreachable garbage that can sit until + the user runs the documented keyring-clear command for + housekeeping. Document this in the README's upgrade section + ("on first run after upgrading, you'll be prompted to sign in + once; the old credentials are stranded under the previous + `client_id` and can be cleared at leisure") and move on. + +## Phase 3 — device-flow removal + +- [x] Delete device-flow code paths: + - `auth/device.ts` (or equivalent) + - `authenticate_start` / `authenticate_finish` tool registrations + - Device-flow-specific tests +- [x] Delete or archive the device-flow section of the operator + runbook. Done by rewriting the runbook in place — the + device-flow / "TV and Limited Input devices" content is replaced + with the Desktop-app + loopback flow (no separate archived + appendix; the device-flow plan remains the historical record). +- [ ] After a migration grace period (typically one release): + - Decommission the device-flow Google OAuth client (canonical + hub) or document the decommissioning step for private operators + - Remove the device-flow `client_id` from the hub's + `--additional-audiences` once migration is confirmed complete +- [x] Headless story documentation: + - README: explicit section covering the two options for headless + users — + (a) SSH port-forward — pick a free port `N`, start + `quarto-hub-mcp --redirect-port N` on the remote host, run + `ssh -L N:127.0.0.1:N ` from the local machine, call + the `authenticate` MCP tool; the manual-paste URL surfaces in + the response and resolves to the local browser via the tunnel. + (b) SPA cookie path from a graphical session on the same hub. + - Operator runbook: matching coverage from the operator side. + +## Phase 4 — threat-model documentation + +- [x] New threat-model section in this plan (or update of the + existing device-flow plan's threat-model) covering the loopback + analysis. The load-bearing argument: + + **Device flow uniquely enables no-malware remote phishing.** + The user-facing leg ("enter `WXYZ-ABCD` at google.com/device, + sign in, approve consent") is decoupled from the device + process. An attacker on the other side of the internet, holding + only the `client_id` (and historically the `client_secret`), + can: + 1. Initiate a device flow from their own server. + 2. Email the victim with a plausible cover story asking them to + enter the `user_code`. + 3. Victim signs in to Google with their own credentials and + approves a genuine, verified-publisher consent screen + showing the legitimate app name. There is nothing on the + consent screen identifying the *initiator* of the flow. + 4. Attacker polls `/token` and receives tokens minted under the + victim's identity. Hub accepts them — correct audience, real + Google signature, real `sub`. + + The trust model assumed by RFC 8628 ("the user trusts the + device they are standing in front of") collapses when the + device is an attacker's server and the only binding is an + eight-character code. This attack class is well documented in + the wild against Microsoft 365 device-flow clients (Storm-2372 + and related campaigns). + + **Loopback structurally cannot be phished the same way.** The + user-facing leg embeds `redirect_uri=http://127.0.0.1:`. + The redirect after consent lands on the victim's own loopback + interface, unreachable from any remote network. An attacker + who only has the `client_id` cannot construct a flow that + delivers tokens to themselves without first achieving code + execution on the victim's machine — at which point the OAuth + design is moot (keyring is already accessible). The remote + no-malware attack mode is closed. + + **Other deltas vs device flow:** + - **Unchanged:** secret-in-distribution-channel threat. Per + Amendment 2026-05-28 the `client_secret` is retained + (Google's Desktop-app client requires it), so the operator + still distributes it and the same channel-exposure + consideration as device flow applies. This is *not* a benefit + of the switch; the switch is justified by the + no-malware-remote-phishing closure below. Note Google + documents the installed-app secret as not truly confidential, + so its exposure is low-severity — but it is not eliminated. + - **Reduced:** auth-code interception. PKCE binds the code to + the originating process's `code_verifier`; even if the code + leaked (browser history, referer, local log scraping), it is + useless without the verifier in process memory. + - **Strengthened:** CSRF defence. The `state` parameter binds + the callback to the originating flow; mismatched `state` + rejects. + - **Unchanged:** stolen ID/refresh tokens authenticate to the + 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. + - **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 + tokens via their own local listener. Same residual as bundled + defaults; documented as accepted per RFC 8252. Dynamic client + registration (RFC 7591) is the deeper fix; out of scope. + + **Loopback-specific threat: DNS-rebinding via the user's + browser.** Binding to the IP literal `127.0.0.1` closes + *outbound* rebinding (we never resolve `localhost`), but the + listener is still reachable from any page open in the user's + browser during the auth window — including via a hostname that + a malicious DNS server has rebound to `127.0.0.1` after its + initial resolution. Without a defence, an attacker page could + forge a `/callback?code=…&state=…` request to our port. The + Phase 1 listener mitigates this by rejecting any request whose + `Host` header is not exactly `127.0.0.1:` (RFC 8252 §8.4), + with the rejection happening *before* `state` validation so + timing cannot leak whether a flow is in progress. Residual: + attacker still needs to learn the listener port — a non-issue + for `port=0` (entropy from kernel choice) but worth flagging + when `--redirect-port` is set to a stable value (the port name + itself is operationally public, e.g. for SSH-tunnel users). + The `Host`-check, the CSRF `state`, and PKCE all have to fail + simultaneously for token capture; that's acceptable defence in + depth for the residual. + +## Open questions + +- **Bundled `open` package vs in-tree browser launcher?** Default to + in-tree (~15 lines, no new dep). Revisit if a corner case argues + otherwise. + +Resolved (recorded for posterity): + +- **`prompt=consent` on every flow?** **Default on**, until Spike A + measures that a returning user receives a non-empty + `refresh_token` without it. This is a correctness gate (refresh + path silently breaks without a refresh_token on the second + authorization), not a UX preference. See Phase 1 Authorization + URL construction + Spike A's "second-run refresh-token return" + item. +- **`--redirect-port ` flag for SSH-tunnel scenarios?** Yes — + committed as a Phase 1 implementation item. The headless-via-SSH + story documented in Phase 3 leans on it; deferring would leave + that doc unactionable. Trivial to implement (one flag, one + validator, one thread-through). +- **PKCE: roll our own vs. `oauth4webapi`?** Use `oauth4webapi`; + it's already a project dep and used by every other auth path. + See Phase 1. +- **What happens to `authenticate_clear`?** Kept on the MCP + surface — flow-independent, documented recovery action. Gains + best-effort Google-side refresh-token revocation: the public + contract widens from "delete the local copy" to "render the + credential unusable, locally and at Google." Revocation failure + does not block local cleanup. See Phase 1 + "`authenticate_clear` revocation" item for the full + read-→-revoke-→-delete order, endpoint discovery, response-text + contract, and the four test cases. +- **Concurrent `authenticate` calls?** Not guarded against. MCP + stdio hosts serialise `tools/call` requests (each + `CallToolResult` is awaited before the next call leaves the + host), so two concurrent invocations cannot arise under a + well-behaved host. The cost of a single-flight guard + (`inflight` slot, detached-cleanup pattern, two extra tests) + outweighs the benefit for an event that shouldn't happen, and + the misbehaving-host failure mode is a UX defect (two listeners + on different ports) not a security one — PKCE and `state` bind + each flow's tokens to its own callback. Same logic retires the + `authenticate_clear`-while-`authenticate`-in-flight question: + under host serialisation it can't happen, and we don't carry + bookkeeping for it. See Phase 1 single-`authenticate` tool + "Concurrency — none" sub-item. +- **MCP-host-issued cancellation mid-flow?** Independent of the + concurrency decision — a single in-flight `authenticate` can + still be cancelled by the host. Observed via the `AbortSignal` + from the MCP SDK's `CallToolRequest` handler context; listener + and browser-opener subprocess tear down on abort, and the next + `handleAuthenticate` call is accepted normally. See Phase 1 + single-`authenticate` tool "MCP-host cancellation mid-flow" + sub-item. +- **Headless-Linux silent `xdg-open` exit-0 lock-out?** Mitigated + by surfacing the authorization URL via *both* an MCP + `notifications/progress` against the `tools/call` progress token + (when the caller supplied one) *and* stderr at INFO level, both + fired at listener-bind time before invoking the browser opener + and regardless of its outcome. MCP progress is the host-UI + primary; stderr is the universal fallback. See Phase 1 Browser + opener item for the full construction (including the + no-progress-token-no-op rule and the test additions). + +## Verification log + +Spike outcomes (Phase 0) and end-to-end checks (Phase 1, manual +end-to-end item) land here. Per Amendment 2026-05-28, spikes no longer +gate the Phase 1 PR; fill these in as the spikes are run (Spike B's +host-deadline result and the manual end-to-end check should still +accompany a merged Phase 1). Each entry: date, +operator, command/invocation, observed result, pass/fail/partial +verdict, link to any captured artefacts (screenshots, raw token +responses with secrets redacted). + +### Spike A — Desktop-app client + loopback + PKCE against real hub + +- **Date:** +- **Operator:** +- **Test-tier Google project / `client_id`:** +- **Hub used (with `--additional-audiences` including the test + `client_id`):** +- **Round-trip outcome (`aud` claim matches `client_id`? hub + accepts the bearer on WebSocket connect?):** +- **Refresh-token behaviour (rotated / not rotated, response body + shape vs the Limited-Input-Devices client):** +- **Second-run refresh-token return (same account, no + `prompt=consent` on the second auth request — was a + `refresh_token` returned?):** ☐ yes (safe to drop default) ☐ no + (default `prompt=consent` stays on) +- **Refresh construct (confirm `oauth.ClientSecretPost` still applies + to the Desktop-app client; secret retained per Amendment + 2026-05-28 — no `oauth.None()` public-client construct):** +- **Verdict:** ☐ pass ☐ fail (→ revisit; the device-flow path + remains available as the fallback while the issue is diagnosed) + +### Spike B — MCP host tool-call lifetime + +| Host | Floor observed (s) | Notes | +| --------------------------- | ------------------ | ----- | +| Claude Code (vX.Y.Z) | | | +| Cursor (vX.Y.Z) | | | +| in-tree `mcp-test-client.ts`| | | + +- **Verdict:** ☐ all hosts ≥300 s (5-min deadline as planned) + ☐ partial (lowest floor = ___ s, deadline set to ___ s) + ☐ any host < 60 s (→ two-tool fallback per Phase 0) +- **Chosen `authenticate` deadline constant value:** ___ s + +### Phase 1 implementation verification (2026-05-28) + +**Implemented in this session** (code + tests; `npm run build` and +`npm test` both green — 141 passed / 2 pre-existing skips across 11 +files, including new `pkce`, `redact`, `browser`, `oauth-config`, +`loopback`, `index` suites and the rewritten `auth-tools` suite): + +CLI end-to-end checks against the **real built binary** (`node +dist/index.js`), inspected output: + +- `--help` lists `--redirect-port ` with the SSH-tunnel guidance. +- `--redirect-port 80` → exits 1 with + `--redirect-port must be a non-privileged port (1024-65535); for SSH + tunnels pick one in the ephemeral range (e.g. 49152-65535). Got 80.` +- `--redirect-port abc` → exits 1 with `must be an integer`. +- No-auth env, `tools/list` → `connect_project, list_files, read_file, + write_file, patch_file, create_file, delete_file, rename_file, + create_project` (no auth tools, as expected). +- With `QUARTO_HUB_MCP_CLIENT_ID` + `QUARTO_HUB_MCP_CLIENT_SECRET` set + (dummy values; live Google discovery succeeded), `tools/list` → + `authenticate, authenticate_clear, …` — the device-flow tool pair is + gone and the single `authenticate` tool plus `authenticate_clear` + are registered. + +**NOT run in this session** (require external resources, left +unchecked above): + +- Spike A / Spike B (real test-tier Google project; live MCP-host + deadline sweep). +- Manual end-to-end with a real Google "Desktop app" client + real + browser + dev hub allowlist (keyring round-trip, hub WebSocket + accept, forced-expiry refresh). The token-exchange and revocation + wire contracts are covered by unit tests against stubbed endpoints + (`code_verifier` + `client_secret` on the exchange; `token` + + `token_type_hint` with no client auth on the revoke), but the live + Google round-trip is unverified here. + +### Phase 1 manual end-to-end check (per `CLAUDE.md` policy) — PENDING + +- **Date:** +- **Invocation:** +- **Observed output (snippet):** +- **Keyring entry verified present, schema matches pre-change:** ☐ +- **Hub WebSocket connect with new token succeeded:** ☐ +- **Forced ID-token expiry → refresh succeeds using `client_secret` + (via `ClientSecretPost`, unchanged):** ☐ + +## Future work (out of scope) + +- **Bundled `client_id` defaults** for the canonical Quarto Hub. + Removes the last per-user config step for canonical-hub users. + Deferred to gather operational signal on end-user friction first. + When revisited: extend `src/config.ts` to export a + `DEFAULT_CLIENT_ID`, used when `QUARTO_HUB_MCP_CLIENT_ID` is unset. +- **Dynamic client registration (RFC 7591)** so each hub-mcp install + registers its own `client_id` and there is no shared brand to + phish under. Requires IdP support; Google does not currently offer + 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. diff --git a/ts-packages/quarto-hub-mcp/README.md b/ts-packages/quarto-hub-mcp/README.md index efde94d7..05bbe6a8 100644 --- a/ts-packages/quarto-hub-mcp/README.md +++ b/ts-packages/quarto-hub-mcp/README.md @@ -3,14 +3,16 @@ MCP server that lets an AI agent (Claude Code, Claude Desktop, Cursor, Continue, …) read and write Quarto Hub projects through Automerge. -Authentication uses Google's OAuth 2.0 device-authorization grant -(RFC 8628). The agent calls one MCP tool to start the flow, you -approve in a browser, and the agent calls a second tool to finish. -The resulting credentials are persisted in your OS keyring; the agent -can use them indefinitely until you revoke them. +Authentication uses Google's OAuth 2.0 Authorization Code grant with +PKCE and a loopback redirect (RFC 8252) — the same pattern as `gcloud +auth login`, `gh auth login`, and `aws sso login`. The agent calls a +single `authenticate` MCP tool, your browser opens to Google's sign-in +page, and the redirect lands back on a short-lived `127.0.0.1` listener +the tool started. The resulting credentials are persisted in your OS +keyring; the agent can use them indefinitely until you revoke them. The design lives in -[`claude-notes/plans/2026-05-05-hub-mcp-device-flow-implementation.md`](../../claude-notes/plans/2026-05-05-hub-mcp-device-flow-implementation.md); +[`claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md`](../../claude-notes/plans/2026-05-28-hub-mcp-loopback-pkce.md); operators registering a hub for end-user use should consult [`claude-notes/instructions/hub-mcp-operator-runbook.md`](../../claude-notes/instructions/hub-mcp-operator-runbook.md). @@ -38,7 +40,7 @@ Add hub-mcp to your MCP client config. For Claude Code the file is "args": [ "@quarto/hub-mcp", "--server", - "wss://hub.example.com/ws" + "wss://quarto-hub.com/ws" ], "env": { "QUARTO_HUB_MCP_CLIENT_ID": ".apps.googleusercontent.com", @@ -57,23 +59,37 @@ Add hub-mcp to your MCP client config. For Claude Code the file is Restart the MCP client. The first agent action against the hub triggers the auth flow: -1. Agent calls the `authenticate_start` MCP tool. -2. Tool response shows `https://www.google.com/device` and a short - `user_code`. Google's response carries its own `verification_uri` - that is also shown — both URLs are valid; the canonical - `https://www.google.com/device` is hard-coded as a - phishing-resistance check. -3. You open the URL, type the code, approve consent in your browser. -4. Agent calls `authenticate_finish`. On success the response is - `"Authenticated as ."` and the agent retries the - original action. +1. Agent calls the `authenticate` MCP tool. The tool binds a local + `127.0.0.1` listener and opens your browser to Google's sign-in + page. The authorization URL is also printed (to the tool's progress + output and to stderr) so you can open it manually if the browser + doesn't launch — useful on headless or SSH sessions. +2. You sign in and approve consent in your browser. The redirect lands + on the local listener, which closes immediately after. +3. The tool exchanges the authorization code (bound to a PKCE verifier + held only in this process) and stores the credentials. On success + the response is `"Authenticated as ."` and the agent + retries the original action. If the hub does **not** require authentication (operator-disabled -auth) the agent just talks to the hub directly — no device flow. +auth) the agent just talks to the hub directly — no sign-in flow. Asking the agent to authenticate against a known no-auth hub returns `"The configured hub does not require authentication; no action needed."`. +## Upgrading from the device-flow version + +Earlier hub-mcp used Google's device-authorization flow with a "TV and +Limited Input devices" OAuth client. The loopback+PKCE version uses a +**Desktop app** client, so your operator will issue new `client_id` / +`client_secret` values. After you switch them, the first agent action +prompts you to sign in once: the credential store keys entries by +`(issuer, client_id)`, so the old entry is simply invisible to the new +lookup and a fresh `authenticate` runs automatically. The stranded old +entry under the previous `client_id` is harmless and can be cleared at +your leisure with the platform command in the **Clearing the entry** +table (substituting the old `client_id`). + ## Credential storage The bundle (`id_token`, `refresh_token`, `id_token_expires_at`, @@ -114,27 +130,60 @@ no-auth hubs.) | Windows | `cmdkey /delete:dev.quarto.hub-mcp::` | Clearing the entry forces the next agent action to start a fresh -device flow. +sign-in. The `authenticate_clear` MCP tool does the same from inside +the agent, and additionally best-effort revokes the stored refresh +token at Google before deleting the local copy (see **Revoking +access** below). + +### Headless / SSH sessions + +The sign-in redirect lands on a `127.0.0.1` listener on the machine +running hub-mcp. On a headless or remote machine there is no local +browser to receive it, so use one of: + +- **SSH port-forward.** Pick a free non-privileged port `N`, start + hub-mcp with a fixed redirect port, and forward it from your + workstation: -### Headless Linux + ```bash + # on the remote host (via your MCP client's args) + quarto-hub-mcp --server wss://quarto-hub.com/ws --redirect-port N + # on your local workstation + ssh -L N:127.0.0.1:N + ``` + + Call `authenticate`; the printed authorization URL opens in your + local browser, and the redirect to `http://127.0.0.1:N/callback` + travels back through the tunnel. Omitting `--redirect-port` lets the + OS pick a port (logged to stderr on bind), which is fine for a local + desktop but awkward to forward. + +- **Hub SPA cookie path** from a graphical session on the same hub. + +### Headless Linux keyring Headless Linux machines without a running Secret Service / libsecret -(`gnome-keyring-daemon`, `kwallet5`, or equivalent) cannot run -hub-mcp. Either install one of those daemons or use the Hub SPA +(`gnome-keyring-daemon`, `kwallet5`, or equivalent) cannot persist +credentials. Either install one of those daemons or use the Hub SPA cookie path from a graphical session on the same hub. ## Revoking access -To revoke the agent's access entirely: +The quickest path is to ask the agent to call `authenticate_clear`. +It best-effort revokes the stored refresh token at Google's revocation +endpoint (which also invalidates any access tokens derived from it), +then deletes the local keyring entry. If the revoke fails (offline, or +the token was already invalid) the local delete still proceeds and the +response tells you to finish the job manually. + +To revoke manually, or to be sure when the revoke step failed: 1. Visit . 2. **Third-party apps with account access** → the hub-mcp client → **Remove Access**. The agent's next action surfaces `ReauthRequired` with a message -asking you to re-authenticate. Clear your local keyring entry too -(see the table above) if you don't intend to authenticate again -soon. +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 @@ -149,12 +198,22 @@ soon. Symmetric with the SPA: the operator owns the Google project, the consent screen, the quota, the audit trail, and the revocation key. The Quarto team does **not** ship a baked-in client_id or -client_secret — there is no shared default to compromise. - -Defence in depth: a leaked `client_secret` alone cannot redeem any -user's approval. Each device flow is bound to a `device_code` that -Google issues per-flow and that hub-mcp never persists; without a -fresh approval against that `device_code` the secret is inert. +client_secret — there is no shared default to compromise. The +operator registers a **Desktop app** OAuth client (Google requires the +`client_secret` on the token exchange even for installed-app clients, +so both values are distributed to end users). + +Defence in depth: the loopback redirect collapses the remote-attack +surface to a local one. After consent, tokens are delivered only to +`http://127.0.0.1:` on your own machine — unreachable from any +remote network — and the authorization code is bound by PKCE to a +`code_verifier` that never leaves this process. An attacker holding the +`client_id`/`client_secret` cannot mint tokens under your identity +without first achieving code execution on your machine (at which point +the keyring is already exposed and OAuth is moot). This is the key +improvement over the previous device-flow design, which an attacker +could phish remotely with only the client credentials and a plausible +cover story. ## Insecure transport (dev only) diff --git a/ts-packages/quarto-hub-mcp/src/auth/auth-tools.test.ts b/ts-packages/quarto-hub-mcp/src/auth/auth-tools.test.ts index 43c62a24..ede3a64a 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/auth-tools.test.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/auth-tools.test.ts @@ -1,21 +1,23 @@ /** - * Phase 7 — MCP auth-tool surface (`authenticate_start` / `authenticate_finish`). + * MCP auth-tool surface — Authorization Code + PKCE + loopback. * - * Tests drive `AuthToolsState` directly: no MCP `Server` instance is - * spun up. Time is injected via `deps.now`; HTTP via `deps.fetch`. The - * `CredentialStore` is wired to an in-memory keyring backend so writes - * are observable without touching the platform keyring. + * Tests drive `AuthToolsState` directly: no MCP `Server` is spun up. The + * loopback listener and the browser opener are injected seams; HTTP is + * injected via `deps.fetch`; the `CredentialStore` is wired to an + * in-memory keyring backend so writes are observable. */ +import { EventEmitter } from 'node:events'; import { describe, it, expect, vi } from 'vitest'; -import * as oauth from 'oauth4webapi'; +import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import type * as oauth from 'oauth4webapi'; import { AUTH_TOOL_DEFINITIONS, AuthToolsState, - CANONICAL_VERIFICATION_URL, - type AuthToolsDeps, + type AuthToolContext, type LastObservedAuthModeSource, + type ProgressNotification, } from './auth-tools.js'; import { CredentialStore, @@ -23,6 +25,14 @@ import { type CredentialStoreConfig, type KeyringBackend, } from './credential-store.js'; +import { + LoopbackAbortedError, + LoopbackTimeoutError, + type LoopbackListener, + type StartLoopbackOptions, + startLoopbackListener, +} from './loopback.js'; +import { openBrowser } from './browser.js'; import { RefreshManager } from './refresh-manager.js'; // --------------------------------------------------------------------------- @@ -36,8 +46,9 @@ const FAKE_EMAIL = 'tester@example.com'; const AS: oauth.AuthorizationServer = { issuer: ISSUER, - device_authorization_endpoint: 'https://oauth2.googleapis.com/device/code', + authorization_endpoint: 'https://accounts.google.com/o/oauth2/v2/auth', token_endpoint: 'https://oauth2.googleapis.com/token', + revocation_endpoint: 'https://oauth2.googleapis.com/revoke', }; const CFG: CredentialStoreConfig = { issuer: ISSUER, clientId: FAKE_CLIENT_ID }; @@ -48,13 +59,8 @@ function b64url(s: string): string { interface IdTokenClaims { readonly exp: number; - readonly iss?: string; - readonly aud?: string; - readonly azp?: string; - readonly sub?: string; readonly email?: string; - readonly email_verified?: boolean; - readonly iat?: number; + readonly [k: string]: unknown; } function fakeIdToken(claims: IdTokenClaims): string { @@ -80,22 +86,19 @@ function farFutureExp(): number { } // --------------------------------------------------------------------------- -// Backend / store helpers +// In-memory keyring backend // --------------------------------------------------------------------------- function memoryBackend(initial: string | null = null): { backend: KeyringBackend; state: { value: string | null }; - writes: number; } { - const counters = { writes: 0 }; const state = { value: initial }; const backend: KeyringBackend = { async read() { return state.value; }, async write(v: string) { - counters.writes += 1; state.value = v; }, async clear() { @@ -104,19 +107,13 @@ function memoryBackend(initial: string | null = null): { return existed; }, }; - return { - backend, - state, - get writes() { - return counters.writes; - }, - }; + return { backend, state }; } function makeBundle(overrides: Partial = {}): CredentialBundle { const exp = farFutureExp(); return { - idToken: fakeIdToken({ exp }), + idToken: fakeIdToken({ exp, email: FAKE_EMAIL }), refreshToken: '1//original-refresh-token', idTokenExpiresAt: new Date(exp * 1000), scopes: ['openid', 'email', 'profile'], @@ -125,13 +122,14 @@ function makeBundle(overrides: Partial = {}): CredentialBundle } // --------------------------------------------------------------------------- -// Fetch stub +// fetch recorder // --------------------------------------------------------------------------- interface RecordedRequest { url: string; method: string; body: URLSearchParams; + headers: Record; } function makeFetch( @@ -148,7 +146,13 @@ function makeFetch( } else { body = new URLSearchParams(); } - requests.push({ url, method: init?.method ?? 'GET', body }); + const headers: Record = {}; + if (init?.headers) { + for (const [k, v] of Object.entries(init.headers as Record)) { + headers[k.toLowerCase()] = v; + } + } + requests.push({ url, method: init?.method ?? 'GET', body, headers }); return responder(requests[requests.length - 1]!); }; return { fetch: stub, requests }; @@ -161,681 +165,635 @@ function jsonResponse(status: number, body: unknown): Response { }); } -function deviceAuthBody(overrides: Partial = {}): Record { - return { - device_code: 'AH-1Ng-test', - user_code: 'FJZL-WTDR', - verification_uri: 'https://www.google.com/device', - expires_in: 1800, - interval: 5, - ...overrides, - }; -} - -function tokenSuccessBody( - overrides: { id_token?: string; refresh_token?: string; access_token?: string } = {}, +function tokenResponseBody( + overrides: { id_token?: string; refresh_token?: string | undefined } = {}, ): Record { - return { - access_token: overrides.access_token ?? 'ya29.fake-access-token', + const body: Record = { + access_token: 'ya29.fresh-access', expires_in: 3599, - refresh_token: overrides.refresh_token ?? '1//fake-refresh-token', - scope: 'openid email profile', token_type: 'Bearer', - id_token: - overrides.id_token ?? - fakeIdToken({ - exp: farFutureExp(), - sub: 'fake-sub', - email: FAKE_EMAIL, - }), + scope: 'openid email profile', + id_token: overrides.id_token ?? fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }), + refresh_token: '1//new-refresh-token', }; + if ('refresh_token' in overrides) { + if (overrides.refresh_token === undefined) delete body.refresh_token; + else body.refresh_token = overrides.refresh_token; + } + return body; } // --------------------------------------------------------------------------- -// Connection-manager stub +// Injected seams: loopback listener + browser opener // --------------------------------------------------------------------------- -function stubConnMgr(mode: 'no-auth' | 'requires-auth' | 'unknown'): LastObservedAuthModeSource { - return { lastObservedAuthMode: () => mode }; +interface FakeListenerControl { + start: typeof startLoopbackListener; + recorded: { expectedState?: string; redirectUri?: string; calls: number; signal?: AbortSignal }; +} + +function fakeListener(opts: { + code?: string; + rejectWith?: Error; + port?: number; +}): FakeListenerControl { + const recorded: FakeListenerControl['recorded'] = { calls: 0 }; + const start: typeof startLoopbackListener = async (o: StartLoopbackOptions) => { + recorded.calls += 1; + recorded.expectedState = o.expectedState; + recorded.signal = o.signal; + const port = opts.port ?? 51234; + const redirectUri = `http://127.0.0.1:${port}/callback`; + recorded.redirectUri = redirectUri; + const code = opts.code ?? 'auth-code-xyz'; + const result: Promise<{ code: string; state: string; params: URLSearchParams }> = + opts.rejectWith + ? Promise.reject(opts.rejectWith) + : Promise.resolve({ + code, + state: o.expectedState, + params: new URLSearchParams({ code, state: o.expectedState }), + }); + // Keep an unobserved rejection from surfacing before the handler awaits. + result.catch(() => undefined); + const listener: LoopbackListener = { port, redirectUri, result, close: () => undefined }; + return listener; + }; + return { start, recorded }; +} + +interface FakeBrowserControl { + open: typeof openBrowser; + calls: string[]; +} + +function fakeBrowser(opts: { fail?: boolean } = {}): FakeBrowserControl { + const calls: string[] = []; + const open: typeof openBrowser = (url: string) => { + calls.push(url); + if (opts.fail) return undefined; + return new EventEmitter() as unknown as ReturnType; + }; + return { open, calls }; } // --------------------------------------------------------------------------- -// Deps assembly +// State construction // --------------------------------------------------------------------------- -interface Harness { - state: AuthToolsState; - store: CredentialStore; - storeState: { value: string | null }; - storeWriteCount: () => number; - requests: RecordedRequest[]; - now: () => Date; - setNow: (d: Date) => void; +function authMode(mode: 'no-auth' | 'requires-auth' | 'unknown'): LastObservedAuthModeSource { + return { lastObservedAuthMode: () => mode }; } -interface HarnessOpts { - readonly seedBundle?: CredentialBundle; - readonly responder: (req: RecordedRequest) => Response | Promise; - readonly authMode?: 'no-auth' | 'requires-auth' | 'unknown'; - readonly initialNow?: Date; - readonly coalesceWindowMs?: number; +interface MakeStateArgs { + store: CredentialStore; + mode?: 'no-auth' | 'requires-auth' | 'unknown'; + fetch?: typeof fetch; + startListener?: typeof startLoopbackListener; + openBrowser?: typeof openBrowser; + promptConsent?: boolean; + logger?: (m: string) => void; + authServer?: () => Promise; } -async function makeHarness(opts: HarnessOpts): Promise { - const mb = memoryBackend(null); - const store = new CredentialStore(CFG, mb.backend); - if (opts.seedBundle) { - await store.write(opts.seedBundle); - } - const { fetch, requests } = makeFetch(opts.responder); - let nowRef = opts.initialNow ?? new Date(); - const now = (): Date => new Date(nowRef.getTime()); +function makeState(args: MakeStateArgs): AuthToolsState { const refreshManager = new RefreshManager({ - as: AS, + authServer: async () => AS, config: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - store, - fetch, - }); - const deps: AuthToolsDeps = { - credentialStore: store, + store: args.store, + // Never used in the loopback path (store.read() short-circuits first). + fetch: (async () => { + throw new Error('RefreshManager fetch should not be called in these tests'); + }) as unknown as typeof fetch, + }); + return new AuthToolsState({ + credentialStore: args.store, refreshManager, - connectionManager: stubConnMgr(opts.authMode ?? 'requires-auth'), - flowConfig: { - clientId: FAKE_CLIENT_ID, - clientSecret: FAKE_CLIENT_SECRET, - issuer: ISSUER, - }, - authorizationServer: AS, - now, - fetch, - coalesceWindowMs: opts.coalesceWindowMs, - }; - const state = new AuthToolsState(deps); - return { - state, - store, - storeState: mb.state, - storeWriteCount: () => mb.writes, - requests, - now, - setNow: (d: Date) => { - nowRef = new Date(d.getTime()); - }, - }; + connectionManager: authMode(args.mode ?? 'requires-auth'), + flowConfig: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, issuer: ISSUER }, + authServer: args.authServer ?? (async () => AS), + fetch: args.fetch, + startListener: args.startListener, + openBrowser: args.openBrowser, + promptConsent: args.promptConsent, + logger: args.logger ?? (() => undefined), + }); } -function textOf(result: { content: ReadonlyArray<{ readonly type: string; readonly text?: string }> }): string { - return result.content.map((c) => c.text ?? '').join(''); +function emptyStore(): { store: CredentialStore; state: { value: string | null } } { + const { backend, state } = memoryBackend(null); + return { store: new CredentialStore(CFG, backend), state }; } -// --------------------------------------------------------------------------- +async function seededStore( + bundle: CredentialBundle = makeBundle(), +): Promise<{ store: CredentialStore; state: { value: string | null } }> { + const { backend, state } = memoryBackend(null); + const store = new CredentialStore(CFG, backend); + await store.write(bundle); + return { store, state }; +} + +function textOf(res: CallToolResult): string { + return res.content + .filter((c): c is { type: 'text'; text: string } => c.type === 'text') + .map((c) => c.text) + .join('\n'); +} + +// =========================================================================== // Tool definitions -// --------------------------------------------------------------------------- +// =========================================================================== describe('AUTH_TOOL_DEFINITIONS', () => { - it('exposes authenticate_start, authenticate_finish, and authenticate_clear', () => { + it('exposes exactly authenticate and authenticate_clear', () => { const names = AUTH_TOOL_DEFINITIONS.map((t) => t.name).sort(); - expect(names).toEqual([ - 'authenticate_clear', - 'authenticate_finish', - 'authenticate_start', - ]); + expect(names).toEqual(['authenticate', 'authenticate_clear']); }); - it('marks authenticate_start and authenticate_finish as non-idempotent and non-destructive', () => { - for (const t of AUTH_TOOL_DEFINITIONS) { - if (t.name === 'authenticate_clear') continue; - expect(t.annotations).toEqual({ - readOnlyHint: false, - destructiveHint: false, - idempotentHint: false, - }); - } + it('marks authenticate non-idempotent / non-destructive and clear destructive / idempotent', () => { + const auth = AUTH_TOOL_DEFINITIONS.find((t) => t.name === 'authenticate')!; + const clear = AUTH_TOOL_DEFINITIONS.find((t) => t.name === 'authenticate_clear')!; + expect(auth.annotations?.idempotentHint).toBe(false); + expect(auth.annotations?.destructiveHint).toBe(false); + expect(clear.annotations?.destructiveHint).toBe(true); + expect(clear.annotations?.idempotentHint).toBe(true); }); - it('marks authenticate_clear as destructive and idempotent', () => { - const t = AUTH_TOOL_DEFINITIONS.find((x) => x.name === 'authenticate_clear'); - expect(t).toBeDefined(); - expect(t!.annotations).toEqual({ - readOnlyHint: false, - destructiveHint: true, - idempotentHint: true, - }); + it('clear description names the best-effort revoke and drops the old disclaimer', () => { + const clear = AUTH_TOOL_DEFINITIONS.find((t) => t.name === 'authenticate_clear')!; + expect(clear.description).toMatch(/revoke/i); + expect(clear.description).toContain('myaccount.google.com'); + expect(clear.description).not.toMatch(/Does not touch Google-side grants/i); }); }); -// --------------------------------------------------------------------------- -// authenticate_start -// --------------------------------------------------------------------------- - -describe('authenticate_start', () => { - it('returns verification_uri, user_code and canonical_url', async () => { - const h = await makeHarness({ - responder: () => jsonResponse(200, deviceAuthBody()), +// =========================================================================== +// authenticate — happy path + token-exchange contract +// =========================================================================== + +describe('authenticate', () => { + it('runs the loopback flow, exchanges the code, and stores the credentials', async () => { + const { store, state } = emptyStore(); + const listener = fakeListener({ code: 'the-code' }); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const auth = makeState({ + store, + fetch, + startListener: listener.start, + openBrowser: browser.open, }); - const res = await h.state.handleStart(); - const txt = textOf(res); - expect(txt).toContain(CANONICAL_VERIFICATION_URL); - expect(txt).toContain('https://www.google.com/device'); // verification_uri - expect(txt).toContain('FJZL-WTDR'); // user_code - }); - it('includes the expires-in seconds in the response', async () => { - const h = await makeHarness({ - responder: () => jsonResponse(200, deviceAuthBody({ expires_in: 1234 })), + const res = await auth.handleAuthenticate({}); + expect(textOf(res)).toBe(`Authenticated as ${FAKE_EMAIL}.`); + expect(res.isError).toBeUndefined(); + + const stored = await store.read(); + expect(stored?.refreshToken).toBe('1//new-refresh-token'); + expect(state.value).not.toBeNull(); + }); + + it('sends both code_verifier and client_secret on the token exchange', async () => { + const { store } = emptyStore(); + const listener = fakeListener({ code: 'the-code' }); + const browser = fakeBrowser(); + const { fetch, requests } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); + + await auth.handleAuthenticate({}); + + const tokenReq = requests.find((r) => r.url === AS.token_endpoint)!; + expect(tokenReq.body.get('grant_type')).toBe('authorization_code'); + expect(tokenReq.body.get('code')).toBe('the-code'); + expect(tokenReq.body.get('code_verifier')).toBeTruthy(); + expect(tokenReq.body.get('client_secret')).toBe(FAKE_CLIENT_SECRET); + expect(tokenReq.body.get('redirect_uri')).toBe(listener.recorded.redirectUri); + }); + + it('builds an authorization URL with PKCE, offline access, and prompt=consent', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); + + await auth.handleAuthenticate({}); + + expect(browser.calls).toHaveLength(1); + const url = new URL(browser.calls[0]!); + expect(url.origin + url.pathname).toBe('https://accounts.google.com/o/oauth2/v2/auth'); + expect(url.searchParams.get('response_type')).toBe('code'); + expect(url.searchParams.get('client_id')).toBe(FAKE_CLIENT_ID); + expect(url.searchParams.get('redirect_uri')).toBe(listener.recorded.redirectUri); + expect(url.searchParams.get('scope')).toBe('openid email profile'); + expect(url.searchParams.get('code_challenge_method')).toBe('S256'); + expect(url.searchParams.get('code_challenge')).toBeTruthy(); + expect(url.searchParams.get('state')).toBe(listener.recorded.expectedState); + expect(url.searchParams.get('access_type')).toBe('offline'); + expect(url.searchParams.get('include_granted_scopes')).toBe('true'); + expect(url.searchParams.get('prompt')).toBe('consent'); + }); + + it('omits prompt=consent when promptConsent is false', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const auth = makeState({ + store, + fetch, + startListener: listener.start, + openBrowser: browser.open, + promptConsent: false, }); - const res = await h.state.handleStart(); - expect(textOf(res)).toContain('1234'); - }); - - it('canonical URL is a hard-coded constant, not from Google', async () => { - const h = await makeHarness({ - responder: () => - jsonResponse( - 200, - deviceAuthBody({ verification_uri: 'https://malicious.example.com/oauth' }), - ), + await auth.handleAuthenticate({}); + const url = new URL(browser.calls[0]!); + expect(url.searchParams.get('prompt')).toBeNull(); + }); + + it('appends a manual-sign-in note when the browser launch fails', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser({ fail: true }); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); + + const res = await auth.handleAuthenticate({}); + expect(textOf(res)).toContain('Browser launch failed; you signed in manually.'); + }); + + // ------------------------------------------------------------------------- + // Pre-flight short-circuits + // ------------------------------------------------------------------------- + + it('short-circuits when already authenticated, without binding a listener', async () => { + const { store } = await seededStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const auth = makeState({ store, startListener: listener.start, openBrowser: browser.open }); + + const res = await auth.handleAuthenticate({}); + expect(textOf(res)).toBe(`Already authenticated as ${FAKE_EMAIL}. No action needed.`); + expect(listener.recorded.calls).toBe(0); + expect(browser.calls).toHaveLength(0); + }); + + it('does not resolve the authorization server when already authenticated', async () => { + // Lazy discovery: the already-authenticated short-circuit must not + // trigger the OIDC discovery network call. + const { store } = await seededStore(); + const authServer = vi.fn(async () => AS); + const auth = makeState({ store, authServer }); + + await auth.handleAuthenticate({}); + expect(authServer).not.toHaveBeenCalled(); + }); + + it('short-circuits when the hub does not require auth', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const auth = makeState({ + store, + mode: 'no-auth', + startListener: listener.start, + openBrowser: browser.open, }); - const res = await h.state.handleStart(); - const txt = textOf(res); - // The canonical URL must be the constant. - expect(txt).toContain(CANONICAL_VERIFICATION_URL); - // The attacker-controlled verification_uri must NOT appear as the - // canonical step-1 URL; it can still be listed as Google's value. - expect(CANONICAL_VERIFICATION_URL).toBe('https://www.google.com/device'); - }); - it('does not write to the credential store (device_code is process-local)', async () => { - const h = await makeHarness({ - responder: () => jsonResponse(200, deviceAuthBody()), - }); - await h.state.handleStart(); - expect(h.storeWriteCount()).toBe(0); - }); - - it('short-circuits when already authenticated', async () => { - const seeded = makeBundle(); - const h = await makeHarness({ - seedBundle: seeded, - responder: () => { - throw new Error('should not be called — already authenticated'); - }, - }); - const res = await h.state.handleStart(); - expect(textOf(res)).toContain('Already authenticated'); - expect(textOf(res)).toContain(FAKE_EMAIL); - expect(h.requests).toHaveLength(0); + const res = await auth.handleAuthenticate({}); + expect(textOf(res)).toMatch(/does not require authentication/); + expect(listener.recorded.calls).toBe(0); + }); + + // ------------------------------------------------------------------------- + // Progress notifications + // ------------------------------------------------------------------------- + + it('sends exactly one bind-time progress notification when a token is supplied', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const sendNotification = vi.fn((_n: ProgressNotification) => Promise.resolve()); + const ctx: AuthToolContext = { progressToken: 'p1', sendNotification }; + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); + + await auth.handleAuthenticate(ctx); + + expect(sendNotification).toHaveBeenCalledTimes(1); + const note = sendNotification.mock.calls[0]![0]; + expect(note.method).toBe('notifications/progress'); + expect(note.params.progressToken).toBe('p1'); + expect(note.params.progress).toBe(0); + expect(note.params.total).toBe(1); + expect(note.params.message).toContain('https://accounts.google.com/o/oauth2/v2/auth'); + }); + + it('sends no progress notification when no token is supplied', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const sendNotification = vi.fn((_n: ProgressNotification) => Promise.resolve()); + const ctx: AuthToolContext = { sendNotification }; + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); + + const res = await auth.handleAuthenticate(ctx); + expect(textOf(res)).toBe(`Authenticated as ${FAKE_EMAIL}.`); + expect(sendNotification).not.toHaveBeenCalled(); + }); + + // ------------------------------------------------------------------------- + // Cancellation / errors / sequential reuse + // ------------------------------------------------------------------------- + + it('returns cancelled without binding a listener if the signal is already aborted', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const auth = makeState({ store, startListener: listener.start, openBrowser: fakeBrowser().open }); + const ac = new AbortController(); + ac.abort(); + + const res = await auth.handleAuthenticate({ signal: ac.signal }); + expect(res.isError).toBe(true); + expect(textOf(res)).toMatch(/cancelled/i); + expect(listener.recorded.calls).toBe(0); }); - it('short-circuits when the hub is known to require no auth', async () => { - const h = await makeHarness({ - authMode: 'no-auth', - responder: () => { - throw new Error('should not be called — hub is no-auth'); - }, - }); - const res = await h.state.handleStart(); - expect(textOf(res)).toMatch(/does not require authentication/i); - expect(h.requests).toHaveLength(0); - }); + it('maps a mid-flight abort (listener rejection) to a typed cancellation result', async () => { + const { store } = emptyStore(); + const listener = fakeListener({ rejectWith: new LoopbackAbortedError() }); + const auth = makeState({ store, startListener: listener.start, openBrowser: fakeBrowser().open }); - it('initiates device flow when the hub is known to require auth', async () => { - const h = await makeHarness({ - authMode: 'requires-auth', - responder: () => jsonResponse(200, deviceAuthBody()), - }); - await h.state.handleStart(); - expect(h.requests).toHaveLength(1); - expect(h.requests[0]!.url).toBe('https://oauth2.googleapis.com/device/code'); + const res = await auth.handleAuthenticate({}); + expect(res.isError).toBe(true); + expect(textOf(res)).toBe('Sign-in was cancelled.'); }); - it('initiates device flow when auth mode is unknown (positive observation required for short-circuit)', async () => { - const h = await makeHarness({ - authMode: 'unknown', - responder: () => jsonResponse(200, deviceAuthBody()), - }); - await h.state.handleStart(); - expect(h.requests).toHaveLength(1); - }); + it('reports a timeout with the manual-paste URL', async () => { + const { store } = emptyStore(); + const listener = fakeListener({ rejectWith: new LoopbackTimeoutError() }); + const browser = fakeBrowser(); + const auth = makeState({ store, startListener: listener.start, openBrowser: browser.open }); - it('coalesces repeated start calls within the configured window', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - const h = await makeHarness({ - initialNow: t0, - coalesceWindowMs: 5000, - responder: () => jsonResponse(200, deviceAuthBody()), - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 2_000)); - await h.state.handleStart(); - expect(h.requests).toHaveLength(1); + const res = await auth.handleAuthenticate({}); + expect(res.isError).toBe(true); + expect(textOf(res)).toMatch(/Timed out/); + expect(textOf(res)).toContain('https://accounts.google.com/o/oauth2/v2/auth'); }); - it('overwrites a prior unconsumed device_code when started outside the coalescing window', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let pass = 0; - const h = await makeHarness({ - initialNow: t0, - coalesceWindowMs: 5000, - responder: () => { - pass += 1; - return jsonResponse( - 200, - deviceAuthBody({ - device_code: `device-${pass}`, - user_code: pass === 1 ? 'FIRST-CODE' : 'SECOND-CODE', - }), - ); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 10_000)); - const second = await h.state.handleStart(); - expect(h.requests).toHaveLength(2); - expect(textOf(second)).toContain('SECOND-CODE'); + it('reports a listener bind failure', async () => { + const { store } = emptyStore(); + const failingStart: typeof startLoopbackListener = async () => { + throw new Error('EADDRINUSE'); + }; + const auth = makeState({ store, startListener: failingStart, openBrowser: fakeBrowser().open }); + const res = await auth.handleAuthenticate({}); + expect(res.isError).toBe(true); + expect(textOf(res)).toMatch(/Failed to start the local sign-in listener/); }); - it('clears an expired cached device_code on the next start', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let pass = 0; - const h = await makeHarness({ - initialNow: t0, - coalesceWindowMs: 60_000, - responder: () => { - pass += 1; - return jsonResponse( - 200, - deviceAuthBody({ - device_code: `device-${pass}`, - user_code: pass === 1 ? 'FIRST-CODE' : 'SECOND-CODE', - expires_in: 60, // expires in 60s - }), - ); - }, - }); - await h.state.handleStart(); - // Advance past expiry — far outside the 60s window means the - // cached code has expired and a fresh flow must initiate. - h.setNow(new Date(t0.getTime() + 120_000)); - const second = await h.state.handleStart(); - expect(h.requests).toHaveLength(2); - expect(textOf(second)).toContain('SECOND-CODE'); - }); -}); - -// --------------------------------------------------------------------------- -// authenticate_finish -// --------------------------------------------------------------------------- + it('reports a token-exchange failure', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const { fetch } = makeFetch(() => + jsonResponse(400, { error: 'invalid_grant', error_description: 'bad code' }), + ); + const auth = makeState({ store, fetch, startListener: listener.start, openBrowser: browser.open }); -describe('authenticate_finish', () => { - it('returns a typed error when called without a prior start', async () => { - const h = await makeHarness({ - responder: () => { - throw new Error('should not be called'); - }, - }); - const res = await h.state.handleFinish(); + const res = await auth.handleAuthenticate({}); expect(res.isError).toBe(true); - expect(textOf(res)).toMatch(/authenticate_start/); - expect(h.requests).toHaveLength(0); + expect(textOf(res)).toMatch(/Token exchange failed/); }); - it('returns user-actionable text on authorization_pending', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - // First request → device_authorization. Second → token (pending). - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(400, { error: 'authorization_pending' }); - }, + it('accepts a follow-up call after a failed one (no stale state)', async () => { + const { store } = emptyStore(); + const browser = fakeBrowser(); + const timeoutListener = fakeListener({ rejectWith: new LoopbackTimeoutError() }); + const first = makeState({ + store, + startListener: timeoutListener.start, + openBrowser: browser.open, }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - const res = await h.state.handleFinish(); - expect(res.isError).toBeFalsy(); - expect(textOf(res)).toMatch(/waiting|pending/i); - }); - - it('returns user-actionable text with a wait hint on slow_down', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(400, { error: 'slow_down' }); - }, + const r1 = await first.handleAuthenticate({}); + expect(r1.isError).toBe(true); + + const okListener = fakeListener({}); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const second = makeState({ + store, + fetch, + startListener: okListener.start, + openBrowser: browser.open, }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - const res = await h.state.handleFinish(); - expect(textOf(res)).toMatch(/slow|wait/i); + const r2 = await second.handleAuthenticate({}); + expect(r2.isError).toBeUndefined(); + expect(textOf(r2)).toBe(`Authenticated as ${FAKE_EMAIL}.`); }); +}); - it('persists the bundle via CredentialStore on success', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const idToken = fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }); - const refresh = '1//new-refresh-token'; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(200, tokenSuccessBody({ id_token: idToken, refresh_token: refresh })); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - expect(h.storeState.value).not.toBeNull(); - const blob = JSON.parse(h.storeState.value!); - expect(blob.id_token).toBe(idToken); - expect(blob.refresh_token).toBe(refresh); - }); +// =========================================================================== +// authenticate_clear — revocation contract +// =========================================================================== - it('clears the cached device_code on success', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(200, tokenSuccessBody()); +describe('authenticate_clear', () => { + it('revokes the refresh token at Google then clears the keyring (clean case)', async () => { + const { store, state } = await seededStore(makeBundle({ refreshToken: '1//to-revoke' })); + const { fetch, requests } = makeFetch(() => new Response('', { status: 200 })); + const auth = makeState({ store, fetch }); + + const res = await auth.handleClear(); + + const revokeReq = requests.find((r) => r.url === AS.revocation_endpoint)!; + expect(revokeReq.method).toBe('POST'); + expect(revokeReq.body.get('token')).toBe('1//to-revoke'); + expect(revokeReq.body.get('token_type_hint')).toBe('refresh_token'); + expect(revokeReq.body.get('client_id')).toBeNull(); + expect(revokeReq.body.get('client_secret')).toBeNull(); + expect(revokeReq.headers['content-type']).toBe('application/x-www-form-urlencoded'); + + expect(state.value).toBeNull(); + expect(textOf(res)).toMatch(/cleared and revoked at Google/); + expect(res.isError).toBeUndefined(); + }); + + it('clears locally even when the revoke fails, without leaking the token', async () => { + const { store, state } = await seededStore(makeBundle({ refreshToken: '1//secret-rt' })); + const { fetch } = makeFetch(() => new Response('boom', { status: 500 })); + const auth = makeState({ store, fetch }); + + const res = await auth.handleClear(); + + expect(state.value).toBeNull(); + expect(textOf(res)).toMatch(/cleared locally/); + expect(textOf(res)).toMatch(/revocation failed/); + expect(textOf(res)).toContain('myaccount.google.com'); + expect(textOf(res)).not.toContain('1//secret-rt'); + expect(res.isError).toBeUndefined(); + }); + + it('does not hit the revoke endpoint when there is nothing to clear', async () => { + const { store } = emptyStore(); + const { fetch, requests } = makeFetch(() => new Response('', { status: 200 })); + const auth = makeState({ store, fetch }); + + const res = await auth.handleClear(); + expect(requests).toHaveLength(0); + expect(textOf(res)).toMatch(/cleared\. Call authenticate to sign in again/); + expect(res.isError).toBeUndefined(); + }); + + it('uses the failure path and notes the revoke ran when the local delete fails', async () => { + // Backend that yields a bundle on read but throws on clear. + const seed = memoryBackend(null); + const store = new CredentialStore(CFG, { + read: seed.backend.read, + write: seed.backend.write, + async clear() { + throw new Error('keyring locked'); }, }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - // Now a second finish without a fresh start must report "no flow". - const res = await h.state.handleFinish(); - expect(res.isError).toBe(true); - expect(textOf(res)).toMatch(/authenticate_start/); - }); + await store.write(makeBundle({ refreshToken: '1//rt' })); - it('clears the cached device_code on terminal access_denied', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(400, { error: 'access_denied' }); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - const res = await h.state.handleFinish(); - expect(res.isError).toBe(true); - expect(textOf(res)).toMatch(/authenticate_start/); - }); + const { fetch } = makeFetch(() => new Response('', { status: 200 })); + const auth = makeState({ store, fetch }); - it('clears the cached device_code on terminal expired_token', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(400, { error: 'expired_token' }); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - const res = await h.state.handleFinish(); + const res = await auth.handleClear(); expect(res.isError).toBe(true); - expect(textOf(res)).toMatch(/authenticate_start/); + expect(textOf(res)).toMatch(/Failed to clear the OS keyring entry/); + expect(textOf(res)).toMatch(/revoked at Google/); }); +}); - it('returns "Authenticated as " from the id_token email claim', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const idToken = fakeIdToken({ exp: farFutureExp(), email: 'someone@posit.co' }); - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(200, tokenSuccessBody({ id_token: idToken })); - }, +// =========================================================================== +// authenticate — pre-flight refresh failure (TokenRefreshError) +// =========================================================================== + +describe('authenticate pre-flight refresh failure', () => { + // A bundle whose id_token is inside the 60s skew so the pre-flight + // getValidIdToken() forces a refresh (rather than returning the cache). + function nearExpiryBundle(): CredentialBundle { + const nearExp = Math.floor(Date.now() / 1000) + 30; + return makeBundle({ + idToken: fakeIdToken({ exp: nearExp, email: FAKE_EMAIL }), + idTokenExpiresAt: new Date(nearExp * 1000), }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - const res = await h.state.handleFinish(); - expect(textOf(res)).toContain('someone@posit.co'); - expect(textOf(res)).toMatch(/authenticated/i); - }); + } - it('tool responses never contain id_token or refresh_token bytes', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const idToken = fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }); - const refresh = '1//super-secret-rt-XYZ'; - const access = 'ya29.super-secret-access'; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse( - 200, - tokenSuccessBody({ id_token: idToken, refresh_token: refresh, access_token: access }), - ); - }, + function stateWithRefreshFetch(args: { + store: CredentialStore; + refreshFetch: typeof fetch; + startListener?: typeof startLoopbackListener; + openBrowser?: typeof openBrowser; + }): AuthToolsState { + const refreshManager = new RefreshManager({ + authServer: async () => AS, + config: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, + store: args.store, + fetch: args.refreshFetch, }); - const startRes = await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - const finishRes = await h.state.handleFinish(); - const blob = textOf(startRes) + '\n' + textOf(finishRes); - expect(blob).not.toContain(idToken); - expect(blob).not.toContain(refresh); - expect(blob).not.toContain(access); - }); - - it('returns slow_down advice without polling Google when called before interval elapsed', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - // Anything beyond the device-auth request would be a violation. - throw new Error('Google should not have been polled before interval elapsed'); - }, + return new AuthToolsState({ + credentialStore: args.store, + refreshManager, + connectionManager: authMode('requires-auth'), + flowConfig: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, issuer: ISSUER }, + authServer: async () => AS, + startListener: args.startListener, + openBrowser: args.openBrowser, + logger: () => undefined, }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 1_000)); // 1s — way before the 5s interval - const res = await h.state.handleFinish(); - expect(textOf(res)).toMatch(/wait|pending/i); - expect(h.requests).toHaveLength(1); // only the device-auth call - }); + } - it('polls Google once the device-auth interval has elapsed', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse(400, { error: 'authorization_pending' }); - }, + it('returns an actionable error (not -32603) and does not open the browser on invalid_client', async () => { + const { store } = await seededStore(nearExpiryBundle()); + const { fetch: refreshFetch } = makeFetch(() => + jsonResponse(401, { + error: 'invalid_client', + error_description: 'The provided client secret is invalid.', + }), + ); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const auth = stateWithRefreshFetch({ + store, + refreshFetch, + startListener: listener.start, + openBrowser: browser.open, }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - expect(h.requests).toHaveLength(2); // device-auth + one token poll - }); - it('increases the subsequent poll interval after slow_down (RFC 8628 §3.5)', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - if (phase === 2) return jsonResponse(400, { error: 'slow_down' }); - throw new Error('Should not poll Google a second time before bumped interval elapses'); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); // slow_down → bump - // Advance by the original interval (5s) — must not poll because - // slow_down has bumped the next-allowed-at by an additional 5s. - h.setNow(new Date(t0.getTime() + 11_000)); - await h.state.handleFinish(); - expect(h.requests).toHaveLength(2); // device-auth + the one slow_down poll - }); + let res: CallToolResult; + try { + res = await auth.handleAuthenticate(); + } catch (err) { + // Pre-fix behaviour: the raw oauth4webapi error escaped as a throw + // (surfacing as MCP -32603). The fix must turn it into a result. + throw new Error( + `handleAuthenticate threw instead of returning an error result: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } - it('serialises concurrent finish calls safely', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const idToken = fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }); - const h = await makeHarness({ - initialNow: t0, - responder: async () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - if (phase === 2) { - // Delay so a second concurrent finish overlaps with the first. - await new Promise((r) => setTimeout(r, 10)); - return jsonResponse(200, tokenSuccessBody({ id_token: idToken })); - } - return jsonResponse(400, { error: 'authorization_pending' }); - }, - }); - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - const [a, b] = await Promise.all([h.state.handleFinish(), h.state.handleFinish()]); - // The first call should succeed; the second sees the cleared - // device_code and surfaces the "no flow in progress" tool error. - const aText = textOf(a); - const bText = textOf(b); - const successCount = [aText, bText].filter((t) => /authenticated as/i.test(t)).length; - expect(successCount).toBe(1); - const errorCount = [a, b].filter((r) => r.isError).length; - expect(errorCount).toBe(1); + expect(res.isError).toBe(true); + const msg = textOf(res); + expect(msg).toContain('invalid_client'); + expect(msg).toContain('The provided client secret is invalid.'); + expect(msg).toMatch(/QUARTO_HUB_MCP_CLIENT_(ID|SECRET)/); + expect(msg).not.toBe('server responded with an error in the response body'); + // A config error is not fixable by a browser sign-in — none was attempted. + expect(browser.calls).toHaveLength(0); + expect(listener.recorded.calls).toBe(0); }); }); -// --------------------------------------------------------------------------- -// authenticate_clear -// --------------------------------------------------------------------------- - -describe('authenticate_clear', () => { - it('clears the credential store', async () => { - const t0 = new Date('2026-05-22T12:00:00Z'); - const h = await makeHarness({ - initialNow: t0, - seedBundle: { - idToken: fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }), - refreshToken: '1//rt-seeded', - idTokenExpiresAt: new Date(t0.getTime() + 60 * 60 * 1000), - scopes: ['openid', 'email', 'profile'], +// =========================================================================== +// Lazy discovery failure handling +// =========================================================================== + +describe('lazy discovery failure', () => { + it('authenticate returns a clean error (no listener, no browser) when discovery fails', async () => { + const { store } = emptyStore(); + const listener = fakeListener({}); + const browser = fakeBrowser(); + const auth = makeState({ + store, + mode: 'requires-auth', + startListener: listener.start, + openBrowser: browser.open, + authServer: async () => { + throw new Error('getaddrinfo ENOTFOUND accounts.google.com'); }, - responder: () => new Response('', { status: 500 }), }); - expect(await h.store.read()).not.toBeNull(); - const res = await h.state.handle('authenticate_clear'); - expect(res.isError).not.toBe(true); - expect(textOf(res).toLowerCase()).toContain('cleared'); - expect(await h.store.read()).toBeNull(); - }); - - it('clears any in-progress device-flow cache', async () => { - const t0 = new Date('2026-05-22T12:00:00Z'); - const h = await makeHarness({ - initialNow: t0, - responder: () => jsonResponse(200, deviceAuthBody({ interval: 5 })), - }); - await h.state.handleStart(); - expect(h.state.hasCachedDeviceCode()).toBe(true); + const res = await auth.handleAuthenticate({}); - await h.state.handle('authenticate_clear'); - expect(h.state.hasCachedDeviceCode()).toBe(false); - }); - - it('is safe when no credentials are present', async () => { - const h = await makeHarness({ - responder: () => new Response('', { status: 500 }), - }); - expect(await h.store.read()).toBeNull(); - - const res = await h.state.handle('authenticate_clear'); - expect(res.isError).not.toBe(true); - expect(await h.store.read()).toBeNull(); - }); - - it('does not call Google', async () => { - const t0 = new Date('2026-05-22T12:00:00Z'); - const h = await makeHarness({ - initialNow: t0, - seedBundle: { - idToken: fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }), - refreshToken: '1//rt-seeded', - idTokenExpiresAt: new Date(t0.getTime() + 60 * 60 * 1000), - scopes: ['openid', 'email', 'profile'], - }, - responder: () => { - throw new Error('authenticate_clear should never hit Google'); + expect(res.isError).toBe(true); + expect(textOf(res)).toMatch(/discovery endpoint/i); + // Discovery is resolved before the listener is bound, so a failure + // leaves nothing started. + expect(listener.recorded.calls).toBe(0); + expect(browser.calls).toHaveLength(0); + }); + + it('authenticate_clear still deletes the local keyring entry when discovery fails during revoke', async () => { + const { store, state } = await seededStore(); + const auth = makeState({ + store, + authServer: async () => { + throw new Error('getaddrinfo ENOTFOUND accounts.google.com'); }, }); - await h.state.handle('authenticate_clear'); - expect(h.requests).toHaveLength(0); - }); -}); -// --------------------------------------------------------------------------- -// Cross-cutting logging redaction -// --------------------------------------------------------------------------- + const res = await auth.handleClear(); -describe('logging redaction', () => { - it('does not log id_token / refresh_token / access_token across any console sink', async () => { - const t0 = new Date('2026-05-21T12:00:00Z'); - let phase = 0; - const idToken = fakeIdToken({ exp: farFutureExp(), email: FAKE_EMAIL }); - const refresh = '1//secret-rt-XYZ'; - const access = 'ya29.secret-access'; - const h = await makeHarness({ - initialNow: t0, - responder: () => { - phase += 1; - if (phase === 1) return jsonResponse(200, deviceAuthBody({ interval: 5 })); - return jsonResponse( - 200, - tokenSuccessBody({ id_token: idToken, refresh_token: refresh, access_token: access }), - ); - }, - }); - const sinks = (['debug', 'log', 'info', 'warn', 'error'] as const).map((m) => - vi.spyOn(console, m).mockImplementation(() => undefined), - ); - try { - await h.state.handleStart(); - h.setNow(new Date(t0.getTime() + 6_000)); - await h.state.handleFinish(); - const blob = sinks - .flatMap((s) => s.mock.calls.flat()) - .map((c) => (typeof c === 'string' ? c : JSON.stringify(c))) - .join(' '); - expect(blob).not.toContain(idToken); - expect(blob).not.toContain(refresh); - expect(blob).not.toContain(access); - } finally { - sinks.forEach((s) => s.mockRestore()); - } + // The local delete is the invariant: a discovery failure on the + // best-effort revoke must not block it. + expect(res.isError).toBeFalsy(); + expect(textOf(res)).toMatch(/cleared locally/i); + expect(textOf(res)).toMatch(/myaccount\.google\.com/); + expect(await store.read()).toBeNull(); + expect(state.value).toBeNull(); }); }); diff --git a/ts-packages/quarto-hub-mcp/src/auth/auth-tools.ts b/ts-packages/quarto-hub-mcp/src/auth/auth-tools.ts index ad337d47..2ac42ec8 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/auth-tools.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/auth-tools.ts @@ -1,25 +1,32 @@ /** - * Phase 7 — MCP auth-tool surface. + * MCP auth-tool surface — Authorization Code + PKCE + loopback. * - * Exposes `authenticate_start` and `authenticate_finish` as MCP tools. - * stdio-transport MCP clients (Claude Code, Cursor, etc.) capture - * stderr to log files; the tool response is the only agent-visible - * channel. Uses only standard `CallToolResult.content` text — no - * client-specific rendering hints. + * Exposes two MCP tools: * - * State lives on {@link AuthToolsState} (closure-local cached - * device_code; never persisted). `device_code` is RFC 8628 §3.5 - * rate-limited against `nextPollAllowedAt`; `slow_down` responses - * bump the interval by 5 s per the RFC. + * - `authenticate` — runs the full loopback sign-in: binds a local + * `127.0.0.1` listener, opens the browser at Google's authorization + * endpoint, waits for the redirect, exchanges the code (with the + * PKCE verifier and the Desktop-app `client_secret`), and stores the + * resulting tokens in the OS keyring. Single blocking call — + * replaces the device-flow `authenticate_start` / `authenticate_finish` + * pair. + * - `authenticate_clear` — best-effort revoke at Google, then delete + * the local keyring entry. Read → revoke → delete order so a crash + * between revoke and delete leaves a retryable state. * - * The canonical verification URL is a hard-coded constant in this - * module — never derived from Google's response — so an attacker - * who controls Google's reply cannot phish the user into typing - * the code on an attacker-controlled URL. + * stdio-transport MCP clients capture stderr to log files; the tool + * response is the only agent-visible channel. All log call sites funnel + * through `redactTokens`. The authorization URL is deliberately *not* + * redacted — it is public by construction and is the actionable link for + * headless machines. */ import type { Server } from '@modelcontextprotocol/sdk/server/index.js'; -import type { CallToolResult, Tool } from '@modelcontextprotocol/sdk/types.js'; +import type { + CallToolResult, + ServerNotification, + Tool, +} from '@modelcontextprotocol/sdk/types.js'; import { CallToolRequestSchema, ListToolsRequestSchema, @@ -27,27 +34,36 @@ import { import { decodeJwt } from 'jose'; import * as oauth from 'oauth4webapi'; +import { openBrowser as defaultOpenBrowser } from './browser.js'; import type { CredentialBundle, CredentialStore } from './credential-store.js'; import { - DeviceFlowDeniedError, - DeviceFlowError, - DeviceFlowExpiredError, - initiateDeviceFlow, - pollDeviceFlowOnce, - redactTokens, -} from './device-flow.js'; -import { type RefreshManager, ReauthRequired } from './refresh-manager.js'; + AUTHENTICATE_TIMEOUT_MS, + LoopbackAbortedError, + LoopbackAuthorizationError, + LoopbackStateMismatchError, + LoopbackTimeoutError, + startLoopbackListener as defaultStartLoopbackListener, + type LoopbackListener, + type LoopbackResult, +} from './loopback.js'; +import type { AuthServerProvider } from './oauth-config.js'; +import { generatePkceParams } from './pkce.js'; +import { redactTokens } from './redact.js'; +import { + type RefreshManager, + ReauthRequired, + TokenRefreshError, +} from './refresh-manager.js'; // --------------------------------------------------------------------------- // Public constants / types // --------------------------------------------------------------------------- -/** Hard-coded canonical verification URL — never sourced from Google. */ -export const CANONICAL_VERIFICATION_URL = 'https://www.google.com/device'; - const DEFAULT_SCOPES = ['openid', 'email', 'profile'] as const; -const SLOW_DOWN_BUMP_SECONDS = 5; -const DEFAULT_COALESCE_WINDOW_MS = 5_000; + +/** Google's authorization endpoint; used if discovery omits the field. */ +const GOOGLE_AUTHORIZATION_ENDPOINT = + 'https://accounts.google.com/o/oauth2/v2/auth'; export interface LastObservedAuthModeSource { lastObservedAuthMode(): 'no-auth' | 'requires-auth' | 'unknown'; @@ -58,6 +74,35 @@ export interface AuthFlowConfig { readonly clientSecret: string; readonly issuer: string; readonly scopes?: readonly string[]; + /** Explicit loopback port (from `--redirect-port`); `0`/undefined = kernel-picks. */ + readonly redirectPort?: number; +} + +/** + * Minimal progress-notification shape — a structural subset of the MCP + * SDK's `ServerNotification` so this module doesn't depend on the SDK's + * exact generic wiring. The server-wiring helper adapts the real + * `extra.sendNotification` onto this. + */ +export interface ProgressNotification { + readonly method: 'notifications/progress'; + readonly params: { + readonly progressToken: string | number; + readonly progress: number; + readonly total?: number; + readonly message?: string; + }; +} + +/** + * Per-call MCP context threaded from the `CallToolRequest` handler: + * cancellation signal, progress token (only present when the caller + * requested progress), and the notification sender. + */ +export interface AuthToolContext { + readonly signal?: AbortSignal; + readonly progressToken?: string | number; + readonly sendNotification?: (n: ProgressNotification) => Promise; } export interface AuthToolsDeps { @@ -65,44 +110,33 @@ export interface AuthToolsDeps { readonly refreshManager: RefreshManager; readonly connectionManager: LastObservedAuthModeSource; readonly flowConfig: AuthFlowConfig; - readonly authorizationServer: oauth.AuthorizationServer; - /** Clock override for tests. */ - readonly now?: () => Date; - /** `fetch` override for tests. */ + /** Lazily-resolved authorization server; called only when a sign-in or revoke fires. */ + readonly authServer: AuthServerProvider; + /** `fetch` override for tests (token exchange + revocation). */ readonly fetch?: typeof fetch; - /** - * Window in ms within which a repeat `authenticate_start` returns - * the cached `device_code` instead of re-initiating. Default: 5 s. - */ - readonly coalesceWindowMs?: number; + /** Deadline override for tests; defaults to {@link AUTHENTICATE_TIMEOUT_MS}. */ + readonly timeoutMs?: number; + /** Whether to send `prompt=consent` (default true — see plan). */ + readonly promptConsent?: boolean; + /** stderr-logger seam for tests. */ + readonly logger?: (msg: string) => void; + /** Loopback-listener seam for tests. */ + readonly startListener?: typeof defaultStartLoopbackListener; + /** Browser-opener seam for tests. */ + readonly openBrowser?: typeof defaultOpenBrowser; } export const AUTH_TOOL_DEFINITIONS: readonly Tool[] = [ { - name: 'authenticate_start', - description: - 'Begin authenticating Quarto Hub MCP against the configured hub. ' + - 'Returns a verification URL and a short user code that the human ' + - 'must enter in their browser to grant access. Idempotent within a ' + - 'short window: calling twice in quick succession returns the same ' + - 'code. If credentials are already valid, returns "Already ' + - 'authenticated as " instead.', - inputSchema: { type: 'object', properties: {} }, - annotations: { - readOnlyHint: false, - destructiveHint: false, - idempotentHint: false, - }, - }, - { - name: 'authenticate_finish', + name: 'authenticate', description: - 'Finalise the device-flow authentication started by ' + - 'authenticate_start. Polls Google exactly once for the result. ' + - 'Returns "Authenticated as " on success; "still pending" / ' + - '"slow down" text while the human has not yet approved; a typed ' + - 'error if the flow expired or was denied. Call again after the ' + - 'browser approval to retry.', + 'Authenticate Quarto Hub MCP against the configured hub. Opens the ' + + "user's browser to a Google sign-in page and waits for them to " + + 'complete it, then stores the credentials in the OS keyring. ' + + 'Returns "Authenticated as " on success. If credentials are ' + + 'already valid, returns "Already authenticated as " without ' + + 'opening a browser. The authorization URL is also printed so a user ' + + 'on a headless or SSH session can open it manually.', inputSchema: { type: 'object', properties: {} }, annotations: { readOnlyHint: false, @@ -114,12 +148,14 @@ export const AUTH_TOOL_DEFINITIONS: readonly Tool[] = [ name: 'authenticate_clear', description: 'Remove any locally-cached Quarto Hub credentials from the OS ' + - 'keyring and discard any in-progress device-flow state. Use this ' + - 'as an escape hatch when the hub rejects the cached credentials ' + - 'and authenticate_start short-circuits with "Already authenticated".' + - ' Does not touch Google-side grants; revoke those at ' + - 'myaccount.google.com if needed. Idempotent: safe to call when ' + - 'no credentials are present.', + 'keyring and discard any in-progress sign-in. Best-effort revokes ' + + 'the stored refresh token at Google before the local delete, so the ' + + 'credential is rendered unusable both locally and server-side; if ' + + 'the revoke fails (offline, token already invalid) the local delete ' + + 'still proceeds and you can revoke the grant manually at ' + + 'myaccount.google.com. Use this as an escape hatch when the hub ' + + 'rejects the cached credentials. Idempotent: safe to call when no ' + + 'credentials are present.', inputSchema: { type: 'object', properties: {} }, annotations: { readOnlyHint: false, @@ -129,10 +165,7 @@ export const AUTH_TOOL_DEFINITIONS: readonly Tool[] = [ }, ]; -export type AuthToolName = - | 'authenticate_start' - | 'authenticate_finish' - | 'authenticate_clear'; +export type AuthToolName = 'authenticate' | 'authenticate_clear'; // --------------------------------------------------------------------------- // Result helpers @@ -146,54 +179,37 @@ function errorResult(msg: string): CallToolResult { return { content: [{ type: 'text', text: msg }], isError: true }; } -// --------------------------------------------------------------------------- -// Cached device-flow state -// --------------------------------------------------------------------------- - -interface CachedDevice { - readonly deviceCode: string; - readonly userCode: string; - readonly verificationUri: string; - readonly expiresAt: Date; - readonly startTime: Date; - // Mutable: bumped on slow_down and on each poll attempt. - interval: number; - nextPollAllowedAt: Date; -} - // --------------------------------------------------------------------------- // AuthToolsState // --------------------------------------------------------------------------- /** - * The handler state. Tests can drive `handleStart`/`handleFinish` + * The handler state. Tests drive `handleAuthenticate` / `handleClear` * directly without spinning up an MCP `Server`. */ export class AuthToolsState { private readonly deps: AuthToolsDeps; - private cached: CachedDevice | undefined; - // Mutex chain — concurrent finish calls serialise so the second - // observes the first's cache mutation. - private finishTail: Promise = Promise.resolve(); constructor(deps: AuthToolsDeps) { this.deps = deps; } - /** Test hook — observe whether a non-expired device_code is cached. */ - hasCachedDeviceCode(): boolean { - this.clearCacheIfExpired(); - return this.cached !== undefined; - } - - async handle(name: AuthToolName): Promise { - if (name === 'authenticate_start') return this.handleStart(); - if (name === 'authenticate_finish') return this.handleFinish(); + async handle(name: AuthToolName, ctx: AuthToolContext = {}): Promise { + if (name === 'authenticate') return this.handleAuthenticate(ctx); if (name === 'authenticate_clear') return this.handleClear(); return errorResult(`Unknown auth tool: ${String(name)}`); } - async handleStart(): Promise { + /** + * Run the loopback sign-in. + * + * MCP stdio hosts serialise `tools/call`; this handler intentionally + * has no concurrency guard. Two simultaneous calls is + * undefined-but-non-corrupting behaviour — PKCE and `state` bind each + * flow's tokens to its own callback, so the worst case is two browser + * tabs, not token cross-contamination. + */ + async handleAuthenticate(ctx: AuthToolContext = {}): Promise { // 1. Already authenticated → short-circuit without touching Google. try { const idToken = await this.deps.refreshManager.getValidIdToken(); @@ -204,87 +220,170 @@ export class AuthToolsState { : 'Already authenticated. No action needed.', ); } catch (err) { - // Only ReauthRequired falls through to the device-flow path — - // every other failure (network blip, malformed JWT, etc.) is - // signal we shouldn't silently start a new device flow. + // A structured refresh failure isn't fixable by a browser sign-in; + // surface the actionable message rather than running the loopback. + if (err instanceof TokenRefreshError) return errorResult(err.message); + // Only ReauthRequired falls through to the loopback path; every + // other failure (network blip, malformed JWT, etc.) propagates. if (!(err instanceof ReauthRequired)) throw err; } - // 2. Hub is known to not require auth → short-circuit. Only the - // positive `'no-auth'` observation triggers; `'requires-auth'` and - // `'unknown'` both fall through. + // 2. Hub is known to not require auth → short-circuit. if (this.deps.connectionManager.lastObservedAuthMode() === 'no-auth') { return textResult( 'The configured hub does not require authentication; no action needed.', ); } - this.clearCacheIfExpired(); + if (ctx.signal?.aborted) return errorResult('Sign-in was cancelled.'); - // 3. Coalesce repeated starts within the configured window so the - // agent doesn't burn a new device_code on each redundant call. - if (this.cached) { - const ageMs = this.now().getTime() - this.cached.startTime.getTime(); - if (ageMs < this.coalesceWindowMs) { - return this.startResponseText(this.cached); - } + // 3. Resolve the authorization server lazily — discovery is deferred + // off the startup path, so the network call (and any failure) lands + // here, on the first sign-in that actually needs it. + let as: oauth.AuthorizationServer; + try { + as = await this.deps.authServer(); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + return errorResult( + `Failed to reach the identity provider's discovery endpoint: ${redactTokens(msg)}`, + ); } - // 4. Initiate a fresh device flow. - const dr = await initiateDeviceFlow( - this.deps.authorizationServer, - { - clientId: this.deps.flowConfig.clientId, - clientSecret: this.deps.flowConfig.clientSecret, - scopes: this.deps.flowConfig.scopes ?? [...DEFAULT_SCOPES], - }, - this.deps.fetch ? { fetch: this.deps.fetch } : undefined, - ); + // 4. PKCE + state. + const pkce = await generatePkceParams(); - const startTime = this.now(); - const interval = dr.interval ?? 5; - const expiresAt = new Date(startTime.getTime() + dr.expires_in * 1000); - const nextPollAllowedAt = new Date(startTime.getTime() + interval * 1000); - - this.cached = { - deviceCode: dr.device_code, - userCode: dr.user_code, - verificationUri: dr.verification_uri, - expiresAt, - startTime, - interval, - nextPollAllowedAt, - }; + // 5. Bind the loopback listener *before* opening the browser, so the + // callback can land regardless of how the user reaches the URL. + let listener: LoopbackListener; + try { + listener = await (this.deps.startListener ?? defaultStartLoopbackListener)({ + expectedState: pkce.state, + port: this.deps.flowConfig.redirectPort ?? 0, + timeoutMs: this.deps.timeoutMs ?? AUTHENTICATE_TIMEOUT_MS, + signal: ctx.signal, + }); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + return errorResult( + `Failed to start the local sign-in listener: ${redactTokens(msg)}`, + ); + } - return this.startResponseText(this.cached); - } + try { + const authUrl = this.buildAuthorizationUrl(as, { + redirectUri: listener.redirectUri, + codeChallenge: pkce.codeChallenge, + state: pkce.state, + }); + + // Log the port so SSH-tunnel users don't need to guess it. + this.log(`loopback listener bound on 127.0.0.1:${listener.port}`); + + // 6. Surface the URL via MCP progress (if requested) and stderr, + // *before* launching the browser and regardless of its outcome — + // a silent `xdg-open` exit-0 on a headless box must not leave the + // user staring at a spinner for the whole deadline. + await this.surfaceAuthUrl(ctx, authUrl); + this.log(`open this URL to sign in: ${authUrl}`); + + // 7. Launch the browser (best-effort; never changes control flow). + let browserFailed = false; + const child = (this.deps.openBrowser ?? defaultOpenBrowser)(authUrl, { + signal: ctx.signal, + }); + if (!child) { + browserFailed = true; + } else { + child.once('error', () => { + browserFailed = true; + }); + } + + // 8. Block on the callback. + let callback: LoopbackResult; + try { + callback = await listener.result; + } catch (err) { + return this.loopbackErrorResult(err, authUrl); + } - async handleFinish(): Promise { - return this.enqueueFinish(() => this.doFinish()); + // 9. Exchange the code (PKCE verifier + client_secret). + let tokens: oauth.TokenEndpointResponse; + try { + tokens = await this.exchangeCode( + as, + callback.params, + listener.redirectUri, + pkce.codeVerifier, + pkce.state, + ); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + return errorResult(`Token exchange failed: ${redactTokens(msg)}`); + } + + // 10. Validate + store. + const stored = await this.storeTokens(tokens); + if (!stored.ok) return errorResult(stored.message); + + const note = browserFailed + ? ' Browser launch failed; you signed in manually.' + : ''; + return textResult( + stored.email + ? `Authenticated as ${stored.email}.${note}` + : `Authenticated.${note}`, + ); + } finally { + // Idempotent — settles a no-op if the flow already completed. + listener.close(); + } } /** - * Remove any persisted credential bundle and discard the in-process - * device-flow cache. Idempotent. The bundle going away forces the - * next `authenticate_start` to fall through `getValidIdToken`'s - * `ReauthRequired` branch and initiate a fresh device flow. + * Read → revoke → delete. Reading the refresh token first means a + * crash between revoke and delete leaves a retryable state; deleting + * first would orphan the token at Google. */ async handleClear(): Promise { - this.cached = undefined; + const bundle = await this.deps.credentialStore.read(); + + let revoke: 'skipped' | 'ok' | { failed: string } = 'skipped'; + if (bundle && bundle.refreshToken) { + revoke = await this.revokeRefreshToken(bundle.refreshToken); + } + try { await this.deps.credentialStore.clear(); } catch (err) { - // Surface as a tool error so the agent can show the user what - // went wrong (e.g. headless Linux without Secret Service). The - // in-memory cache was already cleared above, so a partial - // success is acceptable. - const msg = err instanceof Error ? err.message : String(err); + const msg = redactTokens(err instanceof Error ? err.message : String(err)); + const revokeNote = + revoke === 'ok' + ? ' The refresh token was revoked at Google.' + : revoke === 'skipped' + ? '' + : ' Google-side revocation was attempted and failed.'; return errorResult( - `Cleared in-memory device-flow state, but failed to clear the OS keyring entry: ${redactTokens(msg)}`, + `Failed to clear the OS keyring entry: ${msg}.${revokeNote}`, + ); + } + + if (revoke === 'skipped') { + return textResult( + 'Quarto Hub credentials cleared. Call authenticate to sign in again.', + ); + } + if (revoke === 'ok') { + return textResult( + 'Quarto Hub credentials cleared and revoked at Google. ' + + 'Call authenticate to sign in again.', ); } return textResult( - 'Quarto Hub credentials cleared. Call authenticate_start to authenticate again.', + `Quarto Hub credentials cleared locally. Google-side revocation ` + + `failed (${revoke.failed}); revoke the grant at myaccount.google.com ` + + `if you need it gone server-side.`, ); } @@ -292,86 +391,111 @@ export class AuthToolsState { // Internals // ------------------------------------------------------------------------- - private async doFinish(): Promise { - this.clearCacheIfExpired(); - if (!this.cached) { - return errorResult( - 'No device-flow in progress. Call authenticate_start to begin authentication.', - ); - } - // RFC 8628 §3.5 — gate on nextPollAllowedAt without hitting Google. - const nowMs = this.now().getTime(); - if (nowMs < this.cached.nextPollAllowedAt.getTime()) { - const waitSec = Math.max( - 1, - Math.ceil((this.cached.nextPollAllowedAt.getTime() - nowMs) / 1000), - ); - return textResult( - `Still pending — wait ${waitSec} second${waitSec === 1 ? '' : 's'} before retrying authenticate_finish.`, - ); + private buildAuthorizationUrl( + as: oauth.AuthorizationServer, + p: { + redirectUri: string; + codeChallenge: string; + state: string; + }, + ): string { + const endpoint = as.authorization_endpoint ?? GOOGLE_AUTHORIZATION_ENDPOINT; + const scopes = this.deps.flowConfig.scopes ?? DEFAULT_SCOPES; + const url = new URL(endpoint); + url.searchParams.set('response_type', 'code'); + url.searchParams.set('client_id', this.deps.flowConfig.clientId); + url.searchParams.set('redirect_uri', p.redirectUri); + url.searchParams.set('scope', scopes.join(' ')); + url.searchParams.set('code_challenge', p.codeChallenge); + url.searchParams.set('code_challenge_method', 'S256'); + url.searchParams.set('state', p.state); + // Required for a refresh_token; default (online) yields none. + url.searchParams.set('access_type', 'offline'); + // Lets a future scope addition piggy-back on the existing grant. + url.searchParams.set('include_granted_scopes', 'true'); + // Default on: a returning Desktop-app user is frequently re-issued an + // id_token without a refresh_token unless consent is forced. Drop + // only once Spike A proves the second-run refresh_token is returned. + if (this.deps.promptConsent ?? true) { + url.searchParams.set('prompt', 'consent'); } + return url.toString(); + } - let result: Awaited>; - try { - result = await pollDeviceFlowOnce( - this.deps.authorizationServer, - { - clientId: this.deps.flowConfig.clientId, - clientSecret: this.deps.flowConfig.clientSecret, - }, - this.cached.deviceCode, - this.deps.fetch ? { fetch: this.deps.fetch } : undefined, - ); - } catch (err) { - if ( - err instanceof DeviceFlowDeniedError || - err instanceof DeviceFlowExpiredError - ) { - this.cached = undefined; - return errorResult(redactTokens(err.message)); - } - if (err instanceof DeviceFlowError) { - this.cached = undefined; - return errorResult(redactTokens(err.message)); - } - throw err; - } + private async surfaceAuthUrl(ctx: AuthToolContext, url: string): Promise { + // Only fire if the caller requested progress (carried a progressToken). + if (ctx.progressToken === undefined || !ctx.sendNotification) return; + await ctx.sendNotification({ + method: 'notifications/progress', + params: { + progressToken: ctx.progressToken, + progress: 0, + total: 1, + message: `Open this URL in your browser to sign in: ${url}`, + }, + }); + } - if (result.kind === 'pending') { - this.bumpInterval(0); - return textResult( - "Still waiting for browser approval — once you've completed the consent screen, ask me to finish authentication again.", - ); - } - if (result.kind === 'slow_down') { - this.bumpInterval(SLOW_DOWN_BUMP_SECONDS); - return textResult( - `Google asked us to slow down — wait at least ${this.cached.interval} seconds before retrying authenticate_finish.`, - ); - } + private async exchangeCode( + as: oauth.AuthorizationServer, + callbackParams: URLSearchParams, + redirectUri: string, + codeVerifier: string, + expectedState: string, + ): Promise { + const client: oauth.Client = { client_id: this.deps.flowConfig.clientId }; + const clientAuth = oauth.ClientSecretPost(this.deps.flowConfig.clientSecret); + // Re-validate through oauth4webapi: brands the params for the grant + // request and applies the RFC 9207 `iss` check on top of the + // listener's own constant-time state check. + const validated = oauth.validateAuthResponse( + as, + client, + callbackParams, + expectedState, + ); + const requestOpts = this.deps.fetch + ? ({ [oauth.customFetch]: this.deps.fetch } as const) + : undefined; + const resp = requestOpts + ? await oauth.authorizationCodeGrantRequest( + as, + client, + clientAuth, + validated, + redirectUri, + codeVerifier, + requestOpts, + ) + : await oauth.authorizationCodeGrantRequest( + as, + client, + clientAuth, + validated, + redirectUri, + codeVerifier, + ); + return await oauth.processAuthorizationCodeResponse(as, client, resp); + } - // result.kind === 'tokens' - const bundle = result.bundle; + private async storeTokens( + bundle: oauth.TokenEndpointResponse, + ): Promise<{ ok: true; email: string | null } | { ok: false; message: string }> { if (typeof bundle.id_token !== 'string' || bundle.id_token === '') { - this.cached = undefined; - return errorResult('Token endpoint did not return an id_token.'); + return { ok: false, message: 'Token endpoint did not return an id_token.' }; } if (typeof bundle.refresh_token !== 'string' || bundle.refresh_token === '') { - this.cached = undefined; - return errorResult('Token endpoint did not return a refresh_token.'); + return { ok: false, message: 'Token endpoint did not return a refresh_token.' }; } let claims: ReturnType; try { claims = decodeJwt(bundle.id_token); } catch { - this.cached = undefined; - return errorResult('Token endpoint returned a malformed id_token.'); + return { ok: false, message: 'Token endpoint returned a malformed id_token.' }; } if (typeof claims.exp !== 'number') { - this.cached = undefined; - return errorResult('Token endpoint id_token has no exp claim.'); + return { ok: false, message: 'Token endpoint id_token has no exp claim.' }; } - const cred: CredentialBundle = { idToken: bundle.id_token, refreshToken: bundle.refresh_token, @@ -379,64 +503,85 @@ export class AuthToolsState { scopes: [...(this.deps.flowConfig.scopes ?? DEFAULT_SCOPES)], }; await this.deps.credentialStore.write(cred); - const email = typeof claims.email === 'string' ? claims.email : null; - this.cached = undefined; - return textResult( - email ? `Authenticated as ${email}.` : 'Authenticated.', - ); - } - - private bumpInterval(extraSeconds: number): void { - if (!this.cached) return; - this.cached.interval = this.cached.interval + extraSeconds; - this.cached.nextPollAllowedAt = new Date( - this.now().getTime() + this.cached.interval * 1000, - ); + return { ok: true, email }; } - private clearCacheIfExpired(): void { - if (this.cached && this.cached.expiresAt.getTime() <= this.now().getTime()) { - this.cached = undefined; + /** + * Best-effort refresh-token revocation. Google's revocation endpoint + * needs no client authentication — the token *is* the capability being + * burned — so we POST `token` + `token_type_hint` alone, no + * `client_id` / `client_secret`. Revoking the refresh token also + * invalidates derived access tokens (RFC 7009 §2.1). + */ + private async revokeRefreshToken( + refreshToken: string, + ): Promise<'ok' | { failed: string }> { + // Resolve discovery best-effort: a discovery failure here must not + // block the local keyring delete that follows in `handleClear`. + let as: oauth.AuthorizationServer; + try { + as = await this.deps.authServer(); + } catch (err) { + return { failed: redactTokens(err instanceof Error ? err.message : String(err)) }; + } + const endpoint = as.revocation_endpoint; + if (!endpoint) { + return { failed: 'no revocation endpoint advertised by the issuer' }; + } + try { + const body = new URLSearchParams({ + token: refreshToken, + token_type_hint: 'refresh_token', + }); + const fetchFn = this.deps.fetch ?? fetch; + const resp = await fetchFn(endpoint, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: body.toString(), + }); + if (!resp.ok) { + let reason = `HTTP ${resp.status}`; + try { + const json = (await resp.clone().json()) as { error?: unknown }; + if (typeof json.error === 'string') reason = `HTTP ${resp.status}: ${json.error}`; + } catch { + // Non-JSON body; the status alone is the reason. + } + return { failed: reason }; + } + return 'ok'; + } catch (err) { + const msg = redactTokens(err instanceof Error ? err.message : String(err)); + return { failed: msg }; } } - private startResponseText(c: CachedDevice): CallToolResult { - const expiresInSec = Math.max( - 0, - Math.floor((c.expiresAt.getTime() - this.now().getTime()) / 1000), - ); - const msg = [ - 'To authenticate Quarto Hub MCP:', - '', - `1. Open ${CANONICAL_VERIFICATION_URL} in your browser`, - ` (also valid: ${c.verificationUri})`, - `2. Enter this code: ${c.userCode}`, - '3. Sign in and approve the consent screen.', - '', - `The code expires in ${expiresInSec} seconds. Once you've completed those steps, ask me to finish authentication.`, - ].join('\n'); - return textResult(msg); - } - - private now(): Date { - return this.deps.now ? this.deps.now() : new Date(); - } - - private get coalesceWindowMs(): number { - return this.deps.coalesceWindowMs ?? DEFAULT_COALESCE_WINDOW_MS; + private loopbackErrorResult(err: unknown, authUrl: string): CallToolResult { + if (err instanceof LoopbackTimeoutError) { + return errorResult( + `Timed out waiting for browser sign-in. Open this URL to try again: ${authUrl}`, + ); + } + if (err instanceof LoopbackAbortedError) { + return errorResult('Sign-in was cancelled.'); + } + if (err instanceof LoopbackStateMismatchError) { + return errorResult( + 'Sign-in failed: the authorization callback state did not match ' + + '(possible CSRF). Try again.', + ); + } + if (err instanceof LoopbackAuthorizationError) { + return errorResult(`Sign-in failed: ${err.oauthError}.`); + } + const msg = err instanceof Error ? err.message : String(err); + return errorResult(`Sign-in failed: ${redactTokens(msg)}`); } - // Tail-promise chain mirroring CredentialStore's serialisation. - private enqueueFinish(op: () => Promise): Promise { - const prev = this.finishTail; - let resolveTail!: () => void; - this.finishTail = new Promise((r) => { - resolveTail = r; - }); - const run = prev.then(op, op); - run.finally(resolveTail).catch(() => undefined); - return run; + private log(msg: string): void { + const sink = this.deps.logger ?? ((m: string) => console.error(`[hub-mcp] ${m}`)); + sink(redactTokens(msg)); } } @@ -454,33 +599,42 @@ function extractEmail(idToken: string): string | null { // --------------------------------------------------------------------------- /** - * Registers `authenticate_start` and `authenticate_finish` on the MCP - * server. Must be called **before** {@link registerTools} so the - * read/write tools' "no credentials" error messages can name the auth - * tools as the recovery action. + * Adapt the MCP SDK's per-request `extra` onto {@link AuthToolContext}. + * The cast on `sendNotification` bridges our minimal + * {@link ProgressNotification} shape to the SDK's `ServerNotification`. + */ +export function extractAuthContext(extra: { + signal?: AbortSignal; + _meta?: { progressToken?: string | number }; + sendNotification?: (n: ServerNotification) => Promise; +}): AuthToolContext { + return { + signal: extra.signal, + progressToken: extra._meta?.progressToken, + sendNotification: extra.sendNotification + ? (n) => extra.sendNotification!(n as unknown as ServerNotification) + : undefined, + }; +} + +/** + * Registers `authenticate` / `authenticate_clear` on the MCP server. + * Must be called **before** {@link registerTools} so the read/write + * tools' "no credentials" errors can name the auth tools. * - * Returns the {@link AuthToolsState} so the caller can pass it back - * into `registerTools(server, manager, readOnly, authToolsState)`, - * which dispatches both tool families through a single - * `CallToolRequestSchema` handler. + * Returns the {@link AuthToolsState} so the caller can pass it back into + * `registerTools(...)`, which dispatches both tool families through a + * single `CallToolRequestSchema` handler. */ export function registerAuthTools(server: Server, deps: AuthToolsDeps): AuthToolsState { const state = new AuthToolsState(deps); - // First-pass registration. `registerTools` overrides these handlers - // with a dispatcher that consults both tool families; if it's never - // called (e.g. in a future "auth-only" entry point), the handlers - // installed here still respond correctly. server.setRequestHandler(ListToolsRequestSchema, async () => ({ tools: [...AUTH_TOOL_DEFINITIONS], })); - server.setRequestHandler(CallToolRequestSchema, async (request) => { - const { name, arguments: _args } = request.params; - if ( - name === 'authenticate_start' || - name === 'authenticate_finish' || - name === 'authenticate_clear' - ) { - return state.handle(name); + server.setRequestHandler(CallToolRequestSchema, async (request, extra) => { + const { name } = request.params; + if (name === 'authenticate' || name === 'authenticate_clear') { + return state.handle(name, extractAuthContext(extra)); } return errorResult(`Unknown tool: ${name}`); }); diff --git a/ts-packages/quarto-hub-mcp/src/auth/browser.test.ts b/ts-packages/quarto-hub-mcp/src/auth/browser.test.ts new file mode 100644 index 00000000..ce237310 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/browser.test.ts @@ -0,0 +1,82 @@ +/** + * Browser launcher — argv construction (the Windows form is the fiddly + * one) and abort-kills-subprocess behaviour. + */ + +import { EventEmitter } from 'node:events'; +import { describe, it, expect, vi } from 'vitest'; + +import { browserOpenSpec, openBrowser } from './browser.js'; + +const URL_WITH_AMP = + 'https://accounts.google.com/o/oauth2/v2/auth?response_type=code&client_id=x&redirect_uri=http://127.0.0.1:5000/callback&state=abc'; + +describe('browserOpenSpec', () => { + it('uses `open` on macOS', () => { + expect(browserOpenSpec('darwin', URL_WITH_AMP)).toEqual({ + command: 'open', + args: [URL_WITH_AMP], + }); + }); + + it('uses `xdg-open` on Linux', () => { + expect(browserOpenSpec('linux', URL_WITH_AMP)).toEqual({ + command: 'xdg-open', + args: [URL_WITH_AMP], + }); + }); + + it('uses cmd.exe /c start "" "" on Windows', () => { + // The empty placeholder title and the single quoted URL argv element + // are what keep `&` from being treated as a statement separator. + expect(browserOpenSpec('win32', URL_WITH_AMP)).toEqual({ + command: 'cmd.exe', + args: ['/c', 'start', '', URL_WITH_AMP], + }); + }); +}); + +interface FakeChild extends EventEmitter { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + kill: ReturnType; +} + +function fakeChild(): FakeChild { + const child = new EventEmitter() as FakeChild; + child.exitCode = null; + child.signalCode = null; + child.kill = vi.fn(() => true); + return child; +} + +describe('openBrowser', () => { + it('spawns the platform command with the constructed argv', () => { + const child = fakeChild(); + const spawnFn = vi.fn(() => child) as never; + openBrowser(URL_WITH_AMP, { platform: 'linux', spawnFn }); + expect(spawnFn).toHaveBeenCalledTimes(1); + const [command, args] = (spawnFn as unknown as { mock: { calls: unknown[][] } }).mock + .calls[0]!; + expect(command).toBe('xdg-open'); + expect(args).toEqual([URL_WITH_AMP]); + }); + + it('kills the subprocess when the abort signal fires', () => { + const child = fakeChild(); + const spawnFn = vi.fn(() => child) as never; + const ac = new AbortController(); + openBrowser(URL_WITH_AMP, { platform: 'linux', spawnFn, signal: ac.signal }); + expect(child.kill).not.toHaveBeenCalled(); + ac.abort(); + expect(child.kill).toHaveBeenCalledTimes(1); + }); + + it('returns undefined when spawn throws synchronously', () => { + const spawnFn = vi.fn(() => { + throw new Error('ENOENT'); + }) as never; + const result = openBrowser(URL_WITH_AMP, { platform: 'linux', spawnFn }); + expect(result).toBeUndefined(); + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/browser.ts b/ts-packages/quarto-hub-mcp/src/auth/browser.ts new file mode 100644 index 00000000..1a90b12c --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/browser.ts @@ -0,0 +1,95 @@ +/** + * Cross-platform browser launcher for the loopback auth flow. + * + * The launcher is best-effort and never changes the control flow: the + * loopback listener is bound *before* the browser is opened and the + * `authenticate` tool blocks on the listener regardless of whether the + * launch succeeded. On failure the user can still paste the surfaced + * authorization URL into a browser themselves. + */ + +import { spawn, type ChildProcess } from 'node:child_process'; + +export interface BrowserOpenSpec { + readonly command: string; + readonly args: readonly string[]; +} + +/** + * Pure argv construction — exposed for tests. The Windows form is the + * fiddly one and the reason this is a separate function: + * + * `cmd.exe /c start "" ""` + * + * Two gotchas the naive `start ` form gets wrong: + * 1. `cmd.exe` treats `&` as a statement separator and OAuth URLs are + * dense with `&`; quoting the URL prevents that interpretation. + * 2. `start`'s first *quoted* argument is the window title, not the + * URL. The empty `""` is a placeholder title so the quoted URL is + * parsed as the target. + */ +export function browserOpenSpec(platform: NodeJS.Platform, url: string): BrowserOpenSpec { + switch (platform) { + case 'darwin': + return { command: 'open', args: [url] }; + case 'win32': + return { command: 'cmd.exe', args: ['/c', 'start', '', url] }; + default: + // Linux and other Unix-likes. + return { command: 'xdg-open', args: [url] }; + } +} + +export interface OpenBrowserOptions { + readonly platform?: NodeJS.Platform; + readonly signal?: AbortSignal; + /** Injection seam for tests. */ + readonly spawnFn?: typeof spawn; +} + +/** + * Launch the system browser at `url`. Returns the spawned child (so the + * caller can observe / kill it) or `undefined` if the spawn itself + * threw synchronously. Errors are intentionally swallowed — the caller + * relies on the listener, not the launcher, for completion. + * + * When an `AbortSignal` is supplied, the child is killed on abort so a + * host-issued cancellation tears down the browser subprocess instead of + * leaving it alive until the deadline. + */ +export function openBrowser(url: string, opts: OpenBrowserOptions = {}): ChildProcess | undefined { + const platform = opts.platform ?? process.platform; + const spawnFn = opts.spawnFn ?? spawn; + const { command, args } = browserOpenSpec(platform, url); + let child: ChildProcess; + try { + child = spawnFn(command, args as string[], { + stdio: 'ignore', + // Single argv element per arg, no shell — the URL is never passed + // through a shell that could re-interpret its metacharacters. + windowsVerbatimArguments: false, + }); + } catch { + return undefined; + } + // A spawn error (e.g. ENOENT for a missing `xdg-open`) arrives async. + // Swallow it: the listener-failure / timeout response text is the + // user-facing fallback, not a thrown error here. + child.on('error', () => undefined); + + if (opts.signal) { + const onAbort = (): void => { + if (child.exitCode === null && child.signalCode === null) { + child.kill(); + } + }; + if (opts.signal.aborted) { + onAbort(); + } else { + opts.signal.addEventListener('abort', onAbort, { once: true }); + child.once('exit', () => opts.signal?.removeEventListener('abort', onAbort)); + } + } + + return child; +} diff --git a/ts-packages/quarto-hub-mcp/src/auth/credential-store.test.ts b/ts-packages/quarto-hub-mcp/src/auth/credential-store.test.ts index c03fa8df..235b80cb 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/credential-store.test.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/credential-store.test.ts @@ -33,6 +33,19 @@ const SAMPLE: CredentialBundle = { scopes: ['openid', 'email', 'profile'], }; +/** The canonical on-disk blob for {@link SAMPLE}. */ +function serializeSample(): string { + return JSON.stringify({ + schema_version: 1, + issuer: ISSUER, + client_id: CLIENT_ID, + id_token: SAMPLE.idToken, + refresh_token: SAMPLE.refreshToken, + id_token_expires_at: SAMPLE.idTokenExpiresAt.toISOString(), + scopes: SAMPLE.scopes, + }); +} + // --------------------------------------------------------------------------- // In-memory backends // --------------------------------------------------------------------------- @@ -246,6 +259,70 @@ describe('CredentialStore.write / read round-trip', () => { }); }); +// --------------------------------------------------------------------------- +// In-memory caching (skips OS-keyring IPC on the warm path) +// --------------------------------------------------------------------------- + +describe('CredentialStore caching', () => { + it('serves a second read from the cache without re-hitting the backend', async () => { + const blob = serializeSample(); + const { backend, log } = memoryBackend(blob); + const store = new CredentialStore(CFG, backend); + const first = await store.read(); + const second = await store.read(); + expect(first).not.toBeNull(); + expect(second!.idToken).toBe(SAMPLE.idToken); + expect(log.reads).toBe(1); + }); + + it('caches a definitive miss so a second read does not re-hit the backend', async () => { + const { backend, log } = memoryBackend(null); + const store = new CredentialStore(CFG, backend); + expect(await store.read()).toBeNull(); + expect(await store.read()).toBeNull(); + expect(log.reads).toBe(1); + }); + + it('populates the cache on write so a subsequent read skips the backend', async () => { + const { backend, log } = memoryBackend(); + const store = new CredentialStore(CFG, backend); + await store.write(SAMPLE); + const got = await store.read(); + expect(got!.refreshToken).toBe(SAMPLE.refreshToken); + expect(log.reads).toBe(0); + }); + + it('invalidates the cache on clear so a subsequent read returns null without the backend', async () => { + const { backend, log } = memoryBackend(serializeSample()); + const store = new CredentialStore(CFG, backend); + await store.read(); // warm the cache (reads === 1) + await store.clear(); + expect(await store.read()).toBeNull(); + expect(log.reads).toBe(1); + }); + + it('does not cache a transient read failure — the next read retries the backend', async () => { + let calls = 0; + const blob = serializeSample(); + const backend: KeyringBackend = { + async read() { + calls += 1; + if (calls === 1) throw new Error('transient libsecret hiccup'); + return blob; + }, + async write() {}, + async clear() { + return false; + }, + }; + const store = new CredentialStore(CFG, backend); + expect(await store.read()).toBeNull(); // transient failure → null, uncached + const recovered = await store.read(); // retries, succeeds + expect(recovered!.idToken).toBe(SAMPLE.idToken); + expect(calls).toBe(2); + }); +}); + // --------------------------------------------------------------------------- // Concurrency // --------------------------------------------------------------------------- diff --git a/ts-packages/quarto-hub-mcp/src/auth/credential-store.ts b/ts-packages/quarto-hub-mcp/src/auth/credential-store.ts index 06e8cd13..00c13a33 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/credential-store.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/credential-store.ts @@ -16,7 +16,7 @@ import { AsyncEntry } from '@napi-rs/keyring'; -import { redactTokens } from './device-flow.js'; +import { redactTokens } from './redact.js'; // --------------------------------------------------------------------------- // Public types @@ -141,6 +141,14 @@ export class CredentialStore { // Tail of the in-process mutex chain. Every operation chains onto // this promise so reads and writes serialise in submission order. private tail: Promise = Promise.resolve(); + // In-memory memo of the last *definitive* keyring state, so repeated + // reads (every getBearer / connect probe) skip the OS-IPC round-trip. + // `undefined` = unknown (not yet observed); `{ value }` = known. Only + // definitive outcomes populate it — a successful read, a successful + // write, or a clear. A transient read failure leaves it untouched so + // the next read retries the backend. This process is the only writer + // of its keyring entry, so a cached value cannot go stale underneath us. + private cache: { value: CredentialBundle | null } | undefined; constructor(cfg: CredentialStoreConfig, backend?: KeyringBackend) { this.cfg = cfg; @@ -150,17 +158,23 @@ export class CredentialStore { async read(): Promise { return await this.enqueue(async () => { + // Cache check runs inside the mutex chain so a read submitted after + // a write still observes that write (ordering contract preserved); + // we only skip the keyring IPC, not the serialisation. + if (this.cache !== undefined) return this.cache.value; let raw: string | null; try { raw = await this.backend.read(); } catch (err) { // Read is never fatal — try-without-creds-first depends on it. + // Don't cache a transient failure: the next read retries. const msg = redactTokens(errMessage(err)); console.warn(`CredentialStore: keyring read failed (${msg})`); return null; } - if (raw === null) return null; - return parseBundle(raw); + const value = raw === null ? null : parseBundle(raw); + this.cache = { value }; + return value; }); } @@ -174,6 +188,7 @@ export class CredentialStore { `Failed to write credentials to OS keyring (service '${SERVICE_NAME}'): ${redactTokens(errMessage(err))}` ); } + this.cache = { value: bundle }; }); } @@ -186,6 +201,7 @@ export class CredentialStore { `Failed to clear credentials from OS keyring (service '${SERVICE_NAME}'): ${redactTokens(errMessage(err))}` ); } + this.cache = { value: null }; }); } diff --git a/ts-packages/quarto-hub-mcp/src/auth/device-flow.test.ts b/ts-packages/quarto-hub-mcp/src/auth/device-flow.test.ts deleted file mode 100644 index 4deb0d4f..00000000 --- a/ts-packages/quarto-hub-mcp/src/auth/device-flow.test.ts +++ /dev/null @@ -1,535 +0,0 @@ -/** - * Phase 4 — device-flow primitives. - * - * Tests use a stub `customFetch` injected via `oauth4webapi`'s - * symbol-keyed options; never call live Google. - */ - -import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { readFileSync, readdirSync, statSync } from 'node:fs'; -import { join } from 'node:path'; -import * as oauth from 'oauth4webapi'; - -import { - DeviceFlowDeniedError, - DeviceFlowError, - DeviceFlowExpiredError, - MissingCredentialsConfigError, - buildAuthorizationServer, - initiateDeviceFlow, - loadDeviceFlowConfigFromEnv, - pollDeviceFlowOnce, - redactTokens, -} from './device-flow.js'; - -const ISSUER = 'https://accounts.google.com'; -const FAKE_CLIENT_ID = 'test-client.apps.googleusercontent.com'; -const FAKE_CLIENT_SECRET = 'GOCSPX-test-secret'; - -// Synthetic AuthorizationServer (skips discovery): the spec requires -// only that the endpoints we hit are present, but oauth4webapi also -// verifies `issuer` matches expectations. -const AS: oauth.AuthorizationServer = { - issuer: ISSUER, - device_authorization_endpoint: 'https://oauth2.googleapis.com/device/code', - token_endpoint: 'https://oauth2.googleapis.com/token', -}; - -interface RecordedRequest { - url: string; - method: string; - headers: Record; - body: URLSearchParams; -} - -function makeFetch( - responder: (req: RecordedRequest) => Response | Promise -): { - fetch: typeof fetch; - requests: RecordedRequest[]; -} { - const requests: RecordedRequest[] = []; - const stub: typeof fetch = async (input, init) => { - const url = typeof input === 'string' ? input : (input as URL | Request).toString(); - const headersIn = new Headers(init?.headers); - const headers: Record = {}; - headersIn.forEach((v, k) => { - headers[k] = v; - }); - let body: URLSearchParams; - if (init?.body instanceof URLSearchParams) { - body = new URLSearchParams(init.body.toString()); - } else if (typeof init?.body === 'string') { - body = new URLSearchParams(init.body); - } else { - body = new URLSearchParams(); - } - const req: RecordedRequest = { - url, - method: init?.method ?? 'GET', - headers, - body, - }; - requests.push(req); - return responder(req); - }; - return { fetch: stub, requests }; -} - -function b64url(s: string): string { - return Buffer.from(s, 'utf8').toString('base64url'); -} - -/** - * Builds a structurally-valid (unsigned) JWT for fixture use. - * oauth4webapi validates the header+payload are base64url-encoded JSON - * and that the algorithm matches the expected `none` or signed form; - * for token-endpoint responses the signature is not verified against - * any key, only the shape. - */ -function fakeIdToken(payload: Record): string { - const header = b64url(JSON.stringify({ alg: 'RS256', typ: 'JWT', kid: 'fake' })); - const body = b64url(JSON.stringify(payload)); - // Any non-empty base64url string works as a placeholder signature. - const sig = b64url('signature-bytes'); - return `${header}.${body}.${sig}`; -} - -function jsonResponse(status: number, body: unknown): Response { - return new Response(JSON.stringify(body), { - status, - headers: { 'content-type': 'application/json' }, - }); -} - -function envOnly(values: Record): NodeJS.ProcessEnv { - const e: NodeJS.ProcessEnv = {}; - for (const [k, v] of Object.entries(values)) { - if (v !== undefined) e[k] = v; - } - return e; -} - -describe('loadDeviceFlowConfigFromEnv', () => { - it('reads client_id and client_secret from process.env', () => { - const env = envOnly({ - QUARTO_HUB_MCP_CLIENT_ID: FAKE_CLIENT_ID, - QUARTO_HUB_MCP_CLIENT_SECRET: FAKE_CLIENT_SECRET, - }); - const cfg = loadDeviceFlowConfigFromEnv(env); - expect(cfg.clientId).toBe(FAKE_CLIENT_ID); - expect(cfg.clientSecret).toBe(FAKE_CLIENT_SECRET); - }); - - it('throws MissingCredentialsConfigError naming both env vars when neither set', () => { - const env = envOnly({}); - let err: unknown; - try { - loadDeviceFlowConfigFromEnv(env); - } catch (e) { - err = e; - } - expect(err).toBeInstanceOf(MissingCredentialsConfigError); - const msg = (err as Error).message; - expect(msg).toContain('QUARTO_HUB_MCP_CLIENT_ID'); - expect(msg).toContain('QUARTO_HUB_MCP_CLIENT_SECRET'); - }); - - it('throws MissingCredentialsConfigError when only client_id set', () => { - const env = envOnly({ QUARTO_HUB_MCP_CLIENT_ID: FAKE_CLIENT_ID }); - let err: unknown; - try { - loadDeviceFlowConfigFromEnv(env); - } catch (e) { - err = e; - } - expect(err).toBeInstanceOf(MissingCredentialsConfigError); - expect((err as Error).message).toContain('QUARTO_HUB_MCP_CLIENT_SECRET'); - }); - - it('throws MissingCredentialsConfigError when secret is empty string', () => { - const env = envOnly({ - QUARTO_HUB_MCP_CLIENT_ID: FAKE_CLIENT_ID, - QUARTO_HUB_MCP_CLIENT_SECRET: ' ', - }); - expect(() => loadDeviceFlowConfigFromEnv(env)).toThrow( - MissingCredentialsConfigError - ); - }); -}); - -describe('no_baked_default_client_id_or_secret', () => { - // Sourcing rule lock-in: scan every non-test file under src/ for - // hard-coded Google OAuth credentials. Test files are allowed to - // carry fixture-shaped strings. - it('contains no apps.googleusercontent.com literal in any source file', () => { - const root = join(__dirname, '..'); - const violations: string[] = []; - function walk(dir: string): void { - for (const ent of readdirSync(dir)) { - const p = join(dir, ent); - const st = statSync(p); - if (st.isDirectory()) { - walk(p); - continue; - } - if (!p.endsWith('.ts') || p.endsWith('.test.ts')) continue; - const text = readFileSync(p, 'utf8'); - if (/[A-Za-z0-9_-]+\.apps\.googleusercontent\.com/.test(text)) { - violations.push(`${p}: apps.googleusercontent.com literal`); - } - if (/GOCSPX-[A-Za-z0-9_-]+/.test(text)) { - violations.push(`${p}: GOCSPX- literal`); - } - } - } - walk(root); - expect(violations).toEqual([]); - }); -}); - -describe('initiateDeviceFlow', () => { - let okResponse: oauth.DeviceAuthorizationResponse; - - beforeEach(() => { - okResponse = { - device_code: 'AH-1Ng-test', - user_code: 'FJZL-WTDR', - verification_uri: 'https://www.google.com/device', - expires_in: 1800, - interval: 5, - }; - }); - - it('posts to device_authorization_endpoint with client_id and scope', async () => { - const { fetch, requests } = makeFetch(() => jsonResponse(200, okResponse)); - await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - expect(requests).toHaveLength(1); - const req = requests[0]!; - expect(req.url).toBe('https://oauth2.googleapis.com/device/code'); - expect(req.method).toBe('POST'); - expect(req.body.get('client_id')).toBe(FAKE_CLIENT_ID); - expect(req.body.get('scope')).toBe('openid email profile'); - }); - - it('does not send client_secret on the device-auth body', async () => { - const { fetch, requests } = makeFetch(() => jsonResponse(200, okResponse)); - await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - expect(requests[0]!.body.has('client_secret')).toBe(false); - }); - - it('returns the full DeviceAuthorizationResponse', async () => { - const { fetch } = makeFetch(() => jsonResponse(200, okResponse)); - const got = await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - expect(got.device_code).toBe('AH-1Ng-test'); - expect(got.user_code).toBe('FJZL-WTDR'); - expect(got.verification_uri).toBe('https://www.google.com/device'); - expect(got.expires_in).toBe(1800); - expect(got.interval).toBe(5); - }); - - it('does not log the user_code or device_code', async () => { - const debug = vi.spyOn(console, 'debug').mockImplementation(() => undefined); - const log = vi.spyOn(console, 'log').mockImplementation(() => undefined); - const info = vi.spyOn(console, 'info').mockImplementation(() => undefined); - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); - const err = vi.spyOn(console, 'error').mockImplementation(() => undefined); - try { - const { fetch } = makeFetch(() => jsonResponse(200, okResponse)); - await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - const calls = [debug, log, info, warn, err] - .flatMap((s) => s.mock.calls.flat()) - .map((c) => (typeof c === 'string' ? c : JSON.stringify(c))); - const blob = calls.join(' '); - expect(blob).not.toContain(okResponse.user_code); - expect(blob).not.toContain(okResponse.device_code); - } finally { - debug.mockRestore(); - log.mockRestore(); - info.mockRestore(); - warn.mockRestore(); - err.mockRestore(); - } - }); - - it('normalises Google\'s verification_url to RFC 8628 verification_uri', async () => { - // Live Google /device/code returns `verification_url` (with `_url`, - // not the RFC 8628 `verification_uri`). `oauth4webapi`'s - // `processDeviceAuthorizationResponse` strictly asserts - // `verification_uri` is a string and throws otherwise — so without - // a normaliser, every live `authenticate_start` fails before the - // device flow can begin. The Phase 0 verification log (2026-05-19) - // captured Google's exact shape; this test pins it. - const googleBody = { - device_code: 'AH-1Ng-test', - user_code: 'FJZL-WTDR', - verification_url: 'https://www.google.com/device', - expires_in: 1800, - interval: 5, - }; - const { fetch } = makeFetch(() => jsonResponse(200, googleBody)); - const got = await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - expect(got.verification_uri).toBe('https://www.google.com/device'); - expect(got.device_code).toBe('AH-1Ng-test'); - expect(got.user_code).toBe('FJZL-WTDR'); - }); - - it('preserves verification_uri when both fields are present', async () => { - // Defensive: if Google ever updates to send both spellings (or only - // the RFC one), we must not clobber the canonical field. - const both = { - device_code: 'AH-1Ng-test', - user_code: 'FJZL-WTDR', - verification_uri: 'https://www.google.com/device', - verification_url: 'https://attacker.example/device', - expires_in: 1800, - interval: 5, - }; - const { fetch } = makeFetch(() => jsonResponse(200, both)); - const got = await initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch } - ); - expect(got.verification_uri).toBe('https://www.google.com/device'); - }); - - it('honours abort signal', async () => { - const ctl = new AbortController(); - const { fetch } = makeFetch(async () => { - // never resolve unless aborted - await new Promise((_, reject) => { - ctl.signal.addEventListener('abort', () => { - reject(new DOMException('aborted', 'AbortError')); - }); - }); - return jsonResponse(200, okResponse); - }); - const p = initiateDeviceFlow( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET, scopes: ['openid', 'email', 'profile'] }, - { fetch, signal: ctl.signal } - ); - setTimeout(() => ctl.abort(), 5); - await expect(p).rejects.toThrow(); - }); -}); - -describe('pollDeviceFlowOnce', () => { - const ID_TOKEN = fakeIdToken({ - iss: ISSUER, - aud: FAKE_CLIENT_ID, - azp: FAKE_CLIENT_ID, - sub: 'fake-sub-123', - email: 'tester@example.com', - email_verified: true, - iat: 1_000_000_000, - exp: 9_999_999_999, - }); - const SUCCESS_BODY = { - access_token: 'ya29.fake-access-token', - expires_in: 3599, - refresh_token: '1//fake-refresh-token', - scope: 'openid email profile', - token_type: 'Bearer', - id_token: ID_TOKEN, - }; - - it('returns pending on authorization_pending', async () => { - const { fetch } = makeFetch(() => - jsonResponse(400, { error: 'authorization_pending' }) - ); - const res = await pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ); - expect(res.kind).toBe('pending'); - }); - - it('returns slow_down on slow_down', async () => { - const { fetch } = makeFetch(() => - jsonResponse(400, { error: 'slow_down' }) - ); - const res = await pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ); - expect(res.kind).toBe('slow_down'); - }); - - it('resolves with tokens on success', async () => { - const { fetch, requests } = makeFetch(() => jsonResponse(200, SUCCESS_BODY)); - const res = await pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ); - expect(res.kind).toBe('tokens'); - if (res.kind !== 'tokens') throw new Error('unreachable'); - expect(res.bundle.id_token).toBe(ID_TOKEN); - expect(res.bundle.refresh_token).toBe(SUCCESS_BODY.refresh_token); - expect(res.bundle.access_token).toBe(SUCCESS_BODY.access_token); - // The token endpoint must include client_secret per Google's contract. - expect(requests[0]!.body.get('client_secret')).toBe(FAKE_CLIENT_SECRET); - expect(requests[0]!.body.get('client_id')).toBe(FAKE_CLIENT_ID); - expect(requests[0]!.body.get('device_code')).toBe('AH-1Ng-test'); - expect(requests[0]!.body.get('grant_type')).toBe( - 'urn:ietf:params:oauth:grant-type:device_code' - ); - }); - - it('throws DeviceFlowDeniedError on access_denied', async () => { - const { fetch } = makeFetch(() => - jsonResponse(400, { error: 'access_denied' }) - ); - await expect( - pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ) - ).rejects.toBeInstanceOf(DeviceFlowDeniedError); - }); - - it('throws DeviceFlowExpiredError on expired_token', async () => { - const { fetch } = makeFetch(() => - jsonResponse(400, { error: 'expired_token' }) - ); - await expect( - pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ) - ).rejects.toBeInstanceOf(DeviceFlowExpiredError); - }); - - it('throws generic DeviceFlowError on other oauth errors', async () => { - const { fetch } = makeFetch(() => - jsonResponse(400, { error: 'invalid_grant' }) - ); - const p = pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ); - await expect(p).rejects.toBeInstanceOf(DeviceFlowError); - await expect(p).rejects.not.toBeInstanceOf(DeviceFlowDeniedError); - await expect(p).rejects.not.toBeInstanceOf(DeviceFlowExpiredError); - }); - - it('honours abort signal', async () => { - const ctl = new AbortController(); - const { fetch } = makeFetch(async () => { - await new Promise((_, reject) => { - ctl.signal.addEventListener('abort', () => { - reject(new DOMException('aborted', 'AbortError')); - }); - }); - return jsonResponse(200, SUCCESS_BODY); - }); - const p = pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch, signal: ctl.signal } - ); - setTimeout(() => ctl.abort(), 5); - await expect(p).rejects.toThrow(); - }); - - it('does not log id_token or refresh_token', async () => { - const sinks = (['debug', 'log', 'info', 'warn', 'error'] as const).map((m) => - vi.spyOn(console, m).mockImplementation(() => undefined) - ); - try { - const { fetch } = makeFetch(() => jsonResponse(200, SUCCESS_BODY)); - await pollDeviceFlowOnce( - AS, - { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, - 'AH-1Ng-test', - { fetch } - ); - const blob = sinks - .flatMap((s) => s.mock.calls.flat()) - .map((c) => (typeof c === 'string' ? c : JSON.stringify(c))) - .join(' '); - expect(blob).not.toContain(ID_TOKEN); - expect(blob).not.toContain(SUCCESS_BODY.refresh_token); - expect(blob).not.toContain(SUCCESS_BODY.access_token); - } finally { - sinks.forEach((s) => s.mockRestore()); - } - }); -}); - -describe('redactTokens', () => { - it('redacts Google access tokens (ya29.*)', () => { - const s = 'token=ya29.aBcDeF_-1234567890XYZ end'; - expect(redactTokens(s)).not.toContain('ya29.aBcDeF'); - expect(redactTokens(s)).toContain('[redacted-token]'); - }); - - it('redacts Google refresh tokens (1//*)', () => { - const s = 'rt=1//0abcDEF-1234_xyz end'; - expect(redactTokens(s)).not.toContain('1//0abcDEF'); - expect(redactTokens(s)).toContain('[redacted-token]'); - }); - - it('redacts JWT-shaped substrings (xxx.yyy.zzz)', () => { - const jwt = - 'eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ4In0.signature_bytes_here_AB12'; - expect(redactTokens(`auth: ${jwt} end`)).not.toContain(jwt); - expect(redactTokens(`auth: ${jwt} end`)).toContain('[redacted-token]'); - }); - - it('passes through strings with no token shapes', () => { - expect(redactTokens('hello world')).toBe('hello world'); - }); -}); - -describe('buildAuthorizationServer', () => { - // Sanity: the helper that synthesises (or caches) an AuthorizationServer - // returns a usable object for the local-fetch path. Discovery against - // live Google is not exercised here. - it('returns an AuthorizationServer with the requested issuer', () => { - const as = buildAuthorizationServer({ - issuer: ISSUER, - device_authorization_endpoint: AS.device_authorization_endpoint!, - token_endpoint: AS.token_endpoint!, - }); - expect(as.issuer).toBe(ISSUER); - expect(as.device_authorization_endpoint).toBe( - 'https://oauth2.googleapis.com/device/code' - ); - expect(as.token_endpoint).toBe('https://oauth2.googleapis.com/token'); - }); -}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/device-flow.ts b/ts-packages/quarto-hub-mcp/src/auth/device-flow.ts deleted file mode 100644 index 110e8491..00000000 --- a/ts-packages/quarto-hub-mcp/src/auth/device-flow.ts +++ /dev/null @@ -1,261 +0,0 @@ -/** - * Phase 4 — Google device-flow primitives for quarto-hub-mcp. - * - * Exposes two single-purpose async helpers — `initiateDeviceFlow` and - * `pollDeviceFlowOnce` — built on top of `oauth4webapi`. The polling - * is **one shot per call**; the MCP tool surface in Phase 7 drives the - * retry cadence. All log call sites must funnel through `redactTokens`. - */ - -import * as oauth from 'oauth4webapi'; - -// --------------------------------------------------------------------------- -// Typed errors -// --------------------------------------------------------------------------- - -export class MissingCredentialsConfigError extends Error { - override readonly name = 'MissingCredentialsConfigError'; - constructor(message: string) { - super(message); - } -} - -export class DeviceFlowError extends Error { - override readonly name: string = 'DeviceFlowError'; - readonly oauthError: string; - constructor(cause: oauth.ResponseBodyError) { - super(`${cause.error}: ${redactTokens(cause.message)}`); - this.oauthError = cause.error; - this.cause = cause; - } -} - -export class DeviceFlowDeniedError extends DeviceFlowError { - override readonly name = 'DeviceFlowDeniedError'; -} - -export class DeviceFlowExpiredError extends DeviceFlowError { - override readonly name = 'DeviceFlowExpiredError'; -} - -// --------------------------------------------------------------------------- -// Redaction -// --------------------------------------------------------------------------- - -// Single alternation so we scan the input once per call. Branches: -// ya29\.... — Google access tokens -// 1\/\/... — Google refresh tokens -// eyJ....\....\.... — JWT-shaped (three base64url segments) -const TOKEN_PATTERN = - /ya29\.[A-Za-z0-9_-]+|1\/\/[A-Za-z0-9_-]+|eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+/g; - -export function redactTokens(s: string): string { - return s.replace(TOKEN_PATTERN, '[redacted-token]'); -} - -// --------------------------------------------------------------------------- -// Configuration sourcing -// --------------------------------------------------------------------------- - -export interface DeviceFlowEnvConfig { - readonly clientId: string; - readonly clientSecret: string; -} - -const CLIENT_ID_VAR = 'QUARTO_HUB_MCP_CLIENT_ID'; -const CLIENT_SECRET_VAR = 'QUARTO_HUB_MCP_CLIENT_SECRET'; - -function readNonEmpty(env: NodeJS.ProcessEnv, name: string): string | undefined { - const v = env[name]; - if (v === undefined) return undefined; - return v.trim() === '' ? undefined : v; -} - -export function loadDeviceFlowConfigFromEnv( - env: NodeJS.ProcessEnv = process.env -): DeviceFlowEnvConfig { - const clientId = readNonEmpty(env, CLIENT_ID_VAR); - const clientSecret = readNonEmpty(env, CLIENT_SECRET_VAR); - const missing: string[] = []; - if (clientId === undefined) missing.push(CLIENT_ID_VAR); - if (clientSecret === undefined) missing.push(CLIENT_SECRET_VAR); - if (missing.length > 0) { - throw new MissingCredentialsConfigError( - `${missing.join(' and ')} ${missing.length === 1 ? 'is' : 'are'} not set. ` + - `Hub-mcp requires ${CLIENT_ID_VAR} and ${CLIENT_SECRET_VAR} in the ` + - `MCP-client env. Ask your hub operator for the Google OAuth client ` + - `credentials they registered for hub-mcp.` - ); - } - return { clientId: clientId!, clientSecret: clientSecret! }; -} - -// --------------------------------------------------------------------------- -// AuthorizationServer (discovery cache + synthesis) -// --------------------------------------------------------------------------- - -let cachedAS: { readonly issuer: string; readonly as: oauth.AuthorizationServer } | undefined; - -export interface AuthorizationServerSpec { - readonly issuer: string; - readonly device_authorization_endpoint: string; - readonly token_endpoint: string; -} - -export function buildAuthorizationServer( - spec: AuthorizationServerSpec -): oauth.AuthorizationServer { - return { - issuer: spec.issuer, - device_authorization_endpoint: spec.device_authorization_endpoint, - token_endpoint: spec.token_endpoint, - }; -} - -export async function discoverAuthorizationServer( - issuer: string, - opts?: { fetch?: typeof fetch } -): Promise { - if (cachedAS && cachedAS.issuer === issuer) return cachedAS.as; - const url = new URL(issuer); - const requestOpts = opts?.fetch ? { [oauth.customFetch]: opts.fetch } : undefined; - const resp = await oauth.discoveryRequest(url, requestOpts); - const as = await oauth.processDiscoveryResponse(url, resp); - cachedAS = { issuer, as }; - return as; -} - -/** Test hook — reset the in-process discovery cache. */ -export function _resetDiscoveryCache(): void { - cachedAS = undefined; -} - -// --------------------------------------------------------------------------- -// Device flow -// --------------------------------------------------------------------------- - -export interface DeviceFlowClientConfig { - readonly clientId: string; - readonly clientSecret: string; - readonly scopes: readonly string[]; -} - -export interface DeviceFlowRequestOptions { - readonly fetch?: typeof fetch; - readonly signal?: AbortSignal; -} - -function customFetchOpts(opts?: DeviceFlowRequestOptions): { - [k: symbol]: typeof fetch; - signal?: AbortSignal; -} | undefined { - if (!opts) return undefined; - const out: { [k: symbol]: typeof fetch; signal?: AbortSignal } = {} as never; - if (opts.fetch) out[oauth.customFetch] = opts.fetch; - if (opts.signal) out.signal = opts.signal; - return Object.keys(out).length === 0 && Object.getOwnPropertySymbols(out).length === 0 - ? undefined - : out; -} - -/** - * Initiates an RFC 8628 device-authorization flow. The device-auth body - * carries `client_id` and `scope` only — `client_secret` is reserved - * for the token endpoint to minimise secret-exposure surface. - */ -export async function initiateDeviceFlow( - as: oauth.AuthorizationServer, - config: DeviceFlowClientConfig, - opts?: DeviceFlowRequestOptions -): Promise { - const client: oauth.Client = { client_id: config.clientId }; - // None() — device-auth doesn't require client authentication on - // Google's endpoint; carrying the secret here would expand the - // surface where the secret travels. - const noAuth = oauth.None(); - const params = new URLSearchParams({ scope: config.scopes.join(' ') }); - const requestOpts = customFetchOpts(opts); - const resp = requestOpts - ? await oauth.deviceAuthorizationRequest(as, client, noAuth, params, requestOpts) - : await oauth.deviceAuthorizationRequest(as, client, noAuth, params); - // Google's /device/code endpoint returns `verification_url` instead - // of the RFC 8628 `verification_uri` (empirically confirmed - // 2026-05-19; see Phase 0 verification log). `oauth4webapi` strictly - // asserts `verification_uri`, so we rewrite the JSON body before - // handing it off. Only copy when the canonical field is absent so - // we never overwrite an upstream-supplied value. - const normalised = await normaliseDeviceAuthResponse(resp); - return await oauth.processDeviceAuthorizationResponse(as, client, normalised); -} - -async function normaliseDeviceAuthResponse(resp: Response): Promise { - // Non-2xx responses are passed through unchanged so oauth4webapi's - // error-mapping path (which inspects `error` / `error_description`) - // is left intact. - if (!resp.ok) return resp; - const ct = resp.headers.get('content-type') ?? ''; - if (!ct.includes('json')) return resp; - let json: Record; - try { - json = (await resp.clone().json()) as Record; - } catch { - return resp; - } - if ( - typeof json['verification_uri'] !== 'string' && - typeof json['verification_url'] === 'string' - ) { - json['verification_uri'] = json['verification_url']; - } - return new Response(JSON.stringify(json), { - status: resp.status, - statusText: resp.statusText, - headers: resp.headers, - }); -} - -export type PollResult = - | { readonly kind: 'pending' } - | { readonly kind: 'slow_down' } - | { readonly kind: 'tokens'; readonly bundle: oauth.TokenEndpointResponse }; - -/** - * Performs exactly **one** poll of the token endpoint for the given - * device_code. `authorization_pending` / `slow_down` are returned as - * discriminated-union values (never thrown). Terminal failures - * (`access_denied`, `expired_token`) throw typed errors so callers - * can clear the cached device_code and surface the right tool error. - */ -export async function pollDeviceFlowOnce( - as: oauth.AuthorizationServer, - config: Pick, - deviceCode: string, - opts?: DeviceFlowRequestOptions -): Promise { - const client: oauth.Client = { client_id: config.clientId }; - const clientAuth: oauth.ClientAuth = oauth.ClientSecretPost(config.clientSecret); - const requestOpts = customFetchOpts(opts); - try { - const resp = requestOpts - ? await oauth.deviceCodeGrantRequest(as, client, clientAuth, deviceCode, requestOpts) - : await oauth.deviceCodeGrantRequest(as, client, clientAuth, deviceCode); - const tokens = await oauth.processDeviceCodeResponse(as, client, resp); - return { kind: 'tokens', bundle: tokens }; - } catch (err) { - if (err instanceof oauth.ResponseBodyError) { - switch (err.error) { - case 'authorization_pending': - return { kind: 'pending' }; - case 'slow_down': - return { kind: 'slow_down' }; - case 'access_denied': - throw new DeviceFlowDeniedError(err); - case 'expired_token': - throw new DeviceFlowExpiredError(err); - default: - throw new DeviceFlowError(err); - } - } - throw err; - } -} diff --git a/ts-packages/quarto-hub-mcp/src/auth/loopback.test.ts b/ts-packages/quarto-hub-mcp/src/auth/loopback.test.ts new file mode 100644 index 00000000..8dd42b7b --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/loopback.test.ts @@ -0,0 +1,155 @@ +/** + * Loopback redirect listener. + * + * Drives the real `http.Server` over `127.0.0.1` with `node:http` + * requests (so the `Host` header can be forged for the DNS-rebinding + * test, which `fetch` forbids). + */ + +import * as http from 'node:http'; +import { describe, it, expect } from 'vitest'; + +import { + LoopbackAbortedError, + LoopbackAuthorizationError, + LoopbackStateMismatchError, + LoopbackTimeoutError, + startLoopbackListener, +} from './loopback.js'; + +interface RawResponse { + status: number; + headers: http.IncomingHttpHeaders; + body: string; +} + +function httpGet( + port: number, + pathQuery: string, + headers: http.OutgoingHttpHeaders = {}, +): Promise { + return new Promise((resolve, reject) => { + const req = http.request( + { host: '127.0.0.1', port, path: pathQuery, method: 'GET', headers }, + (res) => { + let body = ''; + res.setEncoding('utf8'); + res.on('data', (c) => (body += c)); + res.on('end', () => + resolve({ status: res.statusCode ?? 0, headers: res.headers, body }), + ); + }, + ); + req.on('error', reject); + req.end(); + }); +} + +function delay(ms: number): Promise<'pending'> { + return new Promise((r) => setTimeout(() => r('pending'), ms)); +} + +describe('startLoopbackListener', () => { + it('resolves with the code and serves the security-headed success page', async () => { + const listener = await startLoopbackListener({ expectedState: 'st-123' }); + expect(listener.redirectUri).toBe(`http://127.0.0.1:${listener.port}/callback`); + + const [resp, result] = await Promise.all([ + httpGet(listener.port, '/callback?code=abc&state=st-123'), + listener.result, + ]); + + expect(result.code).toBe('abc'); + expect(result.state).toBe('st-123'); + expect(resp.status).toBe(200); + expect(resp.headers['content-security-policy']).toContain("default-src 'none'"); + expect(resp.headers['cache-control']).toBe('no-store'); + expect(resp.headers['referrer-policy']).toBe('no-referrer'); + expect(resp.headers['x-content-type-options']).toBe('nosniff'); + // No JavaScript and no external resources in the page. + expect(resp.body).not.toContain(' { + const listener = await startLoopbackListener({ expectedState: 'good' }); + const respP = httpGet(listener.port, '/callback?code=abc&state=bad'); + await expect(listener.result).rejects.toBeInstanceOf(LoopbackStateMismatchError); + expect((await respP).status).toBe(400); + }); + + it('rejects a forged Host header (DNS-rebinding) before completing the flow', async () => { + const listener = await startLoopbackListener({ expectedState: 'st' }); + // Forged Host carries an otherwise-valid code+state; it must NOT be + // able to complete the flow — the Host check fires first. + const forged = await httpGet(listener.port, '/callback?code=abc&state=st', { + host: 'evil.example', + }); + expect(forged.status).toBe(400); + + // Listener is still bound: the result has not settled. + const race = await Promise.race([listener.result.then(() => 'settled'), delay(50)]); + expect(race).toBe('pending'); + + // A legitimate callback still completes it. + const [resp, result] = await Promise.all([ + httpGet(listener.port, '/callback?code=real&state=st'), + listener.result, + ]); + expect(result.code).toBe('real'); + expect(resp.status).toBe(200); + }); + + it('rejects with a typed authorization error on ?error=', async () => { + const listener = await startLoopbackListener({ expectedState: 'st' }); + const respP = httpGet(listener.port, '/callback?error=access_denied&state=st'); + const err = await listener.result.catch((e) => e); + expect(err).toBeInstanceOf(LoopbackAuthorizationError); + expect((err as LoopbackAuthorizationError).oauthError).toBe('access_denied'); + expect((await respP).status).toBe(200); + }); + + it('rejects when the callback carries neither code nor error', async () => { + const listener = await startLoopbackListener({ expectedState: 'st' }); + const respP = httpGet(listener.port, '/callback?state=st'); + await expect(listener.result).rejects.toBeInstanceOf(LoopbackAuthorizationError); + expect((await respP).status).toBe(400); + }); + + it('404s an unknown path without echoing it and stays bound', async () => { + const listener = await startLoopbackListener({ expectedState: 'st' }); + const resp = await httpGet(listener.port, '/secret-probe?x=1'); + expect(resp.status).toBe(404); + expect(resp.body).not.toContain('secret-probe'); + const race = await Promise.race([listener.result.then(() => 'settled'), delay(30)]); + expect(race).toBe('pending'); + listener.close(); + await listener.result.catch(() => undefined); + }); + + it('rejects with LoopbackTimeoutError after the deadline', async () => { + const listener = await startLoopbackListener({ expectedState: 'st', timeoutMs: 40 }); + await expect(listener.result).rejects.toBeInstanceOf(LoopbackTimeoutError); + }); + + it('rejects with LoopbackAbortedError when the signal fires', async () => { + const ac = new AbortController(); + const listener = await startLoopbackListener({ expectedState: 'st', signal: ac.signal }); + ac.abort(); + await expect(listener.result).rejects.toBeInstanceOf(LoopbackAbortedError); + }); + + it('tears down on SIGINT and unregisters its own signal handler', async () => { + const prior = process.listeners('SIGINT'); + process.removeAllListeners('SIGINT'); + try { + const listener = await startLoopbackListener({ expectedState: 'st' }); + const rejected = expect(listener.result).rejects.toBeInstanceOf(LoopbackAbortedError); + process.emit('SIGINT'); + await rejected; + expect(process.listeners('SIGINT').length).toBe(0); + } finally { + for (const l of prior) process.on('SIGINT', l as never); + } + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/loopback.ts b/ts-packages/quarto-hub-mcp/src/auth/loopback.ts new file mode 100644 index 00000000..1af2e8e8 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/loopback.ts @@ -0,0 +1,326 @@ +/** + * RFC 8252 loopback redirect listener for the Authorization Code + PKCE + * flow. + * + * Binds a one-shot HTTP listener on the literal `127.0.0.1` (never + * `localhost` — that resolves via DNS / hosts and has historically been + * hijackable; the IP literal is the RFC 8252 §7.3 fix). The chosen port + * flows verbatim into `redirect_uri` and must match byte-for-byte at the + * token-exchange step. + * + * Defences: + * - Host-header allowlist (RFC 8252 §8.4) rejects DNS-rebinding before + * any query parsing or state validation, so timing cannot leak + * whether a flow is in progress. + * - Constant-time `state` comparison (CSRF defence). + * - The callback response is a self-contained no-JavaScript HTML page + * with a locked-down CSP and no external resources. + * + * The listener settles exactly once — on the first valid callback, an + * authorization error from the IdP, a state/host violation that + * corresponds to our flow, the timeout, abort, or SIGINT/SIGTERM — then + * closes its server and unregisters its signal handlers so nothing + * outlives the flow. + */ + +import { timingSafeEqual } from 'node:crypto'; +import * as http from 'node:http'; +import type { AddressInfo } from 'node:net'; + +/** + * `authenticate` deadline. The whole single-tool design assumes the MCP + * host keeps the `tools/call` RPC alive this long while the user signs + * in (see the plan's Spike B). 5 minutes is the working assumption; + * lower it here if a host is found to enforce a shorter floor. + */ +export const AUTHENTICATE_TIMEOUT_MS = 5 * 60 * 1000; + +const LOOPBACK_HOST = '127.0.0.1'; +const CALLBACK_PATH = '/callback'; + +// --------------------------------------------------------------------------- +// Typed errors +// --------------------------------------------------------------------------- + +export class LoopbackTimeoutError extends Error { + override readonly name = 'LoopbackTimeoutError'; + constructor(message = 'Timed out waiting for the browser sign-in to complete.') { + super(message); + } +} + +export class LoopbackAbortedError extends Error { + override readonly name = 'LoopbackAbortedError'; + constructor(message = 'Sign-in was cancelled.') { + super(message); + } +} + +export class LoopbackStateMismatchError extends Error { + override readonly name = 'LoopbackStateMismatchError'; + constructor(message = 'Authorization callback state did not match; possible CSRF.') { + super(message); + } +} + +export class LoopbackAuthorizationError extends Error { + override readonly name = 'LoopbackAuthorizationError'; + readonly oauthError: string; + constructor(oauthError: string) { + super(`Authorization failed: ${oauthError}`); + this.oauthError = oauthError; + } +} + +// --------------------------------------------------------------------------- +// Public types +// --------------------------------------------------------------------------- + +export interface LoopbackResult { + readonly code: string; + readonly state: string; + /** + * The full callback query string. The handler re-validates it through + * `oauth4webapi.validateAuthResponse` (which also performs the RFC 9207 + * `iss` check and brands the params for `authorizationCodeGrantRequest`). + */ + readonly params: URLSearchParams; +} + +export interface LoopbackListener { + /** The kernel-picked (or requested) port the listener bound to. */ + readonly port: number; + /** `http://127.0.0.1:/callback` — use verbatim as `redirect_uri`. */ + readonly redirectUri: string; + /** Resolves with the callback params; rejects on error/timeout/abort. */ + readonly result: Promise; + /** Force teardown (idempotent). */ + close(): void; +} + +export interface StartLoopbackOptions { + /** The `state` value generated for this flow; compared constant-time. */ + readonly expectedState: string; + /** Bind port; `0` (default) lets the kernel pick. */ + readonly port?: number; + /** Deadline in ms. Defaults to {@link AUTHENTICATE_TIMEOUT_MS}. */ + readonly timeoutMs?: number; + /** Host-cancellation signal — closes the listener on abort. */ + readonly signal?: AbortSignal; +} + +// --------------------------------------------------------------------------- +// Response pages (no JS, no external resources, locked-down CSP) +// --------------------------------------------------------------------------- + +function securityHeaders(): http.OutgoingHttpHeaders { + return { + 'Content-Type': 'text/html; charset=utf-8', + 'Cache-Control': 'no-store', + 'Referrer-Policy': 'no-referrer', + 'X-Content-Type-Options': 'nosniff', + 'Content-Security-Policy': "default-src 'none'; style-src 'unsafe-inline'", + // Tell the client to close so server.close() can complete promptly. + Connection: 'close', + }; +} + +function page(title: string, heading: string, body: string): string { + return [ + '', + '', + `${title}`, + '', + '', + `

${heading}

`, + `

${body}

`, + '', + ].join(''); +} + +const SUCCESS_PAGE = page( + 'Authenticated', + 'Authenticated', + 'You can close this tab and return to your terminal.', +); + +function errorPage(reason: string): string { + // `reason` is an OAuth error code already known to the attacker (it + // came from the query string); no secret is disclosed by echoing it. + return page( + 'Sign-in failed', + 'Sign-in failed', + `${escapeHtml(reason)}. You can close this tab and try again from your terminal.`, + ); +} + +function escapeHtml(s: string): string { + return s + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"'); +} + +// --------------------------------------------------------------------------- +// Constant-time string comparison +// --------------------------------------------------------------------------- + +function constantTimeEqual(a: string, b: string): boolean { + const ab = Buffer.from(a, 'utf8'); + const bb = Buffer.from(b, 'utf8'); + if (ab.length !== bb.length) { + // Length is observable, but run a same-length compare so the + // per-byte path doesn't add an obvious early-out signal. + timingSafeEqual(ab, ab); + return false; + } + return timingSafeEqual(ab, bb); +} + +// --------------------------------------------------------------------------- +// Listener +// --------------------------------------------------------------------------- + +export async function startLoopbackListener( + opts: StartLoopbackOptions, +): Promise { + const server = http.createServer(); + const requestedPort = opts.port ?? 0; + + await new Promise((resolve, reject) => { + const onError = (err: Error): void => { + server.removeListener('error', onError); + reject(err); + }; + server.once('error', onError); + server.listen(requestedPort, LOOPBACK_HOST, () => { + server.removeListener('error', onError); + resolve(); + }); + }); + + const addr = server.address() as AddressInfo; + const port = addr.port; + const expectedHost = `${LOOPBACK_HOST}:${port}`; + const redirectUri = `http://${expectedHost}${CALLBACK_PATH}`; + const timeoutMs = opts.timeoutMs ?? AUTHENTICATE_TIMEOUT_MS; + + let settled = false; + let resolveResult!: (r: LoopbackResult) => void; + let rejectResult!: (e: Error) => void; + const result = new Promise((res, rej) => { + resolveResult = res; + rejectResult = rej; + }); + + let timer: ReturnType | undefined; + + const onSignal = (): void => finish({ kind: 'reject', error: new LoopbackAbortedError() }); + const onAbort = (): void => finish({ kind: 'reject', error: new LoopbackAbortedError() }); + + function teardown(): void { + if (timer) clearTimeout(timer); + process.removeListener('SIGINT', onSignal); + process.removeListener('SIGTERM', onSignal); + if (opts.signal) opts.signal.removeEventListener('abort', onAbort); + server.close(); + } + + type Outcome = + | { kind: 'resolve'; value: LoopbackResult } + | { kind: 'reject'; error: Error }; + + function finish(outcome: Outcome): void { + if (settled) return; + settled = true; + teardown(); + if (outcome.kind === 'resolve') resolveResult(outcome.value); + else rejectResult(outcome.error); + } + + server.on('request', (req, res) => { + // DNS-rebinding defence — reject anything not addressed to our exact + // loopback host:port, *before* parsing query params or touching + // state, so response timing can't leak whether a flow is live. No + // teardown: a forged Host must not be able to cancel a real flow. + if (req.headers.host !== expectedHost) { + res.writeHead(400, { 'Content-Type': 'text/plain; charset=utf-8', Connection: 'close' }); + res.end(); + return; + } + + let url: URL; + try { + url = new URL(req.url ?? '/', `http://${expectedHost}`); + } catch { + res.writeHead(400, securityHeaders()); + res.end(errorPage('bad_request')); + return; + } + + if (url.pathname !== CALLBACK_PATH) { + // Do not echo the path. + res.writeHead(404, { 'Content-Type': 'text/plain; charset=utf-8', Connection: 'close' }); + res.end(); + return; + } + + const stateParam = url.searchParams.get('state') ?? ''; + // CSRF: bind the callback (success *or* error) to our flow first. + if (!constantTimeEqual(stateParam, opts.expectedState)) { + res.writeHead(400, securityHeaders()); + res.end(errorPage('state_mismatch'), () => { + finish({ kind: 'reject', error: new LoopbackStateMismatchError() }); + }); + return; + } + + const errorParam = url.searchParams.get('error'); + if (errorParam) { + res.writeHead(200, securityHeaders()); + res.end(errorPage(errorParam), () => { + finish({ kind: 'reject', error: new LoopbackAuthorizationError(errorParam) }); + }); + return; + } + + const code = url.searchParams.get('code'); + if (!code) { + res.writeHead(400, securityHeaders()); + res.end(errorPage('missing_code'), () => { + finish({ kind: 'reject', error: new LoopbackAuthorizationError('missing_code') }); + }); + return; + } + + // Respond fully before settling/closing so the browser sees a clean + // 200 rather than a connection reset. + res.writeHead(200, securityHeaders()); + const params = new URLSearchParams(url.searchParams); + res.end(SUCCESS_PAGE, () => { + finish({ kind: 'resolve', value: { code, state: stateParam, params } }); + }); + }); + + timer = setTimeout(() => finish({ kind: 'reject', error: new LoopbackTimeoutError() }), timeoutMs); + + process.on('SIGINT', onSignal); + process.on('SIGTERM', onSignal); + if (opts.signal) { + if (opts.signal.aborted) { + finish({ kind: 'reject', error: new LoopbackAbortedError() }); + } else { + opts.signal.addEventListener('abort', onAbort, { once: true }); + } + } + + return { + port, + redirectUri, + result, + close: () => finish({ kind: 'reject', error: new LoopbackAbortedError() }), + }; +} + +export { SUCCESS_PAGE }; diff --git a/ts-packages/quarto-hub-mcp/src/auth/oauth-config.test.ts b/ts-packages/quarto-hub-mcp/src/auth/oauth-config.test.ts new file mode 100644 index 00000000..1ac0e099 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/oauth-config.test.ts @@ -0,0 +1,63 @@ +/** + * OAuth env-config sourcing. Both `QUARTO_HUB_MCP_CLIENT_ID` and + * `QUARTO_HUB_MCP_CLIENT_SECRET` are required (the Desktop-app client + * still needs the secret — Amendment 2026-05-28); a partial config + * fails naming the missing var. + */ + +import { describe, it, expect } from 'vitest'; + +import { loadOAuthConfigFromEnv, MissingOAuthConfigError } from './oauth-config.js'; + +function envOnly(values: Record): NodeJS.ProcessEnv { + const e: NodeJS.ProcessEnv = {}; + for (const [k, v] of Object.entries(values)) { + if (v !== undefined) e[k] = v; + } + return e; +} + +describe('loadOAuthConfigFromEnv', () => { + it('returns both values when both are set', () => { + const cfg = loadOAuthConfigFromEnv( + envOnly({ + QUARTO_HUB_MCP_CLIENT_ID: 'cid.apps.googleusercontent.com', + QUARTO_HUB_MCP_CLIENT_SECRET: 'GOCSPX-secret', + }), + ); + expect(cfg.clientId).toBe('cid.apps.googleusercontent.com'); + expect(cfg.clientSecret).toBe('GOCSPX-secret'); + }); + + it('throws naming the secret when only the id is set', () => { + expect(() => + loadOAuthConfigFromEnv(envOnly({ QUARTO_HUB_MCP_CLIENT_ID: 'cid' })), + ).toThrowError(/QUARTO_HUB_MCP_CLIENT_SECRET/); + }); + + it('throws naming the id when only the secret is set', () => { + expect(() => + loadOAuthConfigFromEnv(envOnly({ QUARTO_HUB_MCP_CLIENT_SECRET: 'sec' })), + ).toThrowError(/QUARTO_HUB_MCP_CLIENT_ID/); + }); + + it('throws MissingOAuthConfigError naming both when neither is set', () => { + let caught: unknown; + try { + loadOAuthConfigFromEnv(envOnly({})); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(MissingOAuthConfigError); + expect((caught as Error).message).toMatch(/QUARTO_HUB_MCP_CLIENT_ID/); + expect((caught as Error).message).toMatch(/QUARTO_HUB_MCP_CLIENT_SECRET/); + }); + + it('treats an empty / whitespace value as unset', () => { + expect(() => + loadOAuthConfigFromEnv( + envOnly({ QUARTO_HUB_MCP_CLIENT_ID: ' ', QUARTO_HUB_MCP_CLIENT_SECRET: 'sec' }), + ), + ).toThrowError(/QUARTO_HUB_MCP_CLIENT_ID/); + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/oauth-config.ts b/ts-packages/quarto-hub-mcp/src/auth/oauth-config.ts new file mode 100644 index 00000000..3a3b1435 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/oauth-config.ts @@ -0,0 +1,96 @@ +/** + * OAuth configuration sourcing + authorization-server discovery. + * + * Houses the IdP-agnostic plumbing that outlives the device-flow → + * loopback+PKCE switch: env-var sourcing for the operator-supplied + * Google OAuth client credentials, and a cached OIDC discovery lookup. + * + * Both `QUARTO_HUB_MCP_CLIENT_ID` and `QUARTO_HUB_MCP_CLIENT_SECRET` + * are required. Google's Desktop-app client type still issues a + * `client_secret` and requires it on the token exchange and the + * refresh-token grant; PKCE is layered on top of the confidential-client + * flow rather than replacing it (see the loopback+PKCE plan, + * Amendment 2026-05-28). + */ + +import * as oauth from 'oauth4webapi'; + +// --------------------------------------------------------------------------- +// Typed errors +// --------------------------------------------------------------------------- + +export class MissingOAuthConfigError extends Error { + override readonly name = 'MissingOAuthConfigError'; + constructor(message: string) { + super(message); + } +} + +// --------------------------------------------------------------------------- +// Configuration sourcing +// --------------------------------------------------------------------------- + +export interface OAuthEnvConfig { + readonly clientId: string; + readonly clientSecret: string; +} + +const CLIENT_ID_VAR = 'QUARTO_HUB_MCP_CLIENT_ID'; +const CLIENT_SECRET_VAR = 'QUARTO_HUB_MCP_CLIENT_SECRET'; + +function readNonEmpty(env: NodeJS.ProcessEnv, name: string): string | undefined { + const v = env[name]; + if (v === undefined) return undefined; + return v.trim() === '' ? undefined : v; +} + +export function loadOAuthConfigFromEnv( + env: NodeJS.ProcessEnv = process.env +): OAuthEnvConfig { + const clientId = readNonEmpty(env, CLIENT_ID_VAR); + const clientSecret = readNonEmpty(env, CLIENT_SECRET_VAR); + const missing: string[] = []; + if (clientId === undefined) missing.push(CLIENT_ID_VAR); + if (clientSecret === undefined) missing.push(CLIENT_SECRET_VAR); + if (missing.length > 0) { + throw new MissingOAuthConfigError( + `${missing.join(' and ')} ${missing.length === 1 ? 'is' : 'are'} not set. ` + + `Hub-mcp requires ${CLIENT_ID_VAR} and ${CLIENT_SECRET_VAR} in the ` + + `MCP-client env. Ask your hub operator for the Google OAuth client ` + + `credentials they registered for hub-mcp.` + ); + } + return { clientId: clientId!, clientSecret: clientSecret! }; +} + +// --------------------------------------------------------------------------- +// AuthorizationServer discovery (cached) +// --------------------------------------------------------------------------- + +/** + * Lazily resolves the discovered {@link oauth.AuthorizationServer}. Lets + * consumers defer the OIDC discovery network call off the startup path — + * it fires on first auth operation, not at process boot. Memoized via the + * module-level discovery cache, so repeated calls are free. + */ +export type AuthServerProvider = () => Promise; + +let cachedAS: { readonly issuer: string; readonly as: oauth.AuthorizationServer } | undefined; + +export async function discoverAuthorizationServer( + issuer: string, + opts?: { fetch?: typeof fetch } +): Promise { + if (cachedAS && cachedAS.issuer === issuer) return cachedAS.as; + const url = new URL(issuer); + const requestOpts = opts?.fetch ? { [oauth.customFetch]: opts.fetch } : undefined; + const resp = await oauth.discoveryRequest(url, requestOpts); + const as = await oauth.processDiscoveryResponse(url, resp); + cachedAS = { issuer, as }; + return as; +} + +/** Test hook — reset the in-process discovery cache. */ +export function _resetDiscoveryCache(): void { + cachedAS = undefined; +} diff --git a/ts-packages/quarto-hub-mcp/src/auth/pkce.test.ts b/ts-packages/quarto-hub-mcp/src/auth/pkce.test.ts new file mode 100644 index 00000000..00ef8e17 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/pkce.test.ts @@ -0,0 +1,39 @@ +/** + * PKCE + state primitives. + * + * Known-answer test against RFC 7636 §4.6 pins the S256 transform; the + * round-trip test confirms our wrapper keeps verifier and challenge in + * agreement. + */ + +import { describe, it, expect } from 'vitest'; +import * as oauth from 'oauth4webapi'; + +import { generatePkceParams } from './pkce.js'; + +// RFC 7636 §4.6 worked example. +const RFC_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; +const RFC_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + +describe('PKCE', () => { + it('computes the RFC 7636 §4.6 S256 challenge for the example verifier', async () => { + const challenge = await oauth.calculatePKCECodeChallenge(RFC_VERIFIER); + expect(challenge).toBe(RFC_CHALLENGE); + }); + + it('generatePkceParams returns a self-consistent S256 verifier/challenge', async () => { + const p = await generatePkceParams(); + expect(p.codeChallengeMethod).toBe('S256'); + expect(p.codeVerifier.length).toBeGreaterThanOrEqual(43); + const recomputed = await oauth.calculatePKCECodeChallenge(p.codeVerifier); + expect(p.codeChallenge).toBe(recomputed); + }); + + it('generates a high-entropy state value', async () => { + const a = await generatePkceParams(); + const b = await generatePkceParams(); + // base64url of ≥128 bits ⇒ ≥22 chars; and two draws must differ. + expect(a.state.length).toBeGreaterThanOrEqual(22); + expect(a.state).not.toBe(b.state); + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/pkce.ts b/ts-packages/quarto-hub-mcp/src/auth/pkce.ts new file mode 100644 index 00000000..1d6b71e7 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/pkce.ts @@ -0,0 +1,30 @@ +/** + * PKCE + CSRF-state primitives. + * + * Thin wrapper over `oauth4webapi` (already a project dep, used by every + * other auth path) so the loopback flow has a small, stable, testable + * surface. Rolling our own with Node `crypto` would diverge from the + * established pattern for no gain. + * + * - `code_verifier` / `code_challenge` (S256) bind the authorization + * code to this process: a leaked code is useless without the + * verifier held in memory here. + * - `state` (≥128 bits of entropy, base64url) binds the callback to + * the originating flow — the CSRF defence. + */ + +import * as oauth from 'oauth4webapi'; + +export interface PkceParams { + readonly codeVerifier: string; + readonly codeChallenge: string; + readonly codeChallengeMethod: 'S256'; + readonly state: string; +} + +export async function generatePkceParams(): Promise { + const codeVerifier = oauth.generateRandomCodeVerifier(); + const codeChallenge = await oauth.calculatePKCECodeChallenge(codeVerifier); + const state = oauth.generateRandomState(); + return { codeVerifier, codeChallenge, codeChallengeMethod: 'S256', state }; +} diff --git a/ts-packages/quarto-hub-mcp/src/auth/redact.test.ts b/ts-packages/quarto-hub-mcp/src/auth/redact.test.ts new file mode 100644 index 00000000..b237ed56 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/redact.test.ts @@ -0,0 +1,33 @@ +/** + * Token redaction — known-answer cases lifted from the former + * device-flow test suite when `redactTokens` moved to its own module. + */ + +import { describe, it, expect } from 'vitest'; + +import { redactTokens } from './redact.js'; + +describe('redactTokens', () => { + it('redacts Google access tokens (ya29.*)', () => { + const s = 'token=ya29.aBcDeF_-1234567890XYZ end'; + expect(redactTokens(s)).not.toContain('ya29.aBcDeF'); + expect(redactTokens(s)).toContain('[redacted-token]'); + }); + + it('redacts Google refresh tokens (1//*)', () => { + const s = 'rt=1//0abcDEF-1234_xyz end'; + expect(redactTokens(s)).not.toContain('1//0abcDEF'); + expect(redactTokens(s)).toContain('[redacted-token]'); + }); + + it('redacts JWT-shaped substrings (xxx.yyy.zzz)', () => { + const jwt = + 'eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ4In0.signature_bytes_here_AB12'; + expect(redactTokens(`auth: ${jwt} end`)).not.toContain(jwt); + expect(redactTokens(`auth: ${jwt} end`)).toContain('[redacted-token]'); + }); + + it('passes through strings with no token shapes', () => { + expect(redactTokens('hello world')).toBe('hello world'); + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/auth/redact.ts b/ts-packages/quarto-hub-mcp/src/auth/redact.ts new file mode 100644 index 00000000..d69c3096 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/auth/redact.ts @@ -0,0 +1,22 @@ +/** + * Token redaction for log call sites. + * + * Every log statement in the auth path funnels through {@link redactTokens} + * so a Google-token-shaped substring never reaches stderr in the clear. + * The pattern is a single alternation so the input is scanned once per call. + * + * Note: `code_verifier`, the authorization `code`, and `state` do **not** + * match these shapes and must be filtered at the call site (i.e. simply + * never passed to a logger). This module only handles token-shaped bytes. + */ + +// Branches: +// ya29\.... — Google access tokens +// 1\/\/... — Google refresh tokens +// eyJ....\....\.... — JWT-shaped (three base64url segments) +const TOKEN_PATTERN = + /ya29\.[A-Za-z0-9_-]+|1\/\/[A-Za-z0-9_-]+|eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+/g; + +export function redactTokens(s: string): string { + return s.replace(TOKEN_PATTERN, '[redacted-token]'); +} diff --git a/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.test.ts b/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.test.ts index 175b58f4..d9cd975d 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.test.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.test.ts @@ -19,6 +19,7 @@ import { import { ReauthRequired, RefreshManager, + TokenRefreshError, type RefreshManagerDeps, } from './refresh-manager.js'; @@ -215,10 +216,10 @@ async function seedStore( function makeRm( store: CredentialStore, fetchImpl: typeof fetch, - extra: Partial> = {}, + extra: Partial> = {}, ): RefreshManager { return new RefreshManager({ - as: AS, + authServer: async () => AS, config: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, store, fetch: fetchImpl, @@ -361,6 +362,56 @@ describe('RefreshManager.forceRefresh failure handling', () => { await expect(rm.forceRefresh()).rejects.toThrow(); expect(state.value).toBe(beforeBlob); }); + + it('throws an actionable TokenRefreshError on invalid_client (wrong client secret), store intact', async () => { + const { store, state } = await seedStore(makeBundle()); + const beforeBlob = state.value; + const { fetch } = makeFetch(() => + jsonResponse(401, { + error: 'invalid_client', + error_description: 'The provided client secret is invalid.', + }), + ); + const rm = makeRm(store, fetch); + let err: unknown; + try { + await rm.forceRefresh(); + } catch (e) { + err = e; + } + expect(err).toBeInstanceOf(TokenRefreshError); + const tre = err as TokenRefreshError; + expect(tre.oauthError).toBe('invalid_client'); + expect(tre.oauthErrorDescription).toBe('The provided client secret is invalid.'); + expect(tre.isConfigError).toBe(true); + // Message surfaces the code, the description, and the config remediation. + expect(tre.message).toContain('invalid_client'); + expect(tre.message).toContain('The provided client secret is invalid.'); + expect(tre.message).toMatch(/QUARTO_HUB_MCP_CLIENT_(ID|SECRET)/); + // Not the opaque oauth4webapi default. + expect(tre.message).not.toBe('server responded with an error in the response body'); + // The stored credential is the user's grant, not the problem — keep it. + expect(state.value).toBe(beforeBlob); + }); + + it('wraps a 4xx non-config oauth error in a TokenRefreshError flagged non-config', async () => { + const { store } = await seedStore(makeBundle()); + // oauth4webapi only parses an error *body* for 4xx; a non-config code + // like invalid_request becomes a ResponseBodyError we can wrap. + const { fetch } = makeFetch(() => jsonResponse(400, { error: 'invalid_request' })); + const rm = makeRm(store, fetch); + let err: unknown; + try { + await rm.forceRefresh(); + } catch (e) { + err = e; + } + expect(err).toBeInstanceOf(TokenRefreshError); + const tre = err as TokenRefreshError; + expect(tre.oauthError).toBe('invalid_request'); + expect(tre.isConfigError).toBe(false); + expect(tre.message).toMatch(/transient/i); + }); }); // --------------------------------------------------------------------------- @@ -422,6 +473,72 @@ describe('RefreshManager.getValidIdToken', () => { }); }); +// --------------------------------------------------------------------------- +// invalidate — best-effort grant wipe +// --------------------------------------------------------------------------- + +describe('RefreshManager.invalidate', () => { + it('clears the stored grant', async () => { + const { store, state } = await seedStore(makeBundle()); + expect(state.value).not.toBeNull(); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const rm = makeRm(store, fetch); + await rm.invalidate(); + expect(state.value).toBeNull(); + expect(await store.read()).toBeNull(); + }); + + it('swallows a keyring failure (the caller surfaces ReauthRequired)', async () => { + const failing: KeyringBackend = { + async read() { + return null; + }, + async write() {}, + async clear() { + throw new Error('Secret Service unavailable'); + }, + }; + const store = new CredentialStore(CFG, failing); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const rm = makeRm(store, fetch); + await expect(rm.invalidate()).resolves.toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// Lazy authorization-server discovery +// --------------------------------------------------------------------------- + +describe('RefreshManager lazy discovery', () => { + it('does not resolve the authorization server on the cached-token fast path', async () => { + const { store } = await seedStore(makeBundle()); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const authServer = vi.fn(async () => AS); + const rm = new RefreshManager({ + authServer, + config: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, + store, + fetch, + }); + await rm.getValidIdToken(); + expect(authServer).not.toHaveBeenCalled(); + }); + + it('resolves the authorization server only when a refresh actually fires', async () => { + const { store } = await seedStore(makeBundle()); + const { fetch } = makeFetch(() => jsonResponse(200, tokenResponseBody())); + const authServer = vi.fn(async () => AS); + const rm = new RefreshManager({ + authServer, + config: { clientId: FAKE_CLIENT_ID, clientSecret: FAKE_CLIENT_SECRET }, + store, + fetch, + }); + await rm.forceRefresh(); + expect(authServer).toHaveBeenCalledOnce(); + }); +}); + // --------------------------------------------------------------------------- // Concurrency // --------------------------------------------------------------------------- diff --git a/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.ts b/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.ts index 1028b983..bcd1403d 100644 --- a/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.ts +++ b/ts-packages/quarto-hub-mcp/src/auth/refresh-manager.ts @@ -16,13 +16,18 @@ * coalesce onto a single `/token` request and observe the same new * id_token. * - * **Refresh-token persistence rule** — empirically (2026-05-19, see - * the plan's Verification log) Google does not rotate refresh tokens - * for the Limited-Input-Devices client type. We follow OAuth's - * defensive rule: if the response carries a `refresh_token` field we - * persist it (handles a future IdP that *does* rotate); otherwise we - * keep the prior value. Discarding on missing field would force a - * re-auth on every refresh against today's live Google. + * **Refresh-token persistence rule** — we follow OAuth's defensive + * rule: if the response carries a `refresh_token` field we persist it + * (handles an IdP that rotates on every grant); otherwise we keep the + * prior value (handles an IdP that issues the refresh token once and + * omits it thereafter). The rule is correct under both behaviours, so + * it does not depend on which Google client type is in use. The earlier + * empirical note about the Limited-Input-Devices client's no-rotation + * behaviour was removed when hub-mcp switched to the loopback+PKCE flow + * on the Desktop-app client (see the loopback+PKCE plan); the + * Desktop-app client's steady-state rotation behaviour is pending + * confirmation by that plan's Spike A, but this defensive rule holds + * either way. * * `invalid_grant` is the one terminal error the manager handles * itself: it clears the credential store (so subsequent @@ -39,6 +44,7 @@ import { type CredentialBundle, type CredentialStore, } from './credential-store.js'; +import type { AuthServerProvider } from './oauth-config.js'; // --------------------------------------------------------------------------- // Public types @@ -50,7 +56,8 @@ export interface RefreshManagerConfig { } export interface RefreshManagerDeps { - readonly as: oauth.AuthorizationServer; + /** Lazily-resolved authorization server; called only when a refresh fires. */ + readonly authServer: AuthServerProvider; readonly config: RefreshManagerConfig; readonly store: CredentialStore; /** @@ -78,6 +85,50 @@ export class ReauthRequired extends Error { } } +/** OAuth errors that point at the server's client config, not the user's grant. */ +const CONFIG_OAUTH_ERRORS = new Set(['invalid_client', 'unauthorized_client']); + +function buildTokenRefreshMessage( + oauthError: string, + oauthErrorDescription: string | undefined, +): string { + const detail = oauthErrorDescription + ? `${oauthError}: ${oauthErrorDescription}` + : oauthError; + if (CONFIG_OAUTH_ERRORS.has(oauthError)) { + return ( + `Token refresh was rejected by the identity provider (${detail}). ` + + `This points to the hub MCP server's OAuth client configuration, ` + + `not your stored credentials — re-authenticating will not fix it. ` + + `Check that QUARTO_HUB_MCP_CLIENT_ID and QUARTO_HUB_MCP_CLIENT_SECRET ` + + `name a valid OAuth client and matching secret.` + ); + } + return ( + `Token refresh failed (${detail}). This is usually transient — retry ` + + `shortly. If it persists, re-authenticate with the authenticate tool.` + ); +} + +/** + * Token refresh failed with a structured OAuth error other than + * `invalid_grant`. Carries the IdP's `error` / `error_description` so the + * failure is actionable instead of oauth4webapi's opaque default message. + */ +export class TokenRefreshError extends Error { + override readonly name = 'TokenRefreshError'; + readonly oauthError: string; + readonly oauthErrorDescription: string | undefined; + /** True for operator/config problems, not the user's grant. */ + readonly isConfigError: boolean; + constructor(oauthError: string, oauthErrorDescription?: string) { + super(buildTokenRefreshMessage(oauthError, oauthErrorDescription)); + this.oauthError = oauthError; + this.oauthErrorDescription = oauthErrorDescription; + this.isConfigError = CONFIG_OAUTH_ERRORS.has(oauthError); + } +} + // --------------------------------------------------------------------------- // RefreshManager // --------------------------------------------------------------------------- @@ -96,6 +147,17 @@ export class RefreshManager { this.deps = deps; } + /** + * Best-effort wipe of the stored grant. Both the IdP's `invalid_grant` + * (here) and the hub's persistent-401 (connection-manager) converge on + * this: forget the dead credential so the next `getValidIdToken` fails + * loud instead of spinning on it. Swallows keyring errors — the caller's + * `ReauthRequired` is the signal that matters. + */ + async invalidate(): Promise { + await this.deps.store.clear().catch(() => undefined); + } + async getValidIdToken(): Promise { const bundle = await this.deps.store.read(); if (bundle === null) throw new ReauthRequired(); @@ -123,6 +185,9 @@ export class RefreshManager { const bundle = await this.deps.store.read(); if (bundle === null) throw new ReauthRequired(); + // Resolve discovery lazily — only reached when an actual refresh fires, + // never on the cached-token fast path. + const as = await this.deps.authServer(); const client: oauth.Client = { client_id: this.deps.config.clientId }; const clientAuth = oauth.ClientSecretPost(this.deps.config.clientSecret); const requestOpts = this.deps.fetch @@ -133,26 +198,31 @@ export class RefreshManager { try { const resp = requestOpts ? await oauth.refreshTokenGrantRequest( - this.deps.as, + as, client, clientAuth, bundle.refreshToken, requestOpts, ) : await oauth.refreshTokenGrantRequest( - this.deps.as, + as, client, clientAuth, bundle.refreshToken, ); - tokens = await oauth.processRefreshTokenResponse(this.deps.as, client, resp); + tokens = await oauth.processRefreshTokenResponse(as, client, resp); } catch (err) { - if (err instanceof oauth.ResponseBodyError && err.error === 'invalid_grant') { - // Stored refresh_token is rejected by Google: revoked, expired, - // or never valid. Clear so the next `getValidIdToken` fails - // loud rather than spinning on the same bad token. - await this.deps.store.clear().catch(() => undefined); - throw new ReauthRequired(REAUTH_MESSAGE, err.error); + if (err instanceof oauth.ResponseBodyError) { + if (err.error === 'invalid_grant') { + // Stored refresh_token is rejected by Google: revoked, expired, + // or never valid. Wipe it so the next `getValidIdToken` fails + // loud rather than spinning on the same bad token. + await this.invalidate(); + throw new ReauthRequired(REAUTH_MESSAGE, err.error); + } + // Any other structured OAuth error: surface code + description, + // leaving the store intact (the grant isn't what was rejected). + throw new TokenRefreshError(err.error, err.error_description); } throw err; } 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 01dbc8bd..33a377e0 100644 --- a/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts +++ b/ts-packages/quarto-hub-mcp/src/connection-manager.test.ts @@ -66,6 +66,7 @@ interface SeededAuth { refresh: RefreshManager; getValid: ReturnType; forceRefresh: ReturnType; + invalidate: ReturnType; } function seededAuth(opts?: { initialToken?: string }): SeededAuth { @@ -87,10 +88,16 @@ function seededAuth(opts?: { initialToken?: string }): SeededAuth { const refresh = Object.create(RefreshManager.prototype) as RefreshManager; const getValid = vi.fn().mockResolvedValue(initial.idToken); const forceRefresh = vi.fn().mockResolvedValue(initial.idToken); + // The real RefreshManager.invalidate clears its own store; the mock + // routes to the same real store so persistent-401 clear assertions hold. + const invalidate = vi.fn(async () => { + await store.clear(); + }); Object.defineProperty(refresh, 'getValidIdToken', { value: getValid }); Object.defineProperty(refresh, 'forceRefresh', { value: forceRefresh }); + Object.defineProperty(refresh, 'invalidate', { value: invalidate }); - return { store, refresh, getValid, forceRefresh }; + return { store, refresh, getValid, forceRefresh, invalidate }; } // --------------------------------------------------------------------------- @@ -316,10 +323,10 @@ describe('ConnectionManager with creds', () => { expect(mgr.lastObservedAuthMode()).toBe('requires-auth'); }); - it('clears the credential store on persistent 401 so authenticate_start starts a fresh flow', async () => { + it('clears the credential store on persistent 401 so authenticate starts a fresh flow', async () => { // Regression: before this, ConnectionManager threw ReauthRequired // but left a freshly-refreshed id_token in the store. The next - // `authenticate_start` would then short-circuit on + // `authenticate` would then short-circuit on // `RefreshManager.getValidIdToken()` and reply "Already // authenticated as …", trapping the agent in a state mismatch // between local view (token works) and hub view (token rejected). @@ -363,6 +370,70 @@ describe('ConnectionManager with creds', () => { }); }); +// --------------------------------------------------------------------------- +// Probe-skip optimization (don't re-probe once auth-mode is established) +// --------------------------------------------------------------------------- + +describe('ConnectionManager probe-skip', () => { + it('skips the /health probe on a later connect once creds are confirmed', async () => { + const auth = seededAuth(); + // Only one scripted response: a second probe would throw "ran out of + // responses", so this asserts the probe is not re-issued. + 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'); + await mgr.connect('idx-2'); + + expect(fetchSpy.calls).toHaveLength(1); + expect(sync.connectCalls).toHaveLength(2); + // Still resolves a token for the skipped connect (proactive refresh / + // early error surfacing). + expect(auth.getValid).toHaveBeenCalled(); + expect(mgr.lastObservedAuthMode()).toBe('requires-auth'); + }); + + it('skips the probe on a later connect to a known no-auth hub (no creds)', async () => { + const fetchSpy = scriptedFetch([200]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + + await mgr.connect('idx-1'); + await mgr.connect('idx-2'); + + expect(fetchSpy.calls).toHaveLength(1); + expect(sync.connectCalls).toHaveLength(2); + expect(sync.connectCalls.every((c) => c.auth === undefined)).toBe(true); + }); + + it('throws AuthRequired without re-probing once the hub is known to require auth (no creds)', async () => { + const fetchSpy = scriptedFetch([401]); + const sync = spySyncClientFactory(); + const mgr = new ConnectionManager({ + serverUrl: 'wss://hub.example.com/ws', + fetch: fetchSpy.fetch, + syncClientFactory: sync.factory, + }); + + await expect(mgr.connect('idx-1')).rejects.toBeInstanceOf(AuthRequiredError); + await expect(mgr.connect('idx-2')).rejects.toBeInstanceOf(AuthRequiredError); + + expect(fetchSpy.calls).toHaveLength(1); + expect(sync.connectCalls).toHaveLength(0); + }); +}); + // --------------------------------------------------------------------------- // Insecure-transport gate // --------------------------------------------------------------------------- diff --git a/ts-packages/quarto-hub-mcp/src/connection-manager.ts b/ts-packages/quarto-hub-mcp/src/connection-manager.ts index 1d41b3c1..44dd0d61 100644 --- a/ts-packages/quarto-hub-mcp/src/connection-manager.ts +++ b/ts-packages/quarto-hub-mcp/src/connection-manager.ts @@ -9,7 +9,7 @@ * 3. On 401 + creds attached → forceRefresh, retry once. * Still 401 → throw {@link ReauthRequired}. * 4. On 401 + no creds attached → throw {@link AuthRequiredError} - * naming `authenticate_start`. The trigger is the hub's 401, not + * naming `authenticate`. The trigger is the hub's 401, not * absence of creds, so hubs that don't require auth keep working. * * Insecure-transport gate: Bearer + `ws://` / `http://` + non-loopback @@ -19,7 +19,7 @@ * * `lastObservedAuthMode()` exposes the most recent auth observation * (`'no-auth'` / `'requires-auth'` / `'unknown'`) so Phase 7's - * `authenticate_start` can short-circuit against a hub that's known to + * `authenticate` can short-circuit against a hub that's known to * not require auth. */ @@ -33,7 +33,7 @@ import { import type { CredentialStore } from './auth/credential-store.js'; import { ReauthRequired, type RefreshManager } from './auth/refresh-manager.js'; -import { redactTokens } from './auth/device-flow.js'; +import { redactTokens } from './auth/redact.js'; // --------------------------------------------------------------------------- // Public types / errors @@ -41,7 +41,7 @@ import { redactTokens } from './auth/device-flow.js'; /** * Observed hub auth-mode (process-local). Phase 7 consults this to - * decide whether `authenticate_start` should short-circuit. + * decide whether `authenticate` should short-circuit. */ export type ObservedAuthMode = 'no-auth' | 'requires-auth' | 'unknown'; @@ -49,7 +49,7 @@ export class AuthRequiredError extends Error { override readonly name = 'AuthRequiredError'; constructor( message: string = 'This Quarto Hub requires authentication. ' + - 'Ask me to call `authenticate_start` to begin the device-flow.', + 'Ask me to call `authenticate` to sign in.', ) { super(message); } @@ -132,6 +132,12 @@ export class ConnectionManager { private readonly projects = new Map(); private observedAuthMode: ObservedAuthMode = 'unknown'; + // Set once a probe has returned 200 with our Bearer attached, i.e. the + // hub has confirmed these creds work. Subsequent connects then skip the + // redundant per-connect /health probe — `getValidIdToken` still refreshes + // proactively and the WS handshake is the backstop if the hub later + // rejects the token. + private authConfirmed = false; constructor(deps: ConnectionManagerDeps | string) { // Backwards-compat: the prior signature was `new ConnectionManager(url)`. @@ -292,7 +298,11 @@ export class ConnectionManager { : null; if (bundle === null || this.refreshManager === undefined) { - // No creds (or no refresh manager wired) → probe without header. + // No creds (or no refresh manager wired). Reuse a prior observation + // to skip the probe once the hub's auth-mode is known. + if (this.observedAuthMode === 'no-auth') return undefined; + if (this.observedAuthMode === 'requires-auth') throw new AuthRequiredError(); + // Mode still unknown → probe without header to learn it. const status = await this.probeAuth(undefined); this.recordObservation(status, false); if (status === 401) throw new AuthRequiredError(); @@ -302,41 +312,45 @@ export class ConnectionManager { // We have creds. Insecure-transport gate fires only when we'd // actually attach a Bearer. this.assertSecureTransport(); + const rm = this.refreshManager; + + // Creds already validated against this hub in this process → skip the + // redundant probe. We still pull a token now so a refresh failure + // (ReauthRequired / TokenRefreshError) surfaces before we open the WS; + // on the happy path that's a cached, network-free call. + if (this.authConfirmed) { + await rm.getValidIdToken(); + return { getBearer: () => rm.getValidIdToken() }; + } // Pull a valid id_token (refreshes proactively within the skew). - let token = await this.refreshManager.getValidIdToken(); + let token = await rm.getValidIdToken(); let status = await this.probeAuth(token); if (status === 401) { // Stale token or revoked grant. Force one refresh and retry. - token = await this.refreshManager.forceRefresh(); + token = await rm.forceRefresh(); status = await this.probeAuth(token); if (status === 401) { this.recordObservation(401, true); // Hub rejects a freshly-refreshed token — local view and hub - // view disagree on whether these credentials are usable. Clear - // the store so `authenticate_start`'s "already authenticated" - // short-circuit cannot keep the agent trapped against a hub - // that won't accept this identity. The next `getValidIdToken` - // call raises ReauthRequired, falling through to the device - // flow. - if (this.credentialStore) { - try { - await this.credentialStore.clear(); - } catch { - // Don't mask the underlying auth failure with a keyring - // unavailability error — the user still needs to see - // ReauthRequired and decide what to do. - } - } + // view disagree on whether these credentials are usable. Wipe the + // grant (via the lifecycle owner) so `authenticate`'s "already + // authenticated" short-circuit cannot keep the agent trapped + // against a hub that won't accept this identity. The next + // `getValidIdToken` raises ReauthRequired, falling through to the + // sign-in flow. `invalidate` is best-effort and swallows keyring + // errors so we never mask this auth failure with a storage one. + await rm.invalidate(); throw new ReauthRequired(); } } this.recordObservation(status, true); if (status === 200) { - // Hand the sync client a getter so each attach + retry sees a - // freshly-refreshed token. - const rm = this.refreshManager; + // Hub accepted the Bearer — remember it so later connects skip the + // probe, and hand the sync client a getter so each attach + retry + // sees a freshly-refreshed token. + this.authConfirmed = true; return { getBearer: () => rm.getValidIdToken() }; } throw new Error( diff --git a/ts-packages/quarto-hub-mcp/src/index.test.ts b/ts-packages/quarto-hub-mcp/src/index.test.ts new file mode 100644 index 00000000..d67fdc23 --- /dev/null +++ b/ts-packages/quarto-hub-mcp/src/index.test.ts @@ -0,0 +1,43 @@ +/** + * `--redirect-port` validation. The kernel-pick default is reached by + * *omitting* the flag, so `0` and the rest of the privileged range are + * rejected; a stable SSH-tunnel port must be non-privileged. + */ + +import { describe, it, expect } from 'vitest'; + +import { parseRedirectPort } from './index.js'; + +describe('parseRedirectPort', () => { + it('accepts the bottom of the non-privileged range', () => { + expect(parseRedirectPort('1024')).toBe(1024); + }); + + it('accepts the top of the valid range', () => { + expect(parseRedirectPort('65535')).toBe(65535); + }); + + it('accepts an ephemeral-range port', () => { + expect(parseRedirectPort('49152')).toBe(49152); + }); + + it('rejects 0 (kernel-pick is reached by omitting the flag)', () => { + expect(() => parseRedirectPort('0')).toThrowError(); + }); + + it('rejects privileged ports (<1024) naming the free range to use', () => { + expect(() => parseRedirectPort('80')).toThrowError(/49152-65535|non-privileged/); + expect(() => parseRedirectPort('1023')).toThrowError(/non-privileged/); + }); + + it('rejects out-of-range ports', () => { + expect(() => parseRedirectPort('65536')).toThrowError(); + expect(() => parseRedirectPort('99999')).toThrowError(); + expect(() => parseRedirectPort('-1')).toThrowError(); + }); + + it('rejects non-numeric input', () => { + expect(() => parseRedirectPort('abc')).toThrowError(/integer/); + expect(() => parseRedirectPort('80.5')).toThrowError(/integer/); + }); +}); diff --git a/ts-packages/quarto-hub-mcp/src/index.ts b/ts-packages/quarto-hub-mcp/src/index.ts index 9ad105c4..db97d855 100644 --- a/ts-packages/quarto-hub-mcp/src/index.ts +++ b/ts-packages/quarto-hub-mcp/src/index.ts @@ -8,8 +8,8 @@ * files in collaborative projects without filesystem access. * * Usage: - * quarto-hub-mcp --server https://hub.example.com - * quarto-hub-mcp --server https://hub.example.com --read-only + * quarto-hub-mcp --server wss://quarto-hub.com/ws + * quarto-hub-mcp --server wss://quarto-hub.com/ws --read-only * * Environment variables: * QUARTO_HUB_SERVER - Sync server URL (overridden by --server) @@ -19,6 +19,8 @@ * to non-loopback hosts (dev only) */ +import { pathToFileURL } from 'node:url'; + import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; @@ -28,17 +30,48 @@ import { AuthToolsState } from './auth/auth-tools.js'; import { CredentialStore } from './auth/credential-store.js'; import { discoverAuthorizationServer, - loadDeviceFlowConfigFromEnv, - MissingCredentialsConfigError, - redactTokens, -} from './auth/device-flow.js'; + loadOAuthConfigFromEnv, + MissingOAuthConfigError, +} from './auth/oauth-config.js'; +import { redactTokens } from './auth/redact.js'; import { RefreshManager } from './auth/refresh-manager.js'; const GOOGLE_ISSUER = 'https://accounts.google.com'; -function parseArgs(argv: string[]): { serverUrl: string; readOnly: boolean } { +interface ParsedArgs { + serverUrl: string; + readOnly: boolean; + /** Explicit loopback redirect port; undefined = kernel-picks. */ + redirectPort?: number; +} + +/** + * Validate a `--redirect-port` value. The kernel-pick default is reached + * by *omitting* the flag, not by passing `0`, so `0` (and the rest of + * the privileged range) is rejected — a stable loopback port for SSH + * tunnelling should be a non-privileged one. + */ +export function parseRedirectPort(raw: string): number { + if (!/^-?\d+$/.test(raw.trim())) { + throw new Error(`--redirect-port must be an integer, got "${raw}".`); + } + const port = Number.parseInt(raw, 10); + if (port < 1024) { + throw new Error( + `--redirect-port must be a non-privileged port (1024-65535); for SSH ` + + `tunnels pick one in the ephemeral range (e.g. 49152-65535). Got ${port}.`, + ); + } + if (port > 65535) { + throw new Error(`--redirect-port must be 1024-65535, got ${port}.`); + } + return port; +} + +function parseArgs(argv: string[]): ParsedArgs { let serverUrl = process.env['QUARTO_HUB_SERVER'] ?? ''; let readOnly = false; + let redirectPort: number | undefined; for (let i = 2; i < argv.length; i++) { const arg = argv[i]; @@ -46,13 +79,24 @@ function parseArgs(argv: string[]): { serverUrl: string; readOnly: boolean } { serverUrl = argv[++i]!; } else if (arg === '--read-only') { readOnly = true; + } else if (arg === '--redirect-port' && i + 1 < argv.length) { + try { + redirectPort = parseRedirectPort(argv[++i]!); + } catch (err) { + console.error(`Error: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } } else if (arg === '--help' || arg === '-h') { - console.error(`Usage: quarto-hub-mcp --server [--read-only] + console.error(`Usage: quarto-hub-mcp --server [--read-only] [--redirect-port ] Options: - --server Automerge sync server URL (or set QUARTO_HUB_SERVER) - --read-only Only expose read tools (no write/create/delete) - --help, -h Show this help message`); + --server Automerge sync server URL (or set QUARTO_HUB_SERVER) + --read-only Only expose read tools (no write/create/delete) + --redirect-port Fixed loopback port for the sign-in redirect + (1024-65535). Omit to let the OS pick one. Set a + stable port when forwarding sign-in over SSH: + ssh -L N:127.0.0.1:N + --help, -h Show this help message`); process.exit(0); } else { console.error(`Unknown argument: ${arg}`); @@ -65,7 +109,7 @@ Options: process.exit(1); } - return { serverUrl, readOnly }; + return { serverUrl, readOnly, redirectPort }; } /** @@ -89,7 +133,7 @@ function installRedactingErrorHandlers(): void { async function main(): Promise { installRedactingErrorHandlers(); - const { serverUrl, readOnly } = parseArgs(process.argv); + const { serverUrl, readOnly, redirectPort } = parseArgs(process.argv); // Optional auth bootstrap: if both env vars are set we wire up the // credential store + refresh manager + auth tools; if not, we run @@ -101,29 +145,31 @@ async function main(): Promise { let credentialStore: CredentialStore | undefined; let refreshManager: RefreshManager | undefined; - let flowConfig: ReturnType | undefined; - let authorizationServer: - | Awaited> - | undefined; + let flowConfig: ReturnType | undefined; + + // Lazy, memoized discovery: defers the Google network call off the + // startup path so a slow/unreachable discovery endpoint can't stop the + // server from coming up (no-auth hubs and reads don't need it). Fires + // on the first refresh / sign-in that actually requires it. + const authServer = () => discoverAuthorizationServer(GOOGLE_ISSUER); if (hasAuthEnv) { try { - flowConfig = loadDeviceFlowConfigFromEnv(); + flowConfig = loadOAuthConfigFromEnv(); } catch (err) { - if (err instanceof MissingCredentialsConfigError) { + if (err instanceof MissingOAuthConfigError) { console.error(`[hub-mcp] ${err.message}`); process.exit(1); } throw err; } - authorizationServer = await discoverAuthorizationServer(GOOGLE_ISSUER); credentialStore = new CredentialStore({ issuer: GOOGLE_ISSUER, clientId: flowConfig.clientId, }); refreshManager = new RefreshManager({ - as: authorizationServer, + authServer, config: { clientId: flowConfig.clientId, clientSecret: flowConfig.clientSecret, @@ -151,7 +197,7 @@ async function main(): Promise { ); const authToolsState = - flowConfig && authorizationServer && credentialStore && refreshManager + flowConfig && credentialStore && refreshManager ? new AuthToolsState({ credentialStore, refreshManager, @@ -160,8 +206,9 @@ async function main(): Promise { clientId: flowConfig.clientId, clientSecret: flowConfig.clientSecret, issuer: GOOGLE_ISSUER, + redirectPort, }, - authorizationServer, + authServer, }) : undefined; @@ -179,8 +226,16 @@ async function main(): Promise { process.on('SIGTERM', shutdown); } -main().catch((err) => { - const msg = err instanceof Error ? (err.stack ?? err.message) : String(err); - console.error('Fatal error:', redactTokens(msg)); - process.exit(1); -}); +// Run only when executed as the binary, not when imported (e.g. by +// unit tests exercising `parseRedirectPort`). +const invokedDirectly = + process.argv[1] !== undefined && + import.meta.url === pathToFileURL(process.argv[1]).href; + +if (invokedDirectly) { + main().catch((err) => { + const msg = err instanceof Error ? (err.stack ?? err.message) : String(err); + console.error('Fatal error:', redactTokens(msg)); + process.exit(1); + }); +} diff --git a/ts-packages/quarto-hub-mcp/src/tools.ts b/ts-packages/quarto-hub-mcp/src/tools.ts index 55358a07..a71305b6 100644 --- a/ts-packages/quarto-hub-mcp/src/tools.ts +++ b/ts-packages/quarto-hub-mcp/src/tools.ts @@ -18,9 +18,10 @@ import { ConnectionManager } from './connection-manager.js'; import { AUTH_TOOL_DEFINITIONS, AuthToolsState, + extractAuthContext, type AuthToolName, } from './auth/auth-tools.js'; -import { redactTokens } from './auth/device-flow.js'; +import { redactTokens } from './auth/redact.js'; function text(msg: string): CallToolResult { return { content: [{ type: 'text', text: msg }] }; @@ -43,7 +44,7 @@ function getReadTools(): Tool[] { 'Returns the list of files in the project. ' + 'If the hub requires authentication and no valid credentials are cached, ' + 'this throws an `AuthRequiredError` / `ReauthRequired` — call ' + - '`authenticate_start` to begin the device-flow.', + '`authenticate` to sign in.', inputSchema: { type: 'object', properties: { @@ -380,16 +381,14 @@ export function registerTools( return { tools: allTools }; }); - server.setRequestHandler(CallToolRequestSchema, async (request) => { + server.setRequestHandler(CallToolRequestSchema, async (request, extra) => { const { name, arguments: args } = request.params; if ( authToolsState && - (name === 'authenticate_start' || - name === 'authenticate_finish' || - name === 'authenticate_clear') + (name === 'authenticate' || name === 'authenticate_clear') ) { - return authToolsState.handle(name as AuthToolName); + return authToolsState.handle(name as AuthToolName, extractAuthContext(extra)); } const tool = dataTools.find(t => t.name === name);