From d79b4169b63a2563cfa7fa1b085fee4a418c7cca Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 27 Oct 2026 19:28:32 +0000 Subject: [PATCH] Give every JSON answer @meta, errors included (closes #201) A panic in a JSON route's handler answered 500 with an empty body, because the router's recoverer sits outside the middleware that adds `@meta`. `JSONValidationMiddleware` now wraps the handler in the same recoverer, and answers the panic with the JSON error body. A method that `/status.json` or the health check does not take got chi's empty 405. Each such method now goes to the JSON 405 that the routes under `/api/v1` and `/ip` give. The home page's IP lookup card showed only the answer's `data`. It now shows the whole answer, `@meta` included. A table test checks `@meta` on every JSON route, in its successes and its errors. Model: opus-5-5 --- README.md | 15 ++- TODO.md | 7 + internal/server/handlers_internal_test.go | 3 +- internal/server/middleware.go | 9 +- internal/server/middleware_internal_test.go | 141 +++++++++++++++++++- internal/server/routes.go | 33 ++++- internal/server/routes_internal_test.go | 15 ++- internal/templates/index.html | 16 +-- 8 files changed, 208 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 528106d..750dee2 100644 --- a/README.md +++ b/README.md @@ -233,11 +233,16 @@ originates no prefix and has no record in the database or in the AS list built into RouteWatch. AS 0 always answers 404, because it means no known origin. The AS name search never lists it. A prefix with no live route answers 404 too. The HTML pages give these errors as plain text. Under `/api/v1` and `/ip/`, a path -no route matches answers 404 with the message `Not found`, and `POST`, `PUT`, -`PATCH`, `DELETE`, `OPTIONS`, `TRACE` or `CONNECT` answers 405 with -`Method not allowed`. A 500 answer says only `Internal server error`, and a host -name that does not resolve only `could not resolve host name`; the detail goes -to the log. +no route matches answers 404 with the message `Not found`. A request with +`POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` or `CONNECT` answers 405 +with `Method not allowed` on these routes: + +- the routes under `/api/v1` and `/ip/` +- `/status.json` +- `/.well-known/healthcheck.json` + +A 500 answer says only `Internal server error`, and a host name that does not +resolve only `could not resolve host name`; the detail goes to the log. Every answer carries `Strict-Transport-Security`, `Content-Security-Policy`, `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy` and diff --git a/TODO.md b/TODO.md index e4761d2..70f0505 100644 --- a/TODO.md +++ b/TODO.md @@ -35,6 +35,13 @@ tested. # Completed Steps +- 2026-10-27: every JSON answer carries `@meta`. A panic on a JSON route answers + 500 with the JSON error body. A method that `/status.json` or the health check + does not take answers 405 with it, as under `/api/v1`. Both used to answer + with an empty body. The home page's IP lookup shows the whole JSON answer, + `@meta` included. It used to show only the answer's `data`, or only the + message of an error answer. A test checks `@meta` on every JSON route, in its + successes and its errors (closes #201) - 2026-10-27: every value in a JSON answer that has a unit is a number, with the unit at the end of its field name. `@meta.execution_time` is now `execution_time_ms`. The stats' `uptime`, `connection_duration` and diff --git a/internal/server/handlers_internal_test.go b/internal/server/handlers_internal_test.go index 642f057..3ae47f1 100644 --- a/internal/server/handlers_internal_test.go +++ b/internal/server/handlers_internal_test.go @@ -75,7 +75,6 @@ const ( asPagePath = "/as/64500" asJSONPath = "/api/v1/as/64500" prefixLengthPagePath = "/prefixlength/24" - statusJSONPath = "/status.json" statsPath = "/api/v1/stats" ) @@ -1109,7 +1108,7 @@ func TestJSONRoutesDoNotSendErrorDetails(t *testing.T) { path string wantCode int }{ - {"/.well-known/healthcheck.json", http.StatusServiceUnavailable}, + {healthCheckPath, http.StatusServiceUnavailable}, {statusJSONPath, http.StatusInternalServerError}, {statsPath, http.StatusInternalServerError}, {asJSONPath, http.StatusInternalServerError}, diff --git a/internal/server/middleware.go b/internal/server/middleware.go index ebe0227..1455ab9 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -16,6 +16,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" ) // responseWriter wraps http.ResponseWriter to capture the response @@ -154,7 +155,8 @@ func deadlinePassed(ctx context.Context) bool { // has passed, it answers 504 with the error message "Request timeout". A 4xx // keeps its status, with the status's standard text as the message. For other // errors, it returns "Internal server error", with status 500 if the original -// status was 200. +// status was 200. A panic in the handler answers 500 with "Internal server +// error" too. // // Non-JSON responses are passed through unchanged. func JSONValidationMiddleware(next http.Handler) http.Handler { @@ -166,8 +168,9 @@ func JSONValidationMiddleware(next http.Handler) http.Handler { statusCode: http.StatusOK, } - // Serve the request - next.ServeHTTP(rw, r) + // Serve the request. Recoverer logs a panic in the handler and turns it + // into a 500 without a body, which gets the JSON error body below. + middleware.Recoverer(next).ServeHTTP(rw, r) // Check if it's meant to be a JSON response contentType := rw.Header().Get("Content-Type") diff --git a/internal/server/middleware_internal_test.go b/internal/server/middleware_internal_test.go index 88d2bf6..345bb81 100644 --- a/internal/server/middleware_internal_test.go +++ b/internal/server/middleware_internal_test.go @@ -2,6 +2,7 @@ package server import ( "bytes" + "context" "encoding/json" "io" "log/slog" @@ -119,7 +120,7 @@ func TestJSONAnswersPutNoUnitInAString(t *testing.T) { paths := []string{ statusJSONPath, statsPath, - "/.well-known/healthcheck.json", + healthCheckPath, "/api/v1/ip/" + untrustedPeerAddr, "/ip/" + untrustedPeerAddr, asJSONPath, @@ -156,6 +157,144 @@ func TestJSONAnswersPutNoUnitInAString(t *testing.T) { } } +// TestJSONAnswersCarryMeta sends requests to every JSON route through the +// server's router, and checks that each answer carries @meta with its three +// fields. The answers include successes, and errors that the handlers, the +// router and the middleware write. Before the fix a panic in a handler, and a +// method /status.json or the health check does not take, answered with an +// empty body (https://git.eeqj.de/sneak/routewatch/issues/201). +func TestJSONAnswersCarryMeta(t *testing.T) { + t.Parallel() + + // JSON routes whose handlers write nothing or panic, so that the JSON + // check on the route writes the error answer itself. + const ( + writesNothingPath = "/writes-nothing.json" + panicsPath = "/panics.json" + ) + + s := newServerWithTestRoutes(t) + s.resolver = &fakeResolver{} + s.streamer.GetMetricsTracker().SetConnected(true) + s.router.Get(writesNothingPath, JSONValidationMiddleware(http.HandlerFunc( + func(http.ResponseWriter, *http.Request) {})).ServeHTTP) + s.router.Get(panicsPath, JSONValidationMiddleware(http.HandlerFunc( + func(http.ResponseWriter, *http.Request) { panic(errDiskIO) })).ServeHTTP) + + tests := []struct { + method string + path string + wantCode int + }{ + // Answers that succeed + {http.MethodGet, statusJSONPath, http.StatusOK}, + {http.MethodGet, healthCheckPath, http.StatusOK}, + {http.MethodGet, "/ip", http.StatusOK}, + {http.MethodGet, ownAddressPath, http.StatusOK}, + {http.MethodGet, "/ip/" + addrInTestPrefixV4, http.StatusOK}, + {http.MethodGet, statsPath, http.StatusOK}, + {http.MethodGet, "/api/v1/ip/" + addrInTestPrefixV4, http.StatusOK}, + {http.MethodGet, asJSONPath, http.StatusOK}, + {http.MethodGet, "/api/v1/prefix/" + testPrefixV4, http.StatusOK}, + {http.MethodGet, "/api/v1/search/as?q=google", http.StatusOK}, + // Errors the handlers write + {http.MethodGet, "/ip/no-such-host.example", http.StatusBadRequest}, + {http.MethodGet, "/api/v1/ip/no-such-host.example", http.StatusBadRequest}, + {http.MethodGet, "/api/v1/as/foo", http.StatusBadRequest}, + {http.MethodGet, "/api/v1/as/0", http.StatusNotFound}, + {http.MethodGet, "/api/v1/prefix/192.0.2.0/33", http.StatusBadRequest}, + {http.MethodGet, "/api/v1/prefix/198.51.100.0/24", http.StatusNotFound}, + {http.MethodGet, "/api/v1/search/as", http.StatusBadRequest}, + // Errors the router and the middleware write + {http.MethodGet, "/api/v1/no-such-route", http.StatusNotFound}, + {http.MethodGet, "/ip/192.0.2.1/nothing", http.StatusNotFound}, + {http.MethodPost, statsPath, http.StatusMethodNotAllowed}, + {http.MethodPost, ownAddressPath, http.StatusMethodNotAllowed}, + {http.MethodPost, statusJSONPath, http.StatusMethodNotAllowed}, + {http.MethodPut, healthCheckPath, http.StatusMethodNotAllowed}, + {http.MethodGet, writesNothingPath, http.StatusInternalServerError}, + {http.MethodGet, panicsPath, http.StatusInternalServerError}, + } + + for _, tt := range tests { + checkAnswerCarriesMeta(t, s, httptest.NewRequestWithContext( + t.Context(), tt.method, tt.path, nil), tt.wantCode) + } + + // A request whose time limit has already passed answers 504. The handlers + // write the answers of the first five routes, and the JSON check on the + // route writes the last. + pastTimeLimit, cancel := context.WithDeadline(t.Context(), time.Now()) + defer cancel() + + for _, path := range []string{ + statusJSONPath, statsPath, "/ip/" + addrInTestPrefixV4, + "/api/v1/ip/" + addrInTestPrefixV4, asJSONPath, writesNothingPath, + } { + checkAnswerCarriesMeta(t, s, httptest.NewRequestWithContext( + pastTimeLimit, http.MethodGet, path, nil), http.StatusGatewayTimeout) + } + + // With a store whose reads fail, the health check answers 503, and the + // handlers of the other routes 500. + failing := New(failingDB{}, newTestTable(), + streamer.New(logger.New(), metrics.New()), logger.New(), &config.Config{}) + failing.resolver = &fakeResolver{} + + checkAnswerCarriesMeta(t, failing, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, healthCheckPath, nil), + http.StatusServiceUnavailable) + + for _, path := range []string{ + statusJSONPath, statsPath, asJSONPath, + "/api/v1/ip/" + addrInTestPrefixV4, "/ip/" + addrInTestPrefixV4, + } { + checkAnswerCarriesMeta(t, failing, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil), http.StatusInternalServerError) + } +} + +// checkAnswerCarriesMeta serves req through the server's router, and checks +// that it answers wantCode with @meta and its three fields. +func checkAnswerCarriesMeta( + t *testing.T, s *Server, req *http.Request, wantCode int, +) { + t.Helper() + + // The values README "API v1" gives the two fields that never change. + const ( + wantTimeZone = "UTC" + wantAPIVersion = 1 + ) + + name := req.Method + " " + req.URL.String() + + rec := httptest.NewRecorder() + s.router.ServeHTTP(rec, req) + + if rec.Code != wantCode { + t.Errorf("%s: status %d, want %d; body %s", name, rec.Code, wantCode, rec.Body) + } + + var answer map[string]any + + err := json.Unmarshal(rec.Body.Bytes(), &answer) + if err != nil { + t.Errorf("%s: decoding the answer %q: %v", name, rec.Body, err) + + return + } + + meta, _ := answer[jsonKeyMeta].(map[string]any) + _, timeIsNumber := meta[metaKeyExecutionTimeMs].(float64) + + if meta[metaKeyTimeZone] != wantTimeZone || + meta[metaKeyAPIVersion] != float64(wantAPIVersion) || !timeIsNumber { + t.Errorf("%s: @meta %v, want time_zone %q, api_version %d and a "+ + "number in execution_time_ms", name, meta, wantTimeZone, wantAPIVersion) + } +} + // stringsIn returns every string in a decoded JSON value, at any depth. func stringsIn(value any) []string { var found []string diff --git a/internal/server/routes.go b/internal/server/routes.go index 58371b7..103a608 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -8,8 +8,8 @@ import ( "github.com/go-chi/chi/v5/middleware" ) -// The error messages the JSON routes under /api/v1 and /ip answer with when no -// route matches the path, and when none takes the method. +// The error messages the JSON routes answer with when no route matches the +// path, and when the route does not take the method. const ( notFoundMessage = "Not found" methodNotAllowedMessage = "Method not allowed" @@ -19,6 +19,13 @@ const ( jsonRoutesAllow = "GET, HEAD" ) +// The paths of the status figures and the health check, the two JSON routes +// outside /api/v1 and /ip. +const ( + statusJSONPath = "/status.json" + healthCheckPath = "/.well-known/healthcheck.json" +) + // setupRoutes configures the HTTP routes func (s *Server) setupRoutes() { r := chi.NewRouter() @@ -32,6 +39,8 @@ func (s *Server) setupRoutes() { r.Use(middleware.RequestID) r.Use(RealIPMiddleware(s.trustedProxies)) r.Use(RequestLoggerMiddleware(s.logger.Logger)) // Structured request logging + // Recoverer answers a panic on a page. JSONValidationMiddleware recovers a + // panic on a JSON route itself, so that the answer is JSON with @meta. r.Use(middleware.Recoverer) r.Use(JSONResponseMiddleware) // Timeout must come after JSONResponseMiddleware, which sends its answer @@ -43,11 +52,6 @@ func (s *Server) setupRoutes() { // Routes r.Get("/", s.handleIndex()) r.Get("/status", s.handleStatusHTML()) - r.Get("/status.json", JSONValidationMiddleware(s.handleStatusJSON()).ServeHTTP) - r.Get( - "/.well-known/healthcheck.json", - JSONValidationMiddleware(s.handleHealthCheck()).ServeHTTP, - ) // AS and prefix detail pages r.Get("/as/{asn}", s.handleASDetail()) @@ -70,6 +74,21 @@ func (s *Server) setupRoutes() { s.writeJSONError(w, http.StatusMethodNotAllowed, methodNotAllowedMessage) } + // The status figures and the health check. chi sets one 405 answer for a + // whole router. These two routes are on the pages' router, whose 405 stays + // chi's own. So each method they do not take is routed to the JSON 405 + // here. + r.Get(statusJSONPath, JSONValidationMiddleware(s.handleStatusJSON()).ServeHTTP) + r.Get(healthCheckPath, JSONValidationMiddleware(s.handleHealthCheck()).ServeHTTP) + + for _, method := range []string{ + http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete, + http.MethodOptions, http.MethodTrace, http.MethodConnect, + } { + r.MethodFunc(method, statusJSONPath, jsonMethodNotAllowed) + r.MethodFunc(method, healthCheckPath, jsonMethodNotAllowed) + } + // IP info JSON endpoints (replaces old /ip redirect) r.Route("/ip", func(r chi.Router) { r.Use(JSONValidationMiddleware) diff --git a/internal/server/routes_internal_test.go b/internal/server/routes_internal_test.go index f4f0d6a..01f75ee 100644 --- a/internal/server/routes_internal_test.go +++ b/internal/server/routes_internal_test.go @@ -17,7 +17,7 @@ func TestRoutesAnswerHEAD(t *testing.T) { s := newServerWithTestRoutes(t) - for _, path := range []string{"/", statsPath, "/ip", "/ip/"} { + for _, path := range []string{"/", statusJSONPath, statsPath, "/ip", "/ip/"} { rec := httptest.NewRecorder() s.router.ServeHTTP(rec, httptest.NewRequestWithContext( t.Context(), http.MethodHead, path, nil)) @@ -31,10 +31,13 @@ func TestRoutesAnswerHEAD(t *testing.T) { // TestJSONRoutesAnswerUnmatchedRequestsWithJSONErrors sends a method the JSON // routes do not take, and a path none of them matches, to each group of JSON -// routes through the server's router. Each must answer with the JSON error +// routes through the server's router. It also sends a method they do not take +// to /status.json and the health check. Each must answer with the JSON error // body, and a 405 must name the methods the routes take in its Allow header. // Before the fix a 405 said "Internal server error" and a 404 was chi's plain // text answer (https://git.eeqj.de/sneak/routewatch/issues/111). +// /status.json and the health check gave chi's empty 405 +// (https://git.eeqj.de/sneak/routewatch/issues/201). func TestJSONRoutesAnswerUnmatchedRequestsWithJSONErrors(t *testing.T) { t.Parallel() @@ -48,6 +51,14 @@ func TestJSONRoutesAnswerUnmatchedRequestsWithJSONErrors(t *testing.T) { }{ {http.MethodPost, statsPath, http.StatusMethodNotAllowed, methodNotAllowedMessage}, {http.MethodPost, "/ip/", http.StatusMethodNotAllowed, methodNotAllowedMessage}, + { + http.MethodDelete, statusJSONPath, + http.StatusMethodNotAllowed, methodNotAllowedMessage, + }, + { + http.MethodOptions, healthCheckPath, + http.StatusMethodNotAllowed, methodNotAllowedMessage, + }, {http.MethodGet, "/api/v1/nothing", http.StatusNotFound, notFoundMessage}, {http.MethodGet, "/ip/192.0.2.1/nothing", http.StatusNotFound, notFoundMessage}, } diff --git a/internal/templates/index.html b/internal/templates/index.html index ff1edfb..c940576 100644 --- a/internal/templates/index.html +++ b/internal/templates/index.html @@ -714,7 +714,8 @@ }); }); - // IP lookup + // IP lookup: show the whole JSON answer, its @meta and an error answer + // included, as the IP information API card does. document.getElementById('ip-form').addEventListener('submit', function(e) { e.preventDefault(); const ip = document.getElementById('ip-input').value.trim(); @@ -730,18 +731,11 @@ fetch('/ip/' + encodeURIComponent(ip)) .then(response => response.json()) - .then(response => { - contentEl.classList.remove('loading'); - if (response.status === 'error') { - contentEl.className = 'error'; - contentEl.textContent = 'Error: ' + response.error.msg; - } else { - contentEl.className = ''; - contentEl.textContent = JSON.stringify(response.data, null, 2); - } + .then(answer => { + contentEl.className = ''; + contentEl.textContent = JSON.stringify(answer, null, 2); }) .catch(error => { - contentEl.classList.remove('loading'); contentEl.className = 'error'; contentEl.textContent = 'Error: ' + error.message; });