From cb4a72bf1c14cd754e545b985a38a53938dda656 Mon Sep 17 00:00:00 2001 From: Benson LUO Date: Mon, 26 Oct 2026 13:37:36 +0800 Subject: [PATCH] fix(hooks): reject unsupported command hook args (#2398) * fix(hooks): reject unsupported command hook args * fix(settings): preserve forbidden hook args diagnostics --------- Co-authored-by: jatmn --- src/schemas/hooks.test.ts | 261 +++++++++++++++++++++++++++++++ src/schemas/hooks.ts | 22 +++ src/utils/settings/validation.ts | 4 +- 3 files changed, 286 insertions(+), 1 deletion(-) create mode 100644 src/schemas/hooks.test.ts diff --git a/src/schemas/hooks.test.ts b/src/schemas/hooks.test.ts new file mode 100644 index 000000000..5a706b3a0 --- /dev/null +++ b/src/schemas/hooks.test.ts @@ -0,0 +1,261 @@ +import { describe, expect, test } from 'bun:test' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { + BashCommandHook, + HookCommandSchema, + HooksSchema, +} from './hooks.js' +import { PluginHooksSchema } from '../utils/plugins/schemas.js' +import { SettingsSchema } from '../utils/settings/types.js' +import { parseSettingsFile } from '../utils/settings/settings.js' +import { validateSettingsFileContent } from '../utils/settings/validation.js' + +const validCommandHook = { + type: 'command', + command: "sh -c 'printf HANDLER_RAN; cat'", + timeout: 30, +} as const + +/** Command hooks do not support a separate `args` field (gh-2304). */ +describe('command hook schema rejects a supplied args field', () => { + test('rejects an args array instead of silently stripping it', () => { + const result = HookCommandSchema().safeParse({ + type: 'command', + command: 'sh', + args: ['-c', 'printf HANDLER_RAN; cat'], + }) + expect(result.success).toBe(false) + if (!result.success) { + const issue = result.error.issues.find(i => i.path.includes('args')) + expect(issue).toBeDefined() + // The error must be actionable: it names the unsupported field and + // says where the complete invocation belongs. + expect(issue?.message).toContain('args') + expect(issue?.message).toContain('command') + } + }) + + test('rejects every JSON-expressible non-undefined args value', () => { + const supplied: unknown[] = [ + null, + [], + ['--flag'], + '', + 'value', + 0, + false, + {}, + ] + for (const args of supplied) { + const result = HookCommandSchema().safeParse({ + type: 'command', + command: 'sh', + args, + }) + expect(result.success).toBe(false) + } + }) + + test('accepts an absent args key and an explicit undefined args', () => { + // JSON cannot express undefined: an absent key (or an explicit undefined + // in programmatically constructed objects) means the field was not + // supplied and must keep parsing. + const absent = HookCommandSchema().safeParse({ + type: 'command', + command: 'sh -c "cat"', + }) + expect(absent.success).toBe(true) + + const explicitUndefined = HookCommandSchema().safeParse({ + type: 'command', + command: 'sh -c "cat"', + args: undefined, + }) + expect(explicitUndefined.success).toBe(true) + }) + + test('valid command hooks parse unchanged, including optional fields', () => { + const result = HookCommandSchema().safeParse(validCommandHook) + expect(result.success).toBe(true) + if (result.success) { + expect(result.data).toEqual(validCommandHook) + } + }) + + test('unknown fields other than args are still tolerated for compatibility', () => { + const result = HookCommandSchema().safeParse({ + type: 'command', + command: 'echo ok', + description: 'author-facing note, not executed', + }) + expect(result.success).toBe(true) + if (result.success) { + expect((result.data as Record).description).toBeUndefined() + expect((result.data as BashCommandHook).command).toBe('echo ok') + } + }) + + test('non-command hook types keep parsing via the discriminated union', () => { + const prompt = HookCommandSchema().safeParse({ + type: 'prompt', + prompt: 'Summarize: $ARGUMENTS', + }) + expect(prompt.success).toBe(true) + + // prompt hooks have no execution-affecting args contract either; a + // supplied args on them is not execution-relevant and stays out of scope. + const http = HookCommandSchema().safeParse({ + type: 'http', + url: 'https://example.invalid/hook', + }) + expect(http.success).toBe(true) + }) +}) + +describe('settings diagnostics for unsupported command hook args', () => { + const settingsContent = (hook: Record) => JSON.stringify({ + hooks: { UserPromptSubmit: [{ hooks: [hook] }] }, + }) + + test('settings loading preserves the explanation and rejects the file', () => { + const fixtureDir = mkdtempSync(join(tmpdir(), 'hook-args-diagnostic-')) + const settingsPath = join(fixtureDir, 'settings.json') + try { + writeFileSync(settingsPath, settingsContent({ + type: 'command', command: 'sh', args: ['-c', 'cat'], + })) + const result = parseSettingsFile(settingsPath) + expect(result.settings).toBeNull() + expect(result.errors).toHaveLength(1) + expect(result.errors[0]?.path).toBe('hooks.UserPromptSubmit.0.hooks.0.args') + expect(result.errors[0]?.message).toContain('do not support an "args" field') + expect(result.errors[0]?.message).toContain('complete invocation in "command"') + } finally { + rmSync(fixtureDir, { recursive: true, force: true }) + } + }) + + test('settings edit validation explains every supplied JSON args value', () => { + for (const args of [null, [], ['--flag'], '', 'value', 0, false, {}]) { + const result = validateSettingsFileContent(settingsContent({ + type: 'command', command: 'sh', args, + })) + expect(result.isValid).toBe(false) + if (!result.isValid) { + expect(result.error).toContain('hooks.UserPromptSubmit.0.hooks.0.args') + expect(result.error).toContain('do not support an "args" field') + expect(result.error).toContain('complete invocation in "command"') + } + } + }) + + test('a complete command remains valid through both settings paths', () => { + const fixtureDir = mkdtempSync(join(tmpdir(), 'hook-command-valid-')) + const settingsPath = join(fixtureDir, 'settings.json') + const content = settingsContent(validCommandHook) + try { + writeFileSync(settingsPath, content) + const result = parseSettingsFile(settingsPath) + expect(result.errors).toEqual([]) + expect(result.settings?.hooks?.UserPromptSubmit?.[0]?.hooks[0]) + .toEqual(validCommandHook) + expect(validateSettingsFileContent(content)).toEqual({ isValid: true }) + } finally { + rmSync(fixtureDir, { recursive: true, force: true }) + } + }) +}) + +describe('args rejection propagates through every hook entry point', () => { + test('HooksSchema (settings and frontmatter) rejects the hook', () => { + const result = HooksSchema().safeParse({ + UserPromptSubmit: [ + { + hooks: [ + { + type: 'command', + command: 'sh', + args: ['-c', 'cat'], + }, + ], + }, + ], + }) + expect(result.success).toBe(false) + }) + + test('SettingsSchema reports the hook so the settings file fails visibly', () => { + const result = SettingsSchema().safeParse({ + hooks: { + UserPromptSubmit: [ + { + hooks: [ + { + type: 'command', + command: 'sh', + args: ['-c', 'cat'], + }, + ], + }, + ], + }, + }) + expect(result.success).toBe(false) + if (!result.success) { + const messages = result.error.issues.map(i => i.message).join(' ') + expect(messages).toContain('args') + } + + // The same file without args still parses and keeps the hook. + const ok = SettingsSchema().safeParse({ + hooks: { + UserPromptSubmit: [ + { + hooks: [{ type: 'command', command: 'cat', timeout: 5 }], + }, + ], + }, + }) + expect(ok.success).toBe(true) + }) + + test('PluginHooksSchema rejects the hook and keeps valid plugin hooks', () => { + const bad = PluginHooksSchema().safeParse({ + description: 'probe plugin', + hooks: { + UserPromptSubmit: [ + { + hooks: [ + { + type: 'command', + command: 'sh', + args: ['-c', 'printf HANDLER_RAN; cat'], + }, + ], + }, + ], + }, + }) + expect(bad.success).toBe(false) + + const good = PluginHooksSchema().safeParse({ + description: 'probe plugin', + hooks: { + UserPromptSubmit: [ + { + hooks: [validCommandHook], + }, + ], + }, + }) + expect(good.success).toBe(true) + if (good.success) { + expect(good.data.hooks.UserPromptSubmit?.[0]?.hooks[0]).toEqual({ + ...validCommandHook, + }) + } + }) +}) diff --git a/src/schemas/hooks.ts b/src/schemas/hooks.ts index c927bb471..7920057ae 100644 --- a/src/schemas/hooks.ts +++ b/src/schemas/hooks.ts @@ -32,6 +32,28 @@ function buildHookSchemas() { const BashCommandHookSchema = z.object({ type: z.literal('command').describe('Shell command hook type'), command: z.string().describe('Shell command to execute'), + // Command hooks have no separate `args` field: the executor spawns the + // `command` string through a shell with an empty argument array. zod's + // default key stripping would silently drop a supplied `args`, turning + // e.g. { command: "sh", args: ["-c", handler] } into a bare `sh` that + // executes its JSON stdin as shell input (gh-2304). Reject the field + // with an actionable error instead, while all other unknown keys keep + // being tolerated for forward compatibility. JSON cannot express + // undefined, so `.optional()` only admits a programmatically-constructed + // explicit `undefined`, which means "not supplied" — every JSON-parsed + // value (null, array, string, …) is rejected. + args: z + .never({ + error: + 'Command hooks do not support an "args" field. Put the complete ' + + 'invocation in "command" (e.g. "command": "sh -c \'printf HANDLER_RAN; cat\'"); ' + + 'a supplied "args" would otherwise be silently discarded and a ' + + 'different command would run.', + }) + .optional() + .describe( + 'Not supported: put the complete invocation in "command" instead', + ), if: IfConditionSchema(), shell: z .enum(SHELL_TYPES) diff --git a/src/utils/settings/validation.ts b/src/utils/settings/validation.ts index 406401f5e..3e542eca1 100644 --- a/src/utils/settings/validation.ts +++ b/src/utils/settings/validation.ts @@ -142,7 +142,9 @@ export function formatZodError( if (isInvalidValueIssue(issue)) { expected = enumValues?.map(v => `"${v}"`).join(', ') message = `Invalid value. Expected one of: ${expected}` - } else if (isInvalidTypeIssue(issue)) { + } else if (isInvalidTypeIssue(issue) && issue.expected !== 'never') { + // Forbidden fields use the schema's explanation of what to remove or + // replace; treating `never` as an ordinary type loses that guidance. const receivedType = extractReceivedFromMessage(issue.message) ?? getReceivedType(issue.input)