From 396fadc7849d2f2828d6081de02af032f1386cba Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 26 Jul 2026 05:16:06 -0400 Subject: [PATCH] fix: pin Hookify rules before path inspection Open rule files before trusting path metadata, compare the descriptor identity to the live path, and only then validate containment and read bytes. Replace the unsafe temp-file race simulation with an open-first descriptor/path-swap regression. --- scripts/hooks/hookify-loader.js | 82 ++++++++++++++++-------------- tests/hooks/hookify-loader.test.js | 28 +++++++--- 2 files changed, 63 insertions(+), 47 deletions(-) diff --git a/scripts/hooks/hookify-loader.js b/scripts/hooks/hookify-loader.js index 451ed967d..5e4e1fe7c 100644 --- a/scripts/hooks/hookify-loader.js +++ b/scripts/hooks/hookify-loader.js @@ -355,6 +355,31 @@ function loadRuleFile({ const filePath = path.join(claudeDir, fileName); let fileDescriptor; try { + const noFollow = fs.constants.O_NOFOLLOW || 0; + try { + fileDescriptor = fs.openSync(filePath, fs.constants.O_RDONLY | noFollow); + } catch (error) { + if ( + error && + ['ELOOP', 'EMLINK', 'ENOENT', 'ENOTDIR'].includes(error.code) + ) { + return { + rule: null, + diagnostic: diagnostic('HOOKIFY_RULE_FILE_UNSAFE', fileName, 'not a regular file'), + bytesRead: 0, + }; + } + throw error; + } + const fileStat = fs.fstatSync(fileDescriptor); + if (!fileStat.isFile()) { + return { + rule: null, + diagnostic: diagnostic('HOOKIFY_RULE_FILE_UNSAFE', fileName, 'not a regular file'), + bytesRead: 0, + }; + } + const linkStat = fs.lstatSync(filePath); if (linkStat.isSymbolicLink() || !linkStat.isFile()) { return { @@ -363,6 +388,23 @@ function loadRuleFile({ bytesRead: 0, }; } + if ( + fileStat.dev !== linkStat.dev || + fileStat.ino !== linkStat.ino || + fileStat.mode !== linkStat.mode || + fileStat.size !== linkStat.size || + fileStat.mtimeMs !== linkStat.mtimeMs + ) { + return { + rule: null, + diagnostic: diagnostic( + 'HOOKIFY_RULE_FILE_UNSAFE', + fileName, + 'rule identity changed during evaluation' + ), + bytesRead: 0, + }; + } const realDirectory = fs.realpathSync(claudeDir); if (expectedRealDirectory && realDirectory !== expectedRealDirectory) { @@ -380,55 +422,17 @@ function loadRuleFile({ if ( path.dirname(realFile) !== realDirectory || realFile !== path.join(realDirectory, fileName) - ) { - return { - rule: null, - diagnostic: diagnostic('HOOKIFY_RULE_FILE_UNSAFE', fileName, 'resolved outside project .claude'), - bytesRead: 0, - }; - } - - const noFollow = fs.constants.O_NOFOLLOW || 0; - fileDescriptor = fs.openSync(filePath, fs.constants.O_RDONLY | noFollow); - const fileStat = fs.fstatSync(fileDescriptor); - if ( - fileStat.dev !== linkStat.dev || - fileStat.ino !== linkStat.ino || - fileStat.mode !== linkStat.mode || - fileStat.size !== linkStat.size || - fileStat.mtimeMs !== linkStat.mtimeMs ) { return { rule: null, diagnostic: diagnostic( 'HOOKIFY_RULE_FILE_UNSAFE', fileName, - 'rule identity changed during evaluation' + 'resolved outside project .claude' ), bytesRead: 0, }; } - if ( - fs.realpathSync(claudeDir) !== realDirectory || - fs.realpathSync(filePath) !== realFile - ) { - return { - rule: null, - diagnostic: diagnostic( - 'HOOKIFY_RULE_FILE_UNSAFE', - fileName, - 'rule path changed during evaluation' - ), - bytesRead: 0, - }; - } - if (!fileStat.isFile()) { - return { - rule: null, - diagnostic: diagnostic('HOOKIFY_RULE_FILE_UNSAFE', fileName, 'not a regular file'), - bytesRead: 0, - }; - } if (fileStat.size > LIMITS.maxFileBytes) { return { rule: null, diff --git a/tests/hooks/hookify-loader.test.js b/tests/hooks/hookify-loader.test.js index 372ed815c..ad8d88545 100644 --- a/tests/hooks/hookify-loader.test.js +++ b/tests/hooks/hookify-loader.test.js @@ -210,7 +210,7 @@ function runTests() { }); })) passed++; else failed++; - if (test('rejects a rule path swapped through a symlink while the file is opened', () => { + if (test('opens a rule descriptor before path inspection and rejects a later symlink swap', () => { withProject(({ projectRoot, claudeDir }) => { const fileName = 'hookify.race.local.md'; const rulePath = path.join(claudeDir, fileName); @@ -231,19 +231,28 @@ function runTests() { 'This outside rule must never be loaded.', ].join('\n')); - const originalOpenSync = fs.openSync; + const originalFstatSync = fs.fstatSync; + const originalLstatSync = fs.lstatSync; + let descriptorOpened = false; + let inspectedBeforeOpen = false; let swapped = false; - fs.openSync = function openWithSwap(target, flags, ...args) { - if (!swapped && target === rulePath) { + fs.fstatSync = function markDescriptorOpen(descriptor, ...args) { + descriptorOpened = true; + return originalFstatSync.call(fs, descriptor, ...args); + }; + fs.lstatSync = function inspectWithSwap(target, ...args) { + if (target === rulePath && !descriptorOpened) { + inspectedBeforeOpen = true; + } else if (!swapped && target === rulePath) { swapped = true; fs.renameSync(rulePath, backupPath); fs.symlinkSync(outsidePath, rulePath); - const descriptor = originalOpenSync.call(fs, target, fs.constants.O_RDONLY, ...args); + const linkStat = originalLstatSync.call(fs, target, ...args); fs.unlinkSync(rulePath); fs.renameSync(backupPath, rulePath); - return descriptor; + return linkStat; } - return originalOpenSync.call(fs, target, flags, ...args); + return originalLstatSync.call(fs, target, ...args); }; try { @@ -255,8 +264,11 @@ function runTests() { }); assert.strictEqual(result.rule, null); assert.strictEqual(result.diagnostic.code, 'HOOKIFY_RULE_FILE_UNSAFE'); + assert.strictEqual(inspectedBeforeOpen, false); + assert.strictEqual(swapped, true); } finally { - fs.openSync = originalOpenSync; + fs.fstatSync = originalFstatSync; + fs.lstatSync = originalLstatSync; if (fs.existsSync(backupPath) && !fs.existsSync(rulePath)) { fs.renameSync(backupPath, rulePath); }