Answer HEAD requests and give 405 a plain message (closes #111)

Every route was registered for GET only, so HEAD answered 405, and on
the JSON routes that 405 said "Internal server error". chi's GetHead
now serves HEAD with the GET handler; /ip and /ip/ register HEAD
themselves, as GetHead does not reach them. The /api/v1 and /ip route
groups answer a path they lack with a JSON 404 "Not found", and the
other methods the router knows (POST, PUT, PATCH, DELETE, OPTIONS,
TRACE, CONNECT) with a JSON 405 "Method not allowed" whose Allow header
names GET and HEAD. JSONValidationMiddleware gives a 4xx sent without a
JSON body the status's standard text instead of "Internal server error".

Model: opus-5-5

internal/server/middleware.go

@@ -142,8 +142,9 @@ func deadlinePassed(ctx context.Context) bool {
 //
 // If a response is not valid JSON or is empty when JSON is expected, the middleware
 // returns a properly formatted JSON error response. If the request's time limit
-// has passed, it answers 504 with the error message "Request timeout". For other
-// errors, it returns "Internal server error" with status 500 if the original
+// 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.
 //
 // Non-JSON responses are passed through unchanged.
@@ -203,8 +204,13 @@ func JSONValidationMiddleware(next http.Handler) http.Handler {
 		w.WriteHeader(statusCode)
 
 		errorMsg := internalErrorMessage
-		if statusCode == http.StatusGatewayTimeout {
+
+		switch {
+		case statusCode == http.StatusGatewayTimeout:
 			errorMsg = requestTimeoutMessage
+		case statusCode >= http.StatusBadRequest &&
+			statusCode < http.StatusInternalServerError:
+			errorMsg = http.StatusText(statusCode)
 		}
 
 		response := map[string]any{

internal/server/middleware_internal_test.go

@@ -58,6 +58,34 @@ func TestResponsesCarrySecurityHeaders(t *testing.T) {
 	}
 }
 
+// TestJSONValidationKeeps4xxWithoutBody wraps a handler that answers 400
+// without a body in JSONValidationMiddleware and checks that the JSON error
+// body keeps the 400 and gives the status's own text. Before the fix it said
+// "Internal server error"
+// (https://git.eeqj.de/sneak/routewatch/issues/111).
+func TestJSONValidationKeeps4xxWithoutBody(t *testing.T) {
+	t.Parallel()
+
+	handler := JSONValidationMiddleware(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusBadRequest)
+		}))
+
+	rec := httptest.NewRecorder()
+	handler.ServeHTTP(rec, httptest.NewRequestWithContext(
+		t.Context(), http.MethodGet, "/", nil))
+
+	const name = "400 without a body"
+
+	if rec.Code != http.StatusBadRequest {
+		t.Errorf("%s: status %d, want %d; body %s",
+			name, rec.Code, http.StatusBadRequest, rec.Body)
+	}
+
+	checkJSONErrorCode(t, name, rec, http.StatusBadRequest)
+	checkJSONErrorMessage(t, name, rec, http.StatusText(http.StatusBadRequest))
+}
+
 // TestPagesCarryTheCSPNonce checks that every script and style block of each
 // page carries the nonce its Content-Security-Policy header names, so that the
 // browser runs and applies them, that each answer has a nonce of its own, and

internal/server/routes.go

@@ -1,18 +1,33 @@
 package server
 
 import (
+	"net/http"
 	"time"
 
 	"github.com/go-chi/chi/v5"
 	"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.
+const (
+	notFoundMessage         = "Not found"
+	methodNotAllowedMessage = "Method not allowed"
+
+	// jsonRoutesAllow is the Allow header of their 405 answer: every one of
+	// them takes GET and HEAD, and no other method.
+	jsonRoutesAllow = "GET, HEAD"
+)
+
 // setupRoutes configures the HTTP routes
 func (s *Server) setupRoutes() {
 	r := chi.NewRouter()
 
 	// Middleware
 	r.Use(DecodedPathMiddleware)
+	// GetHead serves HEAD with the GET handler. It looks the route up by the
+	// path DecodedPathMiddleware sets, so it comes after it.
+	r.Use(middleware.GetHead)
 	r.Use(SecurityHeadersMiddleware)
 	r.Use(middleware.RequestID)
 	r.Use(RealIPMiddleware(s.trustedProxies))
@@ -40,16 +55,34 @@ func (s *Server) setupRoutes() {
 	r.Get("/prefixlength/{length}", s.handlePrefixLength())
 	r.Get("/prefixlength6/{length}", s.handlePrefixLength6())
 
+	// The JSON routes answer a path or method they do not take with the JSON
+	// error body, as they answer everything else.
+	jsonNotFound := func(w http.ResponseWriter, _ *http.Request) {
+		s.writeJSONError(w, http.StatusNotFound, notFoundMessage)
+	}
+	jsonMethodNotAllowed := func(w http.ResponseWriter, _ *http.Request) {
+		// chi's own 405 answer sets Allow; one set in its place must too.
+		w.Header().Set("Allow", jsonRoutesAllow)
+		s.writeJSONError(w, http.StatusMethodNotAllowed, methodNotAllowedMessage)
+	}
+
 	// IP info JSON endpoints (replaces old /ip redirect)
 	r.Route("/ip", func(r chi.Router) {
 		r.Use(JSONValidationMiddleware)
-		r.Get("/", s.handleIPInfo())       // Client IP
+		r.NotFound(jsonNotFound)
+		r.MethodNotAllowed(jsonMethodNotAllowed)
+		r.Get("/", s.handleIPInfo()) // Client IP
+		// GetHead does not reach this route: mounting the group registers
+		// /ip and /ip/ for every method, so HEAD arrives here as HEAD.
+		r.Head("/", s.handleIPInfo())
 		r.Get("/{addr}", s.handleIPInfo()) // Specified IP
 	})
 
 	// API routes
 	r.Route("/api/v1", func(r chi.Router) {
 		r.Use(JSONValidationMiddleware)
+		r.NotFound(jsonNotFound)
+		r.MethodNotAllowed(jsonMethodNotAllowed)
 		r.Get("/stats", s.handleStats())
 		r.Get("/ip/{ip}", s.handleIPLookup())
 		r.Get("/as/{asn}", s.handleASDetailJSON())

internal/server/routes_internal_test.go

@@ -0,0 +1,100 @@
+package server
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// TestRoutesAnswerHEAD sends HEAD for a page and JSON routes through the
+// server's router and checks that each answers as GET does. /ip and /ip/, the
+// lookup of the caller's own address, are in the list because chi's GetHead
+// does not reach them. Before the fix every route answered HEAD with 405
+// (https://git.eeqj.de/sneak/routewatch/issues/111).
+func TestRoutesAnswerHEAD(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+
+	for _, path := range []string{"/", statsPath, "/ip", "/ip/"} {
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodHead, path, nil))
+
+		if rec.Code != http.StatusOK {
+			t.Errorf("HEAD %s: status %d, want %d; body %s",
+				path, rec.Code, http.StatusOK, rec.Body)
+		}
+	}
+}
+
+// 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
+// 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).
+func TestJSONRoutesAnswerUnmatchedRequestsWithJSONErrors(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+
+	tests := []struct {
+		method   string
+		path     string
+		wantCode int
+		wantMsg  string
+	}{
+		{http.MethodPost, statsPath, http.StatusMethodNotAllowed, methodNotAllowedMessage},
+		{http.MethodPost, "/ip/", http.StatusMethodNotAllowed, methodNotAllowedMessage},
+		{http.MethodGet, "/api/v1/nothing", http.StatusNotFound, notFoundMessage},
+		{http.MethodGet, "/ip/192.0.2.1/nothing", http.StatusNotFound, notFoundMessage},
+	}
+
+	for _, tt := range tests {
+		name := tt.method + " " + tt.path
+
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), tt.method, tt.path, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				name, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		checkJSONErrorCode(t, name, rec, tt.wantCode)
+		checkJSONErrorMessage(t, name, rec, tt.wantMsg)
+
+		allow := rec.Header().Get("Allow")
+		if tt.wantCode == http.StatusMethodNotAllowed && allow != jsonRoutesAllow {
+			t.Errorf("%s: Allow %q, want %q", name, allow, jsonRoutesAllow)
+		}
+	}
+}
+
+// checkJSONErrorMessage checks that rec holds the JSON error body with the
+// given message.
+func checkJSONErrorMessage(
+	t *testing.T, name string, rec *httptest.ResponseRecorder, want string,
+) {
+	t.Helper()
+
+	var body struct {
+		Error struct {
+			Msg string `json:"msg"`
+		} `json:"error"`
+	}
+
+	err := json.Unmarshal(rec.Body.Bytes(), &body)
+	if err != nil {
+		t.Errorf("%s: decoding the answer %q: %v", name, rec.Body, err)
+
+		return
+	}
+
+	if body.Error.Msg != want {
+		t.Errorf("%s: error message %q, want %q", name, body.Error.Msg, want)
+	}
+}