diff --git a/main_test.go b/main_test.go index caa3e5e..a1b6ed0 100644 --- a/main_test.go +++ b/main_test.go @@ -143,13 +143,13 @@ var _ = Describe("discordPlugin", func() { host.SchedulerMock.On("CancelSchedule", "testuser-clear").Return(nil) // Cache mocks (Discord image processing) - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) + host.CacheMock.On("SetString", discordImageKey, mock.Anything, mock.Anything).Return(nil) host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("https://example.com/art.jpg", nil) // Mock HTTP POST requests (Discord external assets API) postReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(postReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(postReq) pdk.PDKMock.On("Send", postReq).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`{}`))) // Schedule clear activity callback @@ -196,11 +196,11 @@ var _ = Describe("discordPlugin", func() { host.SchedulerMock.On("CancelSchedule", "testuser-clear").Return(nil) // Cache mocks (Discord image processing) - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) + host.CacheMock.On("SetString", discordImageKey, mock.Anything, mock.Anything).Return(nil) host.ArtworkMock.On("GetTrackUrl", "track1", int32(300)).Return("https://example.com/art.jpg", nil) postReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(postReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(postReq) pdk.PDKMock.On("Send", postReq).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`{}`))) host.SchedulerMock.On("ScheduleOneTime", mock.Anything, payloadClearActivity, "testuser-clear").Return("testuser-clear", nil) diff --git a/plugin_suite_test.go b/plugin_suite_test.go index ad01992..8f69496 100644 --- a/plugin_suite_test.go +++ b/plugin_suite_test.go @@ -1,13 +1,22 @@ package main import ( + "strings" "testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" ) func TestDiscordPlugin(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Discord Plugin Main Suite") } + +// Shared matchers for tighter mock expectations across all test files. +var ( + discordImageKey = mock.MatchedBy(func(key string) bool { return strings.HasPrefix(key, "discord.image.") }) + externalAssetsURL = mock.MatchedBy(func(url string) bool { return strings.Contains(url, "external-assets") }) + spotifyURLKey = mock.MatchedBy(func(key string) bool { return strings.HasPrefix(key, "spotify.url.") }) +) diff --git a/rpc.go b/rpc.go index 3305039..2a18690 100644 --- a/rpc.go +++ b/rpc.go @@ -6,6 +6,8 @@ package main import ( + "crypto/md5" + "encoding/hex" "encoding/json" "fmt" "strings" @@ -130,7 +132,8 @@ func (r *discordRPC) processImage(imageURL, clientID, token string, ttl int64) ( } // Check cache first - cacheKey := fmt.Sprintf("discord.image.%x", imageURL) + h := md5.Sum([]byte(imageURL)) + cacheKey := "discord.image." + hex.EncodeToString(h[:8]) cachedValue, exists, err := host.CacheGetString(cacheKey) if err == nil && exists { pdk.Log(pdk.LogDebug, fmt.Sprintf("Cache hit for image URL: %s", imageURL)) diff --git a/rpc_test.go b/rpc_test.go index 258e9c9..fbee7a3 100644 --- a/rpc_test.go +++ b/rpc_test.go @@ -260,13 +260,13 @@ var _ = Describe("discordRPC", func() { }) It("processes image via Discord API and caches result", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.MatchedBy(func(val string) bool { + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) + host.CacheMock.On("SetString", discordImageKey, mock.MatchedBy(func(val string) bool { return val == "mp:external/new-asset" }), int64(imageCacheTTL)).Return(nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`[{"external_asset_path":"external/new-asset"}]`))) result, err := r.processImage("https://example.com/art.jpg", "client123", "token123", imageCacheTTL) @@ -275,10 +275,10 @@ var _ = Describe("discordRPC", func() { }) It("returns error on HTTP failure", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(500, nil, []byte(`error`))) _, err := r.processImage("https://example.com/art.jpg", "client123", "token123", imageCacheTTL) @@ -287,10 +287,10 @@ var _ = Describe("discordRPC", func() { }) It("returns error on unmarshal failure", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`{"not":"an-array"}`))) _, err := r.processImage("https://example.com/art.jpg", "client123", "token123", imageCacheTTL) @@ -299,10 +299,10 @@ var _ = Describe("discordRPC", func() { }) It("returns error on empty response array", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`[]`))) _, err := r.processImage("https://example.com/art.jpg", "client123", "token123", imageCacheTTL) @@ -311,10 +311,10 @@ var _ = Describe("discordRPC", func() { }) It("returns error on empty external_asset_path", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`[{"external_asset_path":""}]`))) _, err := r.processImage("https://example.com/art.jpg", "client123", "token123", imageCacheTTL) @@ -329,11 +329,11 @@ var _ = Describe("discordRPC", func() { }) It("sends activity with track artwork and SmallImage overlay", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) + host.CacheMock.On("SetString", discordImageKey, mock.Anything, mock.Anything).Return(nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`[{"external_asset_path":"external/art"}]`))) host.WebSocketMock.On("SendText", "testuser", mock.MatchedBy(func(msg string) bool { @@ -361,17 +361,17 @@ var _ = Describe("discordRPC", func() { It("falls back to default image and clears SmallImage", func() { // Track art fails (HTTP error), default image succeeds - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) + host.CacheMock.On("SetString", discordImageKey, mock.Anything, mock.Anything).Return(nil) trackReq := &pdk.HTTPRequest{} defaultReq := &pdk.HTTPRequest{} // First call (track art) returns 500, second call (default) succeeds - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(trackReq).Once() + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(trackReq).Once() pdk.PDKMock.On("Send", trackReq).Return(pdk.NewStubHTTPResponse(500, nil, []byte(`error`))).Once() - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(defaultReq).Once() + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(defaultReq).Once() pdk.PDKMock.On("Send", defaultReq).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`[{"external_asset_path":"external/logo"}]`))).Once() host.WebSocketMock.On("SendText", "testuser", mock.MatchedBy(func(msg string) bool { @@ -398,10 +398,10 @@ var _ = Describe("discordRPC", func() { }) It("clears all images when both track art and default fail", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil) httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(200, nil, []byte(`{"not":"array"}`))) host.WebSocketMock.On("SendText", "testuser", mock.MatchedBy(func(msg string) bool { @@ -428,15 +428,11 @@ var _ = Describe("discordRPC", func() { It("handles SmallImage processing failure gracefully", func() { // LargeImage from cache (succeeds), SmallImage API fails - host.CacheMock.On("GetString", mock.MatchedBy(func(key string) bool { - return strings.HasPrefix(key, "discord.image.") - })).Return("mp:cached/large", true, nil).Once() - host.CacheMock.On("GetString", mock.MatchedBy(func(key string) bool { - return strings.HasPrefix(key, "discord.image.") - })).Return("", false, nil).Once() + host.CacheMock.On("GetString", discordImageKey).Return("mp:cached/large", true, nil).Once() + host.CacheMock.On("GetString", discordImageKey).Return("", false, nil).Once() httpReq := &pdk.HTTPRequest{} - pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, mock.Anything).Return(httpReq) + pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, externalAssetsURL).Return(httpReq) pdk.PDKMock.On("Send", mock.Anything).Return(pdk.NewStubHTTPResponse(500, nil, []byte(`error`))) host.WebSocketMock.On("SendText", "testuser", mock.MatchedBy(func(msg string) bool { diff --git a/spotify.go b/spotify.go index cba0a4b..8f618bd 100644 --- a/spotify.go +++ b/spotify.go @@ -1,11 +1,12 @@ package main import ( - "crypto/sha256" + "crypto/md5" "encoding/hex" "encoding/json" "fmt" "net/url" + "regexp" "strings" "github.com/navidrome/navidrome/plugins/pdk/go/host" @@ -26,7 +27,7 @@ type listenBrainzResult struct { // buildSpotifySearchURL constructs a Spotify search URL using artist and title. // Used as the ultimate fallback when ListenBrainz resolution fails. -func buildSpotifySearchURL(title, artist string) string { +func buildSpotifySearchURL(artist, title string) string { query := strings.TrimSpace(strings.Join([]string{artist, title}, " ")) if query == "" { return "https://open.spotify.com/search/" @@ -45,7 +46,7 @@ func spotifySearch(term string) string { // spotifyCacheKey returns a deterministic cache key for a track's Spotify URL. func spotifyCacheKey(artist, title, album string) string { - h := sha256.Sum256([]byte(strings.ToLower(artist) + "\x00" + strings.ToLower(title) + "\x00" + strings.ToLower(album))) + h := md5.Sum([]byte(strings.ToLower(artist) + "\x00" + strings.ToLower(title) + "\x00" + strings.ToLower(album))) return "spotify.url." + hex.EncodeToString(h[:8]) } @@ -101,7 +102,7 @@ func parseSpotifyID(body []byte) string { } for _, r := range results { for _, id := range r.SpotifyTrackIDs { - if id != "" { + if isValidSpotifyID(id) { return id } } @@ -109,6 +110,13 @@ func parseSpotifyID(body []byte) string { return "" } +// isValidSpotifyID checks that a Spotify track ID contains only base-62 characters. +var spotifyIDRegex = regexp.MustCompile(`^[0-9A-Za-z]+$`) + +func isValidSpotifyID(id string) bool { + return spotifyIDRegex.MatchString(id) +} + // resolveSpotifyURL resolves a direct Spotify track URL via ListenBrainz Labs, // falling back to a search URL. Results are cached. func resolveSpotifyURL(track scrobbler.TrackInfo) string { @@ -150,7 +158,7 @@ func resolveSpotifyURL(track scrobbler.TrackInfo) string { } // 3. Fallback to search URL - searchURL := buildSpotifySearchURL(track.Title, track.Artist) + searchURL := buildSpotifySearchURL(track.Artist, track.Title) _ = host.CacheSetString(cacheKey, searchURL, spotifyCacheTTLMiss) pdk.Log(pdk.LogInfo, fmt.Sprintf("Spotify resolution missed, falling back to search URL for %q - %q: %s", primary, track.Title, searchURL)) return searchURL diff --git a/spotify_test.go b/spotify_test.go index 95fb6de..d825dc3 100644 --- a/spotify_test.go +++ b/spotify_test.go @@ -33,17 +33,17 @@ var _ = Describe("Spotify", func() { Describe("buildSpotifySearchURL", func() { DescribeTable("constructs Spotify search URL", - func(title, artist, expectedSubstring string) { - url := buildSpotifySearchURL(title, artist) + func(artist, title, expectedSubstring string) { + url := buildSpotifySearchURL(artist, title) Expect(url).To(HavePrefix("https://open.spotify.com/search/")) if expectedSubstring != "" { Expect(url).To(ContainSubstring(expectedSubstring)) } }, - Entry("artist and title", "Never Gonna Give You Up", "Rick Astley", "Rick%20Astley"), - Entry("another track", "Karma Police", "Radiohead", "Radiohead"), - Entry("empty title", "", "Solo Artist", "Solo%20Artist"), - Entry("empty artist", "Only Title", "", "Only%20Title"), + Entry("artist and title", "Rick Astley", "Never Gonna Give You Up", "Rick%20Astley"), + Entry("another track", "Radiohead", "Karma Police", "Radiohead"), + Entry("empty artist", "", "Only Title", "Only%20Title"), + Entry("empty title", "Solo Artist", "", "Solo%20Artist"), Entry("both empty", "", "", ""), ) }) @@ -93,7 +93,23 @@ var _ = Describe("Spotify", func() { Entry("invalid JSON", `not json`, ""), Entry("null first result falls through to second", - `[{"spotify_track_ids":[]},{"spotify_track_ids":["abc123"]}]`, "abc123"), + `[{"spotify_track_ids":[]},{"spotify_track_ids":["6vN77lE9LK6HP2DewaN6HZ"]}]`, "6vN77lE9LK6HP2DewaN6HZ"), + Entry("skips invalid ID with special characters", + `[{"spotify_track_ids":["abc!@#$%^&*()_+=-12345"]}]`, ""), + ) + }) + + Describe("isValidSpotifyID", func() { + DescribeTable("validates Spotify track IDs", + func(id string, expected bool) { + Expect(isValidSpotifyID(id)).To(Equal(expected)) + }, + Entry("valid 22-char ID", "6vN77lE9LK6HP2DewaN6HZ", true), + Entry("another valid ID", "4tIGK5G9hNDA50ZdGioZRG", true), + Entry("short valid ID", "abc123", true), + Entry("special characters", "6vN77lE9!K6HP2DewaN6HZ", false), + Entry("spaces", "6vN77 E9LK6HP2DewaN6HZ", false), + Entry("empty string", "", false), ) }) @@ -128,7 +144,7 @@ var _ = Describe("Spotify", func() { }) It("returns cached URL on cache hit", func() { - host.CacheMock.On("GetString", mock.Anything).Return("https://open.spotify.com/track/cached123", true, nil) + host.CacheMock.On("GetString", spotifyURLKey).Return("https://open.spotify.com/track/cached123", true, nil) url := resolveSpotifyURL(scrobbler.TrackInfo{ Title: "Karma Police", @@ -139,14 +155,14 @@ var _ = Describe("Spotify", func() { }) It("resolves via MBID when available", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", spotifyURLKey).Return("", false, nil) + host.CacheMock.On("SetString", spotifyURLKey, mock.Anything, mock.Anything).Return(nil) // Mock the MBID HTTP request mbidReq := &pdk.HTTPRequest{} pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, "https://labs.api.listenbrainz.org/spotify-id-from-mbid/json").Return(mbidReq) pdk.PDKMock.On("Send", mbidReq).Return(pdk.NewStubHTTPResponse(200, nil, - []byte(`[{"spotify_track_ids":["track123"]}]`))) + []byte(`[{"spotify_track_ids":["63OQupATfueTdZMWIV7nzz"]}]`))) url := resolveSpotifyURL(scrobbler.TrackInfo{ Title: "Karma Police", @@ -154,13 +170,13 @@ var _ = Describe("Spotify", func() { Album: "OK Computer", MBZRecordingID: "mbid-123", }) - Expect(url).To(Equal("https://open.spotify.com/track/track123")) - host.CacheMock.AssertCalled(GinkgoT(), "SetString", mock.Anything, "https://open.spotify.com/track/track123", spotifyCacheTTLHit) + Expect(url).To(Equal("https://open.spotify.com/track/63OQupATfueTdZMWIV7nzz")) + host.CacheMock.AssertCalled(GinkgoT(), "SetString", spotifyURLKey, "https://open.spotify.com/track/63OQupATfueTdZMWIV7nzz", spotifyCacheTTLHit) }) It("falls back to metadata lookup when MBID fails", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", spotifyURLKey).Return("", false, nil) + host.CacheMock.On("SetString", spotifyURLKey, mock.Anything, mock.Anything).Return(nil) // MBID request fails mbidReq := &pdk.HTTPRequest{} @@ -171,7 +187,7 @@ var _ = Describe("Spotify", func() { metaReq := &pdk.HTTPRequest{} pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, "https://labs.api.listenbrainz.org/spotify-id-from-metadata/json").Return(metaReq) pdk.PDKMock.On("Send", metaReq).Return(pdk.NewStubHTTPResponse(200, nil, - []byte(`[{"spotify_track_ids":["meta456"]}]`))) + []byte(`[{"spotify_track_ids":["4wlLbLeDWbA6TzwZFp1UaK"]}]`))) url := resolveSpotifyURL(scrobbler.TrackInfo{ Title: "Karma Police", @@ -179,12 +195,12 @@ var _ = Describe("Spotify", func() { Album: "OK Computer", MBZRecordingID: "mbid-123", }) - Expect(url).To(Equal("https://open.spotify.com/track/meta456")) + Expect(url).To(Equal("https://open.spotify.com/track/4wlLbLeDWbA6TzwZFp1UaK")) }) It("falls back to search URL when both lookups fail", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", spotifyURLKey).Return("", false, nil) + host.CacheMock.On("SetString", spotifyURLKey, mock.Anything, mock.Anything).Return(nil) // No MBID, metadata request fails metaReq := &pdk.HTTPRequest{} @@ -198,17 +214,17 @@ var _ = Describe("Spotify", func() { }) Expect(url).To(HavePrefix("https://open.spotify.com/search/")) Expect(url).To(ContainSubstring("Radiohead")) - host.CacheMock.AssertCalled(GinkgoT(), "SetString", mock.Anything, mock.Anything, spotifyCacheTTLMiss) + host.CacheMock.AssertCalled(GinkgoT(), "SetString", spotifyURLKey, mock.Anything, spotifyCacheTTLMiss) }) It("uses Artists fallback when primary artist parse is empty", func() { - host.CacheMock.On("GetString", mock.Anything).Return("", false, nil) - host.CacheMock.On("SetString", mock.Anything, mock.Anything, mock.Anything).Return(nil) + host.CacheMock.On("GetString", spotifyURLKey).Return("", false, nil) + host.CacheMock.On("SetString", spotifyURLKey, mock.Anything, mock.Anything).Return(nil) metaReq := &pdk.HTTPRequest{} pdk.PDKMock.On("NewHTTPRequest", pdk.MethodPost, "https://labs.api.listenbrainz.org/spotify-id-from-metadata/json").Return(metaReq) pdk.PDKMock.On("Send", metaReq).Return(pdk.NewStubHTTPResponse(200, nil, - []byte(`[{"spotify_track_ids":["fromArtists789"]}]`))) + []byte(`[{"spotify_track_ids":["4tIGK5G9hNDA50ZdGioZRG"]}]`))) url := resolveSpotifyURL(scrobbler.TrackInfo{ Title: "Some Song", @@ -216,7 +232,7 @@ var _ = Describe("Spotify", func() { Album: "Some Album", Artists: []scrobbler.ArtistRef{{Name: "Fallback Artist"}}, }) - Expect(url).To(Equal("https://open.spotify.com/track/fromArtists789")) + Expect(url).To(Equal("https://open.spotify.com/track/4tIGK5G9hNDA50ZdGioZRG")) }) }) })