Skip to content

ast_guard: add JS/TS XSS-sink rules - #299

Merged
jamessoubry merged 3 commits into
masterfrom
feat/xss-sink-rules
Sep 23, 2026
Merged

jamessoubry merged 3 commits into
masterfrom
feat/xss-sink-rules

Conversation

@jamessoubry

@jamessoubry jamessoubry commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Summary

Adds AST-based XSS-sink detection for JS/TS to ast_guard:

  • innerHTML / outerHTML assignment detection — flags direct assignment of untrusted/dynamic values to .innerHTML or .outerHTML on DOM elements.
  • insertAdjacentHTML / document.write call detection — flags calls to these DOM sinks, which parse and execute markup the same way innerHTML does.
  • dangerouslySetInnerHTML JSX attribute detection — flags the React escape hatch for raw HTML injection, via a new Lang::Tsx tree-sitter grammar variant added to support JSX/TSX parsing (previously only plain .ts/.js were parsed without JSX support).

Together these close common XSS injection vectors that the existing ast_guard rule set didn't cover for JS/TS/JSX/TSX sources.

Version bumped to v3.19.0.

Closes #262

Test plan

  • Unit tests added in src/main.rs (#[cfg(test)] mod tests) covering each new sink pattern
  • cargo test passes
  • cargo clippy --all-targets -- -D warnings clean
  • cargo fmt --check clean

(Verified in a prior pipeline phase; not re-run here.)


— Claude (Sonnet 5), clawband backlog automation

RetriggerConfidence Score: 5/5

Safe to merge.

Summary

Improves the JavaScript and TypeScript AST guard’s XSS sink coverage, including window-qualified document.write access and bracket-property variants.

Reviews (3) · Last reviewed commit: "[backlog] Fix window.document.write XSS ..."

…sertAdjacentHTML/document.write/dangerouslySetInnerHTML) (v3.19.0)

Adds a new `xss-sink` AST rule covering five browser/React sinks in JS/TS:
`.innerHTML =`/`.outerHTML =` assignment (a new assignment_expression query
shape, distinct from the existing call-expression rules), `.insertAdjacentHTML(...)`,
`document.write(...)`, and React's `dangerouslySetInnerHTML` JSX attribute.

None of the four non-JSX sinks narrow on the assigned/passed value — matching
the upstream security-guidance reference, which flags unconditionally (gated
only by file extension, not value).

dangerouslySetInnerHTML required adding a new Lang::Tsx variant wired to
tree_sitter_typescript::LANGUAGE_TSX: confirmed empirically that plain
LANGUAGE_TYPESCRIPT (used for .ts) has zero JSX node kinds, so a jsx_attribute
query can't even compile against it, while tree-sitter-javascript's default
grammar already parses JSX out of the box for .js/.jsx/.mjs/.cjs. .ts is
therefore excluded from this one sub-rule (genuinely unreachable, not just
unlikely), while .tsx gets full coverage via the new LANGUAGE_TSX grammar.

Closes #262
@deepsource-io

deepsource-io Bot commented Sep 20, 2026 •

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in a3fce6b...3a6f5a7 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 21, 2026 6:35a.m. Review ↗
Rust Sep 21, 2026 6:35a.m. Review ↗
Shell Sep 21, 2026 6:35a.m. Review ↗
Secrets Sep 21, 2026 6:35a.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.

Comment thread src/ast_guard.rs
@greptile-apps

This comment has been minimized.

…tation property access (v3.19.1)

Greptile's review on PR #299 (P1) found that el.innerHTML += x,
el["outerHTML"] = x, el["insertAdjacentHTML"](...), and
document["write"](x) all bypassed xss-sink detection while their
dot-notation equivalents were correctly flagged. The queries only
matched assignment_expression/member_expression shapes.

Added augmented_assignment_expression query variants for the
innerHTML/outerHTML compound-assignment (+=) case, and
subscript_expression query variants (alternated alongside the
existing member_expression branches) for bracket/computed property
access, covering innerHTML/outerHTML assignment, insertAdjacentHTML
calls, and document["write"] calls. document["write"] stays scoped
to the document object specifically, mirroring the existing
document.write scoping — foo["write"](x) is confirmed not to fire.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jamessoubry

Copy link
Copy Markdown
Owner Author

Fixed in 84eba89 (v3.19.1).

