Keep storage exception detail out of the MCP response

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JFDp9HhXDeswadu4cxFojn
This commit is contained in:
Claude
2026-08-01 20:19:09 +00:00
parent 89a2f53126
commit 2e417d4aaa
2 changed files with 22 additions and 9 deletions

View File

@@ -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

View File

@@ -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