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
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
The backend's only write endpoint accepts unauthenticated writes from any origin on the internet, with no rate limit. Verified on
mainatfbfe1df.backend/internal/server/routes.go:27-29:backend/internal/middleware/middleware.go:113-129:Composition of the three:
HandleReportperforms no identity check.report.ClientIDis a client-supplied string with no verification — anyone can claim anyclientId.*"only for public, unauthenticated read-only APIs."POST /api/v1/reportsis neither read-only nor, ideally, unauthenticated.AllowedMethodsalso advertisesPUTandDELETE, which no route implements.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.MaxBytesReaderis applied (report.go:39-41), the decode-error path returns a generic{"status":"error"}with no internal detail leaked (report.go:50-56), andAllowCredentialsisfalse.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:
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).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/reportskeyed 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 with429/507rather than growing without bound; andAllowedOriginsnarrowed to a configurable allowlist withAllowedMethodsreduced toGET, 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))
AllowedOriginsis a configurable allowlist (env var via the existingviperconfig), not*. Document the variable inbackend/README.md.AllowedMethodsis reduced to the methods actually served:GET,POST,OPTIONS.POST /api/v1/reports, using the trusted-proxy-resolved client IP. Limits are configurable with sane defaults.internal/reportbuf, not only at the handler.cd backend && make checkpasses; rootmake checkpasses.backend/README.mddocuments every new environment variable.TODO.mdupdated in the same commit.(closes #N).Implementation requirements
GO_HTTP_SERVER_CONVENTIONS.mdfor config (viper+AutomaticEnv+SetDefault), middleware shape, and fx wiring.RemoteAddrbehind a proxy would rate-limit the proxy, not the client. Sequence this after #19.golang.org/x/time/rateis stdlib-adjacent and the right default. Consult the Go package defaults before adding anything else.