From 84056d63cd95f8d8128eb1bbb133d84aaf3ee0fb Mon Sep 17 00:00:00 2001 From: Ralph Khreish <35776126+Crunchyman-ralph@users.noreply.github.com> Date: Fri, 5 Sep 2025 00:01:53 +0200 Subject: [PATCH] chore: apply requested changes --- .../tm-core/src/auth/credential-store.test.ts | 160 ++++++++++++++++-- packages/tm-core/src/auth/credential-store.ts | 60 ++++--- .../tm-core/src/config/config-manager.spec.ts | 8 +- packages/tm-core/src/config/config-manager.ts | 2 +- .../src/interfaces/configuration.interface.ts | 4 +- .../tm-core/src/storage/storage-factory.ts | 21 ++- packages/tm-core/tsconfig.json | 2 +- 7 files changed, 200 insertions(+), 57 deletions(-) diff --git a/packages/tm-core/src/auth/credential-store.test.ts b/packages/tm-core/src/auth/credential-store.test.ts index db044287..9f49457f 100644 --- a/packages/tm-core/src/auth/credential-store.test.ts +++ b/packages/tm-core/src/auth/credential-store.test.ts @@ -290,12 +290,14 @@ describe('CredentialStore', () => { savedAt: new Date().toISOString() }; - expect(() => store.saveCredentials(credentials)).toThrow( - AuthenticationError - ); - expect(() => store.saveCredentials(credentials)).toThrow( - 'Invalid expiresAt format' - ); + 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', () => { @@ -307,12 +309,14 @@ describe('CredentialStore', () => { savedAt: new Date().toISOString() }; - expect(() => store.saveCredentials(credentials)).toThrow( - AuthenticationError - ); - expect(() => store.saveCredentials(credentials)).toThrow( - 'Invalid expiresAt format' - ); + 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', () => { @@ -324,12 +328,14 @@ describe('CredentialStore', () => { savedAt: new Date().toISOString() }; - expect(() => store.saveCredentials(credentials)).toThrow( - AuthenticationError - ); - expect(() => store.saveCredentials(credentials)).toThrow( - 'Invalid expiresAt format' - ); + 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', () => { @@ -403,6 +409,124 @@ describe('CredentialStore', () => { }); }); + 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(); diff --git a/packages/tm-core/src/auth/credential-store.ts b/packages/tm-core/src/auth/credential-store.ts index 024f0f37..980e80b3 100644 --- a/packages/tm-core/src/auth/credential-store.ts +++ b/packages/tm-core/src/auth/credential-store.ts @@ -8,9 +8,21 @@ 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) { this.config = getAuthConfig(config); @@ -18,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 { @@ -51,13 +64,14 @@ export class CredentialStore { // Update the authData with normalized timestamp authData.expiresAt = expiresAtMs; - // Check if the token has expired + // Check if the token has expired (with clock skew tolerance) const now = Date.now(); const allowExpired = options?.allowExpired ?? false; - if (now >= expiresAtMs && !allowExpired) { - this.logger.warn('Authentication token has expired', { + 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() + currentTime: new Date(now).toISOString(), + skewWindow: `${this.CLOCK_SKEW_MS / 1000}s` }); return null; } @@ -72,7 +86,7 @@ export class CredentialStore { // Quarantine corrupt file to prevent repeated errors try { if (fs.existsSync(this.config.configFile)) { - const corruptFile = `${this.config.configFile}.corrupt-${Date.now()}`; + 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}`); } @@ -89,6 +103,7 @@ export class CredentialStore { /** * Save authentication credentials + * @param authData - Credentials with expiresAt as number or string (will be persisted as ISO string) */ saveCredentials(authData: AuthCredentials): void { try { @@ -179,8 +194,7 @@ export class CredentialStore { try { const dir = path.dirname(this.config.configFile); const baseName = path.basename(this.config.configFile); - const escapedBaseName = baseName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const corruptPattern = new RegExp(`^${escapedBaseName}\\.corrupt-\\d+$`); + const prefix = `${baseName}.corrupt-`; if (!fs.existsSync(dir)) { return; @@ -192,22 +206,26 @@ export class CredentialStore { for (const entry of entries) { if (!entry.isFile()) continue; const file = entry.name; - if (corruptPattern.test(file)) { - const filePath = path.join(dir, file); - try { - const stats = fs.statSync(filePath); - const age = now - stats.mtimeMs; + + // 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}` - ); + 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) { diff --git a/packages/tm-core/src/config/config-manager.spec.ts b/packages/tm-core/src/config/config-manager.spec.ts index ac068d31..a85744d5 100644 --- a/packages/tm-core/src/config/config-manager.spec.ts +++ b/packages/tm-core/src/config/config-manager.spec.ts @@ -309,11 +309,11 @@ describe('ConfigManager', () => { expect(manager.getProjectRoot()).toBe(testProjectRoot); }); - it('should check if using API storage', () => { - expect(manager.isUsingApiStorage()).toBe(false); + it('should check if API is explicitly configured', () => { + expect(manager.isApiExplicitlyConfigured()).toBe(false); }); - it('should detect API storage', () => { + it('should detect when API is explicitly configured', () => { // Update config for current instance (manager as any).config = { storage: { @@ -323,7 +323,7 @@ describe('ConfigManager', () => { } }; - expect(manager.isUsingApiStorage()).toBe(true); + expect(manager.isApiExplicitlyConfigured()).toBe(true); }); }); diff --git a/packages/tm-core/src/config/config-manager.ts b/packages/tm-core/src/config/config-manager.ts index 03bf41a1..64a47d21 100644 --- a/packages/tm-core/src/config/config-manager.ts +++ b/packages/tm-core/src/config/config-manager.ts @@ -142,7 +142,7 @@ export class ConfigManager { // Return the configured type (including 'auto') const storageType = storage?.type || 'auto'; - const basePath = storage?.basePath; + const basePath = storage?.basePath ?? this.projectRoot; if (storageType === 'api' || storageType === 'auto') { return { diff --git a/packages/tm-core/src/interfaces/configuration.interface.ts b/packages/tm-core/src/interfaces/configuration.interface.ts index f03c55db..ab84a45f 100644 --- a/packages/tm-core/src/interfaces/configuration.interface.ts +++ b/packages/tm-core/src/interfaces/configuration.interface.ts @@ -99,7 +99,7 @@ export interface RuntimeStorageConfig { * @computed Derived automatically from presence of apiEndpoint or apiAccessToken * @internal Should not be set manually - computed by ConfigManager */ - apiConfigured: boolean; + readonly apiConfigured: boolean; } /** @@ -108,8 +108,6 @@ export interface RuntimeStorageConfig { */ export interface StorageSettings extends Omit { - /** Storage backend type - 'auto' detects based on auth status */ - type: StorageType; /** Base path for file storage */ basePath?: string; /** diff --git a/packages/tm-core/src/storage/storage-factory.ts b/packages/tm-core/src/storage/storage-factory.ts index 9e445316..e1527a43 100644 --- a/packages/tm-core/src/storage/storage-factory.ts +++ b/packages/tm-core/src/storage/storage-factory.ts @@ -5,7 +5,8 @@ import type { IStorage } from '../interfaces/storage.interface.js'; import type { IConfiguration, - RuntimeStorageConfig + RuntimeStorageConfig, + StorageSettings } from '../interfaces/configuration.interface.js'; import { FileStorage } from './file-storage/index.js'; import { ApiStorage } from './api-storage.js'; @@ -74,15 +75,16 @@ export class StorageFactory { const credentials = authManager.getCredentials(); if (credentials) { // Merge with existing storage config, ensuring required fields - config.storage = { - ...config.storage, - type: 'api' as const, + const nextStorage: StorageSettings = { + ...(config.storage as StorageSettings), + type: 'api', apiAccessToken: credentials.token, apiEndpoint: config.storage?.apiEndpoint || process.env.HAMSTER_API_URL || 'https://tryhamster.com/api' - } as any; // Cast to any to bypass strict type checking for partial config + }; + config.storage = nextStorage; } } logger.info('☁️ Using API storage'); @@ -103,15 +105,16 @@ export class StorageFactory { const credentials = authManager.getCredentials(); if (credentials) { // Configure API storage with auth credentials - config.storage = { - ...config.storage, - type: 'api' as const, + const nextStorage: StorageSettings = { + ...(config.storage as StorageSettings), + type: 'api', apiAccessToken: credentials.token, apiEndpoint: config.storage?.apiEndpoint || process.env.HAMSTER_API_URL || 'https://tryhamster.com/api' - } as any; // Cast to any to bypass strict type checking for partial config + }; + config.storage = nextStorage; logger.info('☁️ Using API storage (authenticated)'); return StorageFactory.createApiStorage(config); } diff --git a/packages/tm-core/tsconfig.json b/packages/tm-core/tsconfig.json index 47e52c40..a34f2f4b 100644 --- a/packages/tm-core/tsconfig.json +++ b/packages/tm-core/tsconfig.json @@ -23,7 +23,7 @@ "esModuleInterop": true, "skipLibCheck": true, "forceConsistentCasingInFileNames": true, - "moduleResolution": "node", + "moduleResolution": "bundler", "resolveJsonModule": true, "isolatedModules": true, "paths": {