POST /api/v1/reports is unauthenticated, wildcard-CORS, and unrated-limited #20

New Issue

clawbot · 2026-08-09T03:40:06+02:00

clawbot commented

2026-08-09 03:40:06 +02:00

Problem

The backend's only write endpoint accepts unauthenticated writes from any origin on the internet, with no rate limit. Verified on main at fbfe1df.

backend/internal/server/routes.go:27-29:

s.router.Route("/api/v1", func(r chi.Router) {
    r.Post("/reports", s.h.HandleReport())
})

backend/internal/middleware/middleware.go:113-129:

func (s *Middleware) CORS() func(http.Handler) http.Handler {
    return cors.Handler(cors.Options{
        AllowedOrigins: []string{"*"},
        AllowedMethods: []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"},
        ...

Composition of the three:

No authentication. HandleReport performs no identity check. report.ClientID is a client-supplied string with no verification — anyone can claim any clientId.
Wildcard CORS on a state-mutating endpoint. Policy permits * "only for public, unauthenticated read-only APIs." POST /api/v1/reports is neither read-only nor, ideally, unauthenticated. AllowedMethods also advertises PUT and DELETE, which no route implements.
No rate limiting. Each accepted request appends to the zstd report buffer (backend/internal/handlers/report.go:71 -> s.buf.Append(rpt)), which is backed by disk. An unauthenticated attacker can drive unbounded disk growth at 1 MiB per request (maxReportBodyBytes), which is a remote denial-of-service and a storage-cost attack.

What is already correct and should not be regressed: http.MaxBytesReader is applied (report.go:39-41), the decode-error path returns a generic {"status":"error"} with no internal detail leaked (report.go:50-56), and AllowCredentials is false.

This needs a decision before implementation

The right answer depends on how NetWatch is meant to be deployed, which is not recorded anywhere in the repo. Three viable postures:

(a) Shared-secret ingest token. Backend requires Authorization: Bearer <token>; the token is baked into the frontend build. Stops casual abuse, but the token is public by construction since it ships in a browser bundle. CORS narrows to the known frontend origin(s).
(b) Keep it open, but bound it. No auth; add per-IP rate limiting plus a global write budget and buffer size cap. Honest about the fact that a browser-delivered credential is not a secret, and defends the actual risk (disk exhaustion).
(c) Private deployment only. Backend is never internet-exposed; it binds to a private network and the reverse proxy restricts source IPs. Document this as a hard deployment constraint and drop the ingest endpoint from the public surface entirely.

Recommendation: (b), with CORS narrowed. The frontend is a static SPA served to arbitrary browsers, so any credential it carries is public — (a) buys the appearance of auth without the substance. (c) is the most secure but forecloses the hosted use case the frontend's design implies. (b) defends the real threat (unbounded disk growth) without pretending the endpoint is authenticated.

Under (b) the concrete requirements would be: per-IP token-bucket rate limiting on POST /api/v1/reports keyed off the trusted-proxy-resolved client IP from #19; a configurable absolute cap on total buffered report bytes on disk, past which writes are rejected with 429/507 rather than growing without bound; and AllowedOrigins narrowed to a configurable allowlist with AllowedMethods reduced to GET, POST, OPTIONS.

@sneak — please pick a posture, or confirm (b). Assigning to you for that decision. Implementation is blocked until it is made; the definition of done below is written for (b) and should be amended if you choose otherwise.

Definition of done (assuming posture (b))

AllowedOrigins is a configurable allowlist (env var via the existing viper config), not *. Document the variable in backend/README.md.
AllowedMethods is reduced to the methods actually served: GET, POST, OPTIONS.
Per-IP rate limiting on POST /api/v1/reports, using the trusted-proxy-resolved client IP. Limits are configurable with sane defaults.
A configurable maximum total on-disk size for the report buffer. When exceeded, the endpoint rejects writes with an appropriate status instead of growing unbounded. This must be enforced in internal/reportbuf, not only at the handler.
Rejection responses leak nothing internal — no paths, no sizes, no buffer state.
Unit tests cover: rate limit triggers and resets; the disk cap rejects writes; a disallowed origin is refused; an allowed origin passes.
cd backend && make check passes; root make check passes.
backend/README.md documents every new environment variable.
TODO.md updated in the same commit.
Commit title ends with (closes #N).

Implementation requirements

Follow GO_HTTP_SERVER_CONVENTIONS.md for config (viper + AutomaticEnv + SetDefault), middleware shape, and fx wiring.
Depends on #19 for correct client IP resolution — rate limiting keyed on RemoteAddr behind a proxy would rate-limit the proxy, not the client. Sequence this after #19.
Prefer a well-maintained rate-limiter over hand-rolling one; golang.org/x/time/rate is stdlib-adjacent and the right default. Consult the Go package defaults before adding anything else.
No attribution trailers in the commit message.

## Problem The backend's only write endpoint accepts unauthenticated writes from any origin on the internet, with no rate limit. Verified on `main` at `fbfe1df`. `backend/internal/server/routes.go:27-29`: ```go s.router.Route("/api/v1", func(r chi.Router) { r.Post("/reports", s.h.HandleReport()) }) ``` `backend/internal/middleware/middleware.go:113-129`: ```go func (s *Middleware) CORS() func(http.Handler) http.Handler { return cors.Handler(cors.Options{ AllowedOrigins: []string{"*"}, AllowedMethods: []string{"GET", "POST", "PUT", "DELETE", "OPTIONS"}, ... ``` Composition of the three: 1. **No authentication.** `HandleReport` performs no identity check. `report.ClientID` is a client-supplied string with no verification — anyone can claim any `clientId`. 2. **Wildcard CORS on a state-mutating endpoint.** Policy permits `*` "only for public, unauthenticated read-only APIs." `POST /api/v1/reports` is neither read-only nor, ideally, unauthenticated. `AllowedMethods` also advertises `PUT` and `DELETE`, which no route implements. 3. **No rate limiting.** Each accepted request appends to the zstd report buffer (`backend/internal/handlers/report.go:71` -> `s.buf.Append(rpt)`), which is backed by disk. An unauthenticated attacker can drive unbounded disk growth at 1 MiB per request (`maxReportBodyBytes`), which is a remote denial-of-service and a storage-cost attack. What is already correct and should not be regressed: `http.MaxBytesReader` is applied (`report.go:39-41`), the decode-error path returns a generic `{"status":"error"}` with no internal detail leaked (`report.go:50-56`), and `AllowCredentials` is `false`. ## This needs a decision before implementation The right answer depends on how NetWatch is meant to be deployed, which is not recorded anywhere in the repo. Three viable postures: - **(a) Shared-secret ingest token.** Backend requires `Authorization: Bearer <token>`; the token is baked into the frontend build. Stops casual abuse, but the token is public by construction since it ships in a browser bundle. CORS narrows to the known frontend origin(s). - **(b) Keep it open, but bound it.** No auth; add per-IP rate limiting plus a global write budget and buffer size cap. Honest about the fact that a browser-delivered credential is not a secret, and defends the actual risk (disk exhaustion). - **(c) Private deployment only.** Backend is never internet-exposed; it binds to a private network and the reverse proxy restricts source IPs. Document this as a hard deployment constraint and drop the ingest endpoint from the public surface entirely. **Recommendation: (b), with CORS narrowed.** The frontend is a static SPA served to arbitrary browsers, so any credential it carries is public — (a) buys the appearance of auth without the substance. (c) is the most secure but forecloses the hosted use case the frontend's design implies. (b) defends the real threat (unbounded disk growth) without pretending the endpoint is authenticated. Under (b) the concrete requirements would be: per-IP token-bucket rate limiting on `POST /api/v1/reports` keyed off the trusted-proxy-resolved client IP from #19; a configurable absolute cap on total buffered report bytes on disk, past which writes are rejected with `429`/`507` rather than growing without bound; and `AllowedOrigins` narrowed to a configurable allowlist with `AllowedMethods` reduced to `GET, POST, OPTIONS`. @sneak — please pick a posture, or confirm (b). Assigning to you for that decision. Implementation is blocked until it is made; the definition of done below is written for (b) and should be amended if you choose otherwise. ## Definition of done (assuming posture (b)) - [ ] `AllowedOrigins` is a configurable allowlist (env var via the existing `viper` config), not `*`. Document the variable in `backend/README.md`. - [ ] `AllowedMethods` is reduced to the methods actually served: `GET`, `POST`, `OPTIONS`. - [ ] Per-IP rate limiting on `POST /api/v1/reports`, using the trusted-proxy-resolved client IP. Limits are configurable with sane defaults. - [ ] A configurable maximum total on-disk size for the report buffer. When exceeded, the endpoint rejects writes with an appropriate status instead of growing unbounded. This must be enforced in `internal/reportbuf`, not only at the handler. - [ ] Rejection responses leak nothing internal — no paths, no sizes, no buffer state. - [ ] Unit tests cover: rate limit triggers and resets; the disk cap rejects writes; a disallowed origin is refused; an allowed origin passes. - [ ] `cd backend && make check` passes; root `make check` passes. - [ ] `backend/README.md` documents every new environment variable. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Implementation requirements - Follow `GO_HTTP_SERVER_CONVENTIONS.md` for config (`viper` + `AutomaticEnv` + `SetDefault`), middleware shape, and fx wiring. - Depends on #19 for correct client IP resolution — rate limiting keyed on `RemoteAddr` behind a proxy would rate-limit the proxy, not the client. Sequence this after #19. - Prefer a well-maintained rate-limiter over hand-rolling one; `golang.org/x/time/rate` is stdlib-adjacent and the right default. Consult the Go package defaults before adding anything else. - No attribution trailers in the commit message.

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

clawbot referenced this issue

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

Shutdown is broken: os.Exit races fx OnStop hooks, losing buffered reports on every restart #22

clawbot referenced this issue

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

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

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

sneak was assigned by clawbot

2026-08-09 03:44:11 +02:00