From 6ed5b1423802cf4272d16f2f77408fbc8caa79fb Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Wed, 29 Apr 2026 16:33:01 +0300 Subject: [PATCH] fix: harden auth token refresh Preserve refresh tokens when providers omit rotation, recover long-running MCP servers from externally refreshed credentials, and serialize auth storage updates across parallel agents. --- src/cli.ts | 17 +- src/cli/errors.test.ts | 52 +++ src/cli/errors.ts | 35 ++ src/commands/login.test.ts | 45 ++- src/commands/login.ts | 15 +- src/commands/logout.test.ts | 30 +- src/commands/logout.ts | 23 +- src/container.ts | 18 +- src/services/auth-service.test.ts | 37 ++- src/services/auth-service.ts | 27 +- src/services/auth-storage.test.ts | 51 +++ src/services/auth-storage.ts | 91 ++++++ src/services/index.ts | 4 + src/services/keychain-auth-storage.test.ts | 22 ++ src/services/keychain-auth-storage.ts | 43 ++- src/services/locked-auth-storage.test.ts | 126 ++++++++ src/services/locked-auth-storage.ts | 302 ++++++++++++++++++ src/services/migrating-auth-storage.ts | 58 ++++ src/services/mode-aware-file-auth-storage.ts | 29 ++ src/services/test-helpers.ts | 4 + src/services/token-manager.test.ts | 284 +++++++++++++++- src/services/token-manager.ts | 80 ++++- src/shared/code-navigation-error-map.test.ts | 1 + src/shared/code-navigation-error-map.ts | 2 + .../package-intelligence-error-map.test.ts | 10 +- src/shared/package-intelligence-error-map.ts | 1 + src/tools/search-language.test.ts | 21 ++ src/tools/shared.ts | 4 +- 28 files changed, 1322 insertions(+), 110 deletions(-) create mode 100644 src/cli/errors.test.ts create mode 100644 src/cli/errors.ts create mode 100644 src/services/locked-auth-storage.test.ts create mode 100644 src/services/locked-auth-storage.ts diff --git a/src/cli.ts b/src/cli.ts index f29d149c..07c60f83 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,6 +1,7 @@ #!/usr/bin/env node import { Command } from "commander"; import { version } from "../package.json"; +import { handleCliError } from "./cli/errors.js"; import { enforceCachedRequiredUpdateForInvocation, runWithUpdateCheckFlush, @@ -21,7 +22,6 @@ import { registerPkgCommandGroup, registerUnifiedSearchCommands, } from "./commands/index.js"; -import { AuthConfigError, AuthStoragePolicyError } from "./services/index.js"; import { FileSystemServiceImpl, NpmRegistryUpdateCheckService, @@ -158,17 +158,10 @@ try { { stderr: process.stderr, requiredUpdateRefreshTask }, ); } catch (error) { - if (isUserFacingError(error)) { - console.error(`${error.message}\n`); - process.exit(1); - } - throw error; -} - -function isUserFacingError(error: unknown): error is Error { - return ( - error instanceof AuthConfigError || error instanceof AuthStoragePolicyError - ); + handleCliError(error, { + stderr: process.stderr, + exit: process.exit as (code: number) => never, + }); } /** diff --git a/src/cli/errors.test.ts b/src/cli/errors.test.ts new file mode 100644 index 00000000..0ef1aa5a --- /dev/null +++ b/src/cli/errors.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "bun:test"; +import { AuthStorageLockTimeoutError } from "../services/index.js"; +import { AuthRequiredError } from "../shared/require-auth.js"; +import { handleCliError } from "./errors.js"; + +describe("handleCliError", () => { + it("exits without writing a stack trace for AuthRequiredError", () => { + const stderrWrites: string[] = []; + const exit = ((code: number) => { + throw new Error(`process.exit:${code}`); + }) as (code: number) => never; + + expect(() => + handleCliError(new AuthRequiredError("Authentication required"), { + stderr: { + write: (chunk: string | Uint8Array) => { + stderrWrites.push(String(chunk)); + return true; + }, + }, + exit, + }), + ).toThrow("process.exit:1"); + + expect(stderrWrites.join("")).not.toContain("AuthRequiredError"); + expect(stderrWrites.join("")).not.toContain("at "); + }); + + it("prints lock timeout errors without an uncaught stack trace", () => { + const stderrWrites: string[] = []; + const exit = ((code: number) => { + throw new Error(`process.exit:${code}`); + }) as (code: number) => never; + + expect(() => + handleCliError(new AuthStorageLockTimeoutError("lock timed out"), { + stderr: { + write: (chunk: string | Uint8Array) => { + stderrWrites.push(String(chunk)); + return true; + }, + }, + exit, + }), + ).toThrow("process.exit:1"); + + const output = stderrWrites.join(""); + expect(output).toContain("lock timed out"); + expect(output).not.toContain("AuthStorageLockTimeoutError"); + expect(output).not.toContain("at "); + }); +}); diff --git a/src/cli/errors.ts b/src/cli/errors.ts new file mode 100644 index 00000000..25ffd7b1 --- /dev/null +++ b/src/cli/errors.ts @@ -0,0 +1,35 @@ +import { + AuthConfigError, + AuthStorageLockTimeoutError, + AuthStoragePolicyError, +} from "../services/index.js"; +import { AuthRequiredError } from "../shared/require-auth.js"; + +export interface CliErrorHandlerDeps { + stderr: Pick; + exit: (code: number) => never; +} + +export function handleCliError( + error: unknown, + deps: CliErrorHandlerDeps, +): never { + if (error instanceof AuthRequiredError) { + deps.exit(1); + } + + if (isUserFacingError(error)) { + deps.stderr.write(`${error.message}\n\n`); + deps.exit(1); + } + + throw error; +} + +function isUserFacingError(error: unknown): error is Error { + return ( + error instanceof AuthConfigError || + error instanceof AuthStorageLockTimeoutError || + error instanceof AuthStoragePolicyError + ); +} diff --git a/src/commands/login.test.ts b/src/commands/login.test.ts index 5f30c237..6d5c63da 100644 --- a/src/commands/login.test.ts +++ b/src/commands/login.test.ts @@ -29,10 +29,10 @@ describe("loginAction", () => { ); expect(browserService.open).toHaveBeenCalled(); expect(authService.exchangeCodeForTokens).toHaveBeenCalled(); - expect(authStorage.saveTokens).toHaveBeenCalled(); - expect(authStorage.saveClient).toHaveBeenCalledWith( + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( expect.stringContaining("__githits_storage_probe__"), expect.any(Object), + expect.any(Object), ); consoleSpy.mockRestore(); @@ -60,7 +60,11 @@ describe("loginAction", () => { }, ); - expect(authStorage.saveTokens).not.toHaveBeenCalled(); + expect(authStorage.saveAuthSession).not.toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -86,7 +90,11 @@ describe("loginAction", () => { }, ); - expect(authStorage.saveTokens).toHaveBeenCalled(); + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -152,9 +160,10 @@ describe("loginAction", () => { ); expect(authService.registerClient).toHaveBeenCalled(); - expect(authStorage.saveClient).toHaveBeenCalledWith( + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( mcpUrl, expect.any(Object), + expect.any(Object), ); consoleSpy.mockRestore(); }); @@ -162,7 +171,7 @@ describe("loginAction", () => { it("fails before remote registration when storage preflight fails", async () => { const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); const authStorage = createMockAuthStorage({ - saveClient: mock(() => Promise.reject(new Error("keychain locked"))), + saveAuthSession: mock(() => Promise.reject(new Error("keychain locked"))), }); const authService = createMockAuthService(); const browserService = createMockBrowserService(); @@ -197,7 +206,11 @@ describe("loginAction", () => { expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); expect(authService.registerClient).toHaveBeenCalled(); - expect(authStorage.saveClient).toHaveBeenCalled(); + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -307,7 +320,11 @@ describe("loginAction", () => { ); expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl); - expect(authStorage.saveTokens).not.toHaveBeenCalled(); + expect(authStorage.saveAuthSession).not.toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -334,7 +351,11 @@ describe("loginAction", () => { ); expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl); - expect(authStorage.saveTokens).toHaveBeenCalled(); + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -360,7 +381,11 @@ describe("loginAction", () => { }, ); - expect(authStorage.saveTokens).toHaveBeenCalled(); + expect(authStorage.saveAuthSession).toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + expect.any(Object), + ); consoleSpy.mockRestore(); }); }); diff --git a/src/commands/login.ts b/src/commands/login.ts index 8a39cd94..693afc8e 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -49,14 +49,11 @@ async function preflightAuthPersistence( createdAt: new Date(0).toISOString(), }; try { - await authStorage.saveClient(probeUrl, probeClient); - await authStorage.saveTokens(probeUrl, probeTokens); - await authStorage.clearTokens(probeUrl); - await authStorage.clearClient(probeUrl); + await authStorage.saveAuthSession(probeUrl, probeClient, probeTokens); + await authStorage.clearAuthSession(probeUrl); return null; } catch (error) { - await authStorage.clearTokens(probeUrl).catch(() => {}); - await authStorage.clearClient(probeUrl).catch(() => {}); + await authStorage.clearAuthSession(probeUrl).catch(() => {}); const message = error instanceof Error ? error.message : String(error); return { status: "failed", @@ -135,7 +132,6 @@ export async function loginFlow( redirectUri, registeredAt: new Date().toISOString(), }; - await authStorage.saveClient(mcpUrl, client); } port = options.port; } else { @@ -158,7 +154,6 @@ export async function loginFlow( redirectUri, registeredAt: new Date().toISOString(), }; - await authStorage.saveClient(mcpUrl, client); } // Step 3: Generate PKCE parameters @@ -244,11 +239,11 @@ export async function loginFlow( }; } - // Step 9: Save tokens + // Step 9: Save auth session const expiresAt = new Date( Date.now() + tokenResponse.expiresIn * 1000, ).toISOString(); - await authStorage.saveTokens(mcpUrl, { + await authStorage.saveAuthSession(mcpUrl, client, { accessToken: tokenResponse.accessToken, refreshToken: tokenResponse.refreshToken, expiresAt, diff --git a/src/commands/logout.test.ts b/src/commands/logout.test.ts index a815f057..008d3b0b 100644 --- a/src/commands/logout.test.ts +++ b/src/commands/logout.test.ts @@ -17,8 +17,7 @@ describe("logoutAction", () => { await logoutAction({ authStorage, mcpUrl }); - expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl); - expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); + expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl); consoleSpy.mockRestore(); }); @@ -28,19 +27,18 @@ describe("logoutAction", () => { await logoutAction({ authStorage, mcpUrl }); - // Idempotent cleanup removes orphaned client registrations - expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl); - expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); + // Idempotent cleanup removes orphaned client registrations. + expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl); const output = consoleSpy.mock.calls.map((c) => c[0]).join("\n"); expect(output).toContain("Not currently logged in"); consoleSpy.mockRestore(); }); - it("clears client even when clearTokens throws", async () => { + it("propagates session clear failures", async () => { const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); const authStorage = createMockAuthStorage({ loadTokens: mock(() => Promise.resolve(createValidTokenData())), - clearTokens: mock(() => + clearAuthSession: mock(() => Promise.reject(new KeychainUnavailableError("keychain locked")), ), }); @@ -48,23 +46,7 @@ describe("logoutAction", () => { await expect(logoutAction({ authStorage, mcpUrl })).rejects.toThrow( KeychainUnavailableError, ); - expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); - consoleSpy.mockRestore(); - }); - - it("clears tokens even when clearClient throws", async () => { - const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); - const authStorage = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(createValidTokenData())), - clearClient: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - - await expect(logoutAction({ authStorage, mcpUrl })).rejects.toThrow( - KeychainUnavailableError, - ); - expect(authStorage.clearTokens).toHaveBeenCalledWith(mcpUrl); + expect(authStorage.clearAuthSession).toHaveBeenCalledWith(mcpUrl); consoleSpy.mockRestore(); }); }); diff --git a/src/commands/logout.ts b/src/commands/logout.ts index 81434d3a..fa91866d 100644 --- a/src/commands/logout.ts +++ b/src/commands/logout.ts @@ -10,29 +10,14 @@ export interface LogoutDependencies { /** * Core logout logic, separated for testability. * - * Always clears both tokens and client registration independently before - * reporting status. This ensures orphaned client registrations are cleaned up - * even when tokens are already absent (e.g. after a partial logout or expired - * token clear). Both clear operations are idempotent and error-isolated so a - * failure in one does not prevent the other from running. + * Clears both tokens and client registration as one auth-session update so + * concurrent MCP servers and login/logout commands cannot observe split state. */ export async function logoutAction(deps: LogoutDependencies): Promise { const { authStorage, mcpUrl } = deps; const auth = await authStorage.loadTokens(mcpUrl); - - // Clear both independently — a failure in one must not prevent the other. - let firstError: unknown; - try { - await authStorage.clearTokens(mcpUrl); - } catch (error) { - firstError = error; - } - try { - await authStorage.clearClient(mcpUrl); - } catch (error) { - firstError ??= error; - } + await authStorage.clearAuthSession(mcpUrl); if (!auth) { console.log("Not currently logged in.\n"); @@ -41,8 +26,6 @@ export async function logoutAction(deps: LogoutDependencies): Promise { console.log("Logged out.\n"); console.log(` Environment: ${mcpUrl}`); } - - if (firstError) throw firstError; } const LOGOUT_DESCRIPTION = `Remove stored credentials. diff --git a/src/container.ts b/src/container.ts index 639e3937..c33923f5 100644 --- a/src/container.ts +++ b/src/container.ts @@ -21,6 +21,7 @@ import { getMcpUrl, KeychainAuthStorage, KeyringServiceImpl, + LockedAuthStorage, loadAuthConfig, MigratingAuthStorage, ModeAwareFileAuthStorage, @@ -77,13 +78,16 @@ function createAuthStorageForMode( : rawKeyring; const keychainStorage = new KeychainAuthStorage(keyring); - return new MigratingAuthStorage( - keychainStorage, - fileStorage, - legacyStorage, - mode, - configPath, - (message) => console.error(message), + return new LockedAuthStorage( + new MigratingAuthStorage( + keychainStorage, + fileStorage, + legacyStorage, + mode, + configPath, + (message) => console.error(message), + ), + fileSystemService, ); } diff --git a/src/services/auth-service.test.ts b/src/services/auth-service.test.ts index d6aa8c4d..8dd6d341 100644 --- a/src/services/auth-service.test.ts +++ b/src/services/auth-service.test.ts @@ -1,8 +1,13 @@ -import { describe, expect, it } from "bun:test"; +import { afterEach, describe, expect, it, mock } from "bun:test"; import { AuthServiceImpl, evaluateCallback } from "./auth-service.js"; describe("AuthServiceImpl", () => { const service = new AuthServiceImpl(); + const originalFetch = globalThis.fetch; + + afterEach(() => { + globalThis.fetch = originalFetch; + }); describe("generatePkceParams", () => { it("returns verifier, challenge, and state", () => { @@ -53,6 +58,36 @@ describe("AuthServiceImpl", () => { }); }); + describe("refreshAccessToken", () => { + it("accepts refresh responses that omit refresh_token", async () => { + const fetchMock = mock(() => + Promise.resolve( + new Response( + JSON.stringify({ + access_token: "new-access-token", + expires_in: 60, + }), + { status: 200 }, + ), + ), + ); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const result = await service.refreshAccessToken({ + tokenEndpoint: "https://auth.example.com/oauth/token", + clientId: "client-id", + clientSecret: "client-secret", + refreshToken: "existing-refresh-token", + }); + + expect(result).toEqual({ + accessToken: "new-access-token", + refreshToken: undefined, + expiresIn: 60, + }); + }); + }); + describe("evaluateCallback", () => { it("returns success outcome and clear success HTML", () => { const callback = evaluateCallback({ diff --git a/src/services/auth-service.ts b/src/services/auth-service.ts index d7218f28..8a04e5c9 100644 --- a/src/services/auth-service.ts +++ b/src/services/auth-service.ts @@ -98,6 +98,12 @@ export interface TokenResponse { expiresIn: number; } +export interface RefreshTokenResponse { + accessToken: string; + refreshToken?: string; + expiresIn: number; +} + /** * Parameters for DCR registration. */ @@ -134,7 +140,7 @@ export interface AuthService { exchangeCodeForTokens(params: ExchangeParams): Promise; /** Refresh an expired access token */ - refreshAccessToken(params: RefreshParams): Promise; + refreshAccessToken(params: RefreshParams): Promise; } /** @@ -312,7 +318,9 @@ export class AuthServiceImpl implements AuthService { return parseTokenResponse(await response.json()); } - async refreshAccessToken(params: RefreshParams): Promise { + async refreshAccessToken( + params: RefreshParams, + ): Promise { const body = new URLSearchParams({ grant_type: "refresh_token", client_id: params.clientId, @@ -331,7 +339,7 @@ export class AuthServiceImpl implements AuthService { throw new Error(`Token refresh failed: ${error}`); } - return parseTokenResponse(await response.json()); + return parseRefreshTokenResponse(await response.json()); } } @@ -347,6 +355,19 @@ function parseTokenResponse(data: unknown): TokenResponse { }; } +function parseRefreshTokenResponse(data: unknown): RefreshTokenResponse { + const d = data as Record; + if (!d.access_token) { + throw new Error("Token response missing required fields"); + } + return { + accessToken: d.access_token as string, + refreshToken: + typeof d.refresh_token === "string" ? d.refresh_token : undefined, + expiresIn: (d.expires_in as number) || 3600, + }; +} + function successHtml(title = "Authentication successful"): string { return ` GitHits CLI diff --git a/src/services/auth-storage.test.ts b/src/services/auth-storage.test.ts index 4eb298b1..8d76af0e 100644 --- a/src/services/auth-storage.test.ts +++ b/src/services/auth-storage.test.ts @@ -242,6 +242,57 @@ describe("AuthStorageImpl", () => { }); }); + describe("clearAuthSession", () => { + it("attempts client cleanup even when token cleanup fails", async () => { + const fs = createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock((path: string) => { + if (path.endsWith("auth.json")) { + return Promise.resolve( + JSON.stringify({ + version: 1, + tokens: { + [BASE_URL]: { + accessToken: "eyJ-test", + refreshToken: "refresh-test", + expiresAt: null, + createdAt: "2025-01-15T10:00:00Z", + }, + }, + }), + ); + } + return Promise.resolve( + JSON.stringify({ + version: 1, + clients: { + [BASE_URL]: { + clientId: "client", + clientSecret: "secret", + redirectUri: "http://127.0.0.1:8080/callback", + registeredAt: "2025-01-15T10:00:00Z", + }, + }, + }), + ); + }), + deleteFile: mock((path: string) => { + if (path.endsWith("auth.json")) { + return Promise.reject(new Error("token delete failed")); + } + return Promise.resolve(); + }), + }); + const storage = new AuthStorageImpl(fs, "/test/.githits"); + + await expect(storage.clearAuthSession(BASE_URL)).rejects.toThrow( + "token delete failed", + ); + + expect(fs.deleteFile).toHaveBeenCalledWith("/test/.githits/client.json"); + }); + }); + describe("loadClient", () => { it("returns null when client file does not exist", async () => { const fs = createMockFileSystemService({ diff --git a/src/services/auth-storage.ts b/src/services/auth-storage.ts index 4acb78d4..4d9024d2 100644 --- a/src/services/auth-storage.ts +++ b/src/services/auth-storage.ts @@ -50,9 +50,22 @@ export interface AuthStorage { /** Save tokens for a specific base URL */ saveTokens(baseUrl: string, data: TokenData): Promise; + /** Save tokens only when the currently stored token still matches expected */ + saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise; + /** Clear tokens for a specific base URL */ clearTokens(baseUrl: string): Promise; + /** Clear tokens only when the currently stored token still matches expected */ + clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise; + /** Load client registration for a specific base URL */ loadClient(baseUrl: string): Promise; @@ -62,6 +75,16 @@ export interface AuthStorage { /** Clear client registration for a specific base URL */ clearClient(baseUrl: string): Promise; + /** Save client registration and tokens as one auth session update */ + saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise; + + /** Clear client registration and tokens as one auth session update */ + clearAuthSession(baseUrl: string): Promise; + /** Get a human-readable description of where credentials are stored */ getStorageLocation(): string; } @@ -112,6 +135,17 @@ export class AuthStorageImpl implements AuthStorage { ); } + async saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!sameTokenData(current, expected)) return false; + await this.saveTokens(baseUrl, data); + return true; + } + async clearTokens(baseUrl: string): Promise { const stored = await this.loadAuthFile(); if (!stored) return; @@ -128,6 +162,16 @@ export class AuthStorageImpl implements AuthStorage { } } + async clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!sameTokenData(current, expected)) return false; + await this.clearTokens(baseUrl); + return true; + } + async loadClient(baseUrl: string): Promise { const stored = await this.loadClientFile(); if (!stored) return null; @@ -164,6 +208,22 @@ export class AuthStorageImpl implements AuthStorage { ); } + async saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise { + await this.saveClient(baseUrl, client); + await this.saveTokens(baseUrl, tokens); + } + + async clearAuthSession(baseUrl: string): Promise { + await clearAuthSessionBestEffort( + () => this.clearTokens(baseUrl), + () => this.clearClient(baseUrl), + ); + } + private async loadAuthFile(): Promise { if (!(await this.fs.exists(this.authPath))) return null; try { @@ -197,3 +257,34 @@ export class AuthStorageImpl implements AuthStorage { export function normalizeBaseUrl(url: string): string { return url.replace(/\/+$/, ""); } + +export function sameTokenData( + a: TokenData | null, + b: TokenData | null, +): boolean { + if (a === null || b === null) return a === b; + return ( + a.accessToken === b.accessToken && + a.refreshToken === b.refreshToken && + a.expiresAt === b.expiresAt && + a.createdAt === b.createdAt + ); +} + +export async function clearAuthSessionBestEffort( + clearTokens: () => Promise, + clearClient: () => Promise, +): Promise { + let firstError: unknown; + try { + await clearTokens(); + } catch (error) { + firstError = error; + } + try { + await clearClient(); + } catch (error) { + firstError ??= error; + } + if (firstError) throw firstError; +} diff --git a/src/services/index.ts b/src/services/index.ts index 0c45b392..981a9c5e 100644 --- a/src/services/index.ts +++ b/src/services/index.ts @@ -117,6 +117,10 @@ export { KeychainUnavailableError, KeyringServiceImpl, } from "./keyring-service.js"; +export { + AuthStorageLockTimeoutError, + LockedAuthStorage, +} from "./locked-auth-storage.js"; export { MigratingAuthStorage } from "./migrating-auth-storage.js"; export { AuthStoragePolicyError, diff --git a/src/services/keychain-auth-storage.test.ts b/src/services/keychain-auth-storage.test.ts index 94975826..0cfe14b4 100644 --- a/src/services/keychain-auth-storage.test.ts +++ b/src/services/keychain-auth-storage.test.ts @@ -139,6 +139,28 @@ describe("KeychainAuthStorage", () => { }); }); + describe("clearAuthSession", () => { + it("attempts client cleanup even when token cleanup fails", async () => { + const deletePassword = mock((_service: string, account: string) => { + if (account.startsWith("v1:tokens:")) { + throw new KeychainUnavailableError("token delete failed"); + } + return true; + }); + const keyring = createMockKeyringService({ deletePassword }); + const storage = new KeychainAuthStorage(keyring); + + await expect(storage.clearAuthSession(BASE_URL)).rejects.toThrow( + "token delete failed", + ); + + expect(deletePassword).toHaveBeenCalledWith( + "githits", + `v1:client:${BASE_URL}`, + ); + }); + }); + describe("loadClient", () => { it("returns null when keyring returns null", async () => { const keyring = createMockKeyringService(); diff --git a/src/services/keychain-auth-storage.ts b/src/services/keychain-auth-storage.ts index acd7ebf5..5d61a8e1 100644 --- a/src/services/keychain-auth-storage.ts +++ b/src/services/keychain-auth-storage.ts @@ -3,7 +3,11 @@ import type { ClientRegistration, TokenData, } from "./auth-storage.js"; -import { normalizeBaseUrl } from "./auth-storage.js"; +import { + clearAuthSessionBestEffort, + normalizeBaseUrl, + sameTokenData, +} from "./auth-storage.js"; import type { KeyringService } from "./keyring-service.js"; const SERVICE_NAME = "githits"; @@ -86,11 +90,32 @@ export class KeychainAuthStorage implements AuthStorage { this.keyring.setPassword(SERVICE_NAME, key, JSON.stringify(data)); } + async saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!sameTokenData(current, expected)) return false; + await this.saveTokens(baseUrl, data); + return true; + } + async clearTokens(baseUrl: string): Promise { const key = `${TOKEN_PREFIX}${normalizeBaseUrl(baseUrl)}`; this.keyring.deletePassword(SERVICE_NAME, key); } + async clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!sameTokenData(current, expected)) return false; + await this.clearTokens(baseUrl); + return true; + } + async loadClient(baseUrl: string): Promise { const key = `${CLIENT_PREFIX}${normalizeBaseUrl(baseUrl)}`; const json = this.keyring.getPassword(SERVICE_NAME, key); @@ -109,6 +134,22 @@ export class KeychainAuthStorage implements AuthStorage { this.keyring.deletePassword(SERVICE_NAME, key); } + async saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise { + await this.saveClient(baseUrl, client); + await this.saveTokens(baseUrl, tokens); + } + + async clearAuthSession(baseUrl: string): Promise { + await clearAuthSessionBestEffort( + () => this.clearTokens(baseUrl), + () => this.clearClient(baseUrl), + ); + } + getStorageLocation(): string { switch (process.platform) { case "darwin": diff --git a/src/services/locked-auth-storage.test.ts b/src/services/locked-auth-storage.test.ts new file mode 100644 index 00000000..9a7331e3 --- /dev/null +++ b/src/services/locked-auth-storage.test.ts @@ -0,0 +1,126 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { getAppConfigDir } from "./app-config-paths.js"; +import { AuthStorageImpl } from "./auth-storage.js"; +import { FileSystemServiceImpl } from "./filesystem-service.js"; +import { LockedAuthStorage } from "./locked-auth-storage.js"; +import { createValidTokenData } from "./test-helpers.js"; + +describe("LockedAuthStorage", () => { + const baseUrl = "https://mcp.githits.com"; + const tempDirs: string[] = []; + + afterEach(async () => { + await Promise.all( + tempDirs + .splice(0) + .map((dir) => rm(dir, { recursive: true, force: true })), + ); + }); + + it("serializes conditional token saves across storage instances", async () => { + const { fs, fsWithHome, configDir } = await createStoragePaths(); + const first = new LockedAuthStorage( + new AuthStorageImpl(fs, configDir), + fsWithHome, + ); + const second = new LockedAuthStorage( + new AuthStorageImpl(fs, configDir), + fsWithHome, + ); + const initial = createValidTokenData({ accessToken: "initial" }); + await first.saveTokens(baseUrl, initial); + + const [firstSaved, secondSaved] = await Promise.all([ + first.saveTokensIfUnchanged( + baseUrl, + initial, + createValidTokenData({ accessToken: "first" }), + ), + second.saveTokensIfUnchanged( + baseUrl, + initial, + createValidTokenData({ accessToken: "second" }), + ), + ]); + + expect([firstSaved, secondSaved].filter(Boolean)).toHaveLength(1); + const finalToken = await first.loadTokens(baseUrl); + expect(finalToken).not.toBeNull(); + expect(["first", "second"]).toContain(finalToken?.accessToken ?? ""); + }); + + it("reclaims stale lock directories", async () => { + const { fs, fsWithHome, configDir, lockPath } = await createStoragePaths(); + await mkdir(lockPath, { recursive: true, mode: 0o700 }); + await writeFile( + join(lockPath, "owner.json"), + JSON.stringify({ + id: "dead-owner", + pid: 999_999_999, + createdAt: new Date().toISOString(), + processStartedAt: null, + }), + ); + const storage = new LockedAuthStorage( + new AuthStorageImpl(fs, configDir), + fsWithHome, + { lockTimeoutMs: 100 }, + ); + const token = createValidTokenData({ accessToken: "fresh" }); + + await storage.saveTokens(baseUrl, token); + + expect(await storage.loadTokens(baseUrl)).toEqual(token); + }); + + it("does not reclaim live locks just because they are old", async () => { + const { fs, fsWithHome, configDir, lockPath } = await createStoragePaths(); + await mkdir(lockPath, { recursive: true, mode: 0o700 }); + await writeFile( + join(lockPath, "owner.json"), + JSON.stringify({ + id: "live-owner", + pid: process.pid, + createdAt: new Date(Date.now() - 120_000).toISOString(), + processStartedAt: "test-start-time", + }), + ); + const storage = new LockedAuthStorage( + new AuthStorageImpl(fs, configDir), + fsWithHome, + { isOwnerAlive: async () => true, lockTimeoutMs: 100 }, + ); + + await expect( + storage.saveTokens( + baseUrl, + createValidTokenData({ accessToken: "fresh" }), + ), + ).rejects.toThrow("Timed out waiting for GitHits auth storage lock"); + }); + + async function createStoragePaths(): Promise<{ + fs: FileSystemServiceImpl; + fsWithHome: FileSystemServiceImpl; + configDir: string; + lockPath: string; + }> { + const root = await mkdtemp(join(tmpdir(), "githits-lock-")); + tempDirs.push(root); + const fs = new FileSystemServiceImpl(); + const homeDir = join(root, "home"); + const fsWithHome = Object.assign(Object.create(fs), fs, { + getHomeDir: () => homeDir, + }) as FileSystemServiceImpl; + const appConfigDir = getAppConfigDir(fsWithHome); + return { + fs, + fsWithHome, + configDir: join(appConfigDir, "auth"), + lockPath: join(appConfigDir, "auth.lock"), + }; + } +}); diff --git a/src/services/locked-auth-storage.ts b/src/services/locked-auth-storage.ts new file mode 100644 index 00000000..8a5c63b7 --- /dev/null +++ b/src/services/locked-auth-storage.ts @@ -0,0 +1,302 @@ +import { execFile } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { dirname } from "node:path"; +import { promisify } from "node:util"; +import { getAppConfigDir } from "./app-config-paths.js"; +import type { + AuthStorage, + ClientRegistration, + TokenData, +} from "./auth-storage.js"; +import type { FileSystemService } from "./filesystem-service.js"; + +const LOCK_DIR = "auth.lock"; +const LOCK_TIMEOUT_MS = 10_000; +const LOCK_RETRY_MS = 25; +const ORPHANED_LOCK_MS = 5_000; +const OWNER_FILE = "owner.json"; +const execFileAsync = promisify(execFile); + +interface LockOwner { + id: string; + pid: number; + createdAt: string; + processStartedAt: string | null; +} + +export class AuthStorageLockTimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = "AuthStorageLockTimeoutError"; + } +} + +export class LockedAuthStorage implements AuthStorage { + private readonly lockPath: string; + private readonly lockTimeoutMs: number; + private readonly isOwnerAlive: ( + pid: number, + processStartedAt: string | null, + ) => Promise; + private currentOwner: LockOwner | null = null; + + constructor( + private readonly storage: AuthStorage, + fileSystemService: FileSystemService, + options: { + lockTimeoutMs?: number; + isOwnerAlive?: ( + pid: number, + processStartedAt: string | null, + ) => Promise; + } = {}, + ) { + this.lockTimeoutMs = options.lockTimeoutMs ?? LOCK_TIMEOUT_MS; + this.isOwnerAlive = options.isOwnerAlive ?? isOriginalProcessAlive; + this.lockPath = fileSystemService.joinPath( + getAppConfigDir(fileSystemService), + LOCK_DIR, + ); + } + + loadTokens(baseUrl: string): Promise { + return this.storage.loadTokens(baseUrl); + } + + saveTokens(baseUrl: string, data: TokenData): Promise { + return this.withLock(() => this.storage.saveTokens(baseUrl, data)); + } + + saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise { + return this.withLock(() => + this.storage.saveTokensIfUnchanged(baseUrl, expected, data), + ); + } + + clearTokens(baseUrl: string): Promise { + return this.withLock(() => this.storage.clearTokens(baseUrl)); + } + + clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise { + return this.withLock(() => + this.storage.clearTokensIfUnchanged(baseUrl, expected), + ); + } + + loadClient(baseUrl: string): Promise { + return this.storage.loadClient(baseUrl); + } + + saveClient(baseUrl: string, data: ClientRegistration): Promise { + return this.withLock(() => this.storage.saveClient(baseUrl, data)); + } + + clearClient(baseUrl: string): Promise { + return this.withLock(() => this.storage.clearClient(baseUrl)); + } + + saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise { + return this.withLock(() => + this.storage.saveAuthSession(baseUrl, client, tokens), + ); + } + + clearAuthSession(baseUrl: string): Promise { + return this.withLock(() => this.storage.clearAuthSession(baseUrl)); + } + + getStorageLocation(): string { + return this.storage.getStorageLocation(); + } + + private async withLock(fn: () => Promise): Promise { + await this.acquireLock(); + try { + return await fn(); + } finally { + await this.releaseLock(); + } + } + + private async acquireLock(): Promise { + const startedAt = Date.now(); + await mkdir(dirname(this.lockPath), { recursive: true, mode: 0o700 }); + while (true) { + try { + await mkdir(this.lockPath, { recursive: false, mode: 0o700 }); + try { + await this.writeOwner(); + } catch (error) { + await rm(this.lockPath, { recursive: true, force: true }).catch( + () => undefined, + ); + throw error; + } + return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + await this.reclaimStaleLock(); + if (Date.now() - startedAt >= this.lockTimeoutMs) { + throw new AuthStorageLockTimeoutError( + `Timed out waiting for GitHits auth storage lock at ${this.lockPath}. If no githits process is running, remove this directory and retry.`, + ); + } + await sleep(LOCK_RETRY_MS); + } + } + } + + private async writeOwner(): Promise { + const owner: LockOwner = { + id: randomUUID(), + pid: process.pid, + createdAt: new Date().toISOString(), + processStartedAt: await getProcessStartedAt(process.pid), + }; + this.currentOwner = owner; + await writeFile(this.ownerPath(), JSON.stringify(owner), { mode: 0o600 }); + } + + private async reclaimStaleLock(): Promise { + const owner = await this.readOwner(); + if (!owner) { + await this.reclaimOldOwnerlessLock(); + return; + } + const ownerDead = !(await this.isOwnerAlive( + owner.pid, + owner.processStartedAt, + )); + if (!ownerDead) return; + + const currentOwner = await this.readOwner(); + if (!currentOwner || currentOwner.id !== owner.id) return; + + await rm(this.lockPath, { recursive: true, force: true }).catch( + () => undefined, + ); + } + + private async reclaimOldOwnerlessLock(): Promise { + const createdAtMs = await lockCreatedAtMs(this.lockPath); + if (Date.now() - createdAtMs < ORPHANED_LOCK_MS) return; + await rm(this.lockPath, { recursive: true, force: true }).catch( + () => undefined, + ); + } + + private async readOwner(): Promise { + try { + const raw = await readFile(this.ownerPath(), "utf8"); + const parsed = JSON.parse(raw) as Partial; + if ( + typeof parsed.id !== "string" || + typeof parsed.pid !== "number" || + typeof parsed.createdAt !== "string" || + !( + typeof parsed.processStartedAt === "string" || + parsed.processStartedAt === null + ) + ) { + return null; + } + return { + id: parsed.id, + pid: parsed.pid, + createdAt: parsed.createdAt, + processStartedAt: parsed.processStartedAt, + }; + } catch { + return null; + } + } + + private async releaseLock(): Promise { + const owner = this.currentOwner; + this.currentOwner = null; + if (!owner) return; + const currentOwner = await this.readOwner(); + if (!currentOwner || currentOwner.id !== owner.id) return; + await rm(this.lockPath, { recursive: true, force: true }).catch( + () => undefined, + ); + } + + private ownerPath(): string { + return `${this.lockPath}/${OWNER_FILE}`; + } +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +async function isOriginalProcessAlive( + pid: number, + processStartedAt: string | null, +): Promise { + if (!isProcessAlive(pid)) return false; + if (!processStartedAt) return true; + return (await getProcessStartedAt(pid)) === processStartedAt; +} + +function isProcessAlive(pid: number): boolean { + if (pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + return code === "EPERM"; + } +} + +export async function getProcessStartedAtForTesting( + pid: number, +): Promise { + return getProcessStartedAt(pid); +} + +async function getProcessStartedAt(pid: number): Promise { + try { + if (process.platform === "win32") { + const { stdout } = await execFileAsync("powershell.exe", [ + "-NoProfile", + "-Command", + `(Get-Process -Id ${pid}).StartTime.ToUniversalTime().ToString('o')`, + ]); + return stdout.trim() || null; + } + const { stdout } = await execFileAsync("ps", [ + "-p", + String(pid), + "-o", + "lstart=", + ]); + const parsed = Date.parse(stdout.trim()); + return Number.isNaN(parsed) ? null : new Date(parsed).toISOString(); + } catch { + return null; + } +} + +async function lockCreatedAtMs(path: string): Promise { + try { + const { stat } = await import("node:fs/promises"); + return (await stat(path)).mtimeMs; + } catch { + return 0; + } +} diff --git a/src/services/migrating-auth-storage.ts b/src/services/migrating-auth-storage.ts index edbfd335..f5b6ab45 100644 --- a/src/services/migrating-auth-storage.ts +++ b/src/services/migrating-auth-storage.ts @@ -55,6 +55,17 @@ export class MigratingAuthStorage implements AuthStorage { } } + async saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!this.sameTokenData(current, expected)) return false; + await this.saveTokens(baseUrl, data); + return true; + } + async clearTokens(baseUrl: string): Promise { const primaryError = await this.clearBestEffort(() => this.primary.clearTokens(baseUrl), @@ -66,6 +77,16 @@ export class MigratingAuthStorage implements AuthStorage { } } + async clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise { + const current = await this.loadTokens(baseUrl); + if (!this.sameTokenData(current, expected)) return false; + await this.clearTokens(baseUrl); + return true; + } + async loadClient(baseUrl: string): Promise { if (this.mode === "file") { return this.loadClientFileMode(baseUrl); @@ -96,6 +117,33 @@ export class MigratingAuthStorage implements AuthStorage { } } + async saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise { + if (this.mode === "file") { + await this.file.saveAuthSession(baseUrl, client, tokens); + return; + } + try { + await this.primary.saveAuthSession(baseUrl, client, tokens); + } catch (error) { + throw this.toPolicyError(error); + } + } + + async clearAuthSession(baseUrl: string): Promise { + const primaryError = await this.clearBestEffort(() => + this.primary.clearAuthSession(baseUrl), + ); + await this.clearBestEffort(() => this.file.clearAuthSession(baseUrl)); + await this.clearBestEffort(() => this.legacy.clearAuthSession(baseUrl)); + if (primaryError && !(primaryError instanceof KeychainUnavailableError)) { + throw primaryError; + } + } + getStorageLocation(): string { return this.mode === "file" ? this.file.getStorageLocation() @@ -356,4 +404,14 @@ export class MigratingAuthStorage implements AuthStorage { "Warning: multiple plaintext auth entries exist with ambiguous timestamps; using the new config auth path and leaving the other entry intact.", ); } + + private sameTokenData(a: TokenData | null, b: TokenData | null): boolean { + if (a === null || b === null) return a === b; + return ( + a.accessToken === b.accessToken && + a.refreshToken === b.refreshToken && + a.expiresAt === b.expiresAt && + a.createdAt === b.createdAt + ); + } } diff --git a/src/services/mode-aware-file-auth-storage.ts b/src/services/mode-aware-file-auth-storage.ts index fb44e9b0..b54af896 100644 --- a/src/services/mode-aware-file-auth-storage.ts +++ b/src/services/mode-aware-file-auth-storage.ts @@ -48,10 +48,26 @@ export class ModeAwareFileAuthStorage implements AuthStorage { await this.storage.saveTokens(baseUrl, data); } + async saveTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + data: TokenData, + ): Promise { + this.assertFileMode(); + return this.storage.saveTokensIfUnchanged(baseUrl, expected, data); + } + clearTokens(baseUrl: string): Promise { return this.storage.clearTokens(baseUrl); } + clearTokensIfUnchanged( + baseUrl: string, + expected: TokenData | null, + ): Promise { + return this.storage.clearTokensIfUnchanged(baseUrl, expected); + } + loadClient(baseUrl: string): Promise { return this.storage.loadClient(baseUrl); } @@ -65,6 +81,19 @@ export class ModeAwareFileAuthStorage implements AuthStorage { return this.storage.clearClient(baseUrl); } + async saveAuthSession( + baseUrl: string, + client: ClientRegistration, + tokens: TokenData, + ): Promise { + this.assertFileMode(); + await this.storage.saveAuthSession(baseUrl, client, tokens); + } + + clearAuthSession(baseUrl: string): Promise { + return this.storage.clearAuthSession(baseUrl); + } + getStorageLocation(): string { return this.storage.getStorageLocation(); } diff --git a/src/services/test-helpers.ts b/src/services/test-helpers.ts index dec8778d..557d31af 100644 --- a/src/services/test-helpers.ts +++ b/src/services/test-helpers.ts @@ -182,10 +182,14 @@ export function createMockAuthStorage( return { loadTokens: mock(() => Promise.resolve(null)), saveTokens: mock(() => Promise.resolve()), + saveTokensIfUnchanged: mock(() => Promise.resolve(true)), clearTokens: mock(() => Promise.resolve()), + clearTokensIfUnchanged: mock(() => Promise.resolve(true)), loadClient: mock(() => Promise.resolve(null)), saveClient: mock(() => Promise.resolve()), clearClient: mock(() => Promise.resolve()), + saveAuthSession: mock(() => Promise.resolve()), + clearAuthSession: mock(() => Promise.resolve()), getStorageLocation: mock(() => "/mock/.githits"), ...impl, }; diff --git a/src/services/token-manager.test.ts b/src/services/token-manager.test.ts index ec6e7beb..26402d5d 100644 --- a/src/services/token-manager.test.ts +++ b/src/services/token-manager.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, mock } from "bun:test"; +import type { TokenData } from "./auth-storage.js"; import { createMockAuthService, createMockAuthStorage, @@ -112,11 +113,41 @@ describe("refreshExpiredToken", () => { const result = await refreshExpiredToken(authService, authStorage, MCP_URL); expect(result).toBe(defaultTokenResponse.accessToken); - expect(authStorage.saveTokens).toHaveBeenCalledTimes(1); + expect(authStorage.saveTokensIfUnchanged).toHaveBeenCalledTimes(1); expect(authService.discoverEndpoints).toHaveBeenCalledWith(MCP_URL); expect(authService.refreshAccessToken).toHaveBeenCalledTimes(1); }); + it("keeps existing refresh token when refresh response omits one", async () => { + const expiredToken = createValidTokenData({ + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() - 60_000).toISOString(), + refreshToken: "existing-refresh-token", + }); + const authService = createMockAuthService({ + refreshAccessToken: mock(() => + Promise.resolve({ + accessToken: "new-access-token", + expiresIn: 3600, + }), + ), + }); + const authStorage = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(expiredToken)), + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + + const result = await refreshExpiredToken(authService, authStorage, MCP_URL); + + expect(result).toBe("new-access-token"); + expect(authStorage.saveTokensIfUnchanged).toHaveBeenCalledTimes(1); + expect(authStorage.saveTokensIfUnchanged).toHaveBeenCalledWith( + MCP_URL, + expiredToken, + expect.objectContaining({ refreshToken: "existing-refresh-token" }), + ); + }); + it("clears tokens and returns undefined on refresh failure", async () => { const expiredToken = createValidTokenData({ createdAt: new Date(Date.now() - 7200_000).toISOString(), @@ -135,7 +166,10 @@ describe("refreshExpiredToken", () => { const result = await refreshExpiredToken(authService, authStorage, MCP_URL); expect(result).toBeUndefined(); - expect(authStorage.clearTokens).toHaveBeenCalledWith(MCP_URL); + expect(authStorage.clearTokensIfUnchanged).toHaveBeenCalledWith( + MCP_URL, + expiredToken, + ); }); }); @@ -213,7 +247,7 @@ describe("TokenManager", () => { const result = await manager.getToken(); expect(result).toBe(defaultTokenResponse.accessToken); expect(authService.refreshAccessToken).toHaveBeenCalledTimes(1); - expect(authStorage.saveTokens).toHaveBeenCalledTimes(1); + expect(authStorage.saveTokensIfUnchanged).toHaveBeenCalledTimes(1); }); it("returns current token when proactive refresh fails", async () => { @@ -262,7 +296,10 @@ describe("TokenManager", () => { const result = await manager.getToken(); expect(result).toBeUndefined(); - expect(authStorage.clearTokens).toHaveBeenCalledWith(MCP_URL); + expect(authStorage.clearTokensIfUnchanged).toHaveBeenCalledWith( + MCP_URL, + tokenData, + ); }); it("coalesces concurrent refresh requests", async () => { @@ -355,7 +392,7 @@ describe("TokenManager", () => { const result = await manager.forceRefresh(); expect(result).toBeUndefined(); // Should NOT clear tokens since the token is still valid (not expired) - expect(authStorage.clearTokens).not.toHaveBeenCalled(); + expect(authStorage.clearTokensIfUnchanged).not.toHaveBeenCalled(); }); it("clears tokens when refresh fails with expired token", async () => { @@ -381,7 +418,240 @@ describe("TokenManager", () => { // forceRefresh should also clear since token is expired const result = await manager.forceRefresh(); expect(result).toBeUndefined(); - expect(authStorage.clearTokens).toHaveBeenCalledWith(MCP_URL); + expect(authStorage.clearTokensIfUnchanged).toHaveBeenCalledWith( + MCP_URL, + tokenData, + ); + }); + + it("recovers when another process writes fresh tokens after cached refresh fails", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "stale-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + }); + const freshToken = createValidTokenData({ + accessToken: "fresh-access-token", + refreshToken: "fresh-refresh-token", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }); + const loadTokens = mock<() => Promise>(() => + Promise.resolve(staleToken), + ); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => Promise.reject(new Error("stale"))), + }), + authStorage, + mcpUrl: MCP_URL, + }); + + expect(await manager.getToken()).toBe("stale-access-token"); + loadTokens.mockImplementation(() => Promise.resolve(freshToken)); + + const result = await manager.forceRefresh(); + + expect(result).toBe("fresh-access-token"); + expect(authStorage.clearTokensIfUnchanged).not.toHaveBeenCalled(); + }); + + it("uses externally refreshed tokens on later getToken calls", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "stale-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + }); + const freshToken = createValidTokenData({ + accessToken: "fresh-access-token", + refreshToken: "fresh-refresh-token", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }); + const loadTokens = mock<() => Promise>(() => + Promise.resolve(staleToken), + ); + const refreshAccessToken = mock(() => Promise.reject(new Error("stale"))); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ refreshAccessToken }), + authStorage, + mcpUrl: MCP_URL, + }); + + expect(await manager.getToken()).toBe("stale-access-token"); + loadTokens.mockImplementation(() => Promise.resolve(freshToken)); + const recovered = await manager.forceRefresh(); + const next = await manager.getToken(); + + expect(recovered).toBe("fresh-access-token"); + expect(next).toBe("fresh-access-token"); + expect(refreshAccessToken).toHaveBeenCalledTimes(2); + }); + + it("recovers externally updated tokens that preserve the same refresh token", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "same-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() + 60_000).toISOString(), + }); + const freshToken = createValidTokenData({ + accessToken: "fresh-access-token", + refreshToken: "same-refresh-token", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }); + const loadTokens = mock<() => Promise>(() => + Promise.resolve(staleToken), + ); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => Promise.reject(new Error("stale"))), + }), + authStorage, + mcpUrl: MCP_URL, + }); + + expect(await manager.getToken()).toBe("stale-access-token"); + loadTokens.mockImplementation(() => Promise.resolve(freshToken)); + + const result = await manager.forceRefresh(); + + expect(result).toBe("fresh-access-token"); + expect(authStorage.clearTokensIfUnchanged).not.toHaveBeenCalled(); + }); + + it("does not clear fresh tokens written after the first failed-refresh reload", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "stale-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() - 60_000).toISOString(), + }); + const freshToken = createValidTokenData({ + accessToken: "fresh-access-token", + refreshToken: "fresh-refresh-token", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }); + const loadTokens = mock<() => Promise>(() => + Promise.resolve(staleToken), + ); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => Promise.reject(new Error("stale"))), + }), + authStorage, + mcpUrl: MCP_URL, + }); + + loadTokens + .mockImplementationOnce(() => Promise.resolve(staleToken)) + .mockImplementationOnce(() => Promise.resolve(staleToken)) + .mockImplementationOnce(() => Promise.resolve(freshToken)); + + const result = await manager.getToken(); + + expect(result).toBe("fresh-access-token"); + expect(authStorage.clearTokensIfUnchanged).not.toHaveBeenCalled(); + }); + + it("does not overwrite fresh tokens written during refresh", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "stale-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() - 60_000).toISOString(), + }); + const freshToken = createValidTokenData({ + accessToken: "fresh-access-token", + refreshToken: "fresh-refresh-token", + createdAt: new Date().toISOString(), + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }); + const loadTokens = mock(() => Promise.resolve(staleToken)); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => + Promise.resolve({ + accessToken: "refresh-access-token", + expiresIn: 3600, + }), + ), + }), + authStorage, + mcpUrl: MCP_URL, + }); + + loadTokens + .mockImplementationOnce(() => Promise.resolve(staleToken)) + .mockImplementationOnce(() => Promise.resolve(freshToken)); + + const result = await manager.getToken(); + + expect(result).toBe("fresh-access-token"); + expect(authStorage.saveTokensIfUnchanged).not.toHaveBeenCalled(); + expect(authStorage.saveTokens).not.toHaveBeenCalled(); + }); + + it("does not rewrite tokens deleted during refresh", async () => { + const staleToken = createValidTokenData({ + accessToken: "stale-access-token", + refreshToken: "stale-refresh-token", + createdAt: new Date(Date.now() - 7200_000).toISOString(), + expiresAt: new Date(Date.now() - 60_000).toISOString(), + }); + const loadTokens = mock<() => Promise>(() => + Promise.resolve(staleToken), + ); + const authStorage = createMockAuthStorage({ + loadTokens, + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), + }); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => + Promise.resolve({ + accessToken: "refresh-access-token", + expiresIn: 3600, + }), + ), + }), + authStorage, + mcpUrl: MCP_URL, + }); + + loadTokens + .mockImplementationOnce(() => Promise.resolve(staleToken)) + .mockImplementationOnce(() => Promise.resolve(null)); + + const result = await manager.getToken(); + + expect(result).toBeUndefined(); + expect(authStorage.saveTokensIfUnchanged).not.toHaveBeenCalled(); + expect(authStorage.saveTokens).not.toHaveBeenCalled(); }); it("resets createdAt on refresh so a fresh token is not immediately refreshed again", async () => { @@ -408,7 +678,7 @@ describe("TokenManager", () => { expect(refreshed).toBe(defaultTokenResponse.accessToken); expect(second).toBe(defaultTokenResponse.accessToken); expect(refreshMock).toHaveBeenCalledTimes(1); - expect(authStorage.saveTokens).toHaveBeenCalledTimes(1); + expect(authStorage.saveTokensIfUnchanged).toHaveBeenCalledTimes(1); }); }); }); diff --git a/src/services/token-manager.ts b/src/services/token-manager.ts index be363153..cbc1ed4c 100644 --- a/src/services/token-manager.ts +++ b/src/services/token-manager.ts @@ -1,5 +1,5 @@ import { withTelemetrySpan } from "../shared/telemetry.js"; -import type { AuthService, TokenResponse } from "./auth-service.js"; +import type { AuthService, RefreshTokenResponse } from "./auth-service.js"; import type { AuthStorage, TokenData } from "./auth-storage.js"; /** @@ -167,7 +167,7 @@ export class TokenManager implements TokenProvider { ); if (!client) return undefined; - let response: TokenResponse; + let response: RefreshTokenResponse; try { const metadata = await withTelemetrySpan( "token-manager.discover-endpoints", @@ -184,24 +184,37 @@ export class TokenManager implements TokenProvider { }), ); } catch { - // Only clear tokens if they are actually expired. - // If refresh was proactive (token still valid), leave storage intact - // so subsequent calls can still serve the current token. + const reloadedToken = await this.loadExternallyUpdatedToken(tokens); + if (reloadedToken) return reloadedToken.accessToken; + + // Only clear tokens if they are actually expired and still match the + // failed in-memory refresh token. A separate `githits login` may have + // already written fresh tokens for long-running MCP servers. const isExpired = tokens.expiresAt ? new Date() >= new Date(tokens.expiresAt) : false; if (isExpired) { - this.cachedToken = null; - await withTelemetrySpan("token-manager.clear-tokens", () => - this.authStorage.clearTokens(this.mcpUrl), + const currentStoredTokens = + await this.loadExternallyUpdatedToken(tokens); + if (currentStoredTokens) return currentStoredTokens.accessToken; + + const cleared = await withTelemetrySpan( + "token-manager.clear-tokens-if-unchanged", + () => this.authStorage.clearTokensIfUnchanged(this.mcpUrl, tokens), ); + if (!cleared) { + const currentToken = await this.authStorage.loadTokens(this.mcpUrl); + this.cachedToken = currentToken; + return currentToken?.accessToken; + } + this.cachedToken = null; } return undefined; } const newTokenData: TokenData = { accessToken: response.accessToken, - refreshToken: response.refreshToken, + refreshToken: response.refreshToken ?? tokens.refreshToken, expiresAt: new Date( Date.now() + response.expiresIn * 1000, ).toISOString(), @@ -211,11 +224,56 @@ export class TokenManager implements TokenProvider { createdAt: new Date().toISOString(), }; - await withTelemetrySpan("token-manager.save-tokens", () => - this.authStorage.saveTokens(this.mcpUrl, newTokenData), + const externallyUpdatedToken = await this.loadExternallyUpdatedToken( + tokens, + { + treatMissingAsExternalUpdate: true, + }, ); + if (externallyUpdatedToken === null) return undefined; + if (externallyUpdatedToken) return externallyUpdatedToken.accessToken; + + const saved = await withTelemetrySpan("token-manager.save-tokens", () => + this.authStorage.saveTokensIfUnchanged( + this.mcpUrl, + tokens, + newTokenData, + ), + ); + if (!saved) { + const currentToken = await this.authStorage.loadTokens(this.mcpUrl); + this.cachedToken = currentToken; + return currentToken?.accessToken; + } this.cachedToken = newTokenData; return response.accessToken; }); } + + private async loadExternallyUpdatedToken( + failedTokens: TokenData, + options: { treatMissingAsExternalUpdate?: boolean } = {}, + ): Promise { + const storedTokens = await withTelemetrySpan( + "token-manager.reload-tokens", + () => this.authStorage.loadTokens(this.mcpUrl), + ); + if (!storedTokens) { + if (options.treatMissingAsExternalUpdate) this.cachedToken = null; + return options.treatMissingAsExternalUpdate ? null : undefined; + } + if (areSameTokenData(storedTokens, failedTokens)) return undefined; + + this.cachedToken = storedTokens; + return storedTokens; + } +} + +function areSameTokenData(a: TokenData, b: TokenData): boolean { + return ( + a.accessToken === b.accessToken && + a.refreshToken === b.refreshToken && + a.expiresAt === b.expiresAt && + a.createdAt === b.createdAt + ); } diff --git a/src/shared/code-navigation-error-map.test.ts b/src/shared/code-navigation-error-map.test.ts index d958cc3e..50915160 100644 --- a/src/shared/code-navigation-error-map.test.ts +++ b/src/shared/code-navigation-error-map.test.ts @@ -150,6 +150,7 @@ describe("mapCodeNavigationError", () => { code: "AUTH_REQUIRED", message: "Login required", retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, }); }); diff --git a/src/shared/code-navigation-error-map.ts b/src/shared/code-navigation-error-map.ts index 600b5191..62321a16 100644 --- a/src/shared/code-navigation-error-map.ts +++ b/src/shared/code-navigation-error-map.ts @@ -38,6 +38,7 @@ export type MappedErrorCode = | "UNKNOWN"; export interface MappedErrorDetails { + action?: string; availableVersions?: AvailableVersion[]; indexingRef?: string; status?: number; @@ -176,6 +177,7 @@ function classify(error: unknown): MappedError { code: "AUTH_REQUIRED", message: error.message, retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, }; } if (error instanceof CodeNavigationNetworkError) { diff --git a/src/shared/package-intelligence-error-map.test.ts b/src/shared/package-intelligence-error-map.test.ts index c8083b1e..d8b27e9e 100644 --- a/src/shared/package-intelligence-error-map.test.ts +++ b/src/shared/package-intelligence-error-map.test.ts @@ -97,9 +97,13 @@ describe("mapPackageIntelligenceError", () => { it("maps AuthenticationError to AUTH_REQUIRED", () => { expect( - mapPackageIntelligenceError(new AuthenticationError("login required")) - .code, - ).toBe("AUTH_REQUIRED"); + mapPackageIntelligenceError(new AuthenticationError("login required")), + ).toEqual({ + code: "AUTH_REQUIRED", + message: "login required", + retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, + }); }); it("maps PackageIntelligenceNetworkError to NETWORK (retryable)", () => { diff --git a/src/shared/package-intelligence-error-map.ts b/src/shared/package-intelligence-error-map.ts index c5930d04..7cb82142 100644 --- a/src/shared/package-intelligence-error-map.ts +++ b/src/shared/package-intelligence-error-map.ts @@ -111,6 +111,7 @@ function classify(error: unknown): MappedError { code: "AUTH_REQUIRED", message: error.message, retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, }; } if (error instanceof PackageIntelligenceNetworkError) { diff --git a/src/tools/search-language.test.ts b/src/tools/search-language.test.ts index 8c5270b3..7739cbfd 100644 --- a/src/tools/search-language.test.ts +++ b/src/tools/search-language.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, mock } from "bun:test"; +import { AuthenticationError } from "../services/githits-service.js"; import { createMockGitHitsService } from "../services/test-helpers.js"; import { createSearchLanguageTool } from "./search-language.js"; import type { ToolResult } from "./types.js"; @@ -33,4 +34,24 @@ describe("searchLanguageTool", () => { expect(result.isError).toBe(true); expect(getText(result)).toContain("API error"); }); + + it("returns recoverable AUTH_REQUIRED envelope on auth failure", async () => { + const service = createMockGitHitsService({ + getLanguages: mock(() => + Promise.reject(new AuthenticationError("Authentication required")), + ), + }); + const tool = createSearchLanguageTool(service); + + const result = await tool.handler({ query: "python" }, {}); + const parsed = JSON.parse(getText(result)); + + expect(result.isError).toBe(true); + expect(parsed).toEqual({ + error: "Authentication required", + code: "AUTH_REQUIRED", + retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, + }); + }); }); diff --git a/src/tools/shared.ts b/src/tools/shared.ts index a8150040..0e3e4f15 100644 --- a/src/tools/shared.ts +++ b/src/tools/shared.ts @@ -22,14 +22,16 @@ interface ToolErrorEnvelope { error: string; code: string; retryable: boolean; + details?: { action: string }; } function classify(operation: string, error: unknown): ToolErrorEnvelope { if (error instanceof AuthenticationError) { return { error: error.message, - code: "UNAUTHENTICATED", + code: "AUTH_REQUIRED", retryable: false, + details: { action: "Run `githits login`, then retry this tool call." }, }; } const message = error instanceof Error ? error.message : "Unknown error";