Backport 5 fixes already live in the website's vendored copy#1
Open
balloob wants to merge 1 commit into
Open
Conversation
These fixes were made in the opendisplay.org website's vendored copy of nrf_web_tools.js but never flowed back upstream to this standalone repo. Backporting them here so both copies match. 1. XSS: DFU error page built HTML via innerHTML interpolating data.message/data.details (a crafted DFU-zip manifest filename could inject markup). Now set via textContent. 2. HCI sequence-number reset on ACK timeout: waitForAck reset hciSequenceNumber = 0 mid-transfer, desyncing the reliable-transport counter so one lost ACK could wedge the rest of the transfer. Removed. 3. ACK-listener race: sendPacket wrote before arming waitForAck, so an ACK landing immediately after the write could be dropped. Arm the ack promise before writing. 4. disconnectPort() threw before closing: called releaseLock() on ReadableStream/WritableStream (which have no such method), so port.close() never ran and this.port went stale, blocking reconnect. Rewrote with per-step try/catch and a finally that always nulls fields. 5. Cancelled-picker misdetection: checked e.message for 'NotFoundError', but a cancelled port picker throws a DOMException carrying that in e.name. Check e.name instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR
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.
The website (opendisplay.org) ships a vendored copy of
nrf_web_tools.jsthat received five bug fixes which never flowed back upstream to this standalone repo. This PR backports those five fixes so both copies match. Each change below was already validated and running in the website copy; after applying them the two files are byte-identical.Fixes
XSS in the DFU error page.
renderErrorPagebuilt its HTML withinnerHTML, interpolatingdata.messageanddata.detailsdirectly. Those strings can include a filename taken from a loaded DFU-zip manifest, so a crafted package could inject markup into the DOM. The message/details are now written viatextContentinto empty placeholder elements.HCI sequence-number reset on ACK timeout.
waitForAck's timeout handler didthis.hciSequenceNumber = 0mid-transfer.sendPacketresends the same packet built with the old sequence number, and the adafruit-nrfutil reference does not reset on timeout, so this desynced the reliable-transport counter and a single lost ACK could wedge the rest of the transfer. The reset is removed.ACK-listener race in
sendPacket. The packet was written beforewaitForAckarmedthis.resolveAck, so an ACK that arrived immediately after the write could be dropped byreadLoop. The ack promise is now created before the write.disconnectPort()threw before closing the port. It called.releaseLock()onport.readable/port.writable(ReadableStream/WritableStreamhave no such method — only their reader/writer do), so it threw beforeport.close()ran, leaving the port open with a stalethis.portand blocking the next reconnect. Rewritten with per-steptry/catchand afinallythat always nullsreader/writer/port.Cancelled-picker misdetection. The DFU error path checked
e.messagefor'NotFoundError', but a cancelled serial-port picker throws aDOMExceptionwhose name isNotFoundError(its message is "No port selected by the user."). The check now usese.name, correctly routing cancellation to the reconnect path.Verification
node --check nrf_web_tools.jspasses.diffagainst the website's vendored copy is empty.DOMExceptionis detected viae.name(note.message) and thattextContentnever parses injected markup.Open question (not addressed here)
It's unclear what the live
opendisplay.org/nrf_web_tools/*.jsURL actually serves versus the vendoredhttpdocs/js/nrf_web_tools.jsused as the reference here. Worth confirming which file is authoritative on the site so future fixes don't diverge again.🤖 Generated with Claude Code
https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR