diff --git a/biome.json b/biome.json index a5b2e9c0..84afa11c 100644 --- a/biome.json +++ b/biome.json @@ -19,5 +19,17 @@ "rules": { "recommended": true } - } + }, + "overrides": [ + { + "includes": ["**/*.test.ts"], + "linter": { + "rules": { + "style": { + "noNonNullAssertion": "off" + } + } + } + } + ] } diff --git a/src/commands/auth-status.test.ts b/src/commands/auth-status.test.ts index b5cafbfa..5028afb9 100644 --- a/src/commands/auth-status.test.ts +++ b/src/commands/auth-status.test.ts @@ -4,7 +4,6 @@ import { createMockAuthStorage, createValidTokenData, defaultClientRegistration, - defaultTokenResponse, } from "../services/test-helpers.js"; import { authStatusAction } from "./auth-status.js"; diff --git a/src/commands/login.test.ts b/src/commands/login.test.ts index 525e1f22..9043586c 100644 --- a/src/commands/login.test.ts +++ b/src/commands/login.test.ts @@ -152,6 +152,164 @@ describe("loginAction", () => { consoleSpy.mockRestore(); }); + it("clears stale client registration when tokens are absent", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const authStorage = createMockAuthStorage(); + const authService = createMockAuthService(); + + await loginAction( + { port: 8080 }, + { + authService, + authStorage, + browserService: createMockBrowserService(), + mcpUrl, + }, + ); + + expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); + expect(authService.registerClient).toHaveBeenCalled(); + expect(authStorage.saveClient).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it("clears client registration on token exchange failure", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + const authStorage = createMockAuthStorage(); + const authService = createMockAuthService({ + exchangeCodeForTokens: mock(() => { + throw new Error("invalid_grant"); + }), + }); + + try { + await loginAction( + { port: 8080 }, + { + authService, + authStorage, + browserService: createMockBrowserService(), + mcpUrl, + }, + ); + } catch { + // Expected: process.exit mock throws + } + + expect(authStorage.clearClient).toHaveBeenCalledWith(mcpUrl); + expect(exitSpy).toHaveBeenCalledWith(1); + + consoleSpy.mockRestore(); + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + it("still shows error and exits when clearClient fails during token exchange error", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit"); + }); + + let clearClientCallCount = 0; + const authStorage = createMockAuthStorage({ + clearClient: mock(() => { + clearClientCallCount++; + // First call is from the !existing path (Part 1), let it succeed. + // Second call is from the catch block (Part 2), make it fail. + if (clearClientCallCount > 1) { + throw new Error("fs error"); + } + return Promise.resolve(); + }), + }); + const authService = createMockAuthService({ + exchangeCodeForTokens: mock(() => { + throw new Error("invalid_grant"); + }), + }); + + try { + await loginAction( + { port: 8080 }, + { + authService, + authStorage, + browserService: createMockBrowserService(), + mcpUrl, + }, + ); + } catch { + // Expected: process.exit mock throws + } + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + + consoleSpy.mockRestore(); + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + it("does not clear client when tokens are still valid", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const authStorage = createMockAuthStorage({ + loadTokens: mock(() => + Promise.resolve( + createValidTokenData({ + expiresAt: new Date(Date.now() + 3600_000).toISOString(), + }), + ), + ), + }); + + await loginAction( + {}, + { + authService: createMockAuthService(), + authStorage, + browserService: createMockBrowserService(), + mcpUrl, + }, + ); + + expect(authStorage.clearClient).not.toHaveBeenCalled(); + expect(authStorage.saveTokens).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it("does not clear client when tokens are expired but present", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const authStorage = createMockAuthStorage({ + loadTokens: mock(() => + Promise.resolve( + createValidTokenData({ + expiresAt: new Date(Date.now() - 3600_000).toISOString(), + }), + ), + ), + }); + + await loginAction( + { port: 8080 }, + { + authService: createMockAuthService(), + authStorage, + browserService: createMockBrowserService(), + mcpUrl, + }, + ); + + expect(authStorage.clearClient).not.toHaveBeenCalled(); + expect(authStorage.saveTokens).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + it("proceeds with login when token is expired", async () => { const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); const authStorage = createMockAuthStorage({ diff --git a/src/commands/login.ts b/src/commands/login.ts index 27132895..e6b5b146 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -59,6 +59,12 @@ export async function loginAction( console.log("Re-authenticating (--force flag)...\n"); } + // If tokens were cleared (expired+refreshFailed or never existed) but a stale + // client registration remains, clear it so we get a fresh DCR registration. + if (!existing) { + await authStorage.clearClient(mcpUrl); + } + // Step 1: Discover OAuth endpoints console.log("Discovering OAuth endpoints..."); const metadata = await authService.discoverEndpoints(mcpUrl); @@ -183,6 +189,13 @@ export async function loginAction( redirectUri, }); } catch (error) { + // Best-effort: clear potentially stale client so next login starts fresh. + // Swallow clearClient errors to ensure we always reach the user-facing error path. + try { + await authStorage.clearClient(mcpUrl); + } catch { + // Ignore -- client cleanup is best-effort + } console.error( `Failed to complete authentication: ${error instanceof Error ? error.message : error}\n`, ); diff --git a/src/services/chunking-keyring-service.test.ts b/src/services/chunking-keyring-service.test.ts index 09893342..5134892f 100644 --- a/src/services/chunking-keyring-service.test.ts +++ b/src/services/chunking-keyring-service.test.ts @@ -222,7 +222,7 @@ describe("ChunkingKeyringService", () => { expect(sentinel).toMatch(/^CHUNKED:[a-z0-9]+:3$/); // Parse sentinel to find writeId and verify chunks - const parsed = parseChunkedSentinel(sentinel!); + const parsed = parseChunkedSentinel(sentinel as string); expect(parsed).not.toBeNull(); expect( inner.getPassword(SERVICE, chunkKey(ACCOUNT, parsed!.writeId, 0)), @@ -242,7 +242,7 @@ describe("ChunkingKeyringService", () => { // Write a chunked value chunking.setPassword(SERVICE, ACCOUNT, "a".repeat(25)); const oldSentinel = parseChunkedSentinel( - inner.getPassword(SERVICE, ACCOUNT)!, + inner.getPassword(SERVICE, ACCOUNT) as string, ); expect(oldSentinel).not.toBeNull(); @@ -268,13 +268,13 @@ describe("ChunkingKeyringService", () => { // Write 3-chunk value chunking.setPassword(SERVICE, ACCOUNT, "a".repeat(25)); const oldSentinel = parseChunkedSentinel( - inner.getPassword(SERVICE, ACCOUNT)!, + inner.getPassword(SERVICE, ACCOUNT) as string, ); // Overwrite with 2-chunk value chunking.setPassword(SERVICE, ACCOUNT, "b".repeat(15)); const newSentinel = parseChunkedSentinel( - inner.getPassword(SERVICE, ACCOUNT)!, + inner.getPassword(SERVICE, ACCOUNT) as string, ); expect(newSentinel!.count).toBe(2); @@ -311,10 +311,14 @@ describe("ChunkingKeyringService", () => { const chunking = new ChunkingKeyringService(inner, 10); chunking.setPassword(SERVICE, ACCOUNT, "a".repeat(15)); - const first = parseChunkedSentinel(inner.getPassword(SERVICE, ACCOUNT)!); + const first = parseChunkedSentinel( + inner.getPassword(SERVICE, ACCOUNT) as string, + ); chunking.setPassword(SERVICE, ACCOUNT, "b".repeat(15)); - const second = parseChunkedSentinel(inner.getPassword(SERVICE, ACCOUNT)!); + const second = parseChunkedSentinel( + inner.getPassword(SERVICE, ACCOUNT) as string, + ); expect(first!.writeId).not.toBe(second!.writeId); }); @@ -371,7 +375,7 @@ describe("ChunkingKeyringService", () => { const chunking = new ChunkingKeyringService(inner, 10); chunking.setPassword(SERVICE, ACCOUNT, "a".repeat(25)); const sentinel = parseChunkedSentinel( - inner.getPassword(SERVICE, ACCOUNT)!, + inner.getPassword(SERVICE, ACCOUNT) as string, ); const result = chunking.deletePassword(SERVICE, ACCOUNT); @@ -414,15 +418,14 @@ describe("ChunkingKeyringService", () => { const sentinel = inner.getPassword(SERVICE, ACCOUNT); expect(sentinel).toMatch(/^CHUNKED:/); - const parsed = parseChunkedSentinel(sentinel!); - expect(parsed!.count).toBe(2); + const parsed = parseChunkedSentinel(sentinel as string); + expect(parsed?.count).toBe(2); }); it("round-trips a chunked value through set and get", () => { const inner = createInMemoryKeyring(); const chunking = new ChunkingKeyringService(inner, 10); - const value = - '{"accessToken":"' + "a".repeat(50) + '","refreshToken":"b"}'; + const value = `{"accessToken":"${"a".repeat(50)}","refreshToken":"b"}`; chunking.setPassword(SERVICE, ACCOUNT, value); const result = chunking.getPassword(SERVICE, ACCOUNT); diff --git a/src/services/token-manager.test.ts b/src/services/token-manager.test.ts index c2c7aafb..c9238a12 100644 --- a/src/services/token-manager.test.ts +++ b/src/services/token-manager.test.ts @@ -1,11 +1,9 @@ import { describe, expect, it, mock } from "bun:test"; -import type { TokenData } from "./auth-storage.js"; import { createMockAuthService, createMockAuthStorage, createValidTokenData, defaultClientRegistration, - defaultOAuthMetadata, defaultTokenResponse, } from "./test-helpers.js"; import {