-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix: http.client_ip vs multiple addresses #2282 #2284
Conversation
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.
The current behavior is to take whatever is returned by the first value. You have changed it so that if we get a header that has multiple values we only take the first.
I've not used this field much, but do you have any examples of why we shouldn't be moving the opposite direction? By which I mean try and combine all of the header fields into one long string?
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.
I just added one little change suggestion
Codecov Report
@@ Coverage Diff @@
## main #2284 +/- ##
=======================================
- Coverage 72.6% 72.6% -0.1%
=======================================
Files 172 172
Lines 11918 11919 +1
=======================================
Hits 8661 8661
Misses 3023 3023
- Partials 234 235 +1
|
@MadVikingGod I don't believe combining all the header values into one long string meets the specification. The HTTP Server Semantic Conventions describe
If they intended the value to contain the IP addresses of the original client and all proxies, I trust they would have said so. |
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.
Wait up: I shouldn't have accepted the suggestion without checking I understood the semantics. As shown in this play.golang.org example with n=1 we'll end up with all the addresses, every time. This is by the design of strings.SplitN
. From the docs:
SplitN slices s into substrings separated by sep and returns a slice of the substrings between those separators.
The count determines the number of substrings to return:
n > 0: at most n substrings; the last substring will be the unsplit remainder.
Instead we must call strings.SplitN
with n=2.
As suggested by @pellared. Good suggestion, that. Co-authored-by: Robert Pająk <pellared@hotmail.com>
c9b30e0
to
e4e4e7c
Compare
The tests caught the regression 🎉 Rebased and fixed. |
Sorry, my bad. It is good that we have unit tests 😄 |
I took the example from the MDN docs for X-Forwarded-For.
Resolves #2282