ast_guard: catch new Function(...) in dynamic-eval rule

[jamessoubry]

Summary

Issue #263 asked for shell-injection rule coverage across os.system, subprocess with shell=True, child_process exec/execSync, and new Function(...).

Investigation found the first three already have full Write/Edit-time AST coverage from the pre-existing shell-invoking-subprocess rule (added for issue #253) — no changes were needed there.

The only genuine gap was new Function(...). Tree-sitter represents new Function(...) as a new_expression node, distinct from the call_expression node the existing dynamic-eval rule matched for bare Function(...). This PR extends dynamic-eval to also match the new_expression form.

Gotcha found along the way

Reusing the same tree-sitter query capture name across alternation branches silently breaks matching on both branches, not just the duplicate one. Each branch in the updated dynamic-eval query now uses a distinct capture name to avoid this.

Test plan

• Unit tests added covering new Function(...) construction
• cargo test — all passing (verified in a prior phase)
• cargo clippy --all-targets -- -D warnings — clean (verified in a prior phase)
• cargo fmt --check — clean (verified in a prior phase)

Closes #263

---

— Claude (Sonnet 5), clawband backlog automation

Confidence Score: 5/5

No outstanding findings block merging.

Summary

The PR expands dynamic-eval detection to new Function(...) and qualified Function calls. The previously noted TSX test gap is closed.

_{Reviews (3) · Last reviewed commit: "fix: catch window/globalThis/self-qualif..."}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 692ad87...c5ff896 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
JavaScript		Sep 24, 2026 9:01a.m.	Review ↗
Rust		Sep 24, 2026 9:01a.m.	Review ↗
Shell		Sep 24, 2026 9:01a.m.	Review ↗
Secrets		Sep 24, 2026 9:01a.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[jamessoubry]

Second-opinion review (stopgap while Codex is capped)

Same disclosure as on my other reviews here: I'm the same model family (Claude/Sonnet) as whatever wrote this, so treat this as a sanity check rather than a genuinely independent perspective.

What this PR is

Small, focused fix to the existing dynamic-eval rule (issue #263): new Function(...) parses as a new_expression node in tree-sitter-javascript, a different grammar shape from the call_expression the original query matched (eval(x)/Function(x) as bare calls) — so new Function(...), arguably the more common real-world spelling of this constructor, was silently unflagged. Fixed by adding a new_expression alternation branch. Comes with a genuinely useful documented gotcha: reusing the same tree-sitter capture name (@fn) across both alternation branches silently broke matching for both branches, not just the new one — caught by a pre-existing regression test.

Verification: cargo fmt --check, cargo test --release, cargo clippy --all-targets --release -- -D warnings on 199a7d8 — running now, results below.

The core fix is correct

Confirmed via the PR's own tests and independently: new Function(x) now flags, Function(x) (bare call, pre-existing form) still flags, new Date() (unrelated constructor) correctly doesn't flag, and the "no new eval()" reasoning is accurate — eval isn't a constructor in real JS engines, new eval(x) throws TypeError: eval is not a constructor at runtime, so there's genuinely nothing to match there.

Finding: qualified access to Function still bypasses both branches — same bypass class as the window.document.write gap fixed two PRs ago in this same file

Both the call_expression branch (pre-existing, unchanged by this PR) and the new new_expression branch require the constructor/function to be a bare identifier named Function. I tested the natural "qualify off window" pattern — the exact same bypass shape this project's own XSS-sink rule was just patched for (window.document.write, PR #299) — and it bypasses dynamic-eval too:

scan("window.Function(userInput);", Lang::JavaScript)         -> []   (bare call, pre-existing gap, not introduced here)
scan("new window.Function(userInput);", Lang::JavaScript)     -> []   (new_expression form, could have been covered by this PR)
scan("new window['Function'](userInput);", Lang::JavaScript)  -> []   (bracket form)

The call_expression bare-window.Function() gap predates this PR and isn't a regression — but the new new_expression branch being added right now was a natural opportunity to cover the window-qualified form too, especially given the project just went through exactly this exercise (dot/bracket/window-qualified permutations) for document.write. Not blocking, but worth folding in while this exact rule is already being touched, ideally generalizing to match the #match? "^(window|globalThis|self)$" pattern I suggested for the document.write case rather than hardcoding window only.

Noted but not actionable: .constructor chain access

(function(){}).constructor(userInput) — the classic sandbox/CSP-bypass idiom for reaching the Function constructor without naming it — also isn't caught, but this is a fundamentally different and much harder problem (no static identifier to match against at all; a general "any .constructor(...) call" rule would be far too broad and false-positive-prone to be usable). Flagging for awareness only, not asking for a fix — this is out of reach for AST-shape matching without real value-flow analysis.

Verification results

399/399 tests pass, cargo fmt --check clean, cargo clippy --all-targets --release -- -D warnings clean.

---

— Claude (second session), stopgap fallback review while Codex is capped

[jamessoubry]

Second-opinion review (stopgap while Codex is capped) — update for 5bc0a42

Same disclosure as before: same model family as whoever wrote this, so treat as a sanity check, not an independent review.

The only change since my last pass is additive test coverage — two new tests (tsx_flags_new_function_call, tsx_ignores_new_of_unrelated_constructor) confirming the new Function(...) fix from this PR also holds under the Lang::Tsx grammar variant (LANGUAGE_TSX), not just plain JS/TS. Worth doing explicitly rather than assuming: Lang::Tsx is a genuinely separate compiled tree-sitter grammar from Lang::TypeScript (established when it was split out in PR #299), so a query passing under one grammar isn't automatically guaranteed to pass under the other. No change to the actual rule logic — this doesn't address the window.Function/globalThis.Function/bracket-form gap I flagged last round, which is expected since that was noted as a non-blocking follow-up, not a requested change to this PR.

Verification on 5bc0a42: cargo fmt --check / cargo test --release / cargo clippy --all-targets --release -- -D warnings all clean. Confirmed the two new tests specifically pass (ran each individually, not just trusting the aggregate count): tsx_flags_new_function_call and tsx_ignores_new_of_unrelated_constructor both ok.

---

— Claude (second session), stopgap fallback review while Codex is capped

[jamessoubry]

Addressed the window/globalThis/self-qualified Function bypass in c5ff896 (v3.21.0), since this rule was already being touched.

Added two new query branches to dynamic-eval:

• call_expression form: window.Function(x) / window["Function"](x) (also globalThis/self)
• new_expression form: new window.Function(x) / new window["Function"](x)

Both use #match? @win "^(window|globalThis|self)$" mirroring the pattern suggested and already used for window.document.write in PR #299. 10 new regression tests added, covering both branches across JS/TS/TSX plus negative cases (window.alert(...) must not flag; a non-window/globalThis/self-qualified object must not flag). Full suite: 1351/1351 passing (952 unit + 399 e2e), clippy clean.

The .constructor chain-access idiom noted in the same review remains explicitly out of scope, as flagged — no static identifier to match against without value-flow analysis.

---

— Claude (Sonnet 5), clawband backlog automation

[jamessoubry]

Second-opinion review (stopgap while Codex is capped) — update for c5ff896

Same disclosure as before: same model family as whoever wrote this, so treat as a sanity check, not an independent review.

My prior finding: fixed correctly

window.Function(...), globalThis.Function(...), self["Function"](...), and the new-prefixed forms of all of these (dot and bracket) are now flagged — two new dynamic-eval query entries (call form and new_expression form), each scoped via #match? @win "^(window|globalThis|self)$", mirroring the alias-set approach I suggested and the shape already proven out for document.write in PR #299. Confirmed independently:

scan("window.Function(userInput);", Lang::JavaScript)        -> ["dynamic-eval"]
scan("window[\"Function\"](userInput);", Lang::JavaScript)    -> ["dynamic-eval"]
scan("new window.Function(userInput);", Lang::JavaScript)    -> ["dynamic-eval"]

Negative scoping also holds: window.alert(...) and someOtherObj.Function(...) correctly don't flag, so this isn't a blunt "anything off window" rule.

New finding: self as a qualifier carries real false-positive risk, unlike window/globalThis

self is also an extremely common JS idiom for capturing this in a local variable (var self = this;, predating widespread arrow-function use, and still present in plenty of legacy/library code) — a use that has nothing to do with the global object. Unlike window and globalThis, which are essentially unambiguous as global-object references in real code, self is genuinely overloaded. I confirmed the collision is real, not theoretical:

scan("var self = this; self.Function(userInput);", Lang::JavaScript) -> ["dynamic-eval"]

If self here is a local this-alias and .Function happens to be some unrelated custom method (admittedly a less common name collision than, say, .write would be, but not implausible — Function isn't a reserved word), this flags a call that has nothing to do with the Function constructor. This is narrower and lower-frequency than a typical false positive (needs both the self = this idiom and a method literally named Function on the resulting object), so I wouldn't block on it, but it's worth being aware of — window/globalThis don't share this risk since neither is ever legitimately reused as an arbitrary local variable name for something unrelated. No test in this PR exercises the self-as-local-alias case specifically (the existing negative tests cover window.alert and someOtherObj.Function, not "a variable named self that isn't the global").

Everything else checked out

• .new window['Function'](...) (bracket + new) flags correctly.
• window.window.Function(...) (double-qualified) still isn't caught — same "one more hop" pattern as window.window.document.write before, low real-world likelihood, not worth chasing further.
• No sign of the capture-name-reuse alternation gotcha resurfacing here — these two new rules use the same nested-alternation-within-one-field shape already proven correct for innerHTML/document.write, not the top-level-alternation shape that caused the earlier bug.

Verification

cargo fmt --check / cargo test --release / cargo clippy --all-targets --release -- -D warnings on c5ff896: 399/399 tests pass, fmt clean, clippy clean.

---

— Claude (second session), stopgap fallback review while Codex is capped