chore: address oauth PR concerns (#1184)

2025-09-08 01:15:19 +02:00
parent 7cf4004038
commit 15900d9fd5
23 changed files with 1042 additions and 139 deletions
--- a/packages/tm-core/src/auth/auth-manager.test.ts
+++ b/packages/tm-core/src/auth/auth-manager.test.ts
@@ -0,0 +1,150 @@
+/**
+ * Tests for AuthManager singleton behavior
+ */
+
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+
+// Mock the logger to verify warnings (must be hoisted before SUT import)
+const mockLogger = {
+	warn: vi.fn(),
+	info: vi.fn(),
+	debug: vi.fn(),
+	error: vi.fn()
+};
+
+vi.mock('../logger/index.js', () => ({
+	getLogger: () => mockLogger
+}));
+
+// Spy on CredentialStore constructor to verify config propagation
+const CredentialStoreSpy = vi.fn();
+vi.mock('./credential-store.js', () => {
+	return {
+		CredentialStore: class {
+			constructor(config: any) {
+				CredentialStoreSpy(config);
+				this.getCredentials = vi.fn(() => null);
+			}
+			getCredentials() {
+				return null;
+			}
+			saveCredentials() {}
+			clearCredentials() {}
+			hasValidCredentials() {
+				return false;
+			}
+		}
+	};
+});
+
+// Mock OAuthService to avoid side effects
+vi.mock('./oauth-service.js', () => {
+	return {
+		OAuthService: class {
+			constructor() {}
+			authenticate() {
+				return Promise.resolve({});
+			}
+			getAuthorizationUrl() {
+				return null;
+			}
+		}
+	};
+});
+
+// Mock SupabaseAuthClient to avoid side effects
+vi.mock('../clients/supabase-client.js', () => {
+	return {
+		SupabaseAuthClient: class {
+			constructor() {}
+			refreshSession() {
+				return Promise.resolve({});
+			}
+			signOut() {
+				return Promise.resolve();
+			}
+		}
+	};
+});
+
+// Import SUT after mocks
+import { AuthManager } from './auth-manager.js';
+
+describe('AuthManager Singleton', () => {
+	beforeEach(() => {
+		// Reset singleton before each test
+		AuthManager.resetInstance();
+		vi.clearAllMocks();
+		CredentialStoreSpy.mockClear();
+	});
+
+	it('should return the same instance on multiple calls', () => {
+		const instance1 = AuthManager.getInstance();
+		const instance2 = AuthManager.getInstance();
+
+		expect(instance1).toBe(instance2);
+	});
+
+	it('should use config on first call', () => {
+		const config = {
+			baseUrl: 'https://test.auth.com',
+			configDir: '/test/config',
+			configFile: '/test/config/auth.json'
+		};
+
+		const instance = AuthManager.getInstance(config);
+		expect(instance).toBeDefined();
+
+		// Assert that CredentialStore was constructed with the provided config
+		expect(CredentialStoreSpy).toHaveBeenCalledTimes(1);
+		expect(CredentialStoreSpy).toHaveBeenCalledWith(config);
+
+		// Verify the config is passed to internal components through observable behavior
+		// getCredentials would look in the configured file path
+		const credentials = instance.getCredentials();
+		expect(credentials).toBeNull(); // File doesn't exist, but config was propagated correctly
+	});
+
+	it('should warn when config is provided after initialization', () => {
+		// Clear previous calls
+		mockLogger.warn.mockClear();
+
+		// First call with config
+		AuthManager.getInstance({ baseUrl: 'https://first.auth.com' });
+
+		// Second call with different config
+		AuthManager.getInstance({ baseUrl: 'https://second.auth.com' });
+
+		// Verify warning was logged
+		expect(mockLogger.warn).toHaveBeenCalledWith(
+			expect.stringMatching(/config.*after initialization.*ignored/i)
+		);
+	});
+
+	it('should not warn when no config is provided after initialization', () => {
+		// Clear previous calls
+		mockLogger.warn.mockClear();
+
+		// First call with config
+		AuthManager.getInstance({ configDir: '/test/config' });
+
+		// Second call without config
+		AuthManager.getInstance();
+
+		// Verify no warning was logged
+		expect(mockLogger.warn).not.toHaveBeenCalled();
+	});
+
+	it('should allow resetting the instance', () => {
+		const instance1 = AuthManager.getInstance();
+
+		// Reset the instance
+		AuthManager.resetInstance();
+
+		// Get new instance
+		const instance2 = AuthManager.getInstance();
+
+		// They should be different instances
+		expect(instance1).not.toBe(instance2);
+	});
+});
--- a/packages/tm-core/src/auth/auth-manager.ts
+++ b/packages/tm-core/src/auth/auth-manager.ts
@@ -7,16 +7,17 @@ import {
 	OAuthFlowOptions,
 	AuthenticationError,
 	AuthConfig
-} from './types';
-import { CredentialStore } from './credential-store';
-import { OAuthService } from './oauth-service';
-import { SupabaseAuthClient } from '../clients/supabase-client';
+} from './types.js';
+import { CredentialStore } from './credential-store.js';
+import { OAuthService } from './oauth-service.js';
+import { SupabaseAuthClient } from '../clients/supabase-client.js';
+import { getLogger } from '../logger/index.js';

 /**
 * Authentication manager class
 */
 export class AuthManager {
-	private static instance: AuthManager;
+	private static instance: AuthManager | null = null;
 	private credentialStore: CredentialStore;
 	private oauthService: OAuthService;
 	private supabaseClient: SupabaseAuthClient;
@@ -33,10 +34,23 @@ export class AuthManager {
 	static getInstance(config?: Partial<AuthConfig>): AuthManager {
 		if (!AuthManager.instance) {
 			AuthManager.instance = new AuthManager(config);
+		} else if (config) {
+			// Warn if config is provided after initialization
+			const logger = getLogger('AuthManager');
+			logger.warn(
+				'getInstance called with config after initialization; config is ignored.'
+			);
 		}
 		return AuthManager.instance;
 	}

+	/**
+	 * Reset the singleton instance (useful for testing)
+	 */
+	static resetInstance(): void {
+		AuthManager.instance = null;
+	}
+
 	/**
 	 * Get stored authentication credentials
 	 */
@@ -60,29 +74,6 @@ export class AuthManager {
 		return this.oauthService.getAuthorizationUrl();
 	}

-	/**
-	 * Authenticate with API key
-	 * Note: This would require a custom implementation or Supabase RLS policies
-	 */
-	async authenticateWithApiKey(apiKey: string): Promise<AuthCredentials> {
-		const token = apiKey.trim();
-		if (!token || token.length < 10) {
-			throw new AuthenticationError('Invalid API key', 'INVALID_API_KEY');
-		}
-
-		const authData: AuthCredentials = {
-			token,
-			tokenType: 'api_key',
-			userId: 'api-user',
-			email: undefined,
-			expiresAt: undefined, // API keys don't expire
-			savedAt: new Date().toISOString()
-		};
-
-		this.credentialStore.saveCredentials(authData);
-		return authData;
-	}
-
 	/**
 	 * Refresh authentication token
 	 */
@@ -129,7 +120,7 @@ export class AuthManager {
 			await this.supabaseClient.signOut();
 		} catch (error) {
 			// Log but don't throw - we still want to clear local credentials
-			console.warn('Failed to sign out from Supabase:', error);
+			getLogger('AuthManager').warn('Failed to sign out from Supabase:', error);
 		}

 		// Always clear local credentials (removes auth.json file)
@@ -142,22 +133,4 @@ export class AuthManager {
 	isAuthenticated(): boolean {
 		return this.credentialStore.hasValidCredentials();
 	}
-
-	/**
-	 * Get authorization headers
-	 */
-	getAuthHeaders(): Record<string, string> {
-		const authData = this.getCredentials();
-
-		if (!authData) {
-			throw new AuthenticationError(
-				'Not authenticated. Please authenticate first.',
-				'NOT_AUTHENTICATED'
-			);
-		}
-
-		return {
-			Authorization: `Bearer ${authData.token}`
-		};
-	}
 }
--- a/packages/tm-core/src/auth/config.ts
+++ b/packages/tm-core/src/auth/config.ts
@@ -4,7 +4,7 @@

 import os from 'os';
 import path from 'path';
-import { AuthConfig } from './types';
+import { AuthConfig } from './types.js';

 // Single base domain for all URLs
 // Build-time: process.env.TM_PUBLIC_BASE_DOMAIN gets replaced by tsup's env option
--- a/packages/tm-core/src/auth/credential-store.test.ts
+++ b/packages/tm-core/src/auth/credential-store.test.ts
@@ -0,0 +1,575 @@
+/**
+ * Tests for CredentialStore with numeric and string timestamp handling
+ */
+
+import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest';
+import { CredentialStore } from './credential-store.js';
+import { AuthenticationError } from './types.js';
+import type { AuthCredentials } from './types.js';
+import fs from 'fs';
+import path from 'path';
+import os from 'os';
+
+// Mock fs module
+vi.mock('fs');
+
+// Mock logger
+const mockLogger = {
+	warn: vi.fn(),
+	info: vi.fn(),
+	debug: vi.fn(),
+	error: vi.fn()
+};
+
+vi.mock('../logger/index.js', () => ({
+	getLogger: () => mockLogger
+}));
+
+describe('CredentialStore', () => {
+	let store: CredentialStore;
+	const testDir = '/test/config';
+	const configFile = '/test/config/auth.json';
+
+	beforeEach(() => {
+		vi.clearAllMocks();
+		store = new CredentialStore({
+			configDir: testDir,
+			configFile: configFile,
+			baseUrl: 'https://api.test.com'
+		});
+	});
+
+	afterEach(() => {
+		vi.restoreAllMocks();
+	});
+
+	describe('getCredentials with timestamp migration', () => {
+		it('should handle string ISO timestamp correctly', () => {
+			const futureDate = new Date(Date.now() + 3600000); // 1 hour from now
+			const mockCredentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-123',
+				email: 'test@example.com',
+				expiresAt: futureDate.toISOString(),
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).not.toBeNull();
+			expect(result?.token).toBe('test-token');
+			// The timestamp should be normalized to numeric milliseconds
+			expect(typeof result?.expiresAt).toBe('number');
+			expect(result?.expiresAt).toBe(futureDate.getTime());
+		});
+
+		it('should handle numeric timestamp correctly', () => {
+			const futureTimestamp = Date.now() + 7200000; // 2 hours from now
+			const mockCredentials = {
+				token: 'test-token',
+				userId: 'user-456',
+				email: 'test2@example.com',
+				expiresAt: futureTimestamp,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).not.toBeNull();
+			expect(result?.token).toBe('test-token');
+			// Numeric timestamp should remain as-is
+			expect(typeof result?.expiresAt).toBe('number');
+			expect(result?.expiresAt).toBe(futureTimestamp);
+		});
+
+		it('should reject invalid string timestamp', () => {
+			const mockCredentials = {
+				token: 'test-token',
+				userId: 'user-789',
+				expiresAt: 'invalid-date-string',
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				'No valid expiration time provided for token'
+			);
+		});
+
+		it('should reject NaN timestamp', () => {
+			const mockCredentials = {
+				token: 'test-token',
+				userId: 'user-nan',
+				expiresAt: NaN,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				'No valid expiration time provided for token'
+			);
+		});
+
+		it('should reject Infinity timestamp', () => {
+			const mockCredentials = {
+				token: 'test-token',
+				userId: 'user-inf',
+				expiresAt: Infinity,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				'No valid expiration time provided for token'
+			);
+		});
+
+		it('should handle missing expiresAt field', () => {
+			const mockCredentials = {
+				token: 'test-token',
+				userId: 'user-no-expiry',
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+				// No expiresAt field
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				'No valid expiration time provided for token'
+			);
+		});
+
+		it('should check token expiration correctly', () => {
+			const expiredTimestamp = Date.now() - 3600000; // 1 hour ago
+			const mockCredentials = {
+				token: 'expired-token',
+				userId: 'user-expired',
+				expiresAt: expiredTimestamp,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				expect.stringContaining('Authentication token has expired'),
+				expect.any(Object)
+			);
+		});
+
+		it('should allow expired tokens when requested', () => {
+			const expiredTimestamp = Date.now() - 3600000; // 1 hour ago
+			const mockCredentials = {
+				token: 'expired-token',
+				userId: 'user-expired',
+				expiresAt: expiredTimestamp,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(
+				JSON.stringify(mockCredentials)
+			);
+
+			const result = store.getCredentials({ allowExpired: true });
+
+			expect(result).not.toBeNull();
+			expect(result?.token).toBe('expired-token');
+		});
+	});
+
+	describe('saveCredentials with timestamp normalization', () => {
+		beforeEach(() => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.mkdirSync).mockImplementation(() => undefined);
+			vi.mocked(fs.writeFileSync).mockImplementation(() => undefined);
+			vi.mocked(fs.renameSync).mockImplementation(() => undefined);
+		});
+
+		it('should normalize string timestamp to ISO string when saving', () => {
+			const futureDate = new Date(Date.now() + 3600000);
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-123',
+				expiresAt: futureDate.toISOString(),
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			store.saveCredentials(credentials);
+
+			expect(fs.writeFileSync).toHaveBeenCalledWith(
+				expect.stringContaining('.tmp'),
+				expect.stringContaining('"expiresAt":'),
+				expect.any(Object)
+			);
+
+			// Check that the written data contains a valid ISO string
+			const writtenData = vi.mocked(fs.writeFileSync).mock
+				.calls[0][1] as string;
+			const parsed = JSON.parse(writtenData);
+			expect(typeof parsed.expiresAt).toBe('string');
+			expect(new Date(parsed.expiresAt).toISOString()).toBe(parsed.expiresAt);
+		});
+
+		it('should convert numeric timestamp to ISO string when saving', () => {
+			const futureTimestamp = Date.now() + 7200000;
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-456',
+				expiresAt: futureTimestamp,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			store.saveCredentials(credentials);
+
+			const writtenData = vi.mocked(fs.writeFileSync).mock
+				.calls[0][1] as string;
+			const parsed = JSON.parse(writtenData);
+			expect(typeof parsed.expiresAt).toBe('string');
+			expect(new Date(parsed.expiresAt).getTime()).toBe(futureTimestamp);
+		});
+
+		it('should reject invalid string timestamp when saving', () => {
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-789',
+				expiresAt: 'invalid-date' as any,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			let err: unknown;
+			try {
+				store.saveCredentials(credentials);
+			} catch (e) {
+				err = e;
+			}
+			expect(err).toBeInstanceOf(AuthenticationError);
+			expect((err as Error).message).toContain('Invalid expiresAt format');
+		});
+
+		it('should reject NaN timestamp when saving', () => {
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-nan',
+				expiresAt: NaN as any,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			let err: unknown;
+			try {
+				store.saveCredentials(credentials);
+			} catch (e) {
+				err = e;
+			}
+			expect(err).toBeInstanceOf(AuthenticationError);
+			expect((err as Error).message).toContain('Invalid expiresAt format');
+		});
+
+		it('should reject Infinity timestamp when saving', () => {
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-inf',
+				expiresAt: Infinity as any,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			let err: unknown;
+			try {
+				store.saveCredentials(credentials);
+			} catch (e) {
+				err = e;
+			}
+			expect(err).toBeInstanceOf(AuthenticationError);
+			expect((err as Error).message).toContain('Invalid expiresAt format');
+		});
+
+		it('should handle missing expiresAt when saving', () => {
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-no-expiry',
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+				// No expiresAt
+			};
+
+			store.saveCredentials(credentials);
+
+			const writtenData = vi.mocked(fs.writeFileSync).mock
+				.calls[0][1] as string;
+			const parsed = JSON.parse(writtenData);
+			expect(parsed.expiresAt).toBeUndefined();
+		});
+
+		it('should not mutate the original credentials object', () => {
+			const originalTimestamp = Date.now() + 3600000;
+			const credentials: AuthCredentials = {
+				token: 'test-token',
+				userId: 'user-123',
+				expiresAt: originalTimestamp,
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			const originalCredentialsCopy = { ...credentials };
+
+			store.saveCredentials(credentials);
+
+			// Original object should not be modified
+			expect(credentials).toEqual(originalCredentialsCopy);
+			expect(credentials.expiresAt).toBe(originalTimestamp);
+		});
+	});
+
+	describe('corrupt file handling', () => {
+		it('should quarantine corrupt file on JSON parse error', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue('invalid json {');
+			vi.mocked(fs.renameSync).mockImplementation(() => undefined);
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(fs.renameSync).toHaveBeenCalledWith(
+				configFile,
+				expect.stringContaining('.corrupt-')
+			);
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				expect.stringContaining('Quarantined corrupt auth file')
+			);
+		});
+
+		it('should handle quarantine failure gracefully', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue('invalid json {');
+			vi.mocked(fs.renameSync).mockImplementation(() => {
+				throw new Error('Permission denied');
+			});
+
+			const result = store.getCredentials();
+
+			expect(result).toBeNull();
+			expect(mockLogger.debug).toHaveBeenCalledWith(
+				expect.stringContaining('Could not quarantine corrupt file')
+			);
+		});
+	});
+
+	describe('clearCredentials', () => {
+		it('should delete the auth file when it exists', () => {
+			// Mock file exists
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.unlinkSync).mockImplementation(() => undefined);
+
+			store.clearCredentials();
+
+			expect(fs.existsSync).toHaveBeenCalledWith('/test/config/auth.json');
+			expect(fs.unlinkSync).toHaveBeenCalledWith('/test/config/auth.json');
+		});
+
+		it('should not throw when auth file does not exist', () => {
+			// Mock file does not exist
+			vi.mocked(fs.existsSync).mockReturnValue(false);
+
+			// Should not throw
+			expect(() => store.clearCredentials()).not.toThrow();
+
+			// Should not try to unlink non-existent file
+			expect(fs.unlinkSync).not.toHaveBeenCalled();
+		});
+
+		it('should throw AuthenticationError when unlink fails', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.unlinkSync).mockImplementation(() => {
+				throw new Error('Permission denied');
+			});
+
+			let err: unknown;
+			try {
+				store.clearCredentials();
+			} catch (e) {
+				err = e;
+			}
+
+			expect(err).toBeInstanceOf(AuthenticationError);
+			expect((err as Error).message).toContain('Failed to clear credentials');
+			expect((err as Error).message).toContain('Permission denied');
+		});
+	});
+
+	describe('hasValidCredentials', () => {
+		it('should return true when valid unexpired credentials exist', () => {
+			const futureDate = new Date(Date.now() + 3600000); // 1 hour from now
+			const credentials = {
+				token: 'valid-token',
+				userId: 'user-123',
+				expiresAt: futureDate.toISOString(),
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials));
+
+			expect(store.hasValidCredentials()).toBe(true);
+		});
+
+		it('should return false when credentials are expired', () => {
+			const pastDate = new Date(Date.now() - 3600000); // 1 hour ago
+			const credentials = {
+				token: 'expired-token',
+				userId: 'user-123',
+				expiresAt: pastDate.toISOString(),
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials));
+
+			expect(store.hasValidCredentials()).toBe(false);
+		});
+
+		it('should return false when no credentials exist', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(false);
+
+			expect(store.hasValidCredentials()).toBe(false);
+		});
+
+		it('should return false when file contains invalid JSON', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue('invalid json {');
+			vi.mocked(fs.renameSync).mockImplementation(() => undefined);
+
+			expect(store.hasValidCredentials()).toBe(false);
+		});
+
+		it('should return false for credentials without expiry', () => {
+			const credentials = {
+				token: 'no-expiry-token',
+				userId: 'user-123',
+				tokenType: 'standard',
+				savedAt: new Date().toISOString()
+			};
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(credentials));
+
+			// Credentials without expiry are considered invalid
+			expect(store.hasValidCredentials()).toBe(false);
+
+			// Should log warning about missing expiration
+			expect(mockLogger.warn).toHaveBeenCalledWith(
+				'No valid expiration time provided for token'
+			);
+		});
+
+		it('should use allowExpired=false by default', () => {
+			// Spy on getCredentials to verify it's called with correct params
+			const getCredentialsSpy = vi.spyOn(store, 'getCredentials');
+
+			vi.mocked(fs.existsSync).mockReturnValue(false);
+			store.hasValidCredentials();
+
+			expect(getCredentialsSpy).toHaveBeenCalledWith({ allowExpired: false });
+		});
+	});
+
+	describe('cleanupCorruptFiles', () => {
+		it('should remove old corrupt files', () => {
+			const now = Date.now();
+			const oldFile = 'auth.json.corrupt-' + (now - 8 * 24 * 60 * 60 * 1000); // 8 days old
+			const newFile = 'auth.json.corrupt-' + (now - 1000); // 1 second old
+
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readdirSync).mockReturnValue([
+				{ name: oldFile, isFile: () => true },
+				{ name: newFile, isFile: () => true },
+				{ name: 'auth.json', isFile: () => true }
+			] as any);
+			vi.mocked(fs.statSync).mockImplementation((filePath) => {
+				if (filePath.includes(oldFile)) {
+					return { mtimeMs: now - 8 * 24 * 60 * 60 * 1000 } as any;
+				}
+				return { mtimeMs: now - 1000 } as any;
+			});
+			vi.mocked(fs.unlinkSync).mockImplementation(() => undefined);
+
+			store.cleanupCorruptFiles();
+
+			expect(fs.unlinkSync).toHaveBeenCalledWith(
+				expect.stringContaining(oldFile)
+			);
+			expect(fs.unlinkSync).not.toHaveBeenCalledWith(
+				expect.stringContaining(newFile)
+			);
+		});
+
+		it('should handle cleanup errors gracefully', () => {
+			vi.mocked(fs.existsSync).mockReturnValue(true);
+			vi.mocked(fs.readdirSync).mockImplementation(() => {
+				throw new Error('Permission denied');
+			});
+
+			// Should not throw
+			expect(() => store.cleanupCorruptFiles()).not.toThrow();
+			expect(mockLogger.debug).toHaveBeenCalledWith(
+				expect.stringContaining('Error during corrupt file cleanup')
+			);
+		});
+	});
+});
--- a/packages/tm-core/src/auth/credential-store.ts
+++ b/packages/tm-core/src/auth/credential-store.ts
@@ -3,13 +3,26 @@
 */

 import fs from 'fs';
