-
-
Notifications
You must be signed in to change notification settings - Fork 220
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.Tunnel
doesn't includeX-Forwarded-To
#1309Summary
The
X-Forwarded-For
header 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
SentryTunnelMiddleware
was 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-For
but, 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
SentryTunnelMiddleware
so that theRemoteIpAddress
is appended to theX-Forwarded-For
so that the client IP address is retained in the left most address from the list in theX-Forwarded-For
header.