Gorilla's default same-origin CheckOrigin compares Origin and Host as
raw strings, port included. This repo's own documented nginx reverse-
proxy config forwards a portless Host header (nginx's $host never
includes the port, unlike $http_host) regardless of what public port
the proxy listens on. That's harmless on the scheme's default port
(the browser's Origin also omits it there), but on a non-default
public port (e.g. :8443, a realistic multi-service-hosting shape) the
browser's Origin keeps the port while the forwarded Host doesn't --
gorilla's strict compare then 403s every WebSocket handshake, silently
breaking the player's live updates in a deployment topology the docs
actively recommend.
Add checkWebSocketOrigin/sameHostIgnoringPort: gorilla's own default
policy, but comparing hostname only. Same-origin and cross-hostname
behavior is unchanged; only a port mismatch on an otherwise-matching
hostname is now tolerated. Also extracted newTestWebSocketServer,
shared by dialTestWebSocket and the origin-policy test, instead of the
origin test re-implementing the same httptest scaffolding inline.
Found in code review of PR #669 (findings #1, #2).
BeginStatusPoll/ApplySpeakerEvent/CompleteStatusPoll gated an entire
poll's merge (NowPlaying/Volume/Presets/Sources/Bass/IsConnected) behind
one shared speakerEventGeneration counter. Any unrelated push event
during the poll's flight discarded the whole result -- not just the
field that event touched. Sources has no push event at all, so it could
go stale indefinitely under ordinary event traffic, defeating both call
sites that depend on this poll (the 30s fallback poll and the
post-reconnect refresh).
Replace it with StatusField + BeginFieldPoll/CompleteFieldPoll/
ApplyFieldEvent: the same two-counter (issued/applied) pattern already
used for Group, generalized to one instance per independently-racing
field via a small fixed-size array. A poll or event for one field can
now only ever supersede that same field, never a different one. This
also removes the map-based generation bookkeeping the old mechanism
needed (issue/lookup/prune per poll) and the unreachable
"unknown generation" branch it required.
applyGroupUpdatedEvent now shares the same queueBroadcastIfChanged
helper as the other five event types, instead of duplicating the
"broadcast if changed" check inline.
Found in code review of PR #666 (findings #1, #2, #3, #4).
Two tests from PR #666 simulated "the browser WS write path is busy" by
holding the global webSocketWriteMu, which #665's fix removed. Adapt
them to the per-connection replacement: register a connection and hold
its own lock directly (same technique as the #665 test suite), rather
than a lock that no longer exists.
webSocketWriteMu serialized writes across ALL browser WebSocket
connections, not just the single connection gorilla actually requires.
HandleDeleteDevice's synchronous BroadcastDeviceList call, and every
other client's own periodic update, all contended on one lock -- directly
contradicting the PR's own goal that a stalled client cannot block
healthy ones.
Replace it with a per-connection *sync.Mutex stored in WSClients
(withConnWrite). Registration is fully decoupled from discovery-status
publication: a new connection reads whatever discoveryStatus.Load()
currently returns and is never blocked by an in-flight publication,
which stays safe because Store() always commits before a publication
takes its client snapshot. BroadcastDeviceList/BroadcastDiscoveryStatus
now write each client under only that client's own lock.
Found in code review of PR #665 (finding #1).
newStereoPairView emitted raw, un-normalized role.DeviceID/role.Role
while validMasterGroup/registeredMembersAgree/sameGroupClaim trim and
uppercase those same fields for internal comparison. Normalize before
assigning so a future frontend feature reading member.Role/.DeviceID
directly doesn't need to re-normalize it itself.
Found in code review of PR #665 (finding #8).
The singular GET /api/control/devices/{id} bypassed the projection that
HandleAPIDevices and both WebSocket frames already apply. A hidden
stereo-pair member was absent from the list but still fully fetchable,
unprojected, by its own id. Add deviceViewForID and return 404 for a
hidden member's own id, consistent with it already being absent from
the list.
Found in code review of PR #665 (finding #3).
BeginGroupRefresh/ApplyPolledGroup keyed invalidation off "has any newer
poll started" via groupGeneration equality. A later poll that starts but
never applies (its own GetGroup fails) still discarded an earlier poll's
still-arriving successful result, even though nothing newer ever actually
landed. Add groupAppliedGeneration and gate on "strictly newer than the
last applied", not "equal to the latest issued".
Found in code review of PR #665 (finding #2).
The default 20s wsURLReadTimeout in chromedp's exec allocator can be
too tight on a loaded shared CI runner spawning headless Chrome,
surfacing as an unrelated "websocket url timeout reached" test
failure. Raise it to 45s and widen the per-test context to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
If the device selected in the Library tab disappeared from the
devices map (e.g. it just became a hidden stereo-pair member per
device_projection.go), the sync effect fell back to entries[0][0] --
whichever key happens to sort first in the map -- silently redirecting
the user's Library browsing session to an unrelated speaker.
Now checks first whether the vanished device reappears as a member of
some other device's stereoPair (the pair's master, which now
represents the same physical speaker for control purposes) and
follows it there. Only falls back to an arbitrary device when the
selection is gone for a genuinely unrelated reason (removed, discovery
gap), matching the prior behavior for that case.
No JS unit-test framework exists in this repo for component-level
logic (consistent with the rest of the client-side code), so this is
verified by manual trace rather than an automated regression test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Zone.js derived its "add speaker" candidate list from the `devices`
prop, which is app.js's projected/collapsed device list. Once the
stereo-pair projection (device_projection.go) started hiding a pair's
non-master member from that list, it silently became impossible to
add that physical device to an unrelated multiroom zone, even though
the backend's HandleZoneAdd/HandleZoneRemove already operate on the
raw device registry directly and never cared about pairing at all.
Zone.js's own file wasn't touched by that change; its effective input
just changed underneath it.
Added GET /api/control/devices/{id}/zone/candidates, deliberately
bypassing deviceViewSnapshot's projection and deliberately not
excluding {id} itself -- which candidates to exclude is a caller
concern (Zone.js already does this via the existing zoneIps set,
which includes the zone master's own IP even when standalone, per
models.ZoneInfo.IsStandalone). Zone and Group are separate, unrelated
groupings and should stay that way.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sameGroupClaim (device_projection.go, used to validate a member's
claim agrees with the master's) compared roles via a device-ID-keyed
map, making it order-insensitive. webtypes.replaceGroup's change
detection used reflect.DeepEqual on the whole *Group, which is
order-sensitive for Roles.Roles. Both the polled /getGroup response
and the pushed groupUpdated event populate Roles.Roles directly from
XML unmarshaling in wire order, so nothing guarantees a pair's roles
list in the same order across two reads -- DeepEqual could then report
a spurious "changed" for a pair that didn't actually change.
Extracted the order-insensitive comparison into models.SameGroup as
the single shared implementation (also handles the nil/nil case
correctly, unlike the old sameGroupClaim, which mattered for
replaceGroup's existing "no prior group" path). Both call sites now
use it; the duplicate sameGroupClaim is gone.
Added TestApplyGroupEventIgnoresRoleOrder, verified to fail against
the prior DeepEqual-based logic and pass with this fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
statusUpdated (which drives IsConnected) ORed in
"stereoCapable && groupErr == nil" alongside the five substantive
status fetches. Since GetGroup is gated to stereo-capable models and
trivially succeeds even when a device is struggling (an empty
<group/> is a near-guaranteed reply, per Client.GetGroup's doc
comment), a round where NowPlaying/Volume/Presets/Sources/Bass all
fail but GetGroup alone succeeds would still report the device
connected -- masking a real status-refresh failure specifically on
ST10 hardware.
Removed the extra OR term entirely: IsConnected now depends only on
the five substantive fetches, matching the comment's own stated
intent ("mirrors prior behaviour"). GetGroup's own success/failure
still drives whether Group gets refreshed (unchanged, see
ApplyPolledGroup below), just no longer feeds the connectivity signal.
Added TestUpdateDeviceStatusNotConnectedWhenOnlyGroupSucceeds,
verified to fail against the prior logic and pass with this fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Refines the previous commit's doc fix with more precise, hardware-
verified detail: the ST20 doesn't just silently drop the connection --
its own firmware ("AllegroWebserver") eventually returns an explicit
"AllegroWebserver timeout: /getGroup" plain-text error after an
internal delay of several+ seconds, well past what client.get()'s
timeout will tolerate.
Also documents a dead-end a future contributor might otherwise try:
the ST20's own /supportedURLs response lists /getGroup (and the other
group endpoints) despite not actually servicing it, confirmed against
the same real hardware. A supportedURLs-based capability probe would
not have caught this either -- the model-name check has to stay.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The exported GetGroup doc comment claimed non-ST10 devices reply to
/getGroup "harmlessly" with an empty group. Verified against real
hardware this is wrong: a SoundTouch 20 does not reply at all -- the
request hangs until the client's own timeout (10-30s depending on how
the Client was constructed) instead of returning quickly. Confirmed
by direct request against a real ST20 (curl, 8s timeout, zero bytes
back) and cross-checked against two actively-paired real ST10 units,
which both replied in ~30-40ms with full group data.
This matters beyond prose accuracy: the newer stereoPairCapable gate
in websocket.go's UpdateDeviceStatus is load-bearing, not an
optimization. A future contributor trusting the old (wrong, and more
prominent/exported) doc could reasonably "simplify" by removing that
gate, reintroducing a 10-30s hang on every poll cycle for every
SoundTouch 20/30 on the network. Rewrote the doc to state the real
behavior and point at the gate that depends on it; the websocket.go
comment now defers to this doc instead of independently (and
incorrectly worded) restating it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 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>
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>
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>
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>
