From 69e70042969a68ed4f7f9a54d13b2ac0f1947fcf Mon Sep 17 00:00:00 2001 From: "Peter H. Boling" Date: Wed, 28 Oct 2026 03:46:19 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Block=20cross-origin=20redirects?= =?UTF-8?q?=20by=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .rubocop_gradual.lock | 16 ++++++++-------- CHANGELOG.md | 2 +- README.md | 6 ++++++ lib/oauth2/client.rb | 17 ++++++++++++++++- spec/oauth2/client_spec.rb | 38 +++++++++++++++++++++++++++++++++++++- 5 files changed, 68 insertions(+), 11 deletions(-) diff --git a/.rubocop_gradual.lock b/.rubocop_gradual.lock index 4f00db8a..b16e71b2 100644 --- a/.rubocop_gradual.lock +++ b/.rubocop_gradual.lock @@ -2,8 +2,8 @@ "bin/bundle:3474730829": [ [66, 5, 20, "ThreadSafety/ClassInstanceVariable: Avoid class instance variables.", 2485198147] ], - "lib/oauth2/client.rb:1044992130": [ - [167, 16, 12, "Style/HashConversion: Prefer `ary.to_h` to `Hash[ary]`.", 4245087757] + "lib/oauth2/client.rb:2571398303": [ + [169, 16, 12, "Style/HashConversion: Prefer `ary.to_h` to `Hash[ary]`.", 4245087757] ], "oauth2.gemspec:4057149467": [ [127, 3, 53, "Gemspec/DependencyVersion: Dependency version specification is required.", 1845662118], @@ -20,17 +20,17 @@ [42, 15, 33, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 297534737], [51, 15, 38, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1480816240] ], - "spec/oauth2/client_spec.rb:626162787": [ + "spec/oauth2/client_spec.rb:2423960917": [ [158, 7, 467, "RSpec/NoExpectationExample: No expectation found in this example.", 1846255259], [177, 7, 570, "RSpec/NoExpectationExample: No expectation found in this example.", 354025061], [190, 15, 20, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 2320605227], [205, 15, 20, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1276531672], [220, 15, 43, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 1383956904], [235, 15, 43, "RSpec/ContextWording: Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.", 3376202107], - [458, 5, 369, "RSpec/NoExpectationExample: No expectation found in this example.", 648287915], - [467, 5, 471, "RSpec/NoExpectationExample: No expectation found in this example.", 1581534062], - [539, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785], - [547, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785], - [582, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785] + [494, 5, 369, "RSpec/NoExpectationExample: No expectation found in this example.", 648287915], + [503, 5, 471, "RSpec/NoExpectationExample: No expectation found in this example.", 1581534062], + [575, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785], + [583, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785], + [618, 45, 2, "RSpec/BeEq: Prefer `be` over `eq`.", 5860785] ] } diff --git a/CHANGELOG.md b/CHANGELOG.md index 5652e366..1ab4df34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,7 +119,7 @@ Please file a bug if you notice a violation of semantic versioning. ### Security - Prevent protocol-relative redirects from sending bearer tokens to an attacker-controlled host ([GHSA-pp92-crg2-gfv9](https://github.com/ruby-oauth/oauth2/security/advisories/GHSA-pp92-crg2-gfv9)). -- Remove authorization headers, request bodies, and query parameters from cross-origin redirects. +- Block cross-origin redirects by default to prevent credential replay and attacker-supplied token responses ([GHSA-h4fh-g8xf-8255](https://github.com/ruby-oauth/oauth2/security/advisories/GHSA-h4fh-g8xf-8255)). Legacy cross-origin redirects can be explicitly enabled with `:allow_cross_origin_redirects => true`. - Redact access tokens, refresh tokens, client secrets, authorization codes, and other sensitive fields from opt-in debug logs. ## [1.4.11] - 2022-09-16 diff --git a/README.md b/README.md index f6b54a86..0b90de9e 100644 --- a/README.md +++ b/README.md @@ -368,6 +368,12 @@ For more see [SECURITY.md][🚎sec-pol]. ## ⚙️ Configuration +Cross-origin redirects are blocked by default so OAuth credentials are not sent +to another origin and a redirected server cannot supply a token response. To +restore legacy cross-origin redirect behavior for a trusted provider, set +`:allow_cross_origin_redirects => true` when creating the client. Enabling this +option accepts the security risks of following a redirect to another origin. + ## 🔧 Basic Usage ## 🦷 FLOSS Funding diff --git a/lib/oauth2/client.rb b/lib/oauth2/client.rb index eea18f58..98aad362 100644 --- a/lib/oauth2/client.rb +++ b/lib/oauth2/client.rb @@ -29,6 +29,7 @@ class Client # rubocop:disable Metrics/ClassLength # @option options [Symbol] :auth_scheme (:basic_auth) HTTP method to use to authorize request (:basic_auth or :request_body) # @option options [Hash] :connection_opts ({}) Hash of connection options to pass to initialize Faraday with # @option options [FixNum] :max_redirects (5) maximum number of redirects to follow + # @option options [Boolean] :allow_cross_origin_redirects (false) whether to follow redirects to a different origin # @option options [Boolean] :raise_errors (true) whether or not to raise an OAuth2::Error on responses with 400+ status codes # @option options [Logger] :logger (::Logger.new($stdout)) which logger to use when OAUTH_DEBUG is enabled # @option options [Proc] (DEPRECATED) :extract_access_token proc that extracts the access token from the response @@ -46,6 +47,7 @@ def initialize(client_id, client_secret, options = {}, &block) :connection_opts => {}, :connection_build => block, :max_redirects => 5, + :allow_cross_origin_redirects => false, :raise_errors => true, :extract_access_token => DEFAULT_EXTRACT_ACCESS_TOKEN, # DEPRECATED :logger => ::Logger.new($stdout)}.merge(opts) @@ -296,9 +298,22 @@ def build_access_token(response, access_token_opts, extract_access_token) def sanitize_redirect_options(opts, current_url, next_url) current_uri = URI.parse(current_url) next_uri = URI.parse(next_url) - return opts if current_uri.scheme == next_uri.scheme && + same_origin = current_uri.scheme == next_uri.scheme && current_uri.host == next_uri.host && current_uri.port == next_uri.port + return opts if same_origin + + unless options[:allow_cross_origin_redirects] + blocked_response = Struct.new(:body, :headers, :status).new( + MultiJson.encode( + "error" => "cross_origin_redirect_blocked", + "error_description" => "Cross-origin redirect blocked. Set :allow_cross_origin_redirects to true to allow it." + ), + {"Content-Type" => "application/json"}, + 0 + ) + raise Error.new(Response.new(blocked_response)) + end safe_opts = opts.dup [:body, "body", :params, "params"].each { |key| safe_opts.delete(key) } diff --git a/spec/oauth2/client_spec.rb b/spec/oauth2/client_spec.rb index f1c2d23d..48a7b6b5 100644 --- a/spec/oauth2/client_spec.rb +++ b/spec/oauth2/client_spec.rb @@ -279,8 +279,44 @@ expect(response.body).to eq("api.example.com") end - it "does not forward credentials across cross-origin redirects" do + it "blocks cross-origin redirects by default" do + redirected = false client = described_class.new("abc", "def", :site => "https://api.example.com") do |builder| + builder.adapter :test do |stub| + stub.post("/start") do + [307, {"Content-Type" => "text/plain", "location" => "https://attacker.example/collect"}, ""] + end + stub.post("/collect") do |env| + redirected = true + [200, {}, MultiJson.encode( + :body => env[:body], + :params => env[:params], + :authorization => env[:request_headers]["Authorization"] + )] + end + end + end + + expect do + client.request( + :post, + "/start", + :body => "client_secret=secret", + :params => {"code" => "sensitive-code"}, + :headers => {"Authorization" => "Bearer sensitive-token"} + ) + end.to raise_error(OAuth2::Error) { |error| expect(error.code).to eq("cross_origin_redirect_blocked") } + + expect(redirected).to be(false) + end + + it "strips per-request credentials when cross-origin redirects are explicitly allowed" do + client = described_class.new( + "abc", + "def", + :site => "https://api.example.com", + :allow_cross_origin_redirects => true + ) do |builder| builder.adapter :test do |stub| stub.post("/start") do [307, {"Content-Type" => "text/plain", "location" => "https://attacker.example/collect"}, ""]