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); }