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 mock services in docker-compose.ci.yml were pinned to
golang:1.27.0-alpine, which is now too old to run against go.mod's
1.27.1 requirement (GOTOOLCHAIN=local makes this an immediate, silent
container crash: "go.mod requires go >= 1.27.1 (running go 1.27.0)").
Bump all three mock images to 1.27.1-alpine to match.
Also make `make test-http-client` dump docker compose logs (and tear
down) whenever `docker compose up --wait` itself fails, not only
after the .http test run — that path previously aborted with no
diagnostic output at all. Switch the post-run log dump to plain
`docker compose logs` (all services) instead of three hardcoded
per-service calls, which had silently omitted tunein-mock.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
TestPrintRoutes compares against a checked-in route list; the new
GET .../zone/candidates route (added for Zone.js's candidate source
fix) needs to be reflected there too.
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 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>
