search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
574 stars 204 forks source link

`Sentry.Tunnel` doesn't include `X-Forwarded-To` #1309

Open bruno-garcia opened 2 years ago

bruno-garcia commented 2 years ago

Summary

A high level overview of why we supply tunnelling in Sentry is described in NextJS: Using the tunnel option.

I'm assuming we're doing something similar in our .NET backend... so to prevent ad blockers etc. getting in the way of sending data to Sentry, your Angular front end is sending these to your back end (which is sitting on host that's allowed by the CORS policy) and the back end then forwards those events to Sentry on behalf of the Angular SPA?

Assuming I've followed along so far, our tunnelling logic then needs to get the user's IP address (i.e. the ip:port of the machine calling the tunnelling endpoint) and use this value to set the X-Forwarded-For header when forwarding requests.

References

Similar to what's discussed here: https://forum.sentry.io/t/real-client-ip-with-sentry-nextjs-tunnel/15438/2

bruno-garcia commented 1 year ago

Doesn't seem to be working, maybe regressed?

Example: https://sentry.io/organizations/nugettrends/replays/?project=1266321&query=duration%3A%3E%3D5

https://github.com/dotnet/nuget-trends/blob/fc196aee7904b9747a58605b0bf37f115444d1e7/src/NuGetTrends.Web/Startup.cs#L100

jamescrosswell commented 11 months ago

The NugetTrends dashboard in the example above shows: image

So it's showing Unknown User where it should be showing the IP address. This may be caused by the lack of an X forwarded header.

Additional context from Bruno:

Ip address shows as "user" If we have {{auto}} as user.ipAddress. But I believe we only set thst if sendDefaultPii is true. And project setting say "use connection as ip" which afaik is true by default

jamescrosswell commented 11 months ago

@bruno-garcia I'm wondering how best to reproduce this. I figured I'd try to get Nuget Trends running locally. This seems to require a Postgres DB but the latest dump isn't available at https://contrib.nugettrends.com/nuget-trends-contrib.dump (I get a connection timed out from cloudflare for that URL).

btw: I can't see anything in the history for SentryTunnelMiddleware.cs that would have caused a regression.

bruno-garcia commented 11 months ago

@bruno-garcia I'm wondering how best to reproduce this. I figured I'd try to get Nuget Trends running locally. This seems to require a Postgres DB but the latest dump isn't available at contrib.nugettrends.com/nuget-trends-contrib.dump (I get a connection timed out from cloudflare for that URL).

Yeah sorry, tracked here: https://github.com/dotnet/nuget-trends/issues/207 DBs are generatad but not getting served, I'll look at it

btw: I can't see anything in the history for SentryTunnelMiddleware.cs that would have caused a regression.

I think this never worked, it's not regressed. What changed was NuGet Trends using tunneling.

bruno-garcia commented 11 months ago

https://github.com/getsentry/sentry-dotnet/blob/d4dda091cd4bc957d2b5f582fd3b3e83f9b157ab/src/Sentry.AspNetCore/SentryTunnelMiddleware.cs#L89-L93

This is using the client's connection IP and setting that as X-Forwarded-For. But it's not taking into account if that connection is already a proxy (I use Cloudflare), that itself includes X-Forwarded-For.

context.Connection?.RemoteIpAddress?.ToString(); might take that into account if the app has but there's some considerations:

Forwarded Headers Middleware default settings can be configured. For the default settings:

  • There is only one proxy between the app and the source of the requests.

Got this from: https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-7.0

rogerfar commented 4 months ago

@bruno-garcia I dug into this issue a bit because I ran into the same problem.

Make sure that UseForwardedHeaders is set before UseSentryTunneling, otherwise the context.Connection?.RemoteIpAddress won't be replaced.

bruno-garcia commented 4 months ago

@jamescrosswell does @rogerfar 's comment help debug it? would love getting this fixed too

jamescrosswell commented 1 month ago

@jamescrosswell does @rogerfar 's comment help debug it? would love getting this fixed too

Not sure how I missed this... massive apology for taking so long to reply!

In any event, @rogerfar 's comment probably would help resolve this if I could reproduce it. Without being able to reproduce, I could make some changes to the code and ask other people who can reproduce this if it helps, but I'm kind of stabbing in the dark.