From 7439f471cedf35056d46a0a8606313c2d9f62060 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 16:46:11 +0200 Subject: [PATCH 1/4] fix(server): JSON 404/405 for unknown /api paths and wrong methods RegisterRoutes now ends with a PathPrefix("/api/") catch-all that answers 404/405 (writeError JSON) for anything none of the real /api/* routes matched. Before this, gorilla/mux let a path-match / method-mismatch (e.g. POST /api/packets) fall through past every registered route to the SPA catch-all in main.go, which answers any method with 200 index.html. Relates to #233, #223, #231 Co-Authored-By: Claude Sonnet 5 --- cmd/server/api_fallback.go | 82 +++++++++ cmd/server/api_fallback_test.go | 182 ++++++++++++++++++++ cmd/server/coverage_test.go | 10 +- cmd/server/openapi.go | 2 +- cmd/server/post_packets_removed_223_test.go | 23 ++- cmd/server/routes.go | 7 + docs/api-spec.md | 3 +- 7 files changed, 298 insertions(+), 11 deletions(-) create mode 100644 cmd/server/api_fallback.go create mode 100644 cmd/server/api_fallback_test.go diff --git a/cmd/server/api_fallback.go b/cmd/server/api_fallback.go new file mode 100644 index 000000000..3d058a3d8 --- /dev/null +++ b/cmd/server/api_fallback.go @@ -0,0 +1,82 @@ +package main + +import ( + "net/http" + "sort" + "strings" + + "github.com/gorilla/mux" +) + +// registerAPIFallback adds a catch-all for /api/* requests that don't match +// any registered route, so an unknown path or a known path called with the +// wrong method gets a JSON error instead of falling through to the SPA's +// index.html (#233). +// +// gorilla/mux tries routes in registration order; a route whose path +// matches but whose method doesn't is a "keep trying", not a final 405. +// Without this, that fall-through reaches the SPA PathPrefix("/") handler +// registered later in main.go, which matches any method and serves 200 +// index.html — e.g. POST /api/packets hitting the GET-only route. +// +// Called as the last statement of RegisterRoutes, so it sits after every +// real /api/* route (registered earlier in the same call) and before +// main.go registers /ws and the SPA catch-all (registered after +// RegisterRoutes returns). Every caller of RegisterRoutes — main.go and +// every test's setupTestServer — gets the fallback for free. +func registerAPIFallback(router *mux.Router) { + router.PathPrefix("/api/").HandlerFunc(apiFallbackHandler(router)) +} + +// apiFallbackHandler responds 405 with an Allow header if the request path +// matches a known /api route under a different method, otherwise 404. Both +// use the existing writeError JSON shape. +func apiFallbackHandler(router *mux.Router) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if allowed := allowedMethodsForPath(router, r); len(allowed) > 0 { + w.Header().Set("Allow", strings.Join(allowed, ", ")) + writeError(w, http.StatusMethodNotAllowed, "method not allowed") + return + } + writeError(w, http.StatusNotFound, "not found") + } +} + +// allowedMethodsForPath walks the router's registered /api/* routes and +// returns the sorted, deduplicated set of methods whose route would match +// r's path if r had been sent with that method. It reuses mux's own route +// matching (path templates, {params}, etc.) rather than reimplementing it — +// the same trick buildOpenAPISpec uses to list routes. +func allowedMethodsForPath(router *mux.Router, r *http.Request) []string { + seen := map[string]bool{} + router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { + path, err := route.GetPathTemplate() + if err != nil || !strings.HasPrefix(path, "/api/") { + return nil + } + methods, err := route.GetMethods() + if err != nil { + // Routes without .Methods() — this fallback itself — match any + // method, so they can never contribute an Allow entry. + return nil + } + for _, m := range methods { + if seen[m] { + continue + } + testReq := r.Clone(r.Context()) + testReq.Method = m + var match mux.RouteMatch + if route.Match(testReq, &match) { + seen[m] = true + } + } + return nil + }) + out := make([]string, 0, len(seen)) + for m := range seen { + out = append(out, m) + } + sort.Strings(out) + return out +} diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go new file mode 100644 index 000000000..8c8e27951 --- /dev/null +++ b/cmd/server/api_fallback_test.go @@ -0,0 +1,182 @@ +package main + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gorilla/mux" +) + +// #233: an unknown /api/* path, or a known one called with the wrong +// method, must get a JSON error, never the SPA's 200 index.html. These +// tests build the router the same two ways production code does: +// - setupTestServer: RegisterRoutes only (what most other _test.go files use) +// - productionStyleRouter below: RegisterRoutes + /ws + the SPA catch-all, +// matching main.go's actual composition order. +func productionStyleRouter(t *testing.T, srv *Server) *mux.Router { + t.Helper() + router := mux.NewRouter() + srv.RegisterRoutes(router) + router.HandleFunc("/ws", NewHub().ServeWS) + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "index.html"), []byte("SPA"), 0o644); err != nil { + t.Fatal(err) + } + router.PathPrefix("/").Handler(wsOrStatic(NewHub(), spaHandler(dir, http.FileServer(http.Dir(dir))))) + return router +} + +func TestAPIFallbackUnknownPathReturns404JSON(t *testing.T) { + _, router := setupTestServer(t) + req := httptest.NewRequest("GET", "/api/this-path-does-not-exist", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("GET /api/this-path-does-not-exist: want 404, got %d (body %q)", w.Code, w.Body.String()) + } + if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Fatalf("want application/json content-type, got %q (body %q)", ct, w.Body.String()) + } + var body map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("response is not JSON: %v (body %q)", err, w.Body.String()) + } + if body["error"] == "" { + t.Errorf("expected a non-empty \"error\" field, got %v", body) + } +} + +func TestAPIFallbackUnknownPathInProductionRouterReturns404JSON(t *testing.T) { + srv, _ := setupTestServer(t) + router := productionStyleRouter(t, srv) + + req := httptest.NewRequest("GET", "/api/this-path-does-not-exist", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("GET /api/this-path-does-not-exist: want 404, got %d (body %q)", w.Code, w.Body.String()) + } + if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Fatalf("want application/json content-type, got %q", ct) + } + if strings.Contains(strings.ToLower(w.Body.String()), " Date: Mon, 5 Oct 2026 18:36:17 +0000 Subject: [PATCH 2/4] test(server): pin HEAD, bare /api and exact Allow for the API fallback (#233) Review of PR #266 found that HEAD on every known /api route now answers 405, that bare /api still serves the SPA page, and that Allow was only checked with strings.Contains. - Walk the served OpenAPI list over a real listener: each GET route must answer HEAD with GET's status and headers and no body; a path without GET answers 405 with the exact Allow set. - GET/POST /api must be a JSON 404 on both router compositions. - /api-docs, /apifoo, /apiary, /apis/x and /api.json must still reach the SPA. - Allow is compared as an exact set, including in the #223 tests, and includes HEAD wherever GET is allowed. Relates to #233 Co-Authored-By: Claude Opus 5.5 --- cmd/server/api_fallback_test.go | 173 +++++++++++++++++++- cmd/server/post_packets_removed_223_test.go | 5 +- 2 files changed, 169 insertions(+), 9 deletions(-) diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go index 8c8e27951..6cb901ba4 100644 --- a/cmd/server/api_fallback_test.go +++ b/cmd/server/api_fallback_test.go @@ -2,12 +2,15 @@ package main import ( "encoding/json" + "io" "net/http" "net/http/httptest" "os" "path/filepath" + "sort" "strings" "testing" + "time" "github.com/gorilla/mux" ) @@ -84,9 +87,7 @@ func TestAPIFallbackPostPacketsReturns405NotSPA(t *testing.T) { if w.Code != http.StatusMethodNotAllowed { t.Fatalf("POST /api/packets: want 405, got %d (body %q)", w.Code, w.Body.String()) } - if allow := w.Header().Get("Allow"); !strings.Contains(allow, "GET") { - t.Errorf("want Allow header containing GET, got %q", allow) - } + assertAllowSet(t, w.Header().Get("Allow"), "GET", "HEAD") if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { t.Fatalf("want application/json content-type, got %q", ct) } @@ -105,9 +106,7 @@ func TestAPIFallbackKnownPathWrongMethodReturns405(t *testing.T) { if w.Code != http.StatusMethodNotAllowed { t.Fatalf("DELETE /api/stats: want 405, got %d (body %q)", w.Code, w.Body.String()) } - if allow := w.Header().Get("Allow"); !strings.Contains(allow, "GET") { - t.Errorf("want Allow header containing GET, got %q", allow) - } + assertAllowSet(t, w.Header().Get("Allow"), "GET", "HEAD") } // Everything else must be unaffected: every route documented by the served @@ -180,3 +179,165 @@ func TestAPIFallbackDoesNotAffectWebSocketOrSPA(t *testing.T) { t.Errorf("/#/packets: want 200 SPA page, got %d %q", w.Code, w.Body.String()) } } + +// assertAllowSet compares an Allow header to an exact method set, ignoring +// order and whitespace, so "GETX" or a missing/extra method fails. +func assertAllowSet(t *testing.T, allow string, want ...string) { + t.Helper() + var got []string + for _, m := range strings.Split(allow, ",") { + if m = strings.TrimSpace(m); m != "" { + got = append(got, m) + } + } + sort.Strings(got) + want = append([]string(nil), want...) + sort.Strings(want) + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("Allow: got %q (set %v), want exactly %v", allow, got, want) + } +} + +// A geo-filter path has GET and PUT routes; the Allow set must compose both +// plus HEAD (served for every GET route). +func TestAPIFallbackAllowComposesMethodsAcrossRoutes(t *testing.T) { + _, router := setupTestServer(t) + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("POST", "/api/config/geo-filter", nil)) + if w.Code != http.StatusMethodNotAllowed { + t.Fatalf("POST /api/config/geo-filter: want 405, got %d (body %q)", w.Code, w.Body.String()) + } + assertAllowSet(t, w.Header().Get("Allow"), "GET", "HEAD", "PUT") +} + +// openAPIOperations returns the served spec's operations as concrete +// request paths ({param} -> "x"), keyed by path, with upper-case methods. +func openAPIOperations(t *testing.T, router http.Handler) map[string][]string { + t.Helper() + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/api/spec", nil)) + if w.Code != http.StatusOK { + t.Fatalf("GET /api/spec: want 200, got %d", w.Code) + } + var spec struct { + Paths map[string]map[string]json.RawMessage `json:"paths"` + } + if err := json.Unmarshal(w.Body.Bytes(), &spec); err != nil { + t.Fatal(err) + } + out := map[string][]string{} + for path, ops := range spec.Paths { + testPath := path + for strings.Contains(testPath, "{") { + start := strings.Index(testPath, "{") + end := strings.Index(testPath[start:], "}") + start + testPath = testPath[:start] + "x" + testPath[end+1:] + } + for method := range ops { + out[testPath] = append(out[testPath], strings.ToUpper(method)) + } + } + if len(out) < 20 { + t.Fatalf("expected at least 20 documented paths, got %d", len(out)) + } + return out +} + +// HEAD on a known /api route must keep succeeding (it was 200 on master, +// via the SPA). Every documented GET route answers HEAD with GET's status +// and headers; net/http drops the body. A documented path without GET +// answers HEAD with 405 and the exact Allow set. Runs over a real listener +// so the HEAD body suppression is net/http's, as in production. +func TestAPIFallbackHeadMatchesGetForEveryOpenAPIRoute(t *testing.T) { + srv, _ := setupTestServer(t) + router := productionStyleRouter(t, srv) + ts := httptest.NewServer(router) + defer ts.Close() + client := &http.Client{Timeout: 30 * time.Second} + + do := func(method, path string) *http.Response { + t.Helper() + req, err := http.NewRequest(method, ts.URL+path, nil) + if err != nil { + t.Fatal(err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, path, err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + if method == "HEAD" && len(body) != 0 { + t.Errorf("HEAD %s: got a %d-byte body", path, len(body)) + } + return resp + } + + ops := openAPIOperations(t, router) + checked := 0 + for path, methods := range ops { + hasGet := false + for _, m := range methods { + if m == "GET" { + hasGet = true + } + } + head := do("HEAD", path) + if !hasGet { + if head.StatusCode != http.StatusMethodNotAllowed { + t.Errorf("HEAD %s (no GET route): want 405, got %d", path, head.StatusCode) + } + assertAllowSet(t, head.Header.Get("Allow"), methods...) + continue + } + get := do("GET", path) + if head.StatusCode == http.StatusMethodNotAllowed { + t.Errorf("HEAD %s: got 405 (Allow %q), want GET's status %d", path, head.Header.Get("Allow"), get.StatusCode) + continue + } + if head.StatusCode != get.StatusCode { + t.Errorf("HEAD %s: status %d, GET status %d", path, head.StatusCode, get.StatusCode) + } + for _, h := range []string{"Content-Type", "Cache-Control", "Allow"} { + if hv, gv := head.Header.Get(h), get.Header.Get(h); hv != gv { + t.Errorf("HEAD %s: %s %q, GET %s %q", path, h, hv, h, gv) + } + } + checked++ + } + if checked < 20 { + t.Fatalf("only checked %d GET routes with HEAD, expected at least 20", checked) + } +} + +// Bare /api (no trailing slash) is an API path too: JSON 404, not the SPA. +func TestAPIFallbackBareAPIReturns404JSON(t *testing.T) { + srv, bare := setupTestServer(t) + for name, router := range map[string]http.Handler{"api router": bare, "production router": productionStyleRouter(t, srv)} { + for _, method := range []string{"GET", "POST"} { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(method, "/api", nil)) + if w.Code != http.StatusNotFound { + t.Errorf("%s: %s /api: want 404, got %d (body %q)", name, method, w.Code, w.Body.String()) + continue + } + if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Errorf("%s: %s /api: want application/json, got %q (body %q)", name, method, ct, w.Body.String()) + } + } + } +} + +// Paths that only share the "/api" string prefix are not API paths and +// must still reach the SPA. +func TestAPIFallbackPrefixSiblingsStillReachSPA(t *testing.T) { + srv, _ := setupTestServer(t) + router := productionStyleRouter(t, srv) + for _, path := range []string{"/api-docs", "/apifoo", "/apiary", "/apis/x", "/api.json"} { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", path, nil)) + if w.Code != http.StatusOK || w.Body.String() != "SPA" { + t.Errorf("GET %s: want 200 SPA page, got %d %q", path, w.Code, w.Body.String()) + } + } +} diff --git a/cmd/server/post_packets_removed_223_test.go b/cmd/server/post_packets_removed_223_test.go index d2e899fe2..36cf3fe11 100644 --- a/cmd/server/post_packets_removed_223_test.go +++ b/cmd/server/post_packets_removed_223_test.go @@ -90,6 +90,7 @@ func TestPostPacketsRemovedReturns405OnReadOnlyDB(t *testing.T) { if w.Code != http.StatusMethodNotAllowed { t.Fatalf("POST /api/packets: want 405, got %d (body: %q)", w.Code, w.Body.String()) } + assertAllowSet(t, w.Header().Get("Allow"), "GET", "HEAD") if body := strings.ToLower(w.Body.String()); strings.Contains(body, "sqlite") || strings.Contains(body, "readonly") || strings.Contains(body, "insert") { t.Errorf("response leaks database error text: %q", w.Body.String()) } @@ -162,9 +163,7 @@ func TestPostPacketsRemovedFallsThroughToSPAInProductionRouter(t *testing.T) { t.Fatalf("POST /api/packets: want 405, got %d %q %q", w.Code, w.Header().Get("Content-Type"), w.Body.String()) } - if allow := w.Header().Get("Allow"); !strings.Contains(allow, "GET") { - t.Errorf("want Allow header containing GET, got %q", allow) - } + assertAllowSet(t, w.Header().Get("Allow"), "GET", "HEAD") if !strings.HasPrefix(w.Header().Get("Content-Type"), "application/json") { t.Errorf("want application/json content-type, got %q", w.Header().Get("Content-Type")) } From c2f6c9b7a69c26a78a46f00a81e73c0b985f497d Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 18:39:24 +0000 Subject: [PATCH 3/4] fix(server): serve HEAD like GET and 404 bare /api in the API fallback (#233) gorilla/mux's .Methods("GET") does not match HEAD, so with the JSON fallback every HEAD on a known /api route became 405 (master answered 200 via the SPA). The fallback now looks up the route a GET to the same URL would reach and runs its handler; net/http drops the body because the connection's request is HEAD. It calls the route's own handler rather than router.ServeHTTP, so the router middleware runs once and the fallback cannot re-enter itself. Allow lists HEAD wherever GET is allowed. Bare /api gets its own exact-path fallback route, so it is a JSON 404 while /api-docs, /apifoo and other prefix siblings still reach the SPA. The OpenAPI walk now compares HEAD with the GET just before and just after it (analytics endpoints answer 202 until their background compute finishes), takes "no GET route" from GET's own 405 rather than from the spec (GET /api/packets/observations is served by /api/packets/{hash}), and checks the HEAD body over a raw connection, since http.Client never returns one. Relates to #233 Co-Authored-By: Claude Opus 5.5 --- cmd/server/api_fallback.go | 47 +++++++++++++++++++-- cmd/server/api_fallback_test.go | 75 ++++++++++++++++++++++----------- cmd/server/openapi.go | 2 +- docs/api-spec.md | 4 +- 4 files changed, 97 insertions(+), 31 deletions(-) diff --git a/cmd/server/api_fallback.go b/cmd/server/api_fallback.go index 3d058a3d8..76d5c63c3 100644 --- a/cmd/server/api_fallback.go +++ b/cmd/server/api_fallback.go @@ -24,15 +24,27 @@ import ( // main.go registers /ws and the SPA catch-all (registered after // RegisterRoutes returns). Every caller of RegisterRoutes — main.go and // every test's setupTestServer — gets the fallback for free. +// +// Bare /api is an API path too, so it gets its own exact-path route; +// PathPrefix("/api") would also swallow SPA paths like /api-docs. func registerAPIFallback(router *mux.Router) { - router.PathPrefix("/api/").HandlerFunc(apiFallbackHandler(router)) + h := apiFallbackHandler(router) + router.Path("/api").HandlerFunc(h) + router.PathPrefix("/api/").HandlerFunc(h) } -// apiFallbackHandler responds 405 with an Allow header if the request path -// matches a known /api route under a different method, otherwise 404. Both -// use the existing writeError JSON shape. +// apiFallbackHandler serves HEAD through the GET route for the same path, +// otherwise responds 405 with an Allow header if the request path matches a +// known /api route under a different method, otherwise 404. Both errors use +// the existing writeError JSON shape. func apiFallbackHandler(router *mux.Router) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodHead { + if h, getReq := getRouteHandler(router, r); h != nil { + h.ServeHTTP(w, getReq) + return + } + } if allowed := allowedMethodsForPath(router, r); len(allowed) > 0 { w.Header().Set("Allow", strings.Join(allowed, ", ")) writeError(w, http.StatusMethodNotAllowed, "method not allowed") @@ -73,6 +85,10 @@ func allowedMethodsForPath(router *mux.Router, r *http.Request) []string { } return nil }) + if seen[http.MethodGet] { + // Every GET route also answers HEAD, via apiFallbackHandler. + seen[http.MethodHead] = true + } out := make([]string, 0, len(seen)) for m := range seen { out = append(out, m) @@ -80,3 +96,26 @@ func allowedMethodsForPath(router *mux.Router, r *http.Request) []string { sort.Strings(out) return out } + +// getRouteHandler returns the handler of the route a GET to r's URL would +// reach, and that GET request with the route's path variables set, or nil if +// only this fallback matches. gorilla/mux's .Methods("GET") does not match +// HEAD, so HEAD on a known route lands here; RFC 9110 §9.3.2 wants it served +// like GET. The handler runs as GET, and net/http drops the body because the +// connection's request is HEAD. The router middleware has already run around +// this fallback, so the route's own handler is called directly, not +// match.Handler or router.ServeHTTP: that would run the middleware twice and +// could re-enter this fallback. +func getRouteHandler(router *mux.Router, r *http.Request) (http.Handler, *http.Request) { + getReq := r.Clone(r.Context()) + getReq.Method = http.MethodGet + var match mux.RouteMatch + if !router.Match(getReq, &match) || match.MatchErr != nil { + return nil, nil + } + if _, err := match.Route.GetMethods(); err != nil { + // A route without .Methods(): this fallback, so no GET route exists. + return nil, nil + } + return match.Route.GetHandler(), mux.SetURLVars(getReq, match.Vars) +} diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go index 6cb901ba4..f936296fa 100644 --- a/cmd/server/api_fallback_test.go +++ b/cmd/server/api_fallback_test.go @@ -1,8 +1,11 @@ package main import ( + "bytes" "encoding/json" + "fmt" "io" + "net" "net/http" "net/http/httptest" "os" @@ -244,10 +247,12 @@ func openAPIOperations(t *testing.T, router http.Handler) map[string][]string { } // HEAD on a known /api route must keep succeeding (it was 200 on master, -// via the SPA). Every documented GET route answers HEAD with GET's status -// and headers; net/http drops the body. A documented path without GET -// answers HEAD with 405 and the exact Allow set. Runs over a real listener -// so the HEAD body suppression is net/http's, as in production. +// via the SPA). Every documented path answers HEAD with the status and +// headers GET gets, and no body; where GET itself is 405 (no GET route for +// that URL), HEAD is 405 with the exact Allow set. Some analytics endpoints +// answer 202 until a background compute finishes, so HEAD is compared with +// the GET just before and just after it. Runs over a real listener so the +// HEAD body suppression is net/http's, as in production. func TestAPIFallbackHeadMatchesGetForEveryOpenAPIRoute(t *testing.T) { srv, _ := setupTestServer(t) router := productionStyleRouter(t, srv) @@ -265,48 +270,68 @@ func TestAPIFallbackHeadMatchesGetForEveryOpenAPIRoute(t *testing.T) { if err != nil { t.Fatalf("%s %s: %v", method, path, err) } - body, _ := io.ReadAll(resp.Body) + io.Copy(io.Discard, resp.Body) resp.Body.Close() - if method == "HEAD" && len(body) != 0 { - t.Errorf("HEAD %s: got a %d-byte body", path, len(body)) - } return resp } + // http.Client never surfaces a HEAD body, so read the raw bytes after + // the header block to prove the server sends none. + rawHeadBody := func(path string) int { + t.Helper() + conn, err := net.DialTimeout("tcp", ts.Listener.Addr().String(), 5*time.Second) + if err != nil { + t.Fatal(err) + } + defer conn.Close() + conn.SetDeadline(time.Now().Add(30 * time.Second)) + fmt.Fprintf(conn, "HEAD %s HTTP/1.0\r\nHost: test\r\n\r\n", path) + raw, err := io.ReadAll(conn) + if err != nil { + t.Fatalf("HEAD %s (raw): %v", path, err) + } + i := bytes.Index(raw, []byte("\r\n\r\n")) + if i < 0 { + t.Fatalf("HEAD %s (raw): no header terminator in %q", path, raw) + } + return len(raw) - (i + 4) + } ops := openAPIOperations(t, router) - checked := 0 + served := 0 for path, methods := range ops { - hasGet := false - for _, m := range methods { - if m == "GET" { - hasGet = true - } - } + before := do("GET", path) head := do("HEAD", path) - if !hasGet { + after := do("GET", path) + + if before.StatusCode == http.StatusMethodNotAllowed { if head.StatusCode != http.StatusMethodNotAllowed { - t.Errorf("HEAD %s (no GET route): want 405, got %d", path, head.StatusCode) + t.Errorf("HEAD %s (GET is 405): want 405, got %d", path, head.StatusCode) } assertAllowSet(t, head.Header.Get("Allow"), methods...) continue } - get := do("GET", path) - if head.StatusCode == http.StatusMethodNotAllowed { - t.Errorf("HEAD %s: got 405 (Allow %q), want GET's status %d", path, head.Header.Get("Allow"), get.StatusCode) - continue + get := before + if head.StatusCode != before.StatusCode { + get = after } if head.StatusCode != get.StatusCode { - t.Errorf("HEAD %s: status %d, GET status %d", path, head.StatusCode, get.StatusCode) + t.Errorf("HEAD %s: status %d (Allow %q), GET status %d then %d", + path, head.StatusCode, head.Header.Get("Allow"), before.StatusCode, after.StatusCode) + continue } for _, h := range []string{"Content-Type", "Cache-Control", "Allow"} { if hv, gv := head.Header.Get(h), get.Header.Get(h); hv != gv { t.Errorf("HEAD %s: %s %q, GET %s %q", path, h, hv, h, gv) } } - checked++ + if n := rawHeadBody(path); n != 0 { + t.Errorf("HEAD %s: server sent a %d-byte body", path, n) + } + served++ } - if checked < 20 { - t.Fatalf("only checked %d GET routes with HEAD, expected at least 20", checked) + t.Logf("HEAD served like GET on %d of %d documented paths; the rest have no GET route (405)", served, len(ops)) + if served < 20 { + t.Fatalf("only %d documented paths served HEAD like GET, expected at least 20", served) } } diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index 8e72f7845..d379bc60a 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -970,7 +970,7 @@ func buildOpenAPISpec(router *mux.Router, version string) map[string]interface{} "openapi": "3.0.3", "info": map[string]interface{}{ "title": "CoreScope API", - "description": "MeshCore network analyzer — packet capture, node tracking, and mesh analytics. An unrecognized /api/* path returns 404; a documented path called with an unsupported method returns 405 with an Allow header. Both are JSON (#233).", + "description": "MeshCore network analyzer — packet capture, node tracking, and mesh analytics. An unrecognized /api or /api/* path returns 404; a documented path called with an unsupported method returns 405 with an Allow header. Both are JSON (#233). HEAD is served on every GET path.", "version": version, "license": map[string]interface{}{ "name": "MIT", diff --git a/docs/api-spec.md b/docs/api-spec.md index a0883cc55..481d3b72b 100644 --- a/docs/api-spec.md +++ b/docs/api-spec.md @@ -99,9 +99,11 @@ They return `total` (the unfiltered/filtered count before pagination). ``` - `400` — Bad request (missing/invalid params) -- `404` — Resource not found, or an unrecognized `/api/*` path +- `404` — Resource not found, or an unrecognized `/api` or `/api/*` path - `405` — A known `/api/*` path called with an unsupported method; the response carries an `Allow` header listing the methods that path does support +`HEAD` is accepted on every path that accepts `GET` and returns the same status and headers without a body. + --- ## GET /api/stats From 74dca70bd283d351e17484952d08faa8547405d2 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 18:41:15 +0000 Subject: [PATCH 4/4] fix(server): fail loudly when an /api route is shadowed by the fallback (#233) The JSON fallback is the last /api route RegisterRoutes adds, so an /api route registered on the same router afterwards (for example in main.go) is never reached, and nothing failed. - main.go's router composition (RegisterRoutes, /ws, the SPA catch-all) moves into newHTTPRouter, which main and the #233 tests now share. - The fallback routes are named, and apiRoutesShadowedByFallback lists every /api route registered after them. - TestProductionRouterHasNoShadowedAPIRoutes checks newHTTPRouter: the fallback is the last /api route and nothing is shadowed. TestAPIRoutesShadowedByFallbackReportsLateRoutes checks the guard. - main refuses to start if any /api route is shadowed, which also covers routes added in main after newHTTPRouter returns. Relates to #233 Co-Authored-By: Claude Opus 5.5 --- cmd/server/api_fallback.go | 36 ++++++++++++++++++-- cmd/server/api_fallback_test.go | 59 +++++++++++++++++++++++++++++---- cmd/server/main.go | 55 +++++++++++++++++++----------- 3 files changed, 122 insertions(+), 28 deletions(-) diff --git a/cmd/server/api_fallback.go b/cmd/server/api_fallback.go index 76d5c63c3..f2f67d3f8 100644 --- a/cmd/server/api_fallback.go +++ b/cmd/server/api_fallback.go @@ -29,10 +29,17 @@ import ( // PathPrefix("/api") would also swallow SPA paths like /api-docs. func registerAPIFallback(router *mux.Router) { h := apiFallbackHandler(router) - router.Path("/api").HandlerFunc(h) - router.PathPrefix("/api/").HandlerFunc(h) + router.Path("/api").HandlerFunc(h).Name(apiFallbackRootRouteName) + router.PathPrefix("/api/").HandlerFunc(h).Name(apiFallbackRouteName) } +// Route names of the two fallback routes, so apiRoutesShadowedByFallback +// can find them. +const ( + apiFallbackRootRouteName = "api-fallback-root" + apiFallbackRouteName = "api-fallback" +) + // apiFallbackHandler serves HEAD through the GET route for the same path, // otherwise responds 405 with an Allow header if the request path matches a // known /api route under a different method, otherwise 404. Both errors use @@ -119,3 +126,28 @@ func getRouteHandler(router *mux.Router, r *http.Request) (http.Handler, *http.R } return match.Route.GetHandler(), mux.SetURLVars(getReq, match.Vars) } + +// apiRoutesShadowedByFallback returns the path template of every /api route +// registered after the API fallback. mux tries routes in registration order +// and the fallback matches every method and path under /api, so such a +// route is never reached. main checks the production router at startup; +// TestProductionRouterHasNoShadowedAPIRoutes checks newHTTPRouter. +func apiRoutesShadowedByFallback(router *mux.Router) []string { + var shadowed []string + fallbackSeen := false + router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { + switch route.GetName() { + case apiFallbackRootRouteName, apiFallbackRouteName: + fallbackSeen = true + return nil + } + if !fallbackSeen { + return nil + } + if tmpl, err := route.GetPathTemplate(); err == nil && (tmpl == "/api" || strings.HasPrefix(tmpl, "/api/")) { + shadowed = append(shadowed, tmpl) + } + return nil + }) + return shadowed +} diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go index f936296fa..1c847fb15 100644 --- a/cmd/server/api_fallback_test.go +++ b/cmd/server/api_fallback_test.go @@ -22,19 +22,15 @@ import ( // method, must get a JSON error, never the SPA's 200 index.html. These // tests build the router the same two ways production code does: // - setupTestServer: RegisterRoutes only (what most other _test.go files use) -// - productionStyleRouter below: RegisterRoutes + /ws + the SPA catch-all, -// matching main.go's actual composition order. +// - productionStyleRouter below: newHTTPRouter, the RegisterRoutes + /ws + +// SPA catch-all composition main.go serves, with a stub index.html. func productionStyleRouter(t *testing.T, srv *Server) *mux.Router { t.Helper() - router := mux.NewRouter() - srv.RegisterRoutes(router) - router.HandleFunc("/ws", NewHub().ServeWS) dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, "index.html"), []byte("SPA"), 0o644); err != nil { t.Fatal(err) } - router.PathPrefix("/").Handler(wsOrStatic(NewHub(), spaHandler(dir, http.FileServer(http.Dir(dir))))) - return router + return newHTTPRouter(srv, NewHub(), dir) } func TestAPIFallbackUnknownPathReturns404JSON(t *testing.T) { @@ -366,3 +362,52 @@ func TestAPIFallbackPrefixSiblingsStillReachSPA(t *testing.T) { } } } + +// The router main.go serves must not register any /api route after the +// fallback: mux tries routes in order and the fallback matches every method +// and path under /api, so such a route would be dead with no error. +func TestProductionRouterHasNoShadowedAPIRoutes(t *testing.T) { + srv, _ := setupTestServer(t) + router := productionStyleRouter(t, srv) + + // The guard is only meaningful if the fallback is there and is the last + // /api route. + var last string + router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { + if tmpl, err := route.GetPathTemplate(); err == nil && (tmpl == "/api" || strings.HasPrefix(tmpl, "/api/")) { + last = route.GetName() + } + return nil + }) + if last != apiFallbackRouteName { + t.Fatalf("last /api route in the production router is %q, want the fallback %q", last, apiFallbackRouteName) + } + if shadowed := apiRoutesShadowedByFallback(router); len(shadowed) > 0 { + t.Errorf("/api routes registered after the fallback are unreachable: %v", shadowed) + } +} + +// The guard itself: a late /api route is reported and really is dead; +// a late non-/api route that merely starts with "api" is not reported. +func TestAPIRoutesShadowedByFallbackReportsLateRoutes(t *testing.T) { + srv, _ := setupTestServer(t) + router := mux.NewRouter() + srv.RegisterRoutes(router) + late := func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("late")) } + router.HandleFunc("/api/late", late).Methods("GET") + router.HandleFunc("/api/late/{id}", late) + router.HandleFunc("/apiary-late", late) + + got := apiRoutesShadowedByFallback(router) + if strings.Join(got, " ") != "/api/late /api/late/{id}" { + t.Errorf("shadowed routes: got %v, want [/api/late /api/late/{id}]", got) + } + + // The fallback answers instead of the late handler (with a 405 that + // names GET, since it sees the late route's method). + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/api/late", nil)) + if w.Code == http.StatusOK || w.Body.String() == "late" { + t.Errorf("GET /api/late: the late handler ran (%d %q); the fallback should shadow it", w.Code, w.Body.String()) + } +} diff --git a/cmd/server/main.go b/cmd/server/main.go index 14221cdc2..c0de39b89 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -365,25 +365,7 @@ func main() { srv := NewServer(database, cfg, hub) srv.configDir = configDir srv.store = store - router := mux.NewRouter() - srv.RegisterRoutes(router) - - // WebSocket endpoint - router.HandleFunc("/ws", hub.ServeWS) - - // Static files + SPA fallback - absPublic, _ := filepath.Abs(publicDir) - if _, err := os.Stat(absPublic); err == nil { - fs := http.FileServer(http.Dir(absPublic)) - router.PathPrefix("/").Handler(wsOrStatic(hub, spaHandler(absPublic, fs))) - log.Printf("[static] serving %s", absPublic) - } else { - log.Printf("[static] directory %s not found — API-only mode", absPublic) - router.PathPrefix("/").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "text/html") - w.Write([]byte(`

CoreScope

Frontend not found. API available at /api/

`)) - }) - } + router := newHTTPRouter(srv, hub, publicDir) // Start SQLite poller for WebSocket broadcast poller := NewPoller(database, hub, time.Duration(pollMs)*time.Millisecond) @@ -526,6 +508,12 @@ func main() { _ = cfg.IncrementalVacuumPages() // kept reachable for config validation; not used here _ = cfg.NeighborMaxAgeDays() // ditto — owned by ingestor now + // Every route is registered by now. An /api route added after the + // API fallback would never be reached (#233). + if shadowed := apiRoutesShadowedByFallback(router); len(shadowed) > 0 { + log.Fatalf("[server] /api routes registered after the API fallback are unreachable: %v (register them in RegisterRoutes)", shadowed) + } + // Graceful shutdown var handler http.Handler = router if cfg.GZipEnabled() { @@ -648,6 +636,35 @@ func main() { } } +// newHTTPRouter builds the production router: the API routes (ending in the +// /api fallback, see registerAPIFallback), the WebSocket endpoint and the +// static/SPA catch-all, in that order. Add new /api routes inside +// RegisterRoutes: one added to this router afterwards is shadowed by the +// fallback, which apiRoutesShadowedByFallback reports, at startup in main +// and in TestProductionRouterHasNoShadowedAPIRoutes. +func newHTTPRouter(srv *Server, hub *Hub, publicDir string) *mux.Router { + router := mux.NewRouter() + srv.RegisterRoutes(router) + + // WebSocket endpoint + router.HandleFunc("/ws", hub.ServeWS) + + // Static files + SPA fallback + absPublic, _ := filepath.Abs(publicDir) + if _, err := os.Stat(absPublic); err == nil { + fs := http.FileServer(http.Dir(absPublic)) + router.PathPrefix("/").Handler(wsOrStatic(hub, spaHandler(absPublic, fs))) + log.Printf("[static] serving %s", absPublic) + } else { + log.Printf("[static] directory %s not found — API-only mode", absPublic) + router.PathPrefix("/").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.Write([]byte(`

CoreScope

Frontend not found. API available at /api/

`)) + }) + } + return router +} + // spaHandler serves static files, falling back to index.html for SPA routes. // It reads index.html once at creation time and replaces the __BUST__ placeholder // with a Unix timestamp so browsers fetch fresh JS/CSS after each server restart.