From f51f7f0f54ce291edcabe94fb9ec5d790567fa98 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 27 Aug 2026 23:35:01 -0700 Subject: [PATCH 1/2] Defer OAuth credential persistence until setup validation --- src/auth/callback-page.test.ts | 8 ++ src/auth/callback-page.ts | 19 +++- src/auth/codex/login.ts | 4 +- src/auth/codex/session.ts | 10 ++ src/auth/codex/usage.ts | 14 ++- src/auth/oauth-scope-check.test.ts | 140 +++++++++++++++++++------- src/auth/oauth-scope-check.ts | 20 ++-- src/auth/oauth/callback-server.ts | 2 +- src/auth/oauth/index.ts | 1 + src/auth/oauth/login.ts | 39 +++++-- src/auth/oauth/oauth.test.ts | 69 ++++++++++++- src/auth/xai/login.ts | 4 +- src/auth/xai/session.ts | 10 ++ src/auth/xai/usage.ts | 11 +- src/tui/onboarding.test.ts | 8 +- src/tui/provider-setup-submit.test.ts | 130 +++++++++++++++++++----- src/tui/provider-setup-submit.ts | 14 +-- src/tui/provider-setup.test.ts | 50 +++++---- src/tui/provider-setup.ts | 34 +++++-- 19 files changed, 453 insertions(+), 134 deletions(-) diff --git a/src/auth/callback-page.test.ts b/src/auth/callback-page.test.ts index d6d17f1ef..360d6b09d 100644 --- a/src/auth/callback-page.test.ts +++ b/src/auth/callback-page.test.ts @@ -7,6 +7,7 @@ import { PRODUCT_SITE_URL, } from "../branding.js"; import { callbackPageHtml, humanizeIdentifier } from "./callback-page.js"; +import { authorizationDoneHtml } from "./oauth/callback-server.js"; describe("humanizeIdentifier", () => { test("machine identifiers lose their separators and lead with a capital", () => { @@ -28,6 +29,13 @@ describe("callbackPageHtml", () => { expect(html).not.toContain("access_denied"); }); + test("provider authorization waits for native setup before claiming connection", () => { + const html = authorizationDoneHtml("Codex"); + expect(html).toContain("Codex authorization received"); + expect(html).toContain("finish setup"); + expect(html).not.toContain("connected successfully"); + }); + test("failure names the server and the humanized reason", () => { const html = callbackPageHtml({ subject: "granola", error: "access_denied" }); expect(html).toContain("Granola failed to connect"); diff --git a/src/auth/callback-page.ts b/src/auth/callback-page.ts index f802540a8..f5da1fcd7 100644 --- a/src/auth/callback-page.ts +++ b/src/auth/callback-page.ts @@ -262,6 +262,8 @@ export interface CallbackPage { readonly subject?: string; /** Why it failed. Omit for the success page. */ readonly error?: string; + /** Authorization succeeded, but the native setup flow still has work to do. */ + readonly pendingSetup?: boolean; } /** @@ -276,19 +278,26 @@ export function callbackPageHtml(page: CallbackPage = {}): string { const failed = page.error !== undefined; const subject = page.subject === undefined ? undefined : escapeHtml(humanizeIdentifier(page.subject)); + const pendingSetup = !failed && page.pendingSetup === true; const tone = failed ? "var(--accent)" : "var(--ok)"; - const label = failed ? "not connected" : "connected"; + const label = failed ? "not connected" : pendingSetup ? "authorization received" : "connected"; const heading = failed ? subject === undefined ? "Authorization did not complete" : `${subject} failed to connect` - : subject === undefined - ? "Authorization complete" - : `${subject} connected successfully`; + : pendingSetup + ? subject === undefined + ? "Authorization received" + : `${subject} authorization received` + : subject === undefined + ? "Authorization complete" + : `${subject} connected successfully`; const reason = escapeHtml(humanizeIdentifier(page.error ?? "")); const body = failed ? `${reason}. Close this tab and try again from ${PRODUCT_NAME}.` - : `You can close this tab and return to ${PRODUCT_NAME}.`; + : pendingSetup + ? `Return to ${PRODUCT_NAME} to finish setup.` + : `You can close this tab and return to ${PRODUCT_NAME}.`; return [ "", '', diff --git a/src/auth/codex/login.ts b/src/auth/codex/login.ts index dcf8f1ccb..91fce0924 100644 --- a/src/auth/codex/login.ts +++ b/src/auth/codex/login.ts @@ -7,11 +7,11 @@ import { import { CODEX_BASE_URL, CODEX_DEFAULT_MODELS } from "./constants.js"; import { startCodexCallbackServer } from "./callback-server.js"; import { buildAuthorizeUrl, exchangeCode } from "./oauth.js"; -import { saveCodexProfile } from "./store.js"; +import { saveCodexProfile, type CodexTokens } from "./store.js"; export { openInBrowser }; -export type CodexLoginHandle = OAuthLoginHandle; +export type CodexLoginHandle = OAuthLoginHandle; export type StartCodexLoginOptions = StartOAuthLoginOptions; // Drive the loopback PKCE login for a Codex profile. diff --git a/src/auth/codex/session.ts b/src/auth/codex/session.ts index f957af0d8..4789ef498 100644 --- a/src/auth/codex/session.ts +++ b/src/auth/codex/session.ts @@ -52,3 +52,13 @@ const session = createTokenSession({ export const isCodexTokenExpired = session.isExpired; export const getValidCodexToken = session.getValidToken; + +export async function refreshStagedCodexTokens( + tokens: CodexTokens, + now: number = Date.now(), +): Promise { + if (!isCodexTokenExpired(tokens, now)) return tokens; + const refreshed = await refreshTokens(tokens.refresh, now); + Object.assign(tokens, refreshed); + return tokens; +} diff --git a/src/auth/codex/usage.ts b/src/auth/codex/usage.ts index b7205d86f..c171093b0 100644 --- a/src/auth/codex/usage.ts +++ b/src/auth/codex/usage.ts @@ -73,17 +73,23 @@ function parseUsage(payload: unknown): CodexUsage { }; } -export async function codexAuthHeaders(profileName: string): Promise> { - const { access, accountId } = await getValidCodexToken(profileName); +export function codexAuthHeadersForToken(token: { + readonly access: string; + readonly accountId?: string | undefined; +}): Record { const headers: Record = { - authorization: `Bearer ${access}`, + authorization: `Bearer ${token.access}`, originator: CODEX_AUTHORIZE_EXTRA_PARAMS["originator"] ?? "codex_cli_rs", "user-agent": `${COMMAND_NAME} (codex_cli_rs/${CODEX_CLIENT_VERSION})`, }; - if (accountId !== undefined) headers["chatgpt-account-id"] = accountId; + if (token.accountId !== undefined) headers["chatgpt-account-id"] = token.accountId; return headers; } +export async function codexAuthHeaders(profileName: string): Promise> { + return codexAuthHeadersForToken(await getValidCodexToken(profileName)); +} + // Fetch the live usage/quota snapshot for a Codex profile. export async function fetchCodexUsage(profileName: string): Promise { const res = await fetch(`${CODEX_BASE_URL}${CODEX_USAGE_PATH}`, { diff --git a/src/auth/oauth-scope-check.test.ts b/src/auth/oauth-scope-check.test.ts index 1a98538b2..85f25402e 100644 --- a/src/auth/oauth-scope-check.test.ts +++ b/src/auth/oauth-scope-check.test.ts @@ -1,33 +1,23 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { withMockedModule } from "../../tests/helpers/mock-module.js"; - -// getValidCodexToken/getValidXaiToken hit the real home-level auth store and -// refresh endpoints; stub the session layer so this test only exercises the -// scope probe's own HTTP call and status classification. Other suites -// (tests/unit/codex-session.test.ts) import the real modules directly, so the -// mocks must be torn down after this file's tests run rather than leaking -// into the rest of the bun test process. -await withMockedModule( - import.meta.resolve("./codex/session.js"), - (real: typeof import("./codex/session.js")) => ({ - ...real, - getValidCodexToken: async () => ({ access: "codex-token", accountId: "acct-1" }), - }), -); -await withMockedModule( - import.meta.resolve("./xai/session.js"), - (real: typeof import("./xai/session.js")) => ({ - ...real, - getValidXaiToken: async () => ({ access: "xai-token" }), - }), -); - -const { checkOAuthProviderScope } = await import("./oauth-scope-check.js"); + +import { checkOAuthProviderScope } from "./oauth-scope-check.js"; const originalFetch = global.fetch; +const codexTokens = { + access: "staged-codex-token", + refresh: "codex-refresh", + expiresAt: Date.now() + 3_600_000, + accountId: "acct-staged", +}; +const xaiTokens = { + access: "staged-xai-token", + refresh: "xai-refresh", + expiresAt: Date.now() + 3_600_000, +}; -function stubFetch(impl: (url: string) => Response | Promise): void { - global.fetch = (async (input: RequestInfo | URL) => impl(String(input))) as typeof fetch; +function stubFetch(impl: (url: string, init?: RequestInit) => Response | Promise): void { + global.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => + impl(String(input), init)) as typeof fetch; } describe("checkOAuthProviderScope", () => { @@ -35,15 +25,55 @@ describe("checkOAuthProviderScope", () => { global.fetch = originalFetch; }); - test("codex: ok when the catalog call succeeds", async () => { - stubFetch(() => new Response(JSON.stringify({ models: ["gpt-5"] }), { status: 200 })); - const result = await checkOAuthProviderScope("codex", "work"); + test("codex: builds the probe from staged tokens", async () => { + stubFetch((_url, init) => { + expect(init?.headers).toMatchObject({ + authorization: "Bearer staged-codex-token", + "chatgpt-account-id": "acct-staged", + }); + return new Response(JSON.stringify({ models: ["gpt-5"] }), { status: 200 }); + }); + const result = await checkOAuthProviderScope("codex", codexTokens); expect(result.status).toBe("ok"); }); + test("codex: refreshes expired staged tokens before classifying the probe", async () => { + const expired = { ...codexTokens, expiresAt: 0 }; + const requests: string[] = []; + stubFetch((url, init) => { + requests.push(url); + if (url.includes("/oauth/token")) { + return new Response(JSON.stringify({ access_token: "refreshed-codex", expires_in: 3600 }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + expect(init?.headers).toMatchObject({ + authorization: "Bearer refreshed-codex", + "chatgpt-account-id": "acct-staged", + }); + return new Response(JSON.stringify({ models: ["gpt-5"] }), { status: 200 }); + }); + + const result = await checkOAuthProviderScope("codex", expired); + + expect(result.status).toBe("ok"); + expect(requests).toHaveLength(2); + expect(expired.access).toBe("refreshed-codex"); + }); + + test("codex: reports an expired staged token refresh failure as unavailable", async () => { + const expired = { ...codexTokens, expiresAt: 0 }; + stubFetch(() => new Response("refresh rejected", { status: 401 })); + + const result = await checkOAuthProviderScope("codex", expired); + + expect(result.status).toBe("unavailable"); + }); + test("codex: insufficient-scope on a definitive 403", async () => { stubFetch(() => new Response("forbidden", { status: 403 })); - const result = await checkOAuthProviderScope("codex", "work"); + const result = await checkOAuthProviderScope("codex", codexTokens); expect(result.status).toBe("insufficient-scope"); if (result.status === "insufficient-scope") { expect(result.message).toMatch(/reconnect/i); @@ -54,7 +84,7 @@ describe("checkOAuthProviderScope", () => { test("codex: insufficient-scope on a definitive 401", async () => { stubFetch(() => new Response("nope", { status: 401 })); - const result = await checkOAuthProviderScope("codex", "work"); + const result = await checkOAuthProviderScope("codex", codexTokens); expect(result.status).toBe("insufficient-scope"); }); @@ -62,25 +92,59 @@ describe("checkOAuthProviderScope", () => { stubFetch(() => { throw new Error("fetch failed"); }); - const result = await checkOAuthProviderScope("codex", "work"); + const result = await checkOAuthProviderScope("codex", codexTokens); expect(result.status).toBe("unavailable"); }); test("codex: unavailable (not scope failure) on a 500", async () => { stubFetch(() => new Response("boom", { status: 500 })); - const result = await checkOAuthProviderScope("codex", "work"); + const result = await checkOAuthProviderScope("codex", codexTokens); expect(result.status).toBe("unavailable"); }); - test("xai: ok when the models call succeeds", async () => { - stubFetch(() => new Response(JSON.stringify({ data: [] }), { status: 200 })); - const result = await checkOAuthProviderScope("xai", "personal"); + test("xai: builds the probe from staged tokens", async () => { + stubFetch((_url, init) => { + expect(init?.headers).toMatchObject({ authorization: "Bearer staged-xai-token" }); + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }); + const result = await checkOAuthProviderScope("xai", xaiTokens); + expect(result.status).toBe("ok"); + }); + + test("xai: refreshes expired staged tokens before classifying the probe", async () => { + const expired = { ...xaiTokens, expiresAt: 0 }; + const requests: string[] = []; + stubFetch((url, init) => { + requests.push(url); + if (url.includes("/oauth2/token")) { + return new Response(JSON.stringify({ access_token: "refreshed-xai", expires_in: 3600 }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + expect(init?.headers).toMatchObject({ authorization: "Bearer refreshed-xai" }); + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }); + + const result = await checkOAuthProviderScope("xai", expired); + expect(result.status).toBe("ok"); + expect(requests).toHaveLength(2); + expect(expired.access).toBe("refreshed-xai"); + }); + + test("xai: reports an expired staged token refresh failure as unavailable", async () => { + const expired = { ...xaiTokens, expiresAt: 0 }; + stubFetch(() => new Response("refresh rejected", { status: 401 })); + + const result = await checkOAuthProviderScope("xai", expired); + + expect(result.status).toBe("unavailable"); }); test("xai: insufficient-scope on a definitive 403", async () => { stubFetch(() => new Response("forbidden", { status: 403 })); - const result = await checkOAuthProviderScope("xai", "personal"); + const result = await checkOAuthProviderScope("xai", xaiTokens); expect(result.status).toBe("insufficient-scope"); }); @@ -88,7 +152,7 @@ describe("checkOAuthProviderScope", () => { stubFetch(() => { throw new DOMException("The operation timed out.", "TimeoutError"); }); - const result = await checkOAuthProviderScope("xai", "personal"); + const result = await checkOAuthProviderScope("xai", xaiTokens); expect(result.status).toBe("unavailable"); }); }); diff --git a/src/auth/oauth-scope-check.ts b/src/auth/oauth-scope-check.ts index cfba41e0b..cf912185a 100644 --- a/src/auth/oauth-scope-check.ts +++ b/src/auth/oauth-scope-check.ts @@ -11,9 +11,13 @@ // status is inspected to classify the result. import { CODEX_BASE_URL, CODEX_MODELS_PATH, CODEX_CLIENT_VERSION } from "./codex/constants.js"; -import { codexAuthHeaders } from "./codex/usage.js"; +import { refreshStagedCodexTokens } from "./codex/session.js"; +import type { CodexTokens } from "./codex/store.js"; +import { codexAuthHeadersForToken } from "./codex/usage.js"; import { XAI_BASE_URL, XAI_TOKEN_TIMEOUT_MS } from "./xai/constants.js"; -import { xaiAuthHeaders } from "./xai/usage.js"; +import { refreshStagedXaiTokens } from "./xai/session.js"; +import type { XaiTokens } from "./xai/store.js"; +import { xaiAuthHeadersForToken } from "./xai/usage.js"; export type OAuthScopeCheckKind = "codex" | "xai"; @@ -53,10 +57,10 @@ function classifyStatus(status: number, providerLabel: string): OAuthScopeCheckR return unavailable(providerLabel); } -async function checkCodexScope(profile: string): Promise { +async function checkCodexScope(tokens: CodexTokens): Promise { const providerLabel = "Codex"; try { - const headers = await codexAuthHeaders(profile); + const headers = codexAuthHeadersForToken(await refreshStagedCodexTokens(tokens)); const url = `${CODEX_BASE_URL}${CODEX_MODELS_PATH}?client_version=${encodeURIComponent(CODEX_CLIENT_VERSION)}`; const res = await fetch(url, { headers, @@ -69,10 +73,10 @@ async function checkCodexScope(profile: string): Promise } } -async function checkXaiScope(profile: string): Promise { +async function checkXaiScope(tokens: XaiTokens): Promise { const providerLabel = "Grok"; try { - const headers = await xaiAuthHeaders(profile); + const headers = xaiAuthHeadersForToken(await refreshStagedXaiTokens(tokens)); const res = await fetch(`${XAI_BASE_URL}/models`, { headers, signal: AbortSignal.timeout(XAI_TOKEN_TIMEOUT_MS), @@ -89,7 +93,7 @@ async function checkXaiScope(profile: string): Promise { // login result alone. export async function checkOAuthProviderScope( kind: OAuthScopeCheckKind, - profile: string, + tokens: CodexTokens | XaiTokens, ): Promise { - return kind === "codex" ? checkCodexScope(profile) : checkXaiScope(profile); + return kind === "codex" ? checkCodexScope(tokens) : checkXaiScope(tokens); } diff --git a/src/auth/oauth/callback-server.ts b/src/auth/oauth/callback-server.ts index 249ee2cd5..c866a120d 100644 --- a/src/auth/oauth/callback-server.ts +++ b/src/auth/oauth/callback-server.ts @@ -121,5 +121,5 @@ export async function startCallbackServer( } export function authorizationDoneHtml(providerName: string): string { - return callbackPageHtml({ subject: providerName }); + return callbackPageHtml({ subject: providerName, pendingSetup: true }); } diff --git a/src/auth/oauth/index.ts b/src/auth/oauth/index.ts index be1412005..b76323bc7 100644 --- a/src/auth/oauth/index.ts +++ b/src/auth/oauth/index.ts @@ -31,6 +31,7 @@ export { startOAuthLogin, type OAuthLoginDeps, type OAuthLoginHandle, + type StagedOAuthProfile, type StartOAuthLoginOptions, } from "./login.js"; export { diff --git a/src/auth/oauth/login.ts b/src/auth/oauth/login.ts index eae4b5f1a..86590768d 100644 --- a/src/auth/oauth/login.ts +++ b/src/auth/oauth/login.ts @@ -1,14 +1,20 @@ import { openInBrowser } from "./browser.js"; import type { CallbackServer } from "./callback-server.js"; import { generatePkce, generateState, type Pkce } from "./pkce.js"; +import type { AuthProfile, BaseTokens } from "./store.js"; -export interface OAuthLoginHandle { +export interface StagedOAuthProfile { + readonly profile: AuthProfile; + readonly commit: () => Promise; +} + +export interface OAuthLoginHandle { // The URL to authorize at — surfaced as a copyable link in the TUI and also // handed to the browser opener. authorizeUrl: string; - // Resolves once the user completes consent and tokens are stored, or rejects - // on error/abort. The resolved name echoes the profile that was saved. - completed: Promise<{ profile: string }>; + // Resolves after consent and exchange with an in-memory profile. The caller + // commits it only once provider setup has authorized durable mutation. + completed: Promise>; // Tear down the callback server (also triggered via the abort signal). cancel: () => void; } @@ -24,7 +30,7 @@ export interface StartOAuthLoginOptions { openBrowser?: boolean; } -export interface OAuthLoginDeps { +export interface OAuthLoginDeps { startCallbackServer: (expectedState: string) => Promise; buildAuthorizeUrl: (pkce: Pkce, state: string) => string; exchangeCode: (code: string, verifier: string, now: number) => Promise; @@ -38,22 +44,35 @@ export interface OAuthLoginDeps { // URL, and return a handle whose `completed` promise resolves after the browser // round-trip and token exchange. The server is always closed, whether the flow // succeeds, fails, or is aborted. -export async function startOAuthLogin( +export async function startOAuthLogin( opts: StartOAuthLoginOptions, deps: OAuthLoginDeps, -): Promise { +): Promise> { const now = opts.now ?? Date.now; const pkce = generatePkce(); const state = generateState(); const server = await deps.startCallbackServer(state); const authorizeUrl = deps.buildAuthorizeUrl(pkce, state); - const completed = (async (): Promise<{ profile: string }> => { + const completed = (async (): Promise> => { try { const code = await server.waitForCode(opts.signal); const tokens = await deps.exchangeCode(code, pkce.verifier, now()); - await deps.saveProfile({ name: opts.profile, tokens, createdAt: now() }, opts.home); - return { profile: opts.profile }; + const profile = { name: opts.profile, tokens, createdAt: now() }; + let committed: Promise | undefined; + return { + profile, + commit: () => { + if (!committed) { + const attempt = deps.saveProfile(profile, opts.home); + committed = attempt; + void attempt.catch(() => { + if (committed === attempt) committed = undefined; + }); + } + return committed; + }, + }; } finally { server.close(); } diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index fcc565851..aa6f225e7 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -4,8 +4,9 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { baseTokensFromResponse, postToken, type OAuthClientConfig } from "./client.js"; +import { startOAuthLogin } from "./login.js"; import { createTokenSession } from "./session.js"; -import { createAuthStore, type BaseTokens } from "./store.js"; +import { createAuthStore, type AuthProfile, type BaseTokens } from "./store.js"; const config: OAuthClientConfig = { clientId: "client-id", @@ -157,6 +158,72 @@ describe("createAuthStore", () => { }); }); +describe("startOAuthLogin", () => { + test("stages the exchanged profile until commit is invoked", async () => { + const saved: AuthProfile[] = []; + let closed = 0; + const tokens = { access: "new-access", refresh: "new-refresh", expiresAt: 10_000 }; + const handle = await startOAuthLogin( + { + profile: "work", + signal: new AbortController().signal, + now: () => 123, + openBrowser: false, + }, + { + startCallbackServer: async () => ({ + waitForCode: async () => "authorization-code", + close: () => { + closed += 1; + }, + }), + buildAuthorizeUrl: () => "https://auth.example.com/authorize", + exchangeCode: async () => tokens, + saveProfile: async (profile) => { + saved.push(profile); + }, + }, + ); + + const staged = await handle.completed; + expect(saved).toEqual([]); + expect(staged.profile).toEqual({ name: "work", tokens, createdAt: 123 }); + expect(closed).toBe(1); + + await Promise.all([staged.commit(), staged.commit()]); + expect(saved).toEqual([staged.profile]); + }); + + test("allows a failed profile commit to be retried", async () => { + let saveAttempts = 0; + const handle = await startOAuthLogin( + { + profile: "work", + signal: new AbortController().signal, + now: () => 123, + openBrowser: false, + }, + { + startCallbackServer: async () => ({ + waitForCode: async () => "authorization-code", + close: () => undefined, + }), + buildAuthorizeUrl: () => "https://auth.example.com/authorize", + exchangeCode: async () => ({ access: "new", refresh: "refresh", expiresAt: 10_000 }), + saveProfile: async () => { + saveAttempts += 1; + if (saveAttempts === 1) throw new Error("transient save failure"); + }, + }, + ); + + const staged = await handle.completed; + await expect(staged.commit()).rejects.toThrow("transient save failure"); + await expect(staged.commit()).resolves.toBeUndefined(); + expect(saveAttempts).toBe(2); + }); +}); + describe("createTokenSession", () => { function makeSession(overrides?: { refreshTokens?: (refreshToken: string, now: number) => Promise; diff --git a/src/auth/xai/login.ts b/src/auth/xai/login.ts index b4689daa6..291a435bb 100644 --- a/src/auth/xai/login.ts +++ b/src/auth/xai/login.ts @@ -6,9 +6,9 @@ import { import { XAI_BASE_URL, XAI_DEFAULT_MODELS } from "./constants.js"; import { startXaiCallbackServer } from "./callback-server.js"; import { buildAuthorizeUrl, exchangeCode } from "./oauth.js"; -import { saveXaiProfile } from "./store.js"; +import { saveXaiProfile, type XaiTokens } from "./store.js"; -export type XaiLoginHandle = OAuthLoginHandle; +export type XaiLoginHandle = OAuthLoginHandle; export type StartXaiLoginOptions = StartOAuthLoginOptions; export async function startXaiLogin(opts: StartXaiLoginOptions): Promise { diff --git a/src/auth/xai/session.ts b/src/auth/xai/session.ts index 936bd2e80..b8aa6085f 100644 --- a/src/auth/xai/session.ts +++ b/src/auth/xai/session.ts @@ -53,3 +53,13 @@ const session = createTokenSession({ export const isXaiTokenExpired = session.isExpired; export const getValidXaiToken = session.getValidToken; + +export async function refreshStagedXaiTokens( + tokens: XaiTokens, + now: number = Date.now(), +): Promise { + if (!isXaiTokenExpired(tokens, now)) return tokens; + const refreshed = await refreshTokens(tokens.refresh, now); + Object.assign(tokens, refreshed); + return tokens; +} diff --git a/src/auth/xai/usage.ts b/src/auth/xai/usage.ts index e2396a5c4..1a787f893 100644 --- a/src/auth/xai/usage.ts +++ b/src/auth/xai/usage.ts @@ -59,19 +59,22 @@ function parseXaiUsage(payload: unknown): XaiUsage { }; } -export async function xaiAuthHeaders(profileName: string): Promise> { - const { access } = await getValidXaiToken(profileName); +export function xaiAuthHeadersForToken(token: { readonly access: string }): Record { const headers: Record = { - authorization: `Bearer ${access}`, + authorization: `Bearer ${token.access}`, "user-agent": XAI_USER_AGENT, "x-grok-client-identifier": XAI_CLIENT_IDENTIFIER, "x-grok-client-version": XAI_CLIENT_VERSION, }; - const userId = xaiUserIdFromAccessToken(access); + const userId = xaiUserIdFromAccessToken(token.access); if (userId !== undefined) headers["x-grok-user-id"] = userId; return headers; } +export async function xaiAuthHeaders(profileName: string): Promise> { + return xaiAuthHeadersForToken(await getValidXaiToken(profileName)); +} + // Fetch the live usage/quota snapshot for an xAI/Grok profile. // On network/HTTP error (common for local grok proxies or unauthenticated), // returns a neutral "unknown" record so the UI can still render a placeholder diff --git a/src/tui/onboarding.test.ts b/src/tui/onboarding.test.ts index ede98ea65..e245af8f5 100644 --- a/src/tui/onboarding.test.ts +++ b/src/tui/onboarding.test.ts @@ -91,7 +91,6 @@ describe("runOnboarding settings source", () => { const config = await unconfiguredConfig(cwd, { cliConfigPath: configPath }); setup = async ({ onSubmit }) => { - await writeXAIAuthProfile(testHome, "work"); await onSubmit( { name: "xai/work", @@ -103,7 +102,12 @@ describe("runOnboarding settings source", () => { () => {}, { skipValidation: true, - oauth: { kind: "xai", profile: "work", providerName: "xai/work" }, + oauth: { + kind: "xai", + providerName: "xai/work", + tokens: { access: "work-access-token", refresh: "work-refresh-token", expiresAt: 0 }, + commit: () => writeXAIAuthProfile(testHome, "work"), + }, }, ); }; diff --git a/src/tui/provider-setup-submit.test.ts b/src/tui/provider-setup-submit.test.ts index 4786aeb0d..4840f383c 100644 --- a/src/tui/provider-setup-submit.test.ts +++ b/src/tui/provider-setup-submit.test.ts @@ -10,20 +10,40 @@ import { withMockedModule } from "../../tests/helpers/mock-module.js"; // check so these tests exercise buildProviderSubmitHandler's own branching // (ok / insufficient-scope / unavailable) without a live call. let scopeCheckResult: OAuthScopeCheckResult = { status: "ok" }; +const scopeCheckCalls: unknown[][] = []; await withMockedModule( import.meta.resolve("../auth/oauth-scope-check.js"), (real: typeof import("../auth/oauth-scope-check.js")) => ({ ...real, - checkOAuthProviderScope: async () => scopeCheckResult, + checkOAuthProviderScope: async (...args: unknown[]) => { + scopeCheckCalls.push(args); + return scopeCheckResult; + }, }), ); const { buildProviderSubmitHandler } = await import("./provider-setup-submit.js"); const { loadLocalSettings, loadSettings, localSettingsPath, resolveLocalSettingsPath } = await import("../config/settings.js"); -import type { ProviderFormValues, SubmitPhase } from "./provider-setup.js"; +import type { OAuthResult, ProviderFormValues, SubmitPhase } from "./provider-setup.js"; const noopSetPhase = (_phase: SubmitPhase): void => {}; +const stagedCodexTokens = { + access: "staged-access", + refresh: "staged-refresh", + expiresAt: 10_000, + accountId: "staged-account", +}; + +function stagedCodexOAuth(commit: () => Promise = async () => {}): OAuthResult { + return { + kind: "codex", + providerName: "codex/work", + profile: "work", + tokens: stagedCodexTokens, + commit, + } as OAuthResult; +} async function withTempDir(run: (dir: string) => Promise): Promise { const dir = await mkdtemp(join(tmpdir(), "provider-setup-submit-")); @@ -137,7 +157,7 @@ describe("buildProviderSubmitHandler", () => { }, options: { skipValidation: true, - oauth: { kind: "codex" as const, providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(), }, provider: "codex/work", }, @@ -233,7 +253,7 @@ describe("buildProviderSubmitHandler", () => { await submit(values, noopSetPhase, { skipValidation: true, - oauth: { kind: "codex", providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(), }); const local = await loadLocalSettings(localPath); @@ -285,14 +305,16 @@ describe("buildProviderSubmitHandler", () => { describe("OAuth-issued token scope validation (CL-5710)", () => { afterEach(() => { scopeCheckResult = { status: "ok" }; + scopeCheckCalls.length = 0; }); - test("valid scope: onboarding completes", async () => { + test("valid scope: onboarding commits staged credentials exactly once", async () => { await withTempDir(async (dir) => { scopeCheckResult = { status: "ok" }; const path = join(dir, "settings.json"); const localPath = localSettingsPath(dir); const submit = buildProviderSubmitHandler(path, null, localPath); + let commits = 0; await submit( { @@ -305,16 +327,22 @@ describe("buildProviderSubmitHandler", () => { noopSetPhase, { skipValidation: false, - oauth: { kind: "codex", providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(async () => { + commits += 1; + }), }, ); - const local = await loadLocalSettings(localPath); - expect(local).toEqual({ provider: "codex/work", model: "gpt-5" }); + expect(commits).toBe(1); + expect(scopeCheckCalls).toEqual([["codex", stagedCodexTokens]]); + expect(await loadLocalSettings(localPath)).toEqual({ + provider: "codex/work", + model: "gpt-5", + }); }); }); - test("definitively insufficient scope: onboarding is rejected with a setup-attributable message, not a raw adapter error", async () => { + test("fresh insufficient scope persists no credential or restart selection", async () => { await withTempDir(async (dir) => { scopeCheckResult = { status: "insufficient-scope", @@ -323,6 +351,7 @@ describe("buildProviderSubmitHandler", () => { const path = join(dir, "settings.json"); const localPath = localSettingsPath(dir); const submit = buildProviderSubmitHandler(path, null, localPath); + let committedProfile: string | undefined; await expect( submit( @@ -336,26 +365,70 @@ describe("buildProviderSubmitHandler", () => { noopSetPhase, { skipValidation: false, - oauth: { kind: "codex", providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(async () => { + committedProfile = "work"; + }), }, ), ).rejects.toThrow(/reconnect codex/i); - // Nothing is persisted on a proven scope failure. + expect(committedProfile).toBeUndefined(); expect(await loadSettings(path)).toBeNull(); expect(await loadLocalSettings(localPath)).toBeNull(); }); }); - test("check-unavailable (network blip): onboarding still completes, not blocked", async () => { + test("failed same-name reauthorization preserves the exact durable profile", async () => { + await withTempDir(async (dir) => { + scopeCheckResult = { status: "insufficient-scope", message: "Reconnect Codex." }; + const oldProfile = { + name: "work", + tokens: { access: "old-access", refresh: "old-refresh", expiresAt: 500 }, + createdAt: 10, + }; + let durableProfile = structuredClone(oldProfile); + const submit = buildProviderSubmitHandler( + join(dir, "settings.json"), + null, + localSettingsPath(dir), + ); + + await expect( + submit( + { + name: "", + baseURL: "https://chatgpt.com/backend-api", + apiKey: "", + model: "gpt-5", + oauthProfile: "work", + }, + noopSetPhase, + { + skipValidation: false, + oauth: stagedCodexOAuth(async () => { + durableProfile = { + name: "work", + tokens: stagedCodexTokens, + createdAt: 20, + }; + }), + }, + ), + ).rejects.toThrow(/reconnect codex/i); + + expect(durableProfile).toEqual(oldProfile); + }); + }); + + test("check-unavailable commits staged credentials exactly once", async () => { await withTempDir(async (dir) => { scopeCheckResult = { status: "unavailable", message: "Couldn't confirm Codex API access right now.", }; - const path = join(dir, "settings.json"); const localPath = localSettingsPath(dir); - const submit = buildProviderSubmitHandler(path, null, localPath); + const submit = buildProviderSubmitHandler(join(dir, "settings.json"), null, localPath); + let commits = 0; await submit( { @@ -368,21 +441,26 @@ describe("buildProviderSubmitHandler", () => { noopSetPhase, { skipValidation: false, - oauth: { kind: "codex", providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(async () => { + commits += 1; + }), }, ); - const local = await loadLocalSettings(localPath); - expect(local).toEqual({ provider: "codex/work", model: "gpt-5" }); + expect(commits).toBe(1); + expect(await loadLocalSettings(localPath)).toEqual({ + provider: "codex/work", + model: "gpt-5", + }); }); }); - test("skipValidation bypasses the scope probe entirely", async () => { + test("explicit save-anyway skips the scope probe and commits exactly once", async () => { await withTempDir(async (dir) => { scopeCheckResult = { status: "insufficient-scope", message: "should never be thrown" }; - const path = join(dir, "settings.json"); const localPath = localSettingsPath(dir); - const submit = buildProviderSubmitHandler(path, null, localPath); + const submit = buildProviderSubmitHandler(join(dir, "settings.json"), null, localPath); + let commits = 0; await submit( { @@ -395,12 +473,18 @@ describe("buildProviderSubmitHandler", () => { noopSetPhase, { skipValidation: true, - oauth: { kind: "codex", providerName: "codex/work", profile: "work" }, + oauth: stagedCodexOAuth(async () => { + commits += 1; + }), }, ); - const local = await loadLocalSettings(localPath); - expect(local).toEqual({ provider: "codex/work", model: "gpt-5" }); + expect(scopeCheckCalls).toEqual([]); + expect(commits).toBe(1); + expect(await loadLocalSettings(localPath)).toEqual({ + provider: "codex/work", + model: "gpt-5", + }); }); }); }); diff --git a/src/tui/provider-setup-submit.ts b/src/tui/provider-setup-submit.ts index d8a9d5425..366d25634 100644 --- a/src/tui/provider-setup-submit.ts +++ b/src/tui/provider-setup-submit.ts @@ -48,11 +48,12 @@ export function buildProviderSubmitHandler( const trimmedKey = apiKey.trim(); const selectedModel = model.trim(); - // A signed-in subscription provider has no key to test or store: the - // tokens are already in the home-level auth store, and config load - // projects that store into the provider catalog. Persist only non-secret - // provider/model metadata globally so the selection survives when a local - // settings target would alias this file. + // A signed-in subscription provider has no key to test or store in + // settings. Its exchanged credentials remain staged until this path has + // authorized persistence; once committed to the home-level auth store, + // config load projects that store into the provider catalog. Persist only + // non-secret provider/model metadata globally so the selection survives + // when a local settings target would alias this file. // // Unlike a pasted key, this credential was just issued by the real // provider's own OAuth server completing a PKCE round-trip — so the @@ -66,12 +67,13 @@ export function buildProviderSubmitHandler( // timeout, rate limit) never blocks — only a proven scope failure does. if (oauth !== undefined) { if (!skipValidation) { - const scopeCheck = await checkOAuthProviderScope(oauth.kind, oauth.profile); + const scopeCheck = await checkOAuthProviderScope(oauth.kind, oauth.tokens); if (scopeCheck.status === "insufficient-scope") { throw new Error(scopeCheck.message); } } setPhase("saving"); + await oauth.commit(); const base = existing ?? { providers: {} }; await saveGlobalSettings(settingsPath, { ...base, diff --git a/src/tui/provider-setup.test.ts b/src/tui/provider-setup.test.ts index a63ad0b90..4d0d68fa6 100644 --- a/src/tui/provider-setup.test.ts +++ b/src/tui/provider-setup.test.ts @@ -26,6 +26,7 @@ import { summaryRows, TYPE_MODEL_ID, validateOAuthProfileSlug, + type OAuthLoginStart, type OAuthLoginStarter, type OAuthProfileLister, type ProviderFormValues, @@ -41,6 +42,19 @@ const EMPTY: ProviderFormValues = { oauthProfile: "", }; +type LoginCompletion = Awaited; + +function stagedLogin(profile: string): LoginCompletion { + return { + profile: { + name: profile, + tokens: { access: "test-access", refresh: "test-refresh", expiresAt: 10_000 }, + createdAt: 1, + }, + commit: async () => {}, + }; +} + // This file mounts a fresh renderer per test; track every one so a single // afterEach can free them regardless of which assertion in a test fails. const activeHarnesses: Harness[] = []; @@ -422,14 +436,14 @@ describe("runProviderSetup sign-in", () => { test("a subscription provider signs in in place and persists the selection", async () => { const seen: ProviderFormValues[] = []; const opts: SubmitOpts[] = []; - let complete: (result: { profile: string }) => void = () => {}; + let complete: (result: LoginCompletion) => void = () => {}; const { done, harness } = await mountLogin({ start: async ({ kind, profile }) => { expect(kind).toBe("codex"); expect(profile).toBe("default"); return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>((resolve) => { + completed: new Promise((resolve) => { complete = resolve; }), cancel: () => {}, @@ -449,7 +463,7 @@ describe("runProviderSetup sign-in", () => { expect(waiting).toContain("auth.example.com/authorize"); expect(waiting).toContain("waiting for browser sign-in"); - complete({ profile: "default" }); + complete(stagedLogin("default")); await flush(harness); expect(harness.captureCharFrame()).toContain("step 4 of 4"); @@ -459,11 +473,13 @@ describe("runProviderSetup sign-in", () => { expect(seen[0]?.name).toBe("codex/default"); // A signed-in provider never carries a key through the form. expect(seen[0]?.apiKey).toBe(""); - expect(opts[0]?.oauth).toEqual({ + expect(opts[0]?.oauth).toMatchObject({ kind: "codex", profile: "default", providerName: "codex/default", + tokens: { access: "test-access", refresh: "test-refresh", expiresAt: 10_000 }, }); + expect(opts[0]?.oauth?.commit).toBeFunction(); }); test("the entered account name reaches startLogin as the profile slug", async () => { @@ -473,7 +489,7 @@ describe("runProviderSetup sign-in", () => { seenProfiles.push(profile); return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => {}, }; }, @@ -493,7 +509,7 @@ describe("runProviderSetup sign-in", () => { seenProfiles.push(profile); return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => {}, }; }, @@ -518,7 +534,7 @@ describe("runProviderSetup sign-in", () => { seenProfiles.push(profile); return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => {}, }; }, @@ -552,7 +568,7 @@ describe("runProviderSetup sign-in", () => { starts += 1; return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => {}, }; }, @@ -583,7 +599,7 @@ describe("runProviderSetup sign-in", () => { starts += 1; return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => {}, }; }, @@ -610,7 +626,7 @@ describe("runProviderSetup sign-in", () => { completed: starts === 1 ? Promise.reject(new Error("access denied by the user")) - : new Promise<{ profile: string }>(() => {}), + : new Promise(() => {}), cancel: () => {}, }; }, @@ -635,7 +651,7 @@ describe("runProviderSetup sign-in", () => { loginTimeoutMs: 5, start: async () => ({ authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => { cancelled += 1; }, @@ -663,7 +679,7 @@ describe("runProviderSetup sign-in", () => { }); return { authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => { cancelled += 1; }, @@ -693,7 +709,7 @@ describe("runProviderSetup sign-in", () => { completed: seenProfiles.length === 1 ? Promise.reject(new Error("access denied by the user")) - : new Promise<{ profile: string }>(() => {}), + : new Promise(() => {}), cancel: () => {}, }; }, @@ -715,11 +731,11 @@ describe("runProviderSetup sign-in", () => { }); test("a late resolution from an abandoned attempt cannot move the screen", async () => { - let complete: (result: { profile: string }) => void = () => {}; + let complete: (result: LoginCompletion) => void = () => {}; const { done, harness } = await mountLogin({ start: async () => ({ authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>((resolve) => { + completed: new Promise((resolve) => { complete = resolve; }), cancel: () => {}, @@ -728,7 +744,7 @@ describe("runProviderSetup sign-in", () => { await pickRow(harness, PROVIDER_IDS, "codex"); await nameOAuthAccount(harness); await pressEscape(harness); - complete({ profile: "default" }); + complete(stagedLogin("default")); await flush(harness); expect(harness.captureCharFrame()).toContain("step 2 of 4"); harness.pressKey("Ctrl+C"); @@ -962,7 +978,7 @@ describe("runProviderSetup", () => { const { done, harness } = await mountLogin({ start: async () => ({ authorizeUrl: AUTHORIZE_URL, - completed: new Promise<{ profile: string }>(() => {}), + completed: new Promise(() => {}), cancel: () => { cancelled += 1; }, diff --git a/src/tui/provider-setup.ts b/src/tui/provider-setup.ts index e22150684..c1061e602 100644 --- a/src/tui/provider-setup.ts +++ b/src/tui/provider-setup.ts @@ -27,7 +27,10 @@ import { type FirstClassProviderDef, } from "../../packages/first-class-providers/src/index.js"; import { CODEX_BASE_URL, CODEX_DEFAULT_MODELS } from "../auth/codex/constants.js"; +import type { CodexTokens } from "../auth/codex/store.js"; +import type { AuthProfile } from "../auth/oauth/store.js"; import { XAI_BASE_URL, XAI_DEFAULT_MODELS } from "../auth/xai/constants.js"; +import type { XaiTokens } from "../auth/xai/store.js"; import { PRODUCT_NAME } from "../branding.js"; import { codexProviderName } from "../config/codex-providers.js"; import { xaiProviderName } from "../config/xai-providers.js"; @@ -636,17 +639,15 @@ export interface SubmitOpts { * four form values cannot express. */ readonly preset?: ProviderPreset; - /** - * Present when the operator signed in rather than pasting a key. The token - * is already on disk in the auth store by then, so the caller persists the - * selection only — never a credential. - */ + /** Present when the operator exchanged OAuth credentials during setup. */ readonly oauth?: OAuthResult; } export interface OAuthResult { readonly kind: OAuthKind; readonly profile: string; + readonly tokens: CodexTokens | XaiTokens; + readonly commit: () => Promise; /** Settings/catalog name the stored profile projects to. */ readonly providerName: string; } @@ -667,7 +668,10 @@ export type ProviderSetupSubmit = ( /** A login in flight: where to authorize, when it finished, how to abandon it. */ export interface OAuthLoginStart { readonly authorizeUrl: string; - readonly completed: Promise<{ profile: string }>; + readonly completed: Promise<{ + readonly profile: AuthProfile; + readonly commit: () => Promise; + }>; readonly cancel: () => void; } @@ -1351,7 +1355,11 @@ export async function runProviderSetup(config: ProviderSetupConfig): Promise { + const finishLogin = ( + attempt: number, + kind: OAuthKind, + staged: Awaited, + ): void => { if (attempt !== loginAttempt) return; clearLoginTimer(); loginHandle = null; @@ -1359,9 +1367,13 @@ export async function runProviderSetup(config: ProviderSetupConfig): Promise { - finishLogin(attempt, kind, result.profile); + finishLogin(attempt, kind, result); }, (err: unknown) => { failLogin(attempt, err instanceof Error ? err.message : String(err)); From 3efe315a6d545b9f11e8a0eb3ffbe081a62a9e81 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 28 Aug 2026 00:27:11 -0700 Subject: [PATCH 2/2] Block OAuth setup bypass for definitive auth failures Ctrl+S save-anyway must not persist staged Codex/xAI credentials after insufficient-scope or revoked refresh rejection. Transient refresh failures stay inconclusive so existing save-anyway UX remains. --- src/auth/oauth-scope-check.test.ts | 51 +++++++++++++---- src/auth/oauth-scope-check.ts | 41 ++++++++++++-- src/auth/oauth/client.ts | 16 +++++- src/tui/provider-setup-submit.test.ts | 47 ++++++++++++++-- src/tui/provider-setup-submit.ts | 32 ++++------- src/tui/provider-setup.test.ts | 80 ++++++++++++++++++++++++++- src/tui/provider-setup.ts | 18 ++++-- 7 files changed, 234 insertions(+), 51 deletions(-) diff --git a/src/auth/oauth-scope-check.test.ts b/src/auth/oauth-scope-check.test.ts index 85f25402e..21e24de3e 100644 --- a/src/auth/oauth-scope-check.test.ts +++ b/src/auth/oauth-scope-check.test.ts @@ -62,30 +62,43 @@ describe("checkOAuthProviderScope", () => { expect(expired.access).toBe("refreshed-codex"); }); - test("codex: reports an expired staged token refresh failure as unavailable", async () => { + test("codex: blocks a definitive staged refresh rejection", async () => { const expired = { ...codexTokens, expiresAt: 0 }; - stubFetch(() => new Response("refresh rejected", { status: 401 })); + stubFetch(() => new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 })); + + const result = await checkOAuthProviderScope("codex", expired); + + expect(result.status).toBe("blocked"); + if (result.status === "blocked") { + expect(result.message).toMatch(/expired|revoked/i); + } + }); + + test("codex: reports a transient staged refresh failure as unavailable", async () => { + const expired = { ...codexTokens, expiresAt: 0 }; + stubFetch(() => { + throw new Error("network down"); + }); const result = await checkOAuthProviderScope("codex", expired); expect(result.status).toBe("unavailable"); }); - test("codex: insufficient-scope on a definitive 403", async () => { + test("codex: blocks a definitive 403 without surfacing the raw body", async () => { stubFetch(() => new Response("forbidden", { status: 403 })); const result = await checkOAuthProviderScope("codex", codexTokens); - expect(result.status).toBe("insufficient-scope"); - if (result.status === "insufficient-scope") { + expect(result.status).toBe("blocked"); + if (result.status === "blocked") { expect(result.message).toMatch(/reconnect/i); - // Must never surface the raw response body. expect(result.message).not.toContain("forbidden"); } }); - test("codex: insufficient-scope on a definitive 401", async () => { + test("codex: blocks a definitive 401", async () => { stubFetch(() => new Response("nope", { status: 401 })); const result = await checkOAuthProviderScope("codex", codexTokens); - expect(result.status).toBe("insufficient-scope"); + expect(result.status).toBe("blocked"); }); test("codex: unavailable on a network failure, not blocked", async () => { @@ -133,19 +146,33 @@ describe("checkOAuthProviderScope", () => { expect(expired.access).toBe("refreshed-xai"); }); - test("xai: reports an expired staged token refresh failure as unavailable", async () => { + test("xai: blocks a definitive staged refresh rejection", async () => { const expired = { ...xaiTokens, expiresAt: 0 }; - stubFetch(() => new Response("refresh rejected", { status: 401 })); + stubFetch(() => new Response(JSON.stringify({ error: "revoked" }), { status: 401 })); + + const result = await checkOAuthProviderScope("xai", expired); + + expect(result.status).toBe("blocked"); + if (result.status === "blocked") { + expect(result.message).toMatch(/expired|revoked/i); + } + }); + + test("xai: reports a transient staged refresh failure as unavailable", async () => { + const expired = { ...xaiTokens, expiresAt: 0 }; + stubFetch(() => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }); const result = await checkOAuthProviderScope("xai", expired); expect(result.status).toBe("unavailable"); }); - test("xai: insufficient-scope on a definitive 403", async () => { + test("xai: blocks a definitive 403", async () => { stubFetch(() => new Response("forbidden", { status: 403 })); const result = await checkOAuthProviderScope("xai", xaiTokens); - expect(result.status).toBe("insufficient-scope"); + expect(result.status).toBe("blocked"); }); test("xai: unavailable on a timeout-style abort", async () => { diff --git a/src/auth/oauth-scope-check.ts b/src/auth/oauth-scope-check.ts index cf912185a..55f8d3694 100644 --- a/src/auth/oauth-scope-check.ts +++ b/src/auth/oauth-scope-check.ts @@ -18,12 +18,13 @@ import { XAI_BASE_URL, XAI_TOKEN_TIMEOUT_MS } from "./xai/constants.js"; import { refreshStagedXaiTokens } from "./xai/session.js"; import type { XaiTokens } from "./xai/store.js"; import { xaiAuthHeadersForToken } from "./xai/usage.js"; +import { OAuthTokenEndpointError } from "./oauth/client.js"; export type OAuthScopeCheckKind = "codex" | "xai"; export type OAuthScopeCheckResult = | { status: "ok" } - | { status: "insufficient-scope"; message: string } + | { status: "blocked"; message: string } // The probe could not run to completion (network blip, timeout, rate // limit, provider hiccup). This must never be treated the same as a // definitive scope failure — a transient failure must not lock a @@ -34,7 +35,7 @@ const SCOPE_CHECK_TIMEOUT_MS = 10_000; function insufficientScope(providerLabel: string): OAuthScopeCheckResult { return { - status: "insufficient-scope", + status: "blocked", message: `Your ${providerLabel} sign-in doesn't carry API access (it looks like a chat-only plan). ` + `Reconnect ${providerLabel} with an account/plan that includes API access, then try again.`, @@ -48,6 +49,36 @@ function unavailable(providerLabel: string): OAuthScopeCheckResult { }; } +function invalidCredentials(providerLabel: string): OAuthScopeCheckResult { + return { + status: "blocked", + message: `${providerLabel} sign-in expired or was revoked. Reconnect ${providerLabel}, then try again.`, + }; +} + +export class OAuthProviderScopeError extends Error { + constructor(message: string) { + super(message); + this.name = "OAuthProviderScopeError"; + } +} + +export function isOAuthProviderScopeError(err: unknown): err is OAuthProviderScopeError { + return err instanceof OAuthProviderScopeError; +} + +export function isBlockingOAuthScopeCheckResult( + result: OAuthScopeCheckResult, +): result is Extract { + return result.status === "blocked"; +} + +function isDefinitiveRefreshAuthRejection(err: unknown): boolean { + if (!(err instanceof OAuthTokenEndpointError)) return false; + if (err.status === 401 || err.status === 403) return true; + return /invalid_grant|revoked/i.test(err.detail); +} + // 401/403 is the provider definitively rejecting the token for this surface — // treated as a real scope failure. Anything else (429, 5xx, a malformed // response) is inconclusive: it says nothing about whether the token has @@ -68,7 +99,8 @@ async function checkCodexScope(tokens: CodexTokens): Promise }); if (res.ok) return { status: "ok" }; return classifyStatus(res.status, providerLabel); - } catch { + } catch (err) { + if (isDefinitiveRefreshAuthRejection(err)) return invalidCredentials(providerLabel); return unavailable(providerLabel); } } diff --git a/src/auth/oauth/client.ts b/src/auth/oauth/client.ts index d1c5f130f..3ff3cf9da 100644 --- a/src/auth/oauth/client.ts +++ b/src/auth/oauth/client.ts @@ -49,6 +49,18 @@ export const TokenResponseSchema = type({ }); export type TokenResponse = typeof TokenResponseSchema.infer; +export class OAuthTokenEndpointError extends Error { + readonly status: number; + readonly detail: string; + + constructor(label: string, status: number, detail: string) { + super(`${label} token endpoint returned ${String(status)}${detail ? `: ${detail}` : ""}`); + this.name = "OAuthTokenEndpointError"; + this.status = status; + this.detail = detail; + } +} + // Default access-token lifetime when the server omits expires_in. Conservative // so the refresh path engages sooner rather than trusting a stale token. const DEFAULT_EXPIRES_IN_S = 3600; @@ -92,9 +104,7 @@ export async function postToken( }); if (!res.ok) { const detail = await res.text().catch(() => ""); - throw new Error( - `${config.label} token endpoint returned ${String(res.status)}${detail ? `: ${detail}` : ""}`, - ); + throw new OAuthTokenEndpointError(config.label, res.status, detail); } // AbortSignal.timeout throws a DOMException (name "TimeoutError") when it // fires; its message ("The operation timed out") is what callers report. diff --git a/src/tui/provider-setup-submit.test.ts b/src/tui/provider-setup-submit.test.ts index 4840f383c..56cc9070b 100644 --- a/src/tui/provider-setup-submit.test.ts +++ b/src/tui/provider-setup-submit.test.ts @@ -8,7 +8,7 @@ import { withMockedModule } from "../../tests/helpers/mock-module.js"; // The oauth branch probes real provider scope over the network; stub the // check so these tests exercise buildProviderSubmitHandler's own branching -// (ok / insufficient-scope / unavailable) without a live call. +// (ok / blocked / unavailable) without a live call. let scopeCheckResult: OAuthScopeCheckResult = { status: "ok" }; const scopeCheckCalls: unknown[][] = []; await withMockedModule( @@ -39,10 +39,9 @@ function stagedCodexOAuth(commit: () => Promise = async () => {}): OAuthRe return { kind: "codex", providerName: "codex/work", - profile: "work", tokens: stagedCodexTokens, commit, - } as OAuthResult; + }; } async function withTempDir(run: (dir: string) => Promise): Promise { @@ -345,7 +344,7 @@ describe("buildProviderSubmitHandler", () => { test("fresh insufficient scope persists no credential or restart selection", async () => { await withTempDir(async (dir) => { scopeCheckResult = { - status: "insufficient-scope", + status: "blocked", message: "Your Codex sign-in doesn't carry API access. Reconnect Codex and try again.", }; const path = join(dir, "settings.json"); @@ -378,9 +377,45 @@ describe("buildProviderSubmitHandler", () => { }); }); + test("invalid staged OAuth credentials persist no credential or restart selection", async () => { + await withTempDir(async (dir) => { + scopeCheckResult = { + status: "blocked", + message: "Codex sign-in expired or was revoked. Reconnect Codex, then try again.", + }; + const path = join(dir, "settings.json"); + const localPath = localSettingsPath(dir); + const submit = buildProviderSubmitHandler(path, null, localPath); + let commits = 0; + + await expect( + submit( + { + name: "", + baseURL: "https://chatgpt.com/backend-api", + apiKey: "", + model: "gpt-5", + oauthProfile: "work", + }, + noopSetPhase, + { + skipValidation: false, + oauth: stagedCodexOAuth(async () => { + commits += 1; + }), + }, + ), + ).rejects.toThrow(/reconnect codex/i); + + expect(commits).toBe(0); + expect(await loadSettings(path)).toBeNull(); + expect(await loadLocalSettings(localPath)).toBeNull(); + }); + }); + test("failed same-name reauthorization preserves the exact durable profile", async () => { await withTempDir(async (dir) => { - scopeCheckResult = { status: "insufficient-scope", message: "Reconnect Codex." }; + scopeCheckResult = { status: "blocked", message: "Reconnect Codex." }; const oldProfile = { name: "work", tokens: { access: "old-access", refresh: "old-refresh", expiresAt: 500 }, @@ -457,7 +492,7 @@ describe("buildProviderSubmitHandler", () => { test("explicit save-anyway skips the scope probe and commits exactly once", async () => { await withTempDir(async (dir) => { - scopeCheckResult = { status: "insufficient-scope", message: "should never be thrown" }; + scopeCheckResult = { status: "blocked", message: "should never be thrown" }; const localPath = localSettingsPath(dir); const submit = buildProviderSubmitHandler(join(dir, "settings.json"), null, localPath); let commits = 0; diff --git a/src/tui/provider-setup-submit.ts b/src/tui/provider-setup-submit.ts index 366d25634..d82f964c8 100644 --- a/src/tui/provider-setup-submit.ts +++ b/src/tui/provider-setup-submit.ts @@ -1,4 +1,8 @@ -import { checkOAuthProviderScope } from "../auth/oauth-scope-check.js"; +import { + OAuthProviderScopeError, + checkOAuthProviderScope, + isBlockingOAuthScopeCheckResult, +} from "../auth/oauth-scope-check.js"; import { mergeProviderIntoSettings, saveGlobalSettings, @@ -48,28 +52,16 @@ export function buildProviderSubmitHandler( const trimmedKey = apiKey.trim(); const selectedModel = model.trim(); - // A signed-in subscription provider has no key to test or store in - // settings. Its exchanged credentials remain staged until this path has - // authorized persistence; once committed to the home-level auth store, - // config load projects that store into the provider catalog. Persist only - // non-secret provider/model metadata globally so the selection survives - // when a local settings target would alias this file. - // - // Unlike a pasted key, this credential was just issued by the real - // provider's own OAuth server completing a PKCE round-trip — so the - // token is real. That still doesn't confirm it carries usable API scope - // (vs. e.g. a chat-only subscription), which would otherwise surface as - // a confusing first-send auth error with no setup-attributable hint. - // Probe the provider's own catalog endpoint with the issued token before - // treating onboarding as complete: a definitive scope rejection blocks - // the submit with an actionable message (mirrors the API-key path's - // connection test); a check that could not run at all (network blip, - // timeout, rate limit) never blocks — only a proven scope failure does. + // OAuth credentials stay staged until setup validation authorizes durable + // persistence. Definitive API-scope or credential failures block the save; + // inconclusive probe failures do not. Once committed to the home-level auth + // store, config load projects it into the provider catalog, so only + // non-secret provider/model metadata is persisted globally. if (oauth !== undefined) { if (!skipValidation) { const scopeCheck = await checkOAuthProviderScope(oauth.kind, oauth.tokens); - if (scopeCheck.status === "insufficient-scope") { - throw new Error(scopeCheck.message); + if (isBlockingOAuthScopeCheckResult(scopeCheck)) { + throw new OAuthProviderScopeError(scopeCheck.message); } } setPhase("saving"); diff --git a/src/tui/provider-setup.test.ts b/src/tui/provider-setup.test.ts index 4d0d68fa6..650980c54 100644 --- a/src/tui/provider-setup.test.ts +++ b/src/tui/provider-setup.test.ts @@ -1,5 +1,16 @@ import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { OAuthProviderScopeError } from "../auth/oauth-scope-check.js"; +import { + loadLocalSettings, + loadSettings, + localSettingsPath, + saveGlobalSettings, + saveLocalSettings, +} from "../config/settings.js"; import { createHarness as createRawHarness, type Harness } from "./harness.js"; import { addProviderSelectorChoices, @@ -289,6 +300,9 @@ describe("provider setup pure helpers", () => { test("failures say what to fix", () => { expect(failureGuidance("testing", null)).toContain("base url"); expect(failureGuidance("saving", null)).toContain("settings could not be written"); + expect(failureGuidance("testing", providerChoiceById("codex") ?? null, false)).not.toContain( + "save anyway", + ); }); }); @@ -371,6 +385,15 @@ async function flush(harness: Harness): Promise { * injects a profile lister so no test touches the real auth-store files; * it defaults to reporting no existing profiles. */ +async function withTempDir(run: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), "provider-setup-")); + try { + await run(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + async function mountLogin(opts: { start: OAuthLoginStarter; onSubmit?: ProviderSetupSubmit; @@ -475,13 +498,68 @@ describe("runProviderSetup sign-in", () => { expect(seen[0]?.apiKey).toBe(""); expect(opts[0]?.oauth).toMatchObject({ kind: "codex", - profile: "default", providerName: "codex/default", tokens: { access: "test-access", refresh: "test-refresh", expiresAt: 10_000 }, }); expect(opts[0]?.oauth?.commit).toBeFunction(); }); + test("definitive OAuth scope failure cannot be saved anyway", async () => { + await withTempDir(async (dir) => { + const settingsPath = join(dir, "settings.json"); + const localPath = localSettingsPath(dir); + let commits = 0; + let complete: (result: LoginCompletion) => void = () => {}; + const { done, harness } = await mountLogin({ + start: async () => ({ + authorizeUrl: AUTHORIZE_URL, + completed: new Promise((resolve) => { + complete = resolve; + }), + cancel: () => {}, + }), + onSubmit: async (values, _setPhase, opts) => { + if (opts.oauth === undefined) throw new Error("expected staged OAuth credentials"); + if (!opts.skipValidation) { + throw new OAuthProviderScopeError("Reconnect Codex with API access."); + } + await opts.oauth.commit(); + await saveGlobalSettings(settingsPath, { + providers: {}, + defaultProvider: opts.oauth.providerName, + }); + await saveLocalSettings(localPath, { + provider: opts.oauth.providerName, + model: values.model, + }); + }, + }); + + await pickRow(harness, PROVIDER_IDS, "codex"); + await nameOAuthAccount(harness); + complete({ + ...stagedLogin("default"), + commit: async () => { + commits += 1; + }, + }); + await flush(harness); + harness.pressKey("Enter"); + await flush(harness); + + const frame = harness.captureCharFrame(); + expect(frame).toContain("reconnect codex"); + expect(frame).not.toContain("save anyway"); + harness.pressKey("s", { ctrl: true }); + await flush(harness); + expect(commits).toBe(0); + expect(await loadSettings(settingsPath)).toBeNull(); + expect(await loadLocalSettings(localPath)).toBeNull(); + harness.pressKey("Ctrl+C"); + expect(await done).toBe(false); + }); + }); + test("the entered account name reaches startLogin as the profile slug", async () => { const seenProfiles: string[] = []; const { done, harness } = await mountLogin({ diff --git a/src/tui/provider-setup.ts b/src/tui/provider-setup.ts index c1061e602..b6273233a 100644 --- a/src/tui/provider-setup.ts +++ b/src/tui/provider-setup.ts @@ -27,6 +27,7 @@ import { type FirstClassProviderDef, } from "../../packages/first-class-providers/src/index.js"; import { CODEX_BASE_URL, CODEX_DEFAULT_MODELS } from "../auth/codex/constants.js"; +import { isOAuthProviderScopeError } from "../auth/oauth-scope-check.js"; import type { CodexTokens } from "../auth/codex/store.js"; import type { AuthProfile } from "../auth/oauth/store.js"; import { XAI_BASE_URL, XAI_DEFAULT_MODELS } from "../auth/xai/constants.js"; @@ -598,10 +599,19 @@ export function summaryColor(row: SummaryRow): string { * What the operator should do about a failure. A bare error message leaves a * first-run user stuck, so every failure names the field to fix. */ -export function failureGuidance(phase: SubmitPhase, choice: ProviderChoice | null): string { +export function failureGuidance( + phase: SubmitPhase, + choice: ProviderChoice | null, + offerSaveAnyway = true, +): string { if (phase === "saving") { return "settings could not be written — check disk permissions, enter to retry"; } + if (!offerSaveAnyway) { + return choice !== null && !choice.custom + ? "the account cannot be saved — esc to reconnect or enter to retry" + : "check the base url and key — esc to go back, enter to retry"; + } return choice !== null && !choice.custom ? "the key was rejected or unreachable — esc to re-enter it, enter to retry, ctrl+s to save anyway" : "check the base url and key — esc to go back, enter to retry, ctrl+s to save anyway"; @@ -645,7 +655,6 @@ export interface SubmitOpts { export interface OAuthResult { readonly kind: OAuthKind; - readonly profile: string; readonly tokens: CodexTokens | XaiTokens; readonly commit: () => Promise; /** Settings/catalog name the stored profile projects to. */ @@ -1181,7 +1190,7 @@ export async function runProviderSetup(config: ProviderSetupConfig): Promise