PR #664 rewrote the comment explaining why the dynamically-inserted
es-module-shims script sets async = false, replacing the actual
execution-order reasoning (it must run before the deferred
type="module" script below, without blocking the parser for browsers
that never reach this branch) with a vaguer, inaccurate description
("inspects module graphs that fail static linking") that doesn't
explain the async choice at all. It also dropped the regression test
asserting .async = false is present, so a future "cleanup" removing
that line would go uncaught -- and the misleading comment no longer
warns against doing so.
The actual .async = false code was untouched by #664; this only
restores the documentation and its test coverage.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeQL alert 313 (go/log-injection). The access-log middleware logged
r.URL.Path and SOAP-body-derived objectID/browseFlag verbatim, without
stripping newlines -- an attacker-controlled request could inject fake
log lines or control characters. Add the same sanitizeLog helper this
repo already uses in ~18 other packages for exactly this class of
finding.
Alert 312 (go/reflected-xss, same file/area) was investigated and left
open deliberately: objectID is only ever used as a lookup key in
pkg/dlna/dlnatest, never echoed into the response, and every actual
output field goes through xmlEsc/xmlAttr (encoding/xml.EscapeText)
before being written -- looks like a CodeQL false positive rather than
a real gap, but not dismissing it yet per discussion.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeQL alert 318 (js/remote-property-injection). setDevices guarded
the status_update write with a plain "!prev[msg.deviceId]" truthy
check; a deviceId of "__proto__" or "constructor" resolves through
the prototype chain to a truthy value, so it would pass the guard
despite not being a real known device, letting the spread write a
bogus own-property (not actual prototype pollution -- computed keys
in object literals use [[DefineOwnProperty]], not the legacy __proto__
setter -- but still corrupts the rendered device list). Use
Object.prototype.hasOwnProperty.call for a real own-property check;
avoided Object.hasOwn (ES2022, Safari 15.4+) given #649's recent
Safari-15.0 compatibility work.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeQL alert 319 (js/log-injection). This logged the WebSocket
discovery_status payload verbatim to the browser console under a
"[DEBUG_LOG]" tag -- development-only cruft left in, not something
that serves any product purpose.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CodeQL alert 320 (js/unused-local-variable). The interactions table
never had a session column; i.session/i.Session was extracted but
never referenced anywhere in the row template.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HandleTuneInToken's docstring described the minted token as "fresh",
but datastore.GenerateSerialSecret("tunein") is a pure function of a
hardcoded literal -- it returns the identical value for every device
and every call, not a per-session secret. Correct the framing and
document why the constant value is safe today (Authorization gate
disabled for all TuneIn handlers, nothing validates uniqueness), so a
future change relying on per-device uniqueness doesn't get misled.
Also restore request-body validation dropped when the handler stopped
using the body's values: a genuine bootstrap call is still well-formed
JSON (confirmed against a captured real request), just with an empty
refresh_token, so decoding-but-discarding the body still rejects only
truly malformed requests with 400, without reintroducing the original
echo bug.
Also fixes 4 pre-existing wsl_v5 lint findings in the reordered
children-check in bmx/tunein.go (whitespace only, no behavior change).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
es-module-shims.js (vendored verbatim from npm) tripped 3 CodeQL
findings (js/incomplete-sanitization, js/bad-code-sanitization x2) --
real escaping-order bugs in the library's own source, verified by hand,
but not reachable in how this project uses it (no dynamic import()
built from untrusted input, no CSP nonce ever set). Reported upstream
separately.
The javascript-typescript CodeQL matrix entry had no path exclusions at
all, unlike the existing Go config's paths-ignore for vendor/generated
code, so preact.module.js and htm.module.js were exposed to the same
risk even though neither had tripped a finding yet. Add a JS-specific
config excluding pkg/service/soundtouchweb/static/lib/** -- we don't
control or modify these files, so findings there aren't actionable
from this repo.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Runs the opt-in "make test-browser" chromedp tests (added in the
previous commit) on their own, independent of ci.yml's main test job,
since they need a Chrome/Chromium binary in the runner. Mirrors
ci.yml's checkout/setup-go/cache steps and pinned action versions, and
verifies google-chrome is present with a clear error message before
running, rather than surfacing a cryptic chromedp allocator failure if
the runner image ever stops shipping it preinstalled.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds two opt-in tests (build tag "browsertest", run via `make
test-browser`) that drive a real headless Chrome instead of only
asserting on the raw HTML/JS source:
- TestPlayerRendersNatively confirms the shipped page still renders
normally and never injects es-module-shims on a browser with native
import map support.
- TestPlayerRendersUnderForcedShimMode forces es-module-shims into its
own shimMode (importmap-shim/module-shim, per the library's docs),
routing the real app.js and vendored dependencies through the
library's actual polyfill resolution. This exercises the old-Safari
code path directly in CI/local headless Chrome, without needing
physical iPadOS 15 hardware.
Not wired into `test`/`check`/CI yet, since chromedp has not
previously been exercised as part of this repo's test suite (only in
the standalone doc-screenshot tool) and needs a Chrome/Chromium binary
available to the runner.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Loading es-module-shims unconditionally would charge every browser an
~80KB uncompressed download on every page view, including the vast
majority that already support import maps natively -- the static
asset server here applies no compression. Feature-detect
HTMLScriptElement.supports('importmap') instead, so only a browser
that actually lacks support ever fetches it.
Insert the script via the DOM with async = false rather than
document.write: document.write is deprecated and subject to browser
interventions that can silently drop externally-sourced scripts it
injects, where DOM insertion with async explicitly disabled gives the
same before-the-deferred-module-script execution guarantee without
that risk.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restores the import map and bare-specifier imports across all
components, and instead polyfills import map support for Safari on
iPadOS 15 (which has ES modules but not import maps) via
es-module-shims, loaded unconditionally since it detects native
support and no-ops there.
The previous approach converted every component to relative imports
through a dependencies.js facade and permanently sed-patched the
vendored preact-hooks build, baking the compatibility workaround into
the whole codebase instead of keeping it encapsulated in index.html.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The handler counted every request regardless of path, so the background
status-update goroutine AddDeviceByHost spawns after a successful probe
could land before the assertion and be mistaken for a second concurrent
seed probe, occasionally failing with request count 2 instead of 1. Filter
by path like the existing TestDiscoverDevicesRetriesConfiguredHosts test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes correctness issues found reviewing the bounded device-seed retry
loop before merging: a datastore read failure could make the readiness
check trivially pass; stale-host pruning only considered hosts inserted
in the current attempt and only ran inside the retry loop, not the
plain SeedExtraDevices path; the retry loop and a devices-changed-hook
seed could probe the same offline host concurrently; and a zero-change
startup window silently dropped the previously-unconditional device-list
broadcast. Also makes the retry interval/window configurable instead of
hardcoded, following the existing discovery-interval flag pattern.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes the loop on #614: presets wiped after a reboot when a speaker
shares its Marge account with other devices. Root cause stays
unconfirmed (firmware-internal), but removing the other devices from
the account is a reporter-confirmed workaround.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
NewTestServer registers its own t.Cleanup(Close) instead of needing a
manual defer, and fails the test on a handler panic instead of just
logging it. It defaults to an in-memory transport reachable only via
Server.Client(), which wouldn't work here since our production
client.NewClient dials a real address rather than using that client --
calling Start() instead of Client() opts back into a real loopback
listener, identical to the old NewServer, confirmed by reading the
actual go1.27.0 source (server.go's Start implementation).
This is a proactive adoption of a new stdlib idiom, not one of the
review findings from the previous commit; it doesn't change the
goroutine-drain fix from that commit, which is a separate concern
Close()'s "wait for outstanding requests" guarantee doesn't fully
cover (a goroutine that hasn't started its request yet at Close() time
isn't "outstanding").
Verified: 10x -count re-run under -race, full suite + lint clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five findings from reviewing gesellix/Bose-SoundTouch#644
(Retry configured player devices during discovery):
1. Source-labeling used exact string equality
(device.DiscoveryMethod == "Configuration"), which breaks once a
configured host is also found via mDNS/UPnP in the same sweep:
mergeDeviceData concatenates methods into e.g.
"Configuration+mDNS/Bonjour", so the check silently failed and the
device got labeled "discovered" instead of "manual". Extracted the
decision into classifySource() and switched to a substring match.
Added TestClassifySource, which fails against the old exact-equality
logic on exactly the composite-string cases (verified) and would
have caught this before merge -- the PR's own test disables
mDNS/UPnP, so it never exercised this path.
2. Manually configured devices no longer registered immediately at
startup -- they now wait for the full mDNS/UPnP sweep (up to the
10s discovery timeout) to complete, since the PR removed the
synchronous registration loop and relies entirely on
PreferredDevices. Restored the immediate loop alongside (not
instead of) folding manualHosts into PreferredDevices, so a
currently-online configured device registers immediately as
before, while an offline one still gets retried on every
subsequent discovery pass -- the actual value this PR adds.
3. The new PreferredDevices-seeding loop didn't dedupe against hosts
already loaded from PREFERRED_DEVICES, so setting both for the same
host produced duplicate entries. Currently harmless (absorbed by
AddDeviceByHost's fast path) but fragile. Added dedup by host.
4. NewDiscoveryService's doc comment didn't mention the new
configuredHosts parameter or its retry-on-every-sweep behavior.
Documented.
5. The new test's second DiscoverDevices call spawns a one-shot
status-update goroutine and a 30s-ticker poll loop with no
guaranteed drain before the deferred server.Close(), risking
benign but real -race/CI flakiness. Added a bounded settle delay
after RemoveDevice.
Verified: full build/vet/race test suite/lint clean; the new
TestClassifySource fails against the pre-fix logic and passes with
it; TestDiscoverDevicesRetriesConfiguredHosts re-run 20x under -race
with no flakiness.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The nine env vars install.sh reads were only ever mentioned inline,
scattered across the file, or not documented at all
(AFTERTOUCH_FORCE_NO_BACKUP, GH_REPO, BINARY_URL, INIT_SCRIPT_URL,
FALLBACK_VERSION, AFTERTOUCH_LAN_PORT). Collect them into one table.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Space Limitation section still described update safety as an open
problem ("we are currently working on this"). Replace it with what the
installer now actually does: gzip-compressed backups, a preflight
disk-space check with an interactive confirm-or-abort before skipping
the backup, and a hard abort before downloading anything if there
isn't even room for the update itself.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The gzip fix in the previous commit only helps once a backup is being
made; it doesn't address the actual moment that broke on real hardware:
the cross-device mv/copy of the new binary into place ran out of space
mid-write, leaving a truncated, non-executable binary as the live one.
UBIFS is a log-structured flash filesystem, so space "freed" by
overwriting the old binary isn't guaranteed reusable in time for the new
one to land -- this happened on a device with 15.7MB available against
a ~14.8MB binary.
Add a preflight check before downloading anything: fetch the new
binary's real size via a HEAD request (adapts automatically as binaries
grow, instead of a threshold that goes stale every release) and compare
against available space plus a flat 5MB safety margin.
- Comfortably enough room for old + new + a compressed backup: proceed
exactly as before, silently.
- Enough for old + new but not enough extra for a backup: warn
interactively and require explicit confirmation before proceeding
without one. Reads from /dev/tty since the script is normally piped
via `curl | sh` (stdin is consumed by the script itself). Defaults to
the safe choice (abort) on empty input, matching the [y/N] prompt.
AFTERTOUCH_FORCE_NO_BACKUP=yes overrides for non-interactive/scripted
use.
- Not enough room even for the replace itself: abort before starting
the download, rather than attempting a doomed download/replace that
could leave a truncated live binary.
- No TTY available and the operator didn't set the override: abort
rather than silently guessing.
- HEAD request fails for any reason: skip the check with a warning
rather than blocking the install on it.
Verified: all five decision branches (plenty of room, warn+decline,
warn+confirm, warn+forced-override, hard abort) produce the correct
result under both dash and a real BusyBox v1.38.0 container, including
the gzip/gunzip streaming backup and glob-based pruning from the
previous commit. The HEAD-request size lookup was separately verified
against a live GitHub release URL with real curl -- catching and fixing
a bug where naively taking the first "content-length:" header grabbed
the 302 redirect's (0), not the actual asset's, size. Not yet re-tested
end-to-end on real hardware.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Binaries are tens of MB and only growing (Go 1.27 alone added ~655KB
to soundtouch-service via its own new stdlib defaults, unrelated to
this project's code), while the on-device install target (/mnt/nv) is
only tens of MB total. A user already hit "no space left on device"
attempting an update on real hardware.
Stream the pre-update backup straight through gzip instead of cp-then-
gzip: at that point in the script the old binary is still live and the
newly-downloaded one is already sitting in the temp dir, so writing an
intermediate uncompressed backup copy would briefly need three full
binary-sized copies on disk at once. Streaming avoids ever creating
that intermediate copy. Falls back to a plain uncompressed backup if
gzip is unavailable or the stream fails partway, matching prior
behavior exactly.
Both GC loops (pre- and post-install) now also prune stale
*.backup.gz artefacts, and the README's documented rollback command
covers both the compressed and (fallback) uncompressed cases.
Verified locally (not yet on real hardware): streaming path produces
no uncompressed intermediate, the gzip-unavailable fallback still
produces a plain backup, and the documented gunzip+chmod rollback
restores a byte-identical, executable binary.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest
silently resolves to the latest v1.x release (v1.64.8) -- Go's semantic
import versioning treats v2+ as a completely separate module path
(.../v2/cmd/golangci-lint), so the unsuffixed path's @latest can never
see v2 releases. That mismatched v1 binary can't even load this
repo's v2-format .golangci.yml, and separately doesn't understand the
go1.27.0 toolchain declared in go.mod.
Fix the install hint in `make lint`'s not-found message to use the /v2
path, and refresh the now-current version noted in .golangci.yml's
header comment (installed locally as v2.13.1, built with go1.27.0).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Go 1.27 changed how runtime.FuncForPC reports the symbol for
HandleWeb()'s returned closure: it now correctly attributes it to its
defining function (handlers.(*Server).HandleWeb) instead of leaking the
inlining call site's enclosing function name (setupRouter) the way
older Go versions did. The registered route itself is unchanged -- this
is purely a difference in the introspected debug name.
The test's cleanup logic had a dedicated special case for stripping a
leading "setupRouter" prefix, added to work around exactly that
inlining artifact. Verified empirically (temporarily instrumented with
the raw runtime.FuncForPC output, then diffed the full 299-route table
with the special case removed) that the general prefix-stripping loop
already produces an identical result for the remaining legitimate
cases (closures actually defined inline in setupRouter, e.g.
/favicon.ico) -- so the dedicated case was already redundant before
this Go bump and can be dropped.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
syncSources never reported how many sources it actually saved, so the
Admin UI's success message always said the meaningless "sources:
synced" regardless of outcome. syncSources now returns the count saved
(-1 if the fetch failed), threaded through SyncResult.SourcesCount.
Also replaces the single run-on results string (which visually mashed
presets/recents/sources together with no separator) with a real <ul>
list, one <li> per resource, matching the presets/recents diff lines.
Built via DOM APIs rather than innerHTML string concatenation, since
preset/recent names ultimately come from user-editable station names
on the speaker.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
startSync() used to POST once and, on any 2xx, render a hardcoded
"Presets: OK / Recents: OK / Sources: OK" regardless of what the
response actually said -- exactly why a silent partial data loss (see
the previous commit) would have looked like success to the user.
Now: on a 409 (destructive) response, build a specific confirm message
from the diff (e.g. "presets: 6 -> 5: Ici Roussillon") and gate via
window.confirm(), matching the existing QuickFix confirm UX; on
confirm, retry with ?confirmed=true. On success, render the actual
per-resource counts from the response body instead of a canned string.
Adds an HTTP-level regression test
(TestHandleInitialSync_DestructiveSyncReturns409ThenAppliesWhenConfirmed)
covering the same refuse-then-confirm flow through the real handler and
router, complementing the lower-level setup package test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SyncDeviceData's syncPresets/syncRecents unconditionally overwrote the
datastore with whatever the speaker's live :8090 API returned at that
instant, with no check against what's already stored. If the speaker's
own local cache was stale or incomplete at that moment (e.g. right
after a burst of preset writes, or shortly after a reboot before the
speaker resyncs with Marge), Sync would silently persist that bad
snapshot over good data. A reporter's fresh #614 repro showed the
account's /full response dropping from 6 to 5 presets right after a
Sync click, consistent with this mechanism.
SyncDeviceData now diffs a fresh live fetch against what's stored
before writing anything; if applying would shrink either list, it
returns the diff (via the new SyncResourceDiff/SyncResult types)
without writing unless the caller passes confirmed=true.
HandleInitialSync surfaces this as a 409 with the diff JSON; every call
(confirmed or not) re-fetches live from the speaker, so a confirmed
retry re-checks reality rather than replaying a stale snapshot. Sources
sync is left unconditional, as before -- lower risk in practice and
out of scope for this fix.
fetchLivePresets/fetchLiveRecents are extracted pure-fetch helpers;
syncPresets/syncRecents keep their unconditional-apply behavior (used
directly by existing tests) since the button-driven path now goes
through the diff/confirm guard instead.
Adds TestSyncDeviceData_DestructiveSyncRequiresConfirmation covering
both the refusal and the confirmed-retry path.
Frontend wiring (script.js's startSync + real per-resource result
rendering, replacing the current hardcoded "OK" text) is a follow-up
commit on this branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Converts the remaining GetX-then-SaveX call sites (UpdatePreset,
RemovePreset, AddRecent's recent + learned-source persistence, AddSource)
to the new datastore.Mutate{Presets,Recents,ConfiguredSources} helpers,
closing the lost-update race for good on the actual write path the
speaker hits on every preset/recent store.
Adds a regression test that fires 6 concurrent UpdatePreset calls (same
shape as #614's rapid-fire repro) and asserts none are lost. Verified it
reliably fails against the pre-fix code (consistently drops presets
across repeated runs) and passes reliably with the fix, including under
-race.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetX-then-SaveX call sites did an unguarded read-modify-write: two
concurrent callers could each read the same starting list, mutate
different entries, and the second writer's Save clobber the first's
update. This is exactly what dropped a preset during #614's rapid-fire
preset-programming repro (overlapping PUT .../preset/N requests).
Add MutatePresets/MutateRecents/MutateConfiguredSources, each holding a
single write lock across the whole read-mutate-write cycle, and switch
resolvePresetSource's auto-add-canonical-source path (the same race,
for sources) to use the new MutateConfiguredSources.
Part of the #614 follow-up; more call sites (UpdatePreset's own preset
write, recents, other sources writers) still need converting.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bumps golang from 1.26.6-alpine to 1.27.0-alpine.
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
</details>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Speakers paired via non-AfterTouch tooling (e.g. the USB-stick SSH-enable
method) can report a margeAccountUUID that isn't Bose's own 7-digit
numeric format, such as "stick@local". Discovery persisted this value
unvalidated, and the datastore's identifier check rejected it outright,
so the device was silently never saved.
Widens datastore.IsSafeIdentifier to accept any identifier that's safe
as a path component, XML value, and telnet-command token (still
excluding whitespace, control characters, and HTML/XML/shell
metacharacters), and makes it the single account-ID validator,
replacing setup's separate, stricter 7-digit-only IsValidAccountID.
Also closes related gaps found while widening the validator:
- postSetMargeAccount now XML-escapes the account ID instead of raw
string interpolation.
- SaveAccountInfo/HandleMargeCreateAccount now validate the account ID
the same way SaveDeviceInfo already did.
- handlers_export.go URL-escapes account/device IDs before building
outbound diagnostic-fetch URLs.
- pkg/service/health gained the sanitizeLog helper every other package
already has, applied to log lines carrying speaker-reported values.
- The admin web UI (script.js) renders account/device IDs via DOM APIs
instead of innerHTML/inline event-handler string interpolation,
closing a stored-XSS path, and a duplicate escape helper was
consolidated into one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
readPresetsLocked's os.IsNotExist branch and GetRecents' equivalent
branch silently returned an empty result with no log line at all,
unlike their sibling 0-byte/malformed-XML branches which already log.
When a reporter's speaker got served an empty preset list at reboot
despite an intact on-disk Presets.xml, there was no durable record of
it anywhere except a live capture at the exact moment.
Also log the per-device preset count going into every /full response
in CreateAccountDevice, distinguishing a disk read that came back
empty from one where source-mapping silently dropped presets
afterward.
Diagnostic only, no behavior change - the actual trigger for the
empty response is still open.
