From 2e417d4aaa30af5d94e0b39abed6e3dd40dcea76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 20:19:09 +0000 Subject: [PATCH] Keep storage exception detail out of the MCP response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upload_failed message interpolated the exception text, which crosses out to an external agent. A storage failure can carry bucket names, object keys, paths or request details, so the agent now gets a fixed message and the exception stays in the server log. The test asserts the absence of detail rather than pinning the leaked string into the contract. Also fixes a test that did not test what it claimed: the urlsafe-base64 case used a fixture encoding to plain base64, so it exercised the padding branch and never the "-_" translation. It now uses content whose encoding contains both characters and asserts that up front. Renames "rejects content that decodes to zero bytes" to "rejects blank content", which is what it actually covers — Base64.strict_encode64("") is "", which is blank and returns before the decoder runs, so invalid_content is correct and empty_file is not reachable from this path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JFDp9HhXDeswadu4cxFojn --- .../function/upload_account_statement.rb | 8 ++++--- .../function/upload_account_statement_test.rb | 23 ++++++++++++++----- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/models/assistant/function/upload_account_statement.rb b/app/models/assistant/function/upload_account_statement.rb index 2faf230af..f557e0818 100644 --- a/app/models/assistant/function/upload_account_statement.rb +++ b/app/models/assistant/function/upload_account_statement.rb @@ -118,10 +118,12 @@ class Assistant::Function::UploadAccountStatement < Assistant::Function error("validation_failed", e.record.errors.full_messages.join("; ")) rescue => e # The shared upload path can raise from content sniffing, storage or a - # validation hook. Those would otherwise surface to the agent as a raw - # exception string; give it something it can act on instead. + # validation hook. The agent gets a fixed message, never the exception text: + # a storage failure can carry bucket names, object keys, paths or request + # details, and this response crosses out to an external client. Diagnostics + # stay in the server log. Rails.logger.error("[UploadAccountStatement] #{e.class}: #{e.message}") - error("upload_failed", "The statement could not be stored: #{e.message.truncate(200)}") + error("upload_failed", "The statement could not be stored due to an unexpected error. It has been logged for the administrator.") end private diff --git a/test/models/assistant/function/upload_account_statement_test.rb b/test/models/assistant/function/upload_account_statement_test.rb index 6a115fd5b..3fcf55734 100644 --- a/test/models/assistant/function/upload_account_statement_test.rb +++ b/test/models/assistant/function/upload_account_statement_test.rb @@ -98,29 +98,40 @@ class Assistant::Function::UploadAccountStatementTest < ActiveSupport::TestCase end test "accepts urlsafe base64 without padding" do - encoded = Base64.urlsafe_encode64(@content, padding: false) + # Chosen so the encoding actually uses the URL-safe alphabet; the usual + # fixture encodes to plain base64, which would exercise the padding branch + # while leaving the "-_" translation untested. + content = "a,b\n1,2\n~~~???\n" + encoded = Base64.urlsafe_encode64(content, padding: false) + assert_match(/-/, encoded, "fixture must exercise the urlsafe alphabet") + assert_match(/_/, encoded, "fixture must exercise the urlsafe alphabet") + assert_no_match(/=/, encoded, "fixture must be unpadded") result = @function.call("filename" => "statement.csv", "content_base64" => encoded) assert result[:success] - assert_equal Digest::SHA256.hexdigest(@content), result[:statement][:content_sha256] + assert_equal Digest::SHA256.hexdigest(content), result[:statement][:content_sha256] end - test "rejects content that decodes to zero bytes" do + test "rejects blank content" do + # Base64.strict_encode64("") is "", which is blank, so this never reaches + # the decoder — hence invalid_content rather than empty_file. result = @function.call("filename" => "statement.csv", "content_base64" => Base64.strict_encode64("")) assert_not result[:success] assert_equal "invalid_content", result[:error] end - test "reports an unexpected storage failure as a tool error" do - AccountStatement.stubs(:create_from_prepared_upload!).raises(StandardError, "storage exploded") + test "reports an unexpected storage failure without leaking the exception" do + AccountStatement.stubs(:create_from_prepared_upload!).raises(StandardError, "s3://bucket/secret-key exploded") result = @function.call(params(filename: "statement.csv")) assert_not result[:success] assert_equal "upload_failed", result[:error] - assert_match(/storage exploded/, result[:message]) + # The response crosses out to an external agent, so it must carry none of + # the exception's detail. + assert_no_match(/s3:|bucket|secret-key|exploded/, result[:message]) end test "rejects an unknown account_id rather than silently uploading unlinked" do