Skip to content

feat: rate-limit public reads and deploy to Fly.io on merge - #122

Merged
koinsaari merged 3 commits into
mainfrom
feat/fly-deploy
Aug 1, 2026
Merged

feat: rate-limit public reads and deploy to Fly.io on merge#122
koinsaari merged 3 commits into
mainfrom
feat/fly-deploy

Conversation

@koinsaari

Copy link
Copy Markdown
Contributor

Summary

  • Adds a per-IP rate limiter (20 req/s, burst 20) for public GET endpoints, which previously had no app-layer limiting at all.
  • Adds fly.toml and a CI job that deploys cmd/api to Fly.io, gated on all required checks passing, only on push to main.

🤖 Generated with Claude Code

GET /v1/places and GET /v1/places/{id} had no rate limiting at the app
layer, only authenticated writes did. Adds a per-IP token-bucket
limiter (20 req/s, burst 20) ahead of the OpenAPI validator, skipped
for requests carrying X-API-Key since those already have their own
per-key limiter.
Adds fly.toml (cmd/api, Frankfurt region, scale-to-zero) and a CI job
that runs flyctl deploy only after all required checks pass on push to
main, so a failing build/test/lint/scan never reaches production.

Also ignores the local Supabase CLI's session cache directory.
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 2m 22s —— View job


Review

  • Read REVIEW.md and CLAUDE.md for review guidelines
  • Review diff against REVIEW.md checklist
  • Post findings and set commit status

🔴 Important

Public rate limiter is bypassed by sending any X-API-Key header, valid or notinternal/middleware/public_rate_limit.go:16

if r.Header.Get("X-API-Key") == "" && !rl.Allow(ClientIP(r)) {

This only checks whether the header is present, not whether it authenticates. middleware.PublicRateLimit wraps v1Handler and runs before nethttp_middleware.OapiRequestValidatorWithOptions/srv.authenticate in cmd/api/main.go:143 (validator + authenticate is applied first when building v1Handler, then PublicRateLimit wraps the result — but per api/openapi.yaml:13, security: [] by default, and ListPlaces/GetPlace don't override it, so authenticate is never invoked for these routes at all). That means an unauthenticated client can defeat the new limiter entirely just by sending X-API-Key: anything on every GET request — no valid key required, since it's never checked for these endpoints. This directly undermines the stated purpose of the PR ("previously had no app-layer limiting at all" for public reads): the limiter is trivially bypassable and provides no real protection.

Fix: gate the skip on a successful authentication result (e.g. check middleware.APIKeyIDFromCtx(r.Context()) post-auth, or move this limiter to run after authenticate and key off whether a valid key was attached), not merely on header presence. Fix this →

🟡 Nits

  • fly.toml sets both memory = '1gb' and memory_mb = 1024 under [[vm]] — redundant (leftover from fly launch autogen); pick one.
  • .gitignore adds supabase/.temp/, which looks unrelated to this PR's stated scope (rate limiting / Fly deploy).
  • The 429 response in public_rate_limit.go doesn't set a Retry-After header, even though RateLimiter.RetryAfterSeconds() already exists and is presumably intended for this.

No other issues found — SQL/GORM usage, JSONB types, and the a11y/validation ordering invariants are untouched by this diff. ClientIP correctly ignores spoofable X-Forwarded-For and keys on RemoteAddr only.


Note: I was unable to set the claude-code-review commit status on HEAD SHA 2b6d1475e0938bb78f97458968014c67a0e3e734 — the GitHub API returned 403 Resource not accessible by integration. This review reported an Important finding, so the status should be failure once permissions allow it.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 1m 48s —— View job


Review

  • Read REVIEW.md and CLAUDE.md for review guidelines
  • Review diff against REVIEW.md checklist (re-review after fix commit 17edf14)
  • Post findings and set commit status

The 🔴 Important finding from the previous review has been fixed in 17edf14:

  • Bypass fixedinternal/middleware/public_rate_limit.go:17 now gates the skip on APIKeyIDFromCtx(r.Context()) == "" instead of raw header presence. Since APIKeyIDFromCtx is only populated by srv.authenticate (cmd/api/auth.go:48) after a key hash actually resolves to a non-revoked row, a bare/garbage X-API-Key header no longer bypasses the limiter. Middleware ordering is correct: OapiRequestValidatorWithOptions (which invokes authenticate when a route has a security requirement) wraps PublicRateLimit, which wraps v1Mux (cmd/api/main.go:174-182), so any context enrichment from authenticate is visible by the time PublicRateLimit runs.
  • Good addition: cmd/api/rate_limit_wiring_test.go exercises the real composed handler chain from buildV1Handler (not just the middleware in isolation) and asserts a garbage X-API-Key on GET /v1/places still gets 429 on the second request — this is exactly the regression that would have caught the original bug.
  • internal/middleware/public_rate_limit_test.go also gained TestPublicRateLimit_DoesNotTrustUnverifiedAPIKeyHeader, covering the unit-level case directly.

All 🟡 Nits from the previous pass are resolved except one minor, non-blocking item:

  • fly.toml's redundant memory_mb is removed — fixed.
  • 429 responses now set Retry-After via rl.RetryAfterSeconds() — fixed.
  • .gitignore's supabase/.temp/ addition is still unrelated to this PR's scope, but it's trivial and not worth blocking on.

No other issues found. No blocking issues.


Note: I was unable to set the claude-code-review commit status on HEAD SHA 17edf14f278599e15b83ed08084a246083185fcc — the GitHub API returned 403 Resource not accessible by integration. Since no Important findings remain, the status should be success once permissions allow it.

Ran before the OpenAPI validator/authenticate and only checked header
presence, not validity. Now runs after the validator, keyed off
APIKeyIDFromCtx, which is only set once a key is verified. Extracts
buildV1Handler so a test covers the real composed chain instead of a
hand-copied approximation. Also drops the redundant memory_mb in
fly.toml.
@InWheelOrg InWheelOrg deleted a comment from claude Bot Aug 1, 2026
@koinsaari
koinsaari enabled auto-merge (squash) August 1, 2026 17:50
@koinsaari
koinsaari disabled auto-merge August 1, 2026 17:50
@koinsaari
koinsaari merged commit 43da8a8 into main Aug 1, 2026
7 of 10 checks passed
@koinsaari
koinsaari deleted the feat/fly-deploy branch August 1, 2026 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant