Skip to content

fix: always rate limit, fall back to client IP when header absent#447

Merged
VaibhavAcharya merged 6 commits into
masterfrom
vaibhavacharya/ex-1881-identity-rate-limiting-bypassable-when-header-absent-gotrue
May 28, 2026
Merged

fix: always rate limit, fall back to client IP when header absent#447
VaibhavAcharya merged 6 commits into
masterfrom
vaibhavacharya/ex-1881-identity-rate-limiting-bypassable-when-header-absent-gotrue

Conversation

@VaibhavAcharya

Copy link
Copy Markdown
Contributor

- Summary

limitHandler only rate-limited when RateLimitHeader was configured AND the client sent that header, which meant unset config disabled rate limiting entirely and missing headers shared one empty-string bucket. This always rate-limits, preferring the configured header and falling back to the client IP via net.SplitHostPort(req.RemoteAddr) when the header is absent. chi/middleware.RealIP already rewrites RemoteAddr from X-Forwarded-For, so this stays correct behind a proxy.

- Test plan

- Description for the changelog

Always rate limit requests, falling back to the client IP when the configured header is missing.

- A picture of a cute animal (not mandatory but encouraged)

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 30393d20-5b55-4674-9d15-3248e1606f22

📥 Commits

Reviewing files that changed from the base of the PR and between a04c6d7 and 34caae4.

📒 Files selected for processing (2)
  • api/middleware.go
  • api/middleware_ratelimit_test.go

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Rate limiting now supports configurable header-based request identification, falling back to client IP when the header is not set or empty.
  • Tests

    • Added comprehensive test coverage for rate limit key derivation logic.

Walkthrough

This pull request refactors the rate-limiting middleware to extract bucket-key derivation into a unified rateLimitKey helper method. The method checks for a configured request header (when set and non-empty) and falls back to the client IP extracted from req.RemoteAddr via net.SplitHostPort. The rate limit handler now calls this helper and uses tooManyRequestsError instead of manually constructing an httpError with status 429. A comprehensive test suite validates header preference, fallback behavior, IPv6 normalization, and edge cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly summarizes the main change: always rate limiting with fallback to client IP when header is absent, which matches the primary objective.
Description check ✅ Passed The description is directly related to the changeset, explaining the previous behavior, new behavior, and technical details of the fix including proxy handling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch vaibhavacharya/ex-1881-identity-rate-limiting-bypassable-when-header-absent-gotrue

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


Comment @coderabbitai help to get the list of available commands and usage tips.

@VaibhavAcharya VaibhavAcharya marked this pull request as ready for review May 26, 2026 11:35
@VaibhavAcharya VaibhavAcharya requested a review from a team as a code owner May 26, 2026 11:35
@VaibhavAcharya VaibhavAcharya enabled auto-merge (squash) May 28, 2026 07:51
@VaibhavAcharya VaibhavAcharya merged commit 153875e into master May 28, 2026
7 checks passed
@VaibhavAcharya VaibhavAcharya deleted the vaibhavacharya/ex-1881-identity-rate-limiting-bypassable-when-header-absent-gotrue branch May 28, 2026 07:54
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.

2 participants