Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/implementation/auth.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
124 changes: 122 additions & 2 deletions src/services/token-manager.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 }) =>
Expand All @@ -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<typeof mock>).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({
Expand Down Expand Up @@ -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);
Expand Down
32 changes: 19 additions & 13 deletions src/services/token-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();
});

Expand Down Expand Up @@ -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();
});
Expand Down Expand Up @@ -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(),
Expand All @@ -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",
);
});
});

Expand All @@ -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(),
Expand All @@ -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();
});
Expand Down Expand Up @@ -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);
});

Expand Down
46 changes: 39 additions & 7 deletions src/services/token-manager.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -82,7 +87,12 @@ export async function refreshExpiredToken(
authStorage: LockingAuthStorage,
mcpUrl: string,
): Promise<string | undefined> {
const manager = new TokenManager({ authService, authStorage, mcpUrl });
const manager = new TokenManager({
authService,
authStorage,
mcpUrl,
refreshFailureMode: "return-undefined",
});
return manager.forceRefresh();
}

Expand All @@ -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<RefreshResult> | null = null;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 = {
Expand Down