summaryrefslogtreecommitdiff
path: root/player-server
diff options
context:
space:
mode:
authorPaul Buetow <paul@buetow.org>2026-05-19 19:22:26 +0300
committerPaul Buetow <paul@buetow.org>2026-05-19 19:22:26 +0300
commit4215db6718d4ea662084b647398536aa51934ac6 (patch)
treeff0b659f172682e4f87ce558d4a66bd3e3e38424 /player-server
parent35aa611038c97a2ce27510e3898318afca37cac0 (diff)
Replace hardcoded public-path whitelist with route registry
Previously internal/api/middleware.go contained an isBootstrapPublic function with a hardcoded switch over public paths plus three /css/ /js/ /images/ prefix checks. Whenever a new public route was added in server.go a developer also had to remember to extend the switch — easy to miss, and the symptom is a silent 307 to /bootstrap.html. Public-route metadata now lives on the Middleware itself: * Middleware gets publicPaths map[string]bool and publicPrefixes []string * RegisterPublic(path) / RegisterPublicPrefix(prefix) populate the registry * BootstrapRedirect consults isPublic(path) instead of a hardcoded list Server.routes() registers each public route through new helpers (handlePublic / handlePublicFunc / handlePublicPrefix) so the mux pattern and the bypass set are declared together — there is no separate whitelist to keep in sync. Routes migrated: 14 exact (bootstrap/login {/api,/api/v1/auth} variants, /healthz, /readyz, /login.html, /bootstrap.html, /favicon.{ico,svg}, /logo.{png,svg}, /manifest.json, /sw.js) and 4 prefixes (/css/, /js/, /images/, /s/ for tokenised share URLs). TestMiddleware_BootstrapRedirect now seeds the registry explicitly (middleware-level unit test, no full server). A new TestServer_PublicRouteRegistry asserts the full set of public paths is registered after NewServer(). Refs: agent task ha. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Diffstat (limited to 'player-server')
-rw-r--r--player-server/internal/api/handlers_test.go12
-rw-r--r--player-server/internal/api/middleware.go74
-rw-r--r--player-server/internal/api/server.go92
-rw-r--r--player-server/internal/api/server_test.go90
4 files changed, 230 insertions, 38 deletions
diff --git a/player-server/internal/api/handlers_test.go b/player-server/internal/api/handlers_test.go
index 2c662b3..6279895 100644
--- a/player-server/internal/api/handlers_test.go
+++ b/player-server/internal/api/handlers_test.go
@@ -146,6 +146,15 @@ func TestMiddleware_BootstrapRedirect(t *testing.T) {
{"count error", "/", 0, errors.New("boom"), http.StatusInternalServerError, ""},
}
+ // Public paths the production server registers via routesPublic /
+ // routesHTML — re-declared here so the middleware-level unit test can
+ // exercise BootstrapRedirect without standing up a full Server.
+ publicPaths := []string{
+ "/bootstrap.html", "/api/bootstrap", "/api/v1/auth/bootstrap",
+ "/login.html", "/api/login", "/api/v1/auth/login",
+ "/healthz", "/readyz",
+ }
+
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
authSvc := &service.MockAuthService{
@@ -154,6 +163,9 @@ func TestMiddleware_BootstrapRedirect(t *testing.T) {
},
}
mw := NewMiddleware(authSvc, nil)
+ for _, p := range publicPaths {
+ mw.RegisterPublic(p)
+ }
handler := mw.BootstrapRedirect(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
diff --git a/player-server/internal/api/middleware.go b/player-server/internal/api/middleware.go
index 80128fd..ec7c1a7 100644
--- a/player-server/internal/api/middleware.go
+++ b/player-server/internal/api/middleware.go
@@ -21,14 +21,59 @@ const (
)
// Middleware holds dependencies for middleware constructors.
+//
+// publicPaths and publicPrefixes form a route registry used by
+// BootstrapRedirect to decide which requests bypass the redirect when no
+// users exist. They are populated at route registration time (see
+// Server.routes()) so that adding a new public route automatically updates
+// the bypass set — no hidden hardcoded whitelist that silently 401s/redirects
+// new routes the developer forgot to add.
type Middleware struct {
- authSvc service.AuthService
- sm auth.SessionManager
+ authSvc service.AuthService
+ sm auth.SessionManager
+ publicPaths map[string]bool
+ publicPrefixes []string
}
// NewMiddleware creates middleware handlers.
+// The public route registry starts empty; callers register public paths
+// via RegisterPublic / RegisterPublicPrefix as routes are wired up.
func NewMiddleware(authSvc service.AuthService, sm auth.SessionManager) *Middleware {
- return &Middleware{authSvc: authSvc, sm: sm}
+ return &Middleware{
+ authSvc: authSvc,
+ sm: sm,
+ publicPaths: make(map[string]bool),
+ }
+}
+
+// RegisterPublic marks an exact path as public (bypasses BootstrapRedirect).
+// Call this at route registration time so the middleware's view of "public"
+// stays in sync with the actual route table.
+func (mw *Middleware) RegisterPublic(path string) {
+ if mw.publicPaths == nil {
+ mw.publicPaths = make(map[string]bool)
+ }
+ mw.publicPaths[path] = true
+}
+
+// RegisterPublicPrefix marks a path prefix as public. Used for routes whose
+// concrete paths contain wildcards (e.g. /s/{token}/...) or that serve a
+// directory tree (e.g. /css/, /js/, /images/).
+func (mw *Middleware) RegisterPublicPrefix(prefix string) {
+ mw.publicPrefixes = append(mw.publicPrefixes, prefix)
+}
+
+// isPublic reports whether the given request path is registered as public.
+func (mw *Middleware) isPublic(path string) bool {
+ if mw.publicPaths[path] {
+ return true
+ }
+ for _, p := range mw.publicPrefixes {
+ if strings.HasPrefix(path, p) {
+ return true
+ }
+ }
+ return false
}
// RequireSession validates the session cookie and injects the session into request context.
@@ -102,9 +147,16 @@ func (mw *Middleware) RequireAdmin(next http.Handler) http.Handler {
}
// BootstrapRedirect redirects all requests to /bootstrap.html when no users exist.
+//
+// Public routes (the bootstrap page itself, login endpoints, health probes,
+// static assets, public share routes, etc.) bypass the redirect so the user
+// can complete the bootstrap flow. The set of public paths is consulted via
+// the route registry on Middleware (populated at route declaration time)
+// rather than a hardcoded list, so adding a new public route in server.go is
+// all that's required — there is no separate whitelist to keep in sync.
func (mw *Middleware) BootstrapRedirect(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- if isBootstrapPublic(r.URL.Path) {
+ if mw.isPublic(r.URL.Path) {
next.ServeHTTP(w, r)
return
}
@@ -124,17 +176,3 @@ func (mw *Middleware) BootstrapRedirect(next http.Handler) http.Handler {
next.ServeHTTP(w, r)
})
}
-
-func isBootstrapPublic(path string) bool {
- switch path {
- case "/bootstrap.html", "/api/bootstrap", "/api/v1/auth/bootstrap",
- "/login.html", "/api/login", "/api/v1/auth/login",
- "/healthz", "/readyz",
- "/favicon.svg", "/favicon.ico", "/logo.svg", "/logo.png", "/manifest.json", "/sw.js":
- return true
- }
- if strings.HasPrefix(path, "/css/") || strings.HasPrefix(path, "/js/") || strings.HasPrefix(path, "/images/") {
- return true
- }
- return false
-}
diff --git a/player-server/internal/api/server.go b/player-server/internal/api/server.go
index 2fccf32..e83cecb 100644
--- a/player-server/internal/api/server.go
+++ b/player-server/internal/api/server.go
@@ -77,14 +77,24 @@ func NewServer(deps ServerDeps) (*Server, error) {
}
// NewServerWithLogger creates a Server with routes and an injected logger.
-// It returns an error if deps.Config is nil; previously this case panicked,
-// but returning an error lets the caller (e.g. cmd/player/main.go) report
-// the failure cleanly and exit with a useful message rather than crashing
-// deep in the wiring code.
+// It returns an error if required dependencies (Config or MediaStreamer) are
+// nil; previously these cases either panicked or were silently filled in at
+// request time with a default streamer, which hid wiring mistakes and
+// violated the Dependency Inversion Principle. Returning an error lets the
+// caller (e.g. cmd/player/main.go) report the failure cleanly and exit with
+// a useful message rather than crashing or quietly degrading.
func NewServerWithLogger(deps ServerDeps, logger *slog.Logger) (*Server, error) {
if deps.Config == nil {
return nil, errors.New("api.NewServerWithLogger: Config is nil")
}
+ // MediaStreamer is required: every file/stream/download/share handler
+ // dispatches through Server.serveFileResult, which uses s.streamer
+ // directly. Callers must inject one (production wiring builds a
+ // service.NewMediaStreamer(remuxer)). This mirrors the explicit-deps
+ // pattern set in commits 622827c (http.Client) and 92edb83 (TokenManager).
+ if deps.MediaStreamer == nil {
+ return nil, errors.New("api.NewServerWithLogger: MediaStreamer is nil")
+ }
if deps.StaticFS == nil {
deps.StaticFS = http.Dir("web")
}
@@ -151,6 +161,30 @@ func (s *Server) handleBoth(method, path string, h http.Handler) {
s.mux.Handle(method+" "+apiV1Path(path), h)
}
+// handlePublic registers a path with the mux and marks it public in the
+// middleware route registry so BootstrapRedirect lets it through without
+// requiring an existing user account. Use for endpoints reachable before
+// bootstrap completes (login, bootstrap, health probes, public HTML pages,
+// individual static files).
+func (s *Server) handlePublic(path string, h http.Handler) {
+ s.mux.Handle(path, h)
+ s.mw.RegisterPublic(path)
+}
+
+// handlePublicFunc is the http.HandlerFunc variant of handlePublic.
+func (s *Server) handlePublicFunc(path string, h http.HandlerFunc) {
+ s.mux.HandleFunc(path, h)
+ s.mw.RegisterPublic(path)
+}
+
+// handlePublicPrefix registers a "directory" handler (e.g. /css/) and
+// records the prefix in the middleware registry so anything served under it
+// is treated as public.
+func (s *Server) handlePublicPrefix(prefix string, h http.Handler) {
+ s.mux.Handle(prefix, h)
+ s.mw.RegisterPublicPrefix(prefix)
+}
+
func apiV1Path(path string) string {
const apiPrefix = "/api/"
if !strings.HasPrefix(path, apiPrefix) {
@@ -160,40 +194,58 @@ func apiV1Path(path string) string {
}
// routesPublic wires the fully-public API endpoints (bootstrap, login, probes).
+// Each route is registered through handlePublic* helpers so the middleware
+// route registry stays in sync — no separate whitelist to maintain.
+//
+// Note: the v1 aliases live under /api/v1/auth/... rather than the
+// straight /api/v1/<rest> shape that apiV1Path() generates, so they are
+// registered explicitly here instead of via a "both" helper.
func (s *Server) routesPublic() {
- s.mux.HandleFunc("/api/bootstrap", publicMethod(http.MethodPost, s.handleBootstrap))
- s.mux.HandleFunc("/api/v1/auth/bootstrap", publicMethod(http.MethodPost, s.handleBootstrap))
- s.mux.HandleFunc("/api/login", publicMethod(http.MethodPost, s.handleLogin))
- s.mux.HandleFunc("/api/v1/auth/login", publicMethod(http.MethodPost, s.handleLogin))
- s.mux.HandleFunc("/healthz", publicMethod(http.MethodGet, s.handleHealthz))
- s.mux.HandleFunc("/readyz", publicMethod(http.MethodGet, s.handleReadyz))
+ s.handlePublicFunc("/api/bootstrap", publicMethod(http.MethodPost, s.handleBootstrap))
+ s.handlePublicFunc("/api/v1/auth/bootstrap", publicMethod(http.MethodPost, s.handleBootstrap))
+ s.handlePublicFunc("/api/login", publicMethod(http.MethodPost, s.handleLogin))
+ s.handlePublicFunc("/api/v1/auth/login", publicMethod(http.MethodPost, s.handleLogin))
+ s.handlePublicFunc("/healthz", publicMethod(http.MethodGet, s.handleHealthz))
+ s.handlePublicFunc("/readyz", publicMethod(http.MethodGet, s.handleReadyz))
}
// routesSharePublic wires public share routes (no session required).
+// Share routes are dynamic (/s/{token}/...), so we register both the
+// specific mux patterns and a /s/ prefix in the public route registry to
+// cover every concrete token-bearing URL.
func (s *Server) routesSharePublic() {
s.mux.HandleFunc("GET /s/{token}", s.handleSharePage)
s.mux.HandleFunc("GET /s/{token}/stream", s.handleShareStream)
s.mux.HandleFunc("GET /s/{token}/thumbnail", s.handleShareThumbnail)
s.mux.HandleFunc("GET /s/{token}/download", s.handleShareDownload)
+ s.mw.RegisterPublicPrefix("/s/")
}
// routesStatic wires static CSS/JS asset serving.
+// Both the directory prefixes (/css/, /js/, /images/) and the individual
+// top-level asset files are registered public so the bootstrap page can
+// load its resources before any user exists.
func (s *Server) routesStatic() {
staticHandler := http.FileServer(s.staticFS)
- s.mux.Handle("/css/", staticHandler)
- s.mux.Handle("/js/", staticHandler)
- s.mux.Handle("/logo.png", staticHandler)
- s.mux.Handle("/logo.svg", staticHandler)
- s.mux.Handle("/favicon.ico", staticHandler)
- s.mux.Handle("/favicon.svg", staticHandler)
- s.mux.Handle("/manifest.json", staticHandler)
- s.mux.Handle("/sw.js", staticHandler)
+ s.handlePublicPrefix("/css/", staticHandler)
+ s.handlePublicPrefix("/js/", staticHandler)
+ // /images/ isn't a mux-registered tree but is referenced by some HTML
+ // pages; mark its prefix public so future asset additions just work.
+ s.mw.RegisterPublicPrefix("/images/")
+ s.handlePublic("/logo.png", staticHandler)
+ s.handlePublic("/logo.svg", staticHandler)
+ s.handlePublic("/favicon.ico", staticHandler)
+ s.handlePublic("/favicon.svg", staticHandler)
+ s.handlePublic("/manifest.json", staticHandler)
+ s.handlePublic("/sw.js", staticHandler)
}
// routesHTML wires the SPA HTML page routes.
+// /login.html and /bootstrap.html are public (the user reaches them before
+// authenticating); the rest sit behind RequireSession.
func (s *Server) routesHTML() {
- s.mux.Handle("/login.html", http.HandlerFunc(s.serveLogin))
- s.mux.Handle("/bootstrap.html", http.HandlerFunc(s.serveBootstrap))
+ s.handlePublic("/login.html", http.HandlerFunc(s.serveLogin))
+ s.handlePublic("/bootstrap.html", http.HandlerFunc(s.serveBootstrap))
s.mux.Handle("/", s.mw.RequireSession(http.HandlerFunc(s.serveIndex)))
s.mux.Handle("GET /index.html", s.mw.RequireSession(http.HandlerFunc(s.serveIndex)))
s.mux.Handle("GET /detach.html", s.mw.RequireSession(http.HandlerFunc(s.serveDetach)))
diff --git a/player-server/internal/api/server_test.go b/player-server/internal/api/server_test.go
index 91f793d..89bc6c6 100644
--- a/player-server/internal/api/server_test.go
+++ b/player-server/internal/api/server_test.go
@@ -3,6 +3,9 @@ package api
import (
"log/slog"
"testing"
+
+ "codeberg.org/snonux/player/internal"
+ "codeberg.org/snonux/player/internal/service"
)
// TestNewServerWithLogger_ErrorsOnNilConfig verifies that the constructor
@@ -20,3 +23,90 @@ func TestNewServerWithLogger_ErrorsOnNilConfig(t *testing.T) {
t.Fatalf("expected nil Server on error, got %v", srv)
}
}
+
+// TestNewServerWithLogger_ErrorsOnNilMediaStreamer verifies that the
+// constructor refuses to build a Server when deps.MediaStreamer is nil.
+// serveFileResult used to fall back to a default streamer at request time,
+// which silently hid wiring mistakes and violated DIP. Construction now
+// fails fast, mirroring the explicit-deps pattern in podcast/auth services.
+func TestNewServerWithLogger_ErrorsOnNilMediaStreamer(t *testing.T) {
+ srv, err := NewServerWithLogger(ServerDeps{
+ Config: &internal.Config{},
+ MediaStreamer: nil,
+ }, slog.Default())
+ if err == nil {
+ t.Fatal("expected error for nil MediaStreamer, got nil")
+ }
+ if srv != nil {
+ t.Fatalf("expected nil Server on error, got %v", srv)
+ }
+}
+
+// TestNewServerWithLogger_SucceedsWithMediaStreamer is a happy-path sanity
+// check ensuring the new MediaStreamer validation does not reject valid
+// dependency sets.
+func TestNewServerWithLogger_SucceedsWithMediaStreamer(t *testing.T) {
+ srv, err := NewServerWithLogger(ServerDeps{
+ Config: &internal.Config{},
+ MediaStreamer: service.NewMediaStreamer(nil),
+ }, slog.Default())
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if srv == nil {
+ t.Fatal("expected non-nil Server")
+ }
+}
+
+// TestServer_PublicRouteRegistry verifies that the public-route registry on
+// the Middleware is populated by Server.routes() — i.e. the new "register
+// at declaration time" mechanism actually wires every previously-hardcoded
+// public path. If a route is added in server.go without using the
+// handlePublic* helpers, this test catches the regression before the
+// silent-401/redirect bug bites users in production.
+func TestServer_PublicRouteRegistry(t *testing.T) {
+ srv, err := NewServerWithLogger(ServerDeps{
+ Config: &internal.Config{},
+ MediaStreamer: service.NewMediaStreamer(nil),
+ }, slog.Default())
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ // Exact paths that must be public for bootstrap/login/probes to work
+ // before any user exists.
+ wantExact := []string{
+ "/bootstrap.html", "/api/bootstrap", "/api/v1/auth/bootstrap",
+ "/login.html", "/api/login", "/api/v1/auth/login",
+ "/healthz", "/readyz",
+ "/favicon.svg", "/favicon.ico", "/logo.svg", "/logo.png",
+ "/manifest.json", "/sw.js",
+ }
+ for _, p := range wantExact {
+ if !srv.mw.isPublic(p) {
+ t.Errorf("expected %q to be public, but isPublic returned false", p)
+ }
+ }
+
+ // Prefixed routes: static asset trees and dynamic share URLs.
+ wantPrefixed := []string{
+ "/css/site.css",
+ "/js/app.js",
+ "/images/logo.png",
+ "/s/abcdef",
+ "/s/abcdef/stream",
+ }
+ for _, p := range wantPrefixed {
+ if !srv.mw.isPublic(p) {
+ t.Errorf("expected %q to be public via prefix, but isPublic returned false", p)
+ }
+ }
+
+ // Negative: an arbitrary protected path must NOT be public.
+ if srv.mw.isPublic("/api/media") {
+ t.Error("/api/media should not be public")
+ }
+ if srv.mw.isPublic("/") {
+ t.Error("/ should not be public")
+ }
+}