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

Sentry dotnet SDK is blowing away User.IpAddress if we set it explicitly and SendDefaultPii is true #3337

Open
nicholashead opened this issue Apr 29, 2024 · 4 comments
Assignees
Labels
ASP.NET Bug Something isn't working

Comments

@nicholashead
Copy link

nicholashead commented Apr 29, 2024

Package

Sentry

.NET Flavor

.NET

.NET Version

4.7.2

OS

Windows

SDK Version

4.4.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

  1. Set SendDefaultPii to true during SentrySdk.Init call
  2. Create new SentryEvent - and set the event's User.IpAddress to a specific value.
  3. Call SentrySdk.CaptureEvent and pass in the event.

Expected Result

The Sentry dashboard should show the User's IP as the one we set.

Actual Result

It appears to always contain an internal/server IP instead. I believe this may be because of this code?

@event.User.IpAddress = context.Request.UserHostAddress;

But I am honestly not certain.

If I am doing something wrong here, would love guidance. But we have our own logic that fires off before a Sentry event sends - and if it's inside a web requests - gets the current user context (who's logged in) and IP address data, then stamps it onto the SentryEvent. We get the IP address ourselves because we're parsing CloudFlare/proxy IPs in the HTTP headers.

@jamescrosswell
Copy link
Collaborator

Hi @nicholashead , thanks for raising this.

SystemWebRequestEventProcessor

At first glance, I think you're right about the SystemWebRequestEventProcessor.

I did notice we have this test which is supposed to avoid the situation you're describing (you set the User.IpAddress explicitly and then our processor overwrites it). That functionality is in the MainSentryEventProcessor (ultimately the Enricher):

eventLike.User.IpAddress ??= DefaultIpAddress;

That event processor gets set on the SentryClient so I believe it runs for both ASP.NET Core and ASP.NET. I think it's getting overridden in ASP.NET by the code you highlighted in SystemWebRequestEventProcessor

REMOTE_ADDR

@nicholashead this won't affect you as you're setting the IP Address explicitly yourselves, but for ASP.NET Core Sentry also has these (which ensure Automatic IP address resolution works):

var host = context.Request.Host.Host;

// these might be reported in a non CGI, old-school way
if (options.SendDefaultPii
&& context.Connection.RemoteIpAddress?.ToString() is { } ipAddress)
{
scope.Request.Env["REMOTE_ADDR"] = ipAddress;
}

When building classic ASP.NET (framework) applications none of the mechanics to populate information in the scope seems to exist. So we probably need to wire up Scope.Request.Env["REMOTE_ADDR"] in ASP.NET too, for the Auto IP Address resolution to work.

@bruno-garcia / @bitsandfoxes is there any additional context for whether/how this should work on ASP.NET? I can't see any equivalent in ASP.NET of the ASP.NET Core scope extensions, which populate this information. It's slightly more work to obtain this information in ASP.NET as we'd need to read it from headers ourselves but I think we could at the very least check for an X-Forwarded-For.

@nicholashead
Copy link
Author

nicholashead commented Apr 30, 2024

Thanks for the research! Really appreciate it. The only reason we’re having issues with this is because of the proxied traffic, so if you were able to improve the default logic to look at the forwarded for header that’d be great. We’re still stuck on older .NET framework for one of our apps for now…

@bitsandfoxes
Copy link
Contributor

bitsandfoxes commented May 8, 2024

There is something off with either the order of processor execution here or the processor is not supposed to simply overwrite data that has already been set.

When building classic ASP.NET (framework) applications none of the mechanics to populate information in the scope seems to exist.

Wdym? Doesn't the application of the scope be taken care of by the client? As part of the regular event processing in the core package?

@bitsandfoxes bitsandfoxes added Bug Something isn't working ASP.NET labels May 8, 2024
@jamescrosswell
Copy link
Collaborator

Wdym? Doesn't the application of the scope be taken care of by the client? As part of the regular event processing in the core package?

We have some ScopeExtensions for ASP.NET Core that get trigged by the SentryMiddleware. I can't see any equivalent of this for ASP.NET.

@bitsandfoxes bitsandfoxes self-assigned this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Bug Something isn't working
Projects
Status: No status
Status: No status
Development

No branches or pull requests

3 participants