handle multiple values in X-Forwarded-Proto #2162
Conversation
use the first entry, which should be the outermost value
|
I've pulled @minrk commit and can confirm that when deployed into AWS it has resolved the issue with JupyterHub thinking the user requested via http instead of https. |
tornado/httpserver.py
Outdated
| @@ -287,6 +287,9 @@ def _apply_xheaders(self, headers): | |||
| proto_header = headers.get( | |||
| "X-Scheme", headers.get("X-Forwarded-Proto", | |||
| self.protocol)) | |||
| if proto_header: | |||
| # use the outermost proto entry if there is more than one | |||
There was a problem hiding this comment.
The outermost entry is the least reliable, so it's probably not what we want here. I think we either want the innermost, or we want to skip the same number of entries here that we did with trusted_downstream in the X-Forwarded-For list.
There was a problem hiding this comment.
In the use-case I'm interested in, the value required is the outermost as I don't want to set a cookie to be Secure if the original request wasn't https.
However, I could see a use-case where the end user was connecting with http but the proxy is connecting to the backend with https and that may have different meaning for the backend in that instance.
There was a problem hiding this comment.
If you want to know whether the user's browser considers the request HTTPS, then even the outermost X-Forwarded-Proto isn't enough. Not all proxies set this header or support the multi-hop format here. I think if you want something that comes all the way from the user's browser you'll have to get it yourself with javascript.
There was a problem hiding this comment.
Matching the trusted_downstream behavior makes sense to me. I'll try to get that implemented soon.
There was a problem hiding this comment.
...except, now that I think of it, it doesn't because it seems most proxies don't in practice append to x-forwarded-proto, so there isn't a reliable mapping of X-Forwarded-For and X-Forwarded-Proto entries.
That would suggest to me that it needs a dedicated trusted_proto_depth setting to set the index to lookup.
I'll set it to [-1] in this PR, and the setting for picking the outermost trusted value can be handled separately.
There was a problem hiding this comment.
There was a problem hiding this comment.
@psyvision in your specific case, I think you'll be okay because both hops happen to be https.
I'll follow @bdarnell's recommendation and investigate a more direct way for JupyterHub to identify whether the browser views the request as coming over https in the future, since what we really care about in that particular case is the Browser's opinion, not any of the proxy hops.
minrk
force-pushed
the
forwarded-proto-list
branch
from
e8f3b5e to
80959e4
Compare
use the first entry, which should be the outermost value
closes #2161