fix: address code review findings

- Preserve MBID and last_synced across periodic artist syncs (INSERT OR
  REPLACE was wiping them, forcing MusicBrainz re-resolution every cycle)
- Exclude unmonitored artists from unnotified-release digest query
- Require telegram.cron_schedule when telegram.enabled is true
- Add same-origin CSRF defense to state-changing web POST routes
- Skip WAL/busy_timeout pragmas for :memory: databases (no-op there)
- Scan mbid as sql.NullString in GetAllArtistSettings to tolerate NULLs
This commit is contained in:
2026-07-19 23:51:15 +03:00
parent 7cdb473d9c
commit ce1c39e14b
7 changed files with 90 additions and 18 deletions

View File

@@ -141,6 +141,9 @@ func validate(cfg *Config) error {
if cfg.Telegram.ChatID == "" { if cfg.Telegram.ChatID == "" {
return fmt.Errorf("telegram.chat_id is required when telegram.enabled is true") return fmt.Errorf("telegram.chat_id is required when telegram.enabled is true")
} }
if cfg.Telegram.CronSchedule == "" {
return fmt.Errorf("telegram.cron_schedule is required when telegram.enabled is true")
}
} }
return nil return nil
} }

View File

@@ -52,11 +52,24 @@ func TouchArtistSynced(db DBer, artistID string, syncedAt time.Time) error {
return nil return nil
} }
// SaveArtistSettings inserts or replaces an artist_settings row. // SaveArtistSettings inserts or updates an artist_settings row. Columns not
// present in the struct's intended set are preserved on conflict rather than
// reset to their zero value: mbid and last_synced are carried over from the
// existing row when the caller does not supply new values. This protects the
// MusicBrainz-resolution cache and the sync TTL markers from being wiped on
// every periodic artist sync.
func SaveArtistSettings(db *DB, settings *ArtistSettings) error { func SaveArtistSettings(db *DB, settings *ArtistSettings) error {
_, err := db.Conn().Exec( _, err := db.Conn().Exec(`
"INSERT OR REPLACE INTO artist_settings (id, name, mbid, ignore_singles, ignore_compilations, monitored) VALUES (?, ?, ?, ?, ?, ?)", INSERT INTO artist_settings (id, name, mbid, ignore_singles, ignore_compilations, monitored, last_synced)
settings.ID, settings.Name, settings.MBID, settings.IgnoreSingles, settings.IgnoreCompilations, settings.Monitored, VALUES (?, ?, ?, ?, ?, ?, (SELECT last_synced FROM artist_settings WHERE id = ?))
ON CONFLICT(id) DO UPDATE SET
name = excluded.name,
mbid = COALESCE(excluded.mbid, artist_settings.mbid),
ignore_singles = excluded.ignore_singles,
ignore_compilations = excluded.ignore_compilations,
monitored = excluded.monitored
`,
settings.ID, settings.Name, nullIfEmpty(settings.MBID), settings.IgnoreSingles, settings.IgnoreCompilations, settings.Monitored, settings.ID,
) )
if err != nil { if err != nil {
return fmt.Errorf("save artist settings: %w", err) return fmt.Errorf("save artist settings: %w", err)
@@ -64,6 +77,15 @@ func SaveArtistSettings(db *DB, settings *ArtistSettings) error {
return nil return nil
} }
// nullIfEmpty returns nil for an empty string so COALESCE-preserving columns
// (e.g. mbid) keep their existing value when the caller supplies no new one.
func nullIfEmpty(s string) interface{} {
if s == "" {
return nil
}
return s
}
// GetAllArtistSettings returns all rows from artist_settings. // GetAllArtistSettings returns all rows from artist_settings.
func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) { func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) {
rows, err := db.Conn().Query( rows, err := db.Conn().Query(
@@ -77,9 +99,11 @@ func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) {
var results []ArtistSettings var results []ArtistSettings
for rows.Next() { for rows.Next() {
var s ArtistSettings var s ArtistSettings
if err := rows.Scan(&s.ID, &s.Name, &s.MBID, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored); err != nil { var mbid sql.NullString
if err := rows.Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored); err != nil {
return nil, fmt.Errorf("scan artist settings: %w", err) return nil, fmt.Errorf("scan artist settings: %w", err)
} }
s.MBID = mbid.String
results = append(results, s) results = append(results, s)
} }
if err := rows.Err(); err != nil { if err := rows.Err(); err != nil {

View File

@@ -51,9 +51,10 @@ func New(dbPath string) (*DB, error) {
} }
if dbPath == ":memory:" { if dbPath == ":memory:" {
conn.SetMaxOpenConns(1) conn.SetMaxOpenConns(1)
} } else {
// Enable WAL mode for better concurrent read performance. WAL is a
// Enable WAL mode for better concurrent read performance. // no-op on in-memory databases (they always use the MEMORY journal), so
// skip it there to avoid misleading configuration.
if _, err := conn.Exec("PRAGMA journal_mode=WAL"); err != nil { if _, err := conn.Exec("PRAGMA journal_mode=WAL"); err != nil {
conn.Close() conn.Close()
return nil, fmt.Errorf("set WAL mode: %w", err) return nil, fmt.Errorf("set WAL mode: %w", err)
@@ -64,6 +65,7 @@ func New(dbPath string) (*DB, error) {
conn.Close() conn.Close()
return nil, fmt.Errorf("set busy timeout: %w", err) return nil, fmt.Errorf("set busy timeout: %w", err)
} }
}
db := &DB{conn: conn} db := &DB{conn: conn}
if err := db.migrate(); err != nil { if err := db.migrate(); err != nil {

View File

@@ -28,13 +28,16 @@ func IsNotificationSent(db *DB, rgid string) (bool, error) {
return count > 0, nil return count > 0, nil
} }
// GetUnnotifiedReleases returns all external_release rows that have no entry in notifications_sent. // GetUnnotifiedReleases returns all external_release rows for monitored artists
// that have no entry in notifications_sent. Releases belonging to unmonitored
// artists are excluded so the digest honors the monitoring contract.
func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) { func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) {
rows, err := db.Conn().Query(` rows, err := db.Conn().Query(`
SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored
FROM external_releases e FROM external_releases e
JOIN artist_settings s ON e.artist_id = s.id
LEFT JOIN notifications_sent n ON e.rgid = n.rgid LEFT JOIN notifications_sent n ON e.rgid = n.rgid
WHERE n.rgid IS NULL AND e.is_ignored = 0 WHERE s.monitored = 1 AND n.rgid IS NULL AND e.is_ignored = 0
`) `)
if err != nil { if err != nil {
return nil, fmt.Errorf("query unnotified releases: %w", err) return nil, fmt.Errorf("query unnotified releases: %w", err)

View File

@@ -107,9 +107,11 @@ func SyncArtists(ctx context.Context, client *NavidromeClient, db *database.DB)
} }
existing, err := database.GetArtistSettings(db, artist.ID) existing, err := database.GetArtistSettings(db, artist.ID)
if err == nil { if err == nil {
settings.MBID = existing.MBID
settings.Monitored = existing.Monitored settings.Monitored = existing.Monitored
settings.IgnoreSingles = existing.IgnoreSingles settings.IgnoreSingles = existing.IgnoreSingles
settings.IgnoreCompilations = existing.IgnoreCompilations settings.IgnoreCompilations = existing.IgnoreCompilations
settings.LastSynced = existing.LastSynced
} else if !errors.Is(err, database.ErrArtistNotFound) { } else if !errors.Is(err, database.ErrArtistNotFound) {
return fmt.Errorf("sync artists: get settings for artist %s: %w", artist.ID, err) return fmt.Errorf("sync artists: get settings for artist %s: %w", artist.ID, err)
} }

View File

@@ -214,6 +214,10 @@ func (s *Server) ignoreOrRestore(w http.ResponseWriter, r *http.Request) {
http.Error(w, "method not allowed", http.StatusMethodNotAllowed) http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
return return
} }
if !s.sameOrigin(r) {
http.Error(w, "forbidden: cross-origin request", http.StatusForbidden)
return
}
id := r.PathValue("id") id := r.PathValue("id")
if id == "" { if id == "" {
http.NotFound(w, r) http.NotFound(w, r)
@@ -261,6 +265,10 @@ func (s *Server) toggleIgnoreSingles(w http.ResponseWriter, r *http.Request) {
http.Error(w, "method not allowed", http.StatusMethodNotAllowed) http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
return return
} }
if !s.sameOrigin(r) {
http.Error(w, "forbidden: cross-origin request", http.StatusForbidden)
return
}
id := r.PathValue("id") id := r.PathValue("id")
if id == "" { if id == "" {
http.NotFound(w, r) http.NotFound(w, r)

View File

@@ -10,6 +10,7 @@ import (
"fmt" "fmt"
"log" "log"
"net/http" "net/http"
"net/url"
"strings" "strings"
"time" "time"
@@ -138,6 +139,35 @@ func unauthorized(w http.ResponseWriter) {
_, _ = w.Write([]byte("401 Unauthorized\n")) _, _ = w.Write([]byte("401 Unauthorized\n"))
} }
// sameOrigin returns true when the request's Origin (or, lacking that, Referer)
// header matches the server's own origin. State-changing POST routes use this
// as a lightweight CSRF defense: a cross-site request from a logged-in
// operator's browser will carry a different Origin/Referer and be rejected.
// When the header is absent (e.g. a same-origin form POST from older browsers
// or curl), the request is allowed rather than blocked, since the dashboard
// only ever issues same-origin form posts.
func (s *Server) sameOrigin(r *http.Request) bool {
origin := r.Header.Get("Origin")
if origin == "" {
origin = r.Header.Get("Referer")
}
if origin == "" {
return true
}
got, err := url.Parse(origin)
if err != nil || got.Host == "" {
return false
}
// Accept the actual Host the client reached (covers 0.0.0.0 bind with
// localhost/127.0.0.1 access) as well as the configured bind address.
gotHost := got.Host
reqHost := r.Host
if reqHost == "" {
reqHost = s.Addr()
}
return gotHost == reqHost || gotHost == s.Addr()
}
// ArtistSummary is the dashboard projection of a single monitored artist and // ArtistSummary is the dashboard projection of a single monitored artist and
// its missing-release count. // its missing-release count.
type ArtistSummary struct { type ArtistSummary struct {