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.

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

Improve handling of IP addresses #13260

Closed
mydea opened this issue Aug 7, 2024 · 1 comment · Fixed by #13272
Closed

Improve handling of IP addresses #13260

mydea opened this issue Aug 7, 2024 · 1 comment · Fixed by #13272
Assignees

Comments

@mydea
Copy link
Member

mydea commented Aug 7, 2024

Description

Today, we attach an IP address to the event if users opt-in to this via requestDataIntegration({ include: { ip: true } }). If this is enabled, we extract the IP address and add it to the event.

However, in scenarios where apps receive requests from a proxy, the IP address is often not the original IP address, but something like ffff:127.0.0.1. In this scenario, the "real" IP address can be found in a header.

We have code in relay that extracts these headers, if they are set, and sets the IP address based on this. Today, because of this we have two problems in the SDK:

  1. If ip: true is configured in the SDK, we always set an IP address. Relay then never infers the IP from the headers, so we get the "wrong" IP address (e.g. the 127.0.0.1 one).
  2. If ip: false, no IP will be set by the SDK, but if it is a proxy scenario the headers are today not filtered out, so relay will still infer this.

To fix this, we want to make two changes:

[ ] Look at headers in the SDK if ip: true is configured
[ ] Do not send these headers if ip: true is not configured

This way, we should have a more consistent outcome - either correct IP addresses is ip: true is opted-into, or else no IP addresses, no matter if it is a proxy/tunnel or not.

We can use these headers, which we currently look at in Remix:

  • X-Client-IP
  • X-Forwarded-For
  • Fly-Client-IP
  • CF-Connecting-IP
  • Fastly-Client-Ip
  • True-Client-Ip
  • X-Real-IP
  • X-Cluster-Client-IP
  • X-Forwarded
  • Forwarded-For
  • Forwarded
@andreiborza
Copy link
Member

We might want to include https://vercel.com/docs/edge-network/headers#x-vercel-forwarded-for in the list.

@mydea mydea self-assigned this Aug 7, 2024
mydea added a commit that referenced this issue Aug 8, 2024
This PR does three things:

1. It ensures we infer the IP (in RequestData integration) from
IP-related headers, if available.
2. It ensures we do not send these headers if IP capturing is not
enabled (which is the default)
3. It removes the custom handling we had for this in remix, as this
should now just be handled generally

Closes #13260
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants