Add HostAuthorization rack-protection middleware
#2053
Merged
Conversation
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
Closed
dentarg
force-pushed
the
host_authorization
branch
from
4b63d7d
to
691a573
Compare
The Sinatra project received a security report with the following details: > Title: Reliance on Untrusted Inputs in a Security Decision > CWE ID: CWE-807 > CVE ID: CVE-2024-21510 > Credit: t0rchwo0d > Description: The sinatra package is vulnerable to Reliance on Untrusted > Inputs in a Security Decision via the `X-Forwarded-Host (XFH)` header. > When making a request to a method with redirect applied, it is possible > to trigger an Open Redirect Attack by inserting an arbitrary address > into this header. If used for caching purposes, such as with servers > like Nginx, or as a reverse proxy, without handling the > `X-Forwarded-Host` header, attackers can potentially exploit Cache > Poisoning or Routing-based SSRF. The vulnerable code was introduced in fae7c01. Sinatra can not know whether the header value can be trusted or not without input from the app creator. This change introduce the `permitted_hosts` setting for that. It is implemented as a Rack middleware, bundled with rack-protection, but not exposed as a default nor opt-in protection. It is meant to be used by itself, as sharing reaction with other protections is not ideal, see sinatra#2012.
dentarg
force-pushed
the
host_authorization
branch
from
691a573
to
fb2d76d
Compare
This makes the test output read better.
In an attempt to improve the spec output further, I discovered that the tests depended on the local variables in unexpected ways.
No need to duplicate the test cases.
dentarg
changed the title
HostAuthorization protectionHostAuthorization rack-protection middleware
If a forwarded header is present, that's what Rack::Request#host returns. Applications may be making decisions based on the Host header, even when the forwarded header is present, so we should ensure both headers are permitted. Reference: rails/rails#33145 (comment)
I doubt this can happen for real, but better safe than sorry.
walro
reviewed
Nov 14, 2024
arielvalentin
added a commit
to arielvalentin/opentelemetry-ruby-contrib
that referenced
this pull request
Nov 19, 2024
Sinatra 4.1 now includes a stricter host authorization check by default, which caused our tests to fail. The host authorization is not enforced in development and test mode so this PR sets the Sinatra environment to run in `test` mode. See: sinatra/sinatra#2053 fixes open-telemetry#1252
kaylareopelle
added a commit
to open-telemetry/opentelemetry-ruby-contrib
that referenced
this pull request
Nov 19, 2024
* test: Fix Sinatra Tests Sinatra 4.1 now includes a stricter host authorization check by default, which caused our tests to fail. The host authorization is not enforced in development and test mode so this PR sets the Sinatra environment to run in `test` mode. See: sinatra/sinatra#2053 fixes #1252 * squash: update appraisals --------- Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
|
Please let me know if there are any plans to backport this to Sinatra 3.x. We unfortunately are stuck with Rack 2.x ATM |
|
No such plans exist. |
csutter
added a commit
to alphagov/search-api
that referenced
this pull request
Nov 20, 2024
Sinatra 4.1 ships with a breaking change that [enables host allowlisting middleware][1] in local development, which in turn breaks running Search API through GOV.UK Docker. This adds an explicit `permitted_hosts` setting for the middleware, which includes the `search-api.dev.gov.uk` host used by GOV.UK Docker. [1]: sinatra/sinatra#2053
joshuafleck
added a commit
to meetcleo/avro_turf
that referenced
this pull request
Nov 20, 2024
Per [this](sinatra/sinatra#2053) change, "Defaults to .localhost, .test and any IP address in development mode." We want our fake server to be able to respond to any request, hence we allow all hosts.
csutter
added a commit
to alphagov/search-api
that referenced
this pull request
Nov 20, 2024
Sinatra 4.1 ships with a breaking change that [enables host allowlisting middleware][1] in local development, which in turn breaks running Search API through GOV.UK Docker. This adds an explicit `permitted_hosts` setting for the middleware in the development environment, which allows access with arbitrary host headers. [1]: sinatra/sinatra#2053
benoittgt
added a commit
to benoittgt/webmock
that referenced
this pull request
Nov 22, 2024
While working on this sinatra/sinatra#2053 in our project. I noticed than when using Webmock, sinatra logs and especially the enforced rack-protection were showing this kind of logs: ``` D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil ``` As you can see, `origin_host` is empty, because the header is missing. When not using webmock, we fallback on `net/http` host header setup. https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
benoittgt
added a commit
to benoittgt/webmock
that referenced
this pull request
Nov 22, 2024
While working on this sinatra/sinatra#2053 in our project. I noticed than when using Webmock, sinatra logs and especially the enforced rack-protection were showing this kind of logs: ``` D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil ``` As you can see, `origin_host` is empty, because the header is missing. When not using webmock, we fallback on `net/http` host header setup. https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
benoittgt
added a commit
to benoittgt/webmock
that referenced
this pull request
Nov 22, 2024
While working on this sinatra/sinatra#2053 in our project. I noticed than when using Webmock, sinatra logs and especially the enforced rack-protection were showing this kind of logs: ``` D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil ``` As you can see, `origin_host` is empty, because the header is missing. When not using webmock, we fallback on `net/http` host header setup. https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
|
For people who use webmock. You could have surprising result. I've open a patch to set the host in case of mock of |
YOU54F
added a commit
to pact-foundation/pact-ruby
that referenced
this pull request
Nov 29, 2024
affects sinatra users 4.1.x post sinatra/sinatra#2053
YOU54F
added a commit
to pact-foundation/pact-ruby
that referenced
this pull request
Nov 29, 2024
affects sinatra users 4.1.x post sinatra/sinatra#2053
TylerHelmuth
added a commit
to honeycombio/libhoney-rb
that referenced
this pull request
Dec 2, 2024
## Which problem is this PR solving? - [Sinatra 4.1.0 introduced a host authorization option to Sinatra apps](sinatra/sinatra#2053). That authorization check was failing requests made in tests. ## Short description of the changes - Update the Honeycomb test server stub to allow anybody to talk to it. Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
robbkidd
added a commit
to honeycombio/beeline-ruby
that referenced
this pull request
Dec 2, 2024
## Which problem is this PR solving? - [Sinatra 4.1.0 introduced a host authorization option to Sinatra apps](sinatra/sinatra#2053). That authorization check was failing requests made in tests. ## Short description of the changes - Update the test Sinatra server to allow anybody to talk to it. Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
robbkidd
added a commit
to honeycombio/beeline-ruby
that referenced
this pull request
Dec 2, 2024
## Which problem is this PR solving? - [Sinatra 4.1.0 introduced a host authorization option to Sinatra apps](sinatra/sinatra#2053). That authorization check was failing requests made in tests. ## Short description of the changes - Update the test Sinatra server to allow anybody to talk to it. --------- Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The Sinatra project received a security report with the following details:
The vulnerable code was introduced in fae7c01. Sinatra can not know whether the header value can be trusted or not without input from the app creator. This change introduce the
host_authorizationsettings for that.It is implemented as a Rack middleware, bundled with rack-protection, but not exposed as a default nor opt-in protection. It is meant to be used by itself, as sharing reaction with other protections is not ideal (see #2012).