diff --git a/cmd/soundtouch-cli/cmd_setup.go b/cmd/soundtouch-cli/cmd_setup.go index c4b8f9a..f835c5c 100644 --- a/cmd/soundtouch-cli/cmd_setup.go +++ b/cmd/soundtouch-cli/cmd_setup.go @@ -15,6 +15,7 @@ import ( "github.com/gesellix/bose-soundtouch/pkg/models" "github.com/gesellix/bose-soundtouch/pkg/service/constants" + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" "github.com/gesellix/bose-soundtouch/pkg/service/setup" "github.com/urfave/cli/v2" "golang.org/x/term" @@ -674,9 +675,9 @@ func setupEnableSSHCmd() *cli.Command { }, &cli.StringFlag{ Name: "account", - Usage: "Only used when the device is unpaired and --no-auto-pair is not set: 7-digit account ID to pair " + - "with (empty = generate one). Use this if you already know which account this device should end up " + - "on (e.g. to match one already in the datastore) rather than getting a random one now", + Usage: "Only used when the device is unpaired and --no-auto-pair is not set: account ID to pair with " + + "(empty = generate a fresh 7-digit one). Use this if you already know which account this device " + + "should end up on (e.g. to match one already in the datastore) rather than getting a random one now", }, &cli.BoolFlag{ Name: "no-reset-urls", @@ -1974,7 +1975,7 @@ func setupPairCmd() *cli.Command { Usage: "Pair the speaker with an account via WebSocket SETUP state machine", Before: RequireHost, Flags: []cli.Flag{ - &cli.StringFlag{Name: "account", Usage: "7-digit account ID (empty = generate)"}, + &cli.StringFlag{Name: "account", Usage: "Account ID to pair with (empty = generate a fresh 7-digit one)"}, &cli.StringFlag{Name: "mode", Value: "full", Usage: "full (state machine) or bare (setMargeAccount only — experimental)"}, &cli.StringFlag{Name: "service-url", Value: "http://aftertouch.local:8000", Usage: "AfterTouch base URL (also populates / in setMargeAccount)"}, &cli.StringFlag{Name: "name", Usage: "Speaker name to set during pairing (empty = keep current)"}, @@ -1998,8 +1999,8 @@ func setupPairCmd() *cli.Command { fmt.Printf("Generated account id: %s\n", accountID) } - if !setup.IsValidAccountID(accountID) { - return fmt.Errorf("invalid account id %q: must be 7 digits", accountID) + if !datastore.IsSafeIdentifier(accountID) { + return fmt.Errorf("invalid account id %q: must be a non-empty, path-safe identifier", accountID) } switch mode { diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index 5092c4a..985734f 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -33,11 +33,30 @@ func exists(path string) bool { return err == nil } -// isSafeIdentifier returns true if the given identifier is safe to use -// as a single path component (for account IDs, device IDs, etc.). -// It rejects empty strings, path separators, and parent directory references. -func isSafeIdentifier(id string) bool { - if id == "" { +// maxSafeIdentifierLength bounds account/device IDs accepted from a +// speaker or third-party pairing tool. Well under typical filesystem +// path-component limits (255 bytes); generous for any realistic +// margeAccountUUID or MAC-derived device ID. +const maxSafeIdentifierLength = 128 + +// IsSafeIdentifier returns true if the given identifier is safe to use +// as a single path component (for account IDs, device IDs, etc.), and +// safe to embed in the other places these values end up: XML sent to a +// speaker, log lines, and datastore-key comparisons. It rejects empty +// or overlong strings, path separators, and parent directory +// references. +// +// The allowed character set intentionally excludes XML/HTML-special +// characters (`< > & " '`), whitespace, and shell/URL metacharacters +// (see #634's `postSetMargeAccount`, which interpolates an account ID +// into an XML body, and `PairAccount`, which interpolates one into a +// literal `envswitch accountid set ` telnet command line) even +// though it accepts more than Bose's own 7-digit account format — +// devices paired via third-party or manual tooling (e.g. the +// USB-stick SSH-enable method) can report arbitrary margeAccountUUID +// values such as "stick@local". +func IsSafeIdentifier(id string) bool { + if id == "" || len(id) > maxSafeIdentifierLength { return false } @@ -46,14 +65,17 @@ func isSafeIdentifier(id string) bool { return false } - // Allow a conservative set of characters commonly found in IDs: - // letters, digits, underscore, dash, dot, and colon (for MAC-like IDs). + // Letters, digits, and a conservative set of punctuation seen in + // real-world IDs: underscore, dash, dot, colon (MAC-like IDs), and + // '@' (e.g. "stick@local"). Everything else — including all XML, + // HTML, shell, and URL metacharacters, whitespace, and control + // characters — is rejected. for i := 0; i < len(id); i++ { c := id[i] if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || - c == '_' || c == '-' || c == '.' || c == ':' { + c == '_' || c == '-' || c == '.' || c == ':' || c == '@' { continue } @@ -1513,7 +1535,7 @@ func (ds *DataStore) SaveDeviceInfo(account, device string, info *models.Service return fmt.Errorf("device ID/name cannot be empty") } - if !isSafeIdentifier(device) { + if !IsSafeIdentifier(device) { return fmt.Errorf("invalid device ID") } @@ -1521,7 +1543,7 @@ func (ds *DataStore) SaveDeviceInfo(account, device string, info *models.Service return fmt.Errorf("account ID cannot be empty") } - if !isSafeIdentifier(account) { + if !IsSafeIdentifier(account) { return fmt.Errorf("invalid account ID") } @@ -1709,6 +1731,10 @@ func (ds *DataStore) SaveAccountInfo(accountID string, info *models.ServiceAccou return nil } + if !IsSafeIdentifier(accountID) { + return fmt.Errorf("invalid account ID") + } + dir := ds.AccountDir(accountID) if err := ds.rootMkdirAll(dir, 0755); err != nil { return err diff --git a/pkg/service/datastore/safe_identifier_test.go b/pkg/service/datastore/safe_identifier_test.go index b551c05..15d8e83 100644 --- a/pkg/service/datastore/safe_identifier_test.go +++ b/pkg/service/datastore/safe_identifier_test.go @@ -2,6 +2,7 @@ package datastore import ( "os" + "strings" "testing" "github.com/gesellix/bose-soundtouch/pkg/models" @@ -19,6 +20,10 @@ func TestIsSafeIdentifier(t *testing.T) { {"abc-123", true}, {"abc.123", true}, {"00:11:22:33:44:55", true}, + // #634: third-party/manual pairing tools (e.g. the USB-stick + // SSH-enable method) can report a non-numeric margeAccountUUID. + {"stick@local", true}, + {strings.Repeat("a", maxSafeIdentifierLength), true}, {"", false}, {"/", false}, {"\\", false}, @@ -30,7 +35,6 @@ func TestIsSafeIdentifier(t *testing.T) { {"a..b", false}, {"a b", false}, {"a!b", false}, - {"a@b", false}, {"a#b", false}, {"a$b", false}, {"a%b", false}, @@ -39,12 +43,17 @@ func TestIsSafeIdentifier(t *testing.T) { {"a*b", false}, {"a(b", false}, {"a)b", false}, + {"ab", false}, + {`a"b`, false}, + {"a'b", false}, + {strings.Repeat("a", maxSafeIdentifierLength+1), false}, } for _, test := range tests { - result := isSafeIdentifier(test.id) + result := IsSafeIdentifier(test.id) if result != test.expected { - t.Errorf("isSafeIdentifier(%q) = %v; expected %v", test.id, result, test.expected) + t.Errorf("IsSafeIdentifier(%q) = %v; expected %v", test.id, result, test.expected) } } } @@ -72,6 +81,8 @@ func TestSaveDeviceInfo_Validation(t *testing.T) { {"acc1", "dev/1", true, "invalid device ID"}, {"acc..1", "dev1", true, "invalid account ID"}, {"acc1", "dev..1", true, "invalid device ID"}, + // #634: a non-numeric margeAccountUUID is now accepted. + {"stick@local", "dev1", false, ""}, } for _, test := range tests { @@ -85,3 +96,40 @@ func TestSaveDeviceInfo_Validation(t *testing.T) { } } } + +func TestSaveAccountInfo_Validation(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "datastore-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + ds := NewDataStore(tmpDir) + + tests := []struct { + account string + wantErr bool + errMsg string + }{ + {"acc1", false, ""}, + // #634: a non-numeric margeAccountUUID reported via + // POST /streaming/account (see HandleMargeCreateAccount) must + // be validated the same way SaveDeviceInfo already validates + // device-reported account IDs. + {"stick@local", false, ""}, + {"acc/1", true, "invalid account ID"}, + {"acc..1", true, "invalid account ID"}, + {"a +Kitchen SoundTouch +SoundTouch 10 +stick@local + + +SCM +27.0.6.46330.5043500 epdbuild.trunk.hepdswbld04.2022-08-04T11:20:29 +I6332527703739342000020 + + +PackagedProduct +27.0.6.46330.5043500 epdbuild.trunk.hepdswbld04.2022-08-04T11:20:29 +069231P63364828AE + + +https://streaming.bose.com + +001122334455 +203.0.113.10 + +sm2 +rhino +normal +US +US +` + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/info" { + w.Header().Set("Content-Type", "application/xml") + fmt.Fprint(w, deviceInfoXML) + } else { + http.NotFound(w, r) + } + })) + defer server.Close() + + deviceIP := server.URL[len("http://"):] + ds := datastore.NewDataStore(tempDir) + sm := setup.NewManager(server.URL, ds, nil) + + srv := NewServer(ds, sm, server.URL, false, false, false) + + discoveredDevice := models.DiscoveredDevice{ + Host: deviceIP, + Name: "Legacy Discovery Name", + ModelID: "SoundTouch 10", + SerialNo: "", + DiscoveryMethod: "UPnP", + } + + t.Logf("Test scenario: /info reports non-numeric margeAccountUUID %q", "stick@local") + + srv.handleDiscoveredDevice(discoveredDevice) + + const ( + expectedAccountID = "stick@local" + expectedDeviceID = "001122334455" + ) + + deviceInfo, err := ds.GetDeviceInfo(expectedAccountID, expectedDeviceID) + if err != nil { + t.Fatalf("device was not saved under account %q: %v (this is the #634 symptom — "+ + "SaveDeviceInfo rejects the raw margeAccountUUID as an invalid account ID)", + expectedAccountID, err) + } + + if deviceInfo.Name != "Kitchen SoundTouch" { + t.Errorf("Name = %q, want %q", deviceInfo.Name, "Kitchen SoundTouch") + } +} diff --git a/pkg/service/handlers/web/js/script.js b/pkg/service/handlers/web/js/script.js index c4a6925..78667ec 100644 --- a/pkg/service/handlers/web/js/script.js +++ b/pkg/service/handlers/web/js/script.js @@ -598,7 +598,19 @@ async function fetchDevices() { if (devices.length === 0) { container.innerHTML = "No devices known yet."; } else { - let html = ""; + // Built via DOM APIs rather than innerHTML/template strings: device + // fields (name, IDs, serials, ...) come from speakers and third-party + // pairing tools (see #634) and are not restricted to HTML/JS-safe + // characters, so they must never be parsed as markup or concatenated + // into inline event-handler attributes. + const table = document.createElement("table"); + const headerRow = document.createElement("tr"); + for (const label of ["Name & Model", "IP Address", "Device & Account ID", "Firmware & Serial", "Method", "Action"]) { + const th = document.createElement("th"); + th.textContent = label; + headerRow.appendChild(th); + } + table.appendChild(headerRow); // Clear and repopulate selectors const currentSyncVal = syncSelector.value; @@ -612,25 +624,83 @@ async function fetchDevices() { devices.forEach((d) => { const methodLabel = d.discovery_method === "manual" ? "👤 Manual" : "🔍 Auto"; - html += ` - - - - - - - - - - - - `; + + const nameModelCell = document.createElement("td"); + nameModelCell.className = "col-name-model"; + const nameDiv = document.createElement("div"); + nameDiv.className = "col-name"; + nameDiv.textContent = d.name; + const modelDiv = document.createElement("div"); + modelDiv.className = "col-model"; + modelDiv.style.cssText = "font-size: 0.8em; color: #666;"; + modelDiv.textContent = d.product_code; + nameModelCell.append(nameDiv, modelDiv); + + const ipCell = document.createElement("td"); + ipCell.className = "col-ip"; + ipCell.textContent = d.ip_address; + + const idsCell = document.createElement("td"); + idsCell.className = "col-ids"; + const deviceIdDiv = document.createElement("div"); + deviceIdDiv.className = "col-deviceid"; + deviceIdDiv.textContent = d.device_id; + const accountIdDiv = document.createElement("div"); + accountIdDiv.className = "col-accountid"; + accountIdDiv.style.cssText = "font-size: 0.8em; color: #666;"; + accountIdDiv.textContent = d.account_id || "default"; + idsCell.append(deviceIdDiv, accountIdDiv); + + const fwCell = document.createElement("td"); + fwCell.className = "col-fw-serial"; + const fwDiv = document.createElement("div"); + fwDiv.className = "col-firmware"; + fwDiv.textContent = d.firmware_version || "0.0.0"; + const serialDiv = document.createElement("div"); + serialDiv.className = "col-serial"; + serialDiv.style.cssText = "font-size: 0.8em; color: #666;"; + serialDiv.textContent = d.device_serial_number; + fwCell.append(fwDiv, serialDiv); + + const methodCell = document.createElement("td"); + methodCell.className = "col-method"; + methodCell.textContent = methodLabel; + + const makeActionButton = (label, onClick, extra) => { + const btn = document.createElement("button"); + btn.textContent = label; + btn.addEventListener("click", onClick); + if (extra) Object.assign(btn, extra); + return btn; + }; + + const actionCell = document.createElement("td"); + actionCell.append( + makeActionButton("Inspect", () => toggleDeviceSummary(d.device_id)), + makeActionButton("Sync Data", () => prepareSync(d.device_id)), + makeActionButton("Migrate", () => prepareMigration(d.device_id)), + makeActionButton("Prime Spotify", () => primeSpotify(d.device_id), { + id: `prime-spotify-${d.device_id}`, + className: "btn-spotify", + }), + makeActionButton("Remove", () => removeDevice(d.device_id, d.name), {className: "btn-danger"}), + ); + actionCell.querySelector(".btn-spotify").style.display = "none"; + + const row = document.createElement("tr"); + row.id = `device-row-${d.device_id}`; + row.append(nameModelCell, ipCell, idsCell, fwCell, methodCell, actionCell); + + const summaryRow = document.createElement("tr"); + summaryRow.id = `device-summary-${d.device_id}`; + summaryRow.style.display = "none"; + const summaryCell = document.createElement("td"); + summaryCell.colSpan = 6; + summaryCell.id = `device-summary-cell-${d.device_id}`; + summaryCell.style.cssText = "background: #fafafa; padding: 12px;"; + summaryRow.appendChild(summaryCell); + + table.append(row, summaryRow); const optSync = document.createElement("option"); optSync.value = d.device_id; @@ -649,8 +719,7 @@ async function fetchDevices() { eventSelector.appendChild(optEvent); } }); - html += "
Name & ModelIP AddressDevice & Account IDFirmware & SerialMethodAction
${d.name}
${d.product_code}
${d.ip_address}
${d.device_id}
${d.account_id || "default"}
${d.firmware_version || "0.0.0"}
${d.device_serial_number}
${methodLabel} - - - - - -
"; - container.innerHTML = html; + container.replaceChildren(table); if (currentSyncVal) syncSelector.value = currentSyncVal; if (currentMigrationVal) migrationSelector.value = currentMigrationVal; @@ -924,7 +993,14 @@ async function fetchAccountList() { const data = await response.json(); const selector = document.getElementById("account-selector"); if (selector) { - selector.innerHTML = data.accounts.map(acc => ``).join(""); + // Account IDs can contain non-alphanumeric characters (e.g. + // "stick@local", #634) — built via DOM APIs, not innerHTML. + selector.replaceChildren(...data.accounts.map(acc => { + const opt = document.createElement("option"); + opt.value = acc; + opt.textContent = acc; + return opt; + })); if (data.accounts.length > 0) { fetchAccountDetails(selector.value); } @@ -949,7 +1025,7 @@ async function fetchAccountDetails(accountId) { try { const response = await fetch(`/api/mgmt/accounts/${encodeURIComponent(accountId)}`); if (!response.ok) { - if (metadataEl) metadataEl.innerHTML = `Failed to load account details: ${response.statusText}`; + if (metadataEl) metadataEl.innerHTML = `Failed to load account details: ${escapeHtml(response.statusText)}`; return; } const data = await response.json(); @@ -964,7 +1040,7 @@ async function fetchAccountDetails(accountId) { metadataEl.innerHTML = ` ${warningNotice} - + '; try { - const response = await fetch(`/api/setup/devices/${deviceId}/events`); + const response = await fetch(`/api/setup/devices/${encodeURIComponent(deviceId)}/events`); const data = await response.json(); const events = data.events; @@ -1907,7 +1990,7 @@ async function removeDevice(deviceId, name) { } try { - const response = await fetch(`/api/setup/devices/${deviceId}`, { + const response = await fetch(`/api/setup/devices/${encodeURIComponent(deviceId)}`, { method: "DELETE", }); @@ -4804,14 +4887,14 @@ async function toggleDeviceSummary(deviceId) { const resp = await fetch(`/api/setup/device-summary/${encodeURIComponent(deviceId)}`); if (!resp.ok) { const txt = await resp.text(); - cell.innerHTML = `Summary failed: ${resp.status} ${escapeHTML(txt)}`; + cell.innerHTML = `Summary failed: ${resp.status} ${escapeHtml(txt)}`; return; } const data = await resp.json(); cell.innerHTML = ""; cell.appendChild(renderDeviceSummary(data)); } catch (e) { - cell.innerHTML = `Summary failed: ${escapeHTML(e.message || String(e))}`; + cell.innerHTML = `Summary failed: ${escapeHtml(e.message || String(e))}`; } } @@ -5016,12 +5099,3 @@ function unreachableBlock(probe) { return wrap; } - -function escapeHTML(s) { - return String(s) - .replace(/&/g, "&") - .replace(//g, ">") - .replace(/"/g, """) - .replace(/'/g, "'"); -} diff --git a/pkg/service/health/checks_consistency.go b/pkg/service/health/checks_consistency.go index ae4719a..356e40e 100644 --- a/pkg/service/health/checks_consistency.go +++ b/pkg/service/health/checks_consistency.go @@ -215,7 +215,7 @@ func detectOrphanDefaultEntries(ds *datastore.DataStore, paired []models.Service if speakerAccount != info.account { log.Printf("[Health] consistency: speaker %s reports margeAccountUUID=%s but ListAllDevices picked %s — preferring the speaker's answer for orphan-deletion suggestions", - deviceID, speakerAccount, info.account) + sanitizeLog(deviceID), sanitizeLog(speakerAccount), sanitizeLog(info.account)) } } @@ -289,21 +289,21 @@ func deleteOrphanAccountEntry(ds *datastore.DataStore, target Target) (string, e if speakerAccount := fetchSpeakerMargeAccount(ctx, speakerIP); speakerAccount != "" { if speakerAccount == target.Account { return "", fmt.Errorf("speaker %s reports margeAccountUUID=%s — refusing to delete /accounts/%s/devices/%s because it's the speaker's currently-active binding (re-paired since the consistency check ran?)", - target.Device, speakerAccount, target.Account, target.Device) + sanitizeLog(target.Device), sanitizeLog(speakerAccount), sanitizeLog(target.Account), sanitizeLog(target.Device)) } log.Printf("[Health] deleteOrphanAccountEntry: speaker %s confirmed margeAccountUUID=%s; target account %s is stale, proceeding with delete", - target.Device, speakerAccount, target.Account) + sanitizeLog(target.Device), sanitizeLog(speakerAccount), sanitizeLog(target.Account)) } else { log.Printf("[Health] deleteOrphanAccountEntry: speaker %s at %s not reachable for re-confirmation; relying on operator's Confirm click", - target.Device, speakerIP) + sanitizeLog(target.Device), sanitizeLog(speakerIP)) } } else { - log.Printf("[Health] deleteOrphanAccountEntry: no IP recorded for device %s — skipping speaker re-probe", target.Device) + log.Printf("[Health] deleteOrphanAccountEntry: no IP recorded for device %s — skipping speaker re-probe", sanitizeLog(target.Device)) } if target.Account == accountIDDefaultPlaceholder { - log.Printf("[Health] deleteOrphanAccountEntry: deleting the \"default\" placeholder entry for device %s; this is normal after pairing completed", target.Device) + log.Printf("[Health] deleteOrphanAccountEntry: deleting the \"default\" placeholder entry for device %s; this is normal after pairing completed", sanitizeLog(target.Device)) } path := ds.AccountDeviceDir(target.Account, target.Device) @@ -316,7 +316,7 @@ func deleteOrphanAccountEntry(ds *datastore.DataStore, target Target) (string, e } log.Printf("[Health] Removed orphan account entry %s (account=%s device=%s) at operator request", - path, target.Account, target.Device) + path, sanitizeLog(target.Account), sanitizeLog(target.Device)) return fmt.Sprintf("Removed stale account entry %s for device %s.", target.Account, target.Device), nil } @@ -479,7 +479,7 @@ func reclassifyCanonicalSourceIDs(ds *datastore.DataStore, target Target) (strin for i := range sources { if newID, ok := rename[sources[i].ID]; ok { log.Printf("[Health] Re-classify %s: id %s → %s (account=%s device=%s)", - sources[i].SourceKeyType, sources[i].ID, newID, target.Account, target.Device) + sanitizeLog(sources[i].SourceKeyType), sanitizeLog(sources[i].ID), sanitizeLog(newID), sanitizeLog(target.Account), sanitizeLog(target.Device)) sources[i].ID = newID diff --git a/pkg/service/health/checks_speaker_info.go b/pkg/service/health/checks_speaker_info.go index 3adcd06..15b290a 100644 --- a/pkg/service/health/checks_speaker_info.go +++ b/pkg/service/health/checks_speaker_info.go @@ -30,9 +30,12 @@ func suggestAccountForPairing(ds *datastore.DataStore, deviceID string) string { return "" } -// isSevenDigitAccountID mirrors setup.IsValidAccountID without -// importing the setup package (which would pull in SSH/telnet/certmgr -// transitively — see the boundary comment near speakerInfoXML). +// isSevenDigitAccountID is intentionally narrower than +// datastore.IsSafeIdentifier: it filters suggestAccountForPairing's +// candidates down to directories that look like a real Bose-issued +// account, not merely safe-to-use ones (a device-reported value like +// "stick@local", #634, is a safe identifier but not something to +// suggest as a pre-existing "real" account to reuse). func isSevenDigitAccountID(s string) bool { if len(s) != 7 { return false diff --git a/pkg/service/health/logutil.go b/pkg/service/health/logutil.go new file mode 100644 index 0000000..70459e5 --- /dev/null +++ b/pkg/service/health/logutil.go @@ -0,0 +1,13 @@ +package health + +import "strings" + +// sanitizeLog strips newline characters from s to prevent log-injection +// (CodeQL go/log-injection). Values from speakers (e.g. margeAccountUUID +// read live via :8090/info) may contain attacker-controlled newlines. +func sanitizeLog(s string) string { + s = strings.ReplaceAll(s, "\n", `\n`) + s = strings.ReplaceAll(s, "\r", `\r`) + + return s +} diff --git a/pkg/service/setup/init_plan.go b/pkg/service/setup/init_plan.go index 7f5dc2d..19dbad2 100644 --- a/pkg/service/setup/init_plan.go +++ b/pkg/service/setup/init_plan.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" ) // InitPlan describes everything required to take a factory-reset (or @@ -250,8 +252,8 @@ func (m *Manager) runURLRewrite(plan InitPlan, emit func(StepKind, string, StepS // ID, or validating a user-supplied value. func (m *Manager) resolveAccountID(plan InitPlan, info *DeviceInfoXML, emit func(StepKind, string, StepStatus, error)) (InitPlan, error) { if plan.AccountID != "" { - if !IsValidAccountID(plan.AccountID) { - invalidErr := fmt.Errorf("invalid AccountID %q: must be exactly 7 digits", plan.AccountID) + if !datastore.IsSafeIdentifier(plan.AccountID) { + invalidErr := fmt.Errorf("invalid AccountID %q: must be a non-empty, path-safe identifier", plan.AccountID) emit(StepGenerateAccountID, "validate account ID", StatusFailed, invalidErr) return plan, invalidErr @@ -260,7 +262,7 @@ func (m *Manager) resolveAccountID(plan InitPlan, info *DeviceInfoXML, emit func return plan, nil } - if info.MargeAccountUUID != "" && IsValidAccountID(info.MargeAccountUUID) { + if info.MargeAccountUUID != "" && datastore.IsSafeIdentifier(info.MargeAccountUUID) { plan.AccountID = info.MargeAccountUUID emit(StepGenerateAccountID, "reuse existing margeAccountUUID="+plan.AccountID, StatusOK, nil) diff --git a/pkg/service/setup/init_plan_test.go b/pkg/service/setup/init_plan_test.go index 39f7fb7..1e466b9 100644 --- a/pkg/service/setup/init_plan_test.go +++ b/pkg/service/setup/init_plan_test.go @@ -9,6 +9,8 @@ import ( "strings" "testing" "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" ) // fakeSession is a StateMachine that records the order of @@ -195,11 +197,13 @@ func TestExecuteInitPlan_ReusesExistingAccountUUID(t *testing.T) { } func TestExecuteInitPlan_GeneratesAccountWhenDeviceUUIDInvalid(t *testing.T) { - // Devices that report a non-7-digit UUID (e.g. a stale local value) must - // not be reused — we treat them as factory-reset for ID purposes. + // Devices that report an unsafe/malformed UUID (e.g. containing a path + // separator) must not be reused — we treat them as factory-reset for ID + // purposes. A merely non-numeric UUID (e.g. "stick@local", #634) IS + // reused now; see resolveAccountID/datastore.IsSafeIdentifier. info := &fakeInfoResponder{ deviceID: "AABBCCDDEEFF", - paired: "not-7-digits", + paired: "not/valid", postInitPaired: "", // we'll learn the generated ID from the result } sess := &fakeSession{} @@ -220,11 +224,11 @@ func TestExecuteInitPlan_GeneratesAccountWhenDeviceUUIDInvalid(t *testing.T) { t.Fatalf("ExecuteInitPlan: %v", err) } - if !IsValidAccountID(got.AccountID) { - t.Errorf("got.AccountID = %q, want a valid 7-digit ID", got.AccountID) + if !datastore.IsSafeIdentifier(got.AccountID) { + t.Errorf("got.AccountID = %q, want a valid generated ID", got.AccountID) } - if got.AccountID == "not-7-digits" { + if got.AccountID == "not/valid" { t.Error("orchestrator should not reuse an invalid UUID") } } @@ -236,7 +240,7 @@ func TestExecuteInitPlan_RejectsInvalidSuppliedAccountID(t *testing.T) { plan := InitPlan{ DeviceIP: "192.0.2.10", - AccountID: "abc", + AccountID: "abc/def", SkipURLRewrite: true, } diff --git a/pkg/service/setup/marge_pairing.go b/pkg/service/setup/marge_pairing.go index e635360..ced113b 100644 --- a/pkg/service/setup/marge_pairing.go +++ b/pkg/service/setup/marge_pairing.go @@ -1,6 +1,7 @@ package setup import ( + "bytes" "crypto/rand" "encoding/xml" "errors" @@ -11,6 +12,8 @@ import ( "net/http" "strings" "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" ) // PairAccountTimeouts bounds every step of the pairing call so a wedged @@ -44,8 +47,8 @@ func (m *Manager) PairAccount(deviceIP, accountID string, t TelnetClient) (PairA logs strings.Builder ) - if !IsValidAccountID(accountID) { - return result, "", fmt.Errorf("invalid account ID %q: must be exactly 7 digits", accountID) + if !datastore.IsSafeIdentifier(accountID) { + return result, "", fmt.Errorf("invalid account ID %q: must be a non-empty, path-safe identifier", accountID) } supported, supportedErr := m.probeSetMargeAccount(deviceIP) @@ -84,6 +87,9 @@ func (m *Manager) PairAccount(deviceIP, accountID string, t TelnetClient) (PairA result.TelnetAttempted = true + // Safe to concatenate: datastore.IsSafeIdentifier (checked above) rejects + // any whitespace or control characters, so accountID can't smuggle extra + // tokens into this single-line telnet command. cmd := "envswitch accountid set " + accountID resp, err := t.SendCommand(cmd) @@ -135,8 +141,8 @@ func (m *Manager) EnsureMargeAccountPaired(deviceIP, wantAccountID string, t Tel } target = generated - } else if !IsValidAccountID(target) { - return "", false, "", fmt.Errorf("invalid account id %q: must be exactly 7 digits", target) + } else if !datastore.IsSafeIdentifier(target) { + return "", false, "", fmt.Errorf("invalid account id %q: must be a non-empty, path-safe identifier", target) } _, pairLogs, pairErr := m.PairAccount(deviceIP, target, t) @@ -196,9 +202,18 @@ func (m *Manager) probeSetMargeAccount(deviceIP string) (bool, error) { func (m *Manager) postSetMargeAccount(deviceIP, accountID string) error { url := buildDeviceURL(deviceIP, "/setMargeAccount") + // accountID is XML-escaped rather than interpolated raw: + // datastore.IsSafeIdentifier already excludes '<', '>', '&', '\'', '"' + // (see #634), but escaping here too means this stays well-formed even + // if that gate is ever bypassed. + var escapedAccountID bytes.Buffer + if err := xml.EscapeText(&escapedAccountID, []byte(accountID)); err != nil { + return fmt.Errorf("escape account ID: %w", err) + } + body := fmt.Sprintf( `%saftertouch`, - accountID, + escapedAccountID.String(), ) client := &http.Client{ @@ -312,23 +327,6 @@ func buildDeviceURL(deviceIP, path string) string { return "http://" + deviceIP + ":8090" + path } -// IsValidAccountID reports whether s is a syntactically valid SoundTouch -// account ID — exactly 7 numeric digits, the format used by every -// Bose-cloud-issued ID we have observed in captures. -func IsValidAccountID(s string) bool { - if len(s) != 7 { - return false - } - - for _, ch := range s { - if ch < '0' || ch > '9' { - return false - } - } - - return true -} - // GenerateAccountID returns a fresh 7-digit account ID that does not collide // with any value in known. It uses crypto/rand and re-rolls on collision. func GenerateAccountID(known []string) (string, error) { diff --git a/pkg/service/setup/marge_pairing_test.go b/pkg/service/setup/marge_pairing_test.go index 1f737e8..ce46c11 100644 --- a/pkg/service/setup/marge_pairing_test.go +++ b/pkg/service/setup/marge_pairing_test.go @@ -10,6 +10,8 @@ import ( "strings" "testing" "time" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" ) // fakeDevice spins up an httptest.Server that pretends to be the SoundTouch @@ -313,7 +315,7 @@ func TestEnsureMargeAccountPaired_UnpairedGeneratesAndPairs(t *testing.T) { t.Error("alreadyPaired should be false for an unpaired device") } - if !IsValidAccountID(accountID) { + if !datastore.IsSafeIdentifier(accountID) { t.Errorf("accountID %q is not a valid generated ID", accountID) } @@ -352,7 +354,7 @@ func TestEnsureMargeAccountPaired_RejectsInvalidWantAccountID(t *testing.T) { m := NewManager("", nil, nil) - _, _, _, err := m.EnsureMargeAccountPaired(d.addr, "not-7-digits", nil) + _, _, _, err := m.EnsureMargeAccountPaired(d.addr, "not/valid", nil) if err == nil { t.Fatal("expected an error for an invalid --account value") } @@ -475,28 +477,9 @@ func TestPreflightInitPlan_UnrecognisedStatusFailsClosed(t *testing.T) { } } -func TestIsValidAccountID(t *testing.T) { - cases := []struct { - in string - want bool - }{ - {"1234567", true}, - {"0000000", true}, - {"9999999", true}, - {"", false}, - {"123456", false}, - {"12345678", false}, - {"123456a", false}, - {"-123456", false}, - {" 123456", false}, - } - - for _, tc := range cases { - if got := IsValidAccountID(tc.in); got != tc.want { - t.Errorf("IsValidAccountID(%q) = %v, want %v", tc.in, got, tc.want) - } - } -} +// Account-ID format validation is now solely datastore.IsSafeIdentifier's +// responsibility (see datastore.TestIsSafeIdentifier); setup no longer has +// its own account-ID validator to test. func TestGenerateAccountID_AvoidsCollisions(t *testing.T) { id, err := GenerateAccountID(nil) @@ -504,7 +487,7 @@ func TestGenerateAccountID_AvoidsCollisions(t *testing.T) { t.Fatalf("GenerateAccountID(nil): %v", err) } - if !IsValidAccountID(id) { + if !datastore.IsSafeIdentifier(id) { t.Errorf("generated ID %q is not valid", id) }
Account ID:${data.account.account_id}
Account ID:${escapeHtml(data.account.account_id)}
Language: @@ -1003,7 +1079,7 @@ async function fetchAccountDetails(accountId) { `; } - return `
  • ${s.key_name}: ${s.value}
    • `; + return `
  • ${escapeHtml(s.key_name)}: ${escapeHtml(s.value)}
    • `; }).join("")} @@ -1024,7 +1100,7 @@ async function fetchAccountDetails(accountId) { statusEl.style.color = "#666"; } try { - const response = await fetch(`/api/mgmt/accounts/${data.account.account_id}/language`, { + const response = await fetch(`/api/mgmt/accounts/${encodeURIComponent(data.account.account_id)}/language`, { method: "POST", headers: { "Content-Type": "application/json", @@ -1068,7 +1144,7 @@ async function fetchAccountDetails(accountId) { } try { - const response = await fetch(`/api/mgmt/accounts/${accID}/provider-settings`, { + const response = await fetch(`/api/mgmt/accounts/${encodeURIComponent(accID)}/provider-settings`, { method: "POST", headers: { "Content-Type": "application/json", @@ -1110,27 +1186,27 @@ async function fetchAccountDetails(accountId) { devicesEl.innerHTML = data.devices.map(device => `
    -
    -

    ${device.name || "Unnamed Device"} (${device.product_code})

    +
    +

    ${escapeHtml(device.name || "Unnamed Device")} (${escapeHtml(device.product_code)})

    - ${device.ip_address} | ${device.device_id} ▾ + ${escapeHtml(device.ip_address)} | ${escapeHtml(device.device_id)} ▾
    - `).join(""); + + // data-toggle-target (not an inline onclick) avoids re-embedding + // speaker-controlled device_id inside a JS-string-in-HTML-attribute + // context, which HTML-escaping alone cannot make safe. + devicesEl.querySelectorAll(".device-summary-header").forEach(el => { + el.addEventListener("click", () => toggleInfo(el.dataset.toggleTarget)); + }); } } catch (error) { - if (metadataEl) metadataEl.innerHTML = `Error: ${error.message}`; + if (metadataEl) metadataEl.innerHTML = `Error: ${escapeHtml(error.message)}`; console.error("Failed to fetch account details", error); } } @@ -1767,7 +1850,7 @@ async function fetchDeviceEvents(deviceId) { list.innerHTML = '
    Loading events...