[breaking] rename xForwardedForIndex to XFF_DEPTH
#4332
Conversation
🦋 Changeset detectedLatest commit: fb3e63e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
benmccann
force-pushed
the
xForwardedForNumProxies
branch
from
3dbc9de
to
8e1e1ae
Compare
packages/adapter-node/README.md
Outdated
| ``` | ||
|
|
||
| For that reason you should always use a negative number (depending on the number of proxies) if you need to trust `event.clientAddress`. In the above example, `0` would yield the spoofed address while `-3` would continue to work. | ||
| If there may be a variable number of proxies, you would have to read the `X-Forwarded-For` header yourself and read the IP address from the left, but be very careful that you do not use the result for any applications with possible security implications since `X-Forwarded-For` is [trivial to spoof](https://adam-p.ca/blog/2022/03/x-forwarded-for/). |
There was a problem hiding this comment.
Is this a realistic scenario? Surely there's going to be a fixed number of proxies unless you changed your network architecture?
I think it might be worth preserving the previous illustration, because most people aren't going to know the implications of 'read from the left' or 'read from the right' (at least, I wouldn't have known before this week)
There was a problem hiding this comment.
I was thinking that there could be proxies that are not controlled by your own server infrastructure. E.g. some users may have the traffic flow through a corporate proxy at the user's workplace while others wouldn't, so there could always be variable number of proxies. This could probably be worded more clearly
There was a problem hiding this comment.
Surely the relevant number isn't the proxies between the server and the client, but the (trusted) proxies between the server and the public internet?
There was a problem hiding this comment.
Why would that be? Any proxy could be added to the X-Forwarded-For
Sometimes you could have:
<client address>, <trusted proxy address>
And sometimes you could have:
<client address>, <untrusted proxy address>, <trusted proxy address>
In this case it doesn't work to go a set number from the right. And actually now I'm remembering @mrkishi suggesting that we would need to not just use an index, but check the addresses in the list and I'm wondering if we should just remove the ability to lookup by index as it actually does not seem very safe to me.
There was a problem hiding this comment.
Using an index is fine, you just need to be careful do it from the right (as we're already doing). If you have an untrusted proxy after a trusted one, you can't trust the header at all, so in that case you shouldn't set the header in order to get remoteAddress instead.
A list of trusted proxy "addresses" is a valid option, but since it was considered too troublesome, we can just let it up to the user since they already have access to the header. Even with a list of trusted proxies, you should still read from the right: keep discarding trusted addresses from the right and use the first untrusted one as the client address.
|
I just realized this should also be an environment variable, since it's not app- but deployment-specific. So
|
|
yeah, making it an environment variable probably makes sense |
benmccann
changed the title
xForwardedForIndex to xForwardedForNumProxiesxForwardedForIndex to XFF_DEPTH
packages/adapter-node/README.md
Outdated
|
|
||
| ``` | ||
| <client address>, <proxy 1 address>, <proxy 2 address> | ||
| ``` | ||
|
|
||
| To get the client address we could use `xForwardedFor: 0` or `xForwardedFor: -3`, which counts back from the number of addresses. | ||
| To get the client address we could use `XFF_DEPTH=3`. |
There was a problem hiding this comment.
I'm not sure how to turn this into a suggestion given that it contains a fenced code block:
Some guides will tell you to read the left-most address, but this leaves you vulnerable to spoofing:
<spoofed address>, <client address>, <proxy 1 address>, <proxy 2 address>
Instead, we read from the right, accounting for the number of trusted proxies. In this case, we would use XFF_DEPTH=3.
* use XFF_DEPTH_ENV * get rid of get_xff_depth * typo
Co-authored-by: Maurício Kishi <mrkishi@users.noreply.github.com>
Having to specify a negative number is a bit of an unusual API, so flip it and make it positive
This is a breaking change for anyone already using
xForwardedForIndex