feat(hooks): Add HTTP Hook, Function Hook and Async Hook support#2827
feat(hooks): Add HTTP Hook, Function Hook and Async Hook support#2827DennisYu07 merged 19 commits intomainfrom
Conversation
Code Review: Hook System Extension (PR #2827)📋 Review SummaryThis PR implements a comprehensive hook system extension with three new hook types (HTTP, Function, and Async command hooks), along with session-level hook management. The implementation is well-structured with good test coverage, but there are several security, type safety, and architectural concerns that should be addressed before merging. 🔍 General FeedbackPositive aspects:
Areas of concern:
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
Recommendation: Address the critical security issues (especially SSRF protection and IPv6 validation) before merging. The high-priority issues should also be resolved to ensure reliability and prevent potential memory or race condition problems. |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…types - Change validated hook types from ['command', 'plugin'] to ['command', 'http', 'function'] - Add validation for HTTP hooks requiring url field - Add validation for function hooks requiring callback field - Add comprehensive test coverage for all hook type validations Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Allow 127.0.0.0/8 (loopback) for local dev hooks - Allow localhost hostname for local dev hooks - Allow ::1 (IPv6 loopback) for local dev hooks - Add 100.64.0.0/10 (CGNAT) to blocked ranges (RFC 6598) - Update tests to match Claude Code's ssrfGuard.ts behavior This fixes HTTP hooks failing to connect to local dev servers. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Add CRLF/NUL sanitization for env var interpolation (header injection) - Implement combined abort signal (external signal + timeout) - Upgrade SSRF protection to DNS-level with ssrfGuard - Allow loopback (127.0.0.0/8, ::1) for local dev hooks - Block CGNAT (100.64.0.0/10) and IPv6 private ranges - Increase default HTTP hook timeout to 10 minutes - Fix VS Code hooks schema to support http type - Add url, headers, allowedEnvVars, async, once, statusMessage, shell fields - Note: "function" type is SDK-only (callback cannot be serialized to JSON)
27ec449 to
0799e97
Compare
…nd matcher support - Add MessagesProvider for automatic conversation history passing to function hooks - Add FunctionHookContext with messages, toolUseID, and signal - Add skillRoot support for skill-scoped session hooks - Add shell parameter support for command hooks (bash/powershell) - Add regex matcher support for hook pattern matching - Add statusMessage to CommandHookConfig - Change default function hook timeout from 60s to 5s - Add comprehensive unit tests for all new features Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
This PR adds HTTP, function, and async hook types with clean separation of concerns and solid security measures. The architecture is well-designed, but there are several issues worth addressing.
Issues
-
Telemetry always reports hook type as 'command' —
getHookTypeFromResult()inhookEventHandler.tsalways returns'command'regardless of actual hook type, andHookCallEvent.hook_typeconstrains to only'command'. Suggestion: update the type to'command' | 'http' | 'function'and returnresult.hookConfig.type. -
AllowedHttpHookUrls config not wired up —
getSecurityConfig()inhookSystem.tsreadsconfig.security.allowedHttpHookUrlsvia unsafe cast, but this path doesn't exist in the settings schema.UrlValidator.isAllowed()always returnstrue, making the URL whitelist feature dead code. Suggestion: either add it to the settings schema or document that HTTP hooks can reach any non-blocked URL by design. -
Duplicate SSRF protection implementations —
ssrfGuard.tsandurlValidator.tsboth implement IP blocking with divergent logic (ssrfGuard handles IPv6 mapped addresses andfc00::/7; urlValidator has simpler checks).urlValidator.validateWithDnsCheck()andvalidateResolvedIp()appear to be dead code. Suggestion: consolidate tossrfGuard.tsas the single authoritative check and remove duplicates. -
User-level hooks may be silently disabled in untrusted folders —
processHooksFromConfig()inhookRegistry.tsgates all hooks behindisTrustedFolder(). IfgetHooks()returns merged config (user + project), user hooks would be blocked in untrusted folders. Suggestion: verify that only project hooks are gated by folder trust.
Verdict
COMMENT — Solid architecture and security approach overall, but the telemetry bug, disconnected URL whitelist, and duplicate SSRF implementations should be addressed. None are blockers for core functionality.
Reslove 1,2,3 |
tanzhenxin
left a comment
There was a problem hiding this comment.
Re-review
Issues 1-3 from the previous review are all correctly fixed — nice work on the SSRF consolidation and allowedHttpHookUrls wiring.
Remaining items
Issue 4 (user hooks in untrusted folders) is still present. processHooksFromConfig() gates all hooks behind isTrustedFolder(), but getHooks() returns merged config (user + project). Combined with getProjectHooks() being a stub, the trust system can't distinguish user hooks from project hooks — user-level hooks get silently dropped in untrusted folders.
Suggestion: split the loading so user hooks bypass the trust check, and implement getProjectHooks() to return only workspace-scoped hooks.
Easy fixes worth doing:
- Chinese characters in JSDoc (
types.ts:107:for关联 to→for linking to) getHookName— show URL for HTTP hooks instead of "unknown-hook"- Add
$QWEN_PROJECT_DIRtoexpandCommandalongside$GEMINI_PROJECT_DIRand$CLAUDE_PROJECT_DIR
Verdict
COMMENT — Core functionality is solid and issues 1-3 are well addressed. Issue 4 should be fixed but isn't a merge blocker.
Resolved |
TLDR
This PR implements a comprehensive Hook system extension with three new hook types:
Key features include:
Screenshots / Video Demo
Http hook:
SessionStart:
sessionStart.mp4
PostToolUse:

Function hook:
PreToolUse:
Async Hook:

PreToolUse:
UI for hooks:

Dive Deeper
Key Components
httpHookRunner.ts)functionHookRunner.ts)asyncHookRegistry.ts)sessionHooksManager.ts)urlValidator.ts: URL whitelist matching and SSRF protectionenvInterpolator.ts: Safe environment variable interpolation with explicit whitelistConfiguration Example:
Reviewer Test Plan
envInterpolator.test.tsurlValidator.test.tshttpHookRunner.test.tsfunctionHookRunner.test.tsasyncHookRegistry.test.tssessionHooksManager.test.tsnpm run build to ensureno TypeScript errorsnpm run lintto verify code styleTesting Matrix
Linked issues / bugs