From 6bc4ee1e738db44ddb839fe923f1b882221297eb Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 15 May 2026 18:58:09 +0200 Subject: [PATCH] fix(marge): reject rename PUT mismatch before persisting HandleMargeUpdateDevice used to call AddDeviceToAccount (an upsert) and only check body-vs-URL deviceID after the row was already written. A speaker sending a malformed PUT with the wrong deviceid attribute would still leave a spurious record before getting 400. Now we parse just the deviceid attribute, compare against the URL segment, and only call into the upsert when they match. The existing regression test gains two GetDeviceInfo assertions to lock the no-spurious-row guarantee in. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/service/handlers/handlers_marge.go | 22 ++++++++++++++----- .../handlers/issue285_regression_test.go | 11 ++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index e6927a6..927ca58 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -637,20 +637,32 @@ func (s *Server) HandleMargeUpdateDevice(w http.ResponseWriter, r *http.Request) return } - bodyDeviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + // Validate body deviceID against the URL segment *before* the + // upsert in AddDeviceToAccount runs — otherwise a mismatched PUT + // would still persist a row for the body's deviceID before the + // 400 response, leaving spurious state in the datastore. + var probe struct { + DeviceID string `xml:"deviceid,attr"` + } + if xmlErr := xml.Unmarshal(body, &probe); xmlErr != nil { + http.Error(w, xmlErr.Error(), http.StatusBadRequest) return } - if bodyDeviceID != device { + if probe.DeviceID != device { http.Error(w, - fmt.Sprintf("device ID in body (%q) does not match URL (%q)", bodyDeviceID, device), + fmt.Sprintf("device ID in body (%q) does not match URL (%q)", probe.DeviceID, device), http.StatusBadRequest) return } + _, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/vnd.bose.streaming-v1.2+xml") w.WriteHeader(http.StatusOK) _, _ = w.Write(data) diff --git a/pkg/service/handlers/issue285_regression_test.go b/pkg/service/handlers/issue285_regression_test.go index 7330a6b..4e63d5f 100644 --- a/pkg/service/handlers/issue285_regression_test.go +++ b/pkg/service/handlers/issue285_regression_test.go @@ -339,4 +339,15 @@ func TestIssue285_RenamePutRejectsMismatchedDeviceID(t *testing.T) { respBody, _ := io.ReadAll(resp.Body) t.Fatalf("PUT status = %d, want 400; body:\n%s", resp.StatusCode, respBody) } + + // Mismatched body must be rejected *before* the upsert runs — + // otherwise the datastore ends up with a row keyed on the body's + // deviceID even though we return 400. Verify by reading both keys. + if got, _ := ds.GetDeviceInfo("3981561", "DEADBEEFCAFE"); got != nil { + t.Fatalf("body deviceID DEADBEEFCAFE was persisted despite 400 response: %+v", got) + } + + if got, _ := ds.GetDeviceInfo("3981561", urlDeviceID); got != nil { + t.Fatalf("URL deviceID %s was persisted despite 400 response: %+v", urlDeviceID, got) + } }