-import { AuthCredentials, AuthenticationError, AuthConfig } from './types';
-import { getAuthConfig } from './config';
-import { getLogger } from '../logger';
+import path from 'path';
+import { AuthCredentials, AuthenticationError, AuthConfig } from './types.js';
+import { getAuthConfig } from './config.js';
+import { getLogger } from '../logger/index.js';

+/**
+ * CredentialStore manages the persistence and retrieval of authentication credentials.
+ *
+ * Runtime vs Persisted Shape:
+ * - When retrieved (getCredentials): expiresAt is normalized to number (milliseconds since epoch)
+ * - When persisted (saveCredentials): expiresAt is stored as ISO string for readability
+ *
+ * This normalization ensures consistent runtime behavior while maintaining
+ * human-readable persisted format in the auth.json file.
+ */
 export class CredentialStore {
 	private logger = getLogger('CredentialStore');
 	private config: AuthConfig;
+	// Clock skew tolerance for expiry checks (30 seconds)
+	private readonly CLOCK_SKEW_MS = 30_000;

 	constructor(config?: Partial<AuthConfig>) {
 		this.config = getAuthConfig(config);
@@ -17,6 +30,7 @@ export class CredentialStore {

 	/**
 	 * Get stored authentication credentials
+	 * @returns AuthCredentials with expiresAt as number (milliseconds) for runtime use
 	 */
 	getCredentials(options?: { allowExpired?: boolean }): AuthCredentials | null {
 		try {
@@ -28,27 +42,71 @@ export class CredentialStore {
 				fs.readFileSync(this.config.configFile, 'utf-8')
 			) as AuthCredentials;

-			// Check if token is expired
-			if (
-				authData.expiresAt &&
-				new Date(authData.expiresAt) < new Date() &&
-				!options?.allowExpired
-			) {
-				this.logger.warn('Authentication token has expired');
+			// Normalize/migrate timestamps to numeric (handles both number and ISO string)
+			let expiresAtMs: number | undefined;
+			if (typeof authData.expiresAt === 'number') {
+				expiresAtMs = Number.isFinite(authData.expiresAt)
+					? authData.expiresAt
+					: undefined;
+			} else if (typeof authData.expiresAt === 'string') {
+				const parsed = Date.parse(authData.expiresAt);
+				expiresAtMs = Number.isNaN(parsed) ? undefined : parsed;
+			} else {
+				expiresAtMs = undefined;
+			}
+
+			// Validate expiration time for tokens
+			if (expiresAtMs === undefined) {
+				this.logger.warn('No valid expiration time provided for token');
 				return null;
 			}

+			// Update the authData with normalized timestamp
+			authData.expiresAt = expiresAtMs;
+
+			// Check if the token has expired (with clock skew tolerance)
+			const now = Date.now();
+			const allowExpired = options?.allowExpired ?? false;
+			if (now >= expiresAtMs - this.CLOCK_SKEW_MS && !allowExpired) {
+				this.logger.warn(
+					'Authentication token has expired or is about to expire',
+					{
+						expiresAt: authData.expiresAt,
+						currentTime: new Date(now).toISOString(),
+						skewWindow: `${this.CLOCK_SKEW_MS / 1000}s`
+					}
+				);
+				return null;
+			}
+
+			// Return valid token
 			return authData;
 		} catch (error) {
 			this.logger.error(
 				`Failed to read auth credentials: ${(error as Error).message}`
 			);
+
+			// Quarantine corrupt file to prevent repeated errors
+			try {
+				if (fs.existsSync(this.config.configFile)) {
+					const corruptFile = `${this.config.configFile}.corrupt-${Date.now()}-${process.pid}-${Math.random().toString(36).slice(2, 8)}`;
+					fs.renameSync(this.config.configFile, corruptFile);
+					this.logger.warn(`Quarantined corrupt auth file to: ${corruptFile}`);
+				}
+			} catch (quarantineError) {
+				// If we can't quarantine, log but don't throw
+				this.logger.debug(
+					`Could not quarantine corrupt file: ${(quarantineError as Error).message}`
+				);
+			}
+
 			return null;
 		}
 	}

 	/**
 	 * Save authentication credentials
+	 * @param authData - Credentials with expiresAt as number or string (will be persisted as ISO string)
 	 */
 	saveCredentials(authData: AuthCredentials): void {
 		try {
@@ -57,8 +115,32 @@ export class CredentialStore {
 				fs.mkdirSync(this.config.configDir, { recursive: true, mode: 0o700 });
 			}

-			// Add timestamp
-			authData.savedAt = new Date().toISOString();
+			// Add timestamp without mutating caller's object
+			authData = { ...authData, savedAt: new Date().toISOString() };
+
+			// Validate and normalize expiresAt timestamp
+			if (authData.expiresAt !== undefined) {
+				let validTimestamp: number | undefined;
+
+				if (typeof authData.expiresAt === 'number') {
+					validTimestamp = Number.isFinite(authData.expiresAt)
+						? authData.expiresAt
+						: undefined;
+				} else if (typeof authData.expiresAt === 'string') {
+					const parsed = Date.parse(authData.expiresAt);
+					validTimestamp = Number.isNaN(parsed) ? undefined : parsed;
+				}
+
+				if (validTimestamp === undefined) {
+					throw new AuthenticationError(
+						`Invalid expiresAt format: ${authData.expiresAt}`,
+						'SAVE_FAILED'
+					);
+				}
+
+				// Store as ISO string for consistency
+				authData.expiresAt = new Date(validTimestamp).toISOString();
+			}

 			// Save credentials atomically with secure permissions
 			const tempFile = `${this.config.configFile}.tmp`;
@@ -106,4 +188,54 @@ export class CredentialStore {
 	getConfig(): AuthConfig {
 		return { ...this.config };
 	}
+
+	/**
+	 * Clean up old corrupt auth files
+	 * Removes corrupt files older than the specified age
+	 */
+	cleanupCorruptFiles(maxAgeMs: number = 7 * 24 * 60 * 60 * 1000): void {
+		try {
+			const dir = path.dirname(this.config.configFile);
+			const baseName = path.basename(this.config.configFile);
+			const prefix = `${baseName}.corrupt-`;
+
+			if (!fs.existsSync(dir)) {
+				return;
+			}
+
+			const entries = fs.readdirSync(dir, { withFileTypes: true });
+			const now = Date.now();
+
+			for (const entry of entries) {
+				if (!entry.isFile()) continue;
+				const file = entry.name;
+
+				// Check if file matches pattern: baseName.corrupt-{timestamp}
+				if (!file.startsWith(prefix)) continue;
+				const suffix = file.slice(prefix.length);
+				if (!/^\d+$/.test(suffix)) continue; // Fixed regex, not from variable input
+
+				const filePath = path.join(dir, file);
+				try {
+					const stats = fs.statSync(filePath);
+					const age = now - stats.mtimeMs;
+
+					if (age > maxAgeMs) {
+						fs.unlinkSync(filePath);
+						this.logger.debug(`Cleaned up old corrupt file: ${file}`);
+					}
+				} catch (error) {
+					// Ignore errors for individual file cleanup
+					this.logger.debug(
+						`Could not clean up corrupt file ${file}: ${(error as Error).message}`
+					);
+				}
+			}
+		} catch (error) {
+			// Log but don't throw - this is a cleanup operation
+			this.logger.debug(
+				`Error during corrupt file cleanup: ${(error as Error).message}`
+			);
+		}
+	}
 }
--- a/packages/tm-core/src/auth/index.ts
+++ b/packages/tm-core/src/auth/index.ts
@@ -2,20 +2,20 @@
 * Authentication module exports
 */

-export { AuthManager } from './auth-manager';
-export { CredentialStore } from './credential-store';
-export { OAuthService } from './oauth-service';
+export { AuthManager } from './auth-manager.js';
+export { CredentialStore } from './credential-store.js';
+export { OAuthService } from './oauth-service.js';

 export type {
 	AuthCredentials,
 	OAuthFlowOptions,
 	AuthConfig,
 	CliData
-} from './types';
+} from './types.js';

-export { AuthenticationError } from './types';
+export { AuthenticationError } from './types.js';

 export {
 	DEFAULT_AUTH_CONFIG,
 	getAuthConfig
-} from './config';
+} from './config.js';
--- a/packages/tm-core/src/auth/oauth-service.ts
+++ b/packages/tm-core/src/auth/oauth-service.ts
@@ -12,11 +12,11 @@ import {
 	OAuthFlowOptions,
 	AuthConfig,
 	CliData
-} from './types';
-import { CredentialStore } from './credential-store';
-import { SupabaseAuthClient } from '../clients/supabase-client';
-import { getAuthConfig } from './config';
-import { getLogger } from '../logger';
+} from './types.js';
+import { CredentialStore } from './credential-store.js';
+import { SupabaseAuthClient } from '../clients/supabase-client.js';
+import { getAuthConfig } from './config.js';
+import { getLogger } from '../logger/index.js';
 import packageJson from '../../../../package.json' with { type: 'json' };

 export class OAuthService {
--- a/packages/tm-core/src/auth/types.ts
+++ b/packages/tm-core/src/auth/types.ts
@@ -7,8 +7,8 @@ export interface AuthCredentials {
 	refreshToken?: string;
 	userId: string;
 	email?: string;
-	expiresAt?: string;
-	tokenType?: 'standard' | 'api_key';
+	expiresAt?: string | number;
+	tokenType?: 'standard';
 	savedAt: string;
 }

@@ -57,7 +57,6 @@ export type AuthErrorCode =
 	| 'INVALID_STATE'
 	| 'NO_TOKEN'
 	| 'TOKEN_EXCHANGE_FAILED'
-	| 'INVALID_API_KEY'
 	| 'INVALID_CREDENTIALS'
 	| 'NO_REFRESH_TOKEN'
 	| 'NOT_AUTHENTICATED'
@@ -65,7 +64,10 @@ export type AuthErrorCode =
 	| 'CONFIG_MISSING'
 	| 'SAVE_FAILED'
 	| 'CLEAR_FAILED'
-	| 'STORAGE_ERROR';
+	| 'STORAGE_ERROR'
+	| 'NOT_SUPPORTED'
+	| 'REFRESH_FAILED'
+	| 'INVALID_RESPONSE';

 /**
 * Authentication error class