Skip to content

Harden the dev flasher's postMessage receiver#1609

Merged
bdraco merged 2 commits into
mainfrom
harden-flasher-postmessage
Jun 19, 2026
Merged

Harden the dev flasher's postMessage receiver#1609
bdraco merged 2 commits into
mainfrom
harden-flasher-postmessage

Conversation

@bdraco

@bdraco bdraco commented Jun 19, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

Three receiver-side hardening fixes for the dev flasher's postMessage handler (flasher/src/main.ts), for parity with the web.esphome.io receiver review (esphome/dashboard#923) that shares the same contract. The device-builder-frontend sender (src/util/usb-flasher.ts) is unaffected; these are receiver concerns.

  • post() no longer throws on a malformed origin. targetOrigin comes from the hash origin param, so a value like origin=null becomes "null" and the first synchronous post() threw before the ready-retry interval was set up, wedging the handshake. post() now wraps postMessage in try/catch, falls back to "*", and pins targetOrigin = "*" so subsequent frames keep flowing.
  • A malformed payload stops the ready re-announce. The isFlashParts failure branch set the error state and returned before stopReadyRetry(), so the opener kept receiving ready frames for up to 10s even though it had clearly attached and sent. It now calls stopReadyRetry() on that path, mirroring the accepted path.
  • erase is coerced explicitly. firmware.erase ?? true only defaults on null/undefined, so a non-boolean erase (e.g. the string "false") read as truthy. Changed to firmware.erase !== false, so erase stays the safe default unless the opener explicitly sent boolean false.

The puppeteer harness (flasher/test/handshake.test.mjs) gains assertions for the first two: a malformed origin=null still yields a ready frame, and a malformed payload stops the ready re-announce. The erase path only runs at flash time against real hardware, so it stays code-only (consistent with the harness's existing scope). I confirmed both new assertions go red when their fix is reverted.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed
  • Companion frontend PR: esphome/device-builder-dashboard-frontend#

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.

Three receiver-side fixes for parity with the web.esphome.io review
(esphome/dashboard#923), which shares this postMessage contract:

- post() wraps postMessage in try/catch and falls back to '*', so a
  malformed origin= hash param (e.g. origin=null) can no longer throw
  and wedge the ready handshake before the retry interval starts.
- A malformed firmware payload now calls stopReadyRetry() before
  bailing, so the opener stops receiving ready frames once it has
  clearly attached, instead of for up to 10s.
- erase coerces with 'firmware.erase !== false' instead of '?? true',
  so a non-boolean erase (e.g. the string "false") no longer reads as
  truthy.

handshake.test.mjs gains assertions for the first two (the erase path
only runs against real hardware).
@github-actions github-actions Bot added the bugfix Bug fix label Jun 19, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing harden-flasher-postmessage (70c1250) with main (fcdad94)

Open in CodSpeed

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (fcdad94) to head (70c1250).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1609   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         226      226           
  Lines       17852    17852           
=======================================
  Hits        17771    17771           
  Misses         81       81           
Flag Coverage Δ
py3.12 99.51% <ø> (ø)
py3.14 99.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@esphbot

esphbot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@bdraco bdraco marked this pull request as ready for review June 19, 2026 22:14
Copilot AI review requested due to automatic review settings June 19, 2026 22:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the dev USB flasher’s postMessage receiver (flasher/src/main.ts) to avoid handshake wedging and to better handle malformed inputs, aligning behavior with the production receiver review findings.

Changes:

  • Wrap outbound postMessage in try/catch and fall back to targetOrigin="*" when the hash-provided origin is malformed (e.g. origin=null) so the ready handshake still proceeds.
  • Stop the ready re-announce interval when a nonce-valid but malformed firmware payload is received (error state), preventing spurious ready frames after the opener has clearly attached.
  • Make erase handling explicitly boolean-safe (erase defaults to true unless the sender provides literal false).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
flasher/src/main.ts Adds receiver-side hardening: safe postMessage fallback, correct ready-retry shutdown on malformed payload, and explicit erase coercion.
flasher/test/handshake.test.mjs Extends puppeteer handshake assertions to cover malformed origin and “malformed payload stops ready retry” behavior.

The malformed-origin fallback caught every postMessage failure and
silently downgraded targetOrigin to '*', masking unrelated errors, and
the fallback post was itself unguarded. Log the caught error before
falling back (outbound frames carry no nonce, so the broader audience
leaks nothing) and wrap the fallback so a dead opener doesn't throw.
@bdraco

bdraco commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

Thanks Kōan — addressed the broad-catch finding in 70c1250: post() now logs the caught error before falling back (so an unrelated failure isn't masked) and the fallback postMessage is wrapped too. I kept the '*' fallback rather than narrowing to SyntaxError because outbound frames carry no nonce, so the broader audience leaks nothing, and the in-browser error type for a bad origin didn't reliably match SyntaxError instanceof DOMException (the malformed-origin test went red with the narrowed check). The same fix is applied to the web.esphome.io receiver in esphome/dashboard#923.

@bdraco bdraco merged commit 3e7b027 into main Jun 19, 2026
22 checks passed
@bdraco bdraco deleted the harden-flasher-postmessage branch June 19, 2026 22:33
@esphbot

esphbot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Review — Harden the dev flasher's postMessage receiver

Three tightly-scoped, correct receiver-hardening fixes with matching puppeteer assertions. Merge-ready.

Verified against the live file (not just the diff):

  • post() (main.ts:120-136) now survives a malformed targetOriginparams.get("origin") turns origin=null into the string "null", which makes postMessage throw before the ready-retry interval is armed. The try/catch pins targetOrigin = "*" and re-sends, so the handshake no longer wedges. The earlier broad-catch concern is already addressed in 70c1250: the caught error is logged before fallback and the fallback postMessage is itself wrapped, so a non-origin failure stays visible.

  • The "*" fallback is self-healing: line 192 re-narrows targetOrigin from "*" to the real ev.origin on the first valid firmware frame, so post-handoff state/progress frames target the true opener rather than staying broadcast.

  • Malformed-payload branch (main.ts:184-189) calls stopReadyRetry() before setState("error", …), mirroring the accepted path at line 195; the message listener stays attached so a later valid payload is still processed.

  • firmware.erase !== false (main.ts:525) is the correct coercion — ?? true left a stringy "false" truthy; the new form keeps erase as the safe default unless the opener sends boolean false.

  • Tests are honest about scope: 2b-ii (700ms > the 500ms retry interval) and 2d (origin=null still yields ready) exercise the two browser-observable fixes; the erase path is correctly left code-only since it only runs against real hardware.

  • No blocking issues. Label bugfix, Fixes #1607, and the no-frontend-change box are all consistent with a receiver-only change.



Checklist

  • Logic correct, edge cases handled
  • Error handling does not swallow real failures
  • New branches covered by tests
  • Diff matches PR description, no scope creep

Automated review by Kōan (Claude) HEAD=70c1250 1 min 27s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@github-actions github-actions Bot locked and limited conversation to collaborators Jun 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden the dev flasher's postMessage receiver (parity with esphome/dashboard#923)

3 participants