Skip to content

Commit b223f4c

Browse files
TheGreatAxiosTheGreatAxios
committed
fix(providers): close removal safety gaps
1 parent 9099b64 commit b223f4c

4 files changed

Lines changed: 196 additions & 14 deletions

File tree

‎src/auth/remove-provider.test.ts‎

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { mkdtemp, rm } from "node:fs/promises";
22
import { tmpdir } from "node:os";
33
import { join } from "node:path";
4-
import { describe, expect, test } from "bun:test";
4+
import { afterEach, describe, expect, test } from "bun:test";
55

66
import {
77
listCodexProfiles,
@@ -13,8 +13,23 @@ import {
1313
loadXaiProfile,
1414
saveXaiProfile,
1515
} from "../config/oauth-stores.js";
16+
import {
17+
clearSourceCredentials,
18+
registerSourceCredentialRecord,
19+
} from "../config/source-credentials.js";
1620
import { oauthStoreForProvider } from "./remove-provider.js";
1721

22+
function registerOAuthProvider(
23+
providerName: string,
24+
provider: "codex" | "xai",
25+
profile: string,
26+
): void {
27+
registerSourceCredentialRecord(providerName, {
28+
provenance: { kind: "oauth", provider, profile },
29+
material: { secret: "token" },
30+
});
31+
}
32+
1833
async function withHome<T>(fn: (home: string) => Promise<T>): Promise<T> {
1934
const home = await mkdtemp(join(tmpdir(), "remove-provider-"));
2035
try {
@@ -25,11 +40,33 @@ async function withHome<T>(fn: (home: string) => Promise<T>): Promise<T> {
2540
}
2641

2742
describe("oauthStoreForProvider", () => {
43+
afterEach(() => {
44+
clearSourceCredentials();
45+
});
46+
2847
test("maps xai/ and codex/ provider names to their store and profile", () => {
48+
clearSourceCredentials();
49+
registerOAuthProvider("xai/work", "xai", "work");
50+
registerOAuthProvider("codex/personal", "codex", "personal");
2951
expect(oauthStoreForProvider("xai/work")?.profile).toBe("work");
3052
expect(oauthStoreForProvider("codex/personal")?.profile).toBe("personal");
3153
});
3254

55+
test("rejects namespaced custom providers without OAuth ownership", () => {
56+
clearSourceCredentials();
57+
registerSourceCredentialRecord("codex/manual", {
58+
provenance: { kind: "api-key" },
59+
material: { secret: "manual-key" },
60+
});
61+
registerSourceCredentialRecord("xai/manual", {
62+
provenance: { kind: "api-key" },
63+
material: { secret: "manual-key" },
64+
});
65+
66+
expect(oauthStoreForProvider("codex/manual")).toBeNull();
67+
expect(oauthStoreForProvider("xai/manual")).toBeNull();
68+
});
69+
3370
test("returns null for API-key, keyless, and degenerate names", () => {
3471
expect(oauthStoreForProvider("openai")).toBeNull();
3572
expect(oauthStoreForProvider("ollama")).toBeNull();
@@ -40,6 +77,8 @@ describe("oauthStoreForProvider", () => {
4077

4178
test("removing an xai profile leaves sibling profiles untouched", async () => {
4279
await withHome(async (home) => {
80+
clearSourceCredentials();
81+
registerOAuthProvider("xai/work", "xai", "work");
4382
const tokens = { access: "a", refresh: "r", expiresAt: 10_000_000 };
4483
await saveXaiProfile({ name: "work", createdAt: 0, tokens }, home);
4584
await saveXaiProfile({ name: "personal", createdAt: 0, tokens }, home);
@@ -58,6 +97,8 @@ describe("oauthStoreForProvider", () => {
5897

5998
test("removing a codex profile leaves sibling profiles untouched", async () => {
6099
await withHome(async (home) => {
100+
clearSourceCredentials();
101+
registerOAuthProvider("codex/night", "codex", "night");
61102
const tokens = { access: "a", refresh: "r", expiresAt: 10_000_000 };
62103
await saveCodexProfile({ name: "day", createdAt: 0, tokens }, home);
63104
await saveCodexProfile({ name: "night", createdAt: 0, tokens }, home);
@@ -75,13 +116,18 @@ describe("oauthStoreForProvider", () => {
75116

76117
test("removing an already-absent profile is a no-op", async () => {
77118
await withHome(async (home) => {
119+
clearSourceCredentials();
120+
registerOAuthProvider("xai/ghost", "xai", "ghost");
78121
const target = oauthStoreForProvider("xai/ghost");
79122
expect(await target?.removeProfile(target.profile, home)).toEqual([]);
80123
});
81124
});
82125

83126
test("missing auth files are tolerated", async () => {
84127
await withHome(async (home) => {
128+
clearSourceCredentials();
129+
registerOAuthProvider("xai/work", "xai", "work");
130+
registerOAuthProvider("codex/work", "codex", "work");
85131
// No profiles ever saved: the store treats a missing file as empty.
86132
const xai = oauthStoreForProvider("xai/work");
87133
expect(await xai?.removeProfile(xai.profile, home)).toEqual([]);

‎src/auth/remove-provider.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
removeCodexProfile,
44
removeXaiProfile,
55
} from "../config/oauth-stores.js";
6+
import { findSourceCredentialRecord } from "../config/source-credentials.js";
67
import { xaiProfileFromProviderName } from "../config/xai-providers.js";
78

89
/**
@@ -20,19 +21,28 @@ export interface OAuthStoreTarget {
2021
) => Promise<string[]>;
2122
}
2223

23-
// Maps a catalog provider name to its OAuth auth store + profile using the
24-
// projection helpers (never string-splitting at the call site). Returns null
25-
// for API-key, Custom, Ollama/keyless, and degenerate empty-profile names —
26-
// those delete settings-side only.
24+
// Maps a catalog provider to its OAuth auth store only when the live source
25+
// credential record proves ownership of the same provider family and profile.
26+
// Names alone are user-controlled and cannot authorize credential deletion.
2727
export function oauthStoreForProvider(
2828
providerName: string,
2929
): OAuthStoreTarget | null {
30+
const provenance = findSourceCredentialRecord(providerName)?.provenance;
31+
if (provenance?.kind !== "oauth") return null;
3032
const codexProfile = codexProfileFromProviderName(providerName);
31-
if (codexProfile !== undefined && codexProfile.length > 0) {
33+
if (
34+
provenance.provider === "codex" &&
35+
codexProfile === provenance.profile &&
36+
codexProfile.length > 0
37+
) {
3238
return { profile: codexProfile, removeProfile: removeCodexProfile };
3339
}
3440
const xaiProfile = xaiProfileFromProviderName(providerName);
35-
if (xaiProfile !== undefined && xaiProfile.length > 0) {
41+
if (
42+
provenance.provider === "xai" &&
43+
xaiProfile === provenance.profile &&
44+
xaiProfile.length > 0
45+
) {
3646
return { profile: xaiProfile, removeProfile: removeXaiProfile };
3747
}
3848
return null;

‎src/tui/provider-remove.test.ts‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { mkdir, mkdtemp, readFile, rm } from "node:fs/promises";
22
import { tmpdir } from "node:os";
33
import { join } from "node:path";
4-
import { describe, expect, test } from "bun:test";
4+
import { afterEach, describe, expect, test } from "bun:test";
55

66
import {
77
saveGlobalSettings,
@@ -11,10 +11,16 @@ import {
1111
type Settings,
1212
} from "../config/settings.js";
1313
import {
14+
loadCodexProfile,
15+
saveCodexProfile,
1416
listXaiProfiles,
1517
loadXaiProfile,
1618
saveXaiProfile,
1719
} from "../config/oauth-stores.js";
20+
import {
21+
clearSourceCredentials,
22+
registerSourceCredentialRecord,
23+
} from "../config/source-credentials.js";
1824
import { createGlobalSettingsWriter } from "../mcp/add-server.js";
1925
import { withMockedHomedir } from "../../testkit/mock-module.js";
2026
import { modelOptionId } from "./model-catalog.js";
@@ -31,6 +37,10 @@ const SEED_WATERMARKS = {
3137
lastChangelogVersion: typeof pkg.version === "string" ? pkg.version : "0.0.0",
3238
} as const;
3339

40+
afterEach(() => {
41+
clearSourceCredentials();
42+
});
43+
3444
function keeperEntry() {
3545
return {
3646
baseURL: "https://keeper/v1",
@@ -48,6 +58,7 @@ interface RemoveHarness {
4858
readonly refreshed: unknown[][];
4959
readonly opened: (string | undefined)[];
5060
readonly wiring: Awaited<ReturnType<typeof wireSettings>>;
61+
readonly state: RunnerState;
5162
readonly cleanup: () => Promise<void>;
5263
}
5364

@@ -96,6 +107,11 @@ async function wireRemoveHarness(opts: {
96107
const services = {
97108
hookManager: { getStatuses: () => [] },
98109
globalSettingsWriter: createGlobalSettingsWriter(settingsPath),
110+
permissionGate: { setProviderIdentity: () => undefined },
111+
buildSessionSources: () => ({ sources: [], defaultSource: "" }),
112+
directorHolder: {},
113+
computeAdvertised: () => [],
114+
toolset: { dynamicRunner: { currentDefinitions: () => [] } },
99115
} as unknown as RunnerServices;
100116
const wiring = await wireSettings(state, services);
101117
return {
@@ -106,6 +122,7 @@ async function wireRemoveHarness(opts: {
106122
refreshed,
107123
opened,
108124
wiring,
125+
state,
109126
cleanup: () => rm(dir, { recursive: true, force: true }),
110127
};
111128
}
@@ -253,6 +270,11 @@ describe("provider removal execute path", () => {
253270
liveProvider: "keeper",
254271
});
255272
try {
273+
clearSourceCredentials();
274+
registerSourceCredentialRecord("xai/work", {
275+
provenance: { kind: "oauth", provider: "xai", profile: "work" },
276+
material: { secret: "token" },
277+
});
256278
await saveXaiProfile(
257279
{ name: "work", createdAt: 0, tokens: PROFILE_TOKENS },
258280
harness.home,
@@ -311,6 +333,11 @@ describe("provider removal execute path", () => {
311333
liveProvider: "keeper",
312334
});
313335
try {
336+
clearSourceCredentials();
337+
registerSourceCredentialRecord("xai/work", {
338+
provenance: { kind: "oauth", provider: "xai", profile: "work" },
339+
material: { secret: "token" },
340+
});
314341
await saveXaiProfile(
315342
{ name: "work", createdAt: 0, tokens: PROFILE_TOKENS },
316343
harness.home,
@@ -319,6 +346,11 @@ describe("provider removal execute path", () => {
319346
{ name: "personal", createdAt: 0, tokens: PROFILE_TOKENS },
320347
harness.home,
321348
);
349+
expect(
350+
harness.wiring.describeRemoveProvider(
351+
modelOptionId("xai/work", "grok-4"),
352+
),
353+
).toContain("auth profile 'work'");
322354
const before = await readFile(harness.settingsPath, "utf8");
323355
await executeRemove(harness, modelOptionId("xai/work", "grok-4"));
324356
// The orphaned credential is gone; siblings are untouched.
@@ -338,6 +370,81 @@ describe("provider removal execute path", () => {
338370
}
339371
});
340372

373+
test("custom namespaced providers leave unrelated OAuth profiles intact", async () => {
374+
for (const provider of ["xai/manual", "codex/manual"] as const) {
375+
const seed: Settings = {
376+
...SEED_WATERMARKS,
377+
providers: {
378+
keeper: keeperEntry(),
379+
[provider]: {
380+
baseURL: "https://manual.example/v1",
381+
apiKey: "manual-key",
382+
models: ["manual-model"],
383+
defaultModel: "manual-model",
384+
},
385+
},
386+
};
387+
const harness = await wireRemoveHarness({
388+
seed,
389+
local: null,
390+
liveProvider: "keeper",
391+
});
392+
try {
393+
clearSourceCredentials();
394+
registerSourceCredentialRecord(provider, {
395+
provenance: { kind: "api-key" },
396+
material: { secret: "manual-key" },
397+
});
398+
if (provider.startsWith("xai/")) {
399+
await saveXaiProfile(
400+
{ name: "manual", createdAt: 0, tokens: PROFILE_TOKENS },
401+
harness.home,
402+
);
403+
} else {
404+
await saveCodexProfile(
405+
{ name: "manual", createdAt: 0, tokens: PROFILE_TOKENS },
406+
harness.home,
407+
);
408+
}
409+
await executeRemove(harness, modelOptionId(provider, "manual-model"));
410+
const profile = provider.startsWith("xai/")
411+
? await loadXaiProfile("manual", harness.home)
412+
: await loadCodexProfile("manual", harness.home);
413+
expect(profile).toBeDefined();
414+
} finally {
415+
await harness.cleanup();
416+
}
417+
}
418+
});
419+
420+
test("cannot switch to a provider while its removal is pending", async () => {
421+
const seed: Settings = {
422+
...SEED_WATERMARKS,
423+
providers: {
424+
keeper: keeperEntry(),
425+
victim: {
426+
baseURL: "https://victim/v1",
427+
apiKey: "victim-key",
428+
models: ["v1"],
429+
defaultModel: "v1",
430+
},
431+
},
432+
};
433+
const harness = await wireRemoveHarness({
434+
seed,
435+
local: null,
436+
liveProvider: "keeper",
437+
});
438+
try {
439+
const victim = modelOptionId("victim", "v1");
440+
harness.wiring.onRemoveProvider(victim);
441+
harness.wiring.onModelSelect(victim);
442+
expect(harness.state.config.providerName).toBe("keeper");
443+
} finally {
444+
await harness.cleanup();
445+
}
446+
});
447+
341448
test("removing the default repoints it and says where it went", async () => {
342449
const seed: Settings = {
343450
...SEED_WATERMARKS,
@@ -438,6 +545,11 @@ describe("describeRemoveProvider", () => {
438545
};
439546
const harness = await wireLines(seed);
440547
try {
548+
clearSourceCredentials();
549+
registerSourceCredentialRecord("xai/work", {
550+
provenance: { kind: "oauth", provider: "xai", profile: "work" },
551+
material: { secret: "token" },
552+
});
441553
const keyLine = harness.wiring.describeRemoveProvider(
442554
modelOptionId("openai/work", "m1"),
443555
);

0 commit comments

Comments
 (0)