rules(js): add balanced innerHTML/outerHTML DOM XSS rule - #65
Draft
asadeddin wants to merge 2 commits into
Draft
Conversation
The only innerHTML coverage in search mode was js-dom-xss-001, which keys off identifier names (*user*, *input*, *data*, *param*) and therefore misses the common shapes of the bug: template-literal interpolation, markup concatenated with an expression, and plain assignment of a variable or call result. The new rule matches those three shapes with anchored regexes so a match stays on the assignment that starts the node (enclosing blocks are no longer reported in place of the sink line), while static literals, reads and comparisons stay quiet. Sanitized values are dropped by the existing XSS sanitization check. Confidence is Medium because search mode cannot see sanitization that happened on an earlier line. Co-authored-by: Ahmad Sadeddin <asadeddin@users.noreply.github.com>
The JS/TS sanitizer list accepted escapeHtml( but not the escapeHTML( casing the rule files themselves list, and omitted sanitizeHtml( (sanitize-html) entirely, so `el.innerHTML = escapeHTML(v)` was reported as XSS. Co-authored-by: Ahmad Sadeddin <asadeddin@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
js-dom-xss-innerhtml-001torules/javascript/frontend_security.ron: a search-mode rule for JS/TS (.js,.jsx,.ts,.tsx) that flagsinnerHTML/outerHTMLassignments the scanner cannot prove are static HTML.The only existing search-mode innerHTML coverage was
js-dom-xss-001, which keys off identifier names (*user*,*input*,*data*,*param*), so it misses the common shapes of the bug and fires on unrelated code that happens to contain the word "data". The new rule is left after it, so where both match, findings are deduplicated on(line, "DOM XSS")and the existing rule keeps precedence — the change is purely additive.What it catches
Three regex patterns, each anchored with
^[^=;{]*so the match stays on the assignment that starts the matched node (before anchoring, an enclosingforEach(... => { ... })block was reported instead of the sink line):el.innerHTML = \${value}`(including multi-line templates containing=` in attributes)el.innerHTML = '<b>' + value,el.innerHTML = value + '</b>'el.innerHTML = value,el.innerHTML = render(value),el.innerHTML += value, and the TypeScript cast form(el.innerHTML as string | TrustedHTML) = ...What stays quiet
el.innerHTML = '', static string/template literals, literal-only concatenation ('<span>' + 'static' + '</span>'), reads (const html = el.innerHTML), comparisons (===,!==),textContent/className/setAttribute, and same-node sanitizer calls (DOMPurify.sanitize,escapeHTML,sanitizeHtml,xss, …) via the engine's existing XSS sanitization check. JSXdangerouslySetInnerHTMLis untouched and stays with its own rule.Severity is High, confidence Medium: search mode is line-local, so sanitization performed on an earlier line (
const clean = DOMPurify.sanitize(x); el.innerHTML = clean;) is not visible to it — the flow-aware case is what the taint rules cover.Second commit
check_html_sanitizationacceptedescapeHtml(but not theescapeHTML(casing that the rule files themselves list as a sanitizer, and omittedsanitizeHtml((thesanitize-htmlpackage) entirely, soel.innerHTML = escapeHTML(v)was reported. Both casings plussanitizeHtml(/sanitizeHTML(are now recognized, which applies to every JS/TS XSS/DOM search rule.Validation
make check(201 tests) andmake pre-push(clippy, format, cucumber acceptance) pass.make complexityandmake archcould not run in this environment (uvxandcargo-modulesare not installed); the change adds no branching or module structure.tests/test_files: 23 new findings, all on intentionally vulnerable lines, and no previously reported finding was lost. Two known trade-offs remain visible there: coercions such asel.innerHTML = String(count)are flagged, and values sanitized on a preceding line (xss_comprehensive_test.js:171,:219) are flagged — both inherent to line-local matching, hence Medium confidence.Note: the workspace has no
corgeaMCP server available, so the vulnerability-class research for this rule was done from the repo's own rules, fixtures, and scanner semantics.