Skip to content

Better validation of HTTP_HOST and SERVER_NAME according to RFCs.#2298

Merged
ioquatix merged 1 commit intomainfrom
rack-lint-server_name
Feb 25, 2025
Merged

Better validation of HTTP_HOST and SERVER_NAME according to RFCs.#2298
ioquatix merged 1 commit intomainfrom
rack-lint-server_name

Conversation

@ioquatix
Copy link
Copy Markdown
Member

@ioquatix ioquatix commented Feb 25, 2025

Fixes #2295

@ioquatix ioquatix force-pushed the rack-lint-server_name branch 5 times, most recently from 5b3ad67 to e1e6771 Compare February 25, 2025 00:26
@ioquatix
Copy link
Copy Markdown
Member Author

ioquatix commented Feb 25, 2025

Found one issue (successfully):

Error:
HostAuthorizationTest#test_blocks_requests_with_invalid_hostnames:
Rack::Lint::LintError: env[HTTP_HOST] must be a valid authority: "attacker.com#x.example.com"
    /home/runner/work/rack/rack/lib/rack/lint.rb:359:in `check_environment'
    /home/runner/work/rack/rack/lib/rack/lint.rb:94:in `response'
    /home/runner/work/rack/rack/lib/rack/lint.rb:72:in `call'
    /opt/hostedtoolcache/Ruby/3.3.7/x64/lib/ruby/gems/3.3.0/gems/rack-test-2.2.0/lib/rack/test.rb:360:in `process_request'
    /opt/hostedtoolcache/Ruby/3.3.7/x64/lib/ruby/gems/3.3.0/gems/rack-test-2.2.0/lib/rack/test.rb:153:in `request'
    lib/action_dispatch/testing/integration.rb:297:in `process'
    lib/action_dispatch/testing/integration.rb:19:in `get'
    lib/action_dispatch/testing/integration.rb:388:in `get'
    test/dispatch/host_authorization_test.rb:448:in `block in <class:HostAuthorizationTest>'

cc @matthewd can we change this test in Rails to not use Rack::Lint as this is a malformed request according to the updated Rack::Lint specification.

jeremyevans
jeremyevans reviewed Feb 25, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I have a few comments, please let me know what you think.

@ioquatix ioquatix force-pushed the rack-lint-server_name branch 4 times, most recently from efee335 to 9ba88fd Compare February 25, 2025 04:09
@ioquatix ioquatix force-pushed the rack-lint-server_name branch from 9ba88fd to be0299c Compare February 25, 2025 04:20
@ioquatix ioquatix requested a review from jeremyevans February 25, 2025 04:21
jeremyevans
jeremyevans approved these changes Feb 25, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Looks good, but please wait until the issue is fixed in Rails so the external tests pass on master.

@ioquatix ioquatix mentioned this pull request Feb 25, 2025
Merged
@ioquatix ioquatix merged commit 90b440b into main Feb 25, 2025
31 checks passed
@ioquatix ioquatix deleted the rack-lint-server_name branch February 25, 2025 09:04
@ioquatix ioquatix mentioned this pull request Mar 4, 2025
Merged
dentarg added a commit to dentarg/sinatra that referenced this pull request Mar 16, 2025
@dentarg
CI: don't use Rack::Lint on invalid hostname
724c0e2 
Due to rack/rack#2298

- rack/rack#2295
- rails/rails#54613
@dentarg dentarg mentioned this pull request Mar 16, 2025
Merged
dentarg added a commit to sinatra/sinatra that referenced this pull request Mar 16, 2025
@dentarg
CI: don't use Rack::Lint on invalid hostname (#2086)
cfcc70d 
Due to better validation in Rack: rack/rack#2298

See also

- rack/rack#2295
- rails/rails#54613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremyevans jeremyevans jeremyevans approved these changes

@tenderlove tenderlove Awaiting requested review from tenderlove

@matthewd matthewd Awaiting requested review from matthewd

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rack::Lint SERVER_NAME and HTTP_HOST are insufficiently validated?

2 participants

@ioquatix @jeremyevans