Stop trusting client-supplied proxy headers for rate-limit IP by default#3238
Merged
Stop trusting client-supplied proxy headers for rate-limit IP by default#3238
Conversation
get_client_ip() previously walked CF-Connecting-IP, X-Forwarded-For, X-Real-IP, and friends before falling back to REMOTE_ADDR. On any site that isn't behind a reverse proxy that strips and rewrites those headers, an attacker controls every one of them and can rotate the spoofed value to bypass the per-IP rate limits on OAuth client registration, OAuth token, and CIMD discovery. Default to REMOTE_ADDR only. Add an activitypub_trusted_proxy_headers filter so operators behind Cloudflare / Akamai / nginx can opt the relevant header back in. Document the X-Forwarded-For prepend pitfall and point at the existing activitypub_client_ip filter for cases where the operator needs to resolve from the right by a known proxy count.
Make activitypub_client_ip_sources the single ordered list of $_SERVER keys to consult, defaulting to ['REMOTE_ADDR']. One loop replaces the foreach + separate REMOTE_ADDR fallback. Cloudflare operators can now swap in ['HTTP_CF_CONNECTING_IP'] without REMOTE_ADDR (which would be the Cloudflare edge IP, not the client) — previously the safety net fallback would have re-added it. Renamed activitypub_trusted_proxy_headers to activitypub_client_ip_sources to match the new semantic; the filter hadn't shipped yet.
There was a problem hiding this comment.
Pull request overview
This PR hardens per-IP rate limiting by changing how Activitypub\get_client_ip() determines the client IP: proxy-supplied headers are no longer trusted by default, and operators must explicitly opt-in to trusted proxy headers via a new filter.
Changes:
- Introduces
activitypub_trusted_proxy_headers(empty by default) to opt-in to specific trusted proxy headers for IP detection. - Updates
get_client_ip()behavior to consult only opted-in headers, otherwise falling back toREMOTE_ADDR. - Adds PHPUnit coverage for the new default-deny and opt-in behaviors, including invalid header fallback and XFF list handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
includes/functions.php |
Adds the opt-in trusted-proxy-headers filter and updates header selection logic for get_client_ip(). |
tests/phpunit/tests/includes/class-test-functions.php |
Adds tests validating default behavior and the new opt-in filter behavior. |
.github/changelog/fix-rate-limit-untrusted-proxy-headers |
Documents the security change and the new filter for operators behind reverse proxies. |
The function-level docblock still described 'Checks common proxy headers before falling back to REMOTE_ADDR, similar to Jetpack's approach' — accurate before this branch but no longer true. Replace it with a description of the activitypub_client_ip_sources filter, the REMOTE_ADDR default, and the trust constraint.
test_get_client_ip_ignores_non_ip_header is dead weight under the new source-list semantics — HTTP_CF_CONNECTING_IP isn't consulted unless the operator opts it in. The case is covered by test_get_client_ip_ignores_proxy_headers_by_default (default doesn't read it) and test_get_client_ip_falls_back_when_trusted_header_invalid (opted in but invalid value). Also replace 'doesn't accidentally fail closed' in the fall-through test docstring with 'doesn't lock all callers out' — 'fail closed' is a security term that suggests denial on uncertainty, which isn't what graceful degradation through the source list looks like.
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
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.
Summary
Why
Per-IP rate limits guard OAuth dynamic client registration (10/min), the OAuth token endpoint (20/min), and CIMD discovery (10/min). Previously, on any site that isn't behind a reverse proxy that strips and rewrites the proxy headers, an attacker controls all of `X-Forwarded-For`, `CF-Connecting-IP`, etc. directly. Rotating the spoofed value gives them effectively unlimited buckets and the cap is bypassed.
Defaulting to `REMOTE_ADDR` makes the rate limit honest on the largest fraction of installs (PHP-FPM directly facing the internet). Sites behind Cloudflare, Akamai, or nginx-with-stripping can redefine the priority order:
```php
add_filter( 'activitypub_client_ip_sources', static function () {
return array( 'HTTP_CF_CONNECTING_IP' );
} );
```
Cloudflare operators specifically should NOT fall back to `REMOTE_ADDR` (which is Cloudflare's edge IP — every legitimate request would land in the same bucket).
The filter docblock also calls out the X-Forwarded-For prepend pitfall: even with a trusted proxy, an attacker can prepend a value before the proxy appends the real client. Operators who need correct end-to-end `X-Forwarded-For` handling should resolve from the right via the existing `activitypub_client_ip` filter.
Test plan