fix(settings): use CIDR-aware proxy detection in ForwardedForHeaders setup check#60315
Open
algojogacor wants to merge 1 commit into
Open
fix(settings): use CIDR-aware proxy detection in ForwardedForHeaders setup check#60315algojogacor wants to merge 1 commit into
algojogacor wants to merge 1 commit into
Conversation
…setup check The setup check used in_array() with strict comparison to determine if REMOTE_ADDR matched a trusted proxy entry. This cannot match CIDR ranges (e.g., 172.16.0.0/12) because the raw IP and the CIDR string are never equal. Replace in_array() with a check that compares the raw REMOTE_ADDR against the resolved address from getRemoteAddress(), which already handles CIDR matching internally via IpUtils::checkIp(). Also add a test case for large CIDR (/12) matching to prevent future regressions. Fixes: nextcloud#60287 Signed-off-by: Arya Rizky <arya@algojogacor.dev>
8 tasks
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
Fixes #60287
The ForwardedForHeaders setup check used
in_array($remoteAddress, $trustedProxies, true)to determine if the connecting proxy was trusted. This strict comparison cannot match CIDR ranges (e.g.,172.16.0.0/12) because the raw IP172.21.0.7and the CIDR string172.16.0.0/12are never equal.Root Cause
ForwardedForHeaders.phpline 75 usedin_array()for exact string matching against the trusted proxies list. WhilegetRemoteAddress()inRequest.phpcorrectly uses Symfony'sIpUtils::checkIp()for CIDR-aware matching, the setup check did its own non-CIDR-aware comparison. This meant:Fix
Replace the
in_array()-based check with a direct comparison between$remoteAddress(rawREMOTE_ADDR) and$detectedRemoteAddress(fromgetRemoteAddress()). SincegetRemoteAddress()already usesIpUtils::checkIp()internally for CIDR matching, this comparison correctly reflects whether proxy detection resolved a different client IP.Added a test case for large
/12CIDR matching inRequestTest.phpto prevent regressions.Changes
apps/settings/lib/SetupChecks/ForwardedForHeaders.php: Replacein_array()with direct address comparison (+12 -10lines)tests/lib/AppFramework/Http/RequestTest.php: Add test case for/12CIDR proxy detection (+9 -0lines)Testing
/12trusted proxy with valid X-Forwarded-For: Client IP resolved correctly ✅/24trusted proxy (existing test preserved): Client IP resolved correctly ✅