From 73b39e485b2465d1db5cb53f253cace2859bffa2 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 14 Oct 2026 12:43:25 +0200 Subject: [PATCH] Reports are taken only as application/json (closes #126) A page on another origin can POST text/plain or a form without a CORS preflight, so its visitors' browsers could store reports although the server sends no CORS headers. decodeJSON now refuses a request whose Content-Type is not application/json (parameters such as charset allowed), and the report handler answers it 415 with the usual error body, storing nothing. A JSON POST waits for the preflight, which only an origin in CORS_ALLOWED_ORIGINS passes; the page already sends application/json. Model: opus-5-5 --- README.md | 8 ++- TODO.md | 11 ++++ internal/handlers/handlers.go | 25 ++++++-- internal/handlers/report.go | 12 +++- internal/handlers/report_test.go | 103 +++++++++++++++++++++++++++++++ internal/server/routes_test.go | 1 + 6 files changed, 152 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index f9063f8..1b14bcf 100644 --- a/README.md +++ b/README.md @@ -231,9 +231,15 @@ Files are created with `O_EXCL` to prevent overwrites. #### Report limits `POST /api/v1/reports` takes reports from anyone who can reach it, without -credentials, so it is bounded instead. Both refusals below answer with the same +credentials, so it is bounded instead. Each refusal below answers with the same `{"status":"error"}` body as any other error. +- **JSON only.** A report must be sent with `Content-Type: application/json`, + parameters such as `charset` allowed, as the page sends it; any other is + refused with 415, and nothing of it is stored. A page on another origin can + send a `text/plain` POST, or a form, without a CORS preflight; refusing them + keeps any site from making its visitors' browsers store reports (see + [CORS](#cors)). - **Rate limit.** Each client address, resolved through `TRUSTED_PROXIES`, may send `REPORTS_PER_MINUTE` reports a minute; past that it gets 429 with `Retry-After: 60`. The minute slides: reports from the minute before still diff --git a/TODO.md b/TODO.md index f666490..ef59740 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,17 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-14: `POST /api/v1/reports` takes only a report sent with + `Content-Type: application/json`, parameters such as `charset` allowed, and + refuses any other with 415 and the usual error body, storing nothing + ([#126](https://git.eeqj.de/sneak/netwatch/issues/126)). A page on another + origin can send a `text/plain` POST without a CORS preflight, so any site + could make its visitors' browsers store reports; a JSON POST waits for the + preflight, which only an origin in `CORS_ALLOWED_ORIGINS` passes. The page + already sent `application/json`. A test posts a report as `text/plain`, as + `application/x-www-form-urlencoded`, as `multipart/form-data`, with no + `Content-Type`, and as `application/json` with and without a charset, and + checks which are stored - 2026-10-14: changing the interval no longer drops the samples not yet reported to the backend ([#124](https://git.eeqj.de/sneak/netwatch/issues/124)). The interval menu resets each host with the new `HostState.reset`, which moves the diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index db51059..f1b5432 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -5,7 +5,9 @@ package handlers import ( "context" "encoding/json" + "errors" "log/slog" + "mime" "net/http" "sneak.berlin/go/netwatch/internal/globals" @@ -18,6 +20,10 @@ import ( const jsonContentType = "application/json; charset=utf-8" +// errNotJSON is what decodeJSON returns for a request whose +// Content-Type is not application/json. +var errNotJSON = errors.New("content type is not application/json") + // reportAppender is the subset of the report buffer the handlers // depend on. Defining it here keeps the storage failure path // exercisable with a stub in tests. @@ -80,14 +86,25 @@ func (s *Handlers) respondJSON( } } -// decodeJSON decodes the request body into v. The body is -// expected to already be bounded by the body-size middleware, so -// a caller can distinguish an over-limit body from malformed -// JSON by testing the returned error for *http.MaxBytesError. +// decodeJSON decodes the request body into v. It returns errNotJSON, +// reading nothing, unless the request's Content-Type is +// application/json, with or without parameters such as charset. The +// body is expected to already be bounded by the body-size middleware, +// so a caller can distinguish an over-limit body from malformed JSON +// by testing the returned error for *http.MaxBytesError. func (s *Handlers) decodeJSON( _ http.ResponseWriter, r *http.Request, v any, ) error { + // A page on another origin can POST text/plain or a form without + // a CORS preflight, so its visitors' browsers would send it + // whatever the CORS settings. A JSON POST waits for the + // preflight, so only JSON is taken. + mediaType, _, err := mime.ParseMediaType(r.Header.Get("Content-Type")) + if err != nil || mediaType != "application/json" { + return errNotJSON + } + return json.NewDecoder(r.Body).Decode(v) } diff --git a/internal/handlers/report.go b/internal/handlers/report.go index 8aa5ad9..8656cfe 100644 --- a/internal/handlers/report.go +++ b/internal/handlers/report.go @@ -30,7 +30,7 @@ type report struct { } // HandleReport returns a handler that accepts telemetry -// reports from NetWatch clients. +// reports from NetWatch clients, sent as application/json. func (s *Handlers) HandleReport() http.HandlerFunc { type response struct { Status string `json:"status"` @@ -66,9 +66,15 @@ func (s *Handlers) HandleReport() http.HandlerFunc { } // decodeErrorStatus logs a report decode failure and returns the -// status to send: 413 when the body exceeded the size limit, -// otherwise 400 for malformed JSON. +// status to send: 415 when the report was not sent as JSON, 413 when +// the body exceeded the size limit, otherwise 400 for malformed JSON. func (s *Handlers) decodeErrorStatus(err error) int { + if errors.Is(err, errNotJSON) { + s.log.Warn("report refused: Content-Type is not application/json") + + return http.StatusUnsupportedMediaType + } + var tooLarge *http.MaxBytesError if errors.As(err, &tooLarge) { s.log.Warn("report body too large", "limit_bytes", tooLarge.Limit) diff --git a/internal/handlers/report_test.go b/internal/handlers/report_test.go index bd6345c..020ba67 100644 --- a/internal/handlers/report_test.go +++ b/internal/handlers/report_test.go @@ -27,6 +27,17 @@ type stubAppender struct { func (s stubAppender) Append(any) error { return s.err } +// recordingAppender keeps every report the handler stores. +type recordingAppender struct { + reports []any +} + +func (a *recordingAppender) Append(v any) error { + a.reports = append(a.reports, v) + + return nil +} + func newTestHandlers(buf stubAppender, out io.Writer) *handlers.Handlers { return handlers.NewForTest(buf, slog.New(slog.NewJSONHandler(out, nil))) } @@ -83,6 +94,7 @@ func TestHandleReportAcceptsValidReports(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(tt.body), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -103,6 +115,89 @@ func TestHandleReportAcceptsValidReports(t *testing.T) { } } +// TestHandleReportStoresOnlyJSON checks that a report is stored only +// when sent as application/json, as the page sends it. One sent as +// text/plain or as a form, which a page on another origin can send +// without a CORS preflight, is refused with 415. +func TestHandleReportStoresOnlyJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + contentType string + wantStatus int + wantStored int + }{ + { + name: "text/plain", + contentType: "text/plain;charset=UTF-8", + wantStatus: http.StatusUnsupportedMediaType, + wantStored: 0, + }, + { + name: "a urlencoded form", + contentType: "application/x-www-form-urlencoded", + wantStatus: http.StatusUnsupportedMediaType, + wantStored: 0, + }, + { + name: "a multipart form", + contentType: "multipart/form-data; boundary=x", + wantStatus: http.StatusUnsupportedMediaType, + wantStored: 0, + }, + { + name: "no Content-Type", + contentType: "", + wantStatus: http.StatusUnsupportedMediaType, + wantStored: 0, + }, + { + name: "application/json", + contentType: "application/json", + wantStatus: http.StatusOK, + wantStored: 1, + }, + { + name: "application/json with a charset", + contentType: "application/json;charset=UTF-8", + wantStatus: http.StatusOK, + wantStored: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var stored recordingAppender + + h := handlers.NewForTest(&stored, slog.New(slog.DiscardHandler)) + + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext(t.Context(), + http.MethodPost, "/api/v1/reports", + strings.NewReader(`{"clientId":"c1","hosts":[]}`), + ) + + if tt.contentType != "" { + req.Header.Set("Content-Type", tt.contentType) + } + + h.HandleReport().ServeHTTP(rec, req) + + if rec.Code != tt.wantStatus { + t.Errorf("status = %d, want %d", rec.Code, tt.wantStatus) + } + + if len(stored.reports) != tt.wantStored { + t.Errorf("reports stored = %d, want %d", + len(stored.reports), tt.wantStored) + } + }) + } +} + func TestHandleReportStorageFailureIsNon2xx(t *testing.T) { t.Parallel() @@ -113,6 +208,7 @@ func TestHandleReportStorageFailureIsNon2xx(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{"clientId":"c1","hosts":[]}`), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -138,6 +234,7 @@ func TestHandleReportFullIs507(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{"clientId":"c1","hosts":[]}`), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -161,6 +258,7 @@ func TestHandleReportMalformedJSONIs400(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{not json`), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -185,6 +283,8 @@ func TestHandleReportOversizeIs413(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{"clientId":"`+strings.Repeat("x", 200)+`"}`), ) + req.Header.Set("Content-Type", "application/json") + // No declared length, so only the middleware's read cap can // stop this body. req.ContentLength = -1 @@ -213,6 +313,7 @@ func TestHandleReportDoesNotLogRawGeo(t *testing.T) { `{"clientId":"c1","geo":{"raw":"`+sentinel+`"},"hosts":[]}`, ), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -245,6 +346,7 @@ func TestHandleReportLogsClientIDCutToBound(t *testing.T) { `{"clientId":"`+long+`","timestamp":"`+long+`","hosts":[]}`, ), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) @@ -284,6 +386,7 @@ func TestHandleReportDecodeErrorLogIsBounded(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{"hosts":[{"history":[{"t":`+huge+`}]}]}`), ) + req.Header.Set("Content-Type", "application/json") h.HandleReport().ServeHTTP(rec, req) diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 8e076cf..fd805f3 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -66,6 +66,7 @@ func TestReportsAreRateLimited(t *testing.T) { http.MethodPost, "/api/v1/reports", strings.NewReader(`{"clientId":"c1","hosts":[]}`), ) + req.Header.Set("Content-Type", "application/json") srv.ServeHTTP(rec, req) return rec.Code