From c1ac0acfab948a48e26e5b8c5220c94e442f4000 Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Sun, 26 Jul 2026 05:47:24 -0400 Subject: [PATCH] fix: harden Hookify review follow-ups --- hooks/hooks.json | 1 + scripts/hooks/hookify-loader.js | 30 ++++++-- scripts/hooks/hookify-regex-worker.js | 20 ++++- scripts/hooks/hookify-runner.js | 9 ++- scripts/hooks/posttooluse-dispatcher.js | 5 +- skills/hookify-rules/SKILL.md | 15 +++- tests/docs/hookify-runtime-docs.test.js | 9 +++ tests/hooks/hookify-engine.test.js | 40 +++++++++- tests/hooks/hookify-loader.test.js | 87 +++++++++++++++++++++- tests/hooks/hookify-runner.test.js | 9 ++- tests/hooks/posttooluse-dispatcher.test.js | 41 ++++++++++ 11 files changed, 249 insertions(+), 17 deletions(-) diff --git a/hooks/hooks.json b/hooks/hooks.json index 6760ec777..12e26ddb0 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -200,6 +200,7 @@ ], "Stop": [ { + "matcher": "*", "hooks": [ { "type": "command", diff --git a/scripts/hooks/hookify-loader.js b/scripts/hooks/hookify-loader.js index 64261f58a..8578a84a7 100644 --- a/scripts/hooks/hookify-loader.js +++ b/scripts/hooks/hookify-loader.js @@ -50,6 +50,7 @@ const EVENT_FIELDS = Object.freeze({ stop: new Set(['content']), all: new Set(['command', 'file_path', 'new_text', 'old_text', 'content', 'user_prompt']), }); +const MODEL_FACING_FORMAT_CONTROL = /[\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069\uFEFF]/u; function diagnostic(code, fileName, detail) { const label = fileName ? ` ${fileName}` : ''; @@ -73,6 +74,10 @@ function hasUnsafeControlCharacters(value, allowNewlines = false) { return false; } +function hasUnsafeModelFacingCharacters(value) { + return MODEL_FACING_FORMAT_CONTROL.test(value); +} + function parseQuotedScalar(rawValue) { if (rawValue.startsWith('"')) { if (!rawValue.endsWith('"')) { @@ -118,6 +123,7 @@ function parseFrontmatter(frontmatterText) { const lines = frontmatterText.split('\n'); let conditions = null; let currentCondition = null; + let insideConditions = false; for (const originalLine of lines) { const line = originalLine.endsWith('\r') @@ -136,15 +142,17 @@ function parseFrontmatter(frontmatterText) { conditions = []; result.conditions = conditions; currentCondition = null; + insideConditions = true; } else { setUnique(result, key, parseScalar(rawValue)); currentCondition = null; + insideConditions = false; } continue; } const listStart = line.match(/^ {2}- ([a-z_]+):(?:[ \t]*(.*))?$/); - if (listStart && conditions) { + if (listStart && insideConditions && conditions) { const [, key, rawValue = ''] = listStart; if (!CONDITION_FIELDS.has(key)) throw new Error(`unknown condition field ${key}`); currentCondition = {}; @@ -154,7 +162,7 @@ function parseFrontmatter(frontmatterText) { } const continuation = line.match(/^ {4}([a-z_]+):(?:[ \t]*(.*))?$/); - if (continuation && currentCondition) { + if (continuation && insideConditions && currentCondition) { const [, key, rawValue = ''] = continuation; if (!CONDITION_FIELDS.has(key)) throw new Error(`unknown condition field ${key}`); setUnique(currentCondition, key, parseScalar(rawValue)); @@ -187,6 +195,9 @@ function validateString(value, field, options = {}) { if (hasUnsafeControlCharacters(value, options.allowNewlines === true)) { throw new Error(`${field} contains control characters`); } + if (options.modelFacing === true && hasUnsafeModelFacingCharacters(value)) { + throw new Error(`${field} contains unsafe invisible or bidirectional characters`); + } if (options.maxLength && value.length > options.maxLength) { throw new Error(`${field} exceeds its length limit`); } @@ -287,7 +298,10 @@ function validateRule(frontmatter, message, source) { conditions = frontmatter.conditions.map(condition => validateCondition(condition, event)); } - validateString(message, 'message', { allowNewlines: true }); + validateString(message, 'message', { + allowNewlines: true, + modelFacing: true, + }); if (Buffer.byteLength(message, 'utf8') > LIMITS.maxMessageBytes) { throw new Error('message exceeds its byte limit'); } @@ -325,6 +339,10 @@ function readFileBounded(fileDescriptor, maxBytes) { }; } +function isRuleFileReadError(error) { + return ['EACCES', 'EIO', 'EISDIR', 'EPERM'].includes(error?.code); +} + function loadRuleFile({ claudeDir, fileName, @@ -469,10 +487,12 @@ function loadRuleFile({ diagnostic: null, bytesRead: consumedBytes, }; - } catch { + } catch (error) { return { rule: null, - diagnostic: diagnostic('HOOKIFY_RULE_INVALID', fileName, 'invalid rule schema or encoding'), + diagnostic: isRuleFileReadError(error) + ? diagnostic('HOOKIFY_RULE_READ_FAILED', fileName, 'could not read rule file') + : diagnostic('HOOKIFY_RULE_INVALID', fileName, 'invalid rule schema or encoding'), bytesRead: consumedBytes, }; } finally { diff --git a/scripts/hooks/hookify-regex-worker.js b/scripts/hooks/hookify-regex-worker.js index 59b10ae73..5bf1c256c 100644 --- a/scripts/hooks/hookify-regex-worker.js +++ b/scripts/hooks/hookify-regex-worker.js @@ -8,6 +8,7 @@ const { isMainThread, workerData } = require('worker_threads'); const HEADER_BYTES = Int32Array.BYTES_PER_ELEMENT * 2; +const MAX_DIAGNOSTICS = 32; function safeSource(value) { return typeof value === 'string' && /^hookify\.[A-Za-z0-9._-]+\.local\.md$/.test(value) @@ -34,7 +35,7 @@ function evaluateCondition(condition, values, diagnostics, source) { try { return new RegExp(condition.pattern, 'i').test(value); } catch { - diagnostics.push({ + addDiagnostic(diagnostics, { code: 'HOOKIFY_REGEX_INVALID', message: `Hookify skipped ${safeSource(source)}: invalid regular expression.`, }); @@ -45,6 +46,22 @@ function evaluateCondition(condition, values, diagnostics, source) { } } +function addDiagnostic(diagnostics, diagnostic) { + if (diagnostics.length < MAX_DIAGNOSTICS) { + diagnostics.push(diagnostic); + return; + } + if ( + diagnostics.length === MAX_DIAGNOSTICS && + !diagnostics.some(item => item.code === 'HOOKIFY_DIAGNOSTICS_TRUNCATED') + ) { + diagnostics.push({ + code: 'HOOKIFY_DIAGNOSTICS_TRUNCATED', + message: 'Hookify skipped additional diagnostics because the diagnostic limit was reached.', + }); + } +} + function evaluateTasks(tasks, values = {}) { const matchedIndexes = []; const diagnostics = []; @@ -105,5 +122,6 @@ if (!isMainThread) { module.exports = { evaluateTasks, + MAX_DIAGNOSTICS, writeResult, }; diff --git a/scripts/hooks/hookify-runner.js b/scripts/hooks/hookify-runner.js index feea80c32..6b2f9d8cb 100644 --- a/scripts/hooks/hookify-runner.js +++ b/scripts/hooks/hookify-runner.js @@ -20,6 +20,9 @@ const LIMITS = Object.freeze({ ...LOADER_LIMITS, maxInputBytes: 256 * 1024, maxOutputBytes: 8192, + maxContextBytes: 7000, + maxContextBytesWithBlock: 3000, + maxBlockReasonBytes: 3800, regexTimeoutMs: 250, }); const EVENTS = new Set([ @@ -216,9 +219,11 @@ function buildOutput(eventName, matches, diagnostics) { ...warnings.map(formatRule), ...formatDiagnostics(diagnostics), ]; - const contextLimit = blocking.length > 0 ? 3000 : 7000; + const contextLimit = blocking.length > 0 + ? LIMITS.maxContextBytesWithBlock + : LIMITS.maxContextBytes; const additionalContext = joinBounded(contextItems, contextLimit); - const blockReason = joinBounded(blocking.map(formatRule), 3800); + const blockReason = joinBounded(blocking.map(formatRule), LIMITS.maxBlockReasonBytes); if (blocking.length === 0) { return contextOutput(eventName, additionalContext); diff --git a/scripts/hooks/posttooluse-dispatcher.js b/scripts/hooks/posttooluse-dispatcher.js index 8e40019b8..a08c8bce9 100644 --- a/scripts/hooks/posttooluse-dispatcher.js +++ b/scripts/hooks/posttooluse-dispatcher.js @@ -36,10 +36,11 @@ const SYNC_HOOKS = [ matcher: '*', profiles: 'minimal,standard,strict', script: 'scripts/hooks/hookify-runner.js', - run(raw) { + run(raw, context = {}) { const result = runHookify(raw, { expectedEvent: 'PostToolUse', - projectRoot: process.cwd() + projectRoot: process.cwd(), + truncated: context.truncated === true }); return result.stdout === '{}' ? { ...result, stdout: '' } : result; } diff --git a/skills/hookify-rules/SKILL.md b/skills/hookify-rules/SKILL.md index cfa0aed64..86e0e7678 100644 --- a/skills/hookify-rules/SKILL.md +++ b/skills/hookify-rules/SKILL.md @@ -5,6 +5,12 @@ description: This skill should be used when the user asks to create a hookify ru # Writing Hookify Rules +## When to Activate + +Use this skill when creating, reviewing, or debugging project-local Hookify +rules, including `.claude/hookify.*.local.md` syntax, event selection, +condition fields, rule limits, and warn/block behavior. + ## Overview Hookify rules are Markdown files with YAML frontmatter that define patterns to @@ -134,7 +140,14 @@ Match Claude Code's submitted `prompt` through the rule field `user_prompt`. ### Testing ```bash -node -e "console.log(new RegExp('your_pattern', 'i').test('test text'))" +node - <<'NODE' +const readline = require('node:readline/promises'); +const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); +const pattern = await rl.question('pattern: '); +const text = await rl.question('text: '); +rl.close(); +console.log(new RegExp(pattern, 'i').test(text)); +NODE ``` Regex evaluation runs in a resource-limited worker with one hard total diff --git a/tests/docs/hookify-runtime-docs.test.js b/tests/docs/hookify-runtime-docs.test.js index db2fc74ed..12b0e62d1 100644 --- a/tests/docs/hookify-runtime-docs.test.js +++ b/tests/docs/hookify-runtime-docs.test.js @@ -81,6 +81,15 @@ function runTests() { assert.ok(skill.includes('project `.claude/` directory')); })) passed++; else failed++; + if (test('skill has activation guidance and a regex test command that does not embed user text in code', () => { + const skill = read('skills/hookify-rules/SKILL.md'); + + assert.ok(skill.includes('## When to Activate')); + assert.ok(skill.includes("readline.createInterface")); + assert.ok(skill.includes("new RegExp(pattern, 'i')")); + assert.ok(!skill.includes("new RegExp('your_pattern'")); + })) passed++; else failed++; + if (test('list and configure commands state that malformed rules are skipped rather than enforced', () => { const list = read('commands/hookify-list.md'); const configure = read('commands/hookify-configure.md'); diff --git a/tests/hooks/hookify-engine.test.js b/tests/hooks/hookify-engine.test.js index 62717b0ae..2a16ec942 100644 --- a/tests/hooks/hookify-engine.test.js +++ b/tests/hooks/hookify-engine.test.js @@ -13,6 +13,7 @@ const { } = require('../../scripts/hooks/hookify-engine'); const { evaluateTasks, + MAX_DIAGNOSTICS, writeResult, } = require('../../scripts/hooks/hookify-regex-worker'); @@ -310,6 +311,29 @@ function runTests() { assert.ok(!result.diagnostics[0].message.includes('anything')); })) passed++; else failed++; + if (test('caps invalid-regex diagnostics without discarding valid matches', () => { + const input = { + hook_event_name: 'PreToolUse', + tool_name: 'Bash', + tool_input: { command: 'npm publish' }, + }; + const invalidRules = Array.from( + { length: MAX_DIAGNOSTICS + 20 }, + (_value, index) => rule({ + name: `invalid-${index}`, + source: `hookify.invalid-${index}.local.md`, + conditions: [{ field: 'command', operator: 'regex_match', pattern: '(' }], + }) + ); + const validRule = rule({ name: 'valid-regex' }); + + const result = evaluateRules([...invalidRules, validRule], input); + + assert.deepStrictEqual(result.matches.map(item => item.name), ['valid-regex']); + assert.ok(result.diagnostics.length <= MAX_DIAGNOSTICS + 1); + assert.ok(result.diagnostics.some(item => item.code === 'HOOKIFY_DIAGNOSTICS_TRUNCATED')); + })) passed++; else failed++; + if (test('a regex timeout preserves an unrelated literal-only block match', () => { const input = { hook_event_name: 'PreToolUse', @@ -451,14 +475,24 @@ function runTests() { assert.ok(evaluated.diagnostics[0].message.includes('a Hookify rule')); const shared = new SharedArrayBuffer(512); + writeResult(shared, { + matchedIndexes: [2], + diagnostics: [], + }); + let header = new Int32Array(shared, 0, 2); + assert.strictEqual(Atomics.load(header, 0), 1); + let bytes = new Uint8Array(shared, 8, Atomics.load(header, 1)); + let result = JSON.parse(Buffer.from(bytes).toString('utf8')); + assert.deepStrictEqual(result.matchedIndexes, [2]); + writeResult(shared, { matchedIndexes: [], diagnostics: [{ code: 'X', message: 'x'.repeat(1000) }], }); - const header = new Int32Array(shared, 0, 2); + header = new Int32Array(shared, 0, 2); assert.strictEqual(Atomics.load(header, 0), 2); - const bytes = new Uint8Array(shared, 8, Atomics.load(header, 1)); - const result = JSON.parse(Buffer.from(bytes).toString('utf8')); + bytes = new Uint8Array(shared, 8, Atomics.load(header, 1)); + result = JSON.parse(Buffer.from(bytes).toString('utf8')); assert.strictEqual(result.diagnostics[0].code, 'HOOKIFY_REGEX_WORKER_FAILED'); })) passed++; else failed++; diff --git a/tests/hooks/hookify-loader.test.js b/tests/hooks/hookify-loader.test.js index 43523ebb5..280b17c55 100644 --- a/tests/hooks/hookify-loader.test.js +++ b/tests/hooks/hookify-loader.test.js @@ -191,6 +191,39 @@ function runTests() { }); })) passed++; else failed++; + if (test('rejects condition list items after the conditions block is closed', () => { + assert.throws( + () => parseFrontmatter([ + 'name: misplaced-condition', + 'enabled: true', + 'event: file', + 'conditions:', + ' - field: file_path', + ' operator: contains', + ' pattern: src/', + 'action: block', + ' - field: content', + ' operator: contains', + ' pattern: API_KEY', + ].join('\n')), + /unsupported YAML structure/ + ); + })) passed++; else failed++; + + if (test('rejects model-facing messages with bidi or invisible controls', () => { + for (const message of ['Looks safe\u202E.gnirts', 'Invisible\uFEFFjoiner']) { + assert.throws( + () => validateRule({ + name: 'bidi-message', + enabled: true, + event: 'bash', + pattern: 'deploy', + }, message, 'hookify.bidi-message.local.md'), + /message contains unsafe invisible or bidirectional characters/ + ); + } + })) passed++; else failed++; + if (test('rejects traversal names, symlinked files, and a symlinked .claude directory', () => { withProject(({ projectRoot, claudeDir }) => { const outside = path.join(projectRoot, 'outside.md'); @@ -323,9 +356,20 @@ function runTests() { const result = loadRules({ projectRoot, event: 'bash' }); - assert.ok(result.rules.length <= LIMITS.maxRuleFiles); + assert.strictEqual(result.rules.length, LIMITS.maxRuleFiles); assert.ok(result.totalBytes <= LIMITS.maxTotalBytes); - assert.ok(result.diagnostics.some(item => item.code === 'HOOKIFY_RULE_LIMIT')); + assert.ok(result.diagnostics.some(item => + item.code === 'HOOKIFY_RULE_LIMIT' && + item.message.includes('rule or directory entry count limit reached') + )); + assert.ok(result.diagnostics.some(item => + item.code === 'HOOKIFY_RULE_LIMIT' && + item.message.includes('file exceeds byte limit') + )); + assert.ok(result.diagnostics.some(item => + item.code === 'HOOKIFY_RULE_INVALID' && + item.message.includes('pattern-too-long') + )); }); })) passed++; else failed++; @@ -348,6 +392,45 @@ function runTests() { }); })) passed++; else failed++; + if (test('reports descriptor read failures separately from schema errors', () => { + withProject(({ claudeDir }) => { + const originalReadSync = fs.readSync; + let readAttempted = false; + writeRule( + claudeDir, + 'hookify.read-fails.local.md', + [ + 'name: read-fails', + 'enabled: true', + 'event: bash', + 'pattern: safe', + ].join('\n') + ); + + try { + fs.readSync = function readSyncFails() { + readAttempted = true; + const error = new Error('sensitive device detail'); + error.code = 'EIO'; + throw error; + }; + const result = loadRuleFile({ + claudeDir, + fileName: 'hookify.read-fails.local.md', + remainingTotalBytes: LIMITS.maxTotalBytes, + expectedRealDirectory: fs.realpathSync(claudeDir), + }); + + assert.strictEqual(readAttempted, true); + assert.strictEqual(result.rule, null); + assert.strictEqual(result.diagnostic.code, 'HOOKIFY_RULE_READ_FAILED'); + assert.ok(!result.diagnostic.message.includes('sensitive device detail')); + } finally { + fs.readSync = originalReadSync; + } + }); + })) passed++; else failed++; + if (test('counts malformed file reads against the hard total byte budget', () => { withProject(({ projectRoot, claudeDir }) => { for (let index = 0; index < 12; index += 1) { diff --git a/tests/hooks/hookify-runner.test.js b/tests/hooks/hookify-runner.test.js index 69711476d..33eff57f0 100644 --- a/tests/hooks/hookify-runner.test.js +++ b/tests/hooks/hookify-runner.test.js @@ -631,10 +631,17 @@ function runTests() { prompt: '', }], ]; - for (const [hookId, _eventName, payload] of cases) { + for (const [hookId, eventName, payload] of cases) { const output = run(JSON.stringify(payload), { projectRoot, hookId }); assert.strictEqual(output.exitCode, 0); JSON.parse(output.stdout); + + const mismatched = run(JSON.stringify({ + ...payload, + hook_event_name: eventName === 'Stop' ? 'PreToolUse' : 'Stop', + }), { projectRoot, hookId }); + const serialized = JSON.stringify(JSON.parse(mismatched.stdout)); + assert.ok(serialized.includes('hook event did not match')); } const invalid = run(Buffer.from('not accepted'), { projectRoot, diff --git a/tests/hooks/posttooluse-dispatcher.test.js b/tests/hooks/posttooluse-dispatcher.test.js index c4ca4b510..9bf6d737c 100644 --- a/tests/hooks/posttooluse-dispatcher.test.js +++ b/tests/hooks/posttooluse-dispatcher.test.js @@ -444,6 +444,21 @@ function runTests() { }); assert.strictEqual(blocked.warning, ''); + const multiBlocked = mergeHookStdout([ + { + id: 'post:test:block-one', + stdout: JSON.stringify({ decision: 'block', reason: 'First reason.' }), + }, + { + id: 'post:test:block-two', + stdout: JSON.stringify({ decision: 'block', reason: 'Second reason.' }), + }, + ]); + assert.deepStrictEqual(JSON.parse(multiBlocked.stdout), { + decision: 'block', + reason: 'First reason.\n\nSecond reason.', + }); + const conflicting = mergeHookStdout([ { id: 'post:test:raw', stdout: 'plain output' }, { id: 'post:test:ctx', stdout: envelope('kept warning') } @@ -456,6 +471,32 @@ function runTests() { passed++; else failed++; + if ( + test('post:hookify receives dispatcher truncation state and fails open without pass-through', () => { + const { SYNC_HOOKS, runHooks } = require(dispatcherPath); + const hookify = SYNC_HOOKS.find(hook => hook.id === 'post:hookify'); + const result = runHooks( + JSON.stringify({ + hook_event_name: 'PostToolUse', + tool_name: 'Bash', + tool_input: { command: 'true' }, + tool_response: {}, + }), + [hookify], + { truncated: true, toolName: 'Bash' } + ); + const parsed = JSON.parse(result.stdout); + assert.ok( + parsed.hookSpecificOutput.additionalContext.includes( + 'input exceeded the byte limit' + ) + ); + assert.strictEqual(result.stderr, ''); + }) + ) + passed++; + else failed++; + if ( test('requiring the dispatcher module never dispatches; hooks.json calls cli()', () => { const raw = JSON.stringify({