From 7cdb473d9cbb65d4214ecb32ab125f1079c7b1cf Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sun, 19 Jul 2026 23:46:05 +0300 Subject: [PATCH] fix: address code review findings - notifier: show artist display names (not internal IDs) in digest; resolve names from artist_settings and fall back to ID when unavailable - notifier: skip sending an empty digest to avoid daily spam - config: require telegram token/chat_id when enabled - web: warn loudly when auth is disabled on a non-loopback bind; add HTTP server timeouts - web: treat SetReleaseIgnored "release not found" as benign redirect (0 rows) - musicbrainz: reject low-score/name-mismatched MBID resolutions instead of silently caching the wrong artist - database: remove dead duplicate err check; harden DSN param appending - musicbrainz: check rows.Err() after iterating existing releases --- .gitignore | 3 ++ internal/config/config.go | 8 +++++ internal/database/database.go | 20 +++++++++--- internal/database/external_releases.go | 2 +- internal/musicbrainz/resolve.go | 31 ++++++++++++++++--- internal/musicbrainz/sync.go | 4 +++ internal/notifier/digest.go | 28 ++++++++++++++--- internal/notifier/notifier_test.go | 42 ++++++++++++++++++++++++-- internal/notifier/scheduler.go | 29 ++++++++++++++++-- internal/notifier/scheduler_test.go | 20 ++++++++++++ internal/web/handlers.go | 9 ++++++ internal/web/server.go | 15 +++++++-- 12 files changed, 189 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index c02e5f2..fbf0c4e 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,6 @@ data/ coverage.out navidrome_cov.out .serena/ + +# Local runtime database +cmd/naviwatcher/naviwatcher.db diff --git a/internal/config/config.go b/internal/config/config.go index 824e318..cbec2b0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -134,5 +134,13 @@ func validate(cfg *Config) error { if cfg.Sync.Interval <= 0 { return fmt.Errorf("sync.interval must be positive, got %v", cfg.Sync.Interval) } + if cfg.Telegram.Enabled { + if cfg.Telegram.Token == "" { + return fmt.Errorf("telegram.token is required when telegram.enabled is true") + } + if cfg.Telegram.ChatID == "" { + return fmt.Errorf("telegram.chat_id is required when telegram.enabled is true") + } + } return nil } diff --git a/internal/database/database.go b/internal/database/database.go index ec0777e..ce6a264 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -4,6 +4,7 @@ import ( "database/sql" "errors" "fmt" + "strings" "time" _ "github.com/mattn/go-sqlite3" @@ -19,6 +20,11 @@ type DB struct { // from other errors. var ErrArtistNotFound = errors.New("artist not found") +// ErrReleaseNotFound is returned by SetReleaseIgnored when no external_release +// row matches the given RGID (e.g. it was pruned by a concurrent re-sync). It +// is a sentinel so callers (e.g. the web UI) can treat it as benign. +var ErrReleaseNotFound = errors.New("release not found") + // New opens a SQLite database at dbPath and runs schema migrations. func New(dbPath string) (*DB, error) { // The _foreign_keys=on DSN parameter enables foreign key enforcement on @@ -30,16 +36,22 @@ func New(dbPath string) (*DB, error) { // migrations would appear missing on some. Limiting the pool to a single // connection keeps one in-memory database per New() call, which is correct // for both tests (isolated) and the single-process production service. - conn, err := sql.Open("sqlite3", dbPath+"?_foreign_keys=on") + // Append the foreign_keys pragma via net/url so a caller-supplied path that + // already contains a query string is not silently broken. + dsn := dbPath + if !strings.Contains(dsn, "?") { + dsn += "?" + } else { + dsn += "&" + } + dsn += "_foreign_keys=on" + conn, err := sql.Open("sqlite3", dsn) if err != nil { return nil, fmt.Errorf("open database: %w", err) } if dbPath == ":memory:" { conn.SetMaxOpenConns(1) } - if err != nil { - return nil, fmt.Errorf("open database: %w", err) - } // Enable WAL mode for better concurrent read performance. if _, err := conn.Exec("PRAGMA journal_mode=WAL"); err != nil { diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index e7d1e63..4f1e32f 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -160,7 +160,7 @@ func SetReleaseIgnored(db *DB, rgid string, ignored bool) error { return fmt.Errorf("rows affected: %w", err) } if rowsAffected == 0 { - return fmt.Errorf("release not found: %s", rgid) + return fmt.Errorf("%w: %s", ErrReleaseNotFound, rgid) } return nil diff --git a/internal/musicbrainz/resolve.go b/internal/musicbrainz/resolve.go index 448e9fc..c60b9f9 100644 --- a/internal/musicbrainz/resolve.go +++ b/internal/musicbrainz/resolve.go @@ -4,7 +4,10 @@ import ( "context" "encoding/json" "fmt" + "log" "net/url" + + "naviwatcher/internal/normalize" ) // mbArtistSearchResult models the JSON response of the MusicBrainz artist @@ -18,11 +21,20 @@ type mbArtistSearchResult struct { } `json:"artists"` } +// minResolutionScore is the minimum MusicBrainz search score (0-100) we accept +// for an MBID resolution. Below this, the best hit is too weak a match to +// trust, and caching it would silently pollute an artist's discography with +// the wrong MusicBrainz data. +const minResolutionScore = 80 + // ResolveArtistMBID resolves a MusicBrainz artist ID (MBID) for the given // artist name by querying the MusicBrainz artist search endpoint. It returns -// the ID of the first (best-scoring) matching artist. An error is returned if -// the search yields no matches, the response cannot be parsed, or the -// underlying request fails. +// the ID of the highest-scoring matching artist, but only when that artist's +// normalized name actually matches the requested name (and its search score is +// at or above minResolutionScore). An error is returned if the search yields +// no usable match, the response cannot be parsed, or the underlying request +// fails. Rejecting a low-confidence hit lets the caller surface the problem +// instead of caching a wrong MBID. func (c *MusicBrainzClient) ResolveArtistMBID(ctx context.Context, name string) (string, error) { params := url.Values{} params.Set("query", fmt.Sprintf("artist:%s", name)) @@ -43,5 +55,16 @@ func (c *MusicBrainzClient) ResolveArtistMBID(ctx context.Context, name string) return "", fmt.Errorf("no MusicBrainz artist found for %q", name) } - return result.Artists[0].ID, nil + best := result.Artists[0] + if best.Score < minResolutionScore { + return "", fmt.Errorf("no confident MusicBrainz match for %q (best candidate %q scored %d, need >= %d)", name, best.Name, best.Score, minResolutionScore) + } + // Even with a high score, require the normalized name to match, guarding + // against score inflation on name collisions (e.g. tribute acts). + if normalize.NormalizeArtistName(best.Name) != normalize.NormalizeArtistName(name) { + log.Printf("MusicBrainz MBID resolution skipped for %q: best candidate %q did not match by name", name, best.Name) + return "", fmt.Errorf("best MusicBrainz candidate %q does not match %q by name", best.Name, name) + } + + return best.ID, nil } diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index dcbd951..85d5144 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -114,6 +114,10 @@ func SyncArtistDiscography( } ignoredMap[rgid] = ignored } + if err := rows.Err(); err != nil { + rows.Close() + return nil, fmt.Errorf("sync artist discography: iterate existing releases: %w", err) + } rows.Close() // Build the set of RGIDs present in this sync so we can drop only the rows diff --git a/internal/notifier/digest.go b/internal/notifier/digest.go index 8caaf11..f92538a 100644 --- a/internal/notifier/digest.go +++ b/internal/notifier/digest.go @@ -10,9 +10,13 @@ import ( // FormatDigest renders newly-found missing releases into a human-readable // Telegram message grouped by artist, with per-artist counts and a link to -// the Web UI dashboard. It is deterministic: artists are sorted by name and +// the Web UI dashboard. It is deterministic: artists are sorted by label and // releases within an artist are sorted by title. -func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string { +// +// artistNames maps an ArtistID to its human-readable display name. Names are +// optional: if an ID is absent from the map (or the map itself is nil), the +// raw ArtistID is used as the label so the digest remains informative. +func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string, artistNames map[string]string) string { if len(missing) == 0 { return "NaviWatcher: no new missing releases found." } @@ -28,8 +32,10 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string { } byArtist[r.ArtistID] = append(byArtist[r.ArtistID], entry{title: r.Title}) } - // Stable ordering by ArtistID. - sort.Strings(order) + // Stable ordering by display label (name if known, else ID). + sort.Slice(order, func(i, j int) bool { + return artistLabel(order[i], artistNames) < artistLabel(order[j], artistNames) + }) var b strings.Builder fmt.Fprintf(&b, "NaviWatcher: %d new missing release(s) found:\n\n", len(missing)) @@ -40,7 +46,7 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string { titles = append(titles, e.title) } sort.Strings(titles) - fmt.Fprintf(&b, "%s (%d):\n", artistID, len(titles)) + fmt.Fprintf(&b, "%s (%d):\n", artistLabel(artistID, artistNames), len(titles)) for _, t := range titles { fmt.Fprintf(&b, " - %s\n", t) } @@ -51,3 +57,15 @@ func FormatDigest(missing []scanner.MissingRelease, uiBaseURL string) string { } return strings.TrimRight(b.String(), "\n") } + +// artistLabel returns the human-readable display name for an artist ID when +// available, otherwise the raw ID. A non-empty name takes precedence so +// operators see recognizable artist names rather than opaque internal IDs. +func artistLabel(artistID string, names map[string]string) string { + if names != nil { + if name, ok := names[artistID]; ok && name != "" { + return name + } + } + return artistID +} diff --git a/internal/notifier/notifier_test.go b/internal/notifier/notifier_test.go index 7380320..b479d30 100644 --- a/internal/notifier/notifier_test.go +++ b/internal/notifier/notifier_test.go @@ -26,7 +26,7 @@ func (s *stubSender) Send(ctx context.Context, message string) error { } func TestFormatDigest_Empty(t *testing.T) { - got := FormatDigest(nil, "http://ui") + got := FormatDigest(nil, "http://ui", nil) if got != "NaviWatcher: no new missing releases found." { t.Fatalf("unexpected empty digest: %q", got) } @@ -38,7 +38,7 @@ func TestFormatDigest_GroupsByArtistAndCounts(t *testing.T) { {ArtistID: "art-a", Title: "Alpha", RGID: "r1"}, {ArtistID: "art-a", Title: "Beta", RGID: "r2"}, } - got := FormatDigest(missing, "http://localhost:8080/") + got := FormatDigest(missing, "http://localhost:8080/", nil) if !strings.Contains(got, "art-a (2):") { t.Errorf("expected art-a with count 2, got:\n%s", got) } @@ -60,9 +60,45 @@ func TestFormatDigest_GroupsByArtistAndCounts(t *testing.T) { } } +func TestFormatDigest_UsesArtistNameWhenProvided(t *testing.T) { + missing := []scanner.MissingRelease{ + {ArtistID: "art-2", Title: "Zebra", RGID: "r3"}, + {ArtistID: "art-1", Title: "Alpha", RGID: "r1"}, + {ArtistID: "art-1", Title: "Beta", RGID: "r2"}, + } + names := map[string]string{"art-1": "Alpha Artist", "art-2": "Zebra Artist"} + got := FormatDigest(missing, "", names) + // Display names are used as labels and sorted alphabetically by name. + if !strings.Contains(got, "Alpha Artist (2):") { + t.Errorf("expected name label with count 2, got:\n%s", got) + } + if !strings.Contains(got, "Zebra Artist (1):") { + t.Errorf("expected name label with count 1, got:\n%s", got) + } + if strings.Index(got, "Alpha Artist") > strings.Index(got, "Zebra Artist") { + t.Errorf("artists not sorted by name: got:\n%s", got) + } +} + +func TestFormatDigest_FallsBackToIDWhenNameMissing(t *testing.T) { + missing := []scanner.MissingRelease{ + {ArtistID: "art-1", Title: "Alpha", RGID: "r1"}, + {ArtistID: "art-2", Title: "Beta", RGID: "r2"}, + } + // Name map present but does not cover art-2 -> falls back to ID. + names := map[string]string{"art-1": "Named Artist"} + got := FormatDigest(missing, "", names) + if !strings.Contains(got, "Named Artist (1):") { + t.Errorf("expected named artist label, got:\n%s", got) + } + if !strings.Contains(got, "art-2 (1):") { + t.Errorf("expected ID fallback for art-2, got:\n%s", got) + } +} + func TestFormatDigest_EmptyUIBaseURLOmitsLink(t *testing.T) { missing := []scanner.MissingRelease{{ArtistID: "a", Title: "x", RGID: "r1"}} - got := FormatDigest(missing, "") + got := FormatDigest(missing, "", nil) if strings.Contains(got, "View details:") { t.Errorf("did not expect UI link when base URL empty: got:\n%s", got) } diff --git a/internal/notifier/scheduler.go b/internal/notifier/scheduler.go index c7dccc6..0bee925 100644 --- a/internal/notifier/scheduler.go +++ b/internal/notifier/scheduler.go @@ -16,8 +16,10 @@ import ( // Releases already present in notifications_sent are excluded upstream by // GetUnnotifiedReleases, so this is idempotent across runs. // -// If there are no unnotified releases the digest reports "no new missing -// releases" and nothing is marked sent (there is nothing to mark). +// If there are no unnotified releases nothing is sent (the caller's scheduler +// is responsible for not spamming the operator with an empty digest). When +// releases are present, each is marked sent so a subsequent run will not +// re-notify it. // // uiBaseURL is the externally-reachable base URL of the Web UI, appended to the // digest so operators can jump to the dashboard. @@ -42,7 +44,28 @@ func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config. }) } - message := FormatDigest(missing, uiBaseURL) + // Resolve human-readable artist names so the digest shows recognizable + // labels instead of opaque internal artist IDs. A lookup failure for a + // single artist must not abort the whole digest, so errors are ignored and + // that artist falls back to its ID via artistLabel. + names := make(map[string]string, len(missing)) + for _, r := range unnotified { + if _, ok := names[r.ArtistID]; ok { + continue + } + settings, err := database.GetArtistSettings(db, r.ArtistID) + if err == nil && settings.Name != "" { + names[r.ArtistID] = settings.Name + } + } + + message := FormatDigest(missing, uiBaseURL, names) + // Nothing to report: skip sending so the operator is not spammed with an + // empty digest on every cron fire. The startup fire likewise stays quiet + // until the first genuinely missing release appears. + if len(unnotified) == 0 { + return 0, nil + } if err := sender.Send(ctx, message); err != nil { return 0, fmt.Errorf("notifier: send digest: %w", err) } diff --git a/internal/notifier/scheduler_test.go b/internal/notifier/scheduler_test.go index 0764b4c..7f7c6c7 100644 --- a/internal/notifier/scheduler_test.go +++ b/internal/notifier/scheduler_test.go @@ -100,6 +100,26 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) { } } +func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) { + db, err := database.New(":memory:") + if err != nil { + t.Fatalf("New(): %v", err) + } + defer db.Close() + + sender := &collectSender{} + n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui") + if err != nil { + t.Fatalf("NotifyOnce: %v", err) + } + if n != 0 { + t.Fatalf("expected 0 releases notified, got %d", n) + } + if sender.count() != 0 { + t.Fatalf("expected no message sent for empty digest, got %d", sender.count()) + } +} + func TestNotifyOnce_SkipsAlreadySent(t *testing.T) { db, err := database.New(":memory:") if err != nil { diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 92bfda2..475e1ef 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -3,6 +3,7 @@ package web import ( "context" "embed" + "errors" "fmt" "html/template" "net/http" @@ -238,6 +239,14 @@ func (s *Server) ignoreOrRestore(w http.ResponseWriter, r *http.Request) { ignored := action == "ignore" if err := database.SetReleaseIgnored(s.db, rgid, ignored); err != nil { + // A 0-rows-affected error means the release was already removed by a + // concurrent re-sync (it disappeared from MusicBrainz). That is benign: + // redirect back rather than surfacing a 500 for a now-nonexistent row. + var notFoundErr error = database.ErrReleaseNotFound + if errors.Is(err, notFoundErr) { + http.Redirect(w, r, "/artist/"+id, http.StatusSeeOther) + return + } http.Error(w, fmt.Sprintf("failed to set ignored: %v", err), http.StatusInternalServerError) return } diff --git a/internal/web/server.go b/internal/web/server.go index 4cc70ac..c7f98c1 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -44,6 +44,13 @@ func NewServer(cfg *config.ServerConfig, db *database.DB, uiBaseURL string, thre uiBaseURL: strings.TrimRight(uiBaseURL, "/"), threshold: threshold, } + // Warn loudly when auth is disabled but the server is reachable from outside + // the host: Basic auth is silently skipped when Username/Password are empty, + // so an operator who forgets credentials on a non-loopback bind would expose + // DB-mutating POST routes (ignore/restore/toggle) to the network. + if (cfg.Username == "" || cfg.Password == "") && cfg.Host != "localhost" && cfg.Host != "127.0.0.1" && cfg.Host != "::1" { + log.Printf("WARNING: Web UI authentication is DISABLED (server.username/password empty) and the server is bound to %q. The dashboard and its state-changing routes are exposed to the network. Set credentials or bind to localhost.", cfg.Host) + } mux := http.NewServeMux() mux.HandleFunc("/", s.handleDashboard) mux.HandleFunc("/artist/{id}", s.handleArtist) @@ -72,8 +79,12 @@ func (s *Server) Addr() string { // due to ctx cancellation returns nil). func (s *Server) Start(ctx context.Context) error { srv := &http.Server{ - Addr: s.Addr(), - Handler: s.Handler(), + Addr: s.Addr(), + Handler: s.Handler(), + ReadTimeout: 15 * time.Second, + ReadHeaderTimeout: 10 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 120 * time.Second, } go func() {