-
-
Notifications
You must be signed in to change notification settings - Fork 226
fix: Sentry.Tunnel overwrites the X-Forwarded-For #4375
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
Conversation
|
|
||
| return string.IsNullOrEmpty(existingForwardedFor) | ||
| ? clientIp | ||
| : $"{existingForwardedFor}, {clientIp}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Header Malformation with Empty Values
The CreateXForwardedForHeader method malforms X-Forwarded-For headers when the incoming header contains only empty string values. This occurs because StringValues.ToString() returns a comma-only string (e.g., ',') for empty values, which string.IsNullOrEmpty() incorrectly evaluates as non-empty. This results in malformed headers like ', 192.168.1.1' or ', , 192.168.1.1' instead of only the client IP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In context:
sentry-dotnet/src/Sentry.AspNetCore/SentryTunnelMiddleware.cs
Lines 134 to 141 in b331224
| if (existingForwardedFor.Count == 0) | |
| { | |
| return clientIp; | |
| } | |
| return string.IsNullOrEmpty(existingForwardedFor) | |
| ? clientIp | |
| : $"{existingForwardedFor}, {clientIp}"; |
So existingForwardedFor will never be empty when this codepath is executed.
Resolves #1309:
Sentry.Tunneldoesn't includeX-Forwarded-To#1309Summary
The
X-Forwarded-Forheader is supposed to preserve a chain of IP addresses that the message was forwarded via (see docs).So for example it might contain:
Which is a specific instance of:
Previously, the
SentryTunnelMiddlewarewas overwriting this value so that it only included the IP address fromDefaultHttpContext.Connection.RemoteIpAddress... which was fine if the request didn't include anyX-Forwarded-Forbut, if the request was proxied (e.g. by CloudFlare) then effectively by doing that we delete the upstream IP addresses so we no longer know where the message came from (or the User's real IP address).Solution
This PR changes the behaviour of the
SentryTunnelMiddlewareso that theRemoteIpAddressis appended to theX-Forwarded-Forso that the client IP address is retained in the left most address from the list in theX-Forwarded-Forheader.