mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(mcp): refresh with the app's redirect URI, cancel sign-in during the lock wait
Tokens from a Client ID Metadata Document sign-in through an https callback route were refreshed with the loopback redirect URI, which the documents do not list, so every expiry needed a browser sign-in. createMcpAuthProvider takes the app's redirectUrl and startMcpSignIn passes its own. startMcpSignIn's wait for the refresh lock now ends when the signal aborts. Only McpRefreshLockBusyError (coding-agent maps proper-lockfile's ELOCKED to it) falls back to the browser step; other lock errors fail the sign-in.
This commit is contained in:
parent
76c040a2ad
commit
797e122cb1
12 changed files with 257 additions and 56 deletions
|
|
@ -9,7 +9,7 @@
|
|||
|
||||
### Fixed
|
||||
|
||||
- A failed token refresh of an MCP connection no longer registers an OAuth client or replaces the PKCE verifier of a sign-in in progress, which made that sign-in fail. MCP sign-in now refreshes the stored tokens with the stored client even when the loopback callback port changed, instead of registering a new client and opening the browser.
|
||||
- A failed token refresh of an MCP connection no longer registers an OAuth client or replaces the PKCE verifier of a sign-in in progress, which made that sign-in fail. MCP sign-in now refreshes the stored tokens with the stored client even when the loopback callback port changed, instead of registering a new client and opening the browser. Ctrl+C in `/mcp` and the `--timeout` of `pi mcp login` now also stop a sign-in while it waits up to 25 seconds for another pi process's token refresh, and a refresh lock that fails for another reason than that wait now fails the sign-in instead of opening the browser.
|
||||
|
||||
## [1.5.0] - 2026-11-02
|
||||
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ import {
|
|||
type McpOAuthServerStore,
|
||||
type McpOAuthSettings,
|
||||
type McpOAuthState,
|
||||
McpRefreshLockBusyError,
|
||||
type OAuthCallbackPage,
|
||||
type OAuthChallenge,
|
||||
startMcpSignIn,
|
||||
|
|
@ -95,23 +96,31 @@ export class McpOAuthCredentialStore {
|
|||
/**
|
||||
* A lock file per server. When the process exits, proper-lockfile removes the locks it holds; when
|
||||
* it is killed, the lock goes stale because it is no longer renewed, and the next process takes it over.
|
||||
* Throws `McpRefreshLockBusyError` when another process holds the lock for the whole wait.
|
||||
*/
|
||||
private async withRefreshLock<T>(key: string, fn: () => Promise<T>): Promise<T> {
|
||||
if (!this.lockDir) return fn();
|
||||
mkdirSync(this.lockDir, { recursive: true, mode: 0o700 });
|
||||
const hash = createHash("sha256").update(key).digest("hex").slice(0, 16);
|
||||
const release = await lockfile.lock(join(this.lockDir, `mcp-auth-refresh-${hash}`), {
|
||||
realpath: false,
|
||||
stale: REFRESH_LOCK_STALE_MS,
|
||||
retries: {
|
||||
retries: REFRESH_LOCK_WAIT_MS / REFRESH_LOCK_RETRY_MS,
|
||||
factor: 1,
|
||||
minTimeout: REFRESH_LOCK_RETRY_MS,
|
||||
maxTimeout: REFRESH_LOCK_RETRY_MS,
|
||||
},
|
||||
// The default throws from a timer. A lost lock at worst lets two refreshes overlap.
|
||||
onCompromised: () => {},
|
||||
});
|
||||
const release = await lockfile
|
||||
.lock(join(this.lockDir, `mcp-auth-refresh-${hash}`), {
|
||||
realpath: false,
|
||||
stale: REFRESH_LOCK_STALE_MS,
|
||||
retries: {
|
||||
retries: REFRESH_LOCK_WAIT_MS / REFRESH_LOCK_RETRY_MS,
|
||||
factor: 1,
|
||||
minTimeout: REFRESH_LOCK_RETRY_MS,
|
||||
maxTimeout: REFRESH_LOCK_RETRY_MS,
|
||||
},
|
||||
// The default throws from a timer. A lost lock at worst lets two refreshes overlap.
|
||||
onCompromised: () => {},
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
// proper-lockfile reports a lock held by another process as `ELOCKED`.
|
||||
if (error instanceof Error && "code" in error && error.code === "ELOCKED")
|
||||
throw new McpRefreshLockBusyError();
|
||||
throw error;
|
||||
});
|
||||
try {
|
||||
return await fn();
|
||||
} finally {
|
||||
|
|
|
|||
|
|
@ -2,7 +2,13 @@ import { createHash } from "node:crypto";
|
|||
import { mkdtempSync, readdirSync, rmSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { type McpOAuthSettings, type OAuthChallenge, OAuthIssuerMismatchError } from "@earendil-works/pi-mcp/oauth";
|
||||
import {
|
||||
type McpOAuthServerStore,
|
||||
type McpOAuthSettings,
|
||||
McpSignInCancelledError,
|
||||
type OAuthChallenge,
|
||||
OAuthIssuerMismatchError,
|
||||
} from "@earendil-works/pi-mcp/oauth";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { InMemoryAuthStorageBackend } from "../src/core/auth-storage.ts";
|
||||
import {
|
||||
|
|
@ -73,6 +79,45 @@ describe("MCP OAuth refresh", () => {
|
|||
expect(readdirSync(lockDir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("cancels a sign-in that waits for another process's refresh", async () => {
|
||||
const { server, lockDir, process } = await signedIn();
|
||||
let release = () => {};
|
||||
const held = process().store.withRefreshLock(
|
||||
() =>
|
||||
new Promise<void>((resolve) => {
|
||||
release = resolve;
|
||||
}),
|
||||
);
|
||||
// The sign-in's store, recording its wait for the lock.
|
||||
const store = process().store;
|
||||
let waiting: Promise<unknown> | undefined;
|
||||
const recording: McpOAuthServerStore = {
|
||||
...store,
|
||||
withRefreshLock: (fn) => {
|
||||
const run = store.withRefreshLock(fn);
|
||||
waiting = run;
|
||||
return run;
|
||||
},
|
||||
};
|
||||
const controller = new AbortController();
|
||||
const signIn = signInMcpServer({
|
||||
serverUrl: server.url,
|
||||
store: recording,
|
||||
settings: {},
|
||||
prompt: { showAuthorizationUrl: () => {}, promptForRedirectUrl: async () => undefined },
|
||||
signal: controller.signal,
|
||||
});
|
||||
setTimeout(() => controller.abort(), 200);
|
||||
await expect(signIn).rejects.toBeInstanceOf(McpSignInCancelledError);
|
||||
|
||||
release();
|
||||
await held;
|
||||
// The cancelled sign-in takes the lock once it is free and releases it without refreshing.
|
||||
await waiting;
|
||||
expect(server.log.filter((entry) => entry === "token refresh")).toHaveLength(0);
|
||||
expect(readdirSync(lockDir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("waits for a running refresh to save the new tokens", async () => {
|
||||
const { server, process } = await signedIn();
|
||||
server.expireAccessTokens();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue