Fix: Using all IPs including x-forwarded-for when checking if the requester has access to metrics #2241
Conversation
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
…uester has access to metrics
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe metricsIPWhitelist middleware now gathers all possible client IPs (including x-forwarded-for) into an array, filters out undefined values, and then checks for any intersection with the configured allowed IPs to authorize metric access. Sequence diagram for updated IP whitelist check in metrics middlewaresequenceDiagram
participant Client
participant "Express Server"
participant "metricsIPWhitelist Middleware"
Client->>"Express Server": Request /metrics
"Express Server"->>"metricsIPWhitelist Middleware": Pass request
"metricsIPWhitelist Middleware"->>"metricsIPWhitelist Middleware": Gather IPs (req.ip, remoteAddress, x-forwarded-for)
"metricsIPWhitelist Middleware"->>"metricsIPWhitelist Middleware": Check intersection with allowed IPs
alt IP allowed
"metricsIPWhitelist Middleware"->>"Express Server": next()
"Express Server"->>Client: Return metrics
else IP not allowed
"metricsIPWhitelist Middleware"->>Client: 403 Forbidden
end
Class diagram for updated metricsIPWhitelist middlewareclassDiagram
class metricsIPWhitelist {
+allowedIPs: string[]
+clientIPs: string[]
+Request, Response, NextFunction
+Gathers IPs from req.ip, req.connection.remoteAddress, req.socket.remoteAddress, req.headers["x-forwarded-for"]
+Checks intersection with allowedIPs
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The conditional
allowedIPs.filter((ip) => clientIPs.includes(ip)) === 0is comparing an array to a number—use.length === 0or!allowedIPs.some(...)instead. req.headers['x-forwarded-for']can be a comma-separated list of IPs, so you should split it and pick the correct client IP rather than treating it as a single string.- Consider enabling Express’s
trust proxysetting and usingreq.ipsfor more reliable handling of proxy and load balancer IPs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The conditional `allowedIPs.filter((ip) => clientIPs.includes(ip)) === 0` is comparing an array to a number—use `.length === 0` or `!allowedIPs.some(...)` instead.
- `req.headers['x-forwarded-for']` can be a comma-separated list of IPs, so you should split it and pick the correct client IP rather than treating it as a single string.
- Consider enabling Express’s `trust proxy` setting and using `req.ips` for more reliable handling of proxy and load balancer IPs.
## Individual Comments
### Comment 1
<location> `src/api/routes/index.router.ts:51-55` </location>
<code_context>
- const clientIP = req.ip || req.connection.remoteAddress || req.socket.remoteAddress;
-
- if (!allowedIPs.includes(clientIP)) {
+ const clientIPs = [
+ req.ip,
+ req.connection.remoteAddress,
+ req.socket.remoteAddress,
+ req.headers['x-forwarded-for'],
+ ].filter((ip) => ip !== undefined);
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider normalizing and validating IPs from 'x-forwarded-for'.
Split the 'x-forwarded-for' header on commas, trim whitespace from each IP, and validate their formats to ensure accurate and secure IP handling.
</issue_to_address>
### Comment 2
<location> `src/api/routes/index.router.ts:58` </location>
<code_context>
+ req.headers['x-forwarded-for'],
+ ].filter((ip) => ip !== undefined);
+
+ if (allowedIPs.filter((ip) => clientIPs.includes(ip)) === 0) {
return res.status(403).send('Forbidden: IP not allowed');
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The IP check logic is incorrect; filter returns an array, not a count.
Update the condition to check 'allowedIPs.filter(...).length === 0' so it correctly determines when no client IPs are allowed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+55
| const clientIPs = [ | ||
| req.ip, | ||
| req.connection.remoteAddress, | ||
| req.socket.remoteAddress, | ||
| req.headers['x-forwarded-for'], |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): Consider normalizing and validating IPs from 'x-forwarded-for'.
Split the 'x-forwarded-for' header on commas, trim whitespace from each IP, and validate their formats to ensure accurate and secure IP handling.
| req.headers['x-forwarded-for'], | ||
| ].filter((ip) => ip !== undefined); | ||
|
|
||
| if (allowedIPs.filter((ip) => clientIPs.includes(ip)) === 0) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The IP check logic is incorrect; filter returns an array, not a count.
Update the condition to check 'allowedIPs.filter(...).length === 0' so it correctly determines when no client IPs are allowed.
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.
📋 Description
We need to use all kind of ips to make sure the requester has access to, including
x-forwarded-forip since when using cloudflare/loadbalance on the front line theipwill be the cloudflare/loadbalance not real requester ip.🔗 Related Issue
🧪 Type of Change
🧪 Testing
✅ Checklist
📝 Additional Notes
Summary by Sourcery
Improve IP whitelist validation for metrics endpoints by aggregating all possible client IP sources and including the X-Forwarded-For header.
Bug Fixes:
Enhancements: