Skip to content

Commit 56aced1

Browse files
committed
Clean OAuth setup hygiene
1 parent a7bec94 commit 56aced1

6 files changed

Lines changed: 32 additions & 52 deletions

src/auth/oauth-scope-check.test.ts

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -62,13 +62,16 @@ describe("checkOAuthProviderScope", () => {
6262
expect(expired.access).toBe("refreshed-codex");
6363
});
6464

65-
test("codex: reports a definitive staged refresh rejection as invalid credentials", async () => {
65+
test("codex: blocks a definitive staged refresh rejection", async () => {
6666
const expired = { ...codexTokens, expiresAt: 0 };
6767
stubFetch(() => new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }));
6868

6969
const result = await checkOAuthProviderScope("codex", expired);
7070

71-
expect(result.status).toBe("invalid-credentials");
71+
expect(result.status).toBe("blocked");
72+
if (result.status === "blocked") {
73+
expect(result.message).toMatch(/expired|revoked/i);
74+
}
7275
});
7376

7477
test("codex: reports a transient staged refresh failure as unavailable", async () => {
@@ -82,21 +85,20 @@ describe("checkOAuthProviderScope", () => {
8285
expect(result.status).toBe("unavailable");
8386
});
8487

85-
test("codex: insufficient-scope on a definitive 403", async () => {
88+
test("codex: blocks a definitive 403 without surfacing the raw body", async () => {
8689
stubFetch(() => new Response("forbidden", { status: 403 }));
8790
const result = await checkOAuthProviderScope("codex", codexTokens);
88-
expect(result.status).toBe("insufficient-scope");
89-
if (result.status === "insufficient-scope") {
91+
expect(result.status).toBe("blocked");
92+
if (result.status === "blocked") {
9093
expect(result.message).toMatch(/reconnect/i);
91-
// Must never surface the raw response body.
9294
expect(result.message).not.toContain("forbidden");
9395
}
9496
});
9597

96-
test("codex: insufficient-scope on a definitive 401", async () => {
98+
test("codex: blocks a definitive 401", async () => {
9799
stubFetch(() => new Response("nope", { status: 401 }));
98100
const result = await checkOAuthProviderScope("codex", codexTokens);
99-
expect(result.status).toBe("insufficient-scope");
101+
expect(result.status).toBe("blocked");
100102
});
101103

102104
test("codex: unavailable on a network failure, not blocked", async () => {
@@ -144,13 +146,16 @@ describe("checkOAuthProviderScope", () => {
144146
expect(expired.access).toBe("refreshed-xai");
145147
});
146148

147-
test("xai: reports a definitive staged refresh rejection as invalid credentials", async () => {
149+
test("xai: blocks a definitive staged refresh rejection", async () => {
148150
const expired = { ...xaiTokens, expiresAt: 0 };
149151
stubFetch(() => new Response(JSON.stringify({ error: "revoked" }), { status: 401 }));
150152

151153
const result = await checkOAuthProviderScope("xai", expired);
152154

153-
expect(result.status).toBe("invalid-credentials");
155+
expect(result.status).toBe("blocked");
156+
if (result.status === "blocked") {
157+
expect(result.message).toMatch(/expired|revoked/i);
158+
}
154159
});
155160

156161
test("xai: reports a transient staged refresh failure as unavailable", async () => {
@@ -164,10 +169,10 @@ describe("checkOAuthProviderScope", () => {
164169
expect(result.status).toBe("unavailable");
165170
});
166171

167-
test("xai: insufficient-scope on a definitive 403", async () => {
172+
test("xai: blocks a definitive 403", async () => {
168173
stubFetch(() => new Response("forbidden", { status: 403 }));
169174
const result = await checkOAuthProviderScope("xai", xaiTokens);
170-
expect(result.status).toBe("insufficient-scope");
175+
expect(result.status).toBe("blocked");
171176
});
172177

173178
test("xai: unavailable on a timeout-style abort", async () => {

src/auth/oauth-scope-check.ts

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ export type OAuthScopeCheckKind = "codex" | "xai";
2424

2525
export type OAuthScopeCheckResult =
2626
| { status: "ok" }
27-
| { status: "insufficient-scope"; message: string }
28-
| { status: "invalid-credentials"; message: string }
27+
| { status: "blocked"; message: string }
2928
// The probe could not run to completion (network blip, timeout, rate
3029
// limit, provider hiccup). This must never be treated the same as a
3130
// definitive scope failure — a transient failure must not lock a
@@ -36,7 +35,7 @@ const SCOPE_CHECK_TIMEOUT_MS = 10_000;
3635

3736
function insufficientScope(providerLabel: string): OAuthScopeCheckResult {
3837
return {
39-
status: "insufficient-scope",
38+
status: "blocked",
4039
message:
4140
`Your ${providerLabel} sign-in doesn't carry API access (it looks like a chat-only plan). ` +
4241
`Reconnect ${providerLabel} with an account/plan that includes API access, then try again.`,
@@ -52,7 +51,7 @@ function unavailable(providerLabel: string): OAuthScopeCheckResult {
5251

5352
function invalidCredentials(providerLabel: string): OAuthScopeCheckResult {
5453
return {
55-
status: "invalid-credentials",
54+
status: "blocked",
5655
message: `${providerLabel} sign-in expired or was revoked. Reconnect ${providerLabel}, then try again.`,
5756
};
5857
}
@@ -65,22 +64,13 @@ export class OAuthProviderScopeError extends Error {
6564
}
6665

6766
export function isOAuthProviderScopeError(err: unknown): err is OAuthProviderScopeError {
68-
return (
69-
err instanceof OAuthProviderScopeError ||
70-
(typeof err === "object" &&
71-
err !== null &&
72-
"name" in err &&
73-
err.name === "OAuthProviderScopeError")
74-
);
67+
return err instanceof OAuthProviderScopeError;
7568
}
7669

7770
export function isBlockingOAuthScopeCheckResult(
7871
result: OAuthScopeCheckResult,
79-
): result is Extract<
80-
OAuthScopeCheckResult,
81-
{ status: "insufficient-scope" | "invalid-credentials" }
82-
> {
83-
return result.status === "insufficient-scope" || result.status === "invalid-credentials";
72+
): result is Extract<OAuthScopeCheckResult, { status: "blocked" }> {
73+
return result.status === "blocked";
8474
}
8575

8676
function isDefinitiveRefreshAuthRejection(err: unknown): boolean {

src/tui/provider-setup-submit.test.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { withMockedModule } from "../../tests/helpers/mock-module.js";
88

99
// The oauth branch probes real provider scope over the network; stub the
1010
// check so these tests exercise buildProviderSubmitHandler's own branching
11-
// (ok / insufficient-scope / unavailable) without a live call.
11+
// (ok / blocked / unavailable) without a live call.
1212
let scopeCheckResult: OAuthScopeCheckResult = { status: "ok" };
1313
const scopeCheckCalls: unknown[][] = [];
1414
await withMockedModule(
@@ -39,10 +39,9 @@ function stagedCodexOAuth(commit: () => Promise<void> = async () => {}): OAuthRe
3939
return {
4040
kind: "codex",
4141
providerName: "codex/work",
42-
profile: "work",
4342
tokens: stagedCodexTokens,
4443
commit,
45-
} as OAuthResult;
44+
};
4645
}
4746

4847
async function withTempDir(run: (dir: string) => Promise<void>): Promise<void> {
@@ -272,7 +271,7 @@ describe("buildProviderSubmitHandler", () => {
272271
test("fresh insufficient scope persists no credential or restart selection", async () => {
273272
await withTempDir(async (dir) => {
274273
scopeCheckResult = {
275-
status: "insufficient-scope",
274+
status: "blocked",
276275
message: "Your Codex sign-in doesn't carry API access. Reconnect Codex and try again.",
277276
};
278277
const path = join(dir, "settings.json");
@@ -308,7 +307,7 @@ describe("buildProviderSubmitHandler", () => {
308307
test("invalid staged OAuth credentials persist no credential or restart selection", async () => {
309308
await withTempDir(async (dir) => {
310309
scopeCheckResult = {
311-
status: "invalid-credentials",
310+
status: "blocked",
312311
message: "Codex sign-in expired or was revoked. Reconnect Codex, then try again.",
313312
};
314313
const path = join(dir, "settings.json");
@@ -343,7 +342,7 @@ describe("buildProviderSubmitHandler", () => {
343342

344343
test("failed same-name reauthorization preserves the exact durable profile", async () => {
345344
await withTempDir(async (dir) => {
346-
scopeCheckResult = { status: "insufficient-scope", message: "Reconnect Codex." };
345+
scopeCheckResult = { status: "blocked", message: "Reconnect Codex." };
347346
const oldProfile = {
348347
name: "work",
349348
tokens: { access: "old-access", refresh: "old-refresh", expiresAt: 500 },
@@ -420,7 +419,7 @@ describe("buildProviderSubmitHandler", () => {
420419

421420
test("explicit save-anyway skips the scope probe and commits exactly once", async () => {
422421
await withTempDir(async (dir) => {
423-
scopeCheckResult = { status: "insufficient-scope", message: "should never be thrown" };
422+
scopeCheckResult = { status: "blocked", message: "should never be thrown" };
424423
const localPath = localSettingsPath(dir);
425424
const submit = buildProviderSubmitHandler(join(dir, "settings.json"), null, localPath);
426425
let commits = 0;

src/tui/provider-setup-submit.ts

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,9 @@ export function buildProviderSubmitHandler(
5151
const trimmedKey = apiKey.trim();
5252
const selectedModel = model.trim();
5353

54-
// A signed-in subscription provider has no key to test or store in
55-
// settings. Its exchanged credentials remain staged until this path has
56-
// authorized persistence.
57-
//
58-
// Unlike a pasted key, this credential was just issued by the real
59-
// provider's own OAuth server completing a PKCE round-trip — so the
60-
// token is real. That still doesn't confirm it carries usable API scope
61-
// (vs. e.g. a chat-only subscription), which would otherwise surface as
62-
// a confusing first-send auth error with no setup-attributable hint.
63-
// Probe the provider's own catalog endpoint with the issued token before
64-
// treating onboarding as complete: a definitive scope rejection blocks
65-
// the submit with an actionable message (mirrors the API-key path's
66-
// connection test); a check that could not run at all (network blip,
67-
// timeout, rate limit) never blocks — only a proven scope failure does.
54+
// OAuth credentials stay staged until setup validation authorizes durable
55+
// persistence. Definitive API-scope or credential failures block the save;
56+
// inconclusive probe failures do not.
6857
if (oauth !== undefined) {
6958
if (!skipValidation) {
7059
const scopeCheck = await checkOAuthProviderScope(oauth.kind, oauth.tokens);

src/tui/provider-setup.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,6 @@ describe("runProviderSetup sign-in", () => {
498498
expect(seen[0]?.apiKey).toBe("");
499499
expect(opts[0]?.oauth).toMatchObject({
500500
kind: "codex",
501-
profile: "default",
502501
providerName: "codex/default",
503502
tokens: { access: "test-access", refresh: "test-refresh", expiresAt: 10_000 },
504503
});

src/tui/provider-setup.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,6 @@ export interface SubmitOpts {
655655

656656
export interface OAuthResult {
657657
readonly kind: OAuthKind;
658-
readonly profile: string;
659658
readonly tokens: CodexTokens | XaiTokens;
660659
readonly commit: () => Promise<void>;
661660
/** Settings/catalog name the stored profile projects to. */
@@ -1379,7 +1378,6 @@ export async function runProviderSetup(config: ProviderSetupConfig): Promise<boo
13791378
loginError = null;
13801379
const result: OAuthResult = {
13811380
kind,
1382-
profile: staged.profile.name,
13831381
tokens: staged.profile.tokens,
13841382
commit: staged.commit,
13851383
providerName: oauthProviderName(kind, staged.profile.name),

0 commit comments

Comments
 (0)