diff --git a/scripts/hooks/hookify-runtime.js b/scripts/hooks/hookify-runtime.js index 76f313480..88cd81068 100644 --- a/scripts/hooks/hookify-runtime.js +++ b/scripts/hooks/hookify-runtime.js @@ -212,21 +212,60 @@ function normalizeRule(frontmatter, message, sourcePath) { }); } -function readRuleFile(filePath) { - const stat = fs.lstatSync(filePath); - if (stat.isSymbolicLink()) throw new Error('symbolic links are not loaded'); - if (!stat.isFile()) throw new Error('rule path is not a regular file'); - if (stat.size > MAX_RULE_BYTES) throw new Error('rule exceeds ' + MAX_RULE_BYTES + ' bytes'); +function sameRuleIdentity(left, right) { + return ['dev', 'ino', 'uid', 'gid', 'mode', 'nlink'].every(key => left[key] === right[key]); +} - const noFollow = fs.constants.O_NOFOLLOW || 0; - const fd = fs.openSync(filePath, fs.constants.O_RDONLY | noFollow); +function sameRuleSnapshot(left, right) { + return sameRuleIdentity(left, right) + && ['size', 'mtimeNs', 'ctimeNs'].every(key => left[key] === right[key]); +} + +function assertRuleSnapshot(filePath, fd, openedStat, directoryStat) { + const directory = fs.lstatSync(path.dirname(filePath), { bigint: true }); + const current = fs.lstatSync(filePath, { bigint: true }); + if (directory.isSymbolicLink() || !directory.isDirectory() + || !sameRuleIdentity(directory, directoryStat) + || current.isSymbolicLink() || !current.isFile() + || !sameRuleSnapshot(current, openedStat) + || !sameRuleSnapshot(fs.fstatSync(fd, { bigint: true }), openedStat)) { + throw new Error('rule file or directory changed while reading'); + } +} + +function readBoundedRule(fd) { + const bytes = Buffer.alloc(MAX_RULE_BYTES + 1); + let total = 0; + while (total < bytes.length) { + const count = fs.readSync(fd, bytes, total, bytes.length - total, total); + if (count === 0) break; + total += count; + } + if (total > MAX_RULE_BYTES) throw new Error('rule exceeds ' + MAX_RULE_BYTES + ' bytes'); + return bytes.subarray(0, total); +} + +function readRuleFile(filePath, directoryStat) { + const flags = fs.constants.O_RDONLY | (fs.constants.O_NOFOLLOW || 0) + | (fs.constants.O_NONBLOCK || 0); + let fd; try { - const openedStat = fs.fstatSync(fd); - if (!openedStat.isFile() || openedStat.size > MAX_RULE_BYTES) { - throw new Error('rule exceeds ' + MAX_RULE_BYTES + ' bytes or is not a regular file'); + fd = fs.openSync(filePath, flags); + } catch (error) { + if (error.code === 'ELOOP') throw new Error('symbolic links are not loaded'); + throw error; + } + try { + const openedStat = fs.fstatSync(fd, { bigint: true }); + if (!openedStat.isFile()) throw new Error('rule path is not a regular file'); + if (openedStat.size > BigInt(MAX_RULE_BYTES)) { + throw new Error('rule exceeds ' + MAX_RULE_BYTES + ' bytes'); } - const source = fs.readFileSync(fd, 'utf8'); - const document = extractFrontmatter(source); + assertRuleSnapshot(filePath, fd, openedStat, directoryStat); + const bytes = readBoundedRule(fd); + assertRuleSnapshot(filePath, fd, openedStat, directoryStat); + if (BigInt(bytes.length) !== openedStat.size) throw new Error('rule size changed while reading'); + const document = extractFrontmatter(bytes.toString('utf8')); if (!document) throw new Error('missing YAML frontmatter'); const frontmatter = parseRuleFrontmatter(document.yaml); return normalizeRule(frontmatter, document.message, filePath); @@ -241,9 +280,10 @@ function loadRules(projectRoot, options = {}) { const excludedPaths = options.excludedPaths || new Set(); const rulesDir = path.join(path.resolve(projectRoot), '.claude'); let entries; + let directoryStat; try { - const dirStat = fs.lstatSync(rulesDir); - if (dirStat.isSymbolicLink() || !dirStat.isDirectory()) { + directoryStat = fs.lstatSync(rulesDir, { bigint: true }); + if (directoryStat.isSymbolicLink() || !directoryStat.isDirectory()) { return { rules, diagnostics: [diagnostic('.claude', 'rule directory must be a real directory')] }; } entries = fs.readdirSync(rulesDir, { withFileTypes: true }); @@ -269,7 +309,7 @@ function loadRules(projectRoot, options = {}) { } for (const entry of eligibleCandidates.slice(0, MAX_RULES)) { try { - const rule = readRuleFile(path.join(rulesDir, entry.name)); + const rule = readRuleFile(path.join(rulesDir, entry.name), directoryStat); if (rule.enabled) rules.push(rule); } catch (error) { diagnostics.push(diagnostic(entry.name, error.message)); diff --git a/tests/hermes-hooks.test.js b/tests/hermes-hooks.test.js index de614a209..24aa7042a 100644 --- a/tests/hermes-hooks.test.js +++ b/tests/hermes-hooks.test.js @@ -90,11 +90,11 @@ run('blocks edits to every protected config name', () => { } }); -run('blocks case-variant names (case-insensitive filesystems)', () => { +run('blocks existing case-variant protected names on every filesystem', () => { const target = path.join(tmp, 'ESLINT.CONFIG.MJS'); - fs.writeFileSync(path.join(tmp, 'eslint.config.mjs'), 'export default {}\n'); - // On a case-sensitive filesystem the variant is a distinct name; the hook - // must still refuse it so a macOS/Windows rename cannot smuggle an edit. + fs.writeFileSync(target, 'export default {}\n'); + // The exact variant must exist: first-time config creation is allowed. + assert.ok(fs.existsSync(target), 'the probed protected file must exist'); const { stdout } = runHook(GUARD, { hook_event_name: 'pre_tool_call', tool_name: 'patch', @@ -248,13 +248,21 @@ run('hermes-hooks module installs via the real installer and the landed guard bl assert.ok(fs.existsSync(landed), 'guard must land under ~/.hermes/hooks/hermes/'); assert.ok(fs.existsSync(path.join(home, '.hermes', 'docs', 'HERMES-HOOKS.md')), 'doc must land'); const t = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-guard-')); - const target = path.join(t, 'eslint.config.mjs'); + const target = path.join(t, 'ESLINT.CONFIG.MJS'); fs.writeFileSync(target, 'export default {}\n'); const g = spawnSync('python3', [landed], { input: JSON.stringify({ hook_event_name: 'pre_tool_call', tool_name: 'patch', tool_input: { path: target, old_string: 'a', new_string: 'b' } }), encoding: 'utf8', timeout: 15000, }); assert.ok((g.stdout || '').includes('"decision": "block"'), 'landed guard must block'); + fs.unlinkSync(target); + const bootstrap = spawnSync('python3', [landed], { + input: JSON.stringify({ hook_event_name: 'pre_tool_call', tool_name: 'patch', tool_input: { path: target } }), + encoding: 'utf8', timeout: 15000, + }); + assert.strictEqual(bootstrap.status, 0, bootstrap.stderr); + assert.strictEqual(bootstrap.stdout.trim(), '', 'first-time config creation remains allowed'); + fs.rmSync(home, { recursive: true, force: true }); fs.rmSync(t, { recursive: true, force: true }); }); diff --git a/tests/hooks/hookify-file-race.test.js b/tests/hooks/hookify-file-race.test.js new file mode 100644 index 000000000..f20e85d08 --- /dev/null +++ b/tests/hooks/hookify-file-race.test.js @@ -0,0 +1,202 @@ +'use strict'; + +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { Module, createRequire } = require('node:module'); +const { spawnSync } = require('node:child_process'); +const { test } = require('node:test'); + +const runtimePath = path.resolve(__dirname, '../../scripts/hooks/hookify-runtime.js'); +const MAX_BYTES = 64 * 1024; +const ruleText = name => '---\nname: ' + name + '\nevent: bash\naction: block\npattern: BLOCK_ME\n---\nLocal policy.\n'; + +function fixture(t, options = {}) { + const root = fs.mkdtempSync(path.join(fs.realpathSync(os.tmpdir()), 'ecc-hookify-race-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const directory = path.join(root, '.claude'); + fs.mkdirSync(directory); + const file = path.join(directory, 'hookify.race.local.md'); + fs.writeFileSync(file, ruleText('trusted')); + const descriptors = new Set(); + const reads = []; + const flags = []; + const io = { + ...fs, + constants: { ...fs.constants, ...(options.noFollow === false ? { O_NOFOLLOW: 0 } : {}) }, + openSync(name, openFlags, ...args) { + if (name === file && options.beforeOpen) options.beforeOpen({ root, directory, file }); + if (options.requireNonblocking) assert.ok(openFlags & fs.constants.O_NONBLOCK); + const fd = fs.openSync(name, openFlags, ...args); + descriptors.add(fd); + flags.push(openFlags); + return fd; + }, + readFileSync(fd, ...args) { + if (descriptors.has(fd) && options.beforeRead) options.beforeRead({ root, directory, file }); + const result = fs.readFileSync(fd, ...args); + if (descriptors.has(fd)) reads.push(Buffer.byteLength(result)); + return result; + }, + readSync(fd, buffer, offset, length, position) { + if (options.beforeRead) options.beforeRead({ root, directory, file }); + const size = fs.readSync(fd, buffer, offset, options.shortReads ? Math.min(7, length) : length, position); + reads.push(size); + return size; + }, + closeSync(fd) { + fs.closeSync(fd); + descriptors.delete(fd); + }, + }; + const copy = new Module(runtimePath, module); + copy.filename = runtimePath; + copy.paths = Module._nodeModulePaths(path.dirname(runtimePath)); + const localRequire = createRequire(runtimePath); + copy.require = name => name === 'fs' ? io : localRequire(name); + copy._compile(fs.readFileSync(runtimePath, 'utf8'), runtimePath); + return { root, directory, file, io, runtime: copy.exports, descriptors, reads, flags }; +} + +function rejected(result, context) { + assert.equal(result.rules.length, 0, 'raced content must not become a rule'); + assert.equal(result.diagnostics.length, 1); + assert.match(result.diagnostics[0], /changed|symbolic|regular|exceeds|EIO/); + assert.equal(context.descriptors.size, 0, 'every opened rule descriptor must close'); +} + +test('growth after metadata validation cannot read over the rule ceiling plus one byte', t => { + let grown = false; + const context = fixture(t, { beforeRead({ file }) { + if (grown) return; + grown = true; + fs.appendFileSync(file, 'x'.repeat(MAX_BYTES * 4)); + } }); + rejected(context.runtime.loadRules(context.root), context); + assert.ok(grown, 'the real file must grow at the read boundary'); + assert.ok(context.reads.reduce((sum, size) => sum + size, 0) <= MAX_BYTES + 1); +}); + +test('same-inode replacement during reading is rejected before parsing', t => { + let replaced = false; + const context = fixture(t, { beforeRead({ file }) { + if (replaced) return; + replaced = true; + fs.writeFileSync(file, ruleText('outside')); + fs.utimesSync(file, new Date(2000), new Date(2000)); + } }); + rejected(context.runtime.loadRules(context.root), context); +}); + +test('permission changes during reading invalidate the pinned rule snapshot', t => { + if (process.platform === 'win32') { t.skip('POSIX file modes required'); return; } + let changed = false; + const context = fixture(t, { beforeRead({ file }) { + if (changed) return; + changed = true; + fs.chmodSync(file, 0o600); + } }); + fs.chmodSync(context.file, 0o644); + rejected(context.runtime.loadRules(context.root), context); +}); + +test('path replacement after opening cannot substitute or retain a stale rule', t => { + let replaced = false; + const context = fixture(t, { beforeRead({ file }) { + if (replaced) return; + replaced = true; + fs.renameSync(file, file + '.original'); + fs.writeFileSync(file, ruleText('outside')); + } }); + rejected(context.runtime.loadRules(context.root), context); +}); + +test('replacement of the rule directory cannot read outside-project rule data', t => { + if (process.platform === 'win32') { t.skip('directory symlink creation requires POSIX'); return; } + let replaced = false; + const context = fixture(t, { beforeOpen({ root, directory }) { + if (replaced) return; + replaced = true; + const outside = path.join(root, 'outside'); + fs.mkdirSync(outside); + fs.writeFileSync(path.join(outside, 'hookify.race.local.md'), ruleText('outside')); + fs.renameSync(directory, path.join(root, 'original')); + fs.symlinkSync(outside, directory); + } }); + rejected(context.runtime.loadRules(context.root), context); + assert.equal(context.reads.length, 0, 'no outside-project bytes may be read'); +}); + +test('platforms without O_NOFOLLOW reject a raced leaf symlink before any read', t => { + if (process.platform === 'win32') { t.skip('symlink creation requires developer mode'); return; } + let replaced = false; + const context = fixture(t, { noFollow: false, beforeOpen({ root, file }) { + if (replaced) return; + replaced = true; + const outside = path.join(root, 'outside.md'); + fs.writeFileSync(outside, ruleText('outside')); + fs.unlinkSync(file); + fs.symlinkSync(outside, file); + } }); + rejected(context.runtime.loadRules(context.root), context); + assert.equal(context.reads.length, 0, 'the fallback must reject before consuming bytes'); +}); + +test('a different real rule directory is rejected before reading its contents', t => { + let replaced = false; + const context = fixture(t, { beforeOpen({ root, directory }) { + if (replaced) return; + replaced = true; + fs.renameSync(directory, path.join(root, 'original')); + fs.mkdirSync(directory); + fs.writeFileSync(path.join(directory, 'hookify.race.local.md'), ruleText('outside')); + } }); + rejected(context.runtime.loadRules(context.root), context); + assert.equal(context.reads.length, 0); +}); + +test('a disappeared rule is diagnosed without opening or reading another file', t => { + const context = fixture(t, { beforeOpen({ file }) { fs.unlinkSync(file); } }); + const result = context.runtime.loadRules(context.root); + assert.deepEqual(result.rules, []); + assert.match(result.diagnostics[0], /ENOENT/); + assert.equal(context.descriptors.size, 0); + assert.equal(context.reads.length, 0); +}); + +test('premature EOF cannot turn an incomplete file snapshot into a rule', t => { + const context = fixture(t); + context.io.readSync = () => 0; + rejected(context.runtime.loadRules(context.root), context); +}); + +test('short descriptor reads load a stable rule exactly at the byte limit', t => { + const context = fixture(t, { shortReads: true }); + const text = ruleText('trusted'); + fs.writeFileSync(context.file, text + 'x'.repeat(MAX_BYTES - Buffer.byteLength(text))); + const result = context.runtime.loadRules(context.root); + assert.deepEqual(result.rules.map(rule => rule.name), ['trusted']); + assert.deepEqual(result.diagnostics, []); + assert.equal(context.reads.reduce((sum, size) => sum + size, 0), MAX_BYTES); + assert.equal(context.descriptors.size, 0); +}); + +test('read failures close the descriptor and report a bounded diagnostic', t => { + const context = fixture(t, { beforeRead() { + const error = new Error('EIO injected read failure'); + error.code = 'EIO'; + throw error; + } }); + rejected(context.runtime.loadRules(context.root), context); +}); + +test('a FIFO is opened nonblocking and rejected without consuming bytes', t => { + if (process.platform === 'win32') { t.skip('POSIX FIFO required'); return; } + const context = fixture(t, { requireNonblocking: true }); + fs.unlinkSync(context.file); + assert.equal(spawnSync('mkfifo', [context.file]).status, 0); + rejected(context.runtime.loadRules(context.root), context); + assert.ok(context.flags[0] & fs.constants.O_NONBLOCK); + assert.equal(context.reads.length, 0); +});