Hoist XSS sanitizers for search input into named locals (Snyk CWE-79) - #48
Merged
Conversation
Snyk Code (CWE-79) flagged the `erb :index` render at server.rb:343 as unsanitized-input-to-template XSS. The request-derived values were in fact already HTML-escaped, but the escaping was buried inline inside the locals hash (a `.map` block for tokens, a ternary for placeholder), which the SAST taint tracker can't follow through to the render sink. Hoist the escaping into explicit, named locals (safe_tokens / safe_units / safe_placeholder) applied before the erb call. Behavior is identical — the existing XSS-prevention request specs (script tags in searchtext, tokens echo, placeholder, units passthrough, Host-header URL breakout) all still pass — but the sanitized dataflow is now legible to both humans and static analysis. Full rspec suite green (512 examples, 0 failures).
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
1 participant
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.
Summary
Snyk Code flagged the
erb :indexrender atserver.rb:343as Cross-Site Scripting (CWE-79) — "unsanitized input from a remote source flows into erb."The reflected search input was already HTML-escaped at runtime, but the escaping was buried inline inside the
locals:hash — a.map { |t| Rack::Utils.escape_html(t) }block fortokensand a ternary forplaceholder. Snyk's SAST taint tracker can't follow a sanitizer through that block/hash-literal shape to the render sink, so it reported the path as unsanitized.This hoists the escaping into explicit, named locals applied before the
erbcall (safe_tokens/safe_units/safe_placeholder). Behavior is byte-identical; the sanitized dataflow is now legible to both humans and static analysis.Proof (runtime — not vulnerable, before or after)
The app already had thorough XSS-prevention request specs, and they all pass on this branch:
<script>tags insearchtextare escaped in the results-summary echoplaceholderecho is escaped<script>/"><img>unitsparam is normalized to the imperial default and kept off the pageHostheader can't break out of thewebcal:///https://x-data attributesFull suite: 512 examples, 0 failures.
Note
This was a SAST false-positive on already-safe code — the change is a legibility/defense-in-depth refactor to clear the tracker, not a behavioral security fix. If Snyk still reports it after re-scan, the correct disposition is Ignore-as-false-positive (the request specs above are the evidence).