Skip to content

Commit a2b0ad8

Browse files
fix(mcp): authenticate() clears only its own OAuth state
A token check around the clear could not stop an older call, waiting on the auth file lock, from deleting a newer flow's state. `clearOAuthState` now takes the expected state and compares it inside the lock. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD
1 parent d66adcc commit a2b0ad8

3 files changed

Lines changed: 43 additions & 9 deletions

File tree

‎packages/opencode/src/mcp/auth.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ export interface Interface {
4949
readonly clearCodeVerifier: (mcpName: string) => Effect.Effect<void>
5050
readonly updateOAuthState: (mcpName: string, oauthState: string) => Effect.Effect<void>
5151
readonly getOAuthState: (mcpName: string) => Effect.Effect<string | undefined>
52-
readonly clearOAuthState: (mcpName: string) => Effect.Effect<void>
52+
// altimate_change start — with `expected`, clears only while the stored state is still that one
53+
readonly clearOAuthState: (mcpName: string, expected?: string) => Effect.Effect<void>
54+
// altimate_change end
5355
readonly isTokenExpired: (mcpName: string) => Effect.Effect<boolean | null>
5456
}
5557

@@ -135,7 +137,18 @@ export const layer = Layer.effect(
135137
const updateCodeVerifier = updateField("codeVerifier", "updateCodeVerifier")
136138
const updateOAuthState = updateField("oauthState", "updateOAuthState")
137139
const clearCodeVerifier = clearField("codeVerifier", "clearCodeVerifier")
138-
const clearOAuthState = clearField("oauthState", "clearOAuthState")
140+
// altimate_change start — compare-and-clear inside the lock: a newer flow for the same server
141+
// may have stored its own state while this caller waited for the lock
142+
const clearOAuthState = Effect.fn("McpAuth.clearOAuthState")(function* (mcpName: string, expected?: string) {
143+
yield* mutate((data) => {
144+
const entry = data[mcpName]
145+
if (!entry) return undefined
146+
if (expected !== undefined && entry.oauthState !== expected) return undefined
147+
delete entry.oauthState
148+
return { ...data, [mcpName]: entry }
149+
})
150+
})
151+
// altimate_change end
139152

140153
const getOAuthState = Effect.fn("McpAuth.getOAuthState")(function* (mcpName: string) {
141154
const entry = yield* get(mcpName)

‎packages/opencode/src/mcp/index.ts‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1437,15 +1437,13 @@ export const layer = Layer.effect(
14371437
}
14381438

14391439
const s = yield* InstanceState.get(state)
1440-
// A newer call owns the server, and may own the OAuth state too: close this client,
1441-
// leave the rest alone. Checked again after the clear, which suspends.
1442-
const superseded = Effect.fnUntraced(function* () {
1440+
// Only this call's own OAuth state: a newer flow may have stored its own meanwhile.
1441+
yield* auth.clearOAuthState(mcpName, result.oauthState)
1442+
// A newer call owns the server: close this client and leave the server to it.
1443+
if (!isCurrent(s, mcpName, token)) {
14431444
yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
14441445
return s.status[mcpName] ?? ({ status: "disabled" } satisfies Status)
1445-
})
1446-
if (!isCurrent(s, mcpName, token)) return yield* superseded()
1447-
yield* auth.clearOAuthState(mcpName)
1448-
if (!isCurrent(s, mcpName, token)) return yield* superseded()
1446+
}
14491447
return yield* storeClient(s, mcpName, client, listing.tools, listing.meta, mcpConfig.timeout)
14501448
// altimate_change end
14511449
}

‎packages/opencode/test/mcp/auth.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,3 +76,26 @@ test("serializes concurrent auth file updates across service instances", async (
7676
}),
7777
)
7878
})
79+
80+
// altimate_change start — a flow clears only its own OAuth state, never a newer flow's (codex)
81+
test("clearOAuthState with an expected state leaves a newer state in place", async () => {
82+
const file = authFile()
83+
84+
await Effect.runPromise(
85+
Effect.gen(function* () {
86+
const auth = yield* authService(file.layer)
87+
yield* auth.updateOAuthState("server", "newer")
88+
89+
yield* auth.clearOAuthState("server", "older")
90+
expect(yield* auth.getOAuthState("server")).toBe("newer")
91+
92+
yield* auth.clearOAuthState("server", "newer")
93+
expect(yield* auth.getOAuthState("server")).toBeUndefined()
94+
95+
yield* auth.updateOAuthState("server", "any")
96+
yield* auth.clearOAuthState("server")
97+
expect(yield* auth.getOAuthState("server")).toBeUndefined()
98+
}),
99+
)
100+
})
101+
// altimate_change end

0 commit comments

Comments
 (0)