fix: Configure trusted proxies #9548
Conversation
Our proxy will add the client ip to the x-forwarded-for header correctly and also itself as it does NAT. Configure the number of trusted proxies to read the correct entry from the header to avoid manipulation by clients.
| @@ -1,7 +1,11 @@ | |||
| import {HttpRequest, HttpResponse} from 'uWebSockets.js' | |||
|
|
|||
| const TRUSTED_PROXY_COUNT = Number(process.env.TRUSTED_PROXY_COUNT) | |||
| // if TRUSTED_PROXY_COUNT is not configured correctly we fall back to reading the first IP to avoid rate limiting our proxy | |||
| const CLIENT_IP_POS = isNaN(TRUSTED_PROXY_COUNT) ? 0 : -1 - TRUSTED_PROXY_COUNT | |||
There was a problem hiding this comment.
Why setting to -1 - TRUSTED_PROXY_COUNT instead of just TRUSTED_PROXY_COUNT? I feel that more flexible. In our case we would use a negative value, in other cases it could be used in other way. Some clarification could be added to the .env.example saying that the number can be negative to pick stuff starting from the end.
There was a problem hiding this comment.
When configured, you should always pick from the end. Everything at the beginning of the list is in client control and the proxies will add stuff to the end. I don't see a situation where one would want to read from the start.
The number to configure is really how many proxies are in your stack.
Previously we used the whole array as a key, but now we seem to check that it's a valid ip, so we need to pick a entry from the array. We cannot default to the last entry as this would rate limit the traffic coming from the proxy. Maybe we should add a warning that configuring this the wrong way would do that?
There was a problem hiding this comment.
Yes, a warning or some clearer instructions would do. That said, LGTM.
Our proxy will add the client ip to the x-forwarded-for header correctly and also itself as it does NAT. Configure the number of trusted proxies to read the correct entry from the header to avoid manipulation by clients.
Description
Fixes/Partially Fixes #[issue number]
[Please include a summary of the changes and the related issue]
Demo
[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]
Testing scenarios
[Please list all the testing scenarios a reviewer has to check before approving the PR]
Scenario A
Scenario B
Final checklist