From 9afe8323a293bb0525d021231737e45b2d6fb807 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 15:04:25 +0200 Subject: [PATCH 1/2] fix(server): API-only banner names /api/spec, bare /api in 405 Allow (#300) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to #285 (#281): F1: the API-only banner (newHTTPRouter's missing-public-dir branch) named only /api/docs, a Swagger UI shell that loads CSS/JS from an external CDN (openapi.go) and renders blank with no outbound egress — the only mode that shows the banner. Now names /api/spec (self-contained OpenAPI JSON, served in-process) as the offline-safe pointer, keeping /api/docs for interactive use. F2: TestAPIOnlyBannerPointsToExistingEndpoint now extracts every /api path the banner actually advertises and GETs each (asserting 200), instead of checking a literal and fetching that same literal, so a banner that mentions a working path in passing while pointing somewhere broken can no longer pass. F3: allowedMethodsForPath filtered on strings.HasPrefix(path, "/api/"), which excludes exactly /api, so a wrong method on a real route at bare /api would answer 404 (no Allow) instead of 405 — the shadow check already counts bare /api, so the two disagreed. Added the path == "/api" case. Latent today (no real method-bearing route sits at bare /api in production; only the fallback, which carries no .Methods()), so production stays 404 there — pinned by a test. Tests (red before, green after; one mutant each): - TestAPIOnlyBannerNamesOfflineSpecEndpoint (F1) - TestAPIOnlyBannerPointsToExistingEndpoint, now strict (F2) - TestAllowedMethodsForBareAPIRoute + TestBareAPIWithoutRealRouteIs404 (F3) cmd/server stays read-only (no SQL added); no new map[string]interface{} outside tests; no frontend files touched; fork-guards unchanged. Co-Authored-By: Claude Opus 4.8 --- cmd/server/api_fallback.go | 6 +- cmd/server/api_fallback_test.go | 104 +++++++++++++++++++++++++++++--- cmd/server/main.go | 2 +- 3 files changed, 100 insertions(+), 12 deletions(-) diff --git a/cmd/server/api_fallback.go b/cmd/server/api_fallback.go index f2f67d3f8..2473a8cd8 100644 --- a/cmd/server/api_fallback.go +++ b/cmd/server/api_fallback.go @@ -70,7 +70,11 @@ 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/") { + // Bare /api is an API path too (registerAPIFallback gives it its own + // route); the shadow check already counts it, so the Allow computation + // must too, or a wrong method on a real /api route would 404 with no + // Allow instead of 405. + if err != nil || (path != "/api" && !strings.HasPrefix(path, "/api/")) { return nil } methods, err := route.GetMethods() diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go index 68c0d6163..45a6e40c0 100644 --- a/cmd/server/api_fallback_test.go +++ b/cmd/server/api_fallback_test.go @@ -10,6 +10,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "regexp" "sort" "strings" "testing" @@ -456,24 +457,107 @@ func TestGetRouteHandlerSelfDispatchGuardReturnsNilForUnknownPath(t *testing.T) } } -// #281 N4: the API-only banner (served by newHTTPRouter when the static -// directory doesn't exist) must point at an endpoint that actually -// resolves. It used to say "/api/", which #233 turned into a JSON 404. -func TestAPIOnlyBannerPointsToExistingEndpoint(t *testing.T) { +// apiOnlyBannerBody builds the production router with a missing public dir +// (the only mode that renders the API-only banner) and returns the banner +// body served for GET /, plus the router itself. +func apiOnlyBannerBody(t *testing.T) (*mux.Router, string) { + t.Helper() srv, _ := setupTestServer(t) missingDir := filepath.Join(t.TempDir(), "does-not-exist") router := newHTTPRouter(srv, NewHub(), missingDir) w := httptest.NewRecorder() router.ServeHTTP(w, httptest.NewRequest("GET", "/", nil)) - body := w.Body.String() - if !strings.Contains(body, "/api/docs") { - t.Fatalf("API-only banner does not mention /api/docs: %q", body) + return router, w.Body.String() +} + +// bannerAPIPath matches an /api... path advertised in the banner text. +var bannerAPIPath = regexp.MustCompile(`/api[A-Za-z0-9/_-]*`) + +// #300 F1: the API-only banner must name an endpoint that works with no +// outbound internet. /api/docs is a Swagger UI shell whose CSS and JS load +// only from an external CDN (openapi.go), so an API-only deployment without +// egress renders it blank. /api/spec is the raw OpenAPI JSON, served +// in-process, so it is the offline-safe pointer the banner must name. +func TestAPIOnlyBannerNamesOfflineSpecEndpoint(t *testing.T) { + _, body := apiOnlyBannerBody(t) + if !strings.Contains(body, "/api/spec") { + t.Fatalf("API-only banner does not name the offline-safe /api/spec: %q", body) + } +} + +// #281 N4 / #300 F2: the API-only banner (served by newHTTPRouter when the +// static directory doesn't exist) must point at endpoints that actually +// resolve. It used to say "/api/", which #233 turned into a JSON 404. +// +// F2 makes the test strict: instead of checking a literal string and then +// GETting that same literal, it extracts every /api path the banner actually +// advertises and GETs each one, asserting 200. A banner that only mentions a +// working path in passing while pointing somewhere broken can no longer pass. +func TestAPIOnlyBannerPointsToExistingEndpoint(t *testing.T) { + router, body := apiOnlyBannerBody(t) + + paths := bannerAPIPath.FindAllString(body, -1) + if len(paths) == 0 { + t.Fatalf("API-only banner advertises no /api path: %q", body) + } + for _, p := range paths { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", p, nil)) + if w.Code != http.StatusOK { + t.Fatalf("banner advertises %q, but GET %q returned %d (want 200); banner=%q", p, p, w.Code, body) + } + } +} + +// #300 F3: allowedMethodsForPath must also cover a method-bearing route +// registered at exactly "/api", not only under the "/api/" prefix, so a +// wrong method on bare /api answers 405 + Allow the same way it does one +// level down. Latent today: the production router registers no real +// method-bearing route at bare /api (only the fallback, which carries no +// .Methods() and so contributes nothing), so this builds a router with a +// POST-only /api route ahead of the fallback to exercise the path. +func TestAllowedMethodsForBareAPIRoute(t *testing.T) { + router := mux.NewRouter() + router.HandleFunc("/api", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTeapot) + }).Methods("POST") + registerAPIFallback(router) + + // Wrong method on bare /api: 405 naming the real route's method. + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/api", nil)) + if w.Code != http.StatusMethodNotAllowed { + t.Fatalf("GET /api with a POST-only route: want 405, got %d", w.Code) + } + if allow := w.Header().Get("Allow"); allow != "POST" { + t.Fatalf("GET /api: want Allow: POST, got %q", allow) } + // The real route is still reachable under its own method. w2 := httptest.NewRecorder() - router.ServeHTTP(w2, httptest.NewRequest("GET", "/api/docs", nil)) - if w2.Code != http.StatusOK { - t.Fatalf("GET /api/docs: want 200, got %d", w2.Code) + router.ServeHTTP(w2, httptest.NewRequest("POST", "/api", nil)) + if w2.Code != http.StatusTeapot { + t.Fatalf("POST /api: want 418 (real route reached), got %d", w2.Code) + } +} + +// #300 F3 (no-route branch): with no real method-bearing route at bare /api +// — the actual production shape, where only the fallback sits there — any +// method on /api answers 404 with no Allow header, never a spurious 405. +func TestBareAPIWithoutRealRouteIs404(t *testing.T) { + srv, _ := setupTestServer(t) + router := mux.NewRouter() + srv.RegisterRoutes(router) // registers the fallback at the end, no real /api route + + for _, method := range []string{"GET", "POST", "DELETE"} { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(method, "/api", nil)) + if w.Code != http.StatusNotFound { + t.Fatalf("%s /api with no real route: want 404, got %d", method, w.Code) + } + if allow := w.Header().Get("Allow"); allow != "" { + t.Fatalf("%s /api: want no Allow header, got %q", method, allow) + } } } diff --git a/cmd/server/main.go b/cmd/server/main.go index e2026ac35..e6da02d85 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -659,7 +659,7 @@ func newHTTPRouter(srv *Server, hub *Hub, publicDir string) *mux.Router { 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/docs

`)) + w.Write([]byte(`

CoreScope

Frontend not found. API available at /api/spec (interactive docs: /api/docs, needs internet).

`)) }) } return router From d832b7c9483e3dbda1f611aaf2f7a6d9e5fa66f8 Mon Sep 17 00:00:00 2001 From: dborup Date: Wed, 7 Oct 2026 07:49:49 +0200 Subject: [PATCH 2/2] test(server): extract banner's real pointer, assert offline JSON + bare /api 405 body (#300 review) Review round 2 on #320 found the strict banner test only extracted /api... tokens, so a banner whose primary pointer was /swagger, an external URL, or /API/spec passed as long as /api/spec was mentioned in passing (M1). It also truncated /api/spec.json to /api/spec at the dot. - F1: TestAPIOnlyBannerNamesOfflineSpecEndpoint now reads the primary pointer after "API available at" and requires GET to be 200, application/json, and not the banner catch-all answering itself. - F1/F2: TestAPIOnlyBannerPointsToExistingEndpoint extracts every URL-ish token (absolute URLs and slash-paths, including /api/spec.json whole), rejects non-local pointers, and requires 200 with a body that is not the banner itself. In API-only mode PathPrefix("/") answers every non-/api path 200 with the banner, so the body check is what distinguishes a real endpoint from the catch-all. - F3: TestAllowedMethodsForBareAPIRoute now also asserts the bare-/api 405 carries the JSON error shape and application/json content-type. Mutants (each dies, was green or truncated before): M1 primary pointer /swagger, M2 /api/spec.json, M3 bare 405 via WriteHeader without JSON. Test-only; cmd/server stays read-only. Relates to #300 --- cmd/server/api_fallback_test.go | 119 ++++++++++++++++++++++++++++---- 1 file changed, 107 insertions(+), 12 deletions(-) diff --git a/cmd/server/api_fallback_test.go b/cmd/server/api_fallback_test.go index 45a6e40c0..772335190 100644 --- a/cmd/server/api_fallback_test.go +++ b/cmd/server/api_fallback_test.go @@ -471,18 +471,89 @@ func apiOnlyBannerBody(t *testing.T) (*mux.Router, string) { return router, w.Body.String() } -// bannerAPIPath matches an /api... path advertised in the banner text. -var bannerAPIPath = regexp.MustCompile(`/api[A-Za-z0-9/_-]*`) +// bannerTag matches an HTML tag, so the banner's prose can be scanned +// without closing tags like

leaking a slash-prefixed token. +var bannerTag = regexp.MustCompile(`<[^>]*>`) + +// bannerReference matches any URL-ish token the banner advertises: an +// absolute http(s) URL, or a slash-prefixed path (preceded by start, +// whitespace, or an opening paren so the leading "/" of a path is caught but +// the "/" inside a closing HTML tag is not). It is deliberately NOT limited +// to "/api..." so a banner that points at /swagger, /docs, /API/spec, or an +// external URL is still extracted and then rejected, instead of silently +// going unchecked (#320 F1/F2). The character class keeps "."/"-"/etc. so +// /api/spec.json is matched whole, not truncated to /api/spec. +var bannerReference = regexp.MustCompile(`https?://[^\s<>"()]+|(?:^|[\s(])/[^\s<>"(),]*`) + +// bannerText strips HTML tags from a banner body, leaving only its prose. +func bannerText(body string) string { + return bannerTag.ReplaceAllString(body, " ") +} + +// bannerReferences returns every URL-ish token the banner's prose advertises, +// trimmed of the leading whitespace/paren the regex captured and of trailing +// sentence punctuation. +func bannerReferences(body string) []string { + var refs []string + for _, m := range bannerReference.FindAllString(bannerText(body), -1) { + ref := strings.TrimRight(strings.TrimLeft(m, " ("), ".,;:") + if ref != "" { + refs = append(refs, ref) + } + } + return refs +} + +// bannerPrimaryPointer returns the endpoint the banner names right after +// "API available at ", or "" if the phrase is absent. +func bannerPrimaryPointer(body string) string { + text := bannerText(body) + const marker = "API available at " + i := strings.Index(text, marker) + if i < 0 { + return "" + } + rest := strings.TrimLeft(text[i+len(marker):], " (") + field := strings.Fields(rest) + if len(field) == 0 { + return "" + } + return strings.TrimRight(field[0], ".,;:") +} // #300 F1: the API-only banner must name an endpoint that works with no // outbound internet. /api/docs is a Swagger UI shell whose CSS and JS load // only from an external CDN (openapi.go), so an API-only deployment without // egress renders it blank. /api/spec is the raw OpenAPI JSON, served // in-process, so it is the offline-safe pointer the banner must name. +// +// #320 F1: this no longer just substring-checks "/api/spec" (which a passing +// mention satisfies even when the banner really points elsewhere). It reads +// the *primary* pointer the banner names ("API available at ") and proves +// it is a local, in-process JSON endpoint: GET must be 200, +// application/json, and must not be the banner catch-all answering itself. +// A banner whose primary pointer is /swagger, an external URL, or the +// CDN-backed /api/docs (text/html) therefore fails. func TestAPIOnlyBannerNamesOfflineSpecEndpoint(t *testing.T) { - _, body := apiOnlyBannerBody(t) - if !strings.Contains(body, "/api/spec") { - t.Fatalf("API-only banner does not name the offline-safe /api/spec: %q", body) + router, body := apiOnlyBannerBody(t) + + primary := bannerPrimaryPointer(body) + if primary == "" { + t.Fatalf("API-only banner names no primary endpoint after %q: %q", "API available at", body) + } + if !strings.HasPrefix(primary, "/") { + t.Fatalf("API-only banner's primary pointer %q is not a local path (needs outbound internet): %q", primary, body) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", primary, nil)) + if w.Code != http.StatusOK { + t.Fatalf("banner's primary pointer %q: GET returned %d (want 200); banner=%q", primary, w.Code, body) + } + if w.Body.String() == body { + t.Fatalf("banner's primary pointer %q is answered only by the banner catch-all, not a real endpoint; banner=%q", primary, body) + } + if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Fatalf("banner's primary pointer %q must be the offline-safe JSON spec, got Content-Type %q; banner=%q", primary, ct, body) } } @@ -491,22 +562,34 @@ func TestAPIOnlyBannerNamesOfflineSpecEndpoint(t *testing.T) { // resolve. It used to say "/api/", which #233 turned into a JSON 404. // // F2 makes the test strict: instead of checking a literal string and then -// GETting that same literal, it extracts every /api path the banner actually -// advertises and GETs each one, asserting 200. A banner that only mentions a -// working path in passing while pointing somewhere broken can no longer pass. +// GETting that same literal, it extracts every reference the banner actually +// advertises and GETs each one. #320 F1/F2 widen that extraction beyond +// "/api..." tokens: any advertised token must be a local path that returns +// 200 with a body that is NOT the banner itself. In API-only mode the +// PathPrefix("/") banner handler answers every non-/api path with 200 and the +// banner body, so "GET -> 200" alone cannot tell a real endpoint from the +// catch-all; the body check closes that. A banner that points at /swagger, +// /api/spec.json, or an external URL — while mentioning a working path only +// in passing — can no longer pass. func TestAPIOnlyBannerPointsToExistingEndpoint(t *testing.T) { router, body := apiOnlyBannerBody(t) - paths := bannerAPIPath.FindAllString(body, -1) - if len(paths) == 0 { - t.Fatalf("API-only banner advertises no /api path: %q", body) + refs := bannerReferences(body) + if len(refs) == 0 { + t.Fatalf("API-only banner advertises no endpoint reference: %q", body) } - for _, p := range paths { + for _, p := range refs { + if !strings.HasPrefix(p, "/") { + t.Fatalf("banner advertises %q, which needs outbound internet (not a local path); banner=%q", p, body) + } w := httptest.NewRecorder() router.ServeHTTP(w, httptest.NewRequest("GET", p, nil)) if w.Code != http.StatusOK { t.Fatalf("banner advertises %q, but GET %q returned %d (want 200); banner=%q", p, p, w.Code, body) } + if w.Body.String() == body { + t.Fatalf("banner advertises %q, but only the banner catch-all answers it (no real endpoint); banner=%q", p, body) + } } } @@ -533,6 +616,18 @@ func TestAllowedMethodsForBareAPIRoute(t *testing.T) { if allow := w.Header().Get("Allow"); allow != "POST" { t.Fatalf("GET /api: want Allow: POST, got %q", allow) } + // #320 F3: the bare-/api 405 shares writeError with /api/*, so it must + // carry the same JSON error shape and Content-Type, not a bare status. + if ct := w.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Fatalf("GET /api (405): want application/json content-type, got %q (body %q)", ct, w.Body.String()) + } + var errBody map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &errBody); err != nil { + t.Fatalf("GET /api (405): body is not JSON: %v (body %q)", err, w.Body.String()) + } + if errBody["error"] == "" { + t.Fatalf("GET /api (405): want a non-empty \"error\" field, got %v", errBody) + } // The real route is still reachable under its own method. w2 := httptest.NewRecorder()