diff options
| author | Paul Buetow <paul@buetow.org> | 2026-05-19 19:22:26 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-05-19 19:22:26 +0300 |
| commit | 4215db6718d4ea662084b647398536aa51934ac6 (patch) | |
| tree | ff0b659f172682e4f87ce558d4a66bd3e3e38424 /player-server | |
| parent | 35aa611038c97a2ce27510e3898318afca37cac0 (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.go | 12 | ||||
| -rw-r--r-- | player-server/internal/api/middleware.go | 74 | ||||
| -rw-r--r-- | player-server/internal/api/server.go | 92 | ||||
| -rw-r--r-- | player-server/internal/api/server_test.go | 90 |
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") + } +} |
