fix: address code review findings
- Honor ignore_singles/ignore_compilations at scanner read time so toggles take effect immediately on the dashboard, artist page, and digest instead of waiting for the MusicBrainz cache to expire and prune rows. - Run notifier notify synchronously in the scheduler loop to avoid overlapping read-send-mark runs double-sending the digest. - Show artist name (with ID fallback) on the archive page instead of raw IDs. - Select last_synced in GetAllArtistSettings for contract consistency. - Fix stale startPeriodicSync comment and remove redundant error var. - Remove dead ignored-branch from the artist template (never rendered). - Add tests: CSRF sameOrigin, ArtistCacheFresh, secondary_types round-trip, and scanner type-toggle filtering. - Update Specification.md schema/config to reflect mbid, last_synced, secondary_types, sync.interval, and server.public_url.
This commit is contained in:
@@ -68,6 +68,7 @@ type LocalAlbumView struct {
|
||||
// artist detail page, including the ignore toggle form target.
|
||||
type MissingReleaseView struct {
|
||||
ArtistID string
|
||||
ArtistName string
|
||||
RGID string
|
||||
Title string
|
||||
Type string
|
||||
@@ -189,14 +190,18 @@ func (s *Server) handleArchive(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
data := &ArchiveData{UIBaseURL: s.uiBaseURL}
|
||||
for _, rel := range ignored {
|
||||
data.Releases = append(data.Releases, MissingReleaseView{
|
||||
view := MissingReleaseView{
|
||||
ArtistID: rel.ArtistID,
|
||||
RGID: rel.RGID,
|
||||
Title: rel.Title,
|
||||
Type: rel.Type,
|
||||
ReleaseDate: rel.ReleaseDate,
|
||||
Ignored: true,
|
||||
})
|
||||
}
|
||||
if settings, err := database.GetArtistSettings(s.db, rel.ArtistID); err == nil {
|
||||
view.ArtistName = settings.Name
|
||||
}
|
||||
data.Releases = append(data.Releases, view)
|
||||
}
|
||||
|
||||
w.Header().Set("Content-Type", "text/html; charset=utf-8")
|
||||
@@ -271,8 +276,7 @@ func (s *Server) ignoreOrRestore(w http.ResponseWriter, r *http.Request) {
|
||||
// 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) {
|
||||
if errors.Is(err, database.ErrReleaseNotFound) {
|
||||
http.Redirect(w, r, "/artist/"+id, http.StatusSeeOther)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -469,3 +469,81 @@ func dashboardMissingCount(t *testing.T, s *Server, artistName string) int {
|
||||
}
|
||||
return 0
|
||||
}
|
||||
|
||||
// postStateChanging issues a state-changing POST to the given route with the
|
||||
// provided Origin/Referer header and basic auth, returning the response code.
|
||||
func postStateChanging(t *testing.T, s *Server, path, originHeader string) int {
|
||||
t.Helper()
|
||||
form := strings.NewReader("rgid=r1")
|
||||
req := httptest.NewRequest(http.MethodPost, path, form)
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
if originHeader != "" {
|
||||
req.Header.Set("Origin", originHeader)
|
||||
}
|
||||
req.SetBasicAuth("admin", "secret")
|
||||
rec := httptest.NewRecorder()
|
||||
s.Handler().ServeHTTP(rec, req)
|
||||
return rec.Code
|
||||
}
|
||||
|
||||
func TestStateChangingEnforcesSameOrigin(t *testing.T) {
|
||||
served := "http://0.0.0.0:8080" // matches the server's Addr()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
route string
|
||||
origin string
|
||||
wantCode int
|
||||
}{
|
||||
{"same-origin Origin allowed", "/artist/a1/ignore", served, http.StatusSeeOther},
|
||||
{"no Origin header allowed (same-origin form post)", "/artist/a1/ignore", "", http.StatusSeeOther},
|
||||
{"cross-origin Origin rejected", "/artist/a1/ignore", "http://evil.example", http.StatusForbidden},
|
||||
{"cross-origin Referer rejected", "/artist/a1/ignore", "", http.StatusForbidden},
|
||||
{"cross-origin on toggle rejected", "/artist/a1/ignore-singles", "http://evil.example", http.StatusForbidden},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
s, db := newServer(t, "admin", "secret")
|
||||
seedArtist(t, db, "a1", "Radiohead", "", true)
|
||||
seedExternalRelease(t, db, "r1", "a1", "Kid A")
|
||||
|
||||
// For the cross-origin Referer case, use Referer instead of Origin.
|
||||
var code int
|
||||
if tt.name == "cross-origin Referer rejected" {
|
||||
form := strings.NewReader("rgid=r1")
|
||||
req := httptest.NewRequest(http.MethodPost, tt.route, form)
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
req.Header.Set("Referer", "http://evil.example/artist/a1")
|
||||
req.SetBasicAuth("admin", "secret")
|
||||
rec := httptest.NewRecorder()
|
||||
s.Handler().ServeHTTP(rec, req)
|
||||
code = rec.Code
|
||||
} else {
|
||||
code = postStateChanging(t, s, tt.route, tt.origin)
|
||||
}
|
||||
|
||||
if code != tt.wantCode {
|
||||
t.Fatalf("route %s origin %q: got %d, want %d", tt.route, tt.origin, code, tt.wantCode)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestStateChanging_MalformedOriginRejected(t *testing.T) {
|
||||
s, db := newServer(t, "admin", "secret")
|
||||
seedArtist(t, db, "a1", "Radiohead", "", true)
|
||||
seedExternalRelease(t, db, "r1", "a1", "Kid A")
|
||||
|
||||
form := strings.NewReader("rgid=r1")
|
||||
req := httptest.NewRequest(http.MethodPost, "/artist/a1/ignore", form)
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
// An Origin that does not parse as a valid URL with a host.
|
||||
req.Header.Set("Origin", "http://")
|
||||
req.SetBasicAuth("admin", "secret")
|
||||
rec := httptest.NewRecorder()
|
||||
s.Handler().ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("expected 403 for malformed origin, got %d", rec.Code)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -27,7 +27,7 @@
|
||||
<tbody>
|
||||
{{ range .Releases }}
|
||||
<tr>
|
||||
<td>{{ .ArtistID }}</td>
|
||||
<td>{{ if .ArtistName }}{{ .ArtistName }}{{ else }}{{ .ArtistID }}{{ end }}</td>
|
||||
<td>{{ .Title }}</td>
|
||||
<td>{{ if .Type }}{{ .Type }}{{ else }}<span class="empty">—</span>{{ end }}</td>
|
||||
<td>{{ if .ReleaseDate }}{{ .ReleaseDate }}{{ else }}<span class="empty">—</span>{{ end }}</td>
|
||||
|
||||
@@ -57,17 +57,10 @@
|
||||
<td>{{ if .Type }}{{ .Type }}{{ else }}<span class="empty">—</span>{{ end }}</td>
|
||||
<td>{{ if .ReleaseDate }}{{ .ReleaseDate }}{{ else }}<span class="empty">—</span>{{ end }}</td>
|
||||
<td>
|
||||
{{ if .Ignored }}
|
||||
<form method="POST" action="/artist/{{ $.ID }}/restore">
|
||||
<input type="hidden" name="rgid" value="{{ .RGID }}">
|
||||
<button type="submit">Restore</button>
|
||||
</form>
|
||||
{{ else }}
|
||||
<form method="POST" action="/artist/{{ $.ID }}/ignore">
|
||||
<input type="hidden" name="rgid" value="{{ .RGID }}">
|
||||
<button type="submit">Ignore</button>
|
||||
</form>
|
||||
{{ end }}
|
||||
</td>
|
||||
</tr>
|
||||
{{ end }}
|
||||
|
||||
Reference in New Issue
Block a user