Cloudfront forwarded proto header #2089

tomharvey · 2023-06-24T19:13:25Z

Ok - taking another swing at solving #2080

This time where we can pass any header in and the value of that will be copied to the value of HTTP_X_FORWARDED_PROTO

This allows the user to rely on the value in CloudFront-Forwarded-Proto provided by AWS Cloudfront. If other vendors have custom headers for this, then this should also provide an option.

jeremyevans

Thanks for the pull request. Please see the requested changes.

lib/rack/set_x_forwarded_proto_header.rb

test/spec_set_x_forwarded_proto_header.rb

lib/rack/set_x_forwarded_proto_header.rb

jeremyevans

Looks good. One more minor spec change requested, then I'm fine with this.

test/spec_set_x_forwarded_proto_header.rb

jeremyevans

Looks good, thanks!

tomharvey · 2023-06-27T22:25:35Z

Looks good, thanks!

Great. Thanks for the speedy and clear feedback on the review

ioquatix · 2024-03-20T22:15:13Z

Do you mind checking why the tests are failing?

DEPRECATED: Use assert_nil if expecting nil from /home/runner/work/rack/rack/test/spec_set_x_forwarded_proto_header.rb:25. This will fail in Minitest 6.

tomharvey · 2024-03-20T23:51:55Z

will have a look tomorrow.

Do you mind checking why the tests are failing?

jeremyevans · 2024-05-30T18:22:34Z

I'm OK with merging this if the conflicts and test issues are fixed.

tenderlove · 2024-05-30T18:57:11Z

I'm OK with merging this if the conflicts and test issues are fixed.

+1

Seems like there's a merge conflict committed.

@tomharvey after you fix the conflicts would you mind squashing this PR? Thank you!

tomharvey · 2024-05-30T19:39:02Z

I'm OK with merging this if the conflicts and test issues are fixed.

+1

Seems like there's a merge conflict committed.

@tomharvey after you fix the conflicts would you mind squashing this PR? Thank you!

Hey hey - will do.

Sorry, thought I'd switch it over to a draft while I sorted out my nonsense. Sorry for the noise.

HTTP_ prefix renaming middleware class to be vendor agnostic testing for early return on missing vendor forwarded header renaming files to match classnames removing unused imports assume uppercase header from client adding a note on usage transforming the header string removing AWS mention in the example docs removing Cloudfront specific headers from the specs inlining the header string transformation sinplifying the call method adding a test to verify that an existing X-Forwarded-Proto header is overwritten by vendor header value adding to README and Changelog fixing spec to check header is unset instead of testing for old method updating the spec description explicit requires for seperate testing

tomharvey · 2024-05-30T20:48:07Z

I'm OK with merging this if the conflicts and test issues are fixed.

+1
Seems like there's a merge conflict committed.
@tomharvey after you fix the conflicts would you mind squashing this PR? Thank you!

Hey hey - will do.

Sorry, thought I'd switch it over to a draft while I sorted out my nonsense. Sorry for the noise.

Fixed my obvious doofus errors.

Squashed

Tests run locally in ruby 3.2.2 - can I have some workflows to check the other versions please?

ioquatix · 2024-05-31T00:11:43Z

I understand the problem trying to be solved with this middleware, but after consideration, I disagree with merging this PR into rack.

As discussed, it seems extremely vendor specific and doesn't align with the goals of "rack the interface". As such, I think this would be a suitable candidate for rack-contrib instead. By vendor specific, I mean that a normal HTTP server handling traffic from a browser would not need this middleware - only vendor-specific load balancers that aren't following the standards require such a mapping.
X-Forwarded-Proto is a de-facto standard at best, and the actual standard is the Forwarded header. I think we should be encouraging use of the Forwarded header and we support this in Rack::Request. It would make more sense to me to have a mapping for X-Forwarded-* -> Forwarded and as a feature, we could specify the prefix, e.g. Foo-Forwarded-*.

As such, I'd like to revert this PR and suggest moving it to rack-contrib, while additionally considering whether we can follow the published standards (e.g. the Forwarded header).

tomharvey · 2024-06-05T08:28:44Z

I understand the problem trying to be solved with this middleware, but after consideration, I disagree with merging this PR into rack.

As discussed, it seems extremely vendor specific and doesn't align with the goals of "rack the interface". As such, I think this would be a suitable candidate for rack-contrib instead. By vendor specific, I mean that a normal HTTP server handling traffic from a browser would not need this middleware - only vendor-specific load balancers that aren't following the standards require such a mapping.

X-Forwarded-Proto is a de-facto standard at best, and the actual standard is the Forwarded header. I think we should be encouraging use of the Forwarded header and we support this in Rack::Request. It would make more sense to me to have a mapping for X-Forwarded-* -> Forwarded and as a feature, we could specify the prefix, e.g. Foo-Forwarded-*.

As such, I'd like to revert this PR and suggest moving it to rack-contrib, while additionally considering whether we can follow the published standards (e.g. the Forwarded header).

I’m unclear if the above is a proposal for the team to consider or a decision on the way forward. Can you clarify how the rack team makes these kinds of decisions?

If rack-contrib is the way forward; I’d appreciate the opportunity to move the code to rack-contrib myself.

ioquatix · 2024-06-05T11:36:44Z

I’m unclear if the above is a proposal for the team to consider or a decision on the way forward. Can you clarify how the rack team makes these kinds of decisions?

It's now a decision on the way forward. Apologies for being opaque, but sufficient consensus was reached within the @rack/release-managers team.

If rack-contrib is the way forward; I’d appreciate the opportunity to move the code to rack-contrib myself.

Please do - either continue the work from here (a copy of this PR) or open a new one. I'd be delighted if you could continue the work.

tomharvey · 2024-06-05T14:08:28Z

I'd prefer to open a new one, with apologies to @mpalmer that the PR will lose his valuable feedback.

My motivation is based on an opportunity to contribute a useful feature to a project that has given the community so much. But, I do still expect that my contributions would be recognised as my own instead of a PR from @ioquatix

tomharvey closed this Jun 24, 2023

tomharvey reopened this Jun 24, 2023

jeremyevans requested changes Jun 26, 2023

View reviewed changes

tomharvey requested a review from jeremyevans June 27, 2023 19:02

jeremyevans requested changes Jun 27, 2023

View reviewed changes

test/spec_set_x_forwarded_proto_header.rb Show resolved Hide resolved

test/spec_set_x_forwarded_proto_header.rb Outdated Show resolved Hide resolved

tomharvey requested a review from jeremyevans June 27, 2023 20:06

jeremyevans approved these changes Jun 27, 2023

View reviewed changes

tomharvey marked this pull request as draft May 30, 2024 18:59

tomharvey force-pushed the cloudfront-forwarded-proto-header branch 4 times, most recently from bca8dca to f30aae3 Compare May 30, 2024 20:27

tomharvey marked this pull request as ready for review May 30, 2024 20:27

tomharvey force-pushed the cloudfront-forwarded-proto-header branch from f30aae3 to 05e7b61 Compare May 30, 2024 20:46

jeremyevans merged commit 62cf545 into rack:main May 30, 2024
14 of 15 checks passed

tomharvey deleted the cloudfront-forwarded-proto-header branch May 30, 2024 20:58

jeremyevans mentioned this pull request May 30, 2024

Handle CloudFront-Forwarded-Proto as a lower priority X-Forwarded-sheme option #2080

Closed

This was referenced Jun 5, 2024

Revert "adding middleware to use CloudFront-Forwarded-Proto in CSRF" #2187

Merged

Middleware for handling vendor-specific $vendor-forwarded-proto -> x-forwarded-proto. rack/rack-contrib#192

Closed

tomharvey mentioned this pull request Jun 5, 2024

Middleware to handle vendor forwarded proto rack/rack-contrib#193

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloudfront forwarded proto header #2089

Cloudfront forwarded proto header #2089

tomharvey commented Jun 24, 2023

jeremyevans left a comment

jeremyevans left a comment

jeremyevans left a comment

tomharvey commented Jun 27, 2023

ioquatix commented Mar 20, 2024

tomharvey commented Mar 20, 2024

jeremyevans commented May 30, 2024

tenderlove commented May 30, 2024

tomharvey commented May 30, 2024

tomharvey commented May 30, 2024

ioquatix commented May 31, 2024 •

edited

tomharvey commented Jun 5, 2024

ioquatix commented Jun 5, 2024

tomharvey commented Jun 5, 2024

Cloudfront forwarded proto header #2089

Cloudfront forwarded proto header #2089

Conversation

tomharvey commented Jun 24, 2023

jeremyevans left a comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

tomharvey commented Jun 27, 2023

ioquatix commented Mar 20, 2024

tomharvey commented Mar 20, 2024

jeremyevans commented May 30, 2024

tenderlove commented May 30, 2024

tomharvey commented May 30, 2024

tomharvey commented May 30, 2024

ioquatix commented May 31, 2024 • edited

tomharvey commented Jun 5, 2024

ioquatix commented Jun 5, 2024

tomharvey commented Jun 5, 2024

ioquatix commented May 31, 2024 •

edited