fix: harden hooks against shell injection and Windows breakage
Self-review of the initial hooks commit turned up four real bugs: - Hook commands always shelled out via `sh -c`, which doesn't exist by default on Windows. `spawn_shell` in tools.rs already handles this with a sh/cmd platform split; run_hook now follows the same pattern. - `{cwd}` and `{tool_name}` were spliced into the command string unescaped, so a directory name or an MCP-server-supplied tool name containing shell metacharacters (backticks, `$(...)`) would execute as shell syntax instead of being treated as literal data. Substituted values are now quote-escaped per platform, and also exported as `SIGIT_HOOK_*` env vars so scripts can read them without any quoting at all. - execute_tool() reloaded settings.toml from disk twice per tool call (once for pre, once for post) even when no hooks were configured, contradicting the "no overhead when unconfigured" doc claim. Now loads once per call. - docs/hooks.md claimed hooks don't run in the interactive TUI at all; in fact PreToolUse/PostToolUse fire there too (tools::execute_tool is shared across TUI/ACP/headless) — only SessionStart is ACP-only today. Also documents that hooks fire for task-subagent tool calls, not just top-level ones. Added an end-to-end test that runs a real `sh -c` with a crafted tool_name to confirm injected shell syntax never executes, plus unit tests for both quoting functions. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
paydii committed
Jul 10, 2026 at 19:04 UTC
7c2f52f7023636a1c632b3423de707a183da05b8
3 files changed
+203
-18
docs/hooks.md
+19
-3
@@ -36,6 +36,15 @@ Hooks support substitution for context variables:
36
- `{tool_result_len}` - Length of the tool result in bytes
37
- `{cwd}` - The session working directory
38
39
+Every `{var}` value is quote-escaped for the platform shell before it's spliced
40
+into the command line, so a directory name or an MCP-server-supplied tool name
41
+that happens to contain shell metacharacters is always treated as literal text,
42
+never executed. Windows `cmd.exe` quoting is best-effort (`cmd` has no fully
43
+safe quoting story); the same values are also exported as environment
44
+variables — `SIGIT_HOOK_CWD`, `SIGIT_HOOK_SESSION_ID`, `SIGIT_HOOK_TOOL_NAME`,
45
+`SIGIT_HOOK_TOOL_ARGS_LEN`, `SIGIT_HOOK_TOOL_RESULT_LEN` — and reading those
46
+in your script instead of `{var}` sidesteps quoting entirely on any platform.
47
+
48
## Examples
49
50
### Log all tool usage to a file
@@ -70,7 +79,14 @@ post_tool_use = ["wc -c <<< {tool_result_len} >> ~/.sigit/result_sizes.txt"]
79
80
## Notes
81
73
-- Hooks are optional. If no hooks are configured, there is no performance overhead.
82
+- Hooks are optional. When none are configured, each tool call still does one
83
+ lightweight settings-file read to check for that; there is no shell process
84
+ spawned and no other measurable overhead.
85
- Hook failures (non-zero exit codes) are logged as warnings but do not interrupt the session.
75
-- Hooks are run synchronously, so slow hooks will impact agent responsiveness.
76
-- Hooks are not available in the interactive TUI yet—only in ACP mode and headless mode.
86
+- Hooks are run synchronously, so slow hooks will impact agent responsiveness — this applies
87
+ in the interactive TUI too, not just ACP/headless mode.
88
+- `PreToolUse` and `PostToolUse` hooks fire in every mode (TUI, ACP, headless) and for every
89
+ call to the shared tool-execution path — including tool calls made by a `task` subagent, not
90
+ just the top-level agent.
91
+- `SessionStart` hooks currently only fire on ACP session lifecycle events (new/load/fork
92
+ session). The interactive TUI does not have an equivalent session-start hook point yet.
src/hooks.rs
+160
-15
@@ -18,8 +18,19 @@
18
//! - PreToolUse: `{tool_name}`, `{tool_args_len}`
19
//! - PostToolUse: `{tool_name}`, `{tool_result_len}`
20
//!
21
-//! Hooks run in the session cwd and inherit the environment. Shell hook failures
22
-//! are logged but do not interrupt the session.
21
+//! Every value used in `{var}` substitution is quote-escaped for the platform
22
+//! shell before being spliced into the command line (`posix_quote` /
23
+//! `windows_quote`), so a project directory or MCP-server-supplied tool name
24
+//! containing shell metacharacters can never execute as shell syntax. The
25
+//! same values are also exported as `SIGIT_HOOK_*` env vars, which hook
26
+//! scripts should prefer when they need to consume a value programmatically
27
+//! rather than embed it in a `{var}` placeholder.
28
+//!
29
+//! Hooks run in the session cwd and inherit the environment. Shell hook
30
+//! failures are logged but do not interrupt the session. `PreToolUse` /
31
+//! `PostToolUse` hooks fire for every call to [`crate::tools::execute_tool`],
32
+//! including tool calls made by a `task` subagent — there is no separate
33
+//! "top-level only" mode today.
34
35
use serde::{Deserialize, Serialize};
36
use std::path::Path;
@@ -69,19 +80,22 @@ impl HookContext {
80
}
81
}
82
72
- /// Substitute variables in the command string.
73
- fn substitute(&self, cmd: &str) -> String {
83
+ /// Substitute `{var}` placeholders in the command string, passing each
84
+ /// value through `quote` first so it can never be interpreted as shell
85
+ /// syntax regardless of its contents.
86
+ fn substitute(&self, cmd: &str, quote: fn(&str) -> String) -> String {
87
let mut result = cmd.to_string();
88
if let Some(cwd) = &self.cwd {
76
- result = result.replace("{cwd}", cwd);
89
+ result = result.replace("{cwd}", "e(cwd));
90
}
91
if let Some(session_id) = &self.session_id {
79
- result = result.replace("{session_id}", session_id);
92
+ result = result.replace("{session_id}", "e(session_id));
93
}
94
if let Some(tool_name) = &self.tool_name {
82
- result = result.replace("{tool_name}", tool_name);
95
+ result = result.replace("{tool_name}", "e(tool_name));
96
}
97
if let Some(tool_args_len) = self.tool_args_len {
98
+ // Numeric — always a safe literal, no quoting needed.
99
result = result.replace("{tool_args_len}", &tool_args_len.to_string());
100
}
101
if let Some(tool_result_len) = self.tool_result_len {
@@ -89,6 +103,28 @@ impl HookContext {
103
}
104
result
105
}
106
+
107
+ /// The same context values as `SIGIT_HOOK_*` environment variables, so a
108
+ /// hook script can read them without any quoting or substitution at all.
109
+ fn env_vars(&self) -> Vec<(&'static str, String)> {
110
+ let mut vars = Vec::new();
111
+ if let Some(cwd) = &self.cwd {
112
+ vars.push(("SIGIT_HOOK_CWD", cwd.clone()));
113
+ }
114
+ if let Some(session_id) = &self.session_id {
115
+ vars.push(("SIGIT_HOOK_SESSION_ID", session_id.clone()));
116
+ }
117
+ if let Some(tool_name) = &self.tool_name {
118
+ vars.push(("SIGIT_HOOK_TOOL_NAME", tool_name.clone()));
119
+ }
120
+ if let Some(tool_args_len) = self.tool_args_len {
121
+ vars.push(("SIGIT_HOOK_TOOL_ARGS_LEN", tool_args_len.to_string()));
122
+ }
123
+ if let Some(tool_result_len) = self.tool_result_len {
124
+ vars.push(("SIGIT_HOOK_TOOL_RESULT_LEN", tool_result_len.to_string()));
125
+ }
126
+ vars
127
+ }
128
}
129
130
impl Default for HookContext {
@@ -97,14 +133,52 @@ impl Default for HookContext {
133
}
134
}
135
100
-/// Run a single hook command in the given context.
136
+/// Wrap `value` as a single POSIX shell word, so it is always treated as
137
+/// literal data by `sh -c` even if it contains spaces, quotes, or shell
138
+/// metacharacters (e.g. a project directory or an MCP-server-supplied tool
139
+/// name that happens to contain `$(...)` or backticks).
140
+///
141
+/// Only reached from production code on Unix (`run_hook`'s `#[cfg(unix)]`
142
+/// arm); always exercised via tests below regardless of host platform.
143
+#[cfg_attr(not(unix), allow(dead_code))]
144
+fn posix_quote(value: &str) -> String {
145
+ format!("'{}'", value.replace('\'', r"'\''"))
146
+}
147
+
148
+/// Best-effort quoting for `cmd.exe`: wrap in double quotes and escape
149
+/// embedded double quotes. `cmd` has no fully safe quoting story (see
150
+/// `spawn_shell`'s platform split in `tools.rs`), so this covers common
151
+/// cases but callers on Windows should prefer the `SIGIT_HOOK_*` env vars
152
+/// (set on every hook invocation, see [`run_hook`]) over `{cwd}`-style
153
+/// substitution when values may contain special characters.
154
+///
155
+/// Only reached from production code on Windows (`run_hook`'s `#[cfg(windows)]`
156
+/// arm); exercised on every platform via `test_windows_quote_neutralizes_embedded_quotes`
157
+/// below, since `aws-lc-sys` can't cross-compile to the Windows target from this host.
158
+#[cfg_attr(not(windows), allow(dead_code))]
159
+fn windows_quote(value: &str) -> String {
160
+ format!("\"{}\"", value.replace('"', "\"\""))
161
+}
162
+
163
+/// Run a single hook command in the given context. Substituted `{var}`
164
+/// placeholders are shell-quoted so a value like a directory name or an
165
+/// MCP tool name can never break out of its position and run as shell
166
+/// syntax. The same values are also exported as `SIGIT_HOOK_*` env vars
167
+/// (`SIGIT_HOOK_CWD`, `SIGIT_HOOK_SESSION_ID`, `SIGIT_HOOK_TOOL_NAME`,
168
+/// `SIGIT_HOOK_TOOL_ARGS_LEN`, `SIGIT_HOOK_TOOL_RESULT_LEN`) so hook scripts
169
+/// can read them without any quoting concerns at all.
170
fn run_hook(cmd: &str, context: &HookContext, cwd: &Path) {
102
- let substituted = context.substitute(cmd);
171
+ #[cfg(unix)]
172
+ let (shell, flag, substituted) = ("sh", "-c", context.substitute(cmd, posix_quote));
173
+ #[cfg(windows)]
174
+ let (shell, flag, substituted) = ("cmd", "/C", context.substitute(cmd, windows_quote));
175
+
176
log::debug!("Running hook: {substituted}");
104
- match Command::new("sh")
105
- .arg("-c")
177
+ match Command::new(shell)
178
+ .arg(flag)
179
.arg(&substituted)
180
.current_dir(cwd)
181
+ .envs(context.env_vars())
182
.output()
183
{
184
Ok(output) => {
@@ -197,13 +271,84 @@ mod tests {
271
tool_result_len: Some(1000),
272
};
273
assert_eq!(
200
- context.substitute("cd {cwd} && echo {session_id}"),
201
- "cd /home/user/project && echo session-123"
274
+ context.substitute("cd {cwd} && echo {session_id}", posix_quote),
275
+ "cd '/home/user/project' && echo 'session-123'"
276
);
277
assert_eq!(
204
- context.substitute("Tool: {tool_name}, args={tool_args_len}, result={tool_result_len}"),
205
- "Tool: read_file, args=42, result=1000"
278
+ context.substitute(
279
+ "Tool: {tool_name}, args={tool_args_len}, result={tool_result_len}",
280
+ posix_quote
281
+ ),
282
+ "Tool: 'read_file', args=42, result=1000"
283
+ );
284
+ }
285
+
286
+ #[test]
287
+ fn test_posix_quote_neutralizes_shell_metacharacters() {
288
+ // A directory or MCP tool name containing shell syntax must come
289
+ // through as inert literal text, not execute.
290
+ let context = HookContext {
291
+ cwd: Some("/tmp/$(rm -rf ~); echo pwned".to_string()),
292
+ ..HookContext::new()
293
+ };
294
+ let substituted = context.substitute("echo {cwd}", posix_quote);
295
+ assert_eq!(
296
+ substituted, "echo '/tmp/$(rm -rf ~); echo pwned'",
297
+ "the payload must stay inside single quotes, not break out of them"
298
+ );
299
+
300
+ // Embedded single quotes are the classic escape attempt; confirm the
301
+ // standard '\'' technique closes and reopens the quoted string
302
+ // safely instead of terminating it early.
303
+ let context = HookContext {
304
+ tool_name: Some("evil'; rm -rf ~; echo '".to_string()),
305
+ ..HookContext::new()
306
+ };
307
+ let substituted = context.substitute("echo {tool_name}", posix_quote);
308
+ assert_eq!(substituted, "echo 'evil'\\''; rm -rf ~; echo '\\'''");
309
+ }
310
+
311
+ #[test]
312
+ fn test_windows_quote_neutralizes_embedded_quotes() {
313
+ // aws-lc-sys can't cross-compile to the Windows target from this
314
+ // host, so this is exercised as a plain unit test rather than a
315
+ // Windows-target integration run.
316
+ assert_eq!(
317
+ windows_quote(r#"evil" & del /f /q *"#),
318
+ r#""evil"" & del /f /q *""#
319
+ );
320
+ assert_eq!(
321
+ windows_quote("/home/user/project"),
322
+ "\"/home/user/project\""
323
+ );
324
+ }
325
+
326
+ #[test]
327
+ fn test_pre_tool_use_hook_does_not_execute_injected_shell_syntax() {
328
+ // End-to-end: run a real `sh -c` (not just the string-substitution
329
+ // layer) with a tool_name crafted to look like a shell command
330
+ // injection, and confirm the injected `touch` never actually runs.
331
+ let dir = std::env::temp_dir().join(format!(
332
+ "sigit-hooks-test-{:?}-{}",
333
+ std::thread::current().id(),
334
+ std::process::id()
335
+ ));
336
+ std::fs::create_dir_all(&dir).unwrap();
337
+ let marker = dir.join("pwned");
338
+ assert!(!marker.exists());
339
+
340
+ let malicious_tool_name = format!("read_file'; touch {}; echo '", marker.display());
341
+ let hooks = HookSettings {
342
+ pre_tool_use: vec!["echo hook-ran {tool_name}".to_string()],
343
+ ..HookSettings::default()
344
+ };
345
+ run_pre_tool_use_hooks(&hooks, &malicious_tool_name, "{}", &dir);
346
+
347
+ assert!(
348
+ !marker.exists(),
349
+ "injected `touch` executed — {{tool_name}} substitution is not shell-safe"
350
);
351
+ std::fs::remove_dir_all(&dir).ok();
352
}
353
354
#[test]
src/tools.rs
+24
@@ -429,6 +429,30 @@ pub fn all_tools() -> Vec<AgentTool> {
429
// ── Tool execution ───────────────────────────────────────────────────────────
430
431
pub async fn execute_tool(name: &str, arguments: &str) -> String {
432
+ // Settings are read once per call (not once per hook phase) so a
433
+ // deployment without any [hooks] configured pays one cheap TOML parse
434
+ // per tool call rather than two.
435
+ let hook_settings = crate::settings::load().hooks;
436
+ let cwd = std::env::current_dir().ok();
437
+
438
+ if let Some(cwd) = &cwd
439
+ && !hook_settings.pre_tool_use.is_empty()
440
+ {
441
+ crate::hooks::run_pre_tool_use_hooks(&hook_settings, name, arguments, cwd);
442
+ }
443
+
444
+ let result = execute_tool_impl(name, arguments).await;
445
+
446
+ if let Some(cwd) = &cwd
447
+ && !hook_settings.post_tool_use.is_empty()
448
+ {
449
+ crate::hooks::run_post_tool_use_hooks(&hook_settings, name, &result, cwd);
450
+ }
451
+
452
+ result
453
+}
454
+
455
+async fn execute_tool_impl(name: &str, arguments: &str) -> String {
456
match name {
457
"read_file" => exec_read_file(arguments),
458
"list_directory" => exec_list_directory(arguments),