Redact structured debug bodies on 1.4

.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:2387364079": [
+    [170, 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]
   ]
 }

CHANGELOG.md

@@ -119,8 +119,8 @@ 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.
-- Redact access tokens, refresh tokens, client secrets, authorization codes, and other sensitive fields from opt-in debug logs.
+- 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 sensitive values in structured request and response bodies in opt-in debug logs ([GHSA-cmh8-5q25-68v6](https://github.com/ruby-oauth/oauth2/security/advisories/GHSA-cmh8-5q25-68v6)).
 
 ## [1.4.11] - 2022-09-16
 - Complete migration to main branch as default (@pboling)

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

lib/oauth2/client.rb

@@ -3,6 +3,7 @@
 require "faraday"
 require "logger"
 require "uri"
+require "oauth2/sanitized_log_formatter"
 
 module OAuth2
   ConnectionError = Class.new(Faraday::ConnectionFailed)
@@ -29,6 +30,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 +48,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 +299,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) }
@@ -322,7 +338,9 @@ def oauth_debug_logging(builder)
         "device_code", "subject_token", "actor_token", "accessToken",
         "refreshToken", "idToken"
       ]
-      builder.response :logger, options[:logger], :bodies => true do |logger|
+      logger_options = {:bodies => !!defined?(OAuth2::SanitizedLogFormatter)}
+      logger_options[:formatter] = OAuth2::SanitizedLogFormatter if defined?(OAuth2::SanitizedLogFormatter)
+      builder.response :logger, options[:logger], logger_options do |logger|
         logger.filter(/^(\s*Authorization\s*:\s*).*$/i, "\\1[FILTERED]")
         sensitive_debug_keys.each do |key|
           pattern = /(["']?#{Regexp.escape(key)}["']?\s*(?:=>|:|=)\s*)(?:"[^"]*"|'[^']*'|[^&,\s}]+)/i

lib/oauth2/sanitized_log_formatter.rb

@@ -0,0 +1,108 @@
+# frozen_string_literal: true
+
+require "faraday"
+require "cgi"
+
+begin
+  require "faraday/logging/formatter"
+rescue LoadError
+  # Faraday 0.x does not support a structured logging formatter.
+end
+
+module OAuth2
+  if defined?(Faraday::Logging::Formatter)
+    # Redacts structured request and response bodies before Faraday renders them.
+    # Faraday 0.x has no formatter API; Client disables body logging there.
+    class SanitizedLogFormatter < Faraday::Logging::Formatter
+      SENSITIVE_KEYS = [
+        "access_token", "refresh_token", "client_secret", "code", "code_verifier",
+        "password", "assertion", "id_token", "token", "client_assertion",
+        "device_code", "subject_token", "actor_token", "accesstoken",
+        "refreshtoken", "idtoken", "clientsecret", "codeverifier",
+        "clientassertion", "devicecode", "subjecttoken", "actortoken"
+      ].freeze
+
+      LABEL = "[FILTERED]"
+
+      def initialize(options = {})
+        super(:logger => options[:logger], :options => options[:options] || {})
+      end
+
+      def request(env)
+        return super unless env.respond_to?(:url) && env.url && env.url.query
+
+        safe_env = env.dup
+        safe_url = env.url.dup
+        safe_url.query = sanitize_query(safe_url.query)
+        safe_env.url = safe_url
+        super(safe_env)
+      end
+
+      def dump_body(body)
+        case body
+        when Hash, Array
+          super(redact_structured(body, {}))
+        when String
+          super(sanitize_string(body))
+        else
+          super
+        end
+      end
+
+    private
+
+      def redact_structured(value, seen)
+        return value unless value.is_a?(Hash) || value.is_a?(Array)
+
+        object_id = value.object_id
+        return LABEL if seen[object_id]
+
+        seen[object_id] = true
+        if value.is_a?(Hash)
+          value.each_with_object({}) do |(key, child), copy|
+            copy[key] = sensitive_key?(key) ? LABEL : redact_structured(child, seen)
+          end
+        elsif pair_array?(value)
+          value.map do |pair|
+            [pair[0], sensitive_key?(pair[0]) ? LABEL : redact_structured(pair[1], seen)]
+          end
+        else
+          value.map { |child| redact_structured(child, seen) }
+        end
+      end
+
+      def pair_array?(value)
+        !value.empty? && value.all? { |element| element.is_a?(Array) && element.length == 2 }
+      end
+
+      def sensitive_key?(key)
+        SENSITIVE_KEYS.include?(key.to_s.downcase)
+      end
+
+      def sanitize_query(query)
+        query.split(/([&;])/).map do |part|
+          if part == "&" || part == ";"
+            part
+          else
+            key, value = part.split("=", 2)
+            if sensitive_key?(CGI.unescape(key.to_s))
+              value ? "#{key}=#{LABEL}" : key
+            else
+              sanitize_string(part)
+            end
+          end
+        end.join
+      end
+
+      def sanitize_string(value)
+        safe_value = value.dup
+        safe_value.force_encoding(Encoding::BINARY) if safe_value.respond_to?(:force_encoding)
+        SENSITIVE_KEYS.each do |key|
+          pattern = /(["']?#{Regexp.escape(key)}["']?\s*(?:=>|:|=)\s*)(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[^&,\s}\]]+)/i
+          safe_value.gsub!(pattern) { "#{Regexp.last_match(1)}#{LABEL}" }
+        end
+        safe_value
+      end
+    end
+  end
+end

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"}, ""]

spec/oauth2/sanitized_log_formatter_spec.rb

@@ -0,0 +1,37 @@
+if defined?(OAuth2::SanitizedLogFormatter)
+  RSpec.describe OAuth2::SanitizedLogFormatter do
+    subject(:formatter) do
+      described_class.new(:logger => Logger.new($stdout), :options => {:bodies => true})
+    end
+
+    it "redacts sensitive values in nested hashes and arrays" do
+      rendered = formatter.dump_body(
+        "client_secret" => "client-secret-value",
+        "nested" => [{"refresh_token" => "refresh-token-value"}]
+      )
+
+      expect(rendered).not_to include("client-secret-value", "refresh-token-value")
+    end
+
+    it "redacts sensitive values in form-pair arrays" do
+      rendered = formatter.dump_body([
+        ["grant_type", "refresh_token"],
+        ["access_token", "array-token-value"],
+      ])
+
+      expect(rendered).not_to include("array-token-value")
+    end
+
+    it "redacts quoted secrets in serialized request and response bodies" do
+      rendered = formatter.dump_body('{"client_secret":"quoted\\"secret-value"}')
+
+      expect(rendered).not_to include("quoted", "secret-value")
+    end
+
+    it "redacts secrets in URL query strings" do
+      sanitized = formatter.send(:sanitize_query, "grant_type=refresh_token&refresh_token=query-token-value")
+
+      expect(sanitized).not_to include("query-token-value")
+    end
+  end
+end