The gap was that the xss-sink queries only matched assignment_expression/member_expression shapes, so two other tree-sitter node kinds weren't covered:

  • el.innerHTML += attackerHtml / el.outerHTML += attackerHtml — compound assignment is a distinct augmented_assignment_expression node (not assignment_expression), with its own left/operator/right fields. Added dedicated query variants for it.
  • el["outerHTML"] = attackerHtml, el["insertAdjacentHTML"](...), document["write"](attackerHtml) — computed/bracket property access is a subscript_expression node (not member_expression), with object/index fields instead of object/property. Added subscript_expression alternatives (matching a (string (string_fragment)) literal at the index field) alongside the existing member_expression branches for all four sinks.

document["write"] stays scoped to the document object specifically (#eq? @obj "document"), same as the pre-existing document.write dot-form rule — added a negative test confirming foo["write"](x) does not fire.

All four bypass forms from the finding are now covered by unit tests (src/ast_guard.rs) and e2e tests (tests/cli.rs); full suite (cargo test, cargo clippy --all-targets -- -D warnings) passes clean with no regressions.


— Claude (Sonnet 5), clawband backlog automation

@jamessoubry

Copy link
Copy Markdown
Owner Author

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

Adds a new xss-sink rule for JS/TS/TSX (issue #262): .innerHTML/.outerHTML assignment (plain and compound +=), .insertAdjacentHTML(...), document.write(...), and React's dangerouslySetInnerHTML JSX attribute. It also splits Lang::TypeScript into TypeScript and a new Tsx variant, since tree-sitter-typescript ships JSX support as a genuinely separate grammar (LANGUAGE_TSX vs LANGUAGE_TYPESCRIPT) — confirmed correct by checking that no other file in the codebase pattern-matches on Lang:: (only ast_guard.rs does, and main.rs just calls detect_language/scan without matching variants), so the enum split can't have silently broken a match arm elsewhere. This PR already includes a follow-up commit fixing two P1 Greptile findings — compound-assignment (+=) and bracket-notation (el["innerHTML"]) bypasses of the original rule — with dedicated regression tests for both.

Finding: window.document.write(...) bypasses the document.write rule (new, not addressed by this PR's own fix round)

The document.write query requires the call's object to be a bare identifier equal to "document" (both the dot-access and the bracket-access forms added in the fix commit use object: (identifier) @obj / (#eq? @obj "document")). I tested the realistic, common pattern of qualifying document off window — which some codebases do deliberately, e.g. to disambiguate from a shadowed local document variable — and confirmed it slips through entirely:

scan("window.document.write(userInput);", Lang::JavaScript) -> []   (not flagged)
scan("document.write(userInput);", Lang::JavaScript)        -> ["xss-sink"]  (flagged, for comparison)

This is the same class of bypass (a chained-member-access route to the sink) that this PR's own fix round already patched for the bracket-notation case — just one hop further along the same access-path axis, and it's a genuinely common real-world spelling, not a contrived one. Worth adding a window-qualified variant (object is a member_expression with object: (identifier) @win, property: (property_identifier) @doc where @win is "window" and @doc is "document") alongside the existing two forms, mirroring how this PR already handles bracket vs. dot access.

Smaller, lower-priority observation

el[innerHTML] = x (template-literal/backtick bracket key with no interpolation) isn't matched — the subscript-index query only accepts a plain string node, not template_string. Confirmed via probe. Much lower real-world likelihood than the window.document case above (nobody writes a static bracket key as a backtick string), so not blocking — just noting for completeness since I was already probing bracket-access edge cases.

Everything else checked out

  • Optional chaining (el?.insertAdjacentHTML(...)) still matches correctly — confirmed via probe, the member_expression query shape isn't broken by the optional-chain grammar variant.
  • The Lang::Tsx split is sound: verified detect_language/scan are the only call sites outside ast_guard.rs (main.rs doesn't match on Lang variants), so nothing elsewhere could have silently mis-routed .tsx files after the split.
  • dangerouslySetInnerHTML correctly excluded from plain .ts (real grammar limitation, not a design choice — confirmed by the PR's own doc comment reasoning, which matches how tree-sitter-typescript actually ships its two grammar variants).
  • document.writeln correctly NOT flagged (distinct method, exact-match query) — good, avoids a plausible false positive.
  • The already-fixed compound-assignment and bracket-notation bypasses (Greptile P1s from this PR's own review round) check out: both have dedicated tests and I didn't find a way around either.

Verification

cargo fmt --check / cargo test --release / cargo clippy --all-targets --release -- -D warnings on 84eba89: 395/395 tests pass, fmt clean, clippy clean.


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

Second-opinion review on PR #299 found that window.document.write(...)
bypassed the xss-sink document.write rule, since it required the call's
object to be a bare identifier equal to "document" (both the dot-access
and bracket-access forms). Add a window-qualified variant matching a
call whose object is itself a member_expression (or subscript_expression)
of shape window.document, covering window.document.write(...),
window["document"].write(...), window.document["write"](...), and
window["document"]["write"](...), while keeping the rule scoped to
exactly "window" as the outer object so unrelated identifiers like
someOtherWindow or foo don't false-positive.
@jamessoubry

Copy link
Copy Markdown
Owner Author

Fixed — added a window-qualified variant of the document.write xss-sink query. The call's object is now matched as either a bare identifier document (existing) or a member_expression/subscript_expression of shape window.document (new), with the outer object strictly required to equal window via #eq? @win "window". This catches window.document.write(...) plus the bracket permutations (window["document"].write(...), window.document["write"](...), window["document"]["write"](...)) for symmetry with the existing dot/bracket coverage, while confirming someOtherWindow.document.write(x) and foo.document.write(x) still do NOT flag (negative tests added).

Covered by new unit tests in src/ast_guard.rs and e2e tests in tests/cli.rs (397 tests passing, clippy clean). Bumped to v3.19.2, commit 3a6f5a7 pushed to this branch.


— Claude (Sonnet 5), clawband backlog automation

@jamessoubry

Copy link
Copy Markdown
Owner Author

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

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 and correctly

window.document.write(...) (and all four dot/bracket permutations of it) is now flagged, with negative tests confirming the fix is properly scoped — someOtherWindow.document.write(...) and foo.document.write(...) correctly do not flag, so this isn't a blunt "any .document.write anywhere" rule; it's specifically document.write bare or qualified through exactly window. That scoping choice is the right call: a generic "any X.document.write" match would false-positive on an unrelated object that happens to have its own .document property (e.g. a non-DOM model object), which the foo.document.write negative test is clearly guarding against.

Confirmed independently rather than just trusting the new tests:

scan("window.document.write(userInput);", Lang::JavaScript)         -> ["xss-sink"]   (fixed)
scan("window[\"document\"][\"write\"](userInput);", Lang::JavaScript) -> ["xss-sink"]   (bracket permutation also fixed)

New, smaller finding: other standard global aliases for window still bypass

The fix scopes the outer object to exactly "window" via #eq?. I probed the other common ways JS code refers to the global object, none of which are exotic or contrived — globalThis is the ES2020+ standardized cross-environment alias (arguably more idiomatic than window in modern/isomorphic code, since it also works in Node and web workers where window doesn't exist), and self is the standard self-reference in Window/Worker contexts:

scan("globalThis.document.write(userInput);", Lang::JavaScript) -> []   (not flagged)
scan("self.document.write(userInput);", Lang::JavaScript)       -> []   (not flagged)

This is the same bypass shape that was just fixed for window, just via a different (equally standard) spelling of the same global object. Given the design intent is clearly "recognize document via its known global-object aliases, not just the bare name," the cleanest fix is to broaden the existing #eq? @win "window" to a #match? regex over the known alias set (e.g. ^(window|globalThis|self)$), mirroring how this same file already handles multi-alias matching elsewhere (e.g. pandas|pd, ET|ElementTree|cElementTree) — rather than adding a third near-duplicate query block per alias. Not blocking (same severity class as the window gap I found last time, now smaller in likely real-world frequency), but worth folding in while this exact code path is already being touched.

Verification

cargo fmt --check / cargo test --release / cargo clippy --all-targets --release -- -D warnings on 3a6f5a7: 397/397 tests pass, fmt clean, clippy clean.


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

@jamessoubry
jamessoubry merged commit 692ad87 into master Sep 23, 2026
7 checks passed
jamessoubry added a commit that referenced this pull request Sep 24, 2026
…(v3.21.0)

Addresses a second-opinion review finding on PR #301: window.Function(...),
new window.Function(...), and new window['Function'](...) all bypassed the
dynamic-eval rule, the same qualification-bypass class already fixed for
document.write in PR #299. Both the pre-existing call_expression branch and
the new_expression branch added for issue #263 required a bare `identifier`
named Function, missing the window/globalThis/self-qualified forms.

Adds two new query branches (call and new-expression forms, mirroring the
window.document.write fix's #match? "^(window|globalThis|self)$" pattern)
plus 10 regression tests covering both branches across JS/TS/TSX and the
negative cases (unrelated window methods, unrelated qualifying objects).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0187k8QeNYjgcEGv74YFdh2J
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ast_guard: add JS/TS XSS-sink rules — innerHTML/outerHTML/insertAdjacentHTML/document.write/dangerouslySetInnerHTML

1 participant