diff --git a/pkg/service/datastore/datastore.go b/pkg/service/datastore/datastore.go index d0ddda2..8f8376d 100644 --- a/pkg/service/datastore/datastore.go +++ b/pkg/service/datastore/datastore.go @@ -3,10 +3,13 @@ package datastore import ( "bytes" + "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "encoding/xml" "fmt" + "io" "os" "path/filepath" "sort" @@ -1612,22 +1615,61 @@ func (ds *DataStore) GetETagForRecents(account, device string) int64 { return info.ModTime().UnixNano() / int64(time.Millisecond) } -// GetETagForAccount returns the highest ETag among presets, sources, and recents for the account and device. -func (ds *DataStore) GetETagForAccount(account, device string) int64 { - e1 := ds.GetETagForPresets(account, device) - e2 := ds.GetETagForSources(account, device) - e3 := ds.GetETagForRecents(account, device) +// contentHashForFiles returns a SHA-256 hex digest over the concatenated contents of the given file paths. +func contentHashForFiles(paths ...string) string { + h := sha256.New() - maxETag := e1 - if e2 > maxETag { - maxETag = e2 + for _, p := range paths { + f, err := os.Open(p) + if err != nil { + continue + } + + _, _ = io.Copy(h, f) + _ = f.Close() } - if e3 > maxETag { - maxETag = e3 + return hex.EncodeToString(h.Sum(nil)) +} + +// GetETagForAccount returns a content hash (SHA-256) over presets, sources, and recents for the account and device. +// If device is empty, it hashes across all devices in the account. +func (ds *DataStore) GetETagForAccount(account, device string) string { + if device != "" { + deviceDir := ds.AccountDeviceDir(account, device) + + return contentHashForFiles( + filepath.Join(deviceDir, constants.PresetsFile), + filepath.Join(deviceDir, constants.SourcesFile), + filepath.Join(deviceDir, constants.RecentsFile), + ) } - return maxETag + devicesDir := ds.AccountDevicesDir(account) + + // Ignore error: missing directory is treated as no devices, producing a + // stable non-empty hash rather than "" which would false-match an absent + // If-None-Match header and return 304 on the first request. + entries, _ := os.ReadDir(devicesDir) + + h := sha256.New() + + for _, entry := range entries { + if entry.IsDir() { + deviceDir := ds.AccountDeviceDir(account, entry.Name()) + for _, name := range []string{constants.PresetsFile, constants.SourcesFile, constants.RecentsFile} { + f, err := os.Open(filepath.Join(deviceDir, name)) + if err != nil { + continue + } + + _, _ = io.Copy(h, f) + _ = f.Close() + } + } + } + + return hex.EncodeToString(h.Sum(nil)) } // Settings represents the global service settings. diff --git a/pkg/service/handlers/handlers_etag_account_test.go b/pkg/service/handlers/handlers_etag_account_test.go new file mode 100644 index 0000000..5c800ec --- /dev/null +++ b/pkg/service/handlers/handlers_etag_account_test.go @@ -0,0 +1,235 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +// TestMargeAccountETagNoDevice tests ETag correctness on the account-level endpoints +// when no ?device= query parameter is provided (the "no-device" code path in GetETagForAccount). +// +// These tests are specifically for the bug where: +// - GetETagForAccount returns "" when the devices directory does not exist +// - A missing If-None-Match header also produces "" via Header.Get +// - The equality check "" == "" causes a false 304 on the very first request +func TestMargeAccountETagNoDevice(t *testing.T) { + newServer := func(t *testing.T) (ts *httptest.Server, tempDir string) { + t.Helper() + tempDir, _ = os.MkdirTemp("", "st-etag-nodev-*") + t.Cleanup(func() { _ = os.RemoveAll(tempDir) }) + ds := datastore.NewDataStore(tempDir) + r, _ := setupRouter("http://localhost:8001", ds) + ts = httptest.NewServer(r) + t.Cleanup(ts.Close) + return ts, tempDir + } + + writeDeviceFiles := func(t *testing.T, tempDir, account, deviceID string) string { + t.Helper() + deviceDir := filepath.Join(tempDir, "accounts", account, "devices", deviceID) + _ = os.MkdirAll(deviceDir, 0755) + _ = os.WriteFile(filepath.Join(deviceDir, "Presets.xml"), []byte(""), 0644) + _ = os.WriteFile(filepath.Join(deviceDir, "Sources.xml"), []byte(""), 0644) + _ = os.WriteFile(filepath.Join(deviceDir, "Recents.xml"), []byte(""), 0644) + return filepath.Join(deviceDir, "Presets.xml") + } + + // Bug: GetETagForAccount returns "" when the devices directory does not exist. + // Handler then checks: r.Header.Get("If-None-Match") == "" which is true on any + // request without the header, causing a 304 before the client has any cached content. + t.Run("first request without If-None-Match never returns 304 when devices dir is missing", func(t *testing.T) { + ts, _ := newServer(t) + // Deliberately no directories created for this account. + res, err := http.Get(ts.URL + "/marge/accounts/no-devices-account/full") + if err != nil { + t.Fatal(err) + } + _ = res.Body.Close() + + if res.StatusCode == http.StatusNotModified { + t.Error("got 304 on first request without If-None-Match — empty ETag matched empty header value") + } + if res.StatusCode != http.StatusOK { + t.Errorf("expected 200, got %v", res.Status) + } + }) + + t.Run("ETag is non-empty even when devices dir does not exist", func(t *testing.T) { + ts, _ := newServer(t) + res, err := http.Get(ts.URL + "/marge/accounts/no-devices-account/full") + if err != nil { + t.Fatal(err) + } + _ = res.Body.Close() + + if etag := res.Header.Get("ETag"); etag == "" { + t.Error("ETag must not be empty string — empty ETag causes false 304 on first request") + } + }) + + t.Run("ETag is non-empty when devices dir exists but contains no device subdirs", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "empty-devices-account" + _ = os.MkdirAll(filepath.Join(tempDir, "accounts", account, "devices"), 0755) + + res, err := http.Get(ts.URL + "/marge/accounts/" + account + "/full") + if err != nil { + t.Fatal(err) + } + _ = res.Body.Close() + + if etag := res.Header.Get("ETag"); etag == "" { + t.Error("ETag must not be empty string when devices dir exists but is empty") + } + }) + + t.Run("ETag is stable across multiple requests when data does not change", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "stable-etag-account" + writeDeviceFiles(t, tempDir, account, "DEV1") + url := ts.URL + "/marge/accounts/" + account + "/full" + + res1, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag1 := res1.Header.Get("ETag") + _ = res1.Body.Close() + + res2, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag2 := res2.Header.Get("ETag") + _ = res2.Body.Close() + + if etag1 == "" { + t.Fatal("ETag must not be empty") + } + if etag1 != etag2 { + t.Errorf("ETag changed between identical requests: %q → %q", etag1, etag2) + } + }) + + t.Run("304 flow works correctly with no device param", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "304-flow-account" + writeDeviceFiles(t, tempDir, account, "DEV1") + url := ts.URL + "/marge/accounts/" + account + "/full" + + res1, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag := res1.Header.Get("ETag") + _ = res1.Body.Close() + + if etag == "" { + t.Fatal("expected non-empty ETag from first request") + } + + req, _ := http.NewRequest("GET", url, nil) + req.Header.Set("If-None-Match", etag) + res2, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + defer func() { _ = res2.Body.Close() }() + + if res2.StatusCode != http.StatusNotModified { + t.Errorf("expected 304 with valid ETag, got %v", res2.Status) + } + }) + + t.Run("ETag changes when file content changes", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "changing-data-account" + presetsFile := writeDeviceFiles(t, tempDir, account, "DEV1") + url := ts.URL + "/marge/accounts/" + account + "/full" + + res1, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag1 := res1.Header.Get("ETag") + _ = res1.Body.Close() + + _ = os.WriteFile(presetsFile, []byte(``), 0644) + + res2, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag2 := res2.Header.Get("ETag") + _ = res2.Body.Close() + + if etag1 == etag2 { + t.Errorf("ETag did not change after file modification: %q", etag1) + } + }) + + t.Run("stale ETag returns 200 after data change", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "stale-etag-account" + presetsFile := writeDeviceFiles(t, tempDir, account, "DEV1") + url := ts.URL + "/marge/accounts/" + account + "/full" + + res1, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + etag1 := res1.Header.Get("ETag") + _ = res1.Body.Close() + + _ = os.WriteFile(presetsFile, []byte(``), 0644) + + req, _ := http.NewRequest("GET", url, nil) + req.Header.Set("If-None-Match", etag1) + res2, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + defer func() { _ = res2.Body.Close() }() + + if res2.StatusCode != http.StatusOK { + t.Errorf("expected 200 for stale ETag after data change, got %v", res2.Status) + } + }) + + // The /full, /sources, and /devices endpoints all call GetETagForAccount(account, "") + // so they should return the same ETag for the same account state. + t.Run("ETag is consistent across full, sources, and devices endpoints", func(t *testing.T) { + ts, tempDir := newServer(t) + account := "consistent-etag-account" + writeDeviceFiles(t, tempDir, account, "DEV1") + + get := func(path string) string { + res, err := http.Get(ts.URL + path) + if err != nil { + t.Fatal(err) + } + etag := res.Header.Get("ETag") + _ = res.Body.Close() + return etag + } + + etagFull := get("/marge/accounts/" + account + "/full") + etagSources := get("/marge/accounts/" + account + "/sources") + etagDevices := get("/marge/accounts/" + account + "/devices") + + if etagFull == "" { + t.Fatal("ETag from /full must not be empty") + } + if etagFull != etagSources { + t.Errorf("/full ETag %q != /sources ETag %q", etagFull, etagSources) + } + if etagFull != etagDevices { + t.Errorf("/full ETag %q != /devices ETag %q", etagFull, etagDevices) + } + }) +} diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index 6da5fbb..8be7d5f 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -151,7 +151,7 @@ func (s *Server) HandleMargeAccountFull(w http.ResponseWriter, r *http.Request) device := r.URL.Query().Get("device") - etag := strconv.FormatInt(s.ds.GetETagForAccount(account, device), 10) + etag := s.ds.GetETagForAccount(account, device) if r.Header.Get("If-None-Match") == etag { w.WriteHeader(http.StatusNotModified) return @@ -174,7 +174,7 @@ func (s *Server) HandleMargeAccountSources(w http.ResponseWriter, r *http.Reques device := r.URL.Query().Get("device") - etag := strconv.FormatInt(s.ds.GetETagForAccount(account, device), 10) + etag := s.ds.GetETagForAccount(account, device) if r.Header.Get("If-None-Match") == etag { w.WriteHeader(http.StatusNotModified) return @@ -197,7 +197,7 @@ func (s *Server) HandleMargeAccountDevices(w http.ResponseWriter, r *http.Reques device := r.URL.Query().Get("device") - etag := strconv.FormatInt(s.ds.GetETagForAccount(account, device), 10) + etag := s.ds.GetETagForAccount(account, device) if r.Header.Get("If-None-Match") == etag { w.WriteHeader(http.StatusNotModified) return