-
Notifications
You must be signed in to change notification settings - Fork 221
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
tomharvey
wants to merge
13
commits into
rack:main
Choose a base branch
from
tomharvey:vendor-forwarded-proto
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
aaebc1e
migrating from rack
tomharvey fbe5006
fixing tests to work within rack-contrib harnass
tomharvey 79158f1
linting with rubocop
tomharvey 10963da
allow the middleware to be used for arbitrary and multiple headers
tomharvey bb21345
adding HeaderNameTransformer description
tomharvey fb1b50d
renaming test file for the new middleware name
tomharvey d37de4b
linting
tomharvey 8a8a0a5
Update lib/rack/contrib/header_name_transformer.rb
tomharvey 2ae7051
Update test/spec_rack_header_name_transformer.rb
tomharvey 2c0f962
whitespace
tomharvey db13706
test that the old header key still exists
tomharvey 85e4394
updating must_equal for minitest6 deprecation
tomharvey cca0b4f
updating test to cover multi-value header
tomharvey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# frozen_string_literal: true | ||
|
||
module Rack | ||
# 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"} | ||
# | ||
# There is a specific use case when ensuring the scheme matches when | ||
# comparing request.origin and request.base_url for CSRF checking, | ||
# but Rack expects that value to be in the X_FORWARDED_PROTO header. | ||
# | ||
# Example Rails usage: | ||
# If you use a vendor managed proxy or CDN which sends the proto in a header add | ||
# `config.middleware.use Rack::SetXForwardedProtoHeader, 'Vendor-Forwarded-Proto-Header', 'X-Forwarded-Proto'` | ||
tomharvey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# to your application.rb file | ||
|
||
class HeaderNameTransformer | ||
def initialize(app, vendor_header, forwarded_header) | ||
@app = app | ||
# Rack expects to see UPPER_UNDERSCORED_HEADERS, never SnakeCased-Dashed-Headers | ||
@vendor_header = "HTTP_#{vendor_header.upcase.gsub '-', '_'}" | ||
@forwarded_header = "HTTP_#{forwarded_header.upcase.gsub '-', '_'}" | ||
end | ||
|
||
def call(env) | ||
if (value = env[@vendor_header]) | ||
env[@forwarded_header] = value | ||
end | ||
@app.call(env) | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'minitest/autorun' | ||
require 'rack/contrib/runtime' | ||
|
||
describe Rack::HeaderNameTransformer do | ||
mpalmer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
response = ->(_e) { [200, {}, []] } | ||
|
||
it 'leaves the value of headers intact if there is no matching vendor header passed to override it in the request' do | ||
vendor_header = 'not passed in the request' | ||
env = Rack::MockRequest.env_for('/', 'HTTP_X_FORWARDED_PROTO' => 'http') | ||
|
||
Rack::Lint.new(Rack::HeaderNameTransformer.new(response, vendor_header, 'bar')).call env | ||
|
||
env['HTTP_X_FORWARDED_PROTO'].must_equal 'http' | ||
end | ||
|
||
it 'copy the value of the vendor header to a newly named header' do | ||
env = Rack::MockRequest.env_for('/', { 'HTTP_VENDOR' => 'value', 'HTTP_FOO' => 'foo' }) | ||
|
||
Rack::Lint.new(Rack::HeaderNameTransformer.new(response, 'Vendor', 'Standard')).call env | ||
Rack::Lint.new(Rack::HeaderNameTransformer.new(response, 'Foo', 'Bar')).call env | ||
|
||
env['HTTP_STANDARD'].must_equal 'value' | ||
env['HTTP_BAR'].must_equal 'foo' | ||
end | ||
|
||
# Real world headers and use cases | ||
it 'copy the value of a vendor forward proto header to the standardised formward proto header' do | ||
tomharvey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
env = Rack::MockRequest.env_for('/', 'HTTP_VENDOR_FORWARDED_PROTO_HEADER' => 'https') | ||
|
||
Rack::Lint.new( | ||
Rack::HeaderNameTransformer.new( | ||
response, | ||
'Vendor-Forwarded-Proto-Header', | ||
'X-Forwarded-Proto' | ||
) | ||
).call env | ||
|
||
env['HTTP_X_FORWARDED_PROTO'].must_equal 'https' | ||
end | ||
|
||
it 'copy the value of a vendor forward proto header to the standardised header, overwriting existing request value' do | ||
env = Rack::MockRequest.env_for( | ||
'/', | ||
'HTTP_VENDOR_FORWARDED_PROTO_HEADER' => 'https', | ||
'HTTP_X_FORWARDED_PROTO' => 'http' | ||
) | ||
|
||
Rack::Lint.new( | ||
Rack::HeaderNameTransformer.new( | ||
response, | ||
'Vendor-Forwarded-Proto-Header', | ||
'X-Forwarded-Proto' | ||
) | ||
).call env | ||
|
||
env['HTTP_X_FORWARDED_PROTO'].must_equal 'https' | ||
end | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It's probably worth describing how existing values in the "target" header will be impacted, since HTTP headers are multi-valued.
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.
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?
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.
If so, the change in the "Foo" header to contain "foo, bar" as the value should cover things.
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.
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.