When all IPs are trusted, use the furthest away#38150
Merged
rafaelfranca merged 2 commits intorails:masterfrom Jan 3, 2020
Merged
When all IPs are trusted, use the furthest away#38150rafaelfranca merged 2 commits intorails:masterfrom
rafaelfranca merged 2 commits intorails:masterfrom
Conversation
Scenario: we have a REMOTE_ADDR of `127.0.0.1`, and X-Forwarded-For is `A, B, C`. Without any relevant trust, the `remote_ip` is `C`. If `C` is trusted, then the `remote_ip` is `B`. If `B` and `C` are trusted, then the `remote_ip` is `A`. If all of `A`, `B`, and `C` are trusted, then the `remote_ip` should still be `A`: if our trust was sufficient to get that far out before, trusting something else should not have us fall back to `127.0.0.1`. It is this last situation that we're correcting here: We trust `A` to give us accurate X-Forwarded-For information, yet it has chosen to leave it unset. Therefore, `A` is telling us that it is itself the client.
Member
|
Can you add a CHANGELOG entry? |
Contributor
Author
|
@rafaelfranca thanks for looking at it. let me know if you want something else in the change log. Happy weekend |
rafaelfranca
added a commit
that referenced
this pull request
Jan 3, 2020
When all IPs are trusted, use the furthest away
Contributor
Author
|
Thanks so much @rafaelfranca (and @matthewd for putting this together) |
|
So glad someone has resurrected this! 😄 Is it likely to be in a 5.x release any time soon? We are upgrading a Rails app to v5 atm but I'm not sure if we'll get as far as 6. |
|
What is the process for getting this into a release? |
Member
|
@jimbali This will be released in Rails 6.1. In accordance with Rails' Maintenance Policy, Rails 5.2 receives only security patches, and Rails 5.1 and below are unsupported. |
|
All the more reason to upgrade! 😊 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a rebase of the stale #14600.
From reading the thread, it looked like no logic changes need to be made, it just needed style and merge updates.
/cc @indirect @jeremy @matthewd
Other Information
As far as I could tell, the merge conflicts were from an added test (bf14a8e) and style changes (e8ba0c0 and 35b3de8). I did move 2 asserts a few lines up, but besides that it should be unchanged.
Original PR Summary
Scenario: we have a REMOTE_ADDR of
127.0.0.1, and X-Forwarded-For isA, B, C.Without any relevant trust, the
remote_ipisC.If
Cis trusted, then theremote_ipisB.If
BandCare trusted, then theremote_ipisA.If all of
A,B, andCare trusted, then theremote_ipshouldstill be
A: if our trust was sufficient to get that far out before,trusting something else should not have us fall back to
127.0.0.1.It is this last situation that we're correcting here:
We trust
Ato give us accurate X-Forwarded-For information, yet it haschosen to leave it unset. Therefore,
Ais telling us that it is itselfthe client.