fix(ASV-4361): close five log-redaction and dispatch findings

lib/httpigeon/log_redactor.rb

@@ -48,15 +48,21 @@ def redact_hash(data)
 
         if filter.present?
           replacement = filter.split('::')[1].presence
-          v = replacement.present? ? replacement : redact_value(v)
-        end
-
-        if v.is_a?(Hash)
-          [k, redact_hash(v)]
-        elsif v.is_a?(Array)
-          [k, v.map { |val| redact(val) }]
-        else
+          # Replace collections wholesale; partial redaction leaks bytes of
+          # nested secrets.
+          v = if replacement.present?
+                replacement
+              elsif v.is_a?(Hash) || v.is_a?(Array)
+                HTTPigeon.redactor_string
+              else
+                redact_value(v)
+              end
           [k, v]
+        else
+          # Recurse through the dispatcher so nested values redact like
+          # top-level ones. This runs content/regex filters over string
+          # values too (e.g. secrets in JSON bodies).
+          [k, redact(v)]
         end
       end
     end
@@ -67,13 +73,23 @@ def redact_string(data)
 
         next unless pattern.match?(%r{^/.*/([guysim]*)$})
 
+        regex = regex_for(pattern)
+
         data = if replacement.present?
-                 data.gsub(regex_for(pattern), replacement)
+                 # Block form keeps the replacement literal; the string form
+                 # expands backreferences (\1, \&) and re-emits the secret.
+                 data.gsub(regex) { replacement }
                else
-                 data.gsub(regex_for(pattern)) do |sub|
-                   captures = sub.match(regex_for(pattern))&.captures
-
-                   captures.present? ? captures[0] + redact_value(captures[1]) : sub
+                 data.gsub(regex) do |sub|
+                   captures = Regexp.last_match&.captures
+
+                   # A zero-width capture yields nil; skip it rather than
+                   # concatenating nil and raising TypeError.
+                   if captures.present? && !captures[1].nil?
+                     captures[0] + redact_value(captures[1])
+                   else
+                     sub
+                   end
                  end
                end
       end

lib/httpigeon/request.rb

@@ -84,12 +84,12 @@ def run(method: :get, path: '/', payload: {})
 
       raw_response = if HTTPigeon.mount_circuit_breaker
                        fuse.execute(request_id: connection.headers[REQUEST_ID_HEADER]) do
-                         connection.send(method, path, payload) do |request|
+                         connection.send(sym_method, path, payload) do |request|
                            yield(request) if block_given?
                          end
                        end
                      else
-                       connection.send(method, path, payload) do |request|
+                       connection.send(sym_method, path, payload) do |request|
                          yield(request) if block_given?
                        end
                      end

spec/httpigeon/log_redactor_spec.rb

@@ -201,6 +201,54 @@
       end
     end
 
+    context 'when a filtered key holds a nested collection' do
+      it 'fully redacts a nested hash instead of leaking fragments' do
+        filters = %w[token]
+        data = { token: { access: 'SUPER_SECRET_ACCESS_TOKEN_123456', refresh: 'REFRESH_SECRET_999' } }
+
+        expect(described_class.new(log_filters: filters).redact(data)).to eq({ token: '<redacted>' })
+      end
+
+      it 'fully redacts a nested array instead of leaking fragments' do
+        filters = %w[credentials]
+        data = { credentials: %w[SUPER_SECRET_ONE SUPER_SECRET_TWO] }
+
+        expect(described_class.new(log_filters: filters).redact(data)).to eq({ credentials: '<redacted>' })
+      end
+    end
+
+    context 'when a content filter matches a string value inside a hash' do
+      it 'redacts the secret regardless of the key name' do
+        filters = [HTTPigeon::FilterPatterns::CLIENT_SECRET]
+        data = { error_uri: 'https://idp/cb?client_id=abc&client_secret=REALSECRET' }
+
+        expect(described_class.new(log_filters: filters).redact(data)).to eq(
+          { error_uri: 'https://idp/cb?client_id=abc&client_secret=REA...<redacted>' }
+        )
+      end
+    end
+
+    context 'when a replacement contains a gsub backreference' do
+      it 'treats the backreference literally instead of re-emitting the secret' do
+        filters = ['/(password=)([^&]+)/::password=\2']
+        data = 'https://api.example.com/auth?password=S3cr3tP@ssw0rd&grant_type=code'
+
+        expect(described_class.new(log_filters: filters).redact(data)).to eq(
+          'https://api.example.com/auth?password=\2&grant_type=code'
+        )
+      end
+    end
+
+    context 'when a grouped filter matches an empty capture' do
+      it 'does not raise and leaves the empty value in place' do
+        filters = %w[/(client_id=)([0-9a-z]+)*/]
+        data = 'action=login&client_id=&next=home'
+
+        expect { described_class.new(log_filters: filters).redact(data) }.not_to raise_error
+        expect(described_class.new(log_filters: filters).redact(data)).to eq('action=login&client_id=&next=home')
+      end
+    end
+
     context 'when filters include an invalid regex' do
       it 'raises an error' do
         filters = %w[/key_3=[0-9a-z]*/im::key_3=<redacted> key_4]

spec/httpigeon/middleware/httpigeon_logger_spec.rb

@@ -0,0 +1,36 @@
+require 'spec_helper'
+
+describe HTTPigeon::Middleware::HTTPigeonLogger do
+  subject(:middleware) { described_class.new(app, logger) }
+
+  let(:app) { ->(env) { env } }
+  let(:logger) { instance_double(HTTPigeon::Logger, on_request_start: nil, on_request_finish: nil, log: nil) }
+  let(:env) { Faraday::Env.new }
+
+  describe '#call' do
+    context 'when the request succeeds' do
+      it 'logs on completion and returns the response' do
+        allow(app).to receive(:call).and_return(Faraday::Response.new(env))
+
+        middleware.call(env)
+
+        expect(logger).to have_received(:on_request_start)
+        expect(logger).to have_received(:on_request_finish)
+        expect(logger).to have_received(:log).with(env)
+      end
+    end
+
+    context 'when the request raises' do
+      let(:error) { StandardError.new('boom') }
+
+      before { allow(app).to receive(:call).and_raise(error) }
+
+      it 'logs the error and re-raises it' do
+        expect { middleware.call(env) }.to raise_error(error)
+
+        expect(logger).to have_received(:on_request_finish)
+        expect(logger).to have_received(:log).with(env, { error: error })
+      end
+    end
+  end
+end

spec/httpigeon/request_spec.rb

@@ -223,6 +223,21 @@
         it 'raises ArgumentError for method as string' do
           expect { request.run(method: 'system', path: '/users', payload: {}) }.to raise_error(ArgumentError, 'Invalid or unsupported HTTP method: system')
         end
+
+        it 'dispatches the validated symbol, not a to_sym/to_str divergent object' do
+          clever_method = Class.new do
+            def to_sym = :get
+            def to_str = 'instance_eval'
+          end.new
+
+          allow(request.connection).to receive(:instance_eval)
+          allow(request.connection).to receive(:get).and_call_original
+
+          request.run(method: clever_method, path: '/users', payload: {})
+
+          expect(request.connection).to have_received(:get)
+          expect(request.connection).not_to have_received(:instance_eval)
+        end
       end
     end