A serial number off the bus is data, not code - #196
Open
Timdebruijn wants to merge 3 commits into
Open
Conversation
The dashboard built its per-device buttons as onclick="togglePanel('m:${esc(id)}')",
and esc() does not make that safe. The browser HTML-decodes an attribute value before
it compiles the inline handler, so the ' that esc() produced is an apostrophe again
by the time the JS parser sees it -- the string closes early and whatever follows runs.
A device id is driverId + '-' + the serial number the inverter reported over RS485, and
the only filter on those bytes is isPrintable(), which spans 0x20..0x7E and therefore
includes the apostrophe. A device supplying a crafted serial got script into the admin's
authenticated session, where sessionStorage holds the Basic-auth token -- which unlocks
read-only mode and factory reset. It needs a malicious or spoofed unit on the bus, not a
remote attacker; bus noise landing an apostrophe would merely break the button.
All nine interpolated handlers now pass their value as a data-* attribute, read back with
getAttribute -- which yields the decoded string and never executable text. Two of the nine
were exploitable (the readings and settings buttons); the rest carried loop indices and
literals and were safe, but the shape was the defect, so the shape is gone. One delegated
listener on document replaces them and survives every innerHTML repaint, which per-element
handlers did not.
Two gates keep it that way, both proven by breaking the code and watching them fail:
check_web_js.py rejects ${...} inside any on* attribute, in both quote styles. It is
shape-based rather than taint-based on purpose -- proving a given value safe means
reading code, and these pages are re-authored from a design tool often enough that
"someone will notice" is not a control.
check_dashboard_layout.py renders a fleet whose EverSolar reports the serial
x');window.__pwned=1;// and asserts both halves: the payload does not run, AND the
button still opens its panel. Either alone passes for the wrong reason -- escaping
harder satisfies the first, deleting the button satisfies both and ships a dead
dashboard. With the old onclick restored alongside the new attribute it fails with
"the payload ran when the button was clicked", so it detects execution rather than a
missing attribute.
Neither gate covered any of this before: the delegation could have been dead in all nine
places and CI would have stayed green.
Review of the first commit found the new check was anchored to HOW the code is
written rather than to whether a dynamic value reaches a handler. Four bypasses,
all confirmed by writing them and watching the check pass:
onclick="f('m:'+esc(id))" string concatenation -- identical vulnerability, no ${
ONCLICK= / onDblClick= attribute names are case-insensitive to the browser
onclick=f(${x}) an unquoted attribute value is legal HTML
a value wrapped over lines the check read one line at a time
The rule is now what it always claimed to be: an inline handler's value must be a
static quoted literal -- no interpolation, no concatenation, no backticks. All four
shapes are proven to fail the check, and the clean tree passes.
Anchored to the HTML event-handler names rather than to /on[a-z]+/, because the
latter matches `online=${f.online}` in a template literal and this page has one. A
lookbehind keeps JS property assignments out: `dlg.onclose=()=>{...}` assigns a
function object and compiles no string, so it is not this defect.
Separately, the release-notes link. esc() makes a URL safe to sit in an attribute; it
does not make it safe to follow, and javascript: survives HTML-escaping intact. That
URL comes from the update feed rather than the RS485 bus, so it is outside the threat
model this branch is about -- but it is the same mistake one layer over, and it costs
six lines to not have. Only http(s) is linked; anything else renders as plain text,
so the reader still sees what the feed offered without the page offering to run it.
The nullish guard in safeUrl() is load-bearing: without it String(null) resolves
against location.href to a perfectly valid http URL, and an absent notes_url would
have rendered a link reading "null" where it previously rendered nothing.
Review ran check_dashboard_layout.py on this branch and it failed. I had run it too --
and read `tail -2`, which showed the last two checks passing. The script prints no
verdict, so a failure in the middle of the list is invisible from the end of the output.
check_web_js.py grew a RESULT line for exactly this reason, and says so in its own
source: "a failing check dumps node's stderr after its own FAIL line, and any tail of
this output then shows a later check's OK. That is not hypothetical -- a real failure
was read as green that way (2026-07-29)." Its sibling never got the fix. Now it has one,
and a mid-list failure is provable from the last line.
WHAT WAS ACTUALLY BROKEN. The inverters-tab scenario looked for buttons by
getAttribute('onclick') matching /removeExtraAt/ and /togglePanel\('x:/. This branch
replaced those attributes with data-act, so the assertions could no longer find the
buttons -- and could no longer tell a working feature from a missing one. The feature
was fine; the gate had stopped being one. Updated to read data-act/data-val.
THE HANDLER CHECK, three ways round it, all found by review and all now closed:
The allowlist of event names was missing onbeforetoggle, onmessageerror,
onanimationcancel, onbeforematch, oncontextlost, onrejectionhandled, ontransitionrun
and the SVG SMIL handlers -- so the original vulnerability, written with any of those,
passed. It is a denylist now: match any on-word, minus the ones that are not handlers.
That set has one entry today (online=) and does not grow when HTML adds events, which
an allowlist has to. A false positive is a loud CI failure; a false negative is script
in someone's session.
data-onclick="reindex(pos+1)" was flagged as a handler. The lookbehind now excludes a
preceding hyphen.
A comment describing the old bug was flagged as the bug. JS comment lines are skipped.
AND THE PART A SHAPE CHECK CANNOT DO. Review evaded it with Array.join(), building the
same attribute without ever writing ${. That is not fixable by widening the pattern --
there is always another way to concatenate. So the real defence no longer depends on how
the source is spelled: the rendered page is now scanned for any on* attribute carrying
the hostile serial, in the DOM, after Chrome has parsed it. Proven by making only the
settings button vulnerable and watching it name the attribute and print the payload.
That scenario also clicks the settings button now. The commit that started this branch
said both buttons were exploitable and then only ever tested one of them.
Timdebruijn
marked this pull request as ready for review
August 28, 2026 23:08
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
The dashboard built its per-device buttons as
onclick="togglePanel('m:${esc(id)}')".esc()does not make that safe: the browser HTML-decodes an attribute value before compiling the inline handler, so the'it produced is an apostrophe again by the time the JS parser sees it. The string closes early and what follows runs.A device id is
driverId + '-' + serialNumber, and the serial comes off RS485 filtered only byisPrintable()—0x20..0x7E, which includes'. A crafted serial put script in the admin's authenticated session, wheresessionStorage.hg_authholds the Basic-auth token.Threat model, stated fairly: this needs a malicious or spoofed device on the bus, not a remote attacker. Bus noise producing an apostrophe would break the button rather than exploit it. It is still the one place on the page where bus bytes reached executable text.
How
All nine interpolated handlers now pass their value as a
data-*attribute read back withgetAttribute, which yields the decoded string and never executable text. One delegated listener replaces them and survivesinnerHTMLrepaints, which per-element handlers did not.Two were exploitable (readings + settings buttons). The other seven carried loop indices and literals and were safe — but the shape was the defect, so the shape is gone everywhere.
Gates, both mutation-proven
check_web_js.py${...}inside anyon*attribute, both quote stylescheck_dashboard_layout.pyx');window.__pwned=1;//The layout scenario asserts both halves — the payload must not run, and the button must still open its panel. Either alone passes for the wrong reason. With the old
onclickrestored alongside the new attribute it fails withthe payload ran when the button was clicked, so it detects execution, not a missing attribute.Neither gate covered any of this before. The delegation could have been dead in all nine places and CI would have stayed green — verified by breaking it.
Verification
check_web_js.py,check_dashboard_layout.py,check_layering.sh,ruff check+ruff format --check(against the pinned 0.16.1, not just local 0.15.18).Note for whoever re-syncs the design project
index_html.his the source of truth; the design tool is upstream of it. The newcheck_web_js.pyrule is what stops a future splice from quietly reintroducing the pattern.