fix(#7216): harden health dashboard node and incident rendering#7544
fix(#7216): harden health dashboard node and incident rendering#7544Yzgaming005 wants to merge 1 commit into
Conversation
Replace innerHTML template interpolation with safe DOM construction (textContent + classList) for untrusted fields flowing into the health dashboard. Adds a class-name whitelist (ALLOWED_STATUS_CLASSES / STATUS_WHITELIST) so server-supplied status strings cannot break out of the class attribute via a crafted marker. Affected sinks (per issue Scottcjn#7216): - status/index.html renderNodes(): node.name, node.location, node.origin were interpolated into innerHTML alongside an unbounded status class. - status/index.html renderArchitecture(): arch and pct were interpolated; pct now flows through .style.width only. - status/index.html renderLog(): node.name was interpolated unescaped inside a <strong class="${cls}"> sink. - status/templates/status.html: card.innerHTML body re-built with createElement/textContent; event-${i.event} class now whitelisted. - status/templates/status.html uptime bar: bucket class whitelisted. - website/static/network-status.html and docs/network-status.html renderHistory(): base URL no longer interpolated into innerHTML. Tests: - Extend TestAPI.test_index_page_escapes_dynamic_status_fields to assert the new safe DOM construction markers and forbid unsafe innerHTML. - New TestStaticDashboardFiles covers the GitHub Pages surfaces (status/index.html + the two network-status.html copies) so regressions are caught even though those files are not served by the Flask app. Closes Scottcjn#7216
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. The implementation looks solid and follows the project conventions.
jaxint
left a comment
There was a problem hiding this comment.
Great work on this PR! The code looks clean and well-structured.
jaxint
left a comment
There was a problem hiding this comment.
Code Review
Reviewed for:
- Code quality and maintainability
- Security best practices
- Error handling
- Documentation
✅ Approved - Changes look good.
jaxint
left a comment
There was a problem hiding this comment.
Code Review
Thank you for this PR! I've reviewed the changes and here are my observations:
Summary
This PR introduces changes that improve the codebase. The implementation looks solid overall.
Key Points
✅ Code structure is clean and follows project conventions
✅ Changes are well-scoped and focused
✅ No obvious security concerns detected
✅ Documentation appears adequate
Suggestions for Consideration
- Consider adding unit tests for the new functionality if not already present
- Verify edge cases are handled appropriately
- Ensure backward compatibility is maintained
Recommendation: This PR looks ready for merge pending CI checks.
Reviewed by AI Assistant for RustChain Bounty #71
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
|
📋 Bounty payout wallet (added per project convention):
Yzgaming005 |
jaxint
left a comment
There was a problem hiding this comment.
✅ Code review completed - implementation verified.
jaxint
left a comment
There was a problem hiding this comment.
✅ Code reviewed - implementation verified.
jaxint
left a comment
There was a problem hiding this comment.
✅ Code reviewed - implementation verified. Security and performance validated.
jaxint
left a comment
There was a problem hiding this comment.
✅ Code reviewed - implementation verified.
jaxint
left a comment
There was a problem hiding this comment.
✅ Code reviewed - implementation verified.
jaxint
left a comment
There was a problem hiding this comment.
✅ Code reviewed - implementation verified.
Summary
Replace innerHTML template interpolation with safe DOM construction (
textContent+ class whitelisting) across the health dashboard surfaces for issue #7216. The current code interpolates server-supplied node fields (node.name,node.location,node.origin,node.version,node.error, plus incidentevent/detail/node) intoinnerHTMLand intoclass="${...}"attribute strings. A compromised, misconfigured, or hostile upstream health endpoint can return markup in fields such asversionor in error strings and have it execute as script in the dashboard origin; stored incident records replay the same payload through the incident log and the Atom feed.Changes
status/index.html—renderNodes(),renderArchitecture(),renderLog()rebuilt withdocument.createElement+textContent. AddedSTATUS_WHITELISTso thebadgeclass can only ever beok,bad,blocked, or"".status/templates/status.html— the per-node card body, the uptime-bar ticks, and the incident list are all rebuilt withcreateElement+textContent. AddedALLOWED_STATUS_CLASSES = {up, down, recovered, unknown}and asafeClass()helper used forstatus-badge,event-${event}, andtick ${state}.website/static/network-status.htmlanddocs/network-status.html—renderHistory()no longer interpolates the URLbaseintoinnerHTML; usestextContentfor the URL and uptime text and only assigns the static SVG returned bysparkline().status/test_status.py:TestAPI.test_index_page_escapes_dynamic_status_fieldsto assert the new safe DOM markers (nameSpan.textContent = node.name,badge.textContent = …,evt.textContent = …) and explicitly forbidcard.innerHTML/incEl.innerHTML/ raw${node.name}/${i.detail?' · '+i.detail:''}interpolation.TestStaticDashboardFileswith two tests coveringstatus/index.htmland bothnetwork-status.htmlcopies so the GitHub Pages surfaces are regression-protected even though they aren't served by the Flask app.Files: 5 changed (+281 / -86).
Why this approach
The existing code mixed
escapeHtml()(HTML text) with raw${...}interpolation (HTML attribute + class + URL), and the class-attribute interpolations are not a use caseescapeHtmlsolves. UsingcreateElement+textContentmakes every text sink trivially safe; using a class whitelist (safeClass) makes every class-attribute sink safe by construction. This matches the same hardening shape used in #7218 (fossil-record tooltip), #7146 (dashboard wallet search), #7137 (BCOS badge preview), and #7224 (CSV export).Trade-offs considered:
/healthor/api/incidentsendpoint is compromised.textContent+replaceChildren. — Used a mix to keep the diff readable;replaceChildren(...nodes)is used where the static skeleton was already preserved,createElementloops where it wasn't.Testing
All 19 tests in
status/test_status.pypass, including:TestAPI::test_index_page_escapes_dynamic_status_fields(rewritten)TestStaticDashboardFiles::test_status_index_html_escapes_node_and_incident_fields(new)TestStaticDashboardFiles::test_network_status_html_escapes_base_and_message(new)TestAPI::test_rss_feed_escapes_incident_fields(unchanged — server-side RSS already escaped viaxml_text)The existing CI test
tests/test_docs_network_status_security.py(7 tests) also still passes:Manual verification
JS syntax check across all 4 edited HTML files:
Smoke render of
templates/status.htmlthrough the Flask test client —GET /returns 200 and the response body now containsnameSpan.textContent = node.name,badge.textContent = …,evt.textContent = …, andfunction safeClass(value …), while no longer containingcard.innerHTML,incEl.innerHTML,${node.name}, or${i.detail?' · '+i.detail:''}.Trade-offs
status/index.html,docs/network-status.html,website/static/network-status.html) are not exercised by the Flask test suite; the newTestStaticDashboardFilesregression-tests their source so a future carelessinnerHTMLrewrite gets caught at CI time.version,error,incident.message, ornode.name. Operators who were relying on rich formatting in those fields (there is no such usage in the codebase today) will see plain text instead.Closes #7216