Support cascade and droid hooks#2
Conversation
There was a problem hiding this comment.
Security Issues
- Command Injection Risk in written agent hook commands
The install routines build a single command string using unvalidated user-controlled binaryPath and embed it into external agent config files (Factory Droid, Windsurf Cascade). If those agents execute the "command" field via a shell, filenames containing shell metacharacters (e.g., ; | ` $ " ) or spaces can inject arbitrary commands, leading to code execution whenever hooks run.
Summary: This PR adds Droid and Cascade platform support, including new install paths that write command strings into agent configs and new unified handlers that pass through execution metadata to hooks.
Risk: High. Writing unescaped, user-controlled command strings into third-party agent configs can result in command injection if the agents invoke them via a shell, enabling code execution under the user's context.
Recommendations:
- Validate and restrict binaryPath to a safe character set (e.g., absolute path, no whitespace or shell metacharacters) before writing to config; reject unsafe inputs.
- Prefer argument-array configuration if supported by the target agents; otherwise, safely quote/escape the path and arguments according to the agent's execution semantics and document that paths with special characters are disallowed.
- Consider resolving to an absolute path and ensuring it points to an executable owned by the current user to reduce tampering risk.
| } | ||
|
|
||
| // Build hooks config (same structure as Claude Code) | ||
| hooks := map[string]any{ |
There was a problem hiding this comment.
Unvalidated user-controlled binaryPath is concatenated into a single command string written to external agent configs. If those agents invoke the command via a shell, shell metacharacters in the path can lead to command injection and arbitrary code execution whenever the hook is triggered.
Examples:
"command": binaryPath + " droid-stop",
"command": binaryPath + " droid-pre-tool-use",
"command": binaryPath + " droid-after-file-edit",
"command": binaryPath + " droid-user-prompt-submit",And for Cascade:
"pre_run_command": []map[string]any{{"command": binaryPath + " cascade-pre-run-command"}},
"pre_mcp_tool_use": []map[string]any{{"command": binaryPath + " cascade-pre-mcp-tool-use"}},
"pre_user_prompt": []map[string]any{{"command": binaryPath + " cascade-pre-user-prompt"}},
"post_write_code": []map[string]any{{"command": binaryPath + " cascade-post-write-code"}},
"post_cascade_response": []map[string]any{{"command": binaryPath + " cascade-post-cascade-response"}},An attacker who influences the binary path (e.g., a filename with ; or backticks) could inject extra commands if the consumer uses sh -c or equivalent. This deviates from the project's guardrail of avoiding shell-constructed command strings.
Remediation: Validate binaryPath to disallow whitespace and shell metacharacters, require an absolute path, and/or use argument arrays if the agent config supports them. If only a single string is allowed, quote and escape per the agent's execution semantics and document constraints.
For more details, see the finding in Corridor.
Provide feedback: Reply with whether this is a valid vulnerability or false positive to help improve Corridor's accuracy.
There was a problem hiding this comment.
this is fine, binaryPath is user controlled
There was a problem hiding this comment.
Thank you for the clarification. I have marked this finding as a false positive based on your feedback.
| hookshot.Register("droid-stop", handleDroidStop) | ||
| hookshot.Register("droid-pre-tool-use", handleDroidPreToolUse) | ||
| hookshot.Register("droid-user-prompt-submit", handleDroidUserPromptSubmit) |
There was a problem hiding this comment.
🔴 Handler overwrite: unified handlers silently replaced by platform-specific handlers
In the example file, unified handlers (OnStop, OnBeforeExecution, OnPromptSubmit) are called first, which internally register handlers for droid-stop, droid-pre-tool-use, droid-user-prompt-submit, cascade-pre-run-command, and cascade-pre-user-prompt. Then the code explicitly registers platform-specific handlers for the same command names, which silently overwrites the unified handlers.
Click to expand
How this happens
The Register function at hookshot.go:89-91 simply overwrites existing handlers:
func Register(name string, handler Handler) {
handlers[name] = handler
}In the example:
- Line 75-78:
hookshot.OnStop(handleStop)registers handlers includingdroid-stop(viaunified.go:116) - Line 103:
hookshot.Register("droid-stop", handleDroidStop)overwrites that handler
Similar overwrites happen for:
droid-pre-tool-use(unified at line 76 → overwritten at line 104)droid-user-prompt-submit(unified at line 78 → overwritten at line 105)cascade-pre-run-command(unified at line 76 → overwritten at line 95)cascade-pre-user-prompt(unified at line 78 → overwritten at line 97)
Impact
The unified handler logic (which provides consistent StopContext, ExecutionContext, PromptContext with platform detection and decision mapping) is completely lost. The platform-specific handlers bypass the unified API contract.
Recommendation: Remove the platform-specific handler registrations for droid and cascade (lines 95-97, 103-105) since they conflict with the unified handlers. Alternatively, if platform-specific behavior is needed, don't use the unified handlers for those platforms.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
this is fine, it's a sample doc
| ToolName string // MCP tool name (e.g., "mcp__server__tool") | ||
| ToolInput json.RawMessage // Tool input parameters as JSON | ||
| ServerURL string // MCP server URL (Cursor only, for URL-based servers) | ||
| ServerURL string // MCP server URL (Cursor/Cascade only, for URL-based servers) |
There was a problem hiding this comment.
Does the ServerURL follow the same format as Cursor?
There was a problem hiding this comment.
Not certain! I'll test it out with the e2e extension flow and then update this as needed.
tanuj-corridor
left a comment
There was a problem hiding this comment.
- i did not test installation via Corridor extension
- the schemas look good
- we should document fields that differ from existing hook parsing logic (how we'd whitelist/blacklist serverUrl, interface for raw payload, Session vs. Execution ID)
Support cascade and droid hooks
Testing instructions: first build the binary.
Cursor
Windsurf Cascade
Cascade uses exit code 2 to block actions:
Run All Tests
go test ./...