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
Posture settled by the ruling on #25 and the shape it fixes: posture (b), bounded and unauthenticated. Two amendments to the definition of done:
The only caller is the same-origin reporting client (#53), and the backend is reachable only through nginx in the same container. So the wildcard CORS middleware is removed rather than narrowed: no Access-Control-Allow-Origin at all by default, with the allowlist setting kept for the case where someone hosts the page elsewhere (empty list means none). AllowedMethods question falls away with it.
Rate limiting is keyed on the client IP resolved through #19's trusted-proxy logic, which now trusts loopback, since nginx is the direct peer.
The disk cap stays as written and is the 1.0 bound; pruning is #54 (post-1.0). Unblocked; sequenced after 19.
model: claude-fable-5
Posture settled by the ruling on https://git.eeqj.de/sneak/netwatch/issues/25 and the shape it fixes: posture (b), bounded and unauthenticated. Two amendments to the definition of done:
- The only caller is the same-origin reporting client (https://git.eeqj.de/sneak/netwatch/issues/53), and the backend is reachable only through nginx in the same container. So the wildcard CORS middleware is removed rather than narrowed: no `Access-Control-Allow-Origin` at all by default, with the allowlist setting kept for the case where someone hosts the page elsewhere (empty list means none). `AllowedMethods` question falls away with it.
- Rate limiting is keyed on the client IP resolved through https://git.eeqj.de/sneak/netwatch/issues/19's trusted-proxy logic, which now trusts loopback, since nginx is the direct peer.
The disk cap stays as written and is the 1.0 bound; pruning is https://git.eeqj.de/sneak/netwatch/issues/54 (post-1.0). Unblocked; sequenced after 19.
model: claude-fable-5
sneak
was unassigned by clawbot2026-09-21 09:48:11 +02:00
clawbot
self-assigned this 2026-09-21 09:48:11 +02:00
Brief update before dispatch, for what is now on next. The definition of done above holds as amended in #20 (comment) (posture (b); no Access-Control-Allow-Origin by default, the origin allowlist setting kept, empty meaning none).
The gate is root make check and script/cibuild; the backend Makefile no longer has a check target.
Rejections (rate limit, disk cap) return the same {"status":"error"} body the report handler already uses, with the status saying why (429 for the rate limit, with Retry-After).
Default limits must never refuse the page's own reporting client (one report per browser per 60 seconds, several browsers behind one address); state the defaults and why in backend/README.md.
The disk cap counts the report files already in DATA_DIR at start, not only what this process wrote.
#52 is changing the Dockerfiles, nginx.conf and the config file's bind address at the same time; do not touch the first two, and rebase onto whichever lands first.
Model: opus-5-5
Brief update before dispatch, for what is now on `next`. The definition of done above holds as amended in https://git.eeqj.de/sneak/netwatch/issues/20#issuecomment-96550 (posture (b); no `Access-Control-Allow-Origin` by default, the origin allowlist setting kept, empty meaning none).
- The gate is root `make check` and `script/cibuild`; the backend `Makefile` no longer has a `check` target.
- Rejections (rate limit, disk cap) return the same `{"status":"error"}` body the report handler already uses, with the status saying why (429 for the rate limit, with `Retry-After`).
- Default limits must never refuse the page's own reporting client (one report per browser per 60 seconds, several browsers behind one address); state the defaults and why in `backend/README.md`.
- The disk cap counts the report files already in `DATA_DIR` at start, not only what this process wrote.
- https://git.eeqj.de/sneak/netwatch/issues/52 is changing the Dockerfiles, `nginx.conf` and the config file's bind address at the same time; do not touch the first two, and rebase onto whichever lands first.
Model: opus-5-5
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.Posture settled by the ruling on #25 and the shape it fixes: posture (b), bounded and unauthenticated. Two amendments to the definition of done:
Access-Control-Allow-Originat all by default, with the allowlist setting kept for the case where someone hosts the page elsewhere (empty list means none).AllowedMethodsquestion falls away with it.The disk cap stays as written and is the 1.0 bound; pruning is #54 (post-1.0). Unblocked; sequenced after 19.
model: claude-fable-5
Brief update before dispatch, for what is now on
next. The definition of done above holds as amended in #20 (comment) (posture (b); noAccess-Control-Allow-Originby default, the origin allowlist setting kept, empty meaning none).make checkandscript/cibuild; the backendMakefileno longer has achecktarget.{"status":"error"}body the report handler already uses, with the status saying why (429 for the rate limit, withRetry-After).backend/README.md.DATA_DIRat start, not only what this process wrote.nginx.confand the config file's bind address at the same time; do not touch the first two, and rebase onto whichever lands first.Model: opus-5-5