chore: resolve first batch of requested changes

This commit is contained in:
Ralph Khreish
2025-09-04 21:52:34 +02:00
parent 70ef1298db
commit 900ccbe960
8 changed files with 497 additions and 68 deletions

View File

@@ -21,6 +21,7 @@ describe('AuthManager Singleton', () => {
beforeEach(() => {
// Reset singleton before each test
AuthManager.resetInstance();
vi.clearAllMocks();
});
it('should return the same instance on multiple calls', () => {
@@ -39,6 +40,12 @@ describe('AuthManager Singleton', () => {
const instance = AuthManager.getInstance(config);
expect(instance).toBeDefined();
// Verify the config is passed to internal components
// This would be observable when attempting operations that use the config
// For example, getCredentials would look in the configured file path
const credentials = instance.getCredentials();
expect(credentials).toBeNull(); // File doesn't exist, but it should check the right path
});
it('should warn when config is provided after initialization', () => {

View File

@@ -41,7 +41,7 @@ export class AuthManager {
'getInstance called with config after initialization; config is ignored.'
);
}
return AuthManager.instance;
return AuthManager.instance!;
}
/**
@@ -74,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
*/

View File

@@ -0,0 +1,414 @@
/**
* 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()
};
expect(() => store.saveCredentials(credentials)).toThrow(AuthenticationError);
expect(() => store.saveCredentials(credentials)).toThrow('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()
};
expect(() => store.saveCredentials(credentials)).toThrow(AuthenticationError);
expect(() => store.saveCredentials(credentials)).toThrow('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()
};
expect(() => store.saveCredentials(credentials)).toThrow(AuthenticationError);
expect(() => store.saveCredentials(credentials)).toThrow('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('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')
);
});
});
});

View File

@@ -29,30 +29,38 @@ export class CredentialStore {
fs.readFileSync(this.config.configFile, 'utf-8')
) as AuthCredentials;
// Parse expiration time for validation (expects ISO string format)
// Normalize/migrate timestamps to numeric (handles both number and ISO string)
let expiresAtMs: number | undefined;
if (authData.expiresAt) {
expiresAtMs = Date.parse(authData.expiresAt);
if (isNaN(expiresAtMs)) {
// Invalid date string - treat as expired
this.logger.error(`Invalid expiresAt format: ${authData.expiresAt}`);
return null;
}
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;
}
// Check if token is expired (API keys never expire)
const isApiKey = authData.tokenType === 'api_key';
if (
!isApiKey &&
expiresAtMs &&
expiresAtMs < Date.now() &&
!options?.allowExpired
) {
this.logger.warn('Authentication token has expired');
// 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
const now = Date.now();
const allowExpired = options?.allowExpired ?? false;
if (now >= expiresAtMs && !allowExpired) {
this.logger.warn('Authentication token has expired', {
expiresAt: authData.expiresAt,
currentTime: new Date(now).toISOString()
});
return null;
}
// Return valid token
return authData;
} catch (error) {
this.logger.error(
@@ -87,18 +95,29 @@ 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 expiresAt is a valid ISO string if present
if (authData.expiresAt) {
const ms = Date.parse(authData.expiresAt);
if (isNaN(ms)) {
// 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
@@ -156,16 +175,19 @@ export class CredentialStore {
try {
const dir = path.dirname(this.config.configFile);
const baseName = path.basename(this.config.configFile);
const corruptPattern = new RegExp(`^${baseName}\\.corrupt-\\d+$`);
const escapedBaseName = baseName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const corruptPattern = new RegExp(`^${escapedBaseName}\\.corrupt-\\d+$`);
if (!fs.existsSync(dir)) {
return;
}
const files = fs.readdirSync(dir);
const entries = fs.readdirSync(dir, { withFileTypes: true });
const now = Date.now();
for (const file of files) {
for (const entry of entries) {
if (!entry.isFile()) continue;
const file = entry.name;
if (corruptPattern.test(file)) {
const filePath = path.join(dir, file);
try {

View File

@@ -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

View File

@@ -142,17 +142,23 @@ export class ConfigManager {
// Return the configured type (including 'auto')
const storageType = storage?.type || 'auto';
const basePath = storage?.basePath;
if (storageType === 'api' || storageType === 'auto') {
return {
type: storageType,
basePath,
apiEndpoint: storage?.apiEndpoint,
apiAccessToken: storage?.apiAccessToken,
apiConfigured: Boolean(storage?.apiEndpoint || storage?.apiAccessToken)
};
}
return { type: storageType, apiConfigured: false };
return {
type: storageType,
basePath,
apiConfigured: false
};
}
/**
@@ -183,9 +189,10 @@ export class ConfigManager {
}
/**
* Check if using API storage
* Check if explicitly configured to use API storage
* Excludes 'auto' type
*/
isUsingApiStorage(): boolean {
isApiExplicitlyConfigured(): boolean {
return this.getStorageConfig().type === 'api';
}
@@ -218,6 +225,7 @@ export class ConfigManager {
await this.persistence.saveConfig(this.config);
// Re-initialize to respect precedence
this.initialized = false;
await this.initialize();
}

View File

@@ -88,6 +88,8 @@ export type StorageType = 'file' | 'api' | 'auto';
export interface RuntimeStorageConfig {
/** Storage backend type */
type: StorageType;
/** Base path for file storage (if configured) */
basePath?: string;
/** API endpoint for API storage (Hamster integration) */
apiEndpoint?: string;
/** Access token for API authentication */
@@ -107,19 +109,15 @@ export interface RuntimeStorageConfig {
export interface StorageSettings
extends Omit<RuntimeStorageConfig, 'apiConfigured'> {
/** Storage backend type - 'auto' detects based on auth status */
type: 'file' | 'api' | 'auto';
type: StorageType;
/** Base path for file storage */
basePath?: string;
/** API endpoint for API storage (Hamster integration) */
apiEndpoint?: string;
/** Access token for API authentication */
apiAccessToken?: string;
/**
* Indicates whether API is configured (has endpoint or token)
/**
* Indicates whether API is configured
* @computed Derived automatically from presence of apiEndpoint or apiAccessToken
* @internal Should not be set manually in user config - computed by ConfigManager
*/
apiConfigured?: boolean;
readonly apiConfigured?: boolean;
/** Enable automatic backups */
enableBackup: boolean;
/** Maximum number of backups to retain */

View File

@@ -28,9 +28,10 @@ export class StorageFactory {
storageConfig: RuntimeStorageConfig,
projectPath: string
): IStorage {
// Wrap the storage config in the expected format
// Wrap the storage config in the expected format, including projectPath
// This ensures ApiStorage receives the projectPath for projectId
return StorageFactory.create(
{ storage: storageConfig } as Partial<IConfiguration>,
{ storage: storageConfig, projectPath } as Partial<IConfiguration>,
projectPath
);
}