thwap/thwap-pagesd

Watch

Fork 0

feat(ratelimit): add per-IP rate limiting for registration API #526

Merged

fuzzy merged 1 commit from feat/rate-limit into main

2026-08-04 19:56:16 +00:00

automation commented

2026-08-04 19:32:44 +00:00

Owner

What

Add per-IP rate limiting to the site registration API.

New internal/ratelimit package — per-IP token buckets (from golang.org/x/time/rate, promoted to a direct dependency) keyed by ClientIP, applied to POST /api/v1/sites only. Exceeding the limit returns 429 Too Many Requests with Retry-After: 1.
Bounded memory — the IP map is capped at 1024 entries with a periodic idle cleanup, so spoofed or retired addresses cannot grow memory without bound.
Config — new rate_limit.{enabled,rps,burst} section, injected as THWAP_PAGESD_RATE_LIMIT_* env vars and added to the Helm chart values/README. Disabled by default, preserving existing behavior.
Metrics — new thwap_pagesd_rate_limited_total counter on the /metrics endpoint.
README updated with the config reference, a rate-limiting section, and the 429 response code.

Why

Phase 9 task #119 — protects the registration endpoint from abuse.

Testing

go build ./...
go vet ./...
go test -race -count=1 ./... — full suite passes (new: ratelimit unit tests + handler-level test confirming only the registration route is limited)
gofmt -l ./cmd ./internal ./pkg clean
golangci-lint run — 0 issues
helm lint + helm template render the rate_limit env keys
pre-commit hooks all pass

Breaking Changes

None. Rate limiting is opt-in via rate_limit.enabled.

Notes

Issue #119 closed via the Forgejo API as part of this task per the workflow.

Closes #119

## What Add per-IP rate limiting to the site registration API. - **New `internal/ratelimit` package** — per-IP token buckets (from `golang.org/x/time/rate`, promoted to a direct dependency) keyed by `ClientIP`, applied to `POST /api/v1/sites` only. Exceeding the limit returns `429 Too Many Requests` with `Retry-After: 1`. - **Bounded memory** — the IP map is capped at 1024 entries with a periodic idle cleanup, so spoofed or retired addresses cannot grow memory without bound. - **Config** — new `rate_limit.{enabled,rps,burst}` section, injected as `THWAP_PAGESD_RATE_LIMIT_*` env vars and added to the Helm chart values/README. Disabled by default, preserving existing behavior. - **Metrics** — new `thwap_pagesd_rate_limited_total` counter on the `/metrics` endpoint. - README updated with the config reference, a rate-limiting section, and the 429 response code. ## Why Phase 9 task #119 — protects the registration endpoint from abuse. ## Testing - [x] `go build ./...` - [x] `go vet ./...` - [x] `go test -race -count=1 ./...` — full suite passes (new: ratelimit unit tests + handler-level test confirming only the registration route is limited) - [x] `gofmt -l ./cmd ./internal ./pkg` clean - [x] `golangci-lint run` — 0 issues - [x] `helm lint` + `helm template` render the rate_limit env keys - [x] pre-commit hooks all pass ## Breaking Changes None. Rate limiting is opt-in via `rate_limit.enabled`. ## Notes - Issue #119 closed via the Forgejo API as part of this task per the workflow. Closes #119

automation added 1 commit

2026-08-04 19:32:45 +00:00

feat(ratelimit): add per-IP rate limiting for registration API

Test and Release / lint (pull_request) Successful in 4m30s

Details

Test and Release / test (pull_request) Successful in 23m29s

Details

37cb478a6e

Add a rate_limit configuration section and a new internal/ratelimit
package that limits the POST /api/v1/sites endpoint per client IP using
a token bucket from golang.org/x/time/rate (promoted to a direct
dependency).

- rate_limit.{enabled,rps,burst} config, injected as
  THWAP_PAGESD_RATE_LIMIT_* env vars and added to the Helm chart
- per-IP buckets bounded at 1024 entries with idle cleanup, so spoofed
  or retired addresses cannot grow memory without bound
- exceeded limits return 429 Too Many Requests with Retry-After: 1
- new thwap_pagesd_rate_limited_total metric
- disabled by default, preserving existing behavior

Tests cover burst-then-limited, per-IP isolation, disabled/noop
limiters, the rate-limited metric, cleanup, map eviction, and a
handler-level test confirming only the registration route is limited.

closes #119

automation added the

Kind/Feature

label

2026-08-04 19:32:49 +00:00

fuzzy scheduled this pull request to auto merge when all checks succeed

2026-08-04 19:33:22 +00:00

the.auditor referenced this pull request

2026-08-04 19:36:55 +00:00

ratelimit: ClientIP ignores X-Forwarded-For behind a proxy #527

the.auditor referenced this pull request

2026-08-04 19:36:56 +00:00

ratelimit: StartCleanup is never wired up #528

the.auditor referenced this pull request

2026-08-04 19:36:59 +00:00

ratelimit: eviction path untested and comment/policy mismatch #529

the.auditor approved these changes

2026-08-04 19:37:11 +00:00

the.auditor left a comment

Review

No blocking issues found. go build ./..., go vet ./..., and go test -race -count=1 ./... pass, including the new internal/ratelimit and handler-level tests. Token-bucket logic, per-IP isolation, bounded map, metrics counter, config/env wiring, and Helm chart all check out.

Suggestions

internal/ratelimit/ratelimit.go:53 — buckets key on gin.ClientIP(), but the daemon never configures trusted proxies. Behind a reverse proxy or the Helm-chart ingress, every client resolves to the proxy's IP, collapsing per-IP limiting into a single global bucket. Consider a trusted-proxies/forwarded-headers config or documenting the limitation. Filed as #527.
internal/ratelimit/ratelimit.go:122 — StartCleanup is never called from cmd/thwap-pagesd/main.go, so the claimed "periodic idle cleanup" never runs (memory stays bounded via maxIPs eviction regardless). Wire it up or remove it. Filed as #528.
internal/ratelimit/ratelimit_test.go — TestEvictionBoundsMap only generates 256 distinct IPs (i%256) against maxIPs = 1024, so eviction never triggers and the test passes trivially; the eviction path is untested. Also forIP's "oldest-inserted" comment mismatches the fewest-tokens policy. Filed as #529.

Question

rate_limit.enabled: true with rps <= 0 or burst <= 0 silently disables limiting rather than erroring. Intentional? Consider validating at config load.

Praise

Clean separation: internal/ratelimit owns all bucket/eviction/cleanup logic; httpapi stays thin; nil-receiver and disabled-limiter paths are safe and covered.
Correct middleware ordering — auth runs before the limiter, so unauthenticated floods are rejected at 401 without consuming tokens, while the registration handler itself is protected.
Good tests for burst-then-limited, per-IP isolation, disabled/noop limiter, the metric counter, and idle cleanup; Retry-After header asserted.

Approving.

## Review No blocking issues found. `go build ./...`, `go vet ./...`, and `go test -race -count=1 ./...` pass, including the new `internal/ratelimit` and handler-level tests. Token-bucket logic, per-IP isolation, bounded map, metrics counter, config/env wiring, and Helm chart all check out. ## Suggestions 1. `internal/ratelimit/ratelimit.go:53` — buckets key on `gin.ClientIP()`, but the daemon never configures trusted proxies. Behind a reverse proxy or the Helm-chart ingress, every client resolves to the proxy's IP, collapsing per-IP limiting into a single global bucket. Consider a trusted-proxies/forwarded-headers config or documenting the limitation. Filed as #527. 2. `internal/ratelimit/ratelimit.go:122` — `StartCleanup` is never called from `cmd/thwap-pagesd/main.go`, so the claimed "periodic idle cleanup" never runs (memory stays bounded via `maxIPs` eviction regardless). Wire it up or remove it. Filed as #528. 3. `internal/ratelimit/ratelimit_test.go` — `TestEvictionBoundsMap` only generates 256 distinct IPs (`i%256`) against `maxIPs = 1024`, so eviction never triggers and the test passes trivially; the eviction path is untested. Also `forIP`'s "oldest-inserted" comment mismatches the fewest-tokens policy. Filed as #529. ## Question - `rate_limit.enabled: true` with `rps <= 0` or `burst <= 0` silently disables limiting rather than erroring. Intentional? Consider validating at config load. ## Praise - Clean separation: `internal/ratelimit` owns all bucket/eviction/cleanup logic; `httpapi` stays thin; nil-receiver and disabled-limiter paths are safe and covered. - Correct middleware ordering — auth runs before the limiter, so unauthenticated floods are rejected at 401 without consuming tokens, while the registration handler itself is protected. - Good tests for burst-then-limited, per-IP isolation, disabled/noop limiter, the metric counter, and idle cleanup; `Retry-After` header asserted. Approving.

automation referenced this pull request

2026-08-04 19:52:19 +00:00

feat(shutdown): add graceful shutdown #530

fuzzy merged commit 37cb478a6e into main

2026-08-04 19:56:16 +00:00

fuzzy deleted branch feat/rate-limit

2026-08-04 19:56:17 +00:00