Skip to content

Commit 3bdda57

Browse files
TheGreatAxiosTheGreatAxios
authored
Merge pull request #499 from corbitsdev/cl-6679-keep-concurrent-mcp-oauth-writes-from-wiping-tokens
Keep concurrent MCP OAuth writes from wiping tokens
2 parents c2e72ce + bb180e5 commit 3bdda57

5 files changed

Lines changed: 301 additions & 19 deletions

File tree

‎src/mcp/auth-store.test.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { mkdtemp, readFile } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
import { loadAuthState, saveAuthState, updateAuthState } from "./auth-store.js";
6+
7+
async function tempHome(): Promise<string> {
8+
return mkdtemp(join(tmpdir(), "mcp-auth-"));
9+
}
10+
11+
describe("mcp auth-store", () => {
12+
test("updateAuthState merges concurrent field writes without losing tokens", async () => {
13+
const home = await tempHome();
14+
await saveAuthState("linear", {
15+
clientInformation: {
16+
client_id: "c1",
17+
redirect_uris: ["http://127.0.0.1:1/callback"],
18+
client_id_issued_at: 1,
19+
},
20+
}, home);
21+
22+
// Reproduce the bug: one writer saves tokens while another overwrites with a
23+
// fresh codeVerifier from a concurrent OAuth start. With full-snapshot
24+
// persists, the verifier write wiped tokens; with updateAuthState, both land.
25+
const writes = await Promise.all([
26+
updateAuthState("linear", (state) => {
27+
state.tokens = {
28+
access_token: "tok",
29+
token_type: "bearer",
30+
expires_in: 3600,
31+
refresh_token: "ref",
32+
};
33+
}, home),
34+
updateAuthState("linear", (state) => {
35+
state.codeVerifier = "verifier-from-other-session";
36+
}, home),
37+
]);
38+
39+
const final = await loadAuthState("linear", home);
40+
expect(final.tokens?.access_token).toBe("tok");
41+
expect(final.codeVerifier).toBe("verifier-from-other-session");
42+
expect(final.clientInformation?.client_id).toBe("c1");
43+
expect(writes[0].tokens?.access_token === "tok" || writes[1].tokens?.access_token === "tok").toBe(true);
44+
});
45+
46+
test("concurrent saveAuthState calls do not throw ENOENT on temp rename", async () => {
47+
const home = await tempHome();
48+
await Promise.all(
49+
Array.from({ length: 20 }, (_, i) =>
50+
saveAuthState("linear", { codeVerifier: `v${String(i)}` }, home),
51+
),
52+
);
53+
const final = await loadAuthState("linear", home);
54+
expect(final.codeVerifier?.startsWith("v")).toBe(true);
55+
// No leftover temp files from failed renames.
56+
const dir = join(home, ".corbits", "mcp-auth");
57+
const raw = await readFile(join(dir, "linear.json"), "utf8");
58+
expect(JSON.parse(raw).codeVerifier).toBe(final.codeVerifier);
59+
});
60+
});

‎src/mcp/auth-store.ts‎

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { mkdir, readFile, rename, writeFile } from "node:fs/promises";
22
import { homedir } from "node:os";
3-
import { join } from "node:path";
3+
import { dirname, join } from "node:path";
44
import type { OAuthClientInformationFull, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
55
import { SETTINGS_DIR_NAME } from "../branding.js";
66

@@ -46,11 +46,59 @@ export async function loadAuthState(serverName: string, home: string = homedir()
4646
return {};
4747
}
4848

49+
// pid alone is not unique per call — concurrent saves in one process must not
50+
// share a temp path or the second rename hits ENOENT after the first moves it.
51+
let tmpWriteCounter = 0;
52+
53+
// Serialize read-modify-write per auth file so two OAuth provider instances for
54+
// the same server cannot clobber each other's fields (classic lost-update: one
55+
// session's saveCodeVerifier wiping another's just-written tokens).
56+
const updateChains = new Map<string, Promise<unknown>>();
57+
58+
async function writeAuthFile(path: string, state: MCPAuthState): Promise<void> {
59+
await mkdir(dirname(path), { recursive: true, mode: 0o700 });
60+
const tmp = `${path}.${process.pid}.${(tmpWriteCounter += 1)}.tmp`;
61+
await writeFile(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
62+
await rename(tmp, path);
63+
}
64+
4965
// Tokens are credentials, so the directory and file are restricted to the owner.
66+
// Full replace — prefer updateAuthState when mutating a single field so concurrent
67+
// writers merge instead of last-writer-wins on a stale snapshot.
5068
export async function saveAuthState(serverName: string, state: MCPAuthState, home: string = homedir()): Promise<void> {
5169
const path = authFilePath(serverName, home);
52-
await mkdir(mcpAuthDir(home), { recursive: true, mode: 0o700 });
53-
const tmp = `${path}.${process.pid}.tmp`;
54-
await writeFile(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
55-
await rename(tmp, path);
70+
const previous = updateChains.get(path) ?? Promise.resolve();
71+
const write = previous.then(
72+
() => writeAuthFile(path, state),
73+
() => writeAuthFile(path, state),
74+
);
75+
updateChains.set(path, write.then(() => undefined, () => undefined));
76+
await write;
77+
}
78+
79+
// Load → mutate → save under the per-file chain. Mutator receives a mutable
80+
// snapshot of the latest on-disk state; the returned object is what was written.
81+
export async function updateAuthState(
82+
serverName: string,
83+
mutator: (state: MCPAuthState) => void,
84+
home: string = homedir(),
85+
): Promise<MCPAuthState> {
86+
const path = authFilePath(serverName, home);
87+
const previous = updateChains.get(path) ?? Promise.resolve();
88+
const run = previous.then(
89+
async () => {
90+
const state = await loadAuthState(serverName, home);
91+
mutator(state);
92+
await writeAuthFile(path, state);
93+
return state;
94+
},
95+
async () => {
96+
const state = await loadAuthState(serverName, home);
97+
mutator(state);
98+
await writeAuthFile(path, state);
99+
return state;
100+
},
101+
);
102+
updateChains.set(path, run.then(() => undefined, () => undefined));
103+
return run;
56104
}

‎src/mcp/oauth-provider.test.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { mkdtemp } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
5+
import { loadAuthState, saveAuthState } from "./auth-store.js";
6+
import { createOAuthProvider } from "./oauth-provider.js";
7+
8+
async function tempHome(): Promise<string> {
9+
return mkdtemp(join(tmpdir(), "mcp-oauth-"));
10+
}
11+
12+
const clientInfo = (port: number) => ({
13+
client_id: `client-on-${String(port)}`,
14+
redirect_uris: [`http://127.0.0.1:${String(port)}/callback`],
15+
client_id_issued_at: 1,
16+
token_endpoint_auth_method: "none" as const,
17+
grant_types: ["authorization_code", "refresh_token"],
18+
response_types: ["code"],
19+
client_name: "interchange-code",
20+
});
21+
22+
async function syncValue<T>(value: T | Promise<T>): Promise<T> {
23+
return await value;
24+
}
25+
26+
describe("createOAuthProvider", () => {
27+
test("drops stale DCR client when redirect port changed and no tokens exist", async () => {
28+
const home = await tempHome();
29+
await saveAuthState("linear", {
30+
clientInformation: clientInfo(60435),
31+
codeVerifier: "old-verifier",
32+
}, home);
33+
34+
const provider = await createOAuthProvider({
35+
serverName: "linear",
36+
redirectUrl: "http://127.0.0.1:62000/callback",
37+
onAuthURL: () => undefined,
38+
home,
39+
});
40+
41+
expect(await syncValue(provider.clientInformation())).toBeUndefined();
42+
const disk = await loadAuthState("linear", home);
43+
expect(disk.clientInformation).toBeUndefined();
44+
expect(disk.codeVerifier).toBeUndefined();
45+
});
46+
47+
test("keeps registered client and tokens when only the loopback port changed", async () => {
48+
const home = await tempHome();
49+
await saveAuthState("linear", {
50+
clientInformation: clientInfo(60435),
51+
tokens: {
52+
access_token: "live",
53+
token_type: "bearer",
54+
expires_in: 3600,
55+
refresh_token: "refresh",
56+
},
57+
}, home);
58+
59+
const provider = await createOAuthProvider({
60+
serverName: "linear",
61+
redirectUrl: "http://127.0.0.1:62000/callback",
62+
onAuthURL: () => undefined,
63+
home,
64+
});
65+
66+
expect((await syncValue(provider.clientInformation()))?.client_id).toBe("client-on-60435");
67+
expect((await syncValue(provider.tokens()))?.access_token).toBe("live");
68+
});
69+
70+
test("concurrent saveTokens and saveCodeVerifier from two providers keep both fields", async () => {
71+
const home = await tempHome();
72+
await saveAuthState("linear", { clientInformation: clientInfo(1) }, home);
73+
74+
const a = await createOAuthProvider({
75+
serverName: "linear",
76+
redirectUrl: "http://127.0.0.1:1/callback",
77+
onAuthURL: () => undefined,
78+
home,
79+
});
80+
const b = await createOAuthProvider({
81+
serverName: "linear",
82+
redirectUrl: "http://127.0.0.1:1/callback",
83+
onAuthURL: () => undefined,
84+
home,
85+
});
86+
87+
await Promise.all([
88+
a.saveTokens({
89+
access_token: "tok-a",
90+
token_type: "bearer",
91+
expires_in: 60,
92+
refresh_token: "ref-a",
93+
}),
94+
b.saveCodeVerifier("verifier-b"),
95+
]);
96+
97+
const disk = await loadAuthState("linear", home);
98+
expect(disk.tokens?.access_token).toBe("tok-a");
99+
expect(disk.codeVerifier).toBe("verifier-b");
100+
});
101+
102+
test("resetAuthorization clears client when redirect no longer matches registration", async () => {
103+
const home = await tempHome();
104+
await saveAuthState("linear", {
105+
clientInformation: clientInfo(60435),
106+
tokens: {
107+
access_token: "live",
108+
token_type: "bearer",
109+
expires_in: 1,
110+
refresh_token: "r",
111+
},
112+
codeVerifier: "v",
113+
}, home);
114+
115+
const provider = await createOAuthProvider({
116+
serverName: "linear",
117+
redirectUrl: "http://127.0.0.1:62000/callback",
118+
onAuthURL: () => undefined,
119+
home,
120+
});
121+
122+
// Tokens present → client kept at create. Reset simulates failed refresh.
123+
await provider.resetAuthorization();
124+
expect(await syncValue(provider.tokens())).toBeUndefined();
125+
expect(await syncValue(provider.clientInformation())).toBeUndefined();
126+
const disk = await loadAuthState("linear", home);
127+
expect(disk.clientInformation).toBeUndefined();
128+
expect(disk.tokens).toBeUndefined();
129+
});
130+
});

‎src/mcp/oauth-provider.ts‎

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js";
22
import type { OAuthClientInformationFull, OAuthClientInformationMixed, OAuthClientMetadata, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
3-
import { loadAuthState, saveAuthState, type MCPAuthState } from "./auth-store.js";
3+
import { updateAuthState, type MCPAuthState } from "./auth-store.js";
44
import { MCP_CLIENT_NAME } from "../branding.js";
55

66
export type OAuthProviderOptions = {
@@ -12,9 +12,45 @@ export type OAuthProviderOptions = {
1212
};
1313
export type CorbitsOAuthProvider = OAuthClientProvider & { resetAuthorization(): Promise<void> };
1414

15+
function redirectUrisInclude(info: OAuthClientInformationFull | undefined, redirectUrl: string): boolean {
16+
const uris = info?.redirect_uris;
17+
if (uris === undefined || uris.length === 0) return true;
18+
return uris.includes(redirectUrl);
19+
}
20+
21+
// Dynamic client registration bakes in the loopback redirect_uri (ephemeral port).
22+
// A later session that binds a new port cannot reuse that client_id for authorize
23+
// / token exchange — drop the stale registration when we have no refreshable
24+
// tokens and must run the browser flow again.
25+
function dropStaleClientRegistration(state: MCPAuthState, redirectUrl: string): void {
26+
if (state.tokens !== undefined) return;
27+
if (redirectUrisInclude(state.clientInformation, redirectUrl)) return;
28+
delete state.clientInformation;
29+
delete state.codeVerifier;
30+
}
31+
32+
function replaceStored(stored: MCPAuthState, next: MCPAuthState): void {
33+
delete stored.clientInformation;
34+
delete stored.tokens;
35+
delete stored.codeVerifier;
36+
if (next.clientInformation !== undefined) stored.clientInformation = next.clientInformation;
37+
if (next.tokens !== undefined) stored.tokens = next.tokens;
38+
if (next.codeVerifier !== undefined) stored.codeVerifier = next.codeVerifier;
39+
}
40+
1541
export async function createOAuthProvider(opts: OAuthProviderOptions): Promise<CorbitsOAuthProvider> {
16-
const stored: MCPAuthState = await loadAuthState(opts.serverName, opts.home);
17-
const persist = (): Promise<void> => saveAuthState(opts.serverName, stored, opts.home);
42+
// Load + scrub stale DCR under the per-file chain so concurrent providers see
43+
// the same cleaned state. Mutations always re-read disk; this in-memory mirror
44+
// only serves the SDK's sync getters (tokens / clientInformation / codeVerifier).
45+
const stored: MCPAuthState = await updateAuthState(opts.serverName, (state) => {
46+
dropStaleClientRegistration(state, opts.redirectUrl);
47+
}, opts.home);
48+
49+
const apply = async (mutator: (state: MCPAuthState) => void): Promise<void> => {
50+
const next = await updateAuthState(opts.serverName, mutator, opts.home);
51+
replaceStored(stored, next);
52+
};
53+
1854
let oauthState: string | undefined;
1955
return {
2056
get redirectUrl(): string { return opts.redirectUrl; },
@@ -27,32 +63,40 @@ export async function createOAuthProvider(opts: OAuthProviderOptions): Promise<C
2763
},
2864
clientInformation(): OAuthClientInformationMixed | undefined { return stored.clientInformation; },
2965
saveClientInformation(info: OAuthClientInformationMixed): Promise<void> {
30-
stored.clientInformation = info as OAuthClientInformationFull;
31-
return persist();
66+
return apply((state) => {
67+
state.clientInformation = info as OAuthClientInformationFull;
68+
});
3269
},
3370
tokens(): OAuthTokens | undefined { return stored.tokens; },
3471
saveTokens(tokens: OAuthTokens): Promise<void> {
35-
stored.tokens = tokens;
36-
return persist();
72+
return apply((state) => {
73+
state.tokens = tokens;
74+
});
3775
},
3876
redirectToAuthorization(authorizationUrl: URL): void {
3977
const state = authorizationUrl.searchParams.get("state");
4078
if (state !== null) opts.onAuthorizationState?.(state);
4179
opts.onAuthURL(opts.serverName, authorizationUrl.toString());
4280
},
4381
saveCodeVerifier(codeVerifier: string): Promise<void> {
44-
stored.codeVerifier = codeVerifier;
45-
return persist();
82+
return apply((state) => {
83+
state.codeVerifier = codeVerifier;
84+
});
4685
},
4786
codeVerifier(): string {
4887
if (stored.codeVerifier === undefined) throw new Error("No PKCE code verifier saved for this authorization.");
4988
return stored.codeVerifier;
5089
},
5190
resetAuthorization(): Promise<void> {
52-
delete stored.tokens;
53-
delete stored.codeVerifier;
5491
oauthState = undefined;
55-
return persist();
92+
return apply((state) => {
93+
delete state.tokens;
94+
delete state.codeVerifier;
95+
// Next browser flow needs a client registered for *this* loopback port.
96+
if (!redirectUrisInclude(state.clientInformation, opts.redirectUrl)) {
97+
delete state.clientInformation;
98+
}
99+
});
56100
},
57101
};
58102
}

‎src/provider/reasoning-effort.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ describe("cycleReasoningEffort", () => {
137137
expect(cycleReasoningEffort("grok-4.6", undefined)).toBe(
138138
cycleReasoningEffort("grok-4.6", "high"),
139139
);
140-
expect(cycleReasoningEffort("grok-4.6", "high")).toBe("low");
140+
expect(cycleReasoningEffort("grok-4.6", "high")).toBe("xhigh");
141141
});
142142

143143
test("unset gpt-5.1 chat cycles from implicit none to minimal", () => {
@@ -152,7 +152,7 @@ describe("cycleReasoningEffort", () => {
152152
test("grok leftover minimal cycles the same as unset / high", () => {
153153
expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe(cycleReasoningEffort("grok-4.6", undefined));
154154
expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe(cycleReasoningEffort("grok-4.6", "high"));
155-
expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe("low");
155+
expect(cycleReasoningEffort("grok-4.6", "minimal")).toBe("xhigh");
156156
});
157157

158158
test("unknown models with rungs still start at supported[0] when no default exists", () => {

0 commit comments

Comments
 (0)