prefilter() had zero logging -- matcher regex failures, tool-name
mismatches, CEL compile errors, and CEL evaluating to false were all
completely silent, making a misconfigured hook impossible to debug.
A user's PreToolUse script hook silently failed to block an
unauthorized file write with no way to determine why.
- Added slog.Debug/Warn logging throughout prefilter() and the
dispatch chain: hooks.prefilter.considered, matcher_checked,
condition_evaluated, error, and hooks.dispatch.decision -- covering
matcher match/miss, CEL compile/eval result, and final decision
with reasoning.
- console.log/console.error output from script hooks (captured via
the existing goja sandbox stdout buffer, previously discarded after
execution) now surfaces via slog.Debug and persists into
HookExecution.ConsoleOutput / Metadata["console_output"] in the
audit record, using the existing metadata JSON column (no
migration needed).
- Fail-closed, narrowly scoped: when a hook's matcher successfully
matches a tool call but CEL condition or script execution then
errors, the dispatcher now blocks that specific tool call
(previously: silently treated as non-match, call proceeded
allowed). Hooks that legitimately don't match remain unaffected
pass-through -- this does not turn one broken hook into a global
outage, it only blocks the specific gated action that couldn't be
confidently evaluated.
Added tests: matched-then-CEL-errors blocks the call and never
invokes the handler; matcher-doesn't-match proceeds normally
unaffected (confirms narrow scope); console.log output is captured
in the audit record.
Co-authored-by: Bruno Clermont <bruno.clermont@gmail.com>
Code-review findings from Wave 1 audit (commits 097776c5..f76a681e).
Critical (security):
- C1 internal/gateway/methods/hooks.go: parseHookConfigParams strips
caller-supplied source/id/created_by/version. Without this a tenant
admin could POST {"source":"builtin"} and escalate their UI hook into
the dispatcher's builtin capability tier (which is allowed to mutate
event input). Default Source resolves to "ui" via Validate.
- C2 internal/gateway/methods/hooks.go: handleUpdate also strips source +
created_by from the patch map. Same forge surface via PATCH instead of
POST.
High:
- H1 internal/gateway/methods/hooks.go: Update + Delete handlers wrap
hooks.ErrBuiltinReadOnly into the i18n key MsgHookBuiltinReadOnly so
users get a localized "builtin hooks are read-only" message instead of
the raw English sentinel string.
- H3 internal/hooks/handlers/script.go: bound tenantSems map with
opportunistic sweep — once map crosses 64 entries, idle slots
(no in-flight grants AND last-used > 1h ago) are reclaimed under the
same lock the acquire path already takes. Goroutine-free; common-path
cost unchanged.
Medium:
- M1 internal/hooks/dispatcher.go: builtinAllowlistLookup uses
atomic.Pointer instead of plain package var. Fixes the parallel-test
race where one test installs a lookup while another reads.
- M3 ui/web/index.html: drop http: https: from CSP connect-src — keep
'self' + ws:/wss: only. Same-origin XHR/WS still works; cross-origin
HTTP is no longer wildcard-permitted.
Tests:
- internal/gateway/methods/hooks_source_strip_test.go pins the C1+C2
strip behavior at the parse layer with a regression case that posts
every forge field and asserts each is zeroed.
ntduc