fix(server): drop duplicate /api/admin route mount that shadowed api.Mount endpoints

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) <noreply@anthropic.com>

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)
 	}
 

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")
+}