diff --git a/AGENTS.md b/AGENTS.md index 148de05..dcba975 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ During `auth login` (see below), WSL2 usually can't open a browser, so the CLI p - **Credentials are per environment.** One `credentials.toml`, one slot per environment, each with its own token pair. Signing into one leaves the other untouched, and `auth logout` only clears the environment you name. Nothing about the active environment is inferable from a command that succeeds, so read it off `gusto auth whoami`'s `environment` field rather than assuming. - **Auth failures name the environment.** They exit `3` and carry `error.environment`. Stored-session failures also name the credential slot they read and may hint when the other environment has a potentially usable session. - **Stored-session codes have different recoveries.** `no_access_token` means no access token is available; `session_expired` means it cannot be refreshed locally; `token_refresh_failed` means the refresh endpoint rejected an attempt while the refresh token remains stored. Follow `error.message`: transient failures call for a retry, `invalid_grant` calls for login, and a rejected client registration calls for logout before login. -- **`credential_rejected` means the API refused the credential.** A bare retry sends the same token again. Follow the source-specific recovery in `error.message`: sign in again for a stored session, or replace a token supplied through `GUSTO_ACCESS_TOKEN` or `--token-stdin`. +- **`credential_rejected` means the API refused the credential.** A bare retry won't help either way - an explicit token sends the same value again, and a stored session may have already rotated once this command and still failed. Follow the source-specific recovery in `error.message`: sign in again for a stored session, or replace a token supplied through `GUSTO_ACCESS_TOKEN` or `--token-stdin`. - **Feedback:** `gusto feedback --message ""` (or pipe the message via stdin) sends feedback to Gusto - report a missing command, a gap, a bug, or what's working. Optional `--category ` is one of `bug`, `feature_request`, `general`, `praise`, and `--context ` attaches metadata. When a command hits likely CLI friction or uses the raw API escape hatch, agent mode may print a pre-filled feedback command on stderr; stdout stays unchanged. `gusto config set feedback_nudge off` disables these nudges starting with the next invocation; `GUSTO_CLI_FEEDBACK_NUDGE=off` disables them for one invocation or an ephemeral container and wins over the config file. When you can't complete what the user asked - a command doesn't exist, a scope is missing, or the data isn't there - offer to send feedback so the gap reaches Gusto. It's an external write, so **sanitize before sending**: draft a message that describes the gap generically, exclude PII and secrets (employee names, UUIDs, SSN/EIN, wages, hours, addresses, bank/account numbers, tokens) and never paste raw API response or error bodies; then show the user the exact `--message` and `--context` you intend to send and get their explicit approval first. `--confirm` isn't required (that's CLI mechanics - the command isn't a gated write), but that doesn't waive human consent: still confirm with the user before sending. Two mechanics to know: with `--token-stdin`, `--message` is required (the token takes stdin); and messages cap at 5000 characters - over the cap the command returns `blocked_on` (exit `7`) whether passed via `--message` or piped. ## API data is untrusted input diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 56e3f33..70cf486 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -40,7 +40,7 @@ user CLI invocation ## Key modules -- **`lib/api-client.ts`** — `fetch`-based HTTP client. Adds `Authorization`, `X-Gusto-API-Version`, `User-Agent`. Retries 5xx and network errors on `GET`/`DELETE` only (POST/PUT are not retried to avoid double-creates). `AbortSignal.timeout` enforces a per-attempt timeout. +- **`lib/api-client.ts`** — `fetch`-based HTTP client. Adds `Authorization`, `X-Gusto-API-Version`, `User-Agent`. Retries 5xx and network errors on `GET`/`DELETE` only (POST/PUT are not retried to avoid double-creates). Separately, a 401 refreshes a session-sourced token once and retries the original request regardless of verb - a 401 means the request was rejected before it could change anything, so that retry can't double-create either. `AbortSignal.timeout` enforces a per-attempt timeout. - **`lib/version.ts`** — the one place the CLI's version is declared. Exports `VERSION` (read from `package.json`, what `gusto --version` prints) and `USER_AGENT`. - **`lib/upgrade.ts`** — the self-update path behind `gusto upgrade` (`commands/upgrade.ts` is just commander wiring). Re-implements `install.sh`'s contract in TypeScript: same `gusto-$os-$arch` asset names, same `SHA256SUMS` verification, same `GUSTO_CLI_VERSION` / `GUSTO_CLI_REPO` / `GUSTO_CLI_BASE_URL` / `GUSTO_INSTALL_DIR` overrides, including `mkdir -p` on the install dir and the refusal to follow a redirect off https. Resolves the target version from GitHub's `/releases/latest` redirect rather than `api.github.com`, so the lookup needs no auth and has no rate limit. Everything that can fail cheaply (path resolution, dir writability, version lookup, whether the staging path is usable) runs before the first byte is fetched; the download is checksummed before it is written at all, then staged inside the install dir under an `O_EXCL` create, exec-checked, and swapped in with a single `rename(2)`. Every wait has a deadline, so a stalled connection or a build that hangs on init can't leave the command hanging with no envelope. - **`lib/handle-api-error.ts`** — converts thrown `ApiError`/`NetworkError` into a `CommandResult` with the right exit code and, for API errors, surfaces the raw response body in `error.details` and the request id in `error.request_id`. diff --git a/README.md b/README.md index ed43054..776143e 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,7 @@ Auth failures exit `3` and name the environment (`error.environment`). Stored-se | `no_access_token` | no access token is available for that environment | `gusto auth login --env `, set `GUSTO_ACCESS_TOKEN`, or pipe one via `--token-stdin` | | `session_expired` | the access token expired and there's no refresh token (or no client credentials) to renew it | `gusto auth login --env ` | | `token_refresh_failed` | a refresh was attempted and the server rejected it; the stored refresh token is untouched | read the message - it differs by reason (see below) | -| `credential_rejected` | the API answered `401`: the credential was sent and refused, so it's stale, revoked, or for another environment | depends which credential was used, and the message names it - sign in again for a stored session, fix the value for `GUSTO_ACCESS_TOKEN` or `--token-stdin`. A bare retry won't help | +| `credential_rejected` | the API answered `401`: the credential was sent and refused - wrong environment, wrong audience/scope, or (for an explicit token) stale or revoked | depends which credential was used, and the message names it - sign in again for a stored session, fix the value for `GUSTO_ACCESS_TOKEN` or `--token-stdin`. A bare retry won't help | `token_refresh_failed` carries the server's reason, which determines the recovery: diff --git a/src/lib/api-client.test.ts b/src/lib/api-client.test.ts index d36203e..900db3f 100644 --- a/src/lib/api-client.test.ts +++ b/src/lib/api-client.test.ts @@ -725,3 +725,112 @@ describe("ApiClient RequestObserver", () => { expect(stderr.buffer).toBe("> GET /v1/me 200 (12ms) request_id=edge-abc,origin-def\n"); }); }); + +describe("ApiClient reactive refresh on a 401", () => { + test("refreshes once and retries with the new token", async () => { + const seq = sequenceFetch([{ status: 401 }, { status: 200, body: { ok: true } }]); + let refreshCalls = 0; + const client = makeClient(seq.fetch, { + onUnauthorized: async () => { + refreshCalls += 1; + return "refreshed-tok"; + }, + }); + const res = await client.get<{ ok: boolean }>("/v1/me"); + expect(res.body.ok).toBe(true); + expect(refreshCalls).toBe(1); + }); + + test("a second 401 on the retry propagates without a second refresh", async () => { + const seq = sequenceFetch([{ status: 401 }, { status: 401 }]); + let refreshCalls = 0; + const client = makeClient(seq.fetch, { + onUnauthorized: async () => { + refreshCalls += 1; + return "refreshed-tok"; + }, + }); + await expect(client.get("/v1/me")).rejects.toBeInstanceOf(ApiError); + expect(refreshCalls).toBe(1); + }); + + test("concurrent 401s on one client share a single refresh instead of each starting their own", async () => { + const responses = [401, 401, 401, 200, 200, 200]; + const authHeaders: string[] = []; + let calls = 0; + const fetchImpl = (async (_url: string | URL | Request, init?: RequestInit) => { + authHeaders.push((init?.headers as Record).Authorization); + const status = responses[Math.min(calls, responses.length - 1)] ?? 200; + calls += 1; + return new Response(status === 200 ? JSON.stringify({ ok: true }) : "", { + status, + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + let refreshCalls = 0; + const client = makeClient(fetchImpl, { + onUnauthorized: async () => { + refreshCalls += 1; + return "refreshed-tok"; + }, + }); + + const [a, b, c] = await Promise.all([client.get("/v1/a"), client.get("/v1/b"), client.get("/v1/c")]); + + expect(a.body).toEqual({ ok: true }); + expect(b.body).toEqual({ ok: true }); + expect(c.body).toEqual({ ok: true }); + expect(refreshCalls).toBe(1); + expect(calls).toBe(6); + expect(authHeaders.filter((h) => h === "Bearer refreshed-tok")).toHaveLength(3); + }); + + test("a 401 that arrives after a concurrent refresh already finished retries directly, without a second refresh", async () => { + // B's 401 lands only after A's refresh finished - the case the in-flight guard alone misses. + let openGate!: () => void; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + let aCalls = 0; + let bCalls = 0; + const fetchImpl = (async (url: string | URL | Request) => { + const u = url.toString(); + if (u.endsWith("/v1/a")) { + aCalls += 1; + const status = aCalls === 1 ? 401 : 200; + return new Response(status === 200 ? JSON.stringify({ ok: true }) : "", { + status, + headers: { "content-type": "application/json" }, + }); + } + bCalls += 1; + if (bCalls === 1) { + await gate; + return new Response("", { status: 401 }); + } + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + let refreshCalls = 0; + const client = makeClient(fetchImpl, { + onUnauthorized: async () => { + refreshCalls += 1; + return "refreshed-tok"; + }, + }); + + const bPromise = client.get("/v1/b"); + const a = await client.get("/v1/a"); + expect(a.body).toEqual({ ok: true }); + expect(refreshCalls).toBe(1); + + openGate(); + const b = await bPromise; + expect(b.body).toEqual({ ok: true }); + expect(refreshCalls).toBe(1); + }); +}); diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index f346cc0..b87e395 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -13,6 +13,8 @@ export type ResolvedTokenSource = Exclude; export interface AuthContext { tokenSource: TokenSource; environment: Environment; + /** True once a reactive refresh replaced this token, so a later rejection reads as the new token failing. */ + refreshed?: boolean; } export class ApiError extends Error { @@ -149,6 +151,8 @@ export interface ApiClientOptions { /** The credential this client authenticates with, stamped onto any `ApiError` it throws so a 401 * can name what was refused. Omitted by clients built without a resolved context. */ auth?: AuthContext; + /** Called once on a 401 for a replacement token; `null` leaves the 401 as-is. */ + onUnauthorized?: () => Promise; } /** One HTTP attempt as seen by the client. `status` is `0` for a pre-response network fault @@ -177,7 +181,7 @@ export type ReadClient = { get: (p: string) => Promise<{ body: T }> }; export class ApiClient { private readonly baseUrl: string; - private readonly token: string; + private token: string; private readonly apiVersion: string; private readonly installId?: string; private readonly fetchImpl: typeof fetch; @@ -186,7 +190,9 @@ export class ApiClient { private readonly retrySleepMs: (attempt: number) => number; private readonly observer?: RequestObserver; private readonly command?: string; - private readonly auth?: AuthContext; + private auth?: AuthContext; + private readonly onUnauthorized?: () => Promise; + private inFlightRefresh?: Promise; constructor(opts: ApiClientOptions) { this.baseUrl = opts.baseUrl.replace(/\/$/, ""); @@ -201,6 +207,7 @@ export class ApiClient { this.observer = opts.observer; this.command = opts.command; this.auth = opts.auth; + this.onUnauthorized = opts.onUnauthorized; } get(path: string, opts?: RequestOptions): Promise> { @@ -311,6 +318,36 @@ export class ApiClient { path: string, body?: unknown, opts: RequestOptions = {}, + ): Promise> { + const tokenUsed = this.token; + try { + return await this.requestAttempt(method, path, body, opts); + } catch (err) { + if (!(err instanceof ApiError) || err.status !== 401 || !this.onUnauthorized) throw err; + // A concurrent request's refresh may have already moved this.token past tokenUsed - retry with + // what's there now instead of spending a second, redundant refresh. + if (this.token === tokenUsed) { + const refreshed = await this.refreshOnce(this.onUnauthorized); + if (refreshed === null) throw err; + this.token = refreshed; + } + if (this.auth) this.auth = { ...this.auth, refreshed: true }; + return await this.requestAttempt(method, path, body, opts); + } + } + + private refreshOnce(onUnauthorized: () => Promise): Promise { + this.inFlightRefresh ??= onUnauthorized().finally(() => { + this.inFlightRefresh = undefined; + }); + return this.inFlightRefresh; + } + + private async requestAttempt( + method: string, + path: string, + body: unknown, + opts: RequestOptions, ): Promise> { const isIdempotent = IDEMPOTENT_METHODS.has(method.toUpperCase()); const now = opts.now ?? (() => Date.now()); diff --git a/src/lib/api-context.test.ts b/src/lib/api-context.test.ts index bb84df0..9b20a41 100644 --- a/src/lib/api-context.test.ts +++ b/src/lib/api-context.test.ts @@ -1043,6 +1043,121 @@ describe("a 401 from a resolved client", () => { }); }); +// A stored token can look locally valid and still be rejected; only a session token can be refreshed. +describe("reactive refresh on a 401", () => { + let restore: () => void = () => {}; + afterEach(() => restore()); + + const refreshableSlot = (overrides: Record = {}) => ({ + clientId: "cli-id", + clientSecret: "cli-secret", + accessToken: "stale-tok", + refreshToken: "refresh-tok", + expiresAt: 10_000_000, + ...overrides, + }); + + test("a session token with a future expiresAt that the server rejects anyway refreshes once and retries", async () => { + // Also confirms the refresh is persisted, so a later command doesn't repeat the round trip. + const s = stubGlobalFetch([ + { status: 401, body: { error: "unauthorized" } }, + { status: 200, body: { ok: true } }, + ]); + restore = s.restore; + const store = memoryStore({ production: refreshableSlot() }); + const result = await fetchResource( + flags, + { + store, + http: mockHttp({ status: 200, body: { access_token: "fresh-tok", expires_in: 3600 } }), + now: () => 1_000, + }, + () => "/v1/me", + ); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("unreachable"); + expect(result.data).toEqual({ ok: true }); + expect(s.calls).toHaveLength(2); + expect(store.data.production?.accessToken).toBe("fresh-tok"); + }); + + test("a session token with no expiresAt at all still refreshes reactively on a 401", async () => { + // classifySession passes an absent expiresAt through as "unknown" - only a 401 disproves it. + const s = stubGlobalFetch([{ status: 401 }, { status: 200, body: { ok: true } }]); + restore = s.restore; + const store = memoryStore({ + production: { + clientId: "cli-id", + clientSecret: "cli-secret", + accessToken: "stale-tok", + refreshToken: "refresh-tok", + }, + }); + const result = await fetchResource( + flags, + { + store, + http: mockHttp({ status: 200, body: { access_token: "fresh-tok", expires_in: 3600 } }), + now: () => 1_000, + }, + () => "/v1/me", + ); + expect(result.ok).toBe(true); + }); + + test("a failed reactive refresh reports token_refresh_failed, not a bare 401", async () => { + const s = stubGlobalFetch([{ status: 401 }]); + restore = s.restore; + const store = memoryStore({ production: refreshableSlot() }); + const result = await fetchResource( + flags, + { store, http: mockHttp({ status: 400, body: { error: "invalid_grant" } }), now: () => 1_000 }, + () => "/v1/me", + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error.code).toBe("token_refresh_failed"); + expect(result.error.environment).toBe("production"); + }); + + test("a 401 on the retried request reports credential_rejected, not a second refresh attempt", async () => { + const s = stubGlobalFetch([{ status: 401 }, { status: 401 }]); + restore = s.restore; + const store = memoryStore({ production: refreshableSlot() }); + const result = await fetchResource( + flags, + { + store, + http: mockHttp({ status: 200, body: { access_token: "fresh-tok", expires_in: 3600 } }), + now: () => 1_000, + }, + () => "/v1/me", + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error.code).toBe("credential_rejected"); + expect(s.calls).toHaveLength(2); // exactly one retry - no refresh loop + // The rejected token is the refreshed replacement, not the original - "stale" would be wrong. + expect(result.error.message).toContain("refreshed during this command"); + expect(result.error.message).not.toContain("stale"); + }); + + test("an explicit --token-stdin token that 401s never attempts a refresh, even with a refreshable session on file", async () => { + const s = stubGlobalFetch([{ status: 401 }]); + restore = s.restore; + const refreshHttp = mockHttp({ status: 200, body: { access_token: "fresh-tok", expires_in: 3600 } }); + const result = await fetchResource( + flags, + { ...stdinAuth(), store: memoryStore({ production: refreshableSlot() }), http: refreshHttp }, + () => "/v1/me", + ); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("unreachable"); + expect(result.error.code).toBe("credential_rejected"); + expect(s.calls).toHaveLength(1); // no retry + }); +}); + describe("writeResource", () => { let restore: () => void = () => {}; afterEach(() => restore()); diff --git a/src/lib/api-context.ts b/src/lib/api-context.ts index 8fa6b65..1977ef4 100644 --- a/src/lib/api-context.ts +++ b/src/lib/api-context.ts @@ -13,12 +13,12 @@ import { ExitCode } from "./exit-codes.ts"; import type { Environment, GlobalFlags } from "./global-flags.ts"; import { toResult } from "./handle-api-error.ts"; import { oauthHttp } from "./oauth/context.ts"; -import type { OAuthError, OAuthHttpOptions } from "./oauth/endpoints.ts"; -import { type SessionOutcome, resolveSessionToken, sessionUsable } from "./oauth/session.ts"; -import { type TokenStore, credentialsFile, resolveStore } from "./oauth/token-store.ts"; +import type { OAuthHttpOptions } from "./oauth/endpoints.ts"; +import { slotDescription, tokenRefreshFailedError } from "./oauth/refresh-failure.ts"; +import { type SessionOutcome, reactiveRefresh, resolveSessionToken, sessionUsable } from "./oauth/session.ts"; +import { type TokenStore, resolveStore } from "./oauth/token-store.ts"; import type { EnvelopeError } from "./output.ts"; import { isValidUuid, uuidReason } from "./parse.ts"; -import { isObject } from "./predicates.ts"; import { readString } from "./read-string.ts"; import type { CommandResult } from "./runner.ts"; import { readTokenFromStdin } from "./stdin.ts"; @@ -71,6 +71,8 @@ export interface ApiContextOpts extends AuthOpts { * 401 name the credential that was refused. A client built without it still classifies a 401 as an * auth failure, just without naming the source. * + * `onUnauthorized` is the reactive-refresh hook, built by `reactiveRefreshHook`. + * * Not routed through: `oauthApiClient` in `oauth/context.ts` (its own bearer client for * `token_info` during login) - so `auth login --verbose` won't emit the token_info line. Tracked * as a follow-up. */ @@ -82,6 +84,7 @@ export function buildApiClient( installId?: string; stderr?: NodeJS.WritableStream; auth?: AuthContext; + onUnauthorized?: () => Promise; }, ): ApiClient { return new ApiClient({ @@ -92,9 +95,23 @@ export function buildApiClient( observer: globals.verbose ? stderrRequestObserver(opts.stderr ?? process.stderr) : undefined, command: resolveCommandHeader(globals), auth: opts.auth, + onUnauthorized: opts.onUnauthorized, }); } +/** The 401 refresh hook, attached only for a session-sourced token - an explicit env/stdin token + * has no refresh token to use. */ +export function reactiveRefreshHook( + globals: GlobalFlags, + opts: AuthOpts, + tokenSource: ResolvedTokenSource, + environment: Environment, +): (() => Promise) | undefined { + if (tokenSource !== "session") return undefined; + return async () => + reactiveRefresh(opts.store ?? resolveStore(), environment, opts.http ?? (await oauthHttp(globals)), opts.now); +} + export type ResolveFailure = "credentials" | "invalid_input"; type Resolved = { ok: true; ctx: T } | { ok: false; reason: ResolveFailure; result: CommandResult }; @@ -142,65 +159,6 @@ export async function resolveAuthToken(globals: GlobalFlags, opts: AuthOpts): Pr return { ok: false, result: await sessionFailure(outcome, env, opts) }; } -/** Why the token endpoint refused, as it described it. `OAuthError.message` is only the request line - * ("/v1/mcp/oauth/token -> 400"), which names a status but not a cause; RFC 6749 puts the cause in - * the body. Lifted into the message because that is what a caller reads first - `details` still - * carries the whole body. */ -function oauthReason(err: OAuthError): string { - if (!isObject(err.body)) return err.message; - const { error, error_description: description } = err.body; - const parts = [error, description].filter((p): p is string => typeof p === "string" && p.length > 0); - return parts.length > 0 ? `${parts.join(": ")} - ${err.message}` : err.message; -} - -/** Where the failing lookup read from, named so an agent doesn't have to infer it. */ -function slotDescription(env: Environment): string { - return `the [${env}] slot of ${credentialsFile()}`; -} - -/** Classify refresh failures by recovery: retry, replace the grant, replace the client - * registration, fix the request, or avoid guessing. */ -type RefreshFailureReason = "transient" | "grant_rejected" | "client_rejected" | "request_rejected" | "unknown"; - -function refreshFailureReason(err: OAuthError): RefreshFailureReason { - if (isObject(err.body)) { - switch (err.body.error) { - case "invalid_grant": - return "grant_rejected"; - case "invalid_client": - case "unauthorized_client": - return "client_rejected"; - case "invalid_request": - case "unsupported_grant_type": - case "invalid_scope": - return "request_rejected"; - case "server_error": - case "temporarily_unavailable": - return "transient"; - } - } - if (err.status === 0 || err.status >= 500) return "transient"; - return "unknown"; -} - -/** Recommend the least expensive recovery supported by the server's reason. A rejected client - * registration requires logout before login because login otherwise reuses the stored registration. */ -function refreshFailureMessage(err: OAuthError, env: Environment, slot: string): string { - const preamble = `refreshing the ${env} session failed (${oauthReason(err)}).`; - switch (refreshFailureReason(err)) { - case "grant_rejected": - return `${preamble} The server rejected the refresh token in ${slot} as invalid, expired, or revoked, so a retry fails the same way. Run \`gusto auth login --env ${env}\` to sign in again - that replaces the refresh token, which is already dead.`; - case "client_rejected": - return `${preamble} The server rejected this CLI's client registration in ${slot}, not the refresh token, so a retry fails the same way. \`gusto auth login\` reuses that registration and would fail too - clear the slot first with \`gusto auth logout --env ${env}\`, then \`gusto auth login --env ${env}\` to register again. Nothing usable is lost: the credentials in that slot are what just got refused.`; - case "request_rejected": - return `${preamble} The token endpoint rejected the refresh request as invalid or unsupported, so the same request will fail the same way. The credentials in ${slot} are still on file; check \`gusto upgrade --dry-run\`, and report this error if the CLI is current.`; - case "transient": - return `${preamble} The refresh token in ${slot} is still on file and was not replaced - retry the command first. Only run \`gusto auth login --env ${env}\` if the retry fails too, since logging in replaces that refresh token.`; - case "unknown": - return `${preamble} The token endpoint did not identify a recovery, so the CLI will not guess that a retry or login can fix it. The credentials in ${slot} are still on file; check \`gusto upgrade --dry-run\`, and report this error if the CLI is current.`; - } -} - /** Map stored-session outcomes to stable auth error codes and recovery guidance. */ async function sessionFailure( outcome: Exclude, @@ -227,12 +185,7 @@ async function sessionFailure( message: `the ${env} access token expired at ${new Date(outcome.expiresAt).toISOString()} and cannot be refreshed - no refresh token or client credentials in ${slot}. Run \`gusto auth login --env ${env}\` to sign in again.`, }); case "refresh_failed": - return withContext({ - code: "token_refresh_failed", - message: refreshFailureMessage(outcome.cause, env, slot), - ...(outcome.cause.body !== undefined && outcome.cause.body !== null ? { details: outcome.cause.body } : {}), - ...(outcome.cause.requestId ? { request_id: outcome.cause.requestId } : {}), - }); + return withContext(tokenRefreshFailedError(outcome.cause, env)); } } @@ -318,7 +271,13 @@ export async function resolveApiContext( const baseUrl = resolveBaseUrl(globals.env); const environment = defaultEnv(globals.env); const installId = await resolveInstallIdHeader(); - const client = buildApiClient(globals, { baseUrl, token, installId, auth: { tokenSource, environment } }); + const client = buildApiClient(globals, { + baseUrl, + token, + installId, + auth: { tokenSource, environment }, + onUnauthorized: reactiveRefreshHook(globals, opts, tokenSource, environment), + }); if (opts.requireCompany === false) { return { ok: true, ctx: { client, baseUrl, tokenSource, hasCompany: false } }; diff --git a/src/lib/handle-api-error.ts b/src/lib/handle-api-error.ts index 2edc321..32291e0 100644 --- a/src/lib/handle-api-error.ts +++ b/src/lib/handle-api-error.ts @@ -1,16 +1,11 @@ import { ApiError, type AuthContext, BlockedDestinationError, NetworkError } from "./api-client.ts"; import { ExitCode } from "./exit-codes.ts"; import { OAuthError } from "./oauth/endpoints.ts"; +import { TokenRefreshFailedError, tokenRefreshFailedResult } from "./oauth/refresh-failure.ts"; +import { errorExtras } from "./output.ts"; import { isObject } from "./predicates.ts"; import type { CommandResult } from "./runner.ts"; -function errorExtras(err: { body: unknown; requestId?: string }): { details?: unknown; request_id?: string } { - return { - ...(err.body !== undefined && err.body !== null ? { details: err.body } : {}), - ...(err.requestId ? { request_id: err.requestId } : {}), - }; -} - /** Pull a scope name out of a 403 body when the server names one: RFC 6750's top-level `scope` * first, falling back to the `missing_scope_name` Gusto's own `missing_oauth_scopes` error entry * carries in its metadata (absent on older API responses). */ @@ -90,6 +85,10 @@ function rejectedCredential(auth: AuthContext | undefined): string { const env = auth.environment; switch (auth.tokenSource) { case "session": + // The refresh already replaced the token, so the "stale" framing below would be wrong. + if (auth.refreshed) { + return `the ${env} session was refreshed during this command, and the API rejected the replacement token too - it may have been minted for the wrong environment, or for a different audience or scope than this endpoint needs. Run \`gusto auth login --env ${env}\` to sign in again.`; + } return `the stored ${env} session was rejected by the API - its access token is stale or was revoked. Run \`gusto auth login --env ${env}\` to sign in again; that mints a new grant and replaces the refresh token in that slot.`; case "env": return `the token in GUSTO_ACCESS_TOKEN was rejected by the API. It is invalid, expired, or issued for an environment other than ${env}.`; @@ -128,6 +127,7 @@ function insufficientScopeMessage(scope: string | undefined, auth: AuthContext | } export function toResult(err: unknown): CommandResult { + if (err instanceof TokenRefreshFailedError) return tokenRefreshFailedResult(err.cause, err.env); if (err instanceof ApiError) { if (err.status === 401) return credentialRejected(err); if (err.status === 403 && isInsufficientScope(err.body)) { diff --git a/src/lib/mcp.test.ts b/src/lib/mcp.test.ts index aba0336..5f5568b 100644 --- a/src/lib/mcp.test.ts +++ b/src/lib/mcp.test.ts @@ -411,6 +411,36 @@ describe("callMcpTool — HTTP-level failures (via ApiClient → toResult)", () } }); + test("a session token that 401s from the MCP gateway refreshes once and retries, same as a REST command", async () => { + const s = stubGlobalFetch([ + { status: 401, body: { error: "unauthorized" } }, + { status: 200, body: successEnvelope({ source: "refreshed" }) }, + ]); + try { + const result = await callMcpTool( + sandbox, + { + store: memoryStore({ + sandbox: { + clientId: "cli-id", + clientSecret: "cli-secret", + accessToken: "stale-tok", + refreshToken: "refresh-tok", + expiresAt: 4_102_444_800_000, + }, + }), + http: mockHttp({ status: 200, body: { access_token: "fresh-tok", expires_in: 3600 } }), + }, + "list_time_records", + { start_date: "2026-06-01", end_date: "2026-06-15" }, + ); + expect(result.ok).toBe(true); + expect(s.calls).toHaveLength(2); + } finally { + s.restore(); + } + }); + test("a fetch throw (DNS/connection) maps to network_error / Network exit", async () => { const original = globalThis.fetch; globalThis.fetch = (() => Promise.reject(new Error("ECONNREFUSED"))) as unknown as typeof fetch; diff --git a/src/lib/mcp.ts b/src/lib/mcp.ts index 70b9a1f..bcedc11 100644 --- a/src/lib/mcp.ts +++ b/src/lib/mcp.ts @@ -1,4 +1,10 @@ -import { type AuthOpts, type ResolvedTokenSource, buildApiClient, resolveAuthToken } from "./api-context.ts"; +import { + type AuthOpts, + type ResolvedTokenSource, + buildApiClient, + reactiveRefreshHook, + resolveAuthToken, +} from "./api-context.ts"; import { resolveInstallIdHeader } from "./config.ts"; import { defaultEnv, resolveMcpBaseUrl } from "./env.ts"; import { ExitCode } from "./exit-codes.ts"; @@ -49,6 +55,7 @@ export async function callMcpTool( token: resolved.token, installId: await resolveInstallIdHeader(), auth: { tokenSource: resolved.source, environment }, + onUnauthorized: reactiveRefreshHook(globals, opts, resolved.source, environment), }); const body = { diff --git a/src/lib/oauth/refresh-failure.ts b/src/lib/oauth/refresh-failure.ts new file mode 100644 index 0000000..21c8c0d --- /dev/null +++ b/src/lib/oauth/refresh-failure.ts @@ -0,0 +1,94 @@ +import { ExitCode } from "../exit-codes.ts"; +import type { Environment } from "../global-flags.ts"; +import { type EnvelopeError, errorExtras } from "../output.ts"; +import { isObject } from "../predicates.ts"; +import type { CommandResult } from "../runner.ts"; +import { OAuthError } from "./endpoints.ts"; +import { credentialsFile } from "./token-store.ts"; + +/** Thrown when a 401-triggered ("reactive") refresh is itself rejected by the token endpoint. + * Reported the same way a pre-request ("proactive") refresh failure is, so the two can't drift on + * wording. */ +export class TokenRefreshFailedError extends Error { + constructor( + readonly cause: OAuthError, + readonly env: Environment, + ) { + super(`refreshing the ${env} session failed: ${cause.message}`); + this.name = "TokenRefreshFailedError"; + } +} + +/** Why the token endpoint refused, as it described it. `OAuthError.message` is only the request line + * ("/v1/mcp/oauth/token -> 400"), which names a status but not a cause; RFC 6749 puts the cause in + * the body. Lifted into the message because that is what a caller reads first - `details` still + * carries the whole body. */ +function oauthReason(err: OAuthError): string { + if (!isObject(err.body)) return err.message; + const { error, error_description: description } = err.body; + const parts = [error, description].filter((p): p is string => typeof p === "string" && p.length > 0); + return parts.length > 0 ? `${parts.join(": ")} - ${err.message}` : err.message; +} + +/** Where the failing lookup read from, named so an agent doesn't have to infer it. */ +export function slotDescription(env: Environment): string { + return `the [${env}] slot of ${credentialsFile()}`; +} + +/** Classify refresh failures by recovery: retry, replace the grant, replace the client + * registration, fix the request, or avoid guessing. */ +type RefreshFailureReason = "transient" | "grant_rejected" | "client_rejected" | "request_rejected" | "unknown"; + +function refreshFailureReason(err: OAuthError): RefreshFailureReason { + if (isObject(err.body)) { + switch (err.body.error) { + case "invalid_grant": + return "grant_rejected"; + case "invalid_client": + case "unauthorized_client": + return "client_rejected"; + case "invalid_request": + case "unsupported_grant_type": + case "invalid_scope": + return "request_rejected"; + case "server_error": + case "temporarily_unavailable": + return "transient"; + } + } + if (err.status === 0 || err.status >= 500) return "transient"; + return "unknown"; +} + +/** Recommend the least expensive recovery supported by the server's reason. A rejected client + * registration requires logout before login because login otherwise reuses the stored registration. */ +export function refreshFailureMessage(err: OAuthError, env: Environment, slot: string): string { + const preamble = `refreshing the ${env} session failed (${oauthReason(err)}).`; + switch (refreshFailureReason(err)) { + case "grant_rejected": + return `${preamble} The server rejected the refresh token in ${slot} as invalid, expired, or revoked, so a retry fails the same way. Run \`gusto auth login --env ${env}\` to sign in again - that replaces the refresh token, which is already dead.`; + case "client_rejected": + return `${preamble} The server rejected this CLI's client registration in ${slot}, not the refresh token, so a retry fails the same way. \`gusto auth login\` reuses that registration and would fail too - clear the slot first with \`gusto auth logout --env ${env}\`, then \`gusto auth login --env ${env}\` to register again. Nothing usable is lost: the credentials in that slot are what just got refused.`; + case "request_rejected": + return `${preamble} The token endpoint rejected the refresh request as invalid or unsupported, so the same request will fail the same way. The credentials in ${slot} are still on file; check \`gusto upgrade --dry-run\`, and report this error if the CLI is current.`; + case "transient": + return `${preamble} The refresh token in ${slot} is still on file and was not replaced - retry the command first. Only run \`gusto auth login --env ${env}\` if the retry fails too, since logging in replaces that refresh token.`; + case "unknown": + return `${preamble} The token endpoint did not identify a recovery, so the CLI will not guess that a retry or login can fix it. The credentials in ${slot} are still on file; check \`gusto upgrade --dry-run\`, and report this error if the CLI is current.`; + } +} + +export function tokenRefreshFailedError(err: OAuthError, env: Environment): EnvelopeError { + return { + code: "token_refresh_failed", + message: refreshFailureMessage(err, env, slotDescription(env)), + environment: env, + ...errorExtras(err), + }; +} + +/** Split from `tokenRefreshFailedError` so `sessionFailure` can merge in its other-environment hint + * before wrapping the fields into a result - `toResult` has no such hint to add. */ +export function tokenRefreshFailedResult(err: OAuthError, env: Environment): CommandResult { + return { ok: false, exitCode: ExitCode.Auth, error: tokenRefreshFailedError(err, env) }; +} diff --git a/src/lib/oauth/session.test.ts b/src/lib/oauth/session.test.ts index 9be6239..f5e4fcf 100644 --- a/src/lib/oauth/session.test.ts +++ b/src/lib/oauth/session.test.ts @@ -1,7 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { ApiError } from "../api-client.ts"; -import { ExitCode } from "../exit-codes.ts"; -import { NoSessionError, ensureClientCreds, resolveSessionToken, withUserToken } from "./session.ts"; +import { TokenRefreshFailedError } from "./refresh-failure.ts"; +import { ensureClientCreds, reactiveRefresh, resolveSessionToken } from "./session.ts"; import { memoryStore, mockHttp as http } from "./test-support.ts"; describe("resolveSessionToken", () => { @@ -147,85 +146,117 @@ describe("ensureClientCreds", () => { }); }); -describe("withUserToken", () => { - test("throws NoSessionError when not logged in", async () => { - await expect( - withUserToken(memoryStore(), "sandbox", http({ status: 200 }), () => Promise.resolve("x")), - ).rejects.toThrow(NoSessionError); +describe("reactiveRefresh", () => { + const creds = { clientId: "c", clientSecret: "s" }; + + test("uses a session another process refreshed while this refresh was rejected", async () => { + const store = memoryStore({ + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, + }); + const fetchImpl = (async (): Promise => { + await store.save("sandbox", { ...creds, accessToken: "new", refreshToken: "rt2", expiresAt: 20_000_000 }); + return new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, + headers: { "Content-Type": "application/json" }, + }); + }) as unknown as typeof fetch; + + const token = await reactiveRefresh(store, "sandbox", { baseUrl: "https://api.test", fetchImpl }, () => 1_000); + expect(token).toBe("new"); + }); + + test("re-classifies the other process's session rather than trusting it blindly - one more refresh when it's itself still near-expiry", async () => { + // The concurrent write's session is near-expiry too, so trusting its token would fail again. + const store = memoryStore({ + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, + }); + let refreshCalls = 0; + const fetchImpl = (async () => { + refreshCalls += 1; + if (refreshCalls === 1) { + await store.save("sandbox", { ...creds, accessToken: "stale2", refreshToken: "rt2", expiresAt: 1_030_000 }); + return new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }); + } + return new Response(JSON.stringify({ access_token: "fresh", expires_in: 3600 }), { status: 200 }); + }) as unknown as typeof fetch; + + const token = await reactiveRefresh(store, "sandbox", { baseUrl: "https://api.test", fetchImpl }, () => 1_000_000); + expect(token).toBe("fresh"); + expect(refreshCalls).toBe(2); }); - test("refreshes once and retries on a 401", async () => { + test("a rejected refresh with no concurrent change throws TokenRefreshFailedError wrapping the OAuthError", async () => { const store = memoryStore({ - sandbox: { clientId: "c", clientSecret: "s", accessToken: "at", refreshToken: "rt", expiresAt: 9_999_999_999 }, + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, }); - let calls = 0; - const result = await withUserToken( + const result = reactiveRefresh( store, "sandbox", - http({ status: 200, body: { access_token: "refreshed", expires_in: 3600 } }), - (token) => { - calls += 1; - if (calls === 1) { - expect(token).toBe("at"); - throw new ApiError(401, null, ExitCode.Auth, "unauthorized"); - } - expect(token).toBe("refreshed"); - return Promise.resolve("ok"); - }, + http({ status: 400, body: { error: "invalid_grant" } }), () => 1_000, ); - expect(result).toBe("ok"); - expect(calls).toBe(2); + await expect(result).rejects.toBeInstanceOf(TokenRefreshFailedError); + await expect(result).rejects.toMatchObject({ env: "sandbox", cause: expect.objectContaining({ status: 400 }) }); }); - test("rethrows a non-401 ApiError without refreshing", async () => { + test("a second refresh attempt on reconciliation that also fails still throws TokenRefreshFailedError, not a loop", async () => { const store = memoryStore({ - sandbox: { clientId: "c", clientSecret: "s", accessToken: "at", refreshToken: "rt", expiresAt: 9_999_999_999 }, + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, }); + let refreshCalls = 0; + const fetchImpl = (async () => { + refreshCalls += 1; + if (refreshCalls === 1) { + await store.save("sandbox", { ...creds, accessToken: "stale2", refreshToken: "rt2", expiresAt: 1_030_000 }); + } + return new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }); + }) as unknown as typeof fetch; + await expect( - withUserToken( - store, - "sandbox", - http({ status: 200 }), - () => { - throw new ApiError(500, null, ExitCode.General, "server error"); - }, - () => 1_000, - ), - ).rejects.toThrow("server error"); - }); - - test("rethrows the 401 when the session has no refresh token", async () => { + reactiveRefresh(store, "sandbox", { baseUrl: "https://api.test", fetchImpl }, () => 1_000_000), + ).rejects.toBeInstanceOf(TokenRefreshFailedError); + expect(refreshCalls).toBe(2); + }); + + test("a second refresh attempt on reconciliation reports its own failure reason, not the first attempt's", async () => { + // invalid_grant first, then a transient 503 on the reconciled session's own refresh - reporting + // the first reason would tell the caller to discard a refresh token that's still good. const store = memoryStore({ - sandbox: { clientId: "c", clientSecret: "s", accessToken: "at", expiresAt: 9_999_999_999 }, + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, }); - await expect( - withUserToken( - store, - "sandbox", - http({ status: 200 }), - () => { - throw new ApiError(401, null, ExitCode.Auth, "unauthorized"); - }, - () => 1_000, - ), - ).rejects.toThrow("unauthorized"); - }); - - test("propagates when the post-401 refresh itself fails", async () => { + let refreshCalls = 0; + const fetchImpl = (async () => { + refreshCalls += 1; + if (refreshCalls === 1) { + await store.save("sandbox", { ...creds, accessToken: "stale2", refreshToken: "rt2", expiresAt: 1_030_000 }); + return new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }); + } + return new Response(JSON.stringify({ error: "temporarily_unavailable" }), { status: 503 }); + }) as unknown as typeof fetch; + + const result = reactiveRefresh(store, "sandbox", { baseUrl: "https://api.test", fetchImpl }, () => 1_000_000); + await expect(result).rejects.toBeInstanceOf(TokenRefreshFailedError); + await expect(result).rejects.toMatchObject({ cause: expect.objectContaining({ status: 503 }) }); + expect(refreshCalls).toBe(2); + }); + + test("the reconciled session having logged out in the meantime still throws, rather than resolving null", async () => { const store = memoryStore({ - sandbox: { clientId: "c", clientSecret: "s", accessToken: "at", refreshToken: "rt", expiresAt: 9_999_999_999 }, + sandbox: { ...creds, accessToken: "old", refreshToken: "rt", expiresAt: 10_000_000 }, }); + const fetchImpl = (async (): Promise => { + await store.clear("sandbox"); + return new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }); + }) as unknown as typeof fetch; + await expect( - withUserToken( - store, - "sandbox", - http({ status: 400, body: { error: "invalid_grant" } }), // refresh call fails - () => { - throw new ApiError(401, null, ExitCode.Auth, "unauthorized"); - }, - () => 1_000, - ), - ).rejects.toBeDefined(); + reactiveRefresh(store, "sandbox", { baseUrl: "https://api.test", fetchImpl }, () => 1_000), + ).rejects.toBeInstanceOf(TokenRefreshFailedError); + }); + + test("null when there is nothing to refresh with, even after a concurrent-change check would apply", async () => { + const store = memoryStore({ sandbox: { accessToken: "old" } }); + const token = await reactiveRefresh(store, "sandbox", http({ status: 200 }), () => 1_000); + expect(token).toBeNull(); }); }); diff --git a/src/lib/oauth/session.ts b/src/lib/oauth/session.ts index 1ed8ebb..d4d612d 100644 --- a/src/lib/oauth/session.ts +++ b/src/lib/oauth/session.ts @@ -1,19 +1,12 @@ -import { ApiError } from "../api-client.ts"; import { OAuthError, type OAuthHttpOptions } from "./endpoints.ts"; import { registerCliClient } from "./dcr.ts"; import { refreshToken } from "./pkce.ts"; +import { TokenRefreshFailedError } from "./refresh-failure.ts"; import type { TokenStore } from "./token-store.ts"; import { type ClientCreds, type StoredSession, hasClientCreds } from "./types.ts"; export const REFRESH_SKEW_MS = 60_000; -export class NoSessionError extends Error { - constructor() { - super("not logged in. Run `gusto auth login` (or set GUSTO_ACCESS_TOKEN)."); - this.name = "NoSessionError"; - } -} - export async function ensureClientCreds( store: TokenStore, env: "sandbox" | "production", @@ -106,10 +99,8 @@ async function resolveSessionTokenAttempt( try { return { kind: "ok", token: await refreshAndStore(store, env, http, state.session, state.refreshToken, now()) }; } catch (err) { - // Proactive (within-skew) refresh failed while the token is still genuinely valid, so the - // failure isn't actionable yet - use it. There is no reactive refresh: a token that turns out - // to be dead comes back 401 and is reported as `credential_rejected`, not swapped for a fresh - // one. This is the last chance to refresh, so passing it through bets on the token's clock. + // Proactive refresh failed while the token is still valid, so it isn't actionable yet - use it. + // If the token is actually dead, the 401 triggers `onUnauthorized`'s reactive refresh. const expiresAt = state.session.expiresAt; if (expiresAt != null && now() < expiresAt) return { kind: "ok", token: state.token }; if (err instanceof OAuthError) { @@ -146,33 +137,57 @@ function sameAuthState(previous: StoredSession, latest: StoredSession | null): b ); } -/** Run `fn` with the session's token, refreshing on near-expiry and once more if the call comes back - * 401. `NoSessionError` for the two states that need a login (nothing on file, expired with no way to - * renew); the `OAuthError` itself when a refresh ran and the server rejected it, since a caller that - * can't tell that state from absence would answer a still-usable refresh token with a login. - * - * No production caller yet - reactive refresh belongs per-request inside `ApiClient`, not around a - * whole operation, which would replay a paginated walk or a poll. Commands resolve their token - * through `resolveSessionToken` instead, which reports the three failure states apart. */ -export async function withUserToken( +/** The last refresh chance after a 401. Throws `TokenRefreshFailedError` rather than the raw + * `OAuthError`, so callers don't wrap it themselves. */ +export async function reactiveRefresh( store: TokenStore, env: "sandbox" | "production", http: OAuthHttpOptions, - fn: (token: string) => Promise, now: () => number = Date.now, -): Promise { - const outcome = await resolveSessionToken(store, env, http, now); - if (outcome.kind === "refresh_failed") throw outcome.cause; - if (outcome.kind !== "ok") throw new NoSessionError(); - const token = outcome.token; +): Promise { + const session = await store.load(env); + if (!session?.refreshToken || !hasClientCreds(session)) return null; try { - return await fn(token); + return await refreshAndStore(store, env, http, session, session.refreshToken, now()); } catch (err) { - if (!(err instanceof ApiError) || err.status !== 401) throw err; - const session = await store.load(env); - if (!session?.refreshToken || !hasClientCreds(session)) throw err; - const refreshed = await refreshAndStore(store, env, http, session, session.refreshToken, now()); - return fn(refreshed); + if (!(err instanceof OAuthError)) throw err; + return await reconcileAfterFailedRefresh(store, env, http, now, session, err); + } +} + +/** Another `gusto` process may have refreshed while ours failed - re-classify instead of trusting + * it, since the token it stored can be near-expiry too. */ +async function reconcileAfterFailedRefresh( + store: TokenStore, + env: "sandbox" | "production", + http: OAuthHttpOptions, + now: () => number, + previous: StoredSession, + err: OAuthError, +): Promise { + const failed = () => new TokenRefreshFailedError(err, env); + let latest: StoredSession | null; + try { + latest = await store.load(env); + } catch { + throw failed(); + } + if (latest === null || sameAuthState(previous, latest)) throw failed(); + + const state = classifySession(latest, now()); + switch (state.kind) { + case "ok": + return state.token; + case "refreshable": + try { + return await refreshAndStore(store, env, http, state.session, state.refreshToken, now()); + } catch (second) { + // Report this attempt's own failure - a transient blip here isn't the first attempt's rejection. + throw second instanceof OAuthError ? new TokenRefreshFailedError(second, env) : second; + } + case "absent": + case "expired": + throw failed(); } } diff --git a/src/lib/output.ts b/src/lib/output.ts index 3a25e72..a97dc3d 100644 --- a/src/lib/output.ts +++ b/src/lib/output.ts @@ -14,6 +14,13 @@ export interface BlockedOn { reason: string; } +export function errorExtras(err: { body: unknown; requestId?: string }): { details?: unknown; request_id?: string } { + return { + ...(err.body !== undefined && err.body !== null ? { details: err.body } : {}), + ...(err.requestId ? { request_id: err.requestId } : {}), + }; +} + export interface EnvelopeError { code: string; message: string;