From 0c5c10d00b0fbd1715458f083dfece5ae5c00e05 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 1 May 2026 22:05:15 -0400 Subject: [PATCH] fix(server): drop duplicate /api/admin route mount that shadowed api.Mount endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: r.Route("/api/admin", ...) in server.go for the /scan endpoint created a second chi subtree at the same prefix as the nested admin Route inside api.Mount. chi.Walk shows both subtrees registered, but runtime routing dispatch only matched one — so every /api/admin/* endpoint registered by api.Mount (Lidarr config, quarantine admin, requests admin) silently returned 404 to authenticated callers. Has shipped this way since 06a1fe1; M5a/b/c admin tests didn't catch it because they call api.Mount on a fresh chi.NewRouter() rather than going through Server.Router(), so only one /api/admin registration existed in those tests. Fix: register /api/admin/scan as a single inline-middleware route (r.With(...).Post(...)) instead of a Route subtree, eliminating the duplicate /api/admin mount. Adds a regression test that uses a real seeded admin session to make an authenticated GET /api/admin/lidarr/config and asserts != 404. Verified the test fails without the fix and passes with it. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/server/server.go | 15 ++--- internal/server/server_test.go | 112 +++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 7 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index a55eba19..c333aead 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -74,13 +74,14 @@ func (s *Server) Router() http.Handler { lidarrReqs := lidarrrequests.NewService(s.Pool, lidarrCfg, lidarrClientFn, nil) lidarrQuar := lidarrquarantine.NewService(s.Pool, lidarrCfg, lidarrClientFn) api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, lidarrCfg, lidarrReqs, lidarrQuar) - r.Route("/api/admin", func(admin chi.Router) { - admin.Use(auth.RequireUser(s.Pool)) - admin.Use(auth.RequireAdmin()) - if s.Scanner != nil { - admin.Post("/scan", s.handleAdminScan) - } - }) + // /api/admin/scan is the only admin route owned by the server package + // (it needs the Scanner). Register it as a single inline-middleware + // route — using r.Route("/api/admin", ...) here would create a second + // subtree that shadows every admin route registered by api.Mount. + if s.Scanner != nil { + r.With(auth.RequireUser(s.Pool), auth.RequireAdmin()). + Post("/api/admin/scan", s.handleAdminScan) + } subsonic.Mount(r, s.Pool, s.Logger, s.SubsonicCfg, writer) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9629ec33..3abcc3d3 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,16 +1,25 @@ package server import ( + "context" "encoding/json" "io" "log/slog" "net/http" "net/http/httptest" + "os" "strings" "testing" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/config" + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/library" "git.fabledsword.com/bvandeusen/minstrel/internal/subsonic" + "time" ) func TestHealthz(t *testing.T) { @@ -111,3 +120,106 @@ func TestRouter_RestPathNotSwallowedBySPA(t *testing.T) { t.Errorf("Content-Type = %q; /rest/* must never return text/html", ct) } } + +// TestRouter_AdminSubtreeNotShadowed is a regression test for a real bug we +// shipped in 06a1fe1 and didn't catch until M6a: r.Route("/api/admin", ...) +// in server.go was creating a second chi subtree at the same prefix as the +// nested admin Route inside api.Mount. chi.Walk shows both subtrees as +// registered, but runtime routing dispatch only matches one — every admin +// endpoint registered by api.Mount (Lidarr config, quarantine, etc.) +// silently 404'd for authenticated callers in production. +// +// chi.Walk enumeration would NOT have caught this (both branches are in +// the tree). The only reliable check is to make a real authenticated +// request and confirm it reaches a handler — i.e. doesn't fall through +// to the JSON NotFound (404) that signals shadowing. +// +// The existing tests in this file pass nil for pool, which skips api.Mount +// entirely (gated behind `if s.Pool != nil`), so the conflict didn't manifest. +// +// Gated on MINSTREL_TEST_DATABASE_URL like other integration-leaning tests. +func TestRouter_AdminSubtreeNotShadowed(t *testing.T) { + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + + // Seed a test admin user + session so the request actually reaches the + // inner route lookup (auth-middleware short-circuits don't catch shadowing). + ctx := context.Background() + q := dbq.New(pool) + _, _ = pool.Exec(ctx, "DELETE FROM sessions WHERE user_agent = 'shadowing-test'") + _, _ = pool.Exec(ctx, "DELETE FROM users WHERE username = 'test-shadowing-admin'") + user, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: "test-shadowing-admin", + PasswordHash: "x", + ApiToken: "test-shadowing-token", + IsAdmin: true, + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + t.Cleanup(func() { _, _ = pool.Exec(ctx, "DELETE FROM users WHERE id = $1", user.ID) }) + token := "shadow-test-" + time.Now().Format("20060102150405") + tokenHash := auth.HashSessionToken(token) + _, err = pool.Exec(ctx, + "INSERT INTO sessions (user_id, token_hash, user_agent) VALUES ($1, $2, 'shadowing-test')", + user.ID, tokenHash[:], + ) + if err != nil { + t.Fatalf("insert session: %v", err) + } + + s := New(slog.New(slog.NewTextHandler(io.Discard, nil)), pool, + stubScanner{}, subsonic.Config{}, config.EventsConfig{}, config.RecommendationConfig{}) + ts := httptest.NewServer(s.Router()) + defer ts.Close() + + // Each path is registered by a different package: lidarr/config from + // api.Mount and admin/scan from server.Router. If chi runtime dispatch + // only routes one /api/admin subtree, one of these returns 404 — that + // IS the bug we shipped in 06a1fe1. + cases := []struct { + method string + path string + owner string + }{ + {http.MethodGet, "/api/admin/lidarr/config", "api.Mount"}, + // /api/admin/scan would actually trigger a real library scan via + // stubScanner; we don't include it. The lidarr/config path is the + // canonical regression check. + } + for _, tc := range cases { + req, err := http.NewRequest(tc.method, ts.URL+tc.path, nil) + if err != nil { + t.Fatalf("build %s %s: %v", tc.method, tc.path, err) + } + req.Header.Set("Authorization", "Bearer "+token) + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", tc.method, tc.path, err) + } + _ = resp.Body.Close() + if resp.StatusCode == http.StatusNotFound { + t.Errorf("%s %s (owner=%s) returned 404 with valid admin auth — route is shadowed", + tc.method, tc.path, tc.owner) + } + } +} + +// stubScanner is a no-op ScanTrigger used only to make Server.Router() +// register /api/admin/scan. Its Scan method must never be called by the +// route-presence assertions in this file. +type stubScanner struct{} + +func (stubScanner) Scan(ctx context.Context) (library.Stats, error) { + panic("stubScanner.Scan called from server_test") +}