fix(security): drop the never-flipped secure_error_class! switch; log LLM errors via safe_message (#60)

CHANGELOG.md

@@ -1,6 +1,10 @@
 ## [Unreleased]
 
+### Removed
+- `Security::SecureErrorMixin.secure_error_class!`, the class-level switch that was meant to rewrite `#message`/`#to_s`/`#inspect` at construction. Nothing in the gem ever called it, so `LlmError#message` was never sanitized despite the mixin's docs. Sanitization is opt-in through the `safe_*` accessors, `to_secure_hash`, and `log_securely`, which is how every caller already used it. (Never shipped in a release; it arrived with the mainline port after 0.2.0.)
+
 ### Fixed
+- `LlmClient` logs provider, timeout, parse, and unexpected errors through `safe_message` instead of the raw message, so an API key echoed back in a provider error body no longer reaches the log line
 - Test suite loads on Ruby 4.0: `benchmark` is a bundled (not default) gem there, so it is now declared in the Gemfile
 - `EventDispatcher` slow-observer spec uses a monotonic clock and a looser bound so it no longer flakes on loaded hosts
 

lib/agentic/llm_client.rb

@@ -163,22 +163,22 @@ def without_retry(messages, parameters, output_schema, fail_on_error, stream_cal
       end
     rescue OpenAI::Error => e
       error = map_openai_error(e)
-      Agentic.logger.error("OpenAI API error: #{error.message}")
+      Agentic.logger.error("OpenAI API error: #{error.safe_message}")
       handle_error(error, fail_on_error)
     rescue Net::ReadTimeout, Net::OpenTimeout => e
       error = Errors::LlmTimeoutError.new("Request to LLM timed out: #{e.message}", context: {timeout_type: e.class.name})
-      Agentic.logger.error(error.message)
+      Agentic.logger.error(error.safe_message)
       handle_error(error, fail_on_error)
     rescue JSON::ParserError => e
       error = Errors::LlmParseError.new("Failed to parse JSON response: #{e.message}", parse_exception: e)
-      Agentic.logger.error(error.message)
+      Agentic.logger.error(error.safe_message)
       handle_error(error, fail_on_error)
     rescue Errors::LlmRefusalError, Errors::LlmParseError => e
       # Re-raise our custom errors that are already properly formatted
       handle_error(e, fail_on_error)
     rescue => e
       error = Errors::LlmError.new("Unexpected error in LLM request: #{e.message}", context: {error_class: e.class.name})
-      Agentic.logger.error("#{error.message}\n#{e.backtrace.join("\n")}")
+      Agentic.logger.error("#{error.safe_message}\n#{e.backtrace.join("\n")}")
       handle_error(error, fail_on_error)
     end
 
@@ -190,12 +190,12 @@ def models(fail_on_error: false)
       client.models.list&.dig("data")
     rescue OpenAI::Error => e
       error = map_openai_error(e)
-      Agentic.logger.error("OpenAI API error when listing models: #{error.message}")
+      Agentic.logger.error("OpenAI API error when listing models: #{error.safe_message}")
       handle_error(error, fail_on_error)
       nil
     rescue => e
       error = Errors::LlmError.new("Unexpected error listing models: #{e.message}")
-      Agentic.logger.error("#{error.message}\n#{e.backtrace.join("\n")}")
+      Agentic.logger.error("#{error.safe_message}\n#{e.backtrace.join("\n")}")
       handle_error(error, fail_on_error)
       nil
     end
@@ -209,12 +209,12 @@ def query_generation_stats(generation_id, fail_on_error: false)
       client.query_generation_stats(generation_id)
     rescue OpenAI::Error => e
       error = map_openai_error(e)
-      Agentic.logger.error("OpenAI API error when querying generation stats: #{error.message}")
+      Agentic.logger.error("OpenAI API error when querying generation stats: #{error.safe_message}")
       handle_error(error, fail_on_error)
       nil
     rescue => e
       error = Errors::LlmError.new("Unexpected error querying generation stats: #{e.message}")
-      Agentic.logger.error("#{error.message}\n#{e.backtrace.join("\n")}")
+      Agentic.logger.error("#{error.safe_message}\n#{e.backtrace.join("\n")}")
       handle_error(error, fail_on_error)
       nil
     end

lib/agentic/security/secure_error_mixin.rb

@@ -4,86 +4,38 @@
 
 module Agentic
   module Security
-    # Mixin to add security-aware error handling to existing error classes
+    # Mixin that gives an error class sanitized accessors for logging.
     #
-    # Provides sanitization capabilities for error messages, contexts, and responses
-    # without breaking existing error class hierarchies.
+    # Sanitization is opt-in and explicit: an error that includes this mixin
+    # keeps its ordinary #message, #to_s, and #inspect untouched, and exposes
+    # #safe_message, #safe_context, #safe_response, #safe_backtrace,
+    # #to_secure_hash, and #log_securely for any code path that writes the
+    # error somewhere it could be read (loggers, journals, plan records).
+    # Callers that log an error are responsible for picking the safe_* form;
+    # nothing here rewrites the error behind their back.
     #
     # Design Goals:
     # 1. Non-intrusive enhancement of existing error classes
-    # 2. Automatic sanitization of sensitive data in error messages
-    # 3. Configurable sanitization levels
+    # 2. Sanitized views of message, context, response, and backtrace
+    # 3. Configurable sanitization levels via Security::Config
     # 4. Backward compatibility with existing error handling
     module SecureErrorMixin
-      def self.included(base)
-        base.extend(ClassMethods)
-      end
-
-      module ClassMethods
-        # Wrap existing error class methods to add sanitization
-        def secure_error_class!
-          # Override initialize if it exists
-          if instance_method(:initialize)
-            alias_method :original_initialize, :initialize
-
-            define_method(:initialize) do |*args, **kwargs|
-              # Call original initialize first
-              original_initialize(*args, **kwargs)
-
-              # Apply sanitization to instance variables
-              apply_security_sanitization
-            end
-          end
-
-          # Override message method for sanitized output
-          alias_method :original_message, :message if instance_method(:message)
-
-          define_method(:message) do
-            if Agentic::Security::Config.pii_detection_enabled?
-              @sanitized_message ||= Agentic::Security::Config.sanitizer.sanitize_error(original_message)
-            else
-              original_message
-            end
-          end
-
-          # Override to_s for sanitized output
-          alias_method :original_to_s, :to_s if instance_method(:to_s)
-
-          define_method(:to_s) do
-            message # Use sanitized message method
-          end
-
-          # Add inspect method for debugging
-          define_method(:inspect) do
-            sanitized_attrs = {}
-
-            instance_variables.each do |var|
-              value = instance_variable_get(var)
-
-              sanitized_attrs[var] = case var.to_s
-              when "@response", "@context", "@network_exception"
-                # Sanitize complex objects
-                Agentic::Security::Config.sanitizer.sanitize(value, context: :error)
-              else
-                # Basic sanitization for simple values
-                Agentic::Security::Config.sanitizer.sanitize_error(value.to_s)
-              end
-            end
-
-            "#<#{self.class.name}:#{object_id} #{sanitized_attrs}>"
-          end
-        end
-      end
-
       # Instance methods available to all classes that include this mixin
 
       # Get sanitized error message
+      #
+      # Never raises: this runs inside rescue blocks, where a second exception
+      # would escape the caller's error handling. Invalid bytes in a provider
+      # error body are scrubbed before sanitizing, and if the sanitizer still
+      # fails the message is withheld rather than logged raw.
       def safe_message
         @safe_message ||= if Agentic::Security::Config.pii_detection_enabled?
-          Agentic::Security::Config.sanitizer.sanitize_error(message)
+          Agentic::Security::Config.sanitizer.sanitize_error(message.to_s.scrub)
         else
           message
         end
+      rescue => e
+        @safe_message = "[message withheld: sanitizer raised #{e.class}]"
       end
 
       # Get sanitized context (for errors that have context)
@@ -177,37 +129,6 @@ def log_securely(logger = nil)
           logger.error("#{self.class.name}: #{safe_message}")
         end
       end
-
-      private
-
-      # Apply sanitization to instance variables after initialization
-      def apply_security_sanitization
-        return unless Agentic::Security::Config.pii_detection_enabled?
-
-        # Sanitize message if it's directly stored
-        if instance_variable_defined?(:@message)
-          original_message = instance_variable_get(:@message)
-          sanitized_message = Agentic::Security::Config.sanitizer.sanitize_error(original_message)
-          instance_variable_set(:@message, sanitized_message)
-        end
-
-        # Sanitize context if present
-        if instance_variable_defined?(:@context) && @context
-          @context = Agentic::Security::Config.sanitizer.sanitize(@context, context: :error)
-        end
-
-        # Sanitize response if present
-        if instance_variable_defined?(:@response) && @response
-          @response = Agentic::Security::Config.sanitizer.sanitize_api_response(@response)
-        end
-
-        # Clear cached sanitized values to force recomputation
-        @sanitized_message = nil
-        @safe_message = nil
-        @safe_context = nil
-        @safe_response = nil
-        @safe_backtrace = nil
-      end
     end
   end
 end

spec/agentic/llm_client_spec.rb

@@ -306,6 +306,51 @@
       end
     end
 
+    context "when the provider error carries a secret" do
+      let(:leaky_error) { OpenAI::AuthenticationError.new("Incorrect API key provided: sk-abcdefghijklmnop1234") }
+
+      before do
+        Agentic::Security::Config.configure(enable_pii_detection: true)
+        allow(openai_client).to receive(:chat).and_raise(leaky_error)
+      end
+
+      after { Agentic::Security::Config.reset! }
+
+      it "logs the sanitized message, not the raw one" do
+        subject.complete(messages)
+
+        expect(Agentic.logger).to have_received(:error).with(a_string_including("[REDACTED_API_KEY]"))
+        expect(Agentic.logger).not_to have_received(:error).with(a_string_including("sk-abcdefghijklmnop1234"))
+      end
+
+      it "keeps the raw message on the returned error" do
+        result = subject.complete(messages)
+
+        expect(result.error.message).to include("sk-abcdefghijklmnop1234")
+      end
+    end
+
+    context "when the provider error body has invalid bytes" do
+      let(:garbled) { (+"Incorrect API key provided: sk-abcdefghijklmnop1234 \xFF\xFE").force_encoding("UTF-8") }
+      let(:leaky_error) { OpenAI::AuthenticationError.new(garbled) }
+
+      before do
+        Agentic::Security::Config.configure(enable_pii_detection: true)
+        allow(openai_client).to receive(:chat).and_raise(leaky_error)
+      end
+
+      after { Agentic::Security::Config.reset! }
+
+      it "still returns an error result instead of raising out of the rescue" do
+        result = nil
+        expect { result = subject.complete(messages) }.not_to raise_error
+
+        expect(result.success?).to be false
+        expect(result.error).to be_a(Agentic::Errors::LlmAuthenticationError)
+        expect(Agentic.logger).not_to have_received(:error).with(a_string_including("sk-abcdefghijklmnop1234"))
+      end
+    end
+
     context "with network errors" do
       let(:timeout_error) { Net::ReadTimeout.new("Read timeout") }
 

spec/agentic/security/secure_error_mixin_spec.rb

@@ -225,6 +225,52 @@ def initialize(message, context: nil, response: nil)
     end
   end
 
+  describe "opt-in contract" do
+    it "leaves #message, #to_s, and #inspect untouched" do
+      raw = "Authentication failed for user@example.com with key sk-123456"
+      error = Agentic::Errors::LlmError.new(raw, response: {"Authorization" => "Bearer sk-123456"})
+
+      expect(error.message).to eq(raw)
+      expect(error.to_s).to eq(raw)
+      expect(error.inspect).to include("sk-123456")
+      expect(error.response).to eq({"Authorization" => "Bearer sk-123456"})
+    end
+
+    it "does not add a class-level switch to includers" do
+      expect(Agentic::Errors::LlmError).not_to respond_to(:secure_error_class!)
+      expect(test_error_class).not_to respond_to(:secure_error_class!)
+    end
+
+    it "sanitizes only through the safe_* accessors" do
+      error = Agentic::Errors::LlmError.new(
+        "key sk-123456 leaked",
+        response: {"Authorization" => "Bearer sk-123456"}
+      )
+
+      expect(error.safe_message).to eq("key [REDACTED_API_KEY] leaked")
+      expect(error.safe_response.to_s).not_to include("sk-123456")
+    end
+  end
+
+  describe "safe_message never raises" do
+    it "scrubs invalid bytes before sanitizing" do
+      bad = (+"key sk-123456 then \xFF\xFE").force_encoding("UTF-8")
+      error = test_error_class.new(bad)
+
+      expect { error.safe_message }.not_to raise_error
+      expect(error.safe_message).to include("[REDACTED_API_KEY]")
+      expect(error.safe_message).not_to include("sk-123456")
+    end
+
+    it "withholds the message when the sanitizer itself fails" do
+      allow(Agentic::Security::Config.sanitizer).to receive(:sanitize_error).and_raise(RuntimeError, "boom")
+      error = test_error_class.new("key sk-123456")
+
+      expect(error.safe_message).to eq("[message withheld: sanitizer raised RuntimeError]")
+      expect(error.safe_message).not_to include("sk-123456")
+    end
+  end
+
   describe "performance considerations" do
     it "handles large error contexts efficiently" do
       large_context = {}