From dbe8bfbba9449e4baeefc27366b9b0eb31e3ad48 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:31:33 -0400 Subject: [PATCH] fix(install): pin Claude settings parent during atomic replacement Reject directory replacement after temporary file creation or staging, preserve unrelated files during cleanup, and retry settings edits observed before the final rename. Add three regression tests for the review findings. --- scripts/lib/atomic-write.js | 15 +++- scripts/lib/install/claude-settings.js | 23 ++++++- tests/lib/claude-settings.test.js | 95 ++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 2 deletions(-) diff --git a/scripts/lib/atomic-write.js b/scripts/lib/atomic-write.js index e3d41df0d..9e9e524fe 100644 --- a/scripts/lib/atomic-write.js +++ b/scripts/lib/atomic-write.js @@ -13,21 +13,34 @@ function writeFileAtomic(filePath, content, options = {}) { ); const mode = options.mode || 0o600; + if (options.validateParent) options.validateParent(); fs.mkdirSync(parentDir, { recursive: true }); let descriptor; try { + if (options.validateParent) options.validateParent(); descriptor = fs.openSync(tempPath, 'wx', mode); + if (options.validateParent) options.validateParent(); fs.writeFileSync(descriptor, content, { encoding: options.encoding || 'utf8' }); fs.fsyncSync(descriptor); fs.closeSync(descriptor); descriptor = undefined; + if (options.validateParent) options.validateParent(); + if (options.beforeRename) options.beforeRename(); fs.renameSync(tempPath, resolvedPath); } catch (error) { if (descriptor !== undefined) { fs.closeSync(descriptor); } - fs.rmSync(tempPath, { force: true }); + // If the parent was replaced, this pathname may now name somebody else's + // file. Leave the private staging file in its original directory. + let parentUnchanged = true; + try { + if (options.validateParent) options.validateParent(); + } catch (_error) { + parentUnchanged = false; + } + if (parentUnchanged) fs.rmSync(tempPath, { force: true }); throw error; } diff --git a/scripts/lib/install/claude-settings.js b/scripts/lib/install/claude-settings.js index dd34dd6fb..3c18668a6 100644 --- a/scripts/lib/install/claude-settings.js +++ b/scripts/lib/install/claude-settings.js @@ -389,9 +389,23 @@ function assertSettingsSnapshotUnchanged(settingsPath, snapshot) { function updateSettingsAtomic(settingsPath, transform, options = {}) { const update = () => { + const parentPath = path.dirname(path.resolve(settingsPath)); + const parentStats = fs.lstatSync(parentPath, { bigint: true }); + const validateParent = () => { + const current = fs.lstatSync(parentPath, { bigint: true }); + if ( + !current.isDirectory() || current.isSymbolicLink() + || current.dev !== parentStats.dev || current.ino !== parentStats.ino + ) { + const error = new Error(`Claude settings parent directory changed: ${parentPath}`); + error.code = 'ECC_SETTINGS_PARENT_CHANGED'; + throw error; + } + }; const maxAttempts = options.maxAttempts || 3; for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { try { + validateParent(); const snapshot = readSettingsSnapshot(settingsPath); const result = transform(snapshot.settings); if (typeof options.beforeCommit === 'function') options.beforeCommit(); @@ -399,7 +413,14 @@ function updateSettingsAtomic(settingsPath, transform, options = {}) { writeFileAtomic( settingsPath, `${JSON.stringify(result.settings, null, 2)}\n`, - { encoding: 'utf8', mode: snapshot.mode } + { + encoding: 'utf8', + mode: snapshot.mode, + validateParent, + beforeRename() { + assertSettingsSnapshotUnchanged(settingsPath, snapshot); + }, + } ); return result; } catch (error) { diff --git a/tests/lib/claude-settings.test.js b/tests/lib/claude-settings.test.js index c31f191f9..26d094fc4 100644 --- a/tests/lib/claude-settings.test.js +++ b/tests/lib/claude-settings.test.js @@ -48,6 +48,67 @@ function clone(value) { return JSON.parse(JSON.stringify(value)); } +function assertAtomicParentReplacementRejected(stage) { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'claude-settings-parent-race-')); + const targetRoot = path.join(tempDir, 'target'); + const parkedRoot = path.join(tempDir, 'parked'); + const victimRoot = path.join(tempDir, 'victim'); + const settingsPath = path.join(targetRoot, 'settings.json'); + const victimPath = path.join(victimRoot, 'settings.json'); + const originalOpen = fs.openSync; + const originalFsync = fs.fsyncSync; + const targetContent = '{"target":true}\n'; + const victimContent = '{"victim":"preserve"}\n'; + let tempDescriptor; + let tempBasename; + let replaced = false; + const replaceParent = () => { + replaced = true; + fs.renameSync(targetRoot, parkedRoot); + fs.symlinkSync(victimRoot, targetRoot, process.platform === 'win32' ? 'junction' : 'dir'); + // A colliding path in the replacement directory must survive error cleanup. + fs.writeFileSync(path.join(victimRoot, tempBasename), 'unrelated replacement file'); + }; + try { + fs.mkdirSync(targetRoot); + fs.mkdirSync(victimRoot); + fs.writeFileSync(settingsPath, targetContent); + fs.writeFileSync(victimPath, victimContent); + fs.openSync = function(file, flags, ...args) { + const isTemp = typeof file === 'string' + && path.basename(file).startsWith('.settings.json.') && file.endsWith('.tmp'); + if (isTemp) tempBasename = path.basename(file); + if (isTemp && !replaced && stage === 'open') { + // Replace immediately after the temporary descriptor has been created. + const descriptor = originalOpen.call(fs, file, flags, ...args); + tempDescriptor = descriptor; + replaceParent(); + return descriptor; + } + const descriptor = originalOpen.call(fs, file, flags, ...args); + if (isTemp) tempDescriptor = descriptor; + return descriptor; + }; + fs.fsyncSync = function(descriptor) { + const result = originalFsync.call(fs, descriptor); + if (!replaced && stage === 'rename' && descriptor === tempDescriptor) replaceParent(); + return result; + }; + assert.throws( + () => updateSettingsAtomic(settingsPath, settings => ({ settings: { ...settings, managed: true } })), + /parent.*changed|changed.*parent/i + ); + assert.ok(replaced, 'must exercise a replacement inside the atomic writer'); + assert.strictEqual(fs.readFileSync(victimPath, 'utf8'), victimContent); + assert.strictEqual(fs.readFileSync(path.join(parkedRoot, 'settings.json'), 'utf8'), targetContent); + assert.strictEqual(fs.readFileSync(path.join(victimRoot, tempBasename), 'utf8'), 'unrelated replacement file'); + } finally { + fs.openSync = originalOpen; + fs.fsyncSync = originalFsync; + fs.rmSync(tempDir, { recursive: true, force: true }); + } +} + function runTests() { console.log('\n=== Testing install/claude-settings.js ===\n'); @@ -224,6 +285,40 @@ function runTests() { } })) passed++; else failed++; + for (const stage of ['open', 'rename']) { + if (test(`atomic settings updates reject parent replacement at ${stage} without touching its files`, () => { + assertAtomicParentReplacementRejected(stage); + })) passed++; else failed++; + } + + if (test('atomic settings updates preserve edits made while the replacement file is staged', () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'claude-settings-late-edit-')); + const settingsPath = path.join(tempDir, 'settings.json'); + const originalFsync = fs.fsyncSync; + let changed = false; + let fsyncCalls = 0; + try { + fs.writeFileSync(settingsPath, '{"theme":"initial"}\n'); + fs.fsyncSync = function(descriptor) { + const result = originalFsync.call(fs, descriptor); + // Lock creation is the first fsync; only change settings after the + // atomic writer has staged its first replacement payload. + fsyncCalls += 1; + if (!changed && fsyncCalls === 2) { + changed = true; + fs.writeFileSync(settingsPath, '{"theme":"late-edit"}\n'); + } + return result; + }; + updateSettingsAtomic(settingsPath, settings => ({ settings: { ...settings, managed: true } })); + assert.ok(changed); + assert.deepStrictEqual(JSON.parse(fs.readFileSync(settingsPath, 'utf8')), { theme: 'late-edit', managed: true }); + } finally { + fs.fsyncSync = originalFsync; + fs.rmSync(tempDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + if (test('atomic settings updates recover a stale invalid lock after its lease', () => { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'claude-settings-stale-lock-')); const settingsPath = path.join(tempDir, 'settings.json');