Reports are taken only as application/json (closes #126)
check / check (push) Waiting to run
check / check (push) Waiting to run
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
This commit was merged in pull request #132.
This commit is contained in:
@@ -231,9 +231,15 @@ Files are created with `O_EXCL` to prevent overwrites.
|
|||||||
#### Report limits
|
#### Report limits
|
||||||
|
|
||||||
`POST /api/v1/reports` takes reports from anyone who can reach it, without
|
`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.
|
`{"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
|
- **Rate limit.** Each client address, resolved through `TRUSTED_PROXIES`, may
|
||||||
send `REPORTS_PER_MINUTE` reports a minute; past that it gets 429 with
|
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
|
`Retry-After: 60`. The minute slides: reports from the minute before still
|
||||||
|
|||||||
@@ -21,6 +21,17 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of
|
|||||||
|
|
||||||
# Completed Steps
|
# 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
|
- 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
|
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
|
interval menu resets each host with the new `HostState.reset`, which moves the
|
||||||
|
|||||||
@@ -5,7 +5,9 @@ package handlers
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
|
"mime"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
|
||||||
"sneak.berlin/go/netwatch/internal/globals"
|
"sneak.berlin/go/netwatch/internal/globals"
|
||||||
@@ -18,6 +20,10 @@ import (
|
|||||||
|
|
||||||
const jsonContentType = "application/json; charset=utf-8"
|
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
|
// reportAppender is the subset of the report buffer the handlers
|
||||||
// depend on. Defining it here keeps the storage failure path
|
// depend on. Defining it here keeps the storage failure path
|
||||||
// exercisable with a stub in tests.
|
// exercisable with a stub in tests.
|
||||||
@@ -80,14 +86,25 @@ func (s *Handlers) respondJSON(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// decodeJSON decodes the request body into v. The body is
|
// decodeJSON decodes the request body into v. It returns errNotJSON,
|
||||||
// expected to already be bounded by the body-size middleware, so
|
// reading nothing, unless the request's Content-Type is
|
||||||
// a caller can distinguish an over-limit body from malformed
|
// application/json, with or without parameters such as charset. The
|
||||||
// JSON by testing the returned error for *http.MaxBytesError.
|
// 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(
|
func (s *Handlers) decodeJSON(
|
||||||
_ http.ResponseWriter,
|
_ http.ResponseWriter,
|
||||||
r *http.Request,
|
r *http.Request,
|
||||||
v any,
|
v any,
|
||||||
) error {
|
) 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)
|
return json.NewDecoder(r.Body).Decode(v)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -30,7 +30,7 @@ type report struct {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// HandleReport returns a handler that accepts telemetry
|
// 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 {
|
func (s *Handlers) HandleReport() http.HandlerFunc {
|
||||||
type response struct {
|
type response struct {
|
||||||
Status string `json:"status"`
|
Status string `json:"status"`
|
||||||
@@ -66,9 +66,15 @@ func (s *Handlers) HandleReport() http.HandlerFunc {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// decodeErrorStatus logs a report decode failure and returns the
|
// decodeErrorStatus logs a report decode failure and returns the
|
||||||
// status to send: 413 when the body exceeded the size limit,
|
// status to send: 415 when the report was not sent as JSON, 413 when
|
||||||
// otherwise 400 for malformed JSON.
|
// the body exceeded the size limit, otherwise 400 for malformed JSON.
|
||||||
func (s *Handlers) decodeErrorStatus(err error) int {
|
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
|
var tooLarge *http.MaxBytesError
|
||||||
if errors.As(err, &tooLarge) {
|
if errors.As(err, &tooLarge) {
|
||||||
s.log.Warn("report body too large", "limit_bytes", tooLarge.Limit)
|
s.log.Warn("report body too large", "limit_bytes", tooLarge.Limit)
|
||||||
|
|||||||
@@ -27,6 +27,17 @@ type stubAppender struct {
|
|||||||
|
|
||||||
func (s stubAppender) Append(any) error { return s.err }
|
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 {
|
func newTestHandlers(buf stubAppender, out io.Writer) *handlers.Handlers {
|
||||||
return handlers.NewForTest(buf, slog.New(slog.NewJSONHandler(out, nil)))
|
return handlers.NewForTest(buf, slog.New(slog.NewJSONHandler(out, nil)))
|
||||||
}
|
}
|
||||||
@@ -83,6 +94,7 @@ func TestHandleReportAcceptsValidReports(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(tt.body),
|
strings.NewReader(tt.body),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
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) {
|
func TestHandleReportStorageFailureIsNon2xx(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
@@ -113,6 +208,7 @@ func TestHandleReportStorageFailureIsNon2xx(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
@@ -138,6 +234,7 @@ func TestHandleReportFullIs507(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
@@ -161,6 +258,7 @@ func TestHandleReportMalformedJSONIs400(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{not json`),
|
strings.NewReader(`{not json`),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
@@ -185,6 +283,8 @@ func TestHandleReportOversizeIs413(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{"clientId":"`+strings.Repeat("x", 200)+`"}`),
|
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
|
// No declared length, so only the middleware's read cap can
|
||||||
// stop this body.
|
// stop this body.
|
||||||
req.ContentLength = -1
|
req.ContentLength = -1
|
||||||
@@ -213,6 +313,7 @@ func TestHandleReportDoesNotLogRawGeo(t *testing.T) {
|
|||||||
`{"clientId":"c1","geo":{"raw":"`+sentinel+`"},"hosts":[]}`,
|
`{"clientId":"c1","geo":{"raw":"`+sentinel+`"},"hosts":[]}`,
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
@@ -245,6 +346,7 @@ func TestHandleReportLogsClientIDCutToBound(t *testing.T) {
|
|||||||
`{"clientId":"`+long+`","timestamp":"`+long+`","hosts":[]}`,
|
`{"clientId":"`+long+`","timestamp":"`+long+`","hosts":[]}`,
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
@@ -284,6 +386,7 @@ func TestHandleReportDecodeErrorLogIsBounded(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{"hosts":[{"history":[{"t":`+huge+`}]}]}`),
|
strings.NewReader(`{"hosts":[{"history":[{"t":`+huge+`}]}]}`),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
|
||||||
h.HandleReport().ServeHTTP(rec, req)
|
h.HandleReport().ServeHTTP(rec, req)
|
||||||
|
|
||||||
|
|||||||
@@ -66,6 +66,7 @@ func TestReportsAreRateLimited(t *testing.T) {
|
|||||||
http.MethodPost, "/api/v1/reports",
|
http.MethodPost, "/api/v1/reports",
|
||||||
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
strings.NewReader(`{"clientId":"c1","hosts":[]}`),
|
||||||
)
|
)
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
srv.ServeHTTP(rec, req)
|
srv.ServeHTTP(rec, req)
|
||||||
|
|
||||||
return rec.Code
|
return rec.Code
|
||||||
|
|||||||
Reference in New Issue
Block a user