Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Configure trusted proxies #9548

Merged
merged 2 commits into from
Mar 19, 2024
Merged

fix: Configure trusted proxies #9548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08ae2fb
fix: Configure trusted proxies
Dschoordsch Mar 19, 2024
d957dd3
Add a warning
Dschoordsch Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ PROTO='http'
SERVER_SECRET='key_SERVER_SECRET'
# Cluster node number 0 - 1023. Must be unique per process.
SERVER_ID='1'
# Used to read the client IP from the X-Forwarded-For header, if not set, it will use the first IP in the list.
# TRUSTED_PROXY_COUNT='1'
# Websocket port for the websocket server, only used in development (yarn dev)
SOCKET_PORT='3001'

Expand Down
6 changes: 5 additions & 1 deletion packages/server/utils/uwsGetIP.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a warning or some clearer instructions would do. That said, LGTM.


const uwsGetIP = (res: HttpResponse, req: HttpRequest) => {
const clientIp = req.getHeader('x-forwarded-for')?.split(',')[0]
const clientIp = req.getHeader('x-forwarded-for')?.split(',').at(CLIENT_IP_POS)
if (clientIp) return clientIp
// returns ipv6 e.g. '0000:0000:0000:0000:0000:ffff:ac11:0001'
return Buffer.from(res.getRemoteAddressAsText()).toString()
Expand Down
Loading