diff --git a/CHANGELOG.md b/CHANGELOG.md index a314b10..755f14a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/agentic/llm_client.rb b/lib/agentic/llm_client.rb index ae7d4dc..eab0e5f 100644 --- a/lib/agentic/llm_client.rb +++ b/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 diff --git a/lib/agentic/security/secure_error_mixin.rb b/lib/agentic/security/secure_error_mixin.rb index e976894..de24290 100644 --- a/lib/agentic/security/secure_error_mixin.rb +++ b/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 diff --git a/spec/agentic/llm_client_spec.rb b/spec/agentic/llm_client_spec.rb index 2871d26..0bd68d2 100644 --- a/spec/agentic/llm_client_spec.rb +++ b/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") } diff --git a/spec/agentic/security/secure_error_mixin_spec.rb b/spec/agentic/security/secure_error_mixin_spec.rb index 3aef70d..d65262b 100644 --- a/spec/agentic/security/secure_error_mixin_spec.rb +++ b/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 = {}