Support RFC 7239 Forwarded header and configurable priorities for checking forwarding headers #1834
Conversation
Co-authored-by: Matt Bostock <matt@mattbostock.com> Co-authored-by: Jeremy Evans <code@jeremyevans.net>
The Request.forwarded_priority accessor sets the priority. Default to considering Forwarded first, since it is now the official standard. Also allow configuring whether X-Forwarded-Proto or X-Forwarded-Scheme has priority, using the Request.x_forwarded_proto_priority accessor. Allowing configurable priorities for these headers is necessary, because which headers should be checked depends on the environment the application runs in. Make Request#forwarded_authority use the last forwarded authority instead of the first forwarded authority, since earlier forwarded authorities can be forged by the client. Fixes rack#1809 Fixes rack#1829 Implements rack#1423 Implements rack#1832
Only used in one place and no interpolation, there is no reason to use a constant in this case.
|
This generally looks really great to me, I just want to understand a bit more what the expected arguments and return values should be. I also wonder how much of this should be public and whether we should make some methods private. I am travelling from tomorrow so my review would be delayed. |
| allowed_scheme(get_header(HTTP_X_FORWARDED_SCHEME)) || | ||
| allowed_scheme(extract_proto_header(get_header(HTTP_X_FORWARDED_PROTO))) | ||
| forwarded_priority.each do |type| | ||
| case type |
There was a problem hiding this comment.
I feel using a case statement here is a little bit cumbersome. Like, we know in advance what code should be executed. Using an array is flexible and could even contain the same entry more than once even thought it doesn't make sense.
Thinking longer term, does the symbol list in forwarded_priority make it hard to evolve this implementation? If we wanted to get rid of the case statement and make the internal implementation more streamlined.
Does it matter? My main thought is performance. I guess probably not. If it was a big issue, maybe later we can construct a lambda based on the expected behaviour and just cache it?
I guess the alternative would be self.send(:"forwarded_scheme_#{type}")??? This would allow users to introduce their own methods if they had a strong desire to do so. Another option would be Rack::Request mixins using prepend which call super...
There was a problem hiding this comment.
I'm not attached to the particular approach used. If we want to change to use send instead of case, that seems fine with me. The important part is that the solution implemented allows the user to choose which header to respect in which order, and allows configuring to explicitly ignore either header or both headers.
There was a problem hiding this comment.
Do you have the energy to make that change and do you prefer it? I'm happy to defer to your judgement here.
There was a problem hiding this comment.
I'm fine keeping the case. We can optimize using send later in a backwards compatible manner if it turns out to be a bottleneck.
|
Thanks for your hard work. |
| @@ -47,6 +47,8 @@ All notable changes to this project will be documented in this file. For info on | |||
| - Explicitly deprecate `Rack::File` which was an alias for `Rack::Files`. ([#1811](https://github.com/rack/rack/pull/1720), [@ioquatix](https://github.com/ioquatix)). | |||
| - Moved `Rack::Session` into [separate gem](https://github.com/rack/rack-session). ([#1805](https://github.com/rack/rack/pull/1805), [@ioquatix](https://github.com/ioquatix)) | |||
| - rackup -D option to daemonizes no longer changes the working directory to the root. ([#1813](https://github.com/rack/rack/pull/1813), [@jeremyevans](https://github.com/jeremyevans)) | |||
| - The X-Forwarded-Proto header is now considered before the X-Forwarded-Scheme header for determining the forwarded protocol. `Rack::Request.x_forwarded_proto_priority` accessor has been added for configuring the priority of which header to check. ([#1809](https://github.com/rack/rack/issues/1809), [@jeremyevans](https://github.com/jeremyevans)) | |||
| - `Rack::Request.forwarded_authority` (and methods that call it, such as `host`) now returns the last authority in the forwarded header, instead of the first, as earlier forwarded authorities can be forged by clients. This restores the Rack 2.1 behavior. ([#1829](https://github.com/rack/rack/issues/1809), [@jeremyevans](https://github.com/jeremyevans)) | |||
There was a problem hiding this comment.
This restores the Rack 2.1 behavior.
Would it be possible to clearly mark this change as a breaking change in the changelog of the release where this different behavior was included?
Would it also be possible to backport the fix to the v2 branch?
There was a problem hiding this comment.
Do you mean backport this entire PR?
This continues @fatkodima's work from #1423, rebasing it against the master branch and making changes so it applies.
To allow Rack users to choose the appropriate forwarding headers to inspect for their environment, this sets up configurable priorities using
Rack::Request.forwarded_priority. This defaults to[:forwarded, :x_forwarded], to check theForwardedheader before theX-Forwarded-*headers.Additionally, to handle the
X-Forwarded-Protovs.X-Forwarded-Schemesecurity issue (#1809), this sets up configurable priorities for that usingRack::Request.x_forwarded_proto_priority. This defaults to[:proto, :scheme], to checkX-Forwarded-ProtobeforeX-Forwarded-Scheme.In both cases, you can omit one of the priorities to never check the related header, or use an empty array to not check any forwarding headers, which is useful if your application does not use a reverse proxy, and you don't want to allow clients to forge forwarded information.
While here, fix #1829 by making
forwarded_authorityuse the last forwarded authority and not the first, since previous forwarded authorities can be forged by clients.