Adding support for HTTP_FORWARDED #2036
Conversation
|
I think we should rely on Rack for parsing the header values. |
Hey @dentarg parsed using the utils form rack |
|
Just a heads-up that this will need to sit for a while. I have some related changes coming up but I won't be able to continue until August with them. I also want to sort out the CI issue, I have seen it before, and I can reproduce it in Docker. A bit strange it hasn't always happened here before. |
Hey @dentarg thank you for the info, let me know if anything else is required from me when this is ready to be merged... |
I'll just use this place to note the details about it (I have tested 3.1.5, 3.1.6): $ docker run -it --rm ruby:3.1.6 bash
root@5a9ca70740a1:/# gem -v
3.3.27
root@5a9ca70740a1:/# gem list psych
*** LOCAL GEMS ***
psych (default: 4.0.4)
root@5a9ca70740a1:/# gem install psych
Fetching psych-5.1.2.gem
Building native extensions. This could take a while...
Successfully installed psych-5.1.2
1 gem installed
root@5a9ca70740a1:/# gem install sass-embedded
Fetching sass-embedded-1.77.8.gem
Fetching google-protobuf-4.27.2-aarch64-linux.gem
Successfully installed google-protobuf-4.27.2-aarch64-linux
Building native extensions. This could take a while...
ERROR: Error installing sass-embedded:
ERROR: Failed to build gem native extension.
...
NoMethodError: undefined method `parse' for #<Psych::Parser:0x0000ffff87c2a920 @handler=#<Psych::Handlers::DocumentStream:0x0000ffff87c2aa60 @stack=[], @last=nil, @root=nil, @start_line=nil, @start_column=nil, @end_line=nil, @end_column=nil, @block=#<Proc:0x0000ffff87c2a948 /usr/local/lib/ruby/3.1.0/psych.rb:399>>, @external_encoding=0>
parser.parse yaml, filename
^^^^^^
/usr/local/bundle/gems/sass-embedded-1.77.8/ext/sass/Rakefile:162:in `fetch'
/usr/local/bundle/gems/sass-embedded-1.77.8/ext/sass/Rakefile:27:in `rescue in block in <top (required)>'
/usr/local/bundle/gems/sass-embedded-1.77.8/ext/sass/Rakefile:21:in `block in <top (required)>'
...It doesn't happen with only psych 4.0.4 installed. It can be solved with |
|
Hey @dentarg any update on this? |
|
Nope |
malikparvez
force-pushed
the
HTTP_FORWARDED
branch
from
4acdc87 to
fb09af1
Compare
| @@ -63,7 +63,7 @@ def preferred_type(*types) | |||
| alias secure? ssl? | |||
|
|
|||
| def forwarded? | |||
| @env.include? 'HTTP_X_FORWARDED_HOST' | |||
| @env.include?('HTTP_X_FORWARDED_HOST') || @env.include?('HTTP_FORWARDED') | |||
There was a problem hiding this comment.
This has been implemented (correctly) in main now
| def parse_forwarded(forwarded_header) | ||
| return [] unless forwarded_header_hash = Rack::Utils.forwarded_values(forwarded_header) | ||
|
|
||
| (forwarded_header_hash.fetch(:for, []) + forwarded_header_hash.fetch(:by, [])).map { |ip| ip.gsub(/\[|\]/, "") } | ||
| end |
There was a problem hiding this comment.
I think we should use the #forwarded_for method from Rack::Request, it will handled both X-Forwarded-For (when Forwarded isn't present) and Forwarded for=...). It is unfortunate that there's no such helper for by though.
I don't think the gsub here is working correctly:
irb(main):001> "[2001:db8:cafe::17]:4711".gsub(/\[|\]/, "")
=> "2001:db8:cafe::17:4711"Just wanted to documented how I tested
I started a simple Rack application like this: echo 'lowlevel_error_handler { |err| puts err.full_message }; require "rack"; app { |env| req=::Rack::Request.new(env); forwarded_values=::Rack::Utils.forwarded_values(env["HTTP_FORWARDED"]); [200, {}, ["#{req.forwarded_for}\n#{forwarded_values}"]] }' | puma --config /dev/stdin --port 4567 --log-requests
And tried various requests
$ curl localhost:4567 -H 'Forwarded: By="[2001:db8:cafe::17]:4711"'
{:by=>["[2001:db8:cafe::17]:4711"]}$ curl localhost:4567 -H 'Forwarded: by=192.0.2.60;proto=http;by=203.0.113.43'
{:by=>["192.0.2.60", "203.0.113.43"], :proto=>["http"]}
$ curl localhost:4567 -H 'Forwarded: For="[2001:db8:cafe::17]:4711"'
["2001:db8:cafe::17"]
{:for=>["[2001:db8:cafe::17]:4711"]}
Adding support for HTTP_FORWARDED wrt #2011