Skip to content
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

Middleware to handle vendor forwarded proto #193

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tomharvey
Copy link

@tomharvey tomharvey commented Jun 5, 2024

@mpalmer sorry to raise this separately and lose your already valuable feedback, but I'd like to own the code that I've contributed.

This is to solve the issue raised in rack/rack#2080

When routing from cloudfront -> ALB -> puma (on ECS) we want to use the existing cloudfront header to inform rack of the protocol in use.

But, we also want to keep this implementation vendor agnostic.

More context can be seen in the feedback leading to this code in rack/rack#2089

Is it best to squash my commits before any merge?

And, I assume I should add to the README?

@tomharvey
Copy link
Author

tomharvey commented Jun 5, 2024

@mpalmer i think this is a great suggestion to further broaden the functionality. I’ll work on that this evening. And the resulting name change.

A generic Foo-Forwarded-* -> Forwarded transmogrifier would probably be the platonic ideal -- I just didn't want to let the enemy be the perfect of the "well, I guess it's better than nothing" 😀 . I can't find any existing support for (eg) CloudFront-Viewer-Address anywhere in Rack, so presumably most people in this situation would want that header translated as well, at the very least.

@ioquatix ioquatix self-requested a review June 5, 2024 17:11
@tomharvey
Copy link
Author

tomharvey commented Jun 8, 2024

@mpalmer I've reworked this so it can be used to handle the transformation of any header name. And, so it can be used multiple times.

Is this in line with your suggestion?

I've also tried to rename it given all those changes.

The tests still include X-Forwarded-Proto specific cases, but also the more generic ones.

@tomharvey
Copy link
Author

@ioquatix would love that review when you've got time.

Copy link
Contributor

@mpalmer mpalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin' good, I've just got a few observations and typos that need fixing.

lib/rack/contrib/header_name_transformer.rb Outdated Show resolved Hide resolved
# Middleware to change the name of a header
#
# So, if a server upstream of Rack sends {'X-Header-Name': "value"}
#  you can change header to {'Whatever-You-Want': "value"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth describing how existing values in the "target" header will be impacted, since HTTP headers are multi-valued.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I misunderstand RFC 2616 the multiple values would come into this stage of middleware as {'Whatever-You-Want': "value1, value2"}.

I've also seen https://github.com/rack/rack/blob/ed25abcde2a64a937efa1e59f1a0bb53d7ccecb8/test/spec_files.rb#L213C10-L213C20 where the multiple values are being passed into HTTP_RANGE.

Is this the kind of multi-value you're talking about?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, the change in the "Foo" header to contain "foo, bar" as the value should cover things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't clear that I was referring to the situation where (for whatever reason) there was already a value in (per the example) Whatever-You-Want. The current code overwrites the existing value, and that's a reasonable behaviour, but it's worth clearly stating that in the docs, and having the tests ensure no regressions in that behaviour.

test/spec_rack_header_name_transformer.rb Outdated Show resolved Hide resolved
test/spec_rack_header_name_transformer.rb Show resolved Hide resolved
Copy link
Contributor

@mpalmer mpalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in re-review. I've commented in the main issue to clarify what I meant on that previous review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants