Report ingest correctness: returns 200 on storage failure, 400 on oversize, logs untrusted body #23

New Issue

clawbot · 2026-08-09T03:42:45+02:00

clawbot commented

2026-08-09 03:42:45 +02:00

Problem

Four defects in POST /api/v1/reports and the surrounding handler layer. Verified on main at fbfe1df.

1. Returns `200 {"status":"ok"}` when the report was NOT stored

backend/internal/handlers/report.go:70-80:

bufErr := s.buf.Append(rpt)
if bufErr != nil {
    s.log.Error("failed to buffer report", "error", bufErr)
}

s.respondJSON(w, r, &response{Status: "ok"}, http.StatusOK)

The error is logged and then discarded. The client is told its report was accepted when it was dropped. Any client-side retry logic is defeated, and the failure is invisible to everything except whoever is reading server logs.

The same pattern exists on the shutdown path: internal/reportbuf/reportbuf.go:78-83's OnStop ignores the result of the final flush, and writeFile (reportbuf.go:161) only logs. A failed final flush loses data silently and the process still exits 0. (The lifecycle half of that is tracked in #22; the error-propagation half belongs here.)

2. Oversize bodies return `400`, not `413`

report.go:39-56: http.MaxBytesReader is correctly applied, but the decode error path never distinguishes a *http.MaxBytesError from malformed JSON. Both produce 400 Bad Request. A client that sent a valid but too-large report gets told its JSON is broken. CODE_STYLEGUIDE_GO.md shows the intended http.StatusRequestEntityTooLarge behaviour.

3. Body size limit is per-route, not global

Policy requires a maximum request body size "enforced on all endpoints". MaxBytesReader appears only in report.go. There is no body-limit middleware in internal/middleware/middleware.go and none registered in routes.go. The healthcheck route and every future route accept unbounded input.

4. Raw untrusted request body is logged at Info level

report.go:62-68 logs "geo", string(rpt.Geo) — the raw, unvalidated, attacker-controlled json.RawMessage declared at report.go:25, up to the full 1 MiB body cap, on every request. client_id and timestamp are logged verbatim too.

This is a log-injection and log-volume amplification vector: an attacker controls both the content and the size of what lands in the log pipeline, at 1 MiB per request, with no authentication (#20).

Also in scope — trivial, same files

No decodeJSON helper. GO_HTTP_SERVER_CONVENTIONS.md defines decodeJSON alongside respondJSON as a base handler helper. handlers.go:59-73 defines only respondJSON; report.go:44 decodes inline.
chi middleware.Recoverer writes panic stack traces to stderr as unstructured plain text, bypassing slog. The client correctly gets a bare 500 with no body — that part is fine — but the stack should go through the structured logger.

Not defects, do not regress them: the client-facing error body is already clean ({"status":"error"}, no internals leaked), and Content-Type is correctly set before WriteHeader in handlers.go:65-66.

Definition of done

A storage failure results in a non-2xx response. The client is never told "ok" for a report that was not persisted.
Oversize bodies return 413; malformed JSON returns 400. Distinguish via errors.As on *http.MaxBytesError.
A body-size-limit middleware is applied to all routes, with the per-route limit remaining configurable where a route needs a different bound.
The raw geo blob is no longer logged. If geo data must be observable, log a bounded, validated projection of it (for example a length, or specific parsed fields), never the raw attacker-controlled bytes. Apply the same reasoning to client_id: bound its logged length.
Error responses still leak nothing internal — no paths, no buffer state, no sizes.
decodeJSON helper added to handlers.go and used by report.go.
Panic output from Recoverer is routed through slog as structured JSON rather than unstructured stderr text.
Tests cover each behaviour change: storage failure -> non-2xx; oversize -> 413; malformed -> 400; oversize on a non-report route is also rejected. Use httptest.
cd backend && make check passes; root make check passes.
TODO.md updated in the same commit.
Commit title ends with (closes #N).

Implementation requirements

Follow GO_HTTP_SERVER_CONVENTIONS.md for the middleware and handler-helper shapes, and CODE_STYLEGUIDE_GO.md for error handling.
Do not change the CORS posture, add authentication, or add rate limiting here — that is #20.
Do not change the shutdown lifecycle here — that is #22. This issue covers only the error-propagation aspect of the write path.
Choosing the failure status code for item 1 is a judgement call; state your choice and reasoning in the PR description.
make targets only; never raw go invocations.
No attribution trailers in the commit message.

## Problem Four defects in `POST /api/v1/reports` and the surrounding handler layer. Verified on `main` at `fbfe1df`. ### 1. Returns `200 {"status":"ok"}` when the report was NOT stored `backend/internal/handlers/report.go:70-80`: ```go bufErr := s.buf.Append(rpt) if bufErr != nil { s.log.Error("failed to buffer report", "error", bufErr) } s.respondJSON(w, r, &response{Status: "ok"}, http.StatusOK) ``` The error is logged and then **discarded**. The client is told its report was accepted when it was dropped. Any client-side retry logic is defeated, and the failure is invisible to everything except whoever is reading server logs. The same pattern exists on the shutdown path: `internal/reportbuf/reportbuf.go:78-83`'s `OnStop` ignores the result of the final flush, and `writeFile` (`reportbuf.go:161`) only logs. A failed final flush loses data silently and the process still exits `0`. (The lifecycle half of that is tracked in #22; the error-propagation half belongs here.) ### 2. Oversize bodies return `400`, not `413` `report.go:39-56`: `http.MaxBytesReader` is correctly applied, but the decode error path never distinguishes a `*http.MaxBytesError` from malformed JSON. Both produce `400 Bad Request`. A client that sent a valid but too-large report gets told its JSON is broken. `CODE_STYLEGUIDE_GO.md` shows the intended `http.StatusRequestEntityTooLarge` behaviour. ### 3. Body size limit is per-route, not global Policy requires a maximum request body size "enforced **on all endpoints**". `MaxBytesReader` appears only in `report.go`. There is no body-limit middleware in `internal/middleware/middleware.go` and none registered in `routes.go`. The healthcheck route and every future route accept unbounded input. ### 4. Raw untrusted request body is logged at Info level `report.go:62-68` logs `"geo", string(rpt.Geo)` — the raw, unvalidated, attacker-controlled `json.RawMessage` declared at `report.go:25`, up to the full 1 MiB body cap, on every request. `client_id` and `timestamp` are logged verbatim too. This is a log-injection and log-volume amplification vector: an attacker controls both the content and the size of what lands in the log pipeline, at 1 MiB per request, with no authentication (#20). ### Also in scope — trivial, same files - **No `decodeJSON` helper.** `GO_HTTP_SERVER_CONVENTIONS.md` defines `decodeJSON` alongside `respondJSON` as a base handler helper. `handlers.go:59-73` defines only `respondJSON`; `report.go:44` decodes inline. - **`chi middleware.Recoverer` writes panic stack traces to stderr as unstructured plain text**, bypassing slog. The client correctly gets a bare 500 with no body — that part is fine — but the stack should go through the structured logger. Not defects, do not regress them: the client-facing error body is already clean (`{"status":"error"}`, no internals leaked), and `Content-Type` is correctly set before `WriteHeader` in `handlers.go:65-66`. ## Definition of done - [ ] A storage failure results in a non-2xx response. The client is never told "ok" for a report that was not persisted. - [ ] Oversize bodies return `413`; malformed JSON returns `400`. Distinguish via `errors.As` on `*http.MaxBytesError`. - [ ] A body-size-limit middleware is applied to all routes, with the per-route limit remaining configurable where a route needs a different bound. - [ ] The raw `geo` blob is no longer logged. If geo data must be observable, log a bounded, validated projection of it (for example a length, or specific parsed fields), never the raw attacker-controlled bytes. Apply the same reasoning to `client_id`: bound its logged length. - [ ] Error responses still leak nothing internal — no paths, no buffer state, no sizes. - [ ] `decodeJSON` helper added to `handlers.go` and used by `report.go`. - [ ] Panic output from `Recoverer` is routed through slog as structured JSON rather than unstructured stderr text. - [ ] Tests cover each behaviour change: storage failure -> non-2xx; oversize -> 413; malformed -> 400; oversize on a non-report route is also rejected. Use `httptest`. - [ ] `cd backend && make check` passes; root `make check` passes. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Implementation requirements - Follow `GO_HTTP_SERVER_CONVENTIONS.md` for the middleware and handler-helper shapes, and `CODE_STYLEGUIDE_GO.md` for error handling. - Do not change the CORS posture, add authentication, or add rate limiting here — that is #20. - Do not change the shutdown lifecycle here — that is #22. This issue covers only the error-propagation aspect of the write path. - Choosing the failure status code for item 1 is a judgement call; state your choice and reasoning in the PR description. - `make` targets only; never raw `go` invocations. - No attribution trailers in the commit message.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:45 +02:00

clawbot referenced this issue

2026-08-09 03:43:57 +02:00

Decision needed: the backend is not connected to anything — does it ship in 1.0? #25

clawbot referenced this issue

2026-08-09 03:45:07 +02:00

Backend observability: fx logs bypass slog, Identify() is dead, Sentry/Prometheus config is parsed but unwired #27

Sign in to join this conversation.