No rate limiting on the Basic-Auth-protected /metrics endpoint #101

New Issue

clawbot · 2026-08-09T03:36:47+02:00

clawbot commented

2026-08-09 03:36:47 +02:00

REPO_POLICIES.md requires "rate limiting on password-based authentication endpoints" before 1.0, and explicitly carves out only high-entropy API keys: "API keys are high-entropy and not susceptible to brute force, so they are exempt."

/metrics is protected by HTTP Basic Auth with an operator-chosen username and password (internal/server/routes.go:54-63; MetricsAuth() at internal/middleware/middleware.go:189-205; credentials from METRICS_USERNAME / METRICS_PASSWORD at internal/config/config.go:161-162). That is password-based auth, not an API key, so the exemption does not apply.

There is no rate limiting anywhere in the repository. A grep for throttle|limiter|x/time/rate|httprate|ratelimit across all *.go returns nothing, and go.mod carries no rate-limiting dependency. Credentials can be brute-forced at whatever rate the network allows.

The underlying github.com/99designs/basicauth-go does use crypto/subtle.ConstantTimeCompare, so timing attacks are already handled — this issue is specifically about attempt-rate, which constant-time comparison does nothing for.

Definition of done

Rate limiting is applied to the /metrics route group using github.com/go-chi/httprate — this is the decided default for HTTP rate limiting in Go repos; do not hand-roll a limiter and do not substitute a different library.
The dependency is added via go get and go mod tidy is run, so go.mod and go.sum are both updated and the module hash is recorded in go.sum (this satisfies the hash-pinning rule for Go modules).
The limiter keys on the real client IP. Note that internal/middleware/middleware.go:110-170 already implements correct trusted-proxy-gated client IP extraction (realIP(), honouring X-Real-IP / X-Forwarded-For only when RemoteAddr is inside RFC1918 / loopback / ULA). Reuse that logic — do not use httprate.KeyByIP directly if it would trust forwarded headers unconditionally, and do not duplicate the trusted-proxy list.
The limit is applied before the Basic Auth middleware in the chain, so rejected requests never reach credential comparison.
Rate-limited requests get 429 Too Many Requests. The response body must not leak whether the username or the password was wrong, or whether /metrics is even configured.
The limit and window are named constants with a comment explaining the choice. They must be permissive enough that a normal Prometheus scrape interval (e.g. every 15s from one scraper) is never throttled — state that reasoning in the comment.
Tests assert: repeated failed auth attempts from one IP eventually receive 429; a request from a different IP is unaffected by another IP's exhausted budget; and a normal scrape cadence is not throttled. Tests must not sleep for the full window — make the window injectable or keep it short enough that the suite stays well inside the repo's 20-second make test ceiling.
README documents the rate limit alongside the METRICS_USERNAME / METRICS_PASSWORD documentation.
make check is green, and TODO.md is updated in the same commit as the work.

The finishing commit's title must end with (closes #N) referencing this issue.

Notes

/metrics is not mounted at all when METRICS_USERNAME is empty (internal/server/routes.go:54), so there is no unauthenticated-by-default exposure to fix — this issue is only about throttling attempts when the endpoint is enabled.

Out of scope

Security response headers (#98), http.Server timeouts (#99), and CORS scoping (#100) are tracked separately. Rate limiting the public read-only routes is not required by policy and is not in scope here.

`REPO_POLICIES.md` requires "rate limiting on password-based authentication endpoints" before 1.0, and explicitly carves out only high-entropy API keys: "API keys are high-entropy and not susceptible to brute force, so they are exempt." `/metrics` is protected by HTTP **Basic Auth** with an operator-chosen username and password (`internal/server/routes.go:54-63`; `MetricsAuth()` at `internal/middleware/middleware.go:189-205`; credentials from `METRICS_USERNAME` / `METRICS_PASSWORD` at `internal/config/config.go:161-162`). That is password-based auth, not an API key, so the exemption does not apply. There is no rate limiting anywhere in the repository. A grep for `throttle|limiter|x/time/rate|httprate|ratelimit` across all `*.go` returns nothing, and `go.mod` carries no rate-limiting dependency. Credentials can be brute-forced at whatever rate the network allows. The underlying `github.com/99designs/basicauth-go` does use `crypto/subtle.ConstantTimeCompare`, so timing attacks are already handled — this issue is specifically about **attempt-rate**, which constant-time comparison does nothing for. ## Definition of done 1. Rate limiting is applied to the `/metrics` route group using `github.com/go-chi/httprate` — this is the decided default for HTTP rate limiting in Go repos; do not hand-roll a limiter and do not substitute a different library. 2. The dependency is added via `go get` and `go mod tidy` is run, so `go.mod` and `go.sum` are both updated and the module hash is recorded in `go.sum` (this satisfies the hash-pinning rule for Go modules). 3. The limiter keys on the real client IP. Note that `internal/middleware/middleware.go:110-170` already implements correct trusted-proxy-gated client IP extraction (`realIP()`, honouring `X-Real-IP` / `X-Forwarded-For` **only** when `RemoteAddr` is inside RFC1918 / loopback / ULA). Reuse that logic — do **not** use `httprate.KeyByIP` directly if it would trust forwarded headers unconditionally, and do not duplicate the trusted-proxy list. 4. The limit is applied **before** the Basic Auth middleware in the chain, so rejected requests never reach credential comparison. 5. Rate-limited requests get `429 Too Many Requests`. The response body must not leak whether the username or the password was wrong, or whether `/metrics` is even configured. 6. The limit and window are named constants with a comment explaining the choice. They must be permissive enough that a normal Prometheus scrape interval (e.g. every 15s from one scraper) is never throttled — state that reasoning in the comment. 7. Tests assert: repeated failed auth attempts from one IP eventually receive `429`; a request from a different IP is unaffected by another IP's exhausted budget; and a normal scrape cadence is not throttled. Tests must not sleep for the full window — make the window injectable or keep it short enough that the suite stays well inside the repo's 20-second `make test` ceiling. 8. README documents the rate limit alongside the `METRICS_USERNAME` / `METRICS_PASSWORD` documentation. 9. `make check` is green, and `TODO.md` is updated in the same commit as the work. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Notes `/metrics` is not mounted at all when `METRICS_USERNAME` is empty (`internal/server/routes.go:54`), so there is no unauthenticated-by-default exposure to fix — this issue is only about throttling attempts when the endpoint **is** enabled. ## Out of scope Security response headers (#98), `http.Server` timeouts (#99), and CORS scoping (#100) are tracked separately. Rate limiting the public read-only routes is **not** required by policy and is not in scope here.

clawbot added this to the 1.0 milestone 2026-08-09 03:36:47 +02:00

clawbot referenced this issue

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

internal/globals, internal/healthcheck, and internal/logger have no tests at all #110

clawbot referenced this issue

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

1.0/mvp #66

clawbot referenced this issue

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

feat: add security response headers middleware (closes #98) #112

clawbot referenced this issue

2026-08-09 07:36:00 +02:00