From 29cceb3a881600d70511bcc6d11b63f5b809f04b Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Mon, 23 Feb 2026 00:05:47 +0700 Subject: [PATCH] fix(persist): harden persist restore safety and edge cases --- src/commands/persist-command.ts | 513 +++++++++++++----- .../commands/persist-command-handler.test.ts | 273 ++++++++++ tests/unit/commands/persist-command.test.js | 171 +++++- 3 files changed, 807 insertions(+), 150 deletions(-) create mode 100644 tests/unit/commands/persist-command-handler.test.ts diff --git a/src/commands/persist-command.ts b/src/commands/persist-command.ts index 4e461e73..1f157a80 100644 --- a/src/commands/persist-command.ts +++ b/src/commands/persist-command.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; +import * as lockfile from 'proper-lockfile'; import { initUI, header, subheader, color, dim, ok, fail, warn, info } from '../utils/ui'; import { InteractivePrompt } from '../utils/prompt'; import ProfileDetector, { @@ -53,6 +54,10 @@ const PERSIST_KNOWN_FLAGS = [ ] as const; const VALID_PERMISSION_MODES = ['default', 'plan', 'acceptEdits', 'bypassPermissions'] as const; +const PERSIST_LOCK_STALE_MS = 10000; +const PERSIST_LOCK_RETRIES = 5; +const PERSIST_LOCK_RETRY_MIN_MS = 100; +const PERSIST_LOCK_RETRY_MAX_MS = 500; type PermissionMode = (typeof VALID_PERMISSION_MODES)[number]; @@ -60,6 +65,10 @@ function isPermissionMode(value: string): value is PermissionMode { return VALID_PERMISSION_MODES.includes(value as PermissionMode); } +function isKnownPersistFlagToken(token: string): boolean { + return PERSIST_KNOWN_FLAGS.some((flag) => token === flag || token.startsWith(`${flag}=`)); +} + function resolvePermissionMode(parsedArgs: PersistCommandArgs): PermissionMode | undefined { if (!parsedArgs.dangerouslySkipPermissions) { return parsedArgs.permissionMode; @@ -106,6 +115,27 @@ function parseArgs(args: string[]): PersistCommandArgs { '--auto-approve', ]); + const unknownFlags = permissionModeOption.remainingArgs.filter( + (arg) => arg.startsWith('-') && !isKnownPersistFlagToken(arg) + ); + if (!result.parseError && unknownFlags.length > 0) { + const unknownList = unknownFlags.map((flag) => `"${flag}"`).join(', '); + result.parseError = `Unknown option(s): ${unknownList}. Run 'ccs persist --help' for usage.`; + } + + if (!result.parseError && result.listBackups && result.restore) { + result.parseError = '--list-backups cannot be used with --restore'; + } + + if ( + !result.parseError && + (result.listBackups || result.restore) && + (result.permissionMode || result.dangerouslySkipPermissions) + ) { + result.parseError = + 'Permission flags are not valid with backup operations. Use them only with ccs persist .'; + } + for (const arg of permissionModeOption.remainingArgs) { if (!arg.startsWith('-')) { result.profile = arg; @@ -140,71 +170,193 @@ function getClaudeSettingsDisplayPath(): string { return formatDisplayPath(getClaudeSettingsPath()); } -/** Read existing Claude settings.json with validation */ -function readClaudeSettings(): Record { - const settingsPath = getClaudeSettingsPath(); +async function pathExists(filePath: string): Promise { try { - const content = fs.readFileSync(settingsPath, 'utf8'); - // Handle empty file (0 bytes) - if (!content.trim()) { - return {}; - } - const parsed: unknown = JSON.parse(content); - // Validate parsed value is a plain object (not array, null, or primitive) - if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { - throw new Error('settings.json must contain a JSON object, not an array or primitive'); - } - return parsed as Record; - } catch (error) { - const nodeError = error as NodeJS.ErrnoException; - if (nodeError.code === 'ENOENT') { - return {}; - } - throw new Error(`Failed to parse settings.json: ${(error as Error).message}`); + await fs.promises.access(filePath, fs.constants.F_OK); + return true; + } catch { + return false; } } -/** - * Write settings back to settings.json - * Note: mode 0o600 only applies when creating a new file. - * Existing file permissions are preserved (acceptable behavior). - */ -function writeClaudeSettings(settings: Record): void { - const settingsPath = getClaudeSettingsPath(); - // Security: Reject symlinks to prevent writing to unexpected locations - if (isSymlink(settingsPath)) { - throw new Error('settings.json is a symlink - refusing to write for security'); - } - const dir = path.dirname(settingsPath); - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); - } - fs.writeFileSync(settingsPath, JSON.stringify(settings, null, 2) + '\n', { mode: 0o600 }); -} - -/** Maximum number of backups to keep (oldest are deleted) */ -const MAX_BACKUPS = 10; - -/** Check if path is a symlink (security check) */ -function isSymlink(filePath: string): boolean { +async function isSymlinkAsync(filePath: string): Promise { try { - const stats = fs.lstatSync(filePath); + const stats = await fs.promises.lstat(filePath); return stats.isSymbolicLink(); } catch { return false; } } -/** Create backup of settings.json with proper permissions and rotation */ -function createBackup(): string { +function getNoFollowFlag(): number { + const candidate = (fs.constants as Record)['O_NOFOLLOW']; + if (process.platform !== 'win32' && typeof candidate === 'number') { + return candidate; + } + return 0; +} + +function createSymlinkReadError(filePath: string): NodeJS.ErrnoException { + const error = new Error( + `Refusing to read symlinked file for security: ${formatDisplayPath(filePath)}` + ) as NodeJS.ErrnoException; + error.code = 'ELOOP'; + return error; +} + +async function readFileUtf8NoFollow(filePath: string): Promise { + if (await isSymlinkAsync(filePath)) { + throw createSymlinkReadError(filePath); + } + + const noFollowFlag = getNoFollowFlag(); + const flags = fs.constants.O_RDONLY | noFollowFlag; + const handle = await fs.promises.open(filePath, flags); + try { + // Best-effort fallback for platforms without O_NOFOLLOW (notably Windows). + // Re-check symlink status after open to reduce check-then-use windows. + if (noFollowFlag === 0 && (await isSymlinkAsync(filePath))) { + throw createSymlinkReadError(filePath); + } + + const stats = await handle.stat(); + if (!stats.isFile()) { + throw new Error('Path is not a regular file'); + } + + if (noFollowFlag === 0) { + const latestStats = await fs.promises.stat(filePath); + if (latestStats.dev !== stats.dev || latestStats.ino !== stats.ino) { + throw new Error('Path changed during secure read'); + } + } + + return await handle.readFile({ encoding: 'utf8' }); + } finally { + await handle.close(); + } +} + +function parseSettingsObject(content: string, sourceLabel: string): Record { + if (!content.trim()) { + return {}; + } + const parsed: unknown = JSON.parse(content); + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error(`${sourceLabel} must contain a JSON object, not an array or primitive`); + } + return parsed as Record; +} + +async function withPersistSettingsLock(operation: () => Promise): Promise { const settingsPath = getClaudeSettingsPath(); - if (!fs.existsSync(settingsPath)) { + const settingsDir = path.dirname(settingsPath); + await fs.promises.mkdir(settingsDir, { recursive: true }); + + let release: (() => Promise) | undefined; + try { + release = await lockfile.lock(settingsDir, { + stale: PERSIST_LOCK_STALE_MS, + retries: { + retries: PERSIST_LOCK_RETRIES, + minTimeout: PERSIST_LOCK_RETRY_MIN_MS, + maxTimeout: PERSIST_LOCK_RETRY_MAX_MS, + }, + realpath: false, + }); + } catch (error) { + throw new Error( + `Failed to lock Claude settings directory (${formatDisplayPath(settingsDir)}): ${(error as Error).message}` + ); + } + + try { + return await operation(); + } finally { + if (release) { + try { + await release(); + } catch { + // Best-effort release. + } + } + } +} + +/** Read existing Claude settings.json with validation */ +async function readClaudeSettings(): Promise> { + const settingsPath = getClaudeSettingsPath(); + try { + const content = await readFileUtf8NoFollow(settingsPath); + return parseSettingsObject(content, 'settings.json'); + } catch (error) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code === 'ENOENT') { + return {}; + } + if (nodeError.code === 'ELOOP') { + throw new Error('settings.json is a symlink - refusing to read for security'); + } + throw new Error(`Failed to parse settings.json: ${(error as Error).message}`); + } +} + +/** Write settings back to settings.json with atomic replace semantics. */ +async function writeClaudeSettings(settings: Record): Promise { + const settingsPath = getClaudeSettingsPath(); + if (await isSymlinkAsync(settingsPath)) { + throw new Error('settings.json is a symlink - refusing to write for security'); + } + + const settingsDir = path.dirname(settingsPath); + await fs.promises.mkdir(settingsDir, { recursive: true }); + + const nonce = `${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; + const tmpPath = path.join(settingsDir, `settings.json.tmp-${nonce}`); + const flags = + fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_EXCL | getNoFollowFlag(); + + let handle: fs.promises.FileHandle | undefined; + try { + handle = await fs.promises.open(tmpPath, flags, 0o600); + await handle.writeFile(JSON.stringify(settings, null, 2) + '\n', { encoding: 'utf8' }); + await handle.sync(); + } finally { + if (handle) { + await handle.close(); + } + } + + try { + await fs.promises.rename(tmpPath, settingsPath); + } catch (error) { + try { + await fs.promises.unlink(tmpPath); + } catch { + // Best-effort cleanup. + } + throw error; + } + + try { + await fs.promises.chmod(settingsPath, 0o600); + } catch { + // Best-effort permission hardening. + } +} + +/** Maximum number of backups to keep (oldest are deleted) */ +const MAX_BACKUPS = 10; + +/** Create backup of settings.json with proper permissions and rotation */ +async function createBackup(): Promise { + const settingsPath = getClaudeSettingsPath(); + if (!(await pathExists(settingsPath))) { throw new Error('No settings.json to backup'); } - // Security: Reject symlinks to prevent writing to unexpected locations - if (isSymlink(settingsPath)) { - throw new Error('settings.json is a symlink - refusing to backup for security'); - } + + const settingsContent = await readFileUtf8NoFollow(settingsPath); + const now = new Date(); const timestamp = now.getFullYear().toString() + @@ -215,9 +367,27 @@ function createBackup(): string { now.getMinutes().toString().padStart(2, '0') + now.getSeconds().toString().padStart(2, '0'); const backupPath = `${settingsPath}.backup.${timestamp}`; - fs.copyFileSync(settingsPath, backupPath); - // Security: Set restrictive permissions on backup (contains API keys) - fs.chmodSync(backupPath, 0o600); + + const flags = + fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_EXCL | getNoFollowFlag(); + + let handle: fs.promises.FileHandle | undefined; + try { + handle = await fs.promises.open(backupPath, flags, 0o600); + await handle.writeFile(settingsContent, { encoding: 'utf8' }); + await handle.sync(); + } finally { + if (handle) { + await handle.close(); + } + } + + try { + await fs.promises.chmod(backupPath, 0o600); + } catch { + // Best-effort permission hardening. + } + // Cleanup: Rotate old backups (keep only MAX_BACKUPS) cleanupOldBackups(); return backupPath; @@ -231,8 +401,12 @@ function cleanupOldBackups(): void { for (const backup of toDelete) { try { fs.unlinkSync(backup.path); - } catch { - // Ignore deletion errors (file may be locked or already deleted) + } catch (error) { + console.log( + warn( + `Failed to delete old backup ${formatDisplayPath(backup.path)}: ${(error as Error).message}` + ) + ); } } } @@ -299,6 +473,46 @@ function maskApiKey(key: string): string { return `${key.slice(0, 4)}...${key.slice(-4)}`; } +const SENSITIVE_ENV_PARTS = new Set([ + 'TOKEN', + 'KEY', + 'SECRET', + 'PASSWORD', + 'PASS', + 'AUTH', + 'CREDENTIAL', + 'PRIVATE', + 'ACCESS', + 'REFRESH', + 'APIKEY', +]); + +function splitSensitiveKeyParts(key: string): string[] { + const withCamelCaseBoundaries = key.replace(/([a-z0-9])([A-Z])/g, '$1_$2'); + return withCamelCaseBoundaries + .toUpperCase() + .split(/[^A-Z0-9]+/) + .filter(Boolean); +} + +function isSensitiveEnvKey(key: string): boolean { + const parts = splitSensitiveKeyParts(key); + if (parts.some((part) => SENSITIVE_ENV_PARTS.has(part))) { + return true; + } + + const compact = parts.join(''); + return ( + compact.includes('TOKEN') || + compact.includes('APIKEY') || + compact.includes('ACCESSKEY') || + compact.includes('AUTHKEY') || + compact.includes('SECRET') || + compact.includes('PASSWORD') || + compact.includes('CREDENTIAL') + ); +} + /** Resolve env vars for a profile */ async function resolveProfileEnvVars( profileName: string, @@ -420,36 +634,60 @@ async function handleRestore(timestamp: string | boolean, yes: boolean): Promise process.exit(0); } } - // Validate backup JSON integrity before restore + + let parsedBackupSettings: Record; try { - const backupContent = fs.readFileSync(backup.path, 'utf8'); - const parsed: unknown = JSON.parse(backupContent); - if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { - console.log(fail('Backup file is corrupted: not a valid JSON object')); - process.exit(1); - } + const backupContent = await readFileUtf8NoFollow(backup.path); + parsedBackupSettings = parseSettingsObject(backupContent, 'Backup file'); } catch (error) { const nodeError = error as NodeJS.ErrnoException; if (nodeError.code === 'ENOENT') { console.log(fail('Backup was deleted during restore')); process.exit(1); } + if (nodeError.code === 'ELOOP') { + console.log(fail('Backup file is a symlink - refusing to restore for security')); + process.exit(1); + } console.log(fail(`Backup file is corrupted: ${(error as Error).message}`)); process.exit(1); } - // Security: Reject symlink backup files - if (isSymlink(backup.path)) { - console.log(fail('Backup file is a symlink - refusing to restore for security')); + + try { + await withPersistSettingsLock(async () => { + const settingsPath = getClaudeSettingsPath(); + if (await isSymlinkAsync(settingsPath)) { + throw new Error('settings.json is a symlink - refusing to restore for security'); + } + + let rollbackBackupPath: string | null = null; + if (await pathExists(settingsPath)) { + rollbackBackupPath = await createBackup(); + } + + try { + await writeClaudeSettings(parsedBackupSettings); + } catch (error) { + const writeError = error as Error; + if (rollbackBackupPath) { + try { + const rollbackContent = await readFileUtf8NoFollow(rollbackBackupPath); + const rollbackSettings = parseSettingsObject(rollbackContent, 'Rollback backup'); + await writeClaudeSettings(rollbackSettings); + } catch (rollbackError) { + throw new Error( + `Restore failed: ${writeError.message}. Rollback also failed: ${(rollbackError as Error).message}. Manual recovery backup: ${formatDisplayPath(rollbackBackupPath)}` + ); + } + } + throw new Error(`Restore failed: ${writeError.message}`); + } + }); + } catch (error) { + console.log(fail((error as Error).message)); process.exit(1); } - // Copy backup over settings.json - const settingsPath = getClaudeSettingsPath(); - // Security: Reject symlink target - if (isSymlink(settingsPath)) { - console.log(fail('settings.json is a symlink - refusing to restore for security')); - process.exit(1); - } - fs.copyFileSync(backup.path, settingsPath); + console.log(ok(`Restored from backup: ${backup.timestamp}`)); } @@ -533,6 +771,9 @@ export async function handlePersistCommand(args: string[]): Promise { return; } const parsedArgs = parseArgs(args); + if (parsedArgs.parseError) { + throw new Error(parsedArgs.parseError); + } // Handle --list-backups if (parsedArgs.listBackups) { await handleListBackups(); @@ -544,9 +785,6 @@ export async function handlePersistCommand(args: string[]): Promise { return; } await initUI(); - if (parsedArgs.parseError) { - throw new Error(parsedArgs.parseError); - } const resolvedPermissionMode = resolvePermissionMode(parsedArgs); if (!parsedArgs.profile) { console.log(fail('Profile name is required')); @@ -596,10 +834,7 @@ export async function handlePersistCommand(args: string[]): Promise { const maxKeyLen = Math.max(...envKeys.map((k) => k.length)); for (const [key, value] of Object.entries(resolved.env)) { const paddedKey = key.padEnd(maxKeyLen + 2); - const displayValue = - key.includes('TOKEN') || key.includes('KEY') || key.includes('SECRET') - ? maskApiKey(value) - : value; + const displayValue = isSensitiveEnvKey(key) ? maskApiKey(value) : value; console.log(` ${color(paddedKey, 'command')} = ${displayValue}`); } console.log(''); @@ -622,26 +857,17 @@ export async function handlePersistCommand(args: string[]): Promise { // Check if settings.json exists for backup const settingsPath = getClaudeSettingsPath(); const settingsExist = fs.existsSync(settingsPath); + let createBackupFlag = false; // Track backup path for error recovery guidance let createdBackupPath: string | null = null; // Backup prompt (unless --yes) if (settingsExist) { - let createBackupFlag: boolean = parsedArgs.yes === true; // Auto-backup with --yes + createBackupFlag = parsedArgs.yes === true; // Auto-backup with --yes if (!parsedArgs.yes) { createBackupFlag = await InteractivePrompt.confirm('Create backup before modifying?', { default: true, }); } - if (createBackupFlag) { - try { - createdBackupPath = createBackup(); - console.log(ok(`Backup created: ${formatDisplayPath(createdBackupPath)}`)); - console.log(''); - } catch (error) { - console.log(fail(`Failed to create backup: ${(error as Error).message}`)); - process.exit(1); - } - } } // Proceed confirmation (unless --yes) if (!parsedArgs.yes) { @@ -651,47 +877,72 @@ export async function handlePersistCommand(args: string[]): Promise { process.exit(0); } } - // Read existing settings and merge - const existingSettings = readClaudeSettings(); - // Validate existing env is an object (not array/primitive) - const rawEnv = existingSettings.env; - let existingEnv: Record = {}; - if (rawEnv !== undefined && rawEnv !== null) { - if (typeof rawEnv !== 'object' || Array.isArray(rawEnv)) { - console.log(warn('Existing env in settings.json is not an object - it will be replaced')); - } else { - existingEnv = rawEnv as Record; - } - } - const mergedSettings: Record = { - ...existingSettings, - env: { - ...existingEnv, - ...resolved.env, - }, - }; - if (resolvedPermissionMode) { - const rawPermissions = existingSettings.permissions; - let existingPermissions: Record = {}; - if (rawPermissions !== undefined && rawPermissions !== null) { - if (typeof rawPermissions !== 'object' || Array.isArray(rawPermissions)) { - console.log( - warn('Existing permissions in settings.json is not an object - it will be replaced') - ); - } else { - existingPermissions = rawPermissions as Record; - } - } - mergedSettings.permissions = { - ...existingPermissions, - defaultMode: resolvedPermissionMode, - }; - } - // Write merged settings try { - writeClaudeSettings(mergedSettings); + await withPersistSettingsLock(async () => { + if (createBackupFlag && (await pathExists(settingsPath))) { + try { + createdBackupPath = await createBackup(); + console.log(ok(`Backup created: ${formatDisplayPath(createdBackupPath)}`)); + console.log(''); + } catch (error) { + throw new Error(`Failed to create backup: ${(error as Error).message}`); + } + } + + // Read existing settings and merge + const existingSettings = await readClaudeSettings(); + // Validate existing env is an object (not array/primitive) + const rawEnv = existingSettings.env; + let existingEnv: Record = {}; + if (rawEnv !== undefined) { + if (rawEnv === null) { + console.log(warn('Existing env in settings.json is null - it will be replaced')); + } else if (typeof rawEnv !== 'object' || Array.isArray(rawEnv)) { + console.log(warn('Existing env in settings.json is not an object - it will be replaced')); + } else { + existingEnv = rawEnv as Record; + } + } + + const mergedSettings: Record = { + ...existingSettings, + env: { + ...existingEnv, + ...resolved.env, + }, + }; + + if (resolvedPermissionMode) { + const rawPermissions = existingSettings.permissions; + let existingPermissions: Record = {}; + if (rawPermissions !== undefined) { + if (rawPermissions === null) { + console.log( + warn('Existing permissions in settings.json is null - it will be replaced') + ); + } else if (typeof rawPermissions !== 'object' || Array.isArray(rawPermissions)) { + console.log( + warn('Existing permissions in settings.json is not an object - it will be replaced') + ); + } else { + existingPermissions = rawPermissions as Record; + } + } + mergedSettings.permissions = { + ...existingPermissions, + defaultMode: resolvedPermissionMode, + }; + } + + await writeClaudeSettings(mergedSettings); + }); } catch (error) { - console.log(fail(`Failed to write settings: ${(error as Error).message}`)); + const message = (error as Error).message; + if (message.startsWith('Failed to create backup:')) { + console.log(fail(message)); + } else { + console.log(fail(`Failed to write settings: ${message}`)); + } if (createdBackupPath) { console.log(''); console.log(info(`A backup was created before this error:`)); diff --git a/tests/unit/commands/persist-command-handler.test.ts b/tests/unit/commands/persist-command-handler.test.ts new file mode 100644 index 00000000..d285a59b --- /dev/null +++ b/tests/unit/commands/persist-command-handler.test.ts @@ -0,0 +1,273 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as lockfile from 'proper-lockfile'; +import { handlePersistCommand } from '../../../src/commands/persist-command'; + +interface RestoreFixture { + claudeDir: string; + settingsPath: string; + backupPath: string; + timestamp: string; + originalSettings: Record; + backupSettings: Record; +} + +let tempRoot: string; +let originalClaudeConfigDir: string | undefined; +let originalProcessExit: typeof process.exit; +let originalFsOpen: typeof fs.promises.open; +let originalFsRename: typeof fs.promises.rename; + +async function pathExists(filePath: string): Promise { + try { + await fs.promises.access(filePath, fs.constants.F_OK); + return true; + } catch { + return false; + } +} + +async function createRestoreFixture( + options: { + timestamp?: string; + originalSettings?: Record; + backupSettings?: Record; + } = {} +): Promise { + const timestamp = options.timestamp ?? '20260110_205324'; + const claudeDir = path.join(tempRoot, '.claude'); + const settingsPath = path.join(claudeDir, 'settings.json'); + const backupPath = `${settingsPath}.backup.${timestamp}`; + + const originalSettings = options.originalSettings ?? { + env: { ORIGINAL_TOKEN: 'original-value' }, + permissions: { defaultMode: 'plan' }, + }; + const backupSettings = options.backupSettings ?? { + env: { NEW_TOKEN: 'new-value' }, + permissions: { defaultMode: 'acceptEdits' }, + }; + + await fs.promises.mkdir(claudeDir, { recursive: true }); + await fs.promises.writeFile(settingsPath, JSON.stringify(originalSettings, null, 2) + '\n', 'utf8'); + await fs.promises.writeFile(backupPath, JSON.stringify(backupSettings, null, 2) + '\n', 'utf8'); + + return { claudeDir, settingsPath, backupPath, timestamp, originalSettings, backupSettings }; +} + +function stubProcessExit(): void { + process.exit = ((code?: number) => { + throw new Error(`process.exit(${code ?? 0})`); + }) as typeof process.exit; +} + +beforeEach(async () => { + tempRoot = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'ccs-persist-handler-test-')); + originalClaudeConfigDir = process.env.CLAUDE_CONFIG_DIR; + originalProcessExit = process.exit; + originalFsOpen = fs.promises.open; + originalFsRename = fs.promises.rename; +}); + +afterEach(async () => { + process.exit = originalProcessExit; + fs.promises.open = originalFsOpen; + fs.promises.rename = originalFsRename; + + if (originalClaudeConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR; + } else { + process.env.CLAUDE_CONFIG_DIR = originalClaudeConfigDir; + } + + if (tempRoot) { + await fs.promises.rm(tempRoot, { recursive: true, force: true }); + } +}); + +describe('persist command real handler paths', () => { + it('throws parseError for missing --permission-mode before profile detection', async () => { + await expect(handlePersistCommand(['glm', '--permission-mode'])).rejects.toThrow( + 'Missing value for --permission-mode' + ); + }); + + it('throws parseError for empty inline --permission-mode before profile detection', async () => { + await expect(handlePersistCommand(['glm', '--permission-mode='])).rejects.toThrow( + 'Missing value for --permission-mode' + ); + }); + + it('throws parseError for invalid --permission-mode before profile detection', async () => { + await expect(handlePersistCommand(['glm', '--permission-mode', 'invalid-mode'])).rejects.toThrow( + /Invalid --permission-mode/ + ); + }); + + it('throws parseError for unknown flags on real handler path', async () => { + await expect(handlePersistCommand(['glm', '--unknown-flag'])).rejects.toThrow( + /Unknown option\(s\)/ + ); + }); + + it('throws parseError for list/restore conflict on real handler path', async () => { + await expect(handlePersistCommand(['--list-backups', '--restore'])).rejects.toThrow( + '--list-backups cannot be used with --restore' + ); + }); + + it('throws parseError for permission flags with --restore on real handler path', async () => { + await expect(handlePersistCommand(['--restore', '--auto-approve'])).rejects.toThrow( + /Permission flags are not valid with backup operations/ + ); + }); + + it('shows help when --help is present even with other invalid args', async () => { + await expect(handlePersistCommand(['--help', '--permission-mode'])).resolves.toBeUndefined(); + }); + + it('does not create CLAUDE_CONFIG_DIR on parseError path', async () => { + const isolatedClaudeDir = path.join(tempRoot, '.claude-parse-early'); + process.env.CLAUDE_CONFIG_DIR = isolatedClaudeDir; + + await expect(handlePersistCommand(['glm', '--permission-mode='])).rejects.toThrow( + 'Missing value for --permission-mode' + ); + expect(await pathExists(isolatedClaudeDir)).toBe(false); + }); +}); + +describe('persist command restore failure handling', () => { + it('exits when lock cannot be acquired (concurrency protection)', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + const release = await lockfile.lock(fixture.claudeDir, { + stale: 60000, + retries: { retries: 0 }, + realpath: false, + }); + + stubProcessExit(); + try { + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + } finally { + await release(); + } + }); + + it('exits when backup read fails with ENOENT after selection', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + fs.promises.open = (async (...args: Parameters) => { + const target = String(args[0]); + if (target === fixture.backupPath) { + const error = new Error('forced missing backup') as NodeJS.ErrnoException; + error.code = 'ENOENT'; + throw error; + } + return originalFsOpen(...args); + }) as typeof fs.promises.open; + + stubProcessExit(); + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + }); + + it('exits when backup read fails with ELOOP (symlink rejection)', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + fs.promises.open = (async (...args: Parameters) => { + const target = String(args[0]); + if (target === fixture.backupPath) { + const error = new Error('forced symlink rejection') as NodeJS.ErrnoException; + error.code = 'ELOOP'; + throw error; + } + return originalFsOpen(...args); + }) as typeof fs.promises.open; + + stubProcessExit(); + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + }); + + it('exits when backup path resolves to a non-regular file', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + fs.promises.open = (async (...args: Parameters) => { + const target = String(args[0]); + if (target === fixture.backupPath) { + const fakeHandle = { + stat: async () => ({ isFile: () => false }), + readFile: async () => '', + close: async () => undefined, + } as unknown as fs.promises.FileHandle; + return fakeHandle; + } + return originalFsOpen(...args); + }) as typeof fs.promises.open; + + stubProcessExit(); + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + }); + + it('rolls back settings when restore write fails mid-flight', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + let renameCalls = 0; + fs.promises.rename = (async (...args: Parameters) => { + renameCalls += 1; + if (renameCalls === 1) { + throw new Error('forced rename failure'); + } + return originalFsRename(...args); + }) as typeof fs.promises.rename; + + stubProcessExit(); + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + + const finalContent = await fs.promises.readFile(fixture.settingsPath, 'utf8'); + const finalSettings = JSON.parse(finalContent); + expect(finalSettings).toEqual(fixture.originalSettings); + }); + + it('includes dual failure context when restore write and rollback both fail', async () => { + const fixture = await createRestoreFixture(); + process.env.CLAUDE_CONFIG_DIR = fixture.claudeDir; + + fs.promises.rename = (async () => { + throw new Error('forced rename failure'); + }) as typeof fs.promises.rename; + + const originalConsoleLog = console.log; + const capturedLogs: string[] = []; + console.log = (...args: unknown[]) => { + capturedLogs.push(args.map((arg) => String(arg)).join(' ')); + }; + + stubProcessExit(); + try { + await expect(handlePersistCommand(['--restore', fixture.timestamp, '--yes'])).rejects.toThrow( + 'process.exit(1)' + ); + expect(capturedLogs.some((line) => line.includes('Rollback also failed'))).toBe(true); + } finally { + console.log = originalConsoleLog; + } + }); +}); diff --git a/tests/unit/commands/persist-command.test.js b/tests/unit/commands/persist-command.test.js index 11e15eb0..894a9157 100644 --- a/tests/unit/commands/persist-command.test.js +++ b/tests/unit/commands/persist-command.test.js @@ -23,6 +23,17 @@ describe('Persist Command', () => { */ function parseArgs(args) { const validPermissionModes = ['default', 'plan', 'acceptEdits', 'bypassPermissions']; + const knownFlags = [ + '--yes', + '-y', + '--list-backups', + '--restore', + '--permission-mode', + '--dangerously-skip-permissions', + '--auto-approve', + '--help', + '-h', + ]; const result = { profile: undefined, yes: false, @@ -32,6 +43,7 @@ describe('Persist Command', () => { dangerouslySkipPermissions: false, parseError: undefined, }; + const unknownFlags = []; for (let i = 0; i < args.length; i++) { const arg = args[i]; if (arg === '--yes' || arg === '-y') { @@ -72,8 +84,32 @@ describe('Persist Command', () => { result.dangerouslySkipPermissions = true; } else if (!arg.startsWith('-') && !result.profile) { result.profile = arg; + } else if (arg.startsWith('-')) { + const known = knownFlags.some((flag) => arg === flag || arg.startsWith(`${flag}=`)); + if (!known) { + unknownFlags.push(arg); + } } } + + if (!result.parseError && unknownFlags.length > 0) { + const unknownList = unknownFlags.map((flag) => `"${flag}"`).join(', '); + result.parseError = `Unknown option(s): ${unknownList}. Run 'ccs persist --help' for usage.`; + } + + if (!result.parseError && result.listBackups && result.restore) { + result.parseError = '--list-backups cannot be used with --restore'; + } + + if ( + !result.parseError && + (result.listBackups || result.restore) && + (result.permissionMode || result.dangerouslySkipPermissions) + ) { + result.parseError = + 'Permission flags are not valid with backup operations. Use them only with ccs persist .'; + } + return result; } @@ -120,10 +156,9 @@ describe('Persist Command', () => { assert.strictEqual(result.yes, false); }); - it('ignores unknown flags', () => { + it('rejects unknown flags with a clear parseError', () => { const result = parseArgs(['glm', '--unknown', '--yes']); - assert.strictEqual(result.profile, 'glm'); - assert.strictEqual(result.yes, true); + assert.match(result.parseError, /Unknown option\(s\)/); }); it('takes only first positional as profile', () => { @@ -177,6 +212,16 @@ describe('Persist Command', () => { assert.match(result.parseError, /Invalid --permission-mode/); }); + it('sets parseError for missing --permission-mode value', () => { + const result = parseArgs(['glm', '--permission-mode']); + assert.strictEqual(result.parseError, 'Missing value for --permission-mode'); + }); + + it('sets parseError for empty inline --permission-mode=', () => { + const result = parseArgs(['glm', '--permission-mode=']); + assert.strictEqual(result.parseError, 'Missing value for --permission-mode'); + }); + it('parses --dangerously-skip-permissions flag', () => { const result = parseArgs(['glm', '--dangerously-skip-permissions']); assert.strictEqual(result.dangerouslySkipPermissions, true); @@ -193,25 +238,68 @@ describe('Persist Command', () => { assert.strictEqual(resolvePermissionMode(result), 'bypassPermissions'); }); - it('throws on conflict between dangerous mode and non-bypass permission mode', () => { - const parsed = parseArgs(['glm', '--permission-mode', 'acceptEdits', '--auto-approve']); + ['acceptEdits', 'plan', 'default'].forEach((mode) => { + it(`throws on conflict between dangerous mode and --permission-mode ${mode}`, () => { + const parsed = parseArgs(['glm', '--permission-mode', mode, '--auto-approve']); - assert.throws( - () => resolvePermissionMode(parsed), - /--dangerously-skip-permissions conflicts with --permission-mode/ - ); - }); - - it('keeps test permission mode list aligned with source constant', () => { - const sourcePath = path.join(__dirname, '../../../src/commands/persist-command.ts'); - const sourceContent = fs.readFileSync(sourcePath, 'utf8'); - ['default', 'plan', 'acceptEdits', 'bypassPermissions'].forEach((mode) => { - assert( - sourceContent.includes(`'${mode}'`), - `Expected mode '${mode}' to exist in source constant` + assert.throws( + () => resolvePermissionMode(parsed), + /--dangerously-skip-permissions conflicts with --permission-mode/ ); }); }); + + it('allows dangerous mode with --permission-mode bypassPermissions', () => { + const parsed = parseArgs([ + 'glm', + '--permission-mode', + 'bypassPermissions', + '--dangerously-skip-permissions', + ]); + assert.strictEqual(resolvePermissionMode(parsed), 'bypassPermissions'); + }); + + it('sets parseError when --list-backups and --restore are both provided', () => { + const parsed = parseArgs(['--list-backups', '--restore']); + assert.strictEqual(parsed.parseError, '--list-backups cannot be used with --restore'); + }); + + it('sets parseError when permission flags are used with --restore', () => { + const parsed = parseArgs(['--restore', '--auto-approve']); + assert.match(parsed.parseError, /Permission flags are not valid with backup operations/); + }); + + it('sets parseError when permission flags are used with --list-backups', () => { + const parsed = parseArgs(['--list-backups', '--permission-mode', 'plan']); + assert.match(parsed.parseError, /Permission flags are not valid with backup operations/); + }); + + it('keeps test permission mode list exactly aligned with source constant', () => { + const sourcePath = path.join(__dirname, '../../../src/commands/persist-command.ts'); + const sourceContent = fs.readFileSync(sourcePath, 'utf8'); + const match = sourceContent.match(/const VALID_PERMISSION_MODES = \[(.*?)\] as const/s); + assert(match, 'Expected VALID_PERMISSION_MODES constant to exist'); + const sourceModes = [...match[1].matchAll(/'([^']+)'/g)].map((entry) => entry[1]).sort(); + const testModes = ['default', 'plan', 'acceptEdits', 'bypassPermissions'].sort(); + assert.deepStrictEqual(sourceModes, testModes); + }); + + it('accepts common option ordering permutations', () => { + const cases = [ + ['glm', '--yes', '--permission-mode', 'plan'], + ['--yes', 'glm', '--permission-mode=plan'], + ['--permission-mode', 'plan', 'glm', '--yes'], + ['--yes', '--permission-mode', 'plan', 'glm'], + ]; + + for (const input of cases) { + const parsed = parseArgs(input); + assert.strictEqual(parsed.parseError, undefined, `Expected no parseError for: ${input.join(' ')}`); + assert.strictEqual(parsed.profile, 'glm'); + assert.strictEqual(parsed.yes, true); + assert.strictEqual(parsed.permissionMode, 'plan'); + } + }); }); // ========================================================================= @@ -469,7 +557,34 @@ describe('Persist Command', () => { * Simulates the logic to detect sensitive keys for masking */ function isSensitiveKey(key) { - return key.includes('TOKEN') || key.includes('KEY') || key.includes('SECRET'); + const sensitiveParts = new Set([ + 'TOKEN', + 'KEY', + 'SECRET', + 'PASSWORD', + 'PASS', + 'AUTH', + 'CREDENTIAL', + 'PRIVATE', + 'ACCESS', + 'REFRESH', + 'APIKEY', + ]); + const withCamelCaseBoundaries = key.replace(/([a-z0-9])([A-Z])/g, '$1_$2'); + const parts = withCamelCaseBoundaries.toUpperCase().split(/[^A-Z0-9]+/).filter(Boolean); + if (parts.some((part) => sensitiveParts.has(part))) { + return true; + } + const compact = parts.join(''); + return ( + compact.includes('TOKEN') || + compact.includes('APIKEY') || + compact.includes('ACCESSKEY') || + compact.includes('AUTHKEY') || + compact.includes('SECRET') || + compact.includes('PASSWORD') || + compact.includes('CREDENTIAL') + ); } it('detects TOKEN in key name', () => { @@ -492,6 +607,24 @@ describe('Persist Command', () => { assert.strictEqual(isSensitiveKey('ANTHROPIC_MODEL'), false); assert.strictEqual(isSensitiveKey('DISABLE_TELEMETRY'), false); }); + + it('detects lowercase and mixed-case sensitive keys', () => { + assert.strictEqual(isSensitiveKey('anthropic_auth_token'), true); + assert.strictEqual(isSensitiveKey('Api_Key'), true); + assert.strictEqual(isSensitiveKey('clientSecret'), true); + }); + + it('detects additional secret families', () => { + assert.strictEqual(isSensitiveKey('DB_PASSWORD'), true); + assert.strictEqual(isSensitiveKey('refresh_token_value'), true); + assert.strictEqual(isSensitiveKey('private_credential_blob'), true); + }); + + it('detects camelCase sensitive key variants', () => { + assert.strictEqual(isSensitiveKey('accessKeyId'), true); + assert.strictEqual(isSensitiveKey('authTokenValue'), true); + assert.strictEqual(isSensitiveKey('apiKeyValue'), true); + }); }); // =========================================================================