support more proxy header #458
Conversation
|
|
📝 WalkthroughWalkthroughReplaces a Cloudflare-specific proxy flag with a configurable header-based approach: backend reads a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as CDN/Proxy
participant Server
Note over Client,Proxy: HTTP request may pass through a proxy that adds headers
Client->>Proxy: HTTP Request (source IP)
Proxy->>Server: HTTP Request with proxy headers (e.g., x-forwarded-for, cf-connecting-ip, cloudfront-viewer-address)
Server->>Server: getIPFromHeaders(headers)
alt CLIENT_IP_HEADER set and present
Server->>Server: return headers[CLIENT_IP_HEADER]
else header found in IP_ADDRESS_HEADERS
Server->>Server: if x-forwarded-for -> take first IP
Server->>Server: if forwarded -> extract for= value
Server->>Server: if cloudfront-viewer-address -> strip port
Server->>Server: return extracted IP
else no header
Server->>Server: return null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
docs/docs/selfhosting/configuring.md (1)
37-37: Clarify the documentation for CLIENT_IP_HEADER.The description should be more specific about:
- Which header name to provide (e.g., "Set to the name of the proxy header containing the client IP, such as
cf-connecting-iporx-forwarded-for")- What happens when not set (automatic detection using common proxy headers)
- Whether the default is truly
nullor empty/unsetSuggested improvement
-| `CLIENT_IP_HEADER` | `null` | If set - Swetrix will get IP address information from header provided by Proxy. | +| `CLIENT_IP_HEADER` | | The name of the proxy header to use for determining client IP addresses (e.g., `cf-connecting-ip`, `x-forwarded-for`). If not set, Swetrix will automatically detect the IP from common proxy headers. |backend/.env.example (1)
84-88: Fix typo in comment."Cloudfront" should be "CloudFront" (capital F) for consistency with the official product name.
🔎 Proposed fix
# Configure proxy header for client IP # Some well know proxies # Cloudflare: cf-connecting-ip -# Cloudfront: cloudFront-viewer-address +# CloudFront: cloudfront-viewer-address CLIENT_IP_HEADER=cf-connecting-ipbackend/apps/community/src/common/utils.ts (1)
1184-1190: Consider improving forwarded header parsing.The regex handles basic IPv4 and IPv6 patterns, but the extracted IPv6 address may include brackets that should be stripped for consistency.
🔎 Proposed improvement
if (header === 'forwarded') { const match = ip.match(/for=(\[?[0-9a-fA-F:.]+\]?)/) if (match) { - return match[1] + // Remove brackets from IPv6 addresses + return match[1].replace(/[\[\]]/g, '') } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/.env.examplebackend/apps/community/src/common/constants.tsbackend/apps/community/src/common/utils.tsdocs/docs/selfhosting/configuring.mddocs/docs/selfhosting/how-to.md
💤 Files with no reviewable changes (1)
- backend/apps/community/src/common/constants.ts
🔇 Additional comments (1)
backend/apps/community/src/common/utils.ts (1)
122-136: Good coverage of proxy headers.The list provides comprehensive coverage of common proxy and CDN headers with reasonable precedence ordering.
| const header = IP_ADDRESS_HEADERS.find(name => { | ||
| return headers.get(name) | ||
| }) | ||
|
|
||
| if (ip) { | ||
| return ip | ||
| if (!header) { | ||
| return null | ||
| } | ||
|
|
||
| ip = headers['x-forwarded-for'] || null | ||
| const ip = headers.get(header) | ||
|
|
||
| if (!ip) { | ||
| return null | ||
| if (header === 'x-forwarded-for') { | ||
| return ip.split(',')[0].trim() | ||
| } | ||
|
|
||
| return _split(ip, ',')[0] | ||
| if (header === 'forwarded') { | ||
| const match = ip.match(/for=(\[?[0-9a-fA-F:.]+\]?)/) | ||
|
|
||
| if (match) { | ||
| return match[1] | ||
| } | ||
| } | ||
| if (header === 'cloudfront-viewer-address') { | ||
| const lastColonIndex = ip.lastIndexOf(':') | ||
| if (lastColonIndex > 0) { | ||
| return ip.substring(0, lastColonIndex) | ||
| } | ||
| } | ||
| return ip | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/apps/community/src/common/utils.ts | sed -n '1150,1210p'Repository: Swetrix/swetrix
Length of output: 1731
🏁 Script executed:
rg -n "getIPFromHeaders" backend/apps/community/src/ -g '!**/test/**' -g '!**/*.spec.ts' -A 3 -B 1Repository: Swetrix/swetrix
Length of output: 8733
🏁 Script executed:
rg -n "validateIP\|isValidIP\|ipv4\|ipv6" backend/apps/community/src/ -g '!**/test/**' -g '!**/*.spec.ts' -iRepository: Swetrix/swetrix
Length of output: 41
🏁 Script executed:
rg -n "const checkRateLimit\|function checkRateLimit" backend/apps/community/src/common/utils.ts -A 20Repository: Swetrix/swetrix
Length of output: 41
🏁 Script executed:
rg -n "checkRateLimit" backend/apps/community/src/ --type=ts -g '!**/test/**' -g '!**/*.spec.ts' | head -5Repository: Swetrix/swetrix
Length of output: 580
🏁 Script executed:
rg -n "checkRateLimit" backend/apps/community/src/common/utils.ts -B 2 -A 15Repository: Swetrix/swetrix
Length of output: 1155
🏁 Script executed:
grep -n "export.*checkRateLimit" backend/apps/community/src/common/utils.tsRepository: Swetrix/swetrix
Length of output: 41
🏁 Script executed:
rg -n "getRateLimitHash" backend/apps/community/src/common/utils.ts -B 2 -A 8Repository: Swetrix/swetrix
Length of output: 806
🏁 Script executed:
rg -n "IP_ADDRESS_HEADERS" backend/apps/community/src/common/utils.ts -B 2 -A 2Repository: Swetrix/swetrix
Length of output: 642
The custom header case returns completely unvalidated IP values; consider sanitizing or validating before use.
The function lacks explicit IP format validation. Most critically, at line 1167, a custom CLIENT_IP_HEADER value is returned raw without any sanitization. Additionally, the regex pattern for the 'forwarded' header (line 1185) does not properly match standard IPv4 addresses (e.g., "192.168.1.1" contains no colons and would not match). The extracted IP is passed directly to checkRateLimit(), which only hashes it without validation—acceptable for rate limiting by hash, but the unvalidated custom header poses a risk if the header is attacker-controlled.
🤖 Prompt for AI Agents
In backend/apps/community/src/common/utils.ts around lines 1170 to 1198, the
function returns header-derived IPs without validation (including the custom
CLIENT_IP_HEADER) and the 'forwarded' regex only matches addresses with colons;
update the code to validate and sanitize any extracted IP before returning it:
use Node's net.isIP (or equivalent) to accept only valid IPv4/IPv6 addresses
(return null or fallback when invalid), normalize/trim values, enhance the
'forwarded' regex to also capture IPv4 (e.g., for=192.168.0.1 and
for="[::1]":port), properly strip optional port information (including bracketed
IPv6 ports), and apply the same validation to the cloudfront and x-forwarded-for
branches so attacker-controlled headers can't produce unvalidated IPs.
There was a problem hiding this comment.
@Blaumaus Im not sure this is something mandatory, the proxies usually provide correct ip address, so IMO we dont really need to verify header value. What's your though?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
good PR. I'll review & merge it later, it will be included in the next release |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @backend/apps/community/src/common/utils.ts:
- Around line 1184-1190: The current forwarded-header parsing block (checking
header === 'forwarded') uses the regex /for=(\[?[0-9a-fA-F:.]+\]?)/ which fails
for quoted tokens and leaves port suffixes; update the logic so you first
extract the token after for= allowing optional surrounding double quotes (e.g.
for="...") and then normalize it by removing an optional port suffix (handle
both bracketed IPv6 like [2001:db8::1]:8080 and unbracketed IPv4:port), finally
return the bare address. Locate the block using variables header, ip and match,
replace the single-group regex/return with a two-step extract-and-normalize
approach (capture optional quotes, strip quotes if present, strip trailing :port
only when it is outside a trailing ']' for IPv6) and return the cleaned address
instead of match[1].
♻️ Duplicate comments (2)
backend/apps/community/src/common/utils.ts (2)
1191-1196: IPv6 handling still broken forcloudfront-viewer-address.This was flagged in a previous review and remains unaddressed. CloudFront uses
[ipv6]:portformat for IPv6 addresses. UsinglastIndexOf(':')will:
- For
[2001:db8::1]:443→ return[2001:db8::1](includes brackets, invalid IP)- For unbracketed IPv6 without port → corrupt the address
🔎 Proposed fix
if (header === 'cloudfront-viewer-address') { + // CloudFront format: "ipv4:port" or "[ipv6]:port" + if (ip.startsWith('[')) { + const bracketEnd = ip.indexOf(']') + if (bracketEnd > 0) { + return ip.substring(1, bracketEnd) + } + } const lastColonIndex = ip.lastIndexOf(':') - if (lastColonIndex > 0) { + // Only strip port for IPv4 (single colon) + if (lastColonIndex > 0 && ip.indexOf(':') === lastColonIndex) { return ip.substring(0, lastColonIndex) } }
1162-1167: Unvalidated custom header value returned directly.When
CLIENT_IP_HEADERis set, its value is returned without any IP format validation. If this header is attacker-controlled or misconfigured, it could contain arbitrary data that gets passed to downstream functions likegetGeoDetails()which expect a valid IP address.Consider validating with Node's
net.isIP()before returning:🔎 Proposed fix
+import { isIP } from 'net' + const getIPFromHeaders = (headers: any) => { const customHeader = process.env.CLIENT_IP_HEADER // only use custom header if not present in IP_ADDRESS_HEADERS // since we might need some extra process based on proxy if ( !!customHeader && !IP_ADDRESS_HEADERS.includes(customHeader) && headers.get(customHeader) ) { - return headers.get(customHeader) + const ip = headers.get(customHeader)?.trim() + return isIP(ip) ? ip : null }
🧹 Nitpick comments (2)
backend/.env.example (1)
81-85: Approve the new CLIENT_IP_HEADER configuration approach.The replacement of
CLOUDFLARE_PROXY_ENABLEDwith the configurableCLIENT_IP_HEADERis a good design. The comments clearly document supported proxy headers and provide useful examples.Optional: Standardize header name casing in comments.
For documentation clarity, consider using the standard AWS casing for the CloudFront header name (e.g.,
CloudFront-Viewer-Addressinstead ofcloudFront-viewer-address). While HTTP headers are case-insensitive, consistent casing in documentation improves clarity.🔎 Proposed improvement
# Configure proxy header for client IP # Some well know proxies # Cloudflare: cf-connecting-ip - # Cloudfront: cloudFront-viewer-address + # CloudFront: CloudFront-Viewer-Address CLIENT_IP_HEADER=cf-connecting-ipbackend/apps/community/src/common/utils.ts (1)
1170-1198: Consider centralizing IP validation for all extraction paths.The function has multiple return paths (custom header, x-forwarded-for, forwarded, cloudfront, default) but none validate the extracted IP. While the custom header case is most concerning, all paths could benefit from a final validation step before returning.
🔎 Suggested pattern
import { isIP } from 'net' const getIPFromHeaders = (headers: any): string | null => { let ip: string | null = null // ... existing extraction logic setting `ip` ... // Final validation if (ip) { ip = ip.trim() return isIP(ip) ? ip : null } return null }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/.env.examplebackend/apps/community/src/common/utils.tsdocs/docs/selfhosting/how-to.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/selfhosting/how-to.md
🔇 Additional comments (3)
backend/.env.example (1)
81-85: Note: CLOUDFLARE_PROXY_ENABLED removal acknowledged.The deprecated
CLOUDFLARE_PROXY_ENABLEDvariable is not present in the current configuration, indicating it has been properly removed as flagged in the prior review. This aligns with the migration to the header-based approach.backend/apps/community/src/common/utils.ts (2)
23-23: LGTM!Clean removal of the
isProxiedByCloudflareimport, consistent with the new header-based approach.
122-136: LGTM!Good header priority order - CDN-specific headers (Cloudfront, Cloudflare, Fastly) are checked before generic forwarding headers. The inline comments documenting each header's origin improve maintainability.
| if (header === 'forwarded') { | ||
| const match = ip.match(/for=(\[?[0-9a-fA-F:.]+\]?)/) | ||
|
|
||
| if (match) { | ||
| return match[1] | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete RFC 7239 forwarded header parsing.
The regex /for=(\[?[0-9a-fA-F:.]+\]?)/ doesn't handle:
- Quoted values per RFC 7239:
for="192.168.1.1"orfor="[2001:db8::1]:port" - Port stripping from bracketed IPv6:
for=[2001:db8::1]:8080
This may cause null returns or include port suffixes for some proxy configurations.
🔎 Proposed fix
if (header === 'forwarded') {
- const match = ip.match(/for=(\[?[0-9a-fA-F:.]+\]?)/)
-
- if (match) {
- return match[1]
+ // Handle both quoted and unquoted formats per RFC 7239
+ // for=192.168.1.1, for="192.168.1.1", for=[::1], for="[::1]:port"
+ const match = ip.match(/for="?\[?([0-9a-fA-F.:]+)\]?(?::\d+)?"?/i)
+ if (match) {
+ return match[1]
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (header === 'forwarded') { | |
| const match = ip.match(/for=(\[?[0-9a-fA-F:.]+\]?)/) | |
| if (match) { | |
| return match[1] | |
| } | |
| } | |
| if (header === 'forwarded') { | |
| // Handle both quoted and unquoted formats per RFC 7239 | |
| // for=192.168.1.1, for="192.168.1.1", for=[::1], for="[::1]:port" | |
| const match = ip.match(/for="?\[?([0-9a-fA-F.:]+)\]?(?::\d+)?"?/i) | |
| if (match) { | |
| return match[1] | |
| } | |
| } |
🤖 Prompt for AI Agents
In @backend/apps/community/src/common/utils.ts around lines 1184-1190, The
current forwarded-header parsing block (checking header === 'forwarded') uses
the regex /for=(\[?[0-9a-fA-F:.]+\]?)/ which fails for quoted tokens and leaves
port suffixes; update the logic so you first extract the token after for=
allowing optional surrounding double quotes (e.g. for="...") and then normalize
it by removing an optional port suffix (handle both bracketed IPv6 like
[2001:db8::1]:8080 and unbracketed IPv4:port), finally return the bare address.
Locate the block using variables header, ip and match, replace the single-group
regex/return with a two-step extract-and-normalize approach (capture optional
quotes, strip quotes if present, strip trailing :port only when it is outside a
trailing ']' for IPv6) and return the cleaned address instead of match[1].
3 participants
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
This resolve issue #457
Summary by CodeRabbit
Configuration Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.