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
Support RFC 7239: HTTP Forwarded header #1017
Conversation
[RFC 7239][] is an IETF proposed standard for a HTTP `Forwarded` request header that aims to replace non-standard `X-Forwarded-*` headers. This commit supports all relevant tokens that may be set in the Forwarded header: - proto - host - for I have prioritised the Forwarded header over the non-standard headers as I think it's preferable to use the standard header when specified. Multiple values for each token are allowed by the standard. When multiple values are supplied, I've followed these rules: - for the `proto` token, use the first value specified as it's most likely to be the protocol used by the original client - for the `host` token, use the last value specified following the convention already used by Rack for multiple `X-Forwarded-For` headers When both `X-Forwarded-For` and `Forwarded` headers are supplied and the latter includes `for` values, I've concatenated both sets of values with the `Forwarded` values coming last. I've been careful to support mixed case in the header and optional double quotes encapsulating the token value as specified in the RFC. [RFC 7239]: https://tools.ietf.org/html/rfc7239
Is this a useful addition? I'd be really grateful for any feedback. 😎 |
|
||
msg = FORMAT % [ | ||
env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-", | ||
forwarded_for || env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-", |
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.
Seems this is kinda-implementing Rack::Request#ip
, maybe nicer to reuse that.
I think this will need to be made configurable now. The problem here is that proxies that have yet to be updated will not strip these headers, and also will not know to overwrite them. A failure to protect against outside spoofing can have important security implications. Users must manage the transition with care. |
Sorry for the lack of activity and thanks for the feedback. Hopefully I'll get time to work some more on this in the next couple of weeks. @raggi: Would you suggest disabling this functionality by default? |
I would not recommend making major API changes without changing major versions, ideally. This situation is unfortunate and tricky, but I don't think we can safely just break this. |
What's the status of this? |
Can we rebase this and land it in Rack 3.x? |
@ioquatix I will move this forward if @mattbostock don't want (have time) to. |
I haven't reviewed this PR for a while, but feel free to rebase it and we can go from there. |
It seems more likely we will use the updated version of this (#1423) than this, so I'm going to close this now. |
RFC 7239 is an IETF proposed standard for a HTTP
Forwarded
requestheader that aims to replace non-standard
X-Forwarded-*
headers.This commit supports all relevant tokens that may be set in the
Forwarded header:
I have prioritised the Forwarded header over the non-standard headers
as I think it's preferable to use the standard header when specified.
Multiple values for each token are allowed by the standard. When
multiple values are supplied, I've followed these rules:
proto
token, use the first value specified as it's mostlikely to be the protocol used by the original client
host
token, use the last value specified following theconvention already used by Rack for multiple
X-Forwarded-For
headers
When both
X-Forwarded-For
andForwarded
headers are supplied and thelatter includes
for
values, I've concatenated both sets of values withthe
Forwarded
values coming last.I've been careful to support mixed case in the header and optional
double quotes encapsulating the token value as specified in the RFC.