Skip to content

Commit 3efe315

Browse files
committed
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.
1 parent f51f7f0 commit 3efe315

7 files changed

Lines changed: 234 additions & 51 deletions

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

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

65-
test("codex: reports an expired staged token refresh failure as unavailable", async () => {
65+
test("codex: blocks a definitive staged refresh rejection", async () => {
6666
const expired = { ...codexTokens, expiresAt: 0 };
67-
stubFetch(() => new Response("refresh rejected", { status: 401 }));
67+
stubFetch(() => new Response(JSON.stringify({ error: "invalid_grant" }), { status: 400 }));
68+
69+
const result = await checkOAuthProviderScope("codex", expired);
70+
71+
expect(result.status).toBe("blocked");
72+
if (result.status === "blocked") {
73+
expect(result.message).toMatch(/expired|revoked/i);
74+
}
75+
});
76+
77+
test("codex: reports a transient staged refresh failure as unavailable", async () => {
78+
const expired = { ...codexTokens, expiresAt: 0 };
79+
stubFetch(() => {
80+
throw new Error("network down");
81+
});
6882

6983
const result = await checkOAuthProviderScope("codex", expired);
7084

7185
expect(result.status).toBe("unavailable");
7286
});
7387

74-
test("codex: insufficient-scope on a definitive 403", async () => {
88+
test("codex: blocks a definitive 403 without surfacing the raw body", async () => {
7589
stubFetch(() => new Response("forbidden", { status: 403 }));
7690
const result = await checkOAuthProviderScope("codex", codexTokens);
77-
expect(result.status).toBe("insufficient-scope");
78-
if (result.status === "insufficient-scope") {
91+
expect(result.status).toBe("blocked");
92+
if (result.status === "blocked") {
7993
expect(result.message).toMatch(/reconnect/i);
80-
// Must never surface the raw response body.
8194
expect(result.message).not.toContain("forbidden");
8295
}
8396
});
8497

85-
test("codex: insufficient-scope on a definitive 401", async () => {
98+
test("codex: blocks a definitive 401", async () => {
8699
stubFetch(() => new Response("nope", { status: 401 }));
87100
const result = await checkOAuthProviderScope("codex", codexTokens);
88-
expect(result.status).toBe("insufficient-scope");
101+
expect(result.status).toBe("blocked");
89102
});
90103

91104
test("codex: unavailable on a network failure, not blocked", async () => {
@@ -133,19 +146,33 @@ describe("checkOAuthProviderScope", () => {
133146
expect(expired.access).toBe("refreshed-xai");
134147
});
135148

136-
test("xai: reports an expired staged token refresh failure as unavailable", async () => {
149+
test("xai: blocks a definitive staged refresh rejection", async () => {
137150
const expired = { ...xaiTokens, expiresAt: 0 };
138-
stubFetch(() => new Response("refresh rejected", { status: 401 }));
151+
stubFetch(() => new Response(JSON.stringify({ error: "revoked" }), { status: 401 }));
152+
153+
const result = await checkOAuthProviderScope("xai", expired);
154+
155+
expect(result.status).toBe("blocked");
156+
if (result.status === "blocked") {
157+
expect(result.message).toMatch(/expired|revoked/i);
158+
}
159+
});
160+
161+
test("xai: reports a transient staged refresh failure as unavailable", async () => {
162+
const expired = { ...xaiTokens, expiresAt: 0 };
163+
stubFetch(() => {
164+
throw new DOMException("The operation timed out.", "TimeoutError");
165+
});
139166

140167
const result = await checkOAuthProviderScope("xai", expired);
141168

142169
expect(result.status).toBe("unavailable");
143170
});
144171

145-
test("xai: insufficient-scope on a definitive 403", async () => {
172+
test("xai: blocks a definitive 403", async () => {
146173
stubFetch(() => new Response("forbidden", { status: 403 }));
147174
const result = await checkOAuthProviderScope("xai", xaiTokens);
148-
expect(result.status).toBe("insufficient-scope");
175+
expect(result.status).toBe("blocked");
149176
});
150177

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

src/auth/oauth-scope-check.ts

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,13 @@ import { XAI_BASE_URL, XAI_TOKEN_TIMEOUT_MS } from "./xai/constants.js";
1818
import { refreshStagedXaiTokens } from "./xai/session.js";
1919
import type { XaiTokens } from "./xai/store.js";
2020
import { xaiAuthHeadersForToken } from "./xai/usage.js";
21+
import { OAuthTokenEndpointError } from "./oauth/client.js";
2122

2223
export type OAuthScopeCheckKind = "codex" | "xai";
2324

2425
export type OAuthScopeCheckResult =
2526
| { status: "ok" }
26-
| { status: "insufficient-scope"; message: string }
27+
| { status: "blocked"; message: string }
2728
// The probe could not run to completion (network blip, timeout, rate
2829
// limit, provider hiccup). This must never be treated the same as a
2930
// definitive scope failure — a transient failure must not lock a
@@ -34,7 +35,7 @@ const SCOPE_CHECK_TIMEOUT_MS = 10_000;
3435

3536
function insufficientScope(providerLabel: string): OAuthScopeCheckResult {
3637
return {
37-
status: "insufficient-scope",
38+
status: "blocked",
3839
message:
3940
`Your ${providerLabel} sign-in doesn't carry API access (it looks like a chat-only plan). ` +
4041
`Reconnect ${providerLabel} with an account/plan that includes API access, then try again.`,
@@ -48,6 +49,36 @@ function unavailable(providerLabel: string): OAuthScopeCheckResult {
4849
};
4950
}
5051

52+
function invalidCredentials(providerLabel: string): OAuthScopeCheckResult {
53+
return {
54+
status: "blocked",
55+
message: `${providerLabel} sign-in expired or was revoked. Reconnect ${providerLabel}, then try again.`,
56+
};
57+
}
58+
59+
export class OAuthProviderScopeError extends Error {
60+
constructor(message: string) {
61+
super(message);
62+
this.name = "OAuthProviderScopeError";
63+
}
64+
}
65+
66+
export function isOAuthProviderScopeError(err: unknown): err is OAuthProviderScopeError {
67+
return err instanceof OAuthProviderScopeError;
68+
}
69+
70+
export function isBlockingOAuthScopeCheckResult(
71+
result: OAuthScopeCheckResult,
72+
): result is Extract<OAuthScopeCheckResult, { status: "blocked" }> {
73+
return result.status === "blocked";
74+
}
75+
76+
function isDefinitiveRefreshAuthRejection(err: unknown): boolean {
77+
if (!(err instanceof OAuthTokenEndpointError)) return false;
78+
if (err.status === 401 || err.status === 403) return true;
79+
return /invalid_grant|revoked/i.test(err.detail);
80+
}
81+
5182
// 401/403 is the provider definitively rejecting the token for this surface —
5283
// treated as a real scope failure. Anything else (429, 5xx, a malformed
5384
// response) is inconclusive: it says nothing about whether the token has
@@ -68,7 +99,8 @@ async function checkCodexScope(tokens: CodexTokens): Promise<OAuthScopeCheckResu
6899
});
69100
if (res.ok) return { status: "ok" };
70101
return classifyStatus(res.status, providerLabel);
71-
} catch {
102+
} catch (err) {
103+
if (isDefinitiveRefreshAuthRejection(err)) return invalidCredentials(providerLabel);
72104
return unavailable(providerLabel);
73105
}
74106
}
@@ -83,7 +115,8 @@ async function checkXaiScope(tokens: XaiTokens): Promise<OAuthScopeCheckResult>
83115
});
84116
if (res.ok) return { status: "ok" };
85117
return classifyStatus(res.status, providerLabel);
86-
} catch {
118+
} catch (err) {
119+
if (isDefinitiveRefreshAuthRejection(err)) return invalidCredentials(providerLabel);
87120
return unavailable(providerLabel);
88121
}
89122
}

src/auth/oauth/client.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,18 @@ export const TokenResponseSchema = type({
4949
});
5050
export type TokenResponse = typeof TokenResponseSchema.infer;
5151

52+
export class OAuthTokenEndpointError extends Error {
53+
readonly status: number;
54+
readonly detail: string;
55+
56+
constructor(label: string, status: number, detail: string) {
57+
super(`${label} token endpoint returned ${String(status)}${detail ? `: ${detail}` : ""}`);
58+
this.name = "OAuthTokenEndpointError";
59+
this.status = status;
60+
this.detail = detail;
61+
}
62+
}
63+
5264
// Default access-token lifetime when the server omits expires_in. Conservative
5365
// so the refresh path engages sooner rather than trusting a stale token.
5466
const DEFAULT_EXPIRES_IN_S = 3600;
@@ -92,9 +104,7 @@ export async function postToken(
92104
});
93105
if (!res.ok) {
94106
const detail = await res.text().catch(() => "");
95-
throw new Error(
96-
`${config.label} token endpoint returned ${String(res.status)}${detail ? `: ${detail}` : ""}`,
97-
);
107+
throw new OAuthTokenEndpointError(config.label, res.status, detail);
98108
}
99109
// AbortSignal.timeout throws a DOMException (name "TimeoutError") when it
100110
// fires; its message ("The operation timed out") is what callers report.

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

Lines changed: 41 additions & 6 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> {
@@ -345,7 +344,7 @@ describe("buildProviderSubmitHandler", () => {
345344
test("fresh insufficient scope persists no credential or restart selection", async () => {
346345
await withTempDir(async (dir) => {
347346
scopeCheckResult = {
348-
status: "insufficient-scope",
347+
status: "blocked",
349348
message: "Your Codex sign-in doesn't carry API access. Reconnect Codex and try again.",
350349
};
351350
const path = join(dir, "settings.json");
@@ -378,9 +377,45 @@ describe("buildProviderSubmitHandler", () => {
378377
});
379378
});
380379

380+
test("invalid staged OAuth credentials persist no credential or restart selection", async () => {
381+
await withTempDir(async (dir) => {
382+
scopeCheckResult = {
383+
status: "blocked",
384+
message: "Codex sign-in expired or was revoked. Reconnect Codex, then try again.",
385+
};
386+
const path = join(dir, "settings.json");
387+
const localPath = localSettingsPath(dir);
388+
const submit = buildProviderSubmitHandler(path, null, localPath);
389+
let commits = 0;
390+
391+
await expect(
392+
submit(
393+
{
394+
name: "",
395+
baseURL: "https://chatgpt.com/backend-api",
396+
apiKey: "",
397+
model: "gpt-5",
398+
oauthProfile: "work",
399+
},
400+
noopSetPhase,
401+
{
402+
skipValidation: false,
403+
oauth: stagedCodexOAuth(async () => {
404+
commits += 1;
405+
}),
406+
},
407+
),
408+
).rejects.toThrow(/reconnect codex/i);
409+
410+
expect(commits).toBe(0);
411+
expect(await loadSettings(path)).toBeNull();
412+
expect(await loadLocalSettings(localPath)).toBeNull();
413+
});
414+
});
415+
381416
test("failed same-name reauthorization preserves the exact durable profile", async () => {
382417
await withTempDir(async (dir) => {
383-
scopeCheckResult = { status: "insufficient-scope", message: "Reconnect Codex." };
418+
scopeCheckResult = { status: "blocked", message: "Reconnect Codex." };
384419
const oldProfile = {
385420
name: "work",
386421
tokens: { access: "old-access", refresh: "old-refresh", expiresAt: 500 },
@@ -457,7 +492,7 @@ describe("buildProviderSubmitHandler", () => {
457492

458493
test("explicit save-anyway skips the scope probe and commits exactly once", async () => {
459494
await withTempDir(async (dir) => {
460-
scopeCheckResult = { status: "insufficient-scope", message: "should never be thrown" };
495+
scopeCheckResult = { status: "blocked", message: "should never be thrown" };
461496
const localPath = localSettingsPath(dir);
462497
const submit = buildProviderSubmitHandler(join(dir, "settings.json"), null, localPath);
463498
let commits = 0;

src/tui/provider-setup-submit.ts

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
import { checkOAuthProviderScope } from "../auth/oauth-scope-check.js";
1+
import {
2+
OAuthProviderScopeError,
3+
checkOAuthProviderScope,
4+
isBlockingOAuthScopeCheckResult,
5+
} from "../auth/oauth-scope-check.js";
26
import {
37
mergeProviderIntoSettings,
48
saveGlobalSettings,
@@ -48,28 +52,16 @@ export function buildProviderSubmitHandler(
4852
const trimmedKey = apiKey.trim();
4953
const selectedModel = model.trim();
5054

51-
// A signed-in subscription provider has no key to test or store in
52-
// settings. Its exchanged credentials remain staged until this path has
53-
// authorized persistence; once committed to the home-level auth store,
54-
// config load projects that store into the provider catalog. Persist only
55-
// non-secret provider/model metadata globally so the selection survives
56-
// when a local settings target would alias this file.
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.
55+
// OAuth credentials stay staged until setup validation authorizes durable
56+
// persistence. Definitive API-scope or credential failures block the save;
57+
// inconclusive probe failures do not. Once committed to the home-level auth
58+
// store, config load projects it into the provider catalog, so only
59+
// non-secret provider/model metadata is persisted globally.
6860
if (oauth !== undefined) {
6961
if (!skipValidation) {
7062
const scopeCheck = await checkOAuthProviderScope(oauth.kind, oauth.tokens);
71-
if (scopeCheck.status === "insufficient-scope") {
72-
throw new Error(scopeCheck.message);
63+
if (isBlockingOAuthScopeCheckResult(scopeCheck)) {
64+
throw new OAuthProviderScopeError(scopeCheck.message);
7365
}
7466
}
7567
setPhase("saving");

0 commit comments

Comments
 (0)