fix(persist): harden persist restore safety and edge cases

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 <profile>.';
+  }
+
   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<string, unknown> {
-  const settingsPath = getClaudeSettingsPath();
+async function pathExists(filePath: string): Promise<boolean> {
   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<string, unknown>;
-  } 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<string, unknown>): 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<boolean> {
   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<string, number>)['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<string> {
+  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<string, unknown> {
+  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<string, unknown>;
+}
+
+async function withPersistSettingsLock<T>(operation: () => Promise<T>): Promise<T> {
   const settingsPath = getClaudeSettingsPath();
-  if (!fs.existsSync(settingsPath)) {
+  const settingsDir = path.dirname(settingsPath);
+  await fs.promises.mkdir(settingsDir, { recursive: true });
+
+  let release: (() => Promise<void>) | 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<Record<string, unknown>> {
+  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<string, unknown>): Promise<void> {
+  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<string> {
+  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<string, unknown>;
   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<void> {
     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<void> {
     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<void> {
   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<void> {
   // 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<void> {
       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<string, string> = {};
-  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<string, string>;
-    }
-  }
-  const mergedSettings: Record<string, unknown> = {
-    ...existingSettings,
-    env: {
-      ...existingEnv,
-      ...resolved.env,
-    },
-  };
-  if (resolvedPermissionMode) {
-    const rawPermissions = existingSettings.permissions;
-    let existingPermissions: Record<string, unknown> = {};
-    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<string, unknown>;
-      }
-    }
-    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<string, string> = {};
+      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<string, string>;
+        }
+      }
+
+      const mergedSettings: Record<string, unknown> = {
+        ...existingSettings,
+        env: {
+          ...existingEnv,
+          ...resolved.env,
+        },
+      };
+
+      if (resolvedPermissionMode) {
+        const rawPermissions = existingSettings.permissions;
+        let existingPermissions: Record<string, unknown> = {};
+        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<string, unknown>;
+          }
+        }
+        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:`));