diff --git a/docs/implementation/auth.md b/docs/implementation/auth.md index 788a6db..84a00db 100644 --- a/docs/implementation/auth.md +++ b/docs/implementation/auth.md @@ -50,7 +50,8 @@ Tokens are JWTs with a configurable expiration (typically 1 hour). The CLI handl - **Shared retry helper** — GitHits REST calls and package/source service calls both use the same token-refresh/retry flow, so auth drift is handled consistently across both service families. - **Concurrent coalescing** — Soft refreshes from `getToken()` coalesce with each other, and strict refreshes from `forceRefresh()` coalesce with each other. A strict refresh waits for any in-flight soft refresh to finish, then refreshes the latest stored token instead of reusing a soft result that may not have hit the token endpoint. Once a strict refresh is active, later `getToken()` calls join it instead of serving cached credentials, even if the cached token still looks time-valid. Refresh attempts run inside the auth storage lock, covering storage reload, token endpoint refresh, and token save/clear as one cross-process transaction. The lock is per OS user, not per active config directory, because keychain credentials are shared even when `APPDATA` or `XDG_CONFIG_HOME` differs between agents. This prevents parallel agents from spending the same rotating refresh token at the same time. Storage writes still use compare-and-swap helpers as a defensive guard. Before refreshing a cached token, the manager reloads storage so long-running MCP servers use credentials written by a separate login/refresh. - **Terminal refresh failures** — Supabase OAuth refresh failures such as `invalid_client`, deleted client registrations, revoked sessions, or refresh-token reuse (`Invalid Refresh Token: Already Used`) are not retried as transient errors. The CLI clears stale token state immediately, and clears the stored client registration for invalid-client failures so the next login performs fresh dynamic client registration. -- **Transient refresh failures** — Transport, timeout, and 5xx failures never clear refresh credentials. If the access token is expired, the current call returns no token, leaves the expired candidate cached and persisted, and the next `getToken()` call attempts refresh again. A later successful refresh persists token rotation normally; the expired access token is never served. +- **Transient refresh failures** — Transport, timeout, and 5xx failures never clear refresh credentials. If the access token is expired, the current authenticated call surfaces the underlying refresh failure instead of misreporting that no local token exists, while leaving the expired candidate cached and persisted so the next `getToken()` call attempts refresh again. Status/token probes retain their non-throwing expired-auth behavior. A later successful refresh persists token rotation normally; the expired access token is never served. +- **Missing client registration** — Refresh requires the stored dynamic OAuth client registration as well as the refresh token. If tokens exist but that companion keychain entry is missing or unreadable, authenticated calls return an explicit re-login error instead of the generic missing-token message. - **Active-backend-scoped automatic clears** — Automatic refresh-failure cleanup (`TokenManager`) and login's client re-registration cleanup clear only the **active storage backend class**, never the inactive one. The active class is everything the active-mode load reads: keychain in keychain mode; the file store plus all legacy plaintext stores in file mode (a leftover legacy copy would otherwise be re-migrated on the next load and resurrect the just-cleared credential). This prevents a stale credential in an inactive backend — e.g. a keychain token left over from a launch without `GITHITS_AUTH_STORAGE=file` — from wiping the good credential in the mode the user actually runs. Only explicit `logout` (`clearAuthSession`) clears every backend. The composite methods are `clearActiveTokensIfUnchanged` / `clearActiveClient` on `AuthStorage`; single-backend stores delegate them to the unscoped clear. - **At login** (`src/commands/login.ts`) — Checks if existing token is still valid before starting the OAuth flow. Respects `--force` flag to re-authenticate regardless. - **At init** (`src/commands/init/init.ts`) — Resolves auth through `createContainer()` at the login step so standard token refresh runs before falling back to browser login. diff --git a/src/services/token-manager.integration.test.ts b/src/services/token-manager.integration.test.ts index e80b73d..95e3f6a 100644 --- a/src/services/token-manager.integration.test.ts +++ b/src/services/token-manager.integration.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, mock } from "bun:test"; import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { CodeNavigationServiceImpl } from "@githits/core-internal"; import { type RefreshTokenResponse, TokenRefreshError, @@ -111,7 +112,9 @@ describe("TokenManager file-backed integration", () => { mcpUrl: baseUrl, }); - expect(await firstManager.getToken()).toBeUndefined(); + await expect(firstManager.getToken()).rejects.toThrow( + "network unavailable", + ); expect(await secondStorage.loadTokens(baseUrl)).toEqual(initial); const refreshAccessToken = mock((_request: { refreshToken: string }) => @@ -137,6 +140,123 @@ describe("TokenManager file-backed integration", () => { }); }); + it("silently refreshes an expired stored login before a search call", async () => { + const { firstStorage } = await createRealStorages(); + await firstStorage.saveAuthSession( + baseUrl, + defaultClientRegistration, + createExpiredToken({ + accessToken: "expired-access-token", + refreshToken: "stored-refresh-token", + }), + ); + const refreshAccessToken = mock(() => + Promise.resolve({ + accessToken: "refreshed-access-token", + refreshToken: "rotated-refresh-token", + expiresIn: 3600, + }), + ); + const manager = new TokenManager({ + authService: createMockAuthService({ refreshAccessToken }), + authStorage: firstStorage, + mcpUrl: baseUrl, + }); + const fetchFn = mock((_url: string | URL | Request, _init?: RequestInit) => + Promise.resolve( + new Response( + JSON.stringify({ + data: { + search: { + completed: true, + searchRef: "search-ref-123", + result: { + query: "test", + queryWarnings: [], + sources: ["CODE"], + results: [], + page: { + offset: 0, + limit: 20, + returned: 0, + hasMore: false, + }, + partialResults: false, + sourceStatus: [], + }, + progress: null, + }, + }, + }), + { headers: { "Content-Type": "application/json" } }, + ), + ), + ) as unknown as typeof fetch; + const service = new CodeNavigationServiceImpl( + "https://pkgseer.dev", + manager, + fetchFn, + ); + + const result = await service.search({ + targets: [{ registry: "NPM", packageName: "express" }], + query: "test", + }); + expect(result.state).toBe("completed"); + expect(refreshAccessToken).toHaveBeenCalledWith( + expect.objectContaining({ refreshToken: "stored-refresh-token" }), + ); + expect(fetchFn).toHaveBeenCalledTimes(1); + const request = (fetchFn as unknown as ReturnType).mock + .calls[0]?.[1] as RequestInit | undefined; + expect(request?.headers).toMatchObject({ + Authorization: "Bearer refreshed-access-token", + }); + expect(await firstStorage.loadTokens(baseUrl)).toMatchObject({ + accessToken: "refreshed-access-token", + refreshToken: "rotated-refresh-token", + }); + }); + + it("preserves the real refresh failure instead of reporting a missing local token", async () => { + const { firstStorage } = await createRealStorages(); + const expired = createExpiredToken({ + accessToken: "expired-access-token", + refreshToken: "stored-refresh-token", + }); + await firstStorage.saveAuthSession( + baseUrl, + defaultClientRegistration, + expired, + ); + const manager = new TokenManager({ + authService: createMockAuthService({ + refreshAccessToken: mock(() => + Promise.reject(new Error("refresh transport unavailable")), + ), + }), + authStorage: firstStorage, + mcpUrl: baseUrl, + }); + const fetchFn = mock(() => + Promise.reject(new Error("backend should not be called")), + ) as unknown as typeof fetch; + const service = new CodeNavigationServiceImpl( + "https://pkgseer.dev", + manager, + fetchFn, + ); + + await expect( + service.search({ + targets: [{ registry: "NPM", packageName: "express" }], + query: "test", + }), + ).rejects.toThrow("refresh transport unavailable"); + expect(fetchFn).not.toHaveBeenCalled(); + expect(await firstStorage.loadTokens(baseUrl)).toEqual(expired); + }); + it("serializes endpoint refresh when rotation invalidates concurrent refreshes", async () => { const { firstStorage, secondStorage } = await createRealStorages(); const initial = createExpiredToken({ @@ -397,7 +517,7 @@ describe("TokenManager file-backed integration", () => { ); rejectRefresh(new Error("refresh failed")); - expect(await result).toBeUndefined(); + await expect(result).rejects.toThrow("refresh failed"); await externalLoginWrite; expect(await firstStorage.loadTokens(baseUrl)).toEqual(externalLogin); expect(refreshAccessToken).toHaveBeenCalledTimes(1); diff --git a/src/services/token-manager.test.ts b/src/services/token-manager.test.ts index 6e7b748..3f1400a 100644 --- a/src/services/token-manager.test.ts +++ b/src/services/token-manager.test.ts @@ -482,7 +482,7 @@ describe("TokenManager", () => { authStorage, }); - expect(await manager.getToken()).toBeUndefined(); + await expect(manager.getToken()).rejects.toThrow("network unavailable"); expect(storedToken).toEqual(tokenData); expect(authStorage.clearActiveTokensIfUnchanged).not.toHaveBeenCalled(); @@ -516,9 +516,7 @@ describe("TokenManager", () => { }), }); - const result = await manager.forceRefresh(); - - expect(result).toBeUndefined(); + await expect(manager.forceRefresh()).rejects.toThrow(FetchTimeoutError); expect(authStorage.clearActiveTokensIfUnchanged).not.toHaveBeenCalled(); }); @@ -547,7 +545,7 @@ describe("TokenManager", () => { }), }); - expect(await manager.getToken()).toBeUndefined(); + await expect(manager.getToken()).rejects.toThrow(TokenRefreshError); expect(authStorage.clearActiveTokensIfUnchanged).not.toHaveBeenCalled(); expect(authStorage.clearActiveClient).not.toHaveBeenCalled(); }); @@ -580,7 +578,7 @@ describe("TokenManager", () => { expect(refreshMock).toHaveBeenCalledTimes(1); }); - it("returns undefined when client registration is missing", async () => { + it("reports when stored tokens cannot be refreshed because the client registration is missing", async () => { const tokenData = createValidTokenData({ createdAt: new Date(Date.now() - 7200_000).toISOString(), expiresAt: new Date(Date.now() - 60_000).toISOString(), @@ -592,8 +590,9 @@ describe("TokenManager", () => { }), }); - const result = await manager.getToken(); - expect(result).toBeUndefined(); + await expect(manager.getToken()).rejects.toThrow( + "OAuth client registration is missing or unreadable", + ); }); }); @@ -619,7 +618,7 @@ describe("TokenManager", () => { expect(authService.refreshAccessToken).toHaveBeenCalledTimes(1); }); - it("returns undefined when refresh fails with valid token", async () => { + it("propagates a forced refresh failure while retaining a valid token", async () => { const tokenData = createValidTokenData({ createdAt: new Date(Date.now() - 60_000).toISOString(), expiresAt: new Date(Date.now() + 3600_000).toISOString(), @@ -639,8 +638,7 @@ describe("TokenManager", () => { // Populate cache await manager.getToken(); - const result = await manager.forceRefresh(); - expect(result).toBeUndefined(); + await expect(manager.forceRefresh()).rejects.toThrow("refresh failed"); // Should NOT clear tokens since the token is still valid (not expired) expect(authStorage.clearActiveTokensIfUnchanged).not.toHaveBeenCalled(); }); @@ -1063,11 +1061,19 @@ describe("TokenManager", () => { const forceResult = manager.forceRefresh(); await refreshStarted; const getTokenDuringForce = manager.getToken(); + const results = Promise.allSettled([forceResult, getTokenDuringForce]); rejectRefresh(new Error("refresh failed")); - await expect(forceResult).resolves.toBeUndefined(); - await expect(getTokenDuringForce).resolves.toBeUndefined(); + const [forceOutcome, getTokenOutcome] = await results; + expect(forceOutcome).toMatchObject({ + status: "rejected", + reason: expect.objectContaining({ message: "refresh failed" }), + }); + expect(getTokenOutcome).toMatchObject({ + status: "rejected", + reason: expect.objectContaining({ message: "refresh failed" }), + }); expect(refreshAccessToken).toHaveBeenCalledTimes(1); }); diff --git a/src/services/token-manager.ts b/src/services/token-manager.ts index b4bf267..0bcb7b1 100644 --- a/src/services/token-manager.ts +++ b/src/services/token-manager.ts @@ -1,5 +1,8 @@ -import type { TokenProvider } from "@githits/core-internal"; -import { withTelemetrySpan } from "@githits/core-internal"; +import { + AuthenticationError, + type TokenProvider, + withTelemetrySpan, +} from "@githits/core-internal"; import type { AuthDiagnosticsStore } from "./auth-diagnostics-storage.js"; import { type AuthService, @@ -26,6 +29,8 @@ export interface TokenManagerDeps { authService: AuthService; authStorage: LockingAuthStorage; mcpUrl: string; + /** Return undefined for refresh failures in local status/token probes. */ + refreshFailureMode?: "throw" | "return-undefined"; /** * Optional diagnostics breadcrumb store. When present, terminal refresh * failures that clear the token record why, so `doctor` can explain it later. @@ -82,7 +87,12 @@ export async function refreshExpiredToken( authStorage: LockingAuthStorage, mcpUrl: string, ): Promise { - const manager = new TokenManager({ authService, authStorage, mcpUrl }); + const manager = new TokenManager({ + authService, + authStorage, + mcpUrl, + refreshFailureMode: "return-undefined", + }); return manager.forceRefresh(); } @@ -94,6 +104,7 @@ export class TokenManager implements TokenProvider { private readonly authService: AuthService; private readonly authStorage: LockingAuthStorage; private readonly mcpUrl: string; + private readonly refreshFailureMode: "throw" | "return-undefined"; private readonly authDiagnostics?: AuthDiagnosticsStore; private cachedToken: TokenData | null = null; private softRefreshPromise: Promise | null = null; @@ -103,6 +114,7 @@ export class TokenManager implements TokenProvider { this.authService = deps.authService; this.authStorage = deps.authStorage; this.mcpUrl = deps.mcpUrl; + this.refreshFailureMode = deps.refreshFailureMode ?? "throw"; this.authDiagnostics = deps.authDiagnostics; } @@ -140,8 +152,17 @@ export class TokenManager implements TokenProvider { return currentToken; } - // Attempt refresh (coalesced if already in-flight) - const refresh = await this.refreshFromGetToken(); + // Attempt refresh (coalesced if already in-flight). A proactive refresh + // failure must not block a still-valid access token, but once the access + // token is expired the caller needs the real failure instead of a false + // "no local token" result. + let refresh: RefreshResult; + try { + refresh = await this.refreshFromGetToken(); + } catch (error) { + if (!expired) return currentToken; + throw error; + } if (refresh.accessToken) { return refresh.accessToken; @@ -228,7 +249,15 @@ export class TokenManager implements TokenProvider { "token-manager.load-client", () => this.authStorage.loadClient(this.mcpUrl), ); - if (!client) return refreshResult(undefined, false); + if (!client) { + if (this.refreshFailureMode === "return-undefined") { + return refreshResult(undefined, false); + } + throw new AuthenticationError( + "Stored GitHits credentials cannot be refreshed because the OAuth client registration is missing or unreadable.", + "local", + ); + } let response: RefreshTokenResponse; try { @@ -273,7 +302,10 @@ export class TokenManager implements TokenProvider { return refreshResult(currentStoredTokens.accessToken, false); } } - return refreshResult(undefined, false); + if (this.refreshFailureMode === "return-undefined") { + return refreshResult(undefined, false); + } + throw error; } const newTokenData: TokenData = {