From 0dd748ddf702c2dcde9c77f537168f88e0582b5c Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 11 Nov 2026 00:34:16 +0000 Subject: [PATCH] Refuse API writes that are not application/json (closes #271) A page on another origin of the same site could send a text/plain or form POST with the user's cookie, since a browser sends such a request without a preflight. RequireJSON on the /api/v1 router answers 415 to a POST, PUT or DELETE whose Content-Type is not application/json, or that has none, before the handler runs. The web client and neoirc-cli already send the header on every request. Model: opus-5-5 --- README.md | 7 +- internal/handlers/api_test.go | 127 ++++++++++++++++++++++++- internal/middleware/middleware.go | 52 ++++++++++ internal/middleware/middleware_test.go | 67 +++++++++++++ internal/server/routes.go | 2 + 5 files changed, 253 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4f5c114..4f0dafa 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,10 @@ either. ### Conventions - Request and response bodies are JSON. +- Every `POST`, `PUT` and `DELETE` request under `/api/v1/` needs the header + `Content-Type: application/json`, even one with no body. A request with + another type, or none, gets `415` with + `{"error": "content type must be application/json"}`. - Endpoints marked "needs auth" need the `neoirc_auth` cookie. `POST /api/v1/session` and `POST /api/v1/login` set it. A request without a valid cookie gets `401` with `{"error": "not registered", "numeric": 451}`. @@ -2006,7 +2010,8 @@ and their sessions to end. Then it closes the database. - `internal/hashcash/`: stamp checking and minting. - `internal/ratelimit/`: the per-address login rate limiter. - `internal/middleware/`: finding the client's address, request logging, - cross-origin requests, the `Content-Security-Policy` header and metrics. + cross-origin requests, the API's `Content-Type` check, the + `Content-Security-Policy` header and metrics. - `internal/healthcheck/` and `internal/stats/`: the health check and its counters. - `internal/logger/` and `internal/globals/`: logging setup and build diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index bc9ce2a..d418cff 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -415,7 +415,7 @@ func doRequestAuth( return nil, fmt.Errorf("new request: %w", err) } - if body != nil { + if method != http.MethodGet { request.Header.Set( "Content-Type", "application/json", ) @@ -903,6 +903,131 @@ func TestRequestBodyLimit(t *testing.T) { } } +// requestWithType sends a request with the Content-Type contentType, +// or none when it is empty, and returns the status and the decoded +// JSON body. +func (tserver *testServer) requestWithType( + method, path, token, contentType, body string, +) (int, map[string]any) { + tserver.t.Helper() + + request, err := http.NewRequestWithContext( + tserver.t.Context(), method, tserver.url(path), + strings.NewReader(body), + ) + if err != nil { + tserver.t.Fatalf("new request: %v", err) + } + + if contentType != "" { + request.Header.Set("Content-Type", contentType) + } + + request.AddCookie(&http.Cookie{ //nolint:exhaustruct,gosec // only name+value + Name: authCookieName, + Value: token, + }) + + resp, err := http.DefaultClient.Do(request) + if err != nil { + tserver.t.Fatalf("%s %s: %v", method, path, err) + } + + defer func() { _ = resp.Body.Close() }() + + var result map[string]any + + _ = json.NewDecoder(resp.Body).Decode(&result) + + return resp.StatusCode, result +} + +// TestRequireJSONContentType checks that a POST whose Content-Type is +// not application/json gets 415 and changes nothing: the message is +// not stored and the client is not logged out. +func TestRequireJSONContentType(t *testing.T) { + t.Parallel() + + tserver := newTestServer(t) + token := tserver.createSession("alice_ctype") + + tserver.sendCommand(token, map[string]any{ + commandKey: joinCmd, toKey: "#ctype", + }) + + status, _ := tserver.requestWithType( + http.MethodGet, apiState, token, "text/plain", "", + ) + if status != http.StatusOK { + t.Errorf("GET with text/plain: expected 200, got %d", status) + } + + tests := []struct { + name, contentType string + want int + }{ + {"text", "text/plain", http.StatusUnsupportedMediaType}, + { + "form", "application/x-www-form-urlencoded", + http.StatusUnsupportedMediaType, + }, + { + "multipart", "multipart/form-data; boundary=x", + http.StatusUnsupportedMediaType, + }, + {"missing", "", http.StatusUnsupportedMediaType}, + {"json", "application/json", http.StatusOK}, + {"json with charset", "application/json; charset=utf-8", http.StatusOK}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + status, result := tserver.requestWithType( + http.MethodPost, apiMessages, token, test.contentType, + `{"command":"PRIVMSG","to":"#ctype","body":["`+test.name+`"]}`, + ) + if status != test.want { + t.Fatalf("send: expected %d, got %d: %v", test.want, status, result) + } + + if status == http.StatusUnsupportedMediaType && + result["error"] != "content type must be application/json" { + t.Errorf("send: unexpected error body %v", result) + } + + msgs, _ := tserver.pollMessages(token, 0) + + stored := slices.ContainsFunc(msgs, func(msg map[string]any) bool { + body, _ := msg[bodyKey].([]any) + + return msg[commandKey] == privmsgCmd && + slices.Contains(body, any(test.name)) + }) + if stored != (test.want == http.StatusOK) { + t.Errorf("message stored: %v, want %v", stored, !stored) + } + + if test.want == http.StatusOK { + return + } + + status, _ = tserver.requestWithType( + http.MethodPost, apiLogout, token, test.contentType, "", + ) + if status != http.StatusUnsupportedMediaType { + t.Errorf("logout: expected 415, got %d", status) + } + + status, _ = tserver.getState(token) + if status != http.StatusOK { + t.Errorf("state after refused logout: expected 200, got %d", status) + } + }) + } +} + func TestAuthNoCookie(t *testing.T) { t.Parallel() diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index a4e67c6..79d8d6b 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -3,6 +3,7 @@ package middleware import ( "log/slog" + "mime" "net" "net/http" "net/netip" @@ -217,6 +218,57 @@ func (mware *Middleware) CORS() func(http.Handler) http.Handler { }) } +// notJSONError is the body of the 415 answer, in the API's usual +// error format. +const notJSONError = `{"error":"content type must be application/json"}` + "\n" + +// needsJSON reports whether a request with method must have the +// Content-Type application/json, whether or not it has a body. +func needsJSON(method string) bool { + return method == http.MethodPost || + method == http.MethodPut || + method == http.MethodDelete +} + +// isJSON reports whether the request's Content-Type is +// application/json, with or without parameters such as charset. +func isJSON(request *http.Request) bool { + mediaType, _, err := mime.ParseMediaType( + request.Header.Get("Content-Type"), + ) + + return err == nil && mediaType == "application/json" +} + +// RequireJSON returns middleware that answers 415 to a POST, PUT or +// DELETE request whose Content-Type is not application/json, or that +// has none. A browser sends a request with another type, such as +// text/plain, to another origin of the same site without asking the +// server first, and attaches the auth cookie. Refusing those types +// stops a page on that origin from acting as the signed-in user. +func (mware *Middleware) RequireJSON() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc( + func( + writer http.ResponseWriter, + request *http.Request, + ) { + if needsJSON(request.Method) && !isJSON(request) { + writer.Header().Set( + "Content-Type", + "application/json; charset=utf-8", + ) + writer.WriteHeader(http.StatusUnsupportedMediaType) + _, _ = writer.Write([]byte(notJSONError)) + + return + } + + next.ServeHTTP(writer, request) + }) + } +} + // Metrics returns middleware that records HTTP metrics. func (mware *Middleware) Metrics() func(http.Handler) http.Handler { metricsMiddleware := ghmm.New(ghmm.Config{ //nolint:exhaustruct // optional fields diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index e2ef069..ac2d852 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -152,6 +152,73 @@ func TestClientIP(t *testing.T) { } } +func TestRequireJSON(t *testing.T) { + t.Parallel() + + mware := newMiddleware(t, trustedNetwork) + + const ( + jsonType = "application/json" + textType = "text/plain" + refused = http.StatusUnsupportedMediaType + ) + + tests := []struct { + method, contentType string + want int + }{ + {http.MethodPost, jsonType, http.StatusOK}, + {http.MethodPost, "application/json; charset=utf-8", http.StatusOK}, + {http.MethodPost, "Application/JSON", http.StatusOK}, + {http.MethodPost, textType, refused}, + {http.MethodPost, "application/x-www-form-urlencoded", refused}, + {http.MethodPost, "multipart/form-data; boundary=x", refused}, + {http.MethodPost, "", refused}, + {http.MethodPost, "application/json; charset", refused}, + {http.MethodPut, textType, refused}, + {http.MethodPut, jsonType, http.StatusOK}, + {http.MethodDelete, "", refused}, + {http.MethodDelete, jsonType, http.StatusOK}, + {http.MethodGet, "", http.StatusOK}, + {http.MethodGet, textType, http.StatusOK}, + } + + for _, test := range tests { + t.Run(test.method+" "+test.contentType, func(t *testing.T) { + t.Parallel() + + handler := mware.RequireJSON()(http.HandlerFunc(func( + http.ResponseWriter, *http.Request, + ) { + })) + + request := httptest.NewRequestWithContext( + t.Context(), test.method, "/", nil, + ) + if test.contentType != "" { + request.Header.Set("Content-Type", test.contentType) + } + + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + if recorder.Code != test.want { + t.Fatalf("status %d, want %d", recorder.Code, test.want) + } + + if test.want == http.StatusOK { + return + } + + want := `{"error":"content type must be application/json"}` + "\n" + if recorder.Body.String() != want { + t.Errorf("body %q, want %q", recorder.Body.String(), want) + } + }) + } +} + // newMiddleware returns a Middleware that trusts the network trusted. func newMiddleware(t *testing.T, trusted string) *middleware.Middleware { t.Helper() diff --git a/internal/server/routes.go b/internal/server/routes.go index 82bbac5..3f54080 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -68,6 +68,8 @@ func (srv *Server) SetupRoutes() { } func (srv *Server) setupAPIv1(router chi.Router) { + router.Use(srv.mw.RequireJSON()) + router.With(srv.mw.CORS()).Get( "/server", srv.handlers.HandleServerInfo(),