chore: apply requested changes

This commit is contained in:
Ralph Khreish
2025-09-05 00:01:53 +02:00
parent 37a8955494
commit 84056d63cd
7 changed files with 200 additions and 57 deletions

View File

@@ -290,12 +290,14 @@ describe('CredentialStore', () => {
savedAt: new Date().toISOString() savedAt: new Date().toISOString()
}; };
expect(() => store.saveCredentials(credentials)).toThrow( let err: unknown;
AuthenticationError try {
); store.saveCredentials(credentials);
expect(() => store.saveCredentials(credentials)).toThrow( } catch (e) {
'Invalid expiresAt format' err = e;
); }
expect(err).toBeInstanceOf(AuthenticationError);
expect((err as Error).message).toContain('Invalid expiresAt format');
}); });
it('should reject NaN timestamp when saving', () => { it('should reject NaN timestamp when saving', () => {
@@ -307,12 +309,14 @@ describe('CredentialStore', () => {
savedAt: new Date().toISOString() savedAt: new Date().toISOString()
}; };
expect(() => store.saveCredentials(credentials)).toThrow( let err: unknown;
AuthenticationError try {
); store.saveCredentials(credentials);
expect(() => store.saveCredentials(credentials)).toThrow( } catch (e) {
'Invalid expiresAt format' err = e;
); }
expect(err).toBeInstanceOf(AuthenticationError);
expect((err as Error).message).toContain('Invalid expiresAt format');
}); });
it('should reject Infinity timestamp when saving', () => { it('should reject Infinity timestamp when saving', () => {
@@ -324,12 +328,14 @@ describe('CredentialStore', () => {
savedAt: new Date().toISOString() savedAt: new Date().toISOString()
}; };
expect(() => store.saveCredentials(credentials)).toThrow( let err: unknown;
AuthenticationError try {
); store.saveCredentials(credentials);
expect(() => store.saveCredentials(credentials)).toThrow( } catch (e) {
'Invalid expiresAt format' err = e;
); }
expect(err).toBeInstanceOf(AuthenticationError);
expect((err as Error).message).toContain('Invalid expiresAt format');
}); });
it('should handle missing expiresAt when saving', () => { 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', () => { describe('cleanupCorruptFiles', () => {
it('should remove old corrupt files', () => { it('should remove old corrupt files', () => {
const now = Date.now(); const now = Date.now();

View File

@@ -8,9 +8,21 @@ import { AuthCredentials, AuthenticationError, AuthConfig } from './types.js';
import { getAuthConfig } from './config.js'; import { getAuthConfig } from './config.js';
import { getLogger } from '../logger/index.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 { export class CredentialStore {
private logger = getLogger('CredentialStore'); private logger = getLogger('CredentialStore');
private config: AuthConfig; private config: AuthConfig;
// Clock skew tolerance for expiry checks (30 seconds)
private readonly CLOCK_SKEW_MS = 30_000;
constructor(config?: Partial<AuthConfig>) { constructor(config?: Partial<AuthConfig>) {
this.config = getAuthConfig(config); this.config = getAuthConfig(config);
@@ -18,6 +30,7 @@ export class CredentialStore {
/** /**
* Get stored authentication credentials * Get stored authentication credentials
* @returns AuthCredentials with expiresAt as number (milliseconds) for runtime use
*/ */
getCredentials(options?: { allowExpired?: boolean }): AuthCredentials | null { getCredentials(options?: { allowExpired?: boolean }): AuthCredentials | null {
try { try {
@@ -51,13 +64,14 @@ export class CredentialStore {
// Update the authData with normalized timestamp // Update the authData with normalized timestamp
authData.expiresAt = expiresAtMs; authData.expiresAt = expiresAtMs;
// Check if the token has expired // Check if the token has expired (with clock skew tolerance)
const now = Date.now(); const now = Date.now();
const allowExpired = options?.allowExpired ?? false; const allowExpired = options?.allowExpired ?? false;
if (now >= expiresAtMs && !allowExpired) { if (now >= (expiresAtMs - this.CLOCK_SKEW_MS) && !allowExpired) {
this.logger.warn('Authentication token has expired', { this.logger.warn('Authentication token has expired or is about to expire', {
expiresAt: authData.expiresAt, expiresAt: authData.expiresAt,
currentTime: new Date(now).toISOString() currentTime: new Date(now).toISOString(),
skewWindow: `${this.CLOCK_SKEW_MS / 1000}s`
}); });
return null; return null;
} }
@@ -72,7 +86,7 @@ export class CredentialStore {
// Quarantine corrupt file to prevent repeated errors // Quarantine corrupt file to prevent repeated errors
try { try {
if (fs.existsSync(this.config.configFile)) { 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); fs.renameSync(this.config.configFile, corruptFile);
this.logger.warn(`Quarantined corrupt auth file to: ${corruptFile}`); this.logger.warn(`Quarantined corrupt auth file to: ${corruptFile}`);
} }
@@ -89,6 +103,7 @@ export class CredentialStore {
/** /**
* Save authentication credentials * Save authentication credentials
* @param authData - Credentials with expiresAt as number or string (will be persisted as ISO string)
*/ */
saveCredentials(authData: AuthCredentials): void { saveCredentials(authData: AuthCredentials): void {
try { try {
@@ -179,8 +194,7 @@ export class CredentialStore {
try { try {
const dir = path.dirname(this.config.configFile); const dir = path.dirname(this.config.configFile);
const baseName = path.basename(this.config.configFile); const baseName = path.basename(this.config.configFile);
const escapedBaseName = baseName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const prefix = `${baseName}.corrupt-`;
const corruptPattern = new RegExp(`^${escapedBaseName}\\.corrupt-\\d+$`);
if (!fs.existsSync(dir)) { if (!fs.existsSync(dir)) {
return; return;
@@ -192,7 +206,12 @@ export class CredentialStore {
for (const entry of entries) { for (const entry of entries) {
if (!entry.isFile()) continue; if (!entry.isFile()) continue;
const file = entry.name; const file = entry.name;
if (corruptPattern.test(file)) {
// 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); const filePath = path.join(dir, file);
try { try {
const stats = fs.statSync(filePath); const stats = fs.statSync(filePath);
@@ -209,7 +228,6 @@ export class CredentialStore {
); );
} }
} }
}
} catch (error) { } catch (error) {
// Log but don't throw - this is a cleanup operation // Log but don't throw - this is a cleanup operation
this.logger.debug( this.logger.debug(

View File

@@ -309,11 +309,11 @@ describe('ConfigManager', () => {
expect(manager.getProjectRoot()).toBe(testProjectRoot); expect(manager.getProjectRoot()).toBe(testProjectRoot);
}); });
it('should check if using API storage', () => { it('should check if API is explicitly configured', () => {
expect(manager.isUsingApiStorage()).toBe(false); expect(manager.isApiExplicitlyConfigured()).toBe(false);
}); });
it('should detect API storage', () => { it('should detect when API is explicitly configured', () => {
// Update config for current instance // Update config for current instance
(manager as any).config = { (manager as any).config = {
storage: { storage: {
@@ -323,7 +323,7 @@ describe('ConfigManager', () => {
} }
}; };
expect(manager.isUsingApiStorage()).toBe(true); expect(manager.isApiExplicitlyConfigured()).toBe(true);
}); });
}); });

View File

@@ -142,7 +142,7 @@ export class ConfigManager {
// Return the configured type (including 'auto') // Return the configured type (including 'auto')
const storageType = storage?.type || 'auto'; const storageType = storage?.type || 'auto';
const basePath = storage?.basePath; const basePath = storage?.basePath ?? this.projectRoot;
if (storageType === 'api' || storageType === 'auto') { if (storageType === 'api' || storageType === 'auto') {
return { return {

View File

@@ -99,7 +99,7 @@ export interface RuntimeStorageConfig {
* @computed Derived automatically from presence of apiEndpoint or apiAccessToken * @computed Derived automatically from presence of apiEndpoint or apiAccessToken
* @internal Should not be set manually - computed by ConfigManager * @internal Should not be set manually - computed by ConfigManager
*/ */
apiConfigured: boolean; readonly apiConfigured: boolean;
} }
/** /**
@@ -108,8 +108,6 @@ export interface RuntimeStorageConfig {
*/ */
export interface StorageSettings export interface StorageSettings
extends Omit<RuntimeStorageConfig, 'apiConfigured'> { extends Omit<RuntimeStorageConfig, 'apiConfigured'> {
/** Storage backend type - 'auto' detects based on auth status */
type: StorageType;
/** Base path for file storage */ /** Base path for file storage */
basePath?: string; basePath?: string;
/** /**

View File

@@ -5,7 +5,8 @@
import type { IStorage } from '../interfaces/storage.interface.js'; import type { IStorage } from '../interfaces/storage.interface.js';
import type { import type {
IConfiguration, IConfiguration,
RuntimeStorageConfig RuntimeStorageConfig,
StorageSettings
} from '../interfaces/configuration.interface.js'; } from '../interfaces/configuration.interface.js';
import { FileStorage } from './file-storage/index.js'; import { FileStorage } from './file-storage/index.js';
import { ApiStorage } from './api-storage.js'; import { ApiStorage } from './api-storage.js';
@@ -74,15 +75,16 @@ export class StorageFactory {
const credentials = authManager.getCredentials(); const credentials = authManager.getCredentials();
if (credentials) { if (credentials) {
// Merge with existing storage config, ensuring required fields // Merge with existing storage config, ensuring required fields
config.storage = { const nextStorage: StorageSettings = {
...config.storage, ...(config.storage as StorageSettings),
type: 'api' as const, type: 'api',
apiAccessToken: credentials.token, apiAccessToken: credentials.token,
apiEndpoint: apiEndpoint:
config.storage?.apiEndpoint || config.storage?.apiEndpoint ||
process.env.HAMSTER_API_URL || process.env.HAMSTER_API_URL ||
'https://tryhamster.com/api' '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'); logger.info('☁️ Using API storage');
@@ -103,15 +105,16 @@ export class StorageFactory {
const credentials = authManager.getCredentials(); const credentials = authManager.getCredentials();
if (credentials) { if (credentials) {
// Configure API storage with auth credentials // Configure API storage with auth credentials
config.storage = { const nextStorage: StorageSettings = {
...config.storage, ...(config.storage as StorageSettings),
type: 'api' as const, type: 'api',
apiAccessToken: credentials.token, apiAccessToken: credentials.token,
apiEndpoint: apiEndpoint:
config.storage?.apiEndpoint || config.storage?.apiEndpoint ||
process.env.HAMSTER_API_URL || process.env.HAMSTER_API_URL ||
'https://tryhamster.com/api' '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)'); logger.info('☁️ Using API storage (authenticated)');
return StorageFactory.createApiStorage(config); return StorageFactory.createApiStorage(config);
} }

View File

@@ -23,7 +23,7 @@
"esModuleInterop": true, "esModuleInterop": true,
"skipLibCheck": true, "skipLibCheck": true,
"forceConsistentCasingInFileNames": true, "forceConsistentCasingInFileNames": true,
"moduleResolution": "node", "moduleResolution": "bundler",
"resolveJsonModule": true, "resolveJsonModule": true,
"isolatedModules": true, "isolatedModules": true,
"paths": { "paths": {