diff --git a/Cargo.lock b/Cargo.lock index 2049e7b9..5a663d91 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.19.2" +version = "3.21.0" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index 9a1741a9..6e71faa8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.19.2" +version = "3.21.0" edition = "2021" description = "Claude Code PreToolUse hook that guards against destructive shell commands and unsafe file-write content" diff --git a/README.md b/README.md index cdc19d05..e3019fd0 100644 --- a/README.md +++ b/README.md @@ -301,7 +301,7 @@ result = eval(userInput); // clawband: flagged — this is a real call expressio **Status**: eight rules across all 4 supported languages (Rust, Python, JavaScript, TypeScript) — a growing set, not yet a complete one: -- `dynamic-eval`: `eval()`/`Function()` in JS/TS, `eval()`/`exec()` in Python (no Rust equivalent — there's no direct analog to construct-code-from-a-string in safe Rust) +- `dynamic-eval`: `eval()`/`Function()` in JS/TS — including the `new Function(...)` constructor-call form, a genuinely different AST node (`new_expression`) from bare `Function(...)` (`call_expression`) that the original query didn't cover (issue #263) — `eval()`/`exec()` in Python (no Rust equivalent — there's no direct analog to construct-code-from-a-string in safe Rust) - `shell-invoking-subprocess`: handing a string to a shell interpreter instead of exec'ing a program directly — `subprocess.run/call/Popen/check_call/check_output(..., shell=True)` and bare `os.system`/`os.popen` in Python; `.exec`/`.execSync` in JS/TS (`.execFile`/`.spawn` and their `*Sync` variants are deliberately not flagged — they take an argv array and never invoke a shell); `Command::new("sh"/"bash"/"/bin/sh"/"/bin/bash").arg("-c")` in Rust - `insecure-deserialize`: APIs that can execute arbitrary code embedded in their input, not just parse data — `pickle.load`/`pickle.loads`, `marshal.loads`, and `yaml.load(...)` without a safe `Loader=` kwarg in Python (`yaml.safe_load` and `yaml.load(data, Loader=yaml.SafeLoader)`/`Loader=SafeLoader` are never flagged — the `Loader=` case needed a Rust-side post-match walk of the call's keyword arguments, since a tree-sitter query can only match a node's presence, not another node's absence); `vm.runInNewContext`/`runInThisContext`/`runInContext` in JS/TS (no Rust equivalent for v1) - `tls-verify-disabled`: explicitly turning off TLS/certificate verification — any call with keyword argument `verify=False` in Python (`verify=some_variable` is deliberately not flagged — a legitimate conditional-TLS pattern like `verify=IS_PRODUCTION`, and the false-positive risk of trying to resolve non-literal values is too high for v1); object literal property `rejectUnauthorized: false` in JS/TS; `.danger_accept_invalid_certs(true)` in Rust (reqwest's `ClientBuilder`) diff --git a/src/ast_guard.rs b/src/ast_guard.rs index 08679e14..2acc2217 100644 --- a/src/ast_guard.rs +++ b/src/ast_guard.rs @@ -59,7 +59,8 @@ pub enum Lang { /// (`.execute`/`.executemany` with an f-string/`%`-format/`.format()`/ /// `+`-concatenated argument). Python, - /// `.js` / `.mjs` / `.cjs` / `.jsx` — `dynamic-eval` (`eval`/`Function`), + /// `.js` / `.mjs` / `.cjs` / `.jsx` — `dynamic-eval` (`eval`/`Function`, + /// including the `new Function(...)` constructor-call form), /// `shell-invoking-subprocess` (`.exec`/`.execSync`), `insecure-deserialize` /// (`vm.runInNewContext`/`runInThisContext`/`runInContext`), `tls-verify-disabled` /// (object literal property `rejectUnauthorized: false`), `dynamic-module-load` @@ -73,7 +74,8 @@ pub enum Lang { /// for why `.tsx` needs a separate grammar variant for the JSX form of /// this rule but `.js`/`.jsx` do not). JavaScript, - /// `.ts` — `dynamic-eval` (`eval`/`Function`), `shell-invoking-subprocess` + /// `.ts` — `dynamic-eval` (`eval`/`Function`, including the + /// `new Function(...)` constructor-call form), `shell-invoking-subprocess` /// (`.exec`/`.execSync`), `insecure-deserialize` /// (`vm.runInNewContext`/`runInThisContext`/`runInContext`), `tls-verify-disabled` /// (object literal property `rejectUnauthorized: false`), `dynamic-module-load` @@ -183,7 +185,90 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { let mut rules = vec![ ( "dynamic-eval", - r#"(call_expression function: (identifier) @fn (#match? @fn "^(eval|Function)$"))"#, + // Two distinct grammar shapes for the same danger: + // `eval(x)`/`Function(x)` called as a bare function is a + // `call_expression`; `new Function(x)` is a genuinely + // different node kind, `new_expression`, with its own + // `constructor`/`arguments` fields (confirmed against + // tree-sitter-javascript's node-types.json — issue #263). + // Before this fix the query only matched the + // `call_expression` form, so `new Function(...)` — the + // far more common way this constructor is actually + // invoked in real code — slipped through entirely; this + // was a genuine gap in the existing rule, not a + // deliberately-scoped exclusion, so it's fixed here + // rather than added as a separate rule. `eval` has no + // `new eval(...)` form worth matching (it isn't a + // constructor and `new eval()` throws at runtime), so + // only `Function` needs the `new_expression` arm. + // + // Gotcha verified empirically while building this: the + // two alternation branches below MUST use distinct + // capture names (`@fn` vs `@ctor`). Reusing the same + // capture name (`@fn`) in both branches of a top-level + // `[ ... ]` alternation silently breaks matching for + // BOTH branches — `cursor.matches()` stopped returning + // the plain `eval(x)` call_expression match entirely, + // not just the new branch — even though `Query::new` + // compiles it without error. Caught by a pre-existing + // regression test (`flags_real_eval_call_in_js`) that + // would otherwise have silently regressed. + r#"[ + (call_expression function: (identifier) @fn (#match? @fn "^(eval|Function)$")) + (new_expression constructor: (identifier) @ctor (#eq? @ctor "Function")) +]"#, + "dynamic code execution (eval/Function constructor) — can run attacker-controlled strings as code", + ), + ( + "dynamic-eval", + // Second-opinion review finding on PR #301: `Function` + // qualified off `window`/`globalThis`/`self` — the same + // bypass class already fixed for `document.write` in + // PR #299 (`js_flags_window_document_write`) — slipped + // past both the bare-identifier `call_expression` branch + // and the `new_expression` branch above, since both + // require the callee/constructor to be a bare + // `identifier` named `Function`, not a qualified member + // access. Covers `window.Function(x)` / + // `window["Function"](x)` (call form) — the `new`-prefixed + // form is a separate entry below since `new_expression` + // is a genuinely different node kind. `globalThis`/`self` + // included since they're the other common ways code + // reaches this same global scope in JS. + r#"(call_expression + function: [ + (member_expression + object: (identifier) @win + property: (property_identifier) @method) + (subscript_expression + object: (identifier) @win + index: (string (string_fragment) @method)) + ] + (#match? @win "^(window|globalThis|self)$") + (#eq? @method "Function"))"#, + "dynamic code execution (eval/Function constructor) — can run attacker-controlled strings as code", + ), + ( + "dynamic-eval", + // `new`-prefixed counterpart to the rule above: `new + // window.Function(x)` / `new window["Function"](x)`. + // `new_expression`'s `constructor` field accepts a + // `member_expression`/`subscript_expression` via + // tree-sitter-javascript's `primary_expression` supertype + // (confirmed against node-types.json), so this is a + // distinct query rather than reachable from the bare- + // `identifier` `new_expression` branch above. + r#"(new_expression + constructor: [ + (member_expression + object: (identifier) @win + property: (property_identifier) @method) + (subscript_expression + object: (identifier) @win + index: (string (string_fragment) @method)) + ] + (#match? @win "^(window|globalThis|self)$") + (#eq? @method "Function"))"#, "dynamic code execution (eval/Function constructor) — can run attacker-controlled strings as code", ), ( @@ -1194,6 +1279,142 @@ mod tests { ); } + #[test] + fn js_flags_bare_function_call() { + // Confirms the pre-existing bare-`Function(...)` call_expression form + // still matches after the query was extended for `new_expression` + // (issue #263) — a regression check on the original shape. + let findings = scan("const f = Function(user_input);", Lang::JavaScript); + assert!( + findings.iter().any(|f| f.rule == "dynamic-eval"), + "bare Function(...) call must still be flagged" + ); + } + + #[test] + fn js_flags_new_function_call() { + // issue #263: `new Function(...)` parses as a `new_expression` node, + // a genuinely different AST shape than the `call_expression` the + // original dynamic-eval query matched — this was a real gap in the + // existing rule (not a new rule), fixed by extending the query. + let findings = scan("const f = new Function(user_input);", Lang::JavaScript); + assert!( + findings.iter().any(|f| f.rule == "dynamic-eval"), + "new Function(...) must be flagged: same danger as bare Function(...), different AST node kind" + ); + } + + #[test] + fn ts_flags_new_function_call() { + let findings = scan("const f = new Function(user_input);", Lang::TypeScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn tsx_flags_new_function_call() { + // Regression coverage for Lang::Tsx specifically: it parses via the + // dedicated LANGUAGE_TSX grammar, a genuinely separate grammar from + // LANGUAGE_TYPESCRIPT, so a query that matches on one is not + // guaranteed to match on the other without this explicit check. + let findings = scan("const f = new Function(user_input);", Lang::Tsx); + assert!( + findings.iter().any(|f| f.rule == "dynamic-eval"), + "new Function(...) must be flagged under the Tsx grammar too" + ); + } + + #[test] + fn tsx_ignores_new_of_unrelated_constructor() { + let findings = scan("const d = new Date();", Lang::Tsx); + assert!( + !findings.iter().any(|f| f.rule == "dynamic-eval"), + "new Date() must not be flagged as dynamic-eval under the Tsx grammar" + ); + } + + #[test] + fn js_flags_window_qualified_function_call() { + // Second-opinion review finding on PR #301: bare `window.Function(x)` + // bypassed dynamic-eval the same way bare `window.document.write(x)` + // bypassed xss-sink before PR #299's fix — same qualification-bypass + // class, different sink. + let findings = scan("window.Function(userInput);", Lang::JavaScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_flags_globalthis_qualified_function_call() { + let findings = scan("globalThis.Function(userInput);", Lang::JavaScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_flags_self_qualified_function_bracket_call() { + let findings = scan("self[\"Function\"](userInput);", Lang::JavaScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_flags_new_window_qualified_function_call() { + let findings = scan("new window.Function(userInput);", Lang::JavaScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_flags_new_window_qualified_function_bracket_call() { + let findings = scan("new window['Function'](userInput);", Lang::JavaScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_ignores_window_qualified_unrelated_method() { + // Must stay scoped to exactly `Function`, not any window-qualified call. + let findings = scan("window.alert(userInput);", Lang::JavaScript); + assert!(!findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_ignores_unrelated_object_qualified_function_call() { + // Must stay scoped to window/globalThis/self, not any arbitrary object. + let findings = scan("someOtherObj.Function(userInput);", Lang::JavaScript); + assert!(!findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn ts_flags_window_qualified_function_call() { + let findings = scan("window.Function(userInput);", Lang::TypeScript); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn tsx_flags_new_window_qualified_function_call() { + let findings = scan("new window.Function(userInput);", Lang::Tsx); + assert!(findings.iter().any(|f| f.rule == "dynamic-eval")); + } + + #[test] + fn js_ignores_new_of_unrelated_constructor() { + // Required false-positive check: `new` on any other constructor must + // not be swept in by the `new_expression` arm. + let findings = scan("const d = new Date();", Lang::JavaScript); + assert!( + !findings.iter().any(|f| f.rule == "dynamic-eval"), + "new Date() must not be flagged as dynamic-eval" + ); + } + + #[test] + fn js_ignores_new_function_mention_in_comment() { + let findings = scan( + "// new Function(x) would be bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!( + !findings.iter().any(|f| f.rule == "dynamic-eval"), + "new Function mentioned in a comment must not be flagged" + ); + } + #[test] fn flags_real_eval_call_in_python() { let findings = scan("eval(user_input)", Lang::Python); diff --git a/tests/cli.rs b/tests/cli.rs index 1e04d11b..3ce00a9d 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -402,6 +402,32 @@ fn e2e_ast_guard_flags_real_eval_call_in_js() { assert!(out.contains("dynamic-eval"), "{out}"); } +#[test] +fn e2e_ast_guard_flags_new_function_call() { + // issue #263: `new Function(...)` parses as a `new_expression` node, a + // different AST shape from the bare `Function(...)` `call_expression` + // the pre-existing dynamic-eval query matched — this was a genuine gap + // in the existing rule, fixed by extending its query rather than adding + // a separate rule. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const f = new Function(user_input);"}}"#; + let out = run(json, &[]); + assert_eq!( + decision(&out), + Some("ask"), + "new Function(...) call must ask: {out}" + ); + assert!(out.contains("dynamic-eval"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_new_date_call() { + // Required false-positive check: `new` on an unrelated constructor must + // not be swept in by the new_expression arm added for `new Function`. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const d = new Date();"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "new Date() must not ask: {out}"); +} + #[test] fn e2e_ast_guard_ignores_eval_in_js_comment() { // The entire reason this module exists over regex: a comment mentioning