Fix XSS vulnerability (CWE-79) via globally disabled HTML escaping in HAML - #443
Merged
Merged
Conversation
…plates
Enable escape_html: true globally in webapp.rb (was false), which HTML-escapes
all #{} interpolations in HAML templates. This fixes reflected XSS via URL
parameters (node, group, num, node_full) and stored XSS via node/model/group
names from the Oxidized configuration.
Two follow-on fixes prevent regressions from the global change:
- layout.haml: change =yield to != yield so the pre-rendered child template
HTML is not double-escaped
- node.haml: change =escape_once(...) to != escape_once(...) so the already
HTML-encoded JSON output is not double-encoded
Adds four security regression tests in spec/web/xss_spec.rb covering the
reflected and stored XSS vectors.
Fixes CWE-79 / CVSSv4.0 6.9 (reflected) and 5.3 (stored).
Contributor
Author
|
Hi @ytti, just checking on the status of this security fix? |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Oxidized Sinatra/HAML web UI against CWE-79 (XSS) by enabling global HAML escaping and adjusting a couple of templates to avoid double-escaping, along with adding regression tests to prevent reintroduction.
Changes:
- Enable global HAML
escape_htmlinWebAppto ensure interpolated values are HTML-escaped by default. - Update
layout.hamlandnode.hamlto use!=where content is already-rendered/escaped to prevent double-encoding. - Add security regression specs covering reflected and stored XSS scenarios, and document the fix in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/web/xss_spec.rb | Adds regression coverage for reflected and stored XSS in key endpoints/views. |
| lib/oxidized/web/webapp.rb | Enables global HAML HTML escaping to close template-level XSS vectors. |
| lib/oxidized/web/views/node.haml | Prevents double-escaping for already-escaped JSON content display. |
| lib/oxidized/web/views/layout.haml | Prevents double-escaping of rendered child template output. |
| CHANGELOG.md | Documents the security fix and added regression tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+69
| it 'escapes node names in /nodes list' do | ||
| malicious = '<script>alert(1)</script>' | ||
| @nodes.expects(:list).returns( | ||
| [{ name: malicious, ip: '10.0.0.1', model: 'ios', | ||
| full_name: malicious, group: 'default', time: Time.now, mtime: Time.now }] | ||
| ) | ||
|
|
||
| get '/nodes' | ||
|
|
||
| _(last_response.ok?).must_equal true | ||
| _(last_response.body).must_include('<script>alert(1)</script>') | ||
| _(last_response.body).wont_include('<script>alert(1)</script>') | ||
| end |
Collaborator
|
I hope more from the AI Review, including a possible regression analysis 😅 I don't understand how we could have a XSS here - if the node name is not stored in oxidized, we will get a Oxidized::NodeNotFound, not the malicious code. Anyway, setting |
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.
Summary
escape_html: trueglobally inwebapp.rb(root fix); all#{}interpolations in HAML now auto-escape, closing reflected XSS via URL parameters (node,group,num,node_full) and stored XSS via node/model/group nameslayout.haml: change=yieldto!= yieldso the pre-rendered child template HTML is not double-escaped by the global settingnode.haml: change=escape_once(...)to!= escape_once(...)so already HTML-encoded JSON output is not double-encodedspec/web/xss_spec.rbcovering reflected XSS on/node/version,/node/version/view,/node/version/diffs, and stored XSS on/nodesFixes CWE-79 / CVSSv4.0 6.9 (reflected XSS) and 5.3 (stored XSS) as reported in the security vulnerability report.
All existing tests continue to pass (39 pre-existing + 4 new = 43 total).
Notes on existing
!=usageAll pre-existing
!=operators in templates are intentional and remain safe:version.hamlanddiffs.haml:!= escape_once("#{line}")renders content that has already been throughHTMLEntities.new.encode()—!=is required to avoid double-encoding the HTML entitieslayout.haml:!=haml :headand!=haml :footerrender trusted internal HAML partialsTest plan
rake test— 43 runs, 0 failures