From c3a781f85b9e871c4470a33d46f73295529fc0ff Mon Sep 17 00:00:00 2001 From: Ben Word Date: Mon, 16 Mar 2026 12:05:29 -0500 Subject: [PATCH 1/3] Replace chi router with net/http ServeMux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the chi dependency in favor of Go 1.22+ ServeMux patterns and a small set of stdlib middleware (Recoverer, RealIP, TimeoutHandler). Drop ADMIN_ALLOW_CIDR and RequireAllowedIP — production already disabled IP restriction. Admin access relies solely on in-app auth. Keep TRUST_PROXY so clientIP() works behind Caddy for rate limiting and telemetry dedupe. Co-Authored-By: Scott Walkinshaw Co-Authored-By: Claude Opus 4.6 (1M context) --- deploy/ansible/group_vars/production/main.yml | 3 - deploy/ansible/roles/app/templates/env.j2 | 2 - docs/admin-access.md | 47 +--- docs/architecture.md | 6 +- docs/development.md | 5 +- docs/operations.md | 7 +- go.mod | 1 - go.sum | 2 - internal/config/config.go | 15 +- internal/http/handlers.go | 7 +- internal/http/middleware.go | 58 ----- internal/http/middleware_test.go | 106 --------- internal/http/router.go | 211 ++++++++++++++---- internal/http/router_test.go | 136 +++++++++++ internal/http/seo.go | 4 +- test/smoke_test.sh | 12 +- 16 files changed, 322 insertions(+), 300 deletions(-) delete mode 100644 internal/http/middleware_test.go create mode 100644 internal/http/router_test.go diff --git a/deploy/ansible/group_vars/production/main.yml b/deploy/ansible/group_vars/production/main.yml index ae3836c..12d1078 100644 --- a/deploy/ansible/group_vars/production/main.yml +++ b/deploy/ansible/group_vars/production/main.yml @@ -32,9 +32,6 @@ r2_cdn_public_url: "https://cdn.wp-composer.com" # Sessions session_lifetime_minutes: 7200 -# Admin access (empty = no IP restriction, set Tailscale CIDRs when ready) -admin_allow_cidr: "" - # Repository repository_path: "{{ app_root }}/shared/storage/repository" diff --git a/deploy/ansible/roles/app/templates/env.j2 b/deploy/ansible/roles/app/templates/env.j2 index c1286fe..bfb98a5 100644 --- a/deploy/ansible/roles/app/templates/env.j2 +++ b/deploy/ansible/roles/app/templates/env.j2 @@ -6,8 +6,6 @@ LOG_LEVEL={{ go_log_level }} DB_PATH={{ db_path }} LISTEN_ADDR={{ go_listen_addr }} TRUST_PROXY=true - -ADMIN_ALLOW_CIDR={{ admin_allow_cidr }} SESSION_LIFETIME_MINUTES={{ session_lifetime_minutes }} SEEDS_FILE={{ seeds_file }} diff --git a/docs/admin-access.md b/docs/admin-access.md index 8c9f39a..521b245 100644 --- a/docs/admin-access.md +++ b/docs/admin-access.md @@ -2,44 +2,17 @@ ## Security Model -Admin access uses defense in depth with two independent layers: +Admin access is protected by in-app authentication. Email/password login and admin authorization are required for all protected `/admin/*` routes. -1. **Network layer** — Tailscale restricts access to `/admin/*` routes. Only devices on the tailnet can reach admin endpoints. -2. **Application layer** — in-app authentication (email/password) and admin authorization required for all protected `/admin/*` routes. +## Proxy Trust -Both layers must pass. A valid Tailscale connection without app auth (or vice versa) is denied. - -## Network Restriction - -The Go app enforces IP-based access control on all `/admin/*` routes via the `ADMIN_ALLOW_CIDR` config. - -Default allowed ranges (Tailscale): -- `100.64.0.0/10` (Tailscale IPv4) -- `fd7a:115c:a1e0::/48` (Tailscale IPv6) - -Override via environment: - -```env -ADMIN_ALLOW_CIDR=100.64.0.0/10,fd7a:115c:a1e0::/48,10.0.0.0/8 -``` - -Set to empty to disable IP restriction (development only): - -```env -ADMIN_ALLOW_CIDR= -``` - -If all configured CIDRs are invalid, the middleware fails closed — all admin access is blocked until the configuration is corrected. - -### Proxy trust - -By default, the app uses the raw `RemoteAddr` for IP checks. If behind a reverse proxy (Caddy, nginx), enable proxy header trust: +When behind a reverse proxy (Caddy, nginx), enable proxy header trust so the app sees the real client IP (used for login rate limiting and telemetry dedupe): ```env TRUST_PROXY=true ``` -This enables Chi's `RealIP` middleware, which reads `X-Forwarded-For` / `X-Real-IP` headers to determine the client IP. **Only enable this when the app is behind a trusted proxy** — otherwise clients can spoof their IP via forwarding headers and bypass admin IP restrictions. +This reads `X-Real-IP` / `X-Forwarded-For` headers to determine the client IP. **Only enable this when the app is behind a trusted proxy** — otherwise clients can spoof their IP. ## Admin Bootstrap @@ -78,18 +51,6 @@ wpcomposer cleanup-sessions Run via systemd timer or cron (daily recommended). -## Exposure Verification - -To verify admin is not publicly accessible: - -```bash -# From outside the tailnet — should return 403 -curl -s -o /dev/null -w "%{http_code}" https://app.example.com/admin/login - -# From inside the tailnet — should return 200 -curl -s -o /dev/null -w "%{http_code}" https://app.example.com/admin/login -``` - ## Emergency Password Reset If locked out of the admin panel: diff --git a/docs/architecture.md b/docs/architecture.md index 9a98f48..6a5519e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -29,7 +29,7 @@ WP Composer has two primary runtime concerns: ### Web UI - Public package browser/detail pages via server-rendered Go templates + Tailwind. -- Admin panel at `/admin` with Tailscale network gating + in-app auth (defense in depth). +- Admin panel at `/admin` with in-app auth (email/password + session). ## Module Layout @@ -43,7 +43,7 @@ internal/ ├── repository/ artifact generation, hashing, integrity validation ├── deploy/ local promote/rollback/cleanup + R2 sync ├── telemetry/ event ingestion, dedupe, rollups -└── http/ Chi router, handlers, templates, static assets +└── http/ stdlib router, handlers, templates, static assets ``` ## Data Flow @@ -93,7 +93,7 @@ storage/repository/ - `GET /admin/packages` — package management - `GET /admin/builds` — build history/status - Admin-triggered sync/build/deploy actions -- Access: Tailscale network gating + in-app auth/authorization required +- Access: in-app auth/authorization required ## Static Repository Deployment diff --git a/docs/development.md b/docs/development.md index d231105..1db45af 100644 --- a/docs/development.md +++ b/docs/development.md @@ -68,7 +68,6 @@ Env-first with optional YAML config file (env overrides YAML). | `R2_SECRET_ACCESS_KEY` | — | R2 credentials | | `R2_BUCKET` | — | R2 bucket name | | `R2_ENDPOINT` | — | R2 S3-compatible endpoint | -| `ADMIN_ALLOW_CIDR` | Tailscale ranges | Comma-separated CIDRs for `/admin/*` access | | `TRUST_PROXY` | `false` | Trust `X-Forwarded-For` for client IP (enable behind proxy) | | `SESSION_LIFETIME_MINUTES` | `7200` | Admin session lifetime | @@ -77,14 +76,14 @@ Env-first with optional YAML config file (env overrides YAML). | Area | Choice | |------|--------| | CLI | Cobra | -| HTTP router | Chi | +| HTTP router | `net/http` (stdlib) | | Migrations | Goose (SQL-first) | | Templates | `html/template` + Tailwind | | Logging | `log/slog` | | SQLite driver | `modernc.org/sqlite` | | R2 | AWS SDK for Go v2 | | Config | env-first + optional YAML | -| Admin access | Tailscale network gating + in-app auth | +| Admin access | In-app auth (email/password + session) | ## Make Targets diff --git a/docs/operations.md b/docs/operations.md index 049f4c8..96625db 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -165,12 +165,7 @@ echo 'new-password' | wpcomposer admin reset-password --email admin@example.com ## Admin Access Model -Admin access uses defense in depth: - -1. **Network layer** — Tailscale restricts access to `/admin/*` routes to authorized devices on the tailnet. -2. **Application layer** — in-app authentication and admin authorization required for all `/admin/*` routes. - -Both layers must pass. A valid Tailscale connection without app auth (or vice versa) is denied. +Admin access is protected by in-app authentication (email/password) and admin authorization for all `/admin/*` routes. ## Server Provisioning diff --git a/go.mod b/go.mod index 8cfe735..a1a07ae 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/s3 v1.97.1 github.com/fogleman/gg v1.3.0 github.com/getsentry/sentry-go v0.43.0 - github.com/go-chi/chi/v5 v5.2.5 github.com/golang/freetype v0.0.0-20170609003504-e2365dfdc4a0 github.com/pressly/goose/v3 v3.27.0 github.com/spf13/cobra v1.10.2 diff --git a/go.sum b/go.sum index 728185a..bbfc120 100644 --- a/go.sum +++ b/go.sum @@ -32,8 +32,6 @@ github.com/fogleman/gg v1.3.0 h1:/7zJX8F6AaYQc57WQCyN9cAIz+4bCJGO9B+dyW29am8= github.com/fogleman/gg v1.3.0/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k= github.com/getsentry/sentry-go v0.43.0 h1:XbXLpFicpo8HmBDaInk7dum18G9KSLcjZiyUKS+hLW4= github.com/getsentry/sentry-go v0.43.0/go.mod h1:XDotiNZbgf5U8bPDUAfvcFmOnMQQceESxyKaObSssW0= -github.com/go-chi/chi/v5 v5.2.5 h1:Eg4myHZBjyvJmAFjFvWgrqDTXFyOzjj7YIm3L3mu6Ug= -github.com/go-chi/chi/v5 v5.2.5/go.mod h1:X7Gx4mteadT3eDOMTsXzmI4/rwUpOwBHLpAfupzFJP0= github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA= github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og= github.com/golang/freetype v0.0.0-20170609003504-e2365dfdc4a0 h1:DACJavvAHhabrF08vX0COfcOBJRhZ8lUbR+ZWIs0Y5g= diff --git a/internal/config/config.go b/internal/config/config.go index 7a4bfcc..d7f3346 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,9 +30,8 @@ type DBConfig struct { } type ServerConfig struct { - Addr string `yaml:"addr"` - AdminAllowCIDR []string `yaml:"admin_allow_cidr"` - TrustProxy bool `yaml:"trust_proxy"` + Addr string `yaml:"addr"` + TrustProxy bool `yaml:"trust_proxy"` } type R2Config struct { @@ -68,8 +67,7 @@ func defaults() *Config { LogLevel: "info", DB: DBConfig{Path: "./storage/wpcomposer.db"}, Server: ServerConfig{ - Addr: ":8080", - AdminAllowCIDR: []string{"100.64.0.0/10", "fd7a:115c:a1e0::/48"}, // Tailscale default ranges + Addr: ":8080", }, Session: SessionConfig{LifetimeMinutes: 7200}, Telemetry: TelemetryConfig{ @@ -159,13 +157,6 @@ func applyEnv(cfg *Config) { if v := os.Getenv("TRUST_PROXY"); v != "" { cfg.Server.TrustProxy = strings.EqualFold(v, "true") || v == "1" } - if v, ok := os.LookupEnv("ADMIN_ALLOW_CIDR"); ok { - if v == "" { - cfg.Server.AdminAllowCIDR = nil // explicitly disable restriction - } else { - cfg.Server.AdminAllowCIDR = strings.Split(v, ",") - } - } if v := os.Getenv("SEEDS_FILE"); v != "" { cfg.Discovery.SeedsFile = v } diff --git a/internal/http/handlers.go b/internal/http/handlers.go index 067dafc..790ebeb 100644 --- a/internal/http/handlers.go +++ b/internal/http/handlers.go @@ -20,7 +20,6 @@ import ( "time" "github.com/getsentry/sentry-go" - "github.com/go-chi/chi/v5" "github.com/roots/wp-composer/internal/app" "github.com/roots/wp-composer/internal/config" "github.com/roots/wp-composer/internal/deploy" @@ -197,8 +196,8 @@ func handleRootsWordpress(a *app.App, tmpl *templateSet) http.HandlerFunc { func handleDetail(a *app.App, tmpl *templateSet) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - pkgType := chi.URLParam(r, "type") - name := chi.URLParam(r, "name") + pkgType := r.PathValue("type") + name := r.PathValue("name") // Strip wp- prefix from type pkgType = strings.TrimPrefix(pkgType, "wp-") @@ -630,7 +629,7 @@ func ogImageURL(cfg *config.Config, key string) string { // handleOGImage serves OG images from local disk (dev mode). func handleOGImage() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - key := chi.URLParam(r, "*") + key := strings.TrimPrefix(r.URL.Path, "/og/") clean := filepath.Clean(key) if strings.Contains(clean, "..") { http.NotFound(w, r) diff --git a/internal/http/middleware.go b/internal/http/middleware.go index 9a968d0..ce65fa5 100644 --- a/internal/http/middleware.go +++ b/internal/http/middleware.go @@ -3,8 +3,6 @@ package http import ( "context" "database/sql" - "log/slog" - "net" "net/http" "github.com/roots/wp-composer/internal/auth" @@ -63,59 +61,3 @@ func RequireAdmin(next http.Handler) http.Handler { next.ServeHTTP(w, r) }) } - -// RequireAllowedIP restricts access to requests from allowed CIDR ranges. -// If allowCIDRs is nil or empty, all requests are allowed (no network restriction). -// If CIDRs are configured but none parse successfully, access is denied (fail closed). -func RequireAllowedIP(allowCIDRs []string, logger *slog.Logger) func(http.Handler) http.Handler { - configured := len(allowCIDRs) > 0 - var nets []*net.IPNet - for _, cidr := range allowCIDRs { - _, ipNet, err := net.ParseCIDR(cidr) - if err != nil { - logger.Error("invalid admin CIDR — admin access will be restricted", "cidr", cidr, "error", err) - continue - } - nets = append(nets, ipNet) - } - - // Fail closed: CIDRs were configured but none parsed - failClosed := configured && len(nets) == 0 - if failClosed { - logger.Error("all admin CIDRs are invalid — admin access is blocked") - } - - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if failClosed { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } - - if !configured { - next.ServeHTTP(w, r) - return - } - - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - host = r.RemoteAddr - } - ip := net.ParseIP(host) - if ip == nil { - http.Error(w, "Forbidden", http.StatusForbidden) - return - } - - for _, n := range nets { - if n.Contains(ip) { - next.ServeHTTP(w, r) - return - } - } - - logger.Warn("admin access denied", "ip", host) - http.Error(w, "Forbidden", http.StatusForbidden) - }) - } -} diff --git a/internal/http/middleware_test.go b/internal/http/middleware_test.go deleted file mode 100644 index b1c4ddb..0000000 --- a/internal/http/middleware_test.go +++ /dev/null @@ -1,106 +0,0 @@ -package http - -import ( - "log/slog" - "net/http" - "net/http/httptest" - "testing" -) - -func TestRequireAllowedIP_Allowed(t *testing.T) { - mw := RequireAllowedIP([]string{"100.64.0.0/10"}, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "100.100.50.1:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 200 { - t.Errorf("Tailscale IP should be allowed, got %d", w.Code) - } -} - -func TestRequireAllowedIP_Denied(t *testing.T) { - mw := RequireAllowedIP([]string{"100.64.0.0/10"}, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "203.0.113.1:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 403 { - t.Errorf("public IP should be denied, got %d", w.Code) - } -} - -func TestRequireAllowedIP_EmptyCIDR(t *testing.T) { - mw := RequireAllowedIP(nil, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "203.0.113.1:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 200 { - t.Errorf("empty CIDR list should allow all, got %d", w.Code) - } -} - -func TestRequireAllowedIP_IPv6(t *testing.T) { - mw := RequireAllowedIP([]string{"fd7a:115c:a1e0::/48"}, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "[fd7a:115c:a1e0::1]:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 200 { - t.Errorf("Tailscale IPv6 should be allowed, got %d", w.Code) - } -} - -func TestRequireAllowedIP_FailClosed(t *testing.T) { - // All CIDRs invalid — should deny everything - mw := RequireAllowedIP([]string{"not-a-cidr", "also-bad"}, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "100.100.50.1:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 403 { - t.Errorf("invalid CIDRs should fail closed, got %d", w.Code) - } -} - -func TestRequireAllowedIP_MultipleCIDRs(t *testing.T) { - mw := RequireAllowedIP([]string{"10.0.0.0/8", "100.64.0.0/10"}, slog.Default()) - handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - // Matches second CIDR - req := httptest.NewRequest("GET", "/admin", nil) - req.RemoteAddr = "100.100.50.1:12345" - w := httptest.NewRecorder() - handler.ServeHTTP(w, req) - - if w.Code != 200 { - t.Errorf("should match second CIDR, got %d", w.Code) - } -} diff --git a/internal/http/router.go b/internal/http/router.go index 8a8fb53..f43b409 100644 --- a/internal/http/router.go +++ b/internal/http/router.go @@ -2,16 +2,18 @@ package http import ( "encoding/json" + "fmt" "io/fs" + "log/slog" "net/http" "os" "path/filepath" "regexp" + "runtime/debug" + "strings" "time" sentryhttp "github.com/getsentry/sentry-go/http" - "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" "github.com/roots/wp-composer/internal/app" ) @@ -36,22 +38,22 @@ func stripAssetHash(next http.Handler) http.Handler { }) } -func NewRouter(a *app.App) chi.Router { - r := chi.NewRouter() +func NewRouter(a *app.App) http.Handler { + mux := http.NewServeMux() tmpl := loadTemplates(a.Config.Env) - r.Use(middleware.RequestID) - if a.Config.Server.TrustProxy { - r.Use(middleware.RealIP) + // route registers a handler on the mux, wrapping it with routeMarker so + // appHandler can distinguish matched routes from mux-internal 404/405. + route := func(pattern string, handler http.Handler) { + mux.Handle(pattern, routeMarker(handler)) + } + routeFunc := func(pattern string, handler http.HandlerFunc) { + route(pattern, handler) } - - r.Use(middleware.Recoverer) sentryMiddleware := sentryhttp.New(sentryhttp.Options{Repanic: true}) - r.Use(sentryMiddleware.Handle) - r.Use(middleware.Timeout(60 * time.Second)) - r.Get("/health", func(w http.ResponseWriter, r *http.Request) { + routeFunc("GET /health", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) }) @@ -60,72 +62,183 @@ func NewRouter(a *app.App) chi.Router { staticServer := http.FileServer(http.FS(staticSub)) cachedStatic := cacheControl("public, max-age=31536000, immutable", stripAssetHash(staticServer)) for _, f := range []string{"/favicon.ico", "/icon.svg", "/icon-192.png", "/icon-512.png", "/apple-touch-icon.png", "/manifest.webmanifest"} { - r.Get(f, cachedStatic.ServeHTTP) + route("GET "+f, cachedStatic) } - r.Get("/assets/*", cachedStatic.ServeHTTP) + route("GET /assets/", cachedStatic) // Ensure fallback OG image exists (uploads to R2 in production) ensureLocalFallbackOG(a.Config) // Serve OG images from local disk (dev mode — production uses CDN) if a.Config.R2.CDNPublicURL == "" { - r.Get("/og/*", handleOGImage()) + routeFunc("GET /og/", handleOGImage()) } - r.Get("/feed.xml", handleFeed(a)) - r.Get("/robots.txt", handleRobotsTxt(a)) + routeFunc("GET /feed.xml", handleFeed(a)) + routeFunc("GET /robots.txt", handleRobotsTxt(a)) sitemaps := &sitemapData{} - r.Get("/sitemap.xml", handleSitemapIndex(a, sitemaps)) - r.Get("/sitemap-pages.xml", handleSitemapPages(a, sitemaps)) - r.Get("/sitemap-packages-{page}.xml", handleSitemapPackages(a, sitemaps)) + routeFunc("GET /sitemap.xml", handleSitemapIndex(a, sitemaps)) + routeFunc("GET /sitemap-pages.xml", handleSitemapPages(a, sitemaps)) + // sitemap-packages routes are handled in appHandler (prefix can't be a ServeMux pattern) + sitemapPackagesHandler := handleSitemapPackages(a, sitemaps) - r.Get("/", handleIndex(a, tmpl)) - r.Get("/packages-partial", handleIndexPartial(a, tmpl)) - r.Get("/packages/{type}/{name}", handleDetail(a, tmpl)) - r.Get("/wp-composer-vs-wpackagist", handleCompare(a, tmpl)) - r.Get("/roots-wordpress", handleRootsWordpress(a, tmpl)) + routeFunc("GET /{$}", handleIndex(a, tmpl)) + routeFunc("GET /packages-partial", handleIndexPartial(a, tmpl)) + routeFunc("GET /packages/{type}/{name}", handleDetail(a, tmpl)) + routeFunc("GET /wp-composer-vs-wpackagist", handleCompare(a, tmpl)) + routeFunc("GET /roots-wordpress", handleRootsWordpress(a, tmpl)) - r.Post("/downloads", handleDownloads(a)) + routeFunc("POST /downloads", handleDownloads(a)) // Serve static repository files from current build (local/dev mode) repoRoot := filepath.Join("storage", "repository", "current") if _, err := os.Stat(repoRoot); err == nil { fileServer := http.FileServer(http.Dir(repoRoot)) - r.Get("/packages.json", func(w http.ResponseWriter, r *http.Request) { + routeFunc("GET /packages.json", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") fileServer.ServeHTTP(w, r) }) - r.Handle("/p/*", fileServer) - r.Handle("/p2/*", fileServer) + route("/p/", fileServer) + route("/p2/", fileServer) } - // Admin subrouter — network-restricted, login is public within that, rest requires auth - admin := chi.NewRouter() - admin.Use(RequireAllowedIP(a.Config.Server.AdminAllowCIDR, a.Logger)) + // Admin subrouter — all admin handlers are behind routeMarker via StripPrefix + adminMux := http.NewServeMux() + adminMux.HandleFunc("GET /login", handleLoginPage(a)) + adminMux.HandleFunc("POST /login", handleLogin(a)) + adminMux.HandleFunc("POST /logout", handleLogout(a)) + + protectedMux := http.NewServeMux() + protectedMux.HandleFunc("GET /{$}", handleAdminDashboard(a, tmpl)) + protectedMux.HandleFunc("GET /packages", handleAdminPackages(a, tmpl)) + protectedMux.HandleFunc("GET /builds", handleAdminBuilds(a, tmpl)) + protectedMux.HandleFunc("POST /builds/trigger", handleTriggerBuild(a)) + protectedMux.HandleFunc("GET /logs", handleAdminLogs(tmpl)) + protectedMux.HandleFunc("GET /logs/stream", noTimeout(handleAdminLogStream(a))) + adminMux.Handle("/", Chain(protectedMux, SessionAuth(a.DB), RequireAdmin)) + + route("/admin/", http.StripPrefix("/admin", adminMux)) + + // Build handler chain + handler := appHandler(mux, tmpl, a, sitemapPackagesHandler) + handler = http.TimeoutHandler(handler, 60*time.Second, "") + handler = Recoverer(handler, a.Logger) + handler = sentryMiddleware.Handle(handler) + if a.Config.Server.TrustProxy { + handler = RealIP(handler) + } + + return handler +} + +// appHandler routes requests that ServeMux can't express (mid-segment prefixes) +// and renders a custom 404 template for unmatched routes. It preserves stdlib +// 405 behavior and does not interfere with handler-generated 404s. +// +// The approach: record the status the mux writes. If it's 404 and no registered +// handler touched the response (checked via a context flag set by routeMarker), +// replace the default body with the custom template. 405s and handler-generated +// 404s pass through untouched. +func appHandler(mux *http.ServeMux, tmpl *templateSet, a *app.App, sitemapPackages http.HandlerFunc) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Sitemap-packages prefix can't be expressed as a ServeMux pattern + // (wildcards must be full path segments). + if r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/sitemap-packages-") { + sitemapPackages(w, r) + return + } + + rec := &statusRecorder{ResponseWriter: w} + mux.ServeHTTP(rec, r) + + // Only replace the default ServeMux 404 — not handler-generated ones. + // A registered handler sets the dispatched flag via routeMarker middleware, + // so rec.dispatched is false only when the mux itself returned 404/405/etc. + if rec.code == http.StatusNotFound && !rec.dispatched { + w.WriteHeader(http.StatusNotFound) + render(w, r, tmpl.notFound, "layout", map[string]any{"Gone": false, "CDNURL": a.Config.R2.CDNPublicURL}) + } + }) +} - admin.Get("/login", handleLoginPage(a)) - admin.Post("/login", handleLogin(a)) - admin.Post("/logout", handleLogout(a)) +type statusRecorder struct { + http.ResponseWriter + code int + dispatched bool // true if a registered handler wrote the response +} + +func (r *statusRecorder) WriteHeader(code int) { + r.code = code + if r.dispatched || code != http.StatusNotFound { + r.ResponseWriter.WriteHeader(code) + } +} - admin.Group(func(r chi.Router) { - r.Use(SessionAuth(a.DB)) - r.Use(RequireAdmin) +func (r *statusRecorder) Write(b []byte) (int, error) { + if !r.dispatched && r.code == http.StatusNotFound { + return len(b), nil // swallow default 404 body + } + return r.ResponseWriter.Write(b) +} - r.Get("/", handleAdminDashboard(a, tmpl)) - r.Get("/packages", handleAdminPackages(a, tmpl)) - r.Get("/builds", handleAdminBuilds(a, tmpl)) - r.Post("/builds/trigger", handleTriggerBuild(a)) - r.Get("/logs", handleAdminLogs(tmpl)) +// markDispatched sets the dispatched flag on the statusRecorder, indicating +// that a registered handler is handling this request (as opposed to the +// mux's internal 404/405 handler). +func (r *statusRecorder) markDispatched() { + r.dispatched = true +} - r.Get("/logs/stream", noTimeout(handleAdminLogStream(a))) +// routeMarker wraps a handler to mark the response as dispatched by a +// registered route. This lets appHandler distinguish mux-generated 404s +// from handler-generated 404s. +func routeMarker(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if rec, ok := w.(*statusRecorder); ok { + rec.markDispatched() + } + h.ServeHTTP(w, r) }) +} - r.Mount("/admin", admin) +// Chain applies middleware in order (first argument is innermost). +func Chain(h http.Handler, mw ...func(http.Handler) http.Handler) http.Handler { + for i := len(mw) - 1; i >= 0; i-- { + h = mw[i](h) + } + return h +} - r.NotFound(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - render(w, r, tmpl.notFound, "layout", map[string]any{"Gone": false, "CDNURL": a.Config.R2.CDNPublicURL}) +// Recoverer recovers from panics, logs the error, and returns 500. +func Recoverer(next http.Handler, logger *slog.Logger) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { + if err := recover(); err != nil { + logger.Error("panic recovered", + "error", fmt.Sprintf("%v", err), + "stack", string(debug.Stack()), + ) + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + } + }() + next.ServeHTTP(w, r) }) +} - return r +// RealIP sets r.RemoteAddr to the client IP from X-Forwarded-For or +// X-Real-IP headers. Only enable behind a trusted reverse proxy. +func RealIP(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if ip := r.Header.Get("X-Real-IP"); ip != "" { + r.RemoteAddr = ip + } else if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + // First entry is the original client + if i := strings.IndexByte(xff, ','); i > 0 { + ip = strings.TrimSpace(xff[:i]) + } else { + ip = strings.TrimSpace(xff) + } + r.RemoteAddr = ip + } + next.ServeHTTP(w, r) + }) } diff --git a/internal/http/router_test.go b/internal/http/router_test.go new file mode 100644 index 0000000..15d5d21 --- /dev/null +++ b/internal/http/router_test.go @@ -0,0 +1,136 @@ +package http + +import ( + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/roots/wp-composer/internal/app" + "github.com/roots/wp-composer/internal/config" + "github.com/roots/wp-composer/internal/db" + "github.com/roots/wp-composer/internal/packagist" +) + +func newTestApp(t *testing.T) *app.App { + t.Helper() + database, err := db.Open(":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = database.Close() }) + + return &app.App{ + Config: &config.Config{}, + DB: database, + Logger: slog.Default(), + Packagist: packagist.NewDownloadsCache(slog.Default()), + } +} + +// TestNewRouter_NoPanic verifies that all ServeMux patterns are valid and +// route registration does not panic. +func TestNewRouter_NoPanic(t *testing.T) { + a := newTestApp(t) + handler := NewRouter(a) + if handler == nil { + t.Fatal("NewRouter returned nil") + } +} + +func TestRouter_MethodNotAllowed(t *testing.T) { + a := newTestApp(t) + handler := NewRouter(a) + + // POST /health should return 405, not 404 + req := httptest.NewRequest("POST", "/health", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("POST /health: got %d, want 405", w.Code) + } + if allow := w.Header().Get("Allow"); allow == "" { + t.Error("POST /health: missing Allow header") + } +} + +func TestRouter_NotFound(t *testing.T) { + a := newTestApp(t) + handler := NewRouter(a) + + req := httptest.NewRequest("GET", "/nonexistent", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Errorf("GET /nonexistent: got %d, want 404", w.Code) + } +} + +func TestRouter_HandlerGenerated404PreservesBody(t *testing.T) { + // A registered handler that returns 404 with its own body should not + // have that body replaced by the custom not-found template. + mux := http.NewServeMux() + mux.Handle("GET /pkg/{name}", routeMarker(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "package not found", http.StatusNotFound) + }))) + + a := newTestApp(t) + tmpl := loadTemplates("") + handler := appHandler(mux, tmpl, a, nil) + + req := httptest.NewRequest("GET", "/pkg/nope", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("GET /pkg/nope: got %d, want 404", w.Code) + } + body := w.Body.String() + if body != "package not found\n" { + t.Errorf("handler-generated 404 body was replaced: got %q", body) + } +} + +func TestRouter_UnmatchedRouteRendersTemplate(t *testing.T) { + // An unmatched route should render the custom 404 template, not the + // default "404 page not found" text from ServeMux. + mux := http.NewServeMux() + mux.Handle("GET /health", routeMarker(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("ok")) + }))) + + a := newTestApp(t) + tmpl := loadTemplates("") + handler := appHandler(mux, tmpl, a, nil) + + req := httptest.NewRequest("GET", "/nonexistent", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("GET /nonexistent: got %d, want 404", w.Code) + } + body := w.Body.String() + if body == "404 page not found\n" { + t.Error("unmatched route got default ServeMux 404 body instead of custom template") + } +} + +func TestRouter_SitemapPackagesRoutes(t *testing.T) { + a := newTestApp(t) + handler := NewRouter(a) + + // Should not 404 — the route should be matched even though + // it can't be a ServeMux pattern. + req := httptest.NewRequest("GET", "/sitemap-packages-0.xml", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + // 200 (if sitemap data generates) or 500 (no DB tables) are both acceptable; + // 404 means the route wasn't matched. + if w.Code == http.StatusNotFound { + t.Error("GET /sitemap-packages-0.xml returned 404 — route not matched") + } +} diff --git a/internal/http/seo.go b/internal/http/seo.go index 06e889b..09c9401 100644 --- a/internal/http/seo.go +++ b/internal/http/seo.go @@ -11,7 +11,6 @@ import ( "sync" "time" - "github.com/go-chi/chi/v5" "github.com/roots/wp-composer/internal/app" ) @@ -254,7 +253,8 @@ func handleSitemapPackages(a *app.App, data *sitemapData) http.HandlerFunc { return } - pageStr := strings.TrimSuffix(chi.URLParam(r, "page"), ".xml") + slug := strings.TrimPrefix(r.URL.Path, "/sitemap-packages-") + pageStr := strings.TrimSuffix(slug, ".xml") page, err := strconv.Atoi(pageStr) if err != nil || page < 0 { http.NotFound(w, r) diff --git a/test/smoke_test.sh b/test/smoke_test.sh index 796e390..859adb0 100755 --- a/test/smoke_test.sh +++ b/test/smoke_test.sh @@ -88,7 +88,7 @@ APP_URL="$APP_URL" "$BINARY" build --db "$DB_PATH" > /dev/null 2>&1 echo "" echo "=== Starting server ===" -ADMIN_ALLOW_CIDR= "$BINARY" serve --db "$DB_PATH" --addr ":${APP_PORT}" > /dev/null 2>&1 & +"$BINARY" serve --db "$DB_PATH" --addr ":${APP_PORT}" > /dev/null 2>&1 & APP_PID=$! sleep 2 @@ -227,8 +227,8 @@ LOGIN_STATUS=$(curl -sf -o /dev/null -w "%{http_code}" "${APP_URL}/admin/login") assert_eq "$LOGIN_STATUS" "200" "GET /admin/login returns 200" # Admin dashboard should redirect to login (no session) -ADMIN_STATUS=$(curl -s -o /dev/null -w "%{http_code}" "${APP_URL}/admin") -assert_eq "$ADMIN_STATUS" "303" "GET /admin without auth redirects (303)" +ADMIN_STATUS=$(curl -s -o /dev/null -w "%{http_code}" "${APP_URL}/admin/") +assert_eq "$ADMIN_STATUS" "303" "GET /admin/ without auth redirects (303)" # ─── Dedupe verification ────────────────────────────────────────── @@ -349,8 +349,8 @@ fi # Access admin with session cookie if [ -n "$SESSION_COOKIE" ]; then AUTHED_STATUS=$(curl -sf -o /dev/null -w "%{http_code}" \ - -b "session=${SESSION_COOKIE}" "${APP_URL}/admin") - assert_eq "$AUTHED_STATUS" "200" "GET /admin with session returns 200" + -b "session=${SESSION_COOKIE}" "${APP_URL}/admin/") + assert_eq "$AUTHED_STATUS" "200" "GET /admin/ with session returns 200" AUTHED_PACKAGES=$(curl -sf -o /dev/null -w "%{http_code}" \ -b "session=${SESSION_COOKIE}" "${APP_URL}/admin/packages") @@ -378,7 +378,7 @@ if [ -n "$SESSION_COOKIE" ]; then else # Check if session is invalidated by trying to access admin POST_LOGOUT=$(curl -s -o /dev/null -w "%{http_code}" \ - -b "session=${SESSION_COOKIE}" "${APP_URL}/admin") + -b "session=${SESSION_COOKIE}" "${APP_URL}/admin/") assert_eq "$POST_LOGOUT" "303" "admin inaccessible after logout" fi fi From a0749b5dcb2727ce21e1908a369e15cf5cd4c690 Mon Sep 17 00:00:00 2001 From: Ben Word Date: Mon, 16 Mar 2026 14:19:39 -0500 Subject: [PATCH 2/3] Remove TrustProxy config, always apply RealIP middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RealIP is needed for login rate limiting and telemetry dedupe behind Caddy. No reason to gate it behind a config flag — the app is always proxied in production, and in dev the headers aren't present so RemoteAddr passes through unchanged. Co-Authored-By: Scott Walkinshaw Co-Authored-By: Claude Opus 4.6 (1M context) --- deploy/ansible/roles/app/templates/env.j2 | 1 - docs/admin-access.md | 10 +--------- docs/development.md | 1 - internal/config/config.go | 6 +----- internal/http/router.go | 4 +--- 5 files changed, 3 insertions(+), 19 deletions(-) diff --git a/deploy/ansible/roles/app/templates/env.j2 b/deploy/ansible/roles/app/templates/env.j2 index bfb98a5..292c340 100644 --- a/deploy/ansible/roles/app/templates/env.j2 +++ b/deploy/ansible/roles/app/templates/env.j2 @@ -5,7 +5,6 @@ LOG_LEVEL={{ go_log_level }} DB_PATH={{ db_path }} LISTEN_ADDR={{ go_listen_addr }} -TRUST_PROXY=true SESSION_LIFETIME_MINUTES={{ session_lifetime_minutes }} SEEDS_FILE={{ seeds_file }} diff --git a/docs/admin-access.md b/docs/admin-access.md index 521b245..9ff91d6 100644 --- a/docs/admin-access.md +++ b/docs/admin-access.md @@ -4,15 +4,7 @@ Admin access is protected by in-app authentication. Email/password login and admin authorization are required for all protected `/admin/*` routes. -## Proxy Trust - -When behind a reverse proxy (Caddy, nginx), enable proxy header trust so the app sees the real client IP (used for login rate limiting and telemetry dedupe): - -```env -TRUST_PROXY=true -``` - -This reads `X-Real-IP` / `X-Forwarded-For` headers to determine the client IP. **Only enable this when the app is behind a trusted proxy** — otherwise clients can spoof their IP. +**Note:** The app always trusts `X-Real-IP` / `X-Forwarded-For` headers for client IP resolution (used for login rate limiting and telemetry dedupe). It must be deployed behind a trusted reverse proxy (Caddy) — never exposed directly to the internet. ## Admin Bootstrap diff --git a/docs/development.md b/docs/development.md index 1db45af..b22c412 100644 --- a/docs/development.md +++ b/docs/development.md @@ -68,7 +68,6 @@ Env-first with optional YAML config file (env overrides YAML). | `R2_SECRET_ACCESS_KEY` | — | R2 credentials | | `R2_BUCKET` | — | R2 bucket name | | `R2_ENDPOINT` | — | R2 S3-compatible endpoint | -| `TRUST_PROXY` | `false` | Trust `X-Forwarded-For` for client IP (enable behind proxy) | | `SESSION_LIFETIME_MINUTES` | `7200` | Admin session lifetime | ## Technical Decisions diff --git a/internal/config/config.go b/internal/config/config.go index d7f3346..a12e0fe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,8 +30,7 @@ type DBConfig struct { } type ServerConfig struct { - Addr string `yaml:"addr"` - TrustProxy bool `yaml:"trust_proxy"` + Addr string `yaml:"addr"` } type R2Config struct { @@ -154,9 +153,6 @@ func applyEnv(cfg *Config) { cfg.Telemetry.DedupeWindowSeconds = n } } - if v := os.Getenv("TRUST_PROXY"); v != "" { - cfg.Server.TrustProxy = strings.EqualFold(v, "true") || v == "1" - } if v := os.Getenv("SEEDS_FILE"); v != "" { cfg.Discovery.SeedsFile = v } diff --git a/internal/http/router.go b/internal/http/router.go index f43b409..5ff426b 100644 --- a/internal/http/router.go +++ b/internal/http/router.go @@ -124,9 +124,7 @@ func NewRouter(a *app.App) http.Handler { handler = http.TimeoutHandler(handler, 60*time.Second, "") handler = Recoverer(handler, a.Logger) handler = sentryMiddleware.Handle(handler) - if a.Config.Server.TrustProxy { - handler = RealIP(handler) - } + handler = RealIP(handler) return handler } From b004f263c91cd8b60022e975e958d57f84b768b2 Mon Sep 17 00:00:00 2001 From: Ben Word Date: Tue, 17 Mar 2026 13:33:42 -0500 Subject: [PATCH 3/3] Fix SSE log stream timeout and Flusher support The global http.TimeoutHandler replaced the ResponseWriter with a buffering writer that doesn't implement http.Flusher, breaking /admin/logs/stream in two ways: 1. The SSE handler's w.(http.Flusher) assertion always failed (500) 2. The previous noTimeout(context.WithoutCancel) couldn't bypass TimeoutHandler since it replaces the writer, not just the context Fix: path-based timeout bypass that skips http.TimeoutHandler entirely for streaming endpoints, preserving the original ResponseWriter. Add Flush()/Unwrap() to statusRecorder so Flusher propagates through the custom 404 layer. Remove the broken context-key noTimeout mechanism. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/http/middleware.go | 9 --- internal/http/router.go | 35 ++++++++- internal/http/router_test.go | 147 ++++++++++++++++++++++++++++++++++- 3 files changed, 179 insertions(+), 12 deletions(-) diff --git a/internal/http/middleware.go b/internal/http/middleware.go index ce65fa5..17cd154 100644 --- a/internal/http/middleware.go +++ b/internal/http/middleware.go @@ -12,15 +12,6 @@ type contextKey string const userContextKey contextKey = "user" -// noTimeout wraps a handler to bypass the global timeout middleware. -// Client disconnects are detected via failed writes in the handler. -func noTimeout(h http.HandlerFunc) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - ctx := context.WithoutCancel(r.Context()) - h.ServeHTTP(w, r.WithContext(ctx)) - } -} - func UserFromContext(ctx context.Context) *auth.User { u, _ := ctx.Value(userContextKey).(*auth.User) return u diff --git a/internal/http/router.go b/internal/http/router.go index 5ff426b..d1106d5 100644 --- a/internal/http/router.go +++ b/internal/http/router.go @@ -114,14 +114,14 @@ func NewRouter(a *app.App) http.Handler { protectedMux.HandleFunc("GET /builds", handleAdminBuilds(a, tmpl)) protectedMux.HandleFunc("POST /builds/trigger", handleTriggerBuild(a)) protectedMux.HandleFunc("GET /logs", handleAdminLogs(tmpl)) - protectedMux.HandleFunc("GET /logs/stream", noTimeout(handleAdminLogStream(a))) + protectedMux.HandleFunc("GET /logs/stream", handleAdminLogStream(a)) adminMux.Handle("/", Chain(protectedMux, SessionAuth(a.DB), RequireAdmin)) route("/admin/", http.StripPrefix("/admin", adminMux)) // Build handler chain handler := appHandler(mux, tmpl, a, sitemapPackagesHandler) - handler = http.TimeoutHandler(handler, 60*time.Second, "") + handler = timeoutBypass(handler, 60*time.Second) handler = Recoverer(handler, a.Logger) handler = sentryMiddleware.Handle(handler) handler = RealIP(handler) @@ -172,6 +172,16 @@ func (r *statusRecorder) WriteHeader(code int) { } } +func (r *statusRecorder) Flush() { + if f, ok := r.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +func (r *statusRecorder) Unwrap() http.ResponseWriter { + return r.ResponseWriter +} + func (r *statusRecorder) Write(b []byte) (int, error) { if !r.dispatched && r.code == http.StatusNotFound { return len(b), nil // swallow default 404 body @@ -222,6 +232,27 @@ func Recoverer(next http.Handler, logger *slog.Logger) http.Handler { }) } +// Paths that bypass the global http.TimeoutHandler because they are +// long-lived streaming connections (SSE). http.TimeoutHandler replaces the +// ResponseWriter with a buffering writer that doesn't implement +// http.Flusher, so these paths must be excluded before TimeoutHandler runs. +var noTimeoutPaths = map[string]bool{ + "/admin/logs/stream": true, +} + +// timeoutBypass applies http.TimeoutHandler to all requests except those +// whose path is in noTimeoutPaths. +func timeoutBypass(next http.Handler, dt time.Duration) http.Handler { + th := http.TimeoutHandler(next, dt, "") + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if noTimeoutPaths[r.URL.Path] { + next.ServeHTTP(w, r) + return + } + th.ServeHTTP(w, r) + }) +} + // RealIP sets r.RemoteAddr to the client IP from X-Forwarded-For or // X-Real-IP headers. Only enable behind a trusted reverse proxy. func RealIP(next http.Handler) http.Handler { diff --git a/internal/http/router_test.go b/internal/http/router_test.go index 15d5d21..b368eb8 100644 --- a/internal/http/router_test.go +++ b/internal/http/router_test.go @@ -1,12 +1,16 @@ package http import ( + "context" "log/slog" "net/http" "net/http/httptest" + "sync" "testing" + "time" "github.com/roots/wp-composer/internal/app" + "github.com/roots/wp-composer/internal/auth" "github.com/roots/wp-composer/internal/config" "github.com/roots/wp-composer/internal/db" "github.com/roots/wp-composer/internal/packagist" @@ -97,7 +101,7 @@ func TestRouter_UnmatchedRouteRendersTemplate(t *testing.T) { // An unmatched route should render the custom 404 template, not the // default "404 page not found" text from ServeMux. mux := http.NewServeMux() - mux.Handle("GET /health", routeMarker(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mux.Handle("GET /health", routeMarker(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte("ok")) }))) @@ -118,6 +122,62 @@ func TestRouter_UnmatchedRouteRendersTemplate(t *testing.T) { } } +func TestTimeoutBypass_AppliesTimeoutToNormalPaths(t *testing.T) { + slow := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(200 * time.Millisecond) + _, _ = w.Write([]byte("ok")) + }) + handler := timeoutBypass(slow, 50*time.Millisecond) + + req := httptest.NewRequest("GET", "/health", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusServiceUnavailable { + t.Errorf("GET /health (slow): got %d, want 503 (timeout)", w.Code) + } +} + +func TestTimeoutBypass_SkipsTimeoutForStreamingPaths(t *testing.T) { + var mu sync.Mutex + var flusherAvailable bool + + handler := timeoutBypass(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, ok := w.(http.Flusher) + mu.Lock() + flusherAvailable = ok + mu.Unlock() + _, _ = w.Write([]byte("streaming")) + }), 50*time.Millisecond) + + req := httptest.NewRequest("GET", "/admin/logs/stream", nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("GET /admin/logs/stream: got %d, want 200", w.Code) + } + mu.Lock() + if !flusherAvailable { + t.Error("GET /admin/logs/stream: http.Flusher not available (timeout handler was not bypassed)") + } + mu.Unlock() +} + +func TestStatusRecorder_ForwardsFlusher(t *testing.T) { + inner := httptest.NewRecorder() // implements http.Flusher + rec := &statusRecorder{ResponseWriter: inner, dispatched: true} + + // Assert via interface variable to test dynamic dispatch + var w http.ResponseWriter = rec + flusher, ok := w.(http.Flusher) + if !ok { + t.Fatal("statusRecorder does not implement http.Flusher") + } + // Should not panic + flusher.Flush() +} + func TestRouter_SitemapPackagesRoutes(t *testing.T) { a := newTestApp(t) handler := NewRouter(a) @@ -134,3 +194,88 @@ func TestRouter_SitemapPackagesRoutes(t *testing.T) { t.Error("GET /sitemap-packages-0.xml returned 404 — route not matched") } } + +// newTestAppWithAuth creates a test app with users/sessions tables and an +// admin user+session. Returns the app and a valid session cookie value. +func newTestAppWithAuth(t *testing.T) (*app.App, string) { + t.Helper() + database, err := db.Open(":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = database.Close() }) + + _, err = database.Exec(` + CREATE TABLE users ( + id INTEGER PRIMARY KEY, + email TEXT NOT NULL UNIQUE, + name TEXT NOT NULL, + password_hash TEXT NOT NULL, + is_admin INTEGER NOT NULL DEFAULT 0, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL + ); + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + expires_at TEXT NOT NULL, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL + ); + `) + if err != nil { + t.Fatal(err) + } + + ctx := context.Background() + user, err := auth.CreateUser(ctx, database, "test@example.com", "Test", "hash", true) + if err != nil { + t.Fatal(err) + } + session, err := auth.CreateSession(ctx, database, user.ID, 60) + if err != nil { + t.Fatal(err) + } + + a := &app.App{ + Config: &config.Config{Session: config.SessionConfig{LifetimeMinutes: 60}}, + DB: database, + Logger: slog.Default(), + Packagist: packagist.NewDownloadsCache(slog.Default()), + } + return a, session +} + +// TestRouter_LogStreamBypassesTimeout is an integration test that verifies +// /admin/logs/stream through the full NewRouter handler chain (RealIP → +// Sentry → Recoverer → timeoutBypass → appHandler → mux → StripPrefix → +// SessionAuth → RequireAdmin → handler) gets SSE headers and isn't cut off +// by http.TimeoutHandler. +func TestRouter_LogStreamBypassesTimeout(t *testing.T) { + a, session := newTestAppWithAuth(t) + handler := NewRouter(a) + + srv := httptest.NewServer(handler) + defer srv.Close() + + client := srv.Client() + client.Timeout = 2 * time.Second + + httpReq, _ := http.NewRequest("GET", srv.URL+"/admin/logs/stream?file=wpcomposer", nil) + httpReq.AddCookie(&http.Cookie{Name: "session", Value: session}) + + resp, err := client.Do(httpReq) + if err != nil { + // Client timeout is expected since the stream never ends and the log + // file doesn't exist. But we should get a response before that. + t.Fatalf("request failed: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("GET /admin/logs/stream: got %d, want 200", resp.StatusCode) + } + if ct := resp.Header.Get("Content-Type"); ct != "text/event-stream" { + t.Errorf("Content-Type: got %q, want text/event-stream", ct) + } +}