diff --git a/Cargo.lock b/Cargo.lock index dad9a877..2049e7b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,7 +50,7 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clawband" -version = "3.18.1" +version = "3.19.2" dependencies = [ "libc", "regex", diff --git a/Cargo.toml b/Cargo.toml index a731706d..9a1741a9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clawband" -version = "3.18.1" +version = "3.19.2" 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 c50164a0..cdc19d05 100644 --- a/README.md +++ b/README.md @@ -299,7 +299,7 @@ const msg = "eval(x) is dangerous"; // clawband: ignored — this is a string li result = eval(userInput); // clawband: flagged — this is a real call expression ``` -**Status**: seven rules across all 4 supported languages (Rust, Python, JavaScript, TypeScript) — a growing set, not yet a complete one: +**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) - `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 @@ -308,6 +308,7 @@ result = eval(userInput); // clawband: flagged — this is a real call expressio - `dynamic-module-load`: `require(...)`/dynamic `import(...)` in JS/TS whose argument isn't a fixed string literal — the "flag everything except a specific node kind" shape, so like the `Loader=` case above it needed a Rust-side check of the argument node's kind rather than a pure query; a template literal with `${...}` interpolation counts as non-literal and flags (the exact i18n-loader-built-from-a-request-param bug class this rule targets), but a plain template literal with no interpolation is treated as literal-equivalent and doesn't (Python/Rust have no v1 equivalent — `importlib.import_module` is a deliberate v2 follow-up) - `sql-string-interpolation`: a query-execution call whose argument is built via string interpolation/concatenation/formatting instead of passed as a separate parameter — the structural shape of SQL injection, independent of whether the interpolated value is actually attacker-controlled. **Higher false-positive risk than the other rules** — flagged for extra review in its PR. `.execute`/`.executemany` in Python (covers `sqlite3`/`psycopg2`/`pymysql`/SQLAlchemy raw-connection calls, matched by method-name suffix, not object name) where the argument is an f-string with real `{...}` interpolation, a `%`-format or `.format()` call, or `+`-concatenation — a parameterized call (`execute("...?", (val,))`) or an f-string with zero interpolations both correctly fall through as safe, same argument-node-kind-inspection approach as `dynamic-module-load`; `.query`/`.execute` in JS/TS where the argument is a template literal containing `${...}` interpolation (no Rust equivalent for v1 — sqlx's compile-time-checked macros and diesel's query builder make this a much rarer footgun there) - `rust-unsafe-block`: the odd one out — `unsafe { ... }` isn't inherently a bug the way the rules above are; it's a routine, sometimes-necessary tool (FFI, raw pointer manipulation for performance). The value is *visibility*, making sure an agent-written `unsafe` block gets a human's eyes on it, not "this is always wrong." Rust only, a direct `(unsafe_block)` match with no predicate needed; a bare `unsafe fn` signature with no `{ }` body is a distinct grammar node (`function_modifiers`, no `unsafe_block` child) and is deliberately out of scope for v1 +- `xss-sink`: browser/React APIs that render their argument as live HTML/script rather than as inert text — JS/TS only. `.innerHTML =`/`.outerHTML =` assignment (a `member_expression` assignment *target*, a different query shape than the call-expression rules above), `.insertAdjacentHTML(...)`, and `document.write(...)` are matched unconditionally, with no attempt to tell a "safe-looking" static-string assignment from a dynamic one — the upstream reference this rule targets parity with (the `security-guidance` Claude Code plugin's `innerHTML_xss`/`outerHTML_xss`/`insertAdjacentHTML_xss`/`document_write_xss` rules) doesn't narrow on value either, just on file extension, so there's no narrower behavior to reproduce here. React's `dangerouslySetInnerHTML` JSX attribute is also flagged, but only where the grammar can actually parse JSX: `tree-sitter-javascript`'s default grammar parses JSX out of the box (so `.js`/`.jsx` are covered), and `.tsx` needed a second, dedicated TypeScript grammar variant (`tree_sitter_typescript::LANGUAGE_TSX`, alongside the existing `LANGUAGE_TYPESCRIPT` used for plain `.ts`) since confirmed empirically that `LANGUAGE_TYPESCRIPT` has zero JSX node kinds at all — a `.ts` file can't contain JSX syntax, so this sub-rule doesn't apply there Unsupported languages fail open: the path-based checks above still apply, this only ever adds coverage, it never removes it. Runs in the same `PreToolUse` handler as the path checks — deny (self-protection, protect.paths) always takes priority over an AST-guard `ask`, since the handler returns as soon as an earlier, higher-severity check fires. @@ -553,7 +554,7 @@ Removing the entry from that list lets clawband's ask/deny patterns take over as - **Force-push gaps** — `git push :` (colon-prefix deletion) and `git push origin +main` (plus-refspec force) are not blocked by the force-push pattern; use `--delete` / `--force-with-lease` instead. - **Commit messages containing blocked patterns** — if a commit message itself contains a pattern like `rm -rf /` (e.g. documenting a fix), clawband will block the `git commit` command. Workaround: write the message to a temp file and use `git commit -F /tmp/msg.txt`, or rephrase to avoid the literal pattern. - **Fail-closed on parse error** — if clawband cannot read or parse the hook input (stdin read failure, malformed JSON), it emits `deny` and blocks the command. It does **not** fail-open. -- **AST content guard covers seven rules so far** — `dynamic-eval` (`eval`/`Function`/`exec`), `shell-invoking-subprocess`, `insecure-deserialize`, `tls-verify-disabled`, `dynamic-module-load`, `sql-string-interpolation`, and `rust-unsafe-block` (see [AST content guard](#ast-content-guard) above); any language outside Rust/Python/JS/TS falls open. This augments, not replaces, path-based Write/Edit protection. +- **AST content guard covers eight rules so far** — `dynamic-eval` (`eval`/`Function`/`exec`), `shell-invoking-subprocess`, `insecure-deserialize`, `tls-verify-disabled`, `dynamic-module-load`, `sql-string-interpolation`, `rust-unsafe-block`, and `xss-sink` (see [AST content guard](#ast-content-guard) above); any language outside Rust/Python/JS/TS falls open. This augments, not replaces, path-based Write/Edit protection. ### Known shell obfuscation bypasses (issue #129) diff --git a/src/ast_guard.rs b/src/ast_guard.rs index 22955fba..08679e14 100644 --- a/src/ast_guard.rs +++ b/src/ast_guard.rs @@ -65,16 +65,42 @@ pub enum Lang { /// (object literal property `rejectUnauthorized: false`), `dynamic-module-load` /// (`require`/`import()` with a non-string-literal argument), /// `sql-string-interpolation` (`.query`/`.execute` with a template-literal - /// argument containing `${...}` interpolation). + /// argument containing `${...}` interpolation), `xss-sink` + /// (`.innerHTML =`/`.outerHTML =` assignment, `.insertAdjacentHTML(...)`, + /// `document.write(...)`, and — since the default `tree-sitter-javascript` + /// grammar parses JSX out of the box, even in a plain `.js` file — the + /// `dangerouslySetInnerHTML` JSX attribute; see `Lang::Tsx`'s doc comment + /// for why `.tsx` needs a separate grammar variant for the JSX form of + /// this rule but `.js`/`.jsx` do not). JavaScript, - /// `.ts` / `.tsx` — `dynamic-eval` (`eval`/`Function`), - /// `shell-invoking-subprocess` (`.exec`/`.execSync`), `insecure-deserialize` + /// `.ts` — `dynamic-eval` (`eval`/`Function`), `shell-invoking-subprocess` + /// (`.exec`/`.execSync`), `insecure-deserialize` /// (`vm.runInNewContext`/`runInThisContext`/`runInContext`), `tls-verify-disabled` /// (object literal property `rejectUnauthorized: false`), `dynamic-module-load` /// (`require`/`import()` with a non-string-literal argument), /// `sql-string-interpolation` (`.query`/`.execute` with a template-literal - /// argument containing `${...}` interpolation). + /// argument containing `${...}` interpolation), `xss-sink` (`.innerHTML =`/ + /// `.outerHTML =` assignment, `.insertAdjacentHTML(...)`, `document.write(...)` + /// — but NOT the `dangerouslySetInnerHTML` JSX-attribute form, since plain + /// `.ts` files can't contain JSX syntax and `tree-sitter-typescript`'s + /// `LANGUAGE_TYPESCRIPT` grammar has no JSX node kinds at all; see + /// `Lang::Tsx` for the `.tsx` variant that does parse JSX). TypeScript, + /// `.tsx` — same rule set as `Lang::TypeScript` (all of `.ts`'s rules + /// apply verbatim, since `.tsx` is a superset of `.ts` syntax), PLUS the + /// `dangerouslySetInnerHTML` JSX-attribute form of `xss-sink`, which + /// `.ts` cannot have. This needs its own `Lang` variant (rather than + /// reusing `Lang::TypeScript` for both `.ts` and `.tsx` as clawband did + /// prior to issue #262) because `tree-sitter-typescript` ships JSX + /// support as a genuinely separate compiled grammar, + /// `tree_sitter_typescript::LANGUAGE_TSX` — confirmed empirically + /// (issue #262 investigation) that `LANGUAGE_TYPESCRIPT`'s + /// `node-types.json` has zero `jsx_*` node kinds, while `LANGUAGE_TSX`'s + /// does; a `jsx_attribute` tree-sitter query fails to even compile + /// (`Query::new` returns `Err`) against `LANGUAGE_TYPESCRIPT`, so + /// `dangerouslySetInnerHTML` is genuinely unreachable there and not just + /// unlikely to match syntactically. + Tsx, } /// Extensions this module can parse. Anything else returns `None` and the @@ -86,7 +112,8 @@ pub fn detect_language(path: &str) -> Option { "rs" => Some(Lang::Rust), "py" => Some(Lang::Python), "js" | "mjs" | "cjs" | "jsx" => Some(Lang::JavaScript), - "ts" | "tsx" => Some(Lang::TypeScript), + "ts" => Some(Lang::TypeScript), + "tsx" => Some(Lang::Tsx), _ => None, } } @@ -97,6 +124,7 @@ fn ts_language(lang: &Lang) -> TsLanguage { Lang::Python => tree_sitter_python::LANGUAGE.into(), Lang::JavaScript => tree_sitter_javascript::LANGUAGE.into(), Lang::TypeScript => tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into(), + Lang::Tsx => tree_sitter_typescript::LANGUAGE_TSX.into(), } } @@ -151,39 +179,215 @@ fn rules_for(lang: &Lang) -> Vec<(&'static str, &'static str, &'static str)> { let insecure_deserialize_reason = "insecure deserialization — this API can execute arbitrary code embedded in its input; if the input isn't fully trusted, use a data-only parser instead"; let tls_verify_disabled_reason = "TLS certificate verification disabled — this accepts connections to servers with invalid/self-signed/expired certificates, defeating TLS's protection against MITM; should not ship to production"; match lang { - Lang::JavaScript | Lang::TypeScript => vec![ - ( - "dynamic-eval", - r#"(call_expression function: (identifier) @fn (#match? @fn "^(eval|Function)$"))"#, - "dynamic code execution (eval/Function constructor) — can run attacker-controlled strings as code", - ), - ( - "shell-invoking-subprocess", - r#"(call_expression + Lang::JavaScript | Lang::TypeScript | Lang::Tsx => { + let mut rules = vec![ + ( + "dynamic-eval", + r#"(call_expression function: (identifier) @fn (#match? @fn "^(eval|Function)$"))"#, + "dynamic code execution (eval/Function constructor) — can run attacker-controlled strings as code", + ), + ( + "shell-invoking-subprocess", + r#"(call_expression function: (member_expression property: (property_identifier) @method) (#match? @method "^(exec|execSync)$"))"#, - shell_invoking_reason, - ), - ( - "insecure-deserialize", - r#"(call_expression + shell_invoking_reason, + ), + ( + "insecure-deserialize", + r#"(call_expression function: (member_expression object: (identifier) @obj property: (property_identifier) @method) (#eq? @obj "vm") (#match? @method "^(runInNewContext|runInThisContext|runInContext)$"))"#, - insecure_deserialize_reason, - ), - ( - "tls-verify-disabled", - r#"(pair + insecure_deserialize_reason, + ), + ( + "tls-verify-disabled", + r#"(pair key: (property_identifier) @key value: (false) (#eq? @key "rejectUnauthorized"))"#, - tls_verify_disabled_reason, - ), - ], + tls_verify_disabled_reason, + ), + ( + "xss-sink", + r#"(assignment_expression + left: [ + (member_expression + property: (property_identifier) @prop) + (subscript_expression + index: (string (string_fragment) @prop)) + ] + (#eq? @prop "innerHTML"))"#, + "cross-site scripting (XSS) sink — assigning to innerHTML renders its value as live HTML/script; if the value isn't fully trusted, use textContent for plain text, or sanitize with a library like DOMPurify if HTML is genuinely needed", + ), + ( + "xss-sink", + // `+=`/`||=`/etc. on innerHTML is the same sink as `=` — + // this is an `augmented_assignment_expression` node, a + // distinct grammar rule from `assignment_expression` + // (confirmed against tree-sitter-javascript's grammar.js: + // `augmented_assignment_expression` has its own `left`/ + // `operator`/`right` fields and its own `_augmented_assignment_lhs` + // choice, which is why it needs its own query rather than + // being covered by the plain-assignment pattern above). + // Verified P1 Greptile finding on PR #299: `el.innerHTML + // += attackerHtml` bypassed the guard entirely before + // this rule existed. + r#"(augmented_assignment_expression + left: [ + (member_expression + property: (property_identifier) @prop) + (subscript_expression + index: (string (string_fragment) @prop)) + ] + (#eq? @prop "innerHTML"))"#, + "cross-site scripting (XSS) sink — compound-assigning (+=) to innerHTML is equivalent to a plain assignment for XSS purposes; if the value isn't fully trusted, use textContent for plain text, or sanitize with a library like DOMPurify if HTML is genuinely needed", + ), + ( + "xss-sink", + r#"(assignment_expression + left: [ + (member_expression + property: (property_identifier) @prop) + (subscript_expression + index: (string (string_fragment) @prop)) + ] + (#eq? @prop "outerHTML"))"#, + "cross-site scripting (XSS) sink — outerHTML assignment is equivalent to innerHTML for XSS purposes; use textContent or sanitize with a library like DOMPurify", + ), + ( + "xss-sink", + // See the innerHTML `augmented_assignment_expression` + // comment above — same node kind, same bypass shape, + // just for outerHTML. + r#"(augmented_assignment_expression + left: [ + (member_expression + property: (property_identifier) @prop) + (subscript_expression + index: (string (string_fragment) @prop)) + ] + (#eq? @prop "outerHTML"))"#, + "cross-site scripting (XSS) sink — compound-assigning (+=) to outerHTML is equivalent to innerHTML for XSS purposes; use textContent or sanitize with a library like DOMPurify", + ), + ( + "xss-sink", + // Covers both `el.insertAdjacentHTML(...)` (dot access, + // `member_expression`) and `el["insertAdjacentHTML"](...)` + // (computed/bracket access, `subscript_expression` — a + // genuinely different grammar node from `member_expression`, + // with its own `object`/`index` fields rather than + // `object`/`property`; verified against + // tree-sitter-javascript's grammar.js and node-types.json). + // Verified P1 Greptile finding on PR #299: the bracket + // form bypassed the guard entirely before this rule + // covered it. + r#"(call_expression + function: [ + (member_expression + property: (property_identifier) @method) + (subscript_expression + index: (string (string_fragment) @method)) + ] + (#eq? @method "insertAdjacentHTML"))"#, + "cross-site scripting (XSS) sink — insertAdjacentHTML renders its argument as live HTML/script; if it isn't fully trusted, use insertAdjacentText() or sanitize with a library like DOMPurify", + ), + ( + "xss-sink", + // Covers `document.write(...)` and `document["write"](...)` + // alike, but — same as the pre-existing dot-form rule — + // deliberately scoped to the `document` object only + // (`#eq? @obj "document"`), not `.write()`/`["write"]()` + // on any arbitrary object; `foo["write"](x)` must not + // flag. Verified P1 Greptile finding on PR #299: + // `document["write"](attackerHtml)` bypassed the guard + // entirely before this rule covered the bracket form. + r#"(call_expression + function: [ + (member_expression + object: (identifier) @obj + property: (property_identifier) @method) + (subscript_expression + object: (identifier) @obj + index: (string (string_fragment) @method)) + ] + (#eq? @obj "document") + (#eq? @method "write"))"#, + "cross-site scripting (XSS) sink — document.write() with untrusted content injects and executes attacker-controlled HTML/script; use safe DOM methods like createElement()/appendChild() instead", + ), + ( + "xss-sink", + // Covers `window.document.write(...)` and its bracket + // permutations (`window["document"].write(...)`, + // `window.document["write"](...)`, + // `window["document"]["write"](...)`). Some codebases + // qualify `document` off `window` deliberately, to + // disambiguate from a shadowed local `document` + // variable — this must still be caught by the same rule + // as bare `document.write(...)`. Deliberately requires + // the outer object to be exactly `window` + // (`#eq? @win "window"`), so `someOtherWindow.document + // .write(x)` / `foo.document.write(x)` do NOT flag — this + // rule is scoped to the `window.document` access path + // specifically, mirroring how the bare-`document` rule + // above is scoped to `document` specifically. Fixes a + // gap found by second-opinion review on PR #299 (the + // dot-form/bracket-form `document`-only rules above did + // not cover this one extra hop of chained member access). + r#"(call_expression + function: [ + (member_expression + object: [ + (member_expression + object: (identifier) @win + property: (property_identifier) @doc) + (subscript_expression + object: (identifier) @win + index: (string (string_fragment) @doc)) + ] + property: (property_identifier) @method) + (subscript_expression + object: [ + (member_expression + object: (identifier) @win + property: (property_identifier) @doc) + (subscript_expression + object: (identifier) @win + index: (string (string_fragment) @doc)) + ] + index: (string (string_fragment) @method)) + ] + (#eq? @win "window") + (#eq? @doc "document") + (#eq? @method "write"))"#, + "cross-site scripting (XSS) sink — window.document.write() with untrusted content injects and executes attacker-controlled HTML/script; use safe DOM methods like createElement()/appendChild() instead", + ), + ]; + // `dangerouslySetInnerHTML` is a JSX attribute — a grammar + // construct that only `Lang::JavaScript` (default + // `tree-sitter-javascript` grammar, which parses JSX out of the + // box) and `Lang::Tsx` (dedicated `LANGUAGE_TSX` grammar) can + // even syntactically contain. Plain `Lang::TypeScript`'s + // `LANGUAGE_TYPESCRIPT` grammar has no `jsx_attribute` node kind + // at all, so including this query there would make `Query::new` + // fail (harmlessly skipped by `scan()`'s `Err(_) => continue`) — + // excluded here instead so the rule list documents what's + // actually reachable per-language rather than relying on that + // fallback. See `Lang::Tsx`'s doc comment for the empirical + // grammar-support check. + if !matches!(lang, Lang::TypeScript) { + rules.push(( + "xss-sink", + r#"(jsx_attribute (property_identifier) @name (#eq? @name "dangerouslySetInnerHTML"))"#, + "cross-site scripting (XSS) sink — React's dangerouslySetInnerHTML renders its __html value as raw HTML, executing attacker-controlled markup/script if the value isn't fully trusted; sanitize with a library like DOMPurify or avoid raw HTML rendering", + )); + } + rules + } Lang::Python => vec![ ( "dynamic-eval", @@ -946,7 +1150,7 @@ pub fn scan(content: &str, lang: Lang) -> Vec { findings.extend(python_xxe_findings(&tree, content)); findings.extend(python_sql_string_interpolation_findings(&tree, content)); } - if matches!(lang, Lang::JavaScript | Lang::TypeScript) { + if matches!(lang, Lang::JavaScript | Lang::TypeScript | Lang::Tsx) { findings.extend(js_dynamic_module_load_findings(&tree, content, &ts_lang)); findings.extend(js_sql_string_interpolation_findings( &tree, content, &ts_lang, @@ -2014,6 +2218,344 @@ mod tests { assert!(!has_sql_string_interpolation_finding(&findings)); } + // ── xss-sink (issue #262) ── + // No RHS/value narrowing here — the reference (security-guidance's + // innerHTML_xss/outerHTML_xss/insertAdjacentHTML_xss/document_write_xss/ + // react_dangerously_set_html rules) flags every occurrence of these + // sinks via a plain substring match, gated only by file extension + // (path_filter), not by whether the assigned/passed value looks + // static or dynamic — so there is no narrower upstream behavior to + // match here, unlike e.g. `tls-verify-disabled`'s literal-`False`-only + // narrowing. AST matching still eliminates the comment/string-literal + // false positives a regex would hit, same as every other rule in this + // file. + + fn has_xss_sink_finding(findings: &[Finding]) -> bool { + findings.iter().any(|f| f.rule == "xss-sink") + } + + // .innerHTML = + + #[test] + fn js_flags_inner_html_assignment() { + let findings = scan("el.innerHTML = userInput;", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_inner_html_assignment_of_static_string() { + // No RHS narrowing (see section doc comment) — even an apparently + // static string literal assignment flags, matching the reference's + // unnarrowed substring behavior. + let findings = scan(r#"el.innerHTML = "hi";"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_text_content_assignment() { + // Required false-positive test: textContent is the safe alternative + // and must never be flagged. + let findings = scan("el.textContent = userInput;", Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_inner_html_mention_in_comment() { + let findings = scan( + "// el.innerHTML = userInput; is bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_inner_html_mention_in_string_literal() { + let findings = scan(r#"const s = "el.innerHTML = userInput";"#, Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_flags_inner_html_assignment() { + let findings = scan("el.innerHTML = userInput;", Lang::TypeScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_inner_html_compound_assignment() { + // Verified P1 Greptile finding on PR #299: `+=` is an + // `augmented_assignment_expression`, a distinct grammar node from + // plain `assignment_expression`, and previously bypassed the guard. + let findings = scan("el.innerHTML += userInput;", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_inner_html_bracket_assignment() { + // Verified P1 Greptile finding on PR #299: computed/bracket property + // access is a `subscript_expression`, a distinct grammar node from + // `member_expression`, and previously bypassed the guard. + let findings = scan(r#"el["innerHTML"] = userInput;"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + // .outerHTML = + + #[test] + fn js_flags_outer_html_assignment() { + let findings = scan("el.outerHTML = userInput;", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_outer_html_mention_in_comment() { + let findings = scan( + "// el.outerHTML = userInput; is bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_flags_outer_html_assignment() { + let findings = scan("el.outerHTML = userInput;", Lang::TypeScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_outer_html_compound_assignment() { + // Verified P1 Greptile finding on PR #299. + let findings = scan("el.outerHTML += userInput;", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_outer_html_bracket_assignment() { + // Verified P1 Greptile finding on PR #299. + let findings = scan(r#"el["outerHTML"] = userInput;"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + // .insertAdjacentHTML(...) + + #[test] + fn js_flags_insert_adjacent_html() { + let findings = scan( + "el.insertAdjacentHTML('beforeend', userInput);", + Lang::JavaScript, + ); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_insert_adjacent_text() { + // Required false-positive test: insertAdjacentText is the safe + // alternative and must never be flagged. + let findings = scan( + "el.insertAdjacentText('beforeend', userInput);", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_insert_adjacent_html_mention_in_comment() { + let findings = scan( + "// el.insertAdjacentHTML('beforeend', x) is bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_flags_insert_adjacent_html() { + let findings = scan( + "el.insertAdjacentHTML('beforeend', userInput);", + Lang::TypeScript, + ); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_insert_adjacent_html_bracket_call() { + // Verified P1 Greptile finding on PR #299: computed-property call + // form (`subscript_expression` as the call's `function`) previously + // bypassed the guard entirely. + let findings = scan( + r#"el["insertAdjacentHTML"]("beforeend", userInput);"#, + Lang::JavaScript, + ); + assert!(has_xss_sink_finding(&findings)); + } + + // document.write(...) + + #[test] + fn js_flags_document_write() { + let findings = scan("document.write(userInput);", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_document_writeln() { + // document.writeln is a distinct method name; the query matches + // "write" exactly, not as a prefix. + let findings = scan("document.writeln(userInput);", Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_document_write_mention_in_comment() { + let findings = scan( + "// document.write(x) is bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_document_write_mention_in_string_literal() { + let findings = scan(r#"const s = "document.write(x)";"#, Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_flags_document_write() { + let findings = scan("document.write(userInput);", Lang::TypeScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_document_write_bracket_call() { + // Verified P1 Greptile finding on PR #299: `document["write"](...)` + // previously bypassed the guard entirely. + let findings = scan(r#"document["write"](userInput);"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_bracket_write_on_other_object() { + // Negative case: the document.write rule is deliberately scoped to + // the `document` object, not any object with a `.write()`/`["write"]()` + // method — `foo["write"](x)` must not flag, mirroring the existing + // scoping of the dot-access form to `document` specifically. + let findings = scan(r#"foo["write"](userInput);"#, Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_window_document_write() { + // Second-opinion review finding on PR #299: `window.document.write(...)` + // bypassed the guard entirely — the `document`-only dot/bracket rules + // required a bare `identifier` object equal to `"document"`, and + // qualifying `document` off `window` slipped through undetected. + let findings = scan("window.document.write(userInput);", Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_window_bracket_document_write() { + let findings = scan(r#"window["document"].write(userInput);"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_window_document_bracket_write() { + let findings = scan(r#"window.document["write"](userInput);"#, Lang::JavaScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_flags_window_bracket_document_bracket_write() { + let findings = scan( + r#"window["document"]["write"](userInput);"#, + Lang::JavaScript, + ); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_flags_window_document_write() { + let findings = scan("window.document.write(userInput);", Lang::TypeScript); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_window_document_write_on_other_outer_object() { + // Negative case: the window-qualified rule must be scoped to exactly + // `window`, not any outer object named similarly — `someOtherWindow + // .document.write(x)` must not flag. + let findings = scan( + "someOtherWindow.document.write(userInput);", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_non_document_write_off_window() { + // Negative case: `foo.document.write(x)` — the middle property isn't + // `document` — must not flag either. + let findings = scan("foo.document.write(userInput);", Lang::JavaScript); + assert!(!has_xss_sink_finding(&findings)); + } + + // dangerouslySetInnerHTML (JSX attribute — reachable in .js/.jsx via + // tree-sitter-javascript's built-in JSX support, and in .tsx via the + // dedicated LANGUAGE_TSX grammar; NOT reachable in plain .ts, which + // can't contain JSX syntax at all). + + #[test] + fn js_flags_dangerously_set_inner_html_in_jsx() { + let findings = scan( + "const el =
;", + Lang::JavaScript, + ); + assert!( + has_xss_sink_finding(&findings), + "tree-sitter-javascript parses JSX by default even in a .js/.jsx file" + ); + } + + #[test] + fn tsx_flags_dangerously_set_inner_html() { + let findings = scan( + "const el =
;", + Lang::Tsx, + ); + assert!(has_xss_sink_finding(&findings)); + } + + #[test] + fn ts_has_no_dangerously_set_inner_html_rule() { + // Plain .ts can't contain JSX syntax; LANGUAGE_TYPESCRIPT has no + // jsx_attribute node kind at all, so this is genuinely unreachable, + // not just an unlikely false negative. A bare mention of the + // identifier (not inside a JSX attribute) must not flag either. + let findings = scan("const dangerouslySetInnerHTML = true;", Lang::TypeScript); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn js_ignores_dangerously_set_inner_html_mention_in_comment() { + let findings = scan( + "// dangerouslySetInnerHTML is bad\nfunction f(){return 1;}", + Lang::JavaScript, + ); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn python_has_no_xss_sink_rule() { + let findings = scan("eval(x)", Lang::Python); + assert!(!has_xss_sink_finding(&findings)); + } + + #[test] + fn rust_has_no_xss_sink_rule() { + let findings = scan(r#"fn main() { let x = 1; }"#, Lang::Rust); + assert!(!has_xss_sink_finding(&findings)); + } + // ── rust-unsafe-block (issue #258) ── fn has_rust_unsafe_block_finding(findings: &[Finding]) -> bool { diff --git a/tests/cli.rs b/tests/cli.rs index 5514ee6e..1e04d11b 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1001,6 +1001,184 @@ fn e2e_ast_guard_ignores_js_query_template_literal_no_interpolation() { assert_eq!(decision(&out), None, "{out}"); } +// ── issue #262: xss-sink ──────────────────────────────────────────────── + +#[test] +fn e2e_ast_guard_flags_js_inner_html_assignment() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el.innerHTML = userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_js_text_content_assignment() { + // Required false-positive test: textContent is the safe alternative. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el.textContent = userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_inner_html_mention_in_comment() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"// el.innerHTML = x; is bad\nfunction f(){return 1;}"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_inner_html_compound_assignment() { + // Verified P1 Greptile finding on PR #299: `+=` (augmented_assignment_expression) + // previously bypassed the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el.innerHTML += userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_inner_html_bracket_assignment() { + // Verified P1 Greptile finding on PR #299: `el["innerHTML"] = ...` + // (subscript_expression) previously bypassed the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el[\"innerHTML\"] = userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_ts_outer_html_assignment() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.ts","content":"el.outerHTML = userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_outer_html_bracket_assignment() { + // Verified P1 Greptile finding on PR #299: `el["outerHTML"] = ...` + // previously bypassed the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el[\"outerHTML\"] = userInput;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_insert_adjacent_html() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el.insertAdjacentHTML('beforeend', userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_insert_adjacent_text() { + // Required false-positive test: insertAdjacentText is the safe alternative. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el.insertAdjacentText('beforeend', userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_insert_adjacent_html_bracket_call() { + // Verified P1 Greptile finding on PR #299: `el["insertAdjacentHTML"](...)` + // (subscript_expression as the call's function) previously bypassed + // the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"el[\"insertAdjacentHTML\"](\"beforeend\", userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_document_write() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"document.write(userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_document_writeln() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"document.writeln(userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_document_write_mention_in_string_literal() { + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"const s = \"document.write(x)\";"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_document_write_bracket_call() { + // Verified P1 Greptile finding on PR #299: `document["write"](...)` + // previously bypassed the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"document[\"write\"](userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_bracket_write_on_other_object() { + // Negative case: the document.write rule is scoped to the `document` + // object specifically — `foo["write"](x)` must not flag. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"foo[\"write\"](userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_js_window_document_write() { + // Second-opinion review finding on PR #299: `window.document.write(...)` + // previously bypassed the guard entirely. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"window.document.write(userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ignores_window_document_write_on_other_outer_object() { + // Negative case: the window-qualified rule is scoped to exactly + // `window` — `someOtherWindow.document.write(x)` must not flag. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.js","content":"someOtherWindow.document.write(userInput);"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_dangerously_set_inner_html_in_jsx() { + // tree-sitter-javascript parses JSX by default, even in a .jsx file. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.jsx","content":"const el =
;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_flags_dangerously_set_inner_html_in_tsx() { + // .tsx needs the dedicated LANGUAGE_TSX grammar (plain LANGUAGE_TYPESCRIPT + // has no JSX support at all) — see ast_guard.rs's Lang::Tsx doc comment. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.tsx","content":"const el =
;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), Some("ask"), "{out}"); + assert!(out.contains("xss-sink"), "{out}"); +} + +#[test] +fn e2e_ast_guard_ts_has_no_dangerously_set_inner_html_rule() { + // Plain .ts can't contain JSX syntax at all — a bare mention of the + // identifier outside a JSX attribute must not flag either. + let json = r#"{"tool_name":"Write","tool_input":{"file_path":"a.ts","content":"const dangerouslySetInnerHTML = true;"}}"#; + let out = run(json, &[]); + assert_eq!(decision(&out), None, "{out}"); +} + // ── issue #258: rust-unsafe-block ───────────────────────────────────────── #[test]