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

HTTP_HOST not parsing port correctly at 2.X #2070

Closed
caiofct opened this issue Apr 20, 2023 · 6 comments
Closed

HTTP_HOST not parsing port correctly at 2.X #2070

caiofct opened this issue Apr 20, 2023 · 6 comments
Milestone

Comments

@caiofct
Copy link

caiofct commented Apr 20, 2023

Hello, I think we might have a regression from this PR #1606.

With Rack 2.1.4.3 (Rack 3.X also works as expected)

irb(main):001:0> r = Rack::Request.new({"HTTP_HOST" => "some_service:3001"})
irb(main):002:0> r.host
=> "some_service"
irb(main):003:0> r.port
=> 3001

With Rack 2.2.6.4 (But any version at 2.2.X will behave the same way)

irb(main):001:0> r = Rack::Request.new({ "HTTP_HOST" => "some_service:3001" })
irb(main):002:0> r.host
=> "http://some_service:3001"
irb(main):003:0> r.port
=> nil

Am I missing something here?

Ruby: 2.7.8
Rails 7.0.4

@jeremyevans
Copy link
Contributor

Originally I thought this was a non issue, because your code for the failing example uses "HTTP_HOST" => "http://some_service:3001" (which is an invalid Host header as it includes the protocol). However, when using "HTTP_HOST" => "some_service:3001", the port is not split correctly from the host in Rack 2.2.

I checked some rack versions:

2.1.4.3, 3.0.3:  ["some_service", 3001]
2.2.0: ["some", nil]
2.2.2, 2.2.3, 2.2.3.1: ["some_service:3001", nil]

The issue is likely related to the use of _ in the host names. In DNS, _ is not a valid hostname character. However, for non-DNS host naming, _ may be valid.

A potential fix would be adding _ as a valid character here: https://github.com/rack/rack/blob/2-2-stable/lib/rack/request.rb#L611

@ioquatix
Copy link
Member

Is there a definition of valid hostname characters and or formats from any RFCs? i.e. what is the standard we should be following?

@caiofct
Copy link
Author

caiofct commented Apr 21, 2023

@jeremyevans Thanks for taking a look at it. The failing example with the protocol was my mistake. And as you tested it also fails without it. Also your point about valid dns hostnames makes sense and I wasn't considering it. The some_service host comes from docker compose which is a service we have and using underscore there is common practice however we could very easily use a dash instead of underscore and that would solve the issue but I wonder if rack 2.2 behavior compared to 2.1 or even 3.0 is not a regression.

Originally I thought this was a non issue, because your code for the failing example uses "HTTP_HOST" => "http://some_service:3001" (which is an invalid Host header as it includes the protocol). However, when using "HTTP_HOST" => "some_service:3001", the port is not split correctly from the host in Rack 2.2.

I checked some rack versions:

2.1.4.3, 3.0.3:  ["some_service", 3001]
2.2.0: ["some", nil]
2.2.2, 2.2.3, 2.2.3.1: ["some_service:3001", nil]

The issue is likely related to the use of _ in the host names. In DNS, _ is not a valid hostname character. However, for non-DNS host naming, _ may be valid.

A potential fix would be adding _ as a valid character here: https://github.com/rack/rack/blob/2-2-stable/lib/rack/request.rb#L611

jeremyevans added a commit to jeremyevans/rack that referenced this issue Apr 23, 2023
This makes Rack 2.2 behavior similar to Rack 2.1 and Rack 3.0 in
regards to underscore in host names.
@jeremyevans
Copy link
Contributor

I submitted #2071 to change the Rack 2.2 behavior to align with Rack 2.1 and 3.0. I'm not saying with the behavior is correct, but this at least makes it consistent.

ioquatix pushed a commit that referenced this issue Apr 24, 2023
This makes Rack 2.2 behavior similar to Rack 2.1 and Rack 3.0 in
regards to underscore in host names.
@ioquatix
Copy link
Member

The proposed fix was merged.

@ioquatix
Copy link
Member

Released rack 2.2.7 with the fix.

@ioquatix ioquatix added this to the v2.2.7 milestone Apr 24, 2023
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

No branches or pull requests

3 participants