fix(security): trust proxy hop so IP rate limits key on real client IP (#316)#328
Open
dmytrocraft wants to merge 3 commits into
Open
fix(security): trust proxy hop so IP rate limits key on real client IP (#316)#328dmytrocraft wants to merge 3 commits into
dmytrocraft wants to merge 3 commits into
Conversation
#316) IP-keyed rate limiters (signin_ip, twofa_verification_ip, registration, refresh_token, email_confirmation, password_reset_confirm, oauth_social_*, user_collection, resend_confirmation, global_api_*) built their key from Request::getClientIp(). Production framework.yaml never configured trusted_proxies/trusted_headers, so behind Caddy/FrankenPHP and the AWS App Runner load balancer getClientIp() returned REMOTE_ADDR — the LB's internal IP, identical for every external client. All per-IP buckets therefore collapsed onto one shared bucket (CWE-307): one noisy client could lock out the whole tenant and no attacker could be isolated. Fix: explicitly configure framework.trusted_proxies and framework.trusted_headers, env-driven with prod-safe defaults (TRUSTED_PROXIES=REMOTE_ADDR trusts a single directly-connected hop, TRUSTED_HEADERS=x-forwarded-for trusts only X-Forwarded-For). getClientIp() now returns the real client IP; forged X-Forwarded-For from untrusted clients is ignored and other X-Forwarded-* headers are not trusted. Operators override TRUSTED_PROXIES with the exact proxy CIDR in production. Pinned the same header set in the test env config and added regression unit tests covering the positive, spoof, and no-trust paths. Local verification: - PHPUnit Unit (filter RateLimit): OK (143 tests, 258 assertions) - PHPUnit Unit (full): OK (2262 tests, 6215 assertions) - Deptrac: Errors 0 - Psalm: No errors found - PHP CS Fixer: 0 of 1 files Closes #316 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR addresses a security issue ( ChangesRate limiter IP spoofing fix (
Sequence DiagramsequenceDiagram
participant Client
participant AppRunner as App Runner LB
participant Caddy as FrankenPHP/Caddy
participant Framework as Symfony Request
participant Limiter as Rate Limiter
Client->>AppRunner: Request from 203.0.113.50
AppRunner->>Caddy: Forward request<br/>REMOTE_ADDR=10.0.0.1 (LB IP)<br/>X-Forwarded-For=203.0.113.50
Caddy->>Framework: Pass request
Framework->>Framework: Check trusted_proxies<br/>(configured to 10.0.0.1)
Framework->>Framework: Extract real IP from XFF<br/>getClientIp() = 203.0.113.50
Framework->>Limiter: Use real client IP as bucket key
Limiter->>Limiter: Apply per-IP rate limit<br/>for 203.0.113.50 only
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 5/5
- This looks low risk to merge: the only finding is a low-severity (3/10) documentation/story inconsistency rather than a functional code regression.
- The most notable issue is in
specs/security-316-rate-limit-untrusted-client-ip/stories.md, where the Docker tagsecfix-312-php:latestappears to be a copy-paste mistake and should align with PR #316 naming. - Pay close attention to
specs/security-316-rate-limit-untrusted-client-ip/stories.md- fix the mismatched Docker image tag to avoid confusion during verification.
Architecture diagram
sequenceDiagram
participant Client as External Client
participant LB as AWS App Runner LB
participant Proxy as Caddy/FrankenPHP
participant App as Symfony App
participant RateLimit as Rate Limit Resolvers
participant Cache as Rate Limit Bucket Store
Note over Client,Cache: IP-keyed Rate Limiting Behind Trusted Proxy
Client->>LB: HTTP request (no XFF)
LB->>Proxy: Forward request (may inject XFF)
Proxy->>App: Forward request with X-Forwarded-For header
Note over App: Framework config reads TRUSTED_PROXIES & TRUSTED_HEADERS env vars
Note over App: Default: TRUSTED_PROXIES=REMOTE_ADDR, TRUSTED_HEADERS=x-forwarded-for
App->>App: Request::getClientIp()
alt Trusted proxy identified (REMOTE_ADDR matches trusted CIDR)
App->>App: Extract client IP from X-Forwarded-For header
App-->>RateLimit: Pass real client IP (e.g., 203.0.113.42)
else Untrusted client (REMOTE_ADDR not in trusted proxies)
App->>App: Ignore X-Forwarded-For, use REMOTE_ADDR
App-->>RateLimit: Pass REMOTE_ADDR (untrusted client's actual IP)
else No trusted proxies configured
App->>App: Ignore X-Forwarded-For entirely, use REMOTE_ADDR
App-->>RateLimit: Pass REMOTE_ADDR
end
RateLimit->>RateLimit: buildIpKey() → "ip:{client IP}"
alt Bucket not exhausted
RateLimit->>Cache: Check rate limit bucket
Cache-->>RateLimit: Remaining tokens
RateLimit-->>App: Allow request (decrement bucket)
App-->>Proxy: 200 OK response
Proxy-->>LB: Forward response
LB-->>Client: Response
else Bucket exhausted
RateLimit->>Cache: Check rate limit bucket
Cache-->>RateLimit: No tokens remaining
RateLimit-->>App: Rate limit exceeded
App-->>Proxy: 429 Too Many Requests
Proxy-->>LB: Forward 429
LB-->>Client: 429 response
end
Note over Client,Cache: Edge Cases
Client->>LB: Request with forged X-Forwarded-For: 10.0.0.1
LB->>Proxy: Forward (may overwrite XFF or add hop)
Proxy->>App: X-Forwarded-For: <original>,<proxy-ip>
alt Attacker is untrusted (REMOTE_ADDR not in TRUSTED_PROXIES)
App->>App: Discard client-supplied XFF
App->>RateLimit: key = ip:<attacker's real IP>
else Attacker is behind trusted proxy
App->>App: Extract real client from XFF (before proxy hop)
App->>RateLimit: key = ip:<real client IP from XFF>
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 551
Lines 9541 9541
=========================================
Hits 9541 9541
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env:
- Around line 12-13: Reorder the two environment variables so they match
dotenv-linter ordering: move TRUSTED_HEADERS to appear before TRUSTED_PROXIES in
the .env file; specifically swap the lines for TRUSTED_HEADERS and
TRUSTED_PROXIES so the key TRUSTED_HEADERS=x-forwarded-for precedes
TRUSTED_PROXIES=REMOTE_ADDR.
In `@specs/security-316-rate-limit-untrusted-client-ip/stories.md`:
- Around line 38-43: The fenced code blocks in
specs/security-316-rate-limit-untrusted-client-ip/stories.md lack a language
identifier and trigger markdownlint MD040; update each triple-backtick block
that contains the docker run invoking 'vendor/bin/phpunit --testsuite=Unit
--filter "RateLimit"' and the block that runs 'vendor/bin/php-cs-fixer' (and the
block with the Unit suite docker run) to use a language identifier (e.g.,
```bash) so the command blocks are marked as bash; ensure you add the identifier
to all occurrences (including the blocks referencing
tests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitTrustedProxyIpKeyTest.php).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 698fdc32-9055-4fc1-8b81-f3bd460e7dc2
📒 Files selected for processing (6)
.envconfig/packages/framework.yamlconfig/packages/test/framework.yamlspecs/security-316-rate-limit-untrusted-client-ip/prd.mdspecs/security-316-rate-limit-untrusted-client-ip/stories.mdtests/Unit/Shared/Application/Resolver/RateLimit/ApiRateLimitTrustedProxyIpKeyTest.php
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
- stories.md: correct docker image tag secfix-312-php -> secfix-316-php (copy-paste from PR #312; align with this PR's secfix-316 worktree) - stories.md: add bash language id to fenced code blocks (markdownlint MD040) - .env: order TRUSTED_HEADERS before TRUSTED_PROXIES (dotenv-linter UnorderedKey) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security fix — Closes #316
Vulnerability (HIGH, CWE-307)
IP-keyed rate limiters (
signin_ip,twofa_verification_ip,registration,refresh_token,email_confirmation,password_reset_confirm,oauth_social_*,user_collection,resend_confirmation, andglobal_api_*) build their key fromRequest::getClientIp(). Productionconfig/packages/framework.yamlnever settrusted_proxies/trusted_headers, so behind Caddy/FrankenPHP and the AWS App Runner load balancergetClientIp()returnedREMOTE_ADDR— the LB's internal IP, identical for every external client. Every per-IP bucket therefore collapsed onto a single shared bucket: one noisy client could lock out the whole tenant (self-inflicted DoS) and no attacker could be isolated. Naively trusting the proxy without pinning the hop would instead make the key fully spoofable via client-suppliedX-Forwarded-For.Fix
config/packages/framework.yaml: explicitly configuretrusted_proxies: '%env(TRUSTED_PROXIES)%'andtrusted_headers: '%env(TRUSTED_HEADERS)%'..env: prod-safe defaultsTRUSTED_PROXIES=REMOTE_ADDR(trusts a single directly-connected hop) andTRUSTED_HEADERS=x-forwarded-for(onlyX-Forwarded-Foris trusted; host/proto/port/prefix are not). Operators overrideTRUSTED_PROXIESwith the exact proxy/App Runner CIDR in production.config/packages/test/framework.yaml: pin the sametrusted_headers: 'x-forwarded-for'set.ApiRateLimitTrustedProxyIpKeyTest) covering positive (trusted proxy ⇒ real client IP), spoof (untrusted client's forgedX-Forwarded-Forignored), and no-trust (blanket XFF ignored) paths for both resolvers'buildIpKey().Result:
getClientIp()returns the real client IP; forgedX-Forwarded-Forfrom untrusted clients is dropped; local requests with no XFF still key onREMOTE_ADDR, preserving existing limiter and integration-test behaviour.Local verification (one-off containers, vendor read-only)
RateLimit): OK (143 tests, 258 assertions)BMAD spec
specs/security-316-rate-limit-untrusted-client-ip/(prd.md,stories.md).🤖 Generated with Claude Code
Summary by cubic
Fixes IP-based rate limiting behind a proxy by trusting only the directly connected hop and
X-Forwarded-For, so keys use the real client IP and spoofed headers are ignored. Prevents shared buckets on the load balancer IP and restores per-client isolation.Bug Fixes
framework.trusted_proxiesandframework.trusted_headersvia env.TRUSTED_PROXIES=REMOTE_ADDR,TRUSTED_HEADERS=x-forwarded-for.trusted_headers: 'x-forwarded-for'..envkeys and fixstories.mdDocker tag/code fences.Migration
TRUSTED_PROXIESto your reverse proxy/App Runner CIDR. No other changes required.Written for commit c6cad40. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Documentation