From 71b99c1e18bf49f19839e088b8abad417a119fbb Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 10 Mar 2026 11:03:40 -0400 Subject: [PATCH] Fix path traversal vulnerability in ActiveStorage blob key import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BlobRecordSet#import_batch preserved blob keys from exported ZIP files verbatim. A crafted ZIP with a traversal key like "../../config/deploy.yml" would create a blob whose key resolves to an arbitrary filesystem path when served by ActiveStorage's DiskService, allowing an attacker to read local files. Fix: generate fresh blob keys on import, discarding whatever key was in the ZIP. This is safe because nothing else in the import pipeline references blobs by key — attachments use blob_id, ActionText uses GIDs based on record IDs, and only FileRecordSet used the old key for file data lookup. Update FileRecordSet to build an old_key→blob_id mapping from the blob JSON metadata in the ZIP, then look up blobs by ID instead of by key. Both check_record and import_batch are now fail-closed: they raise IntegrityError for unmapped storage files, missing blobs, and duplicate keys in the export. Backfill test coverage for BlobRecordSet and FileRecordSet import, and add a round-trip test verifying blob data survives export/import with regenerated keys. --- .../active_storage/blob_record_set.rb | 1 + .../active_storage/file_record_set.rb | 38 +++- test/integration/blob_key_traversal_test.rb | 45 +++++ .../active_storage/blob_record_set_test.rb | 58 ++++++ .../active_storage/file_record_set_test.rb | 177 ++++++++++++++++++ test/models/account/import_test.rb | 46 +++++ 6 files changed, 357 insertions(+), 8 deletions(-) create mode 100644 test/integration/blob_key_traversal_test.rb create mode 100644 test/models/account/data_transfer/active_storage/blob_record_set_test.rb create mode 100644 test/models/account/data_transfer/active_storage/file_record_set_test.rb diff --git a/app/models/account/data_transfer/active_storage/blob_record_set.rb b/app/models/account/data_transfer/active_storage/blob_record_set.rb index 1637768f3..0c4a553c1 100644 --- a/app/models/account/data_transfer/active_storage/blob_record_set.rb +++ b/app/models/account/data_transfer/active_storage/blob_record_set.rb @@ -13,6 +13,7 @@ class Account::DataTransfer::ActiveStorage::BlobRecordSet < Account::DataTransfe data = load(file) data.slice(*attributes).merge( "account_id" => account.id, + "key" => ::ActiveStorage::Blob.generate_unique_secure_token(length: ::ActiveStorage::Blob::MINIMUM_TOKEN_LENGTH), "service_name" => ::ActiveStorage::Blob.service.name ) end diff --git a/app/models/account/data_transfer/active_storage/file_record_set.rb b/app/models/account/data_transfer/active_storage/file_record_set.rb index 15d359353..421a2e520 100644 --- a/app/models/account/data_transfer/active_storage/file_record_set.rb +++ b/app/models/account/data_transfer/active_storage/file_record_set.rb @@ -22,9 +22,12 @@ class Account::DataTransfer::ActiveStorage::FileRecordSet < Account::DataTransfe def import_batch(files) files.each do |file| - key = File.basename(file) - blob = ::ActiveStorage::Blob.find_by(key: key, account: account) - next unless blob + old_key = file.delete_prefix("storage/") + blob_id = old_key_to_blob_id[old_key] + raise IntegrityError, "Storage file #{old_key} has no matching blob metadata in export" unless blob_id + + blob = ::ActiveStorage::Blob.find_by(id: blob_id, account: account) + raise IntegrityError, "Blob #{blob_id} not found for storage key #{old_key}" unless blob zip.read(file) do |stream| blob.upload(stream) @@ -32,12 +35,31 @@ class Account::DataTransfer::ActiveStorage::FileRecordSet < Account::DataTransfe end end - def check_record(file_path) - key = File.basename(file_path) + def old_key_to_blob_id + @old_key_to_blob_id ||= build_old_key_to_blob_id + end - unless zip.exists?("data/active_storage_blobs/#{key}.json") || ::ActiveStorage::Blob.exists?(key: key, account: account) - # File exists without corresponding blob record - could be orphaned or blob not yet imported - # We allow this since blob metadata is imported before files + def build_old_key_to_blob_id + zip.glob("data/active_storage_blobs/*.json").each_with_object({}) do |file, map| + data = load(file) + old_key = data["key"] + if map.key?(old_key) + raise IntegrityError, "Duplicate blob key in export: #{old_key}" + end + map[old_key] = data["id"] + end + end + + def with_zip(zip) + @old_key_to_blob_id = nil + super + end + + def check_record(file_path) + old_key = file_path.delete_prefix("storage/") + + unless old_key_to_blob_id.key?(old_key) + raise IntegrityError, "Storage file #{old_key} has no matching blob metadata in export" end end end diff --git a/test/integration/blob_key_traversal_test.rb b/test/integration/blob_key_traversal_test.rb new file mode 100644 index 000000000..31dc8e179 --- /dev/null +++ b/test/integration/blob_key_traversal_test.rb @@ -0,0 +1,45 @@ +require "test_helper" + +class Account::DataTransfer::ActiveStorage::BlobKeyTraversalTest < ActionDispatch::IntegrationTest + test "import with path traversal blob key does not leak local files" do + blob_id = ActiveRecord::Type::Uuid.generate + traversal_key = "../../config/deploy.yml" + zip = build_zip_with_blob(id: blob_id, key: traversal_key) + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + blob = ActiveStorage::Blob.find(blob_id) + + assert_not_equal traversal_key, blob.key + + sign_in_as identities(:david) + get rails_blob_path(blob, disposition: "inline") + + assert_response :redirect + + follow_redirect! + + assert_response :not_found + end + + private + def build_zip_with_blob(id:, key:) + tempfile = Tempfile.new([ "malicious", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/active_storage_blobs/#{id}.json", { + id: id, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: 32, + checksum: "", + content_type: "text/plain", + created_at: Time.current.iso8601, + filename: "traversal.txt", + key: key, + metadata: {} + }.to_json) + writer.close + + tempfile.rewind + ZipFile::Reader.new(tempfile) + end +end diff --git a/test/models/account/data_transfer/active_storage/blob_record_set_test.rb b/test/models/account/data_transfer/active_storage/blob_record_set_test.rb new file mode 100644 index 000000000..82b83f428 --- /dev/null +++ b/test/models/account/data_transfer/active_storage/blob_record_set_test.rb @@ -0,0 +1,58 @@ +require "test_helper" + +class Account::DataTransfer::ActiveStorage::BlobRecordSetTest < ActiveSupport::TestCase + test "import generates fresh keys instead of using exported keys" do + blob_id = ActiveRecord::Type::Uuid.generate + exported_key = "original-exported-key-abc123" + + zip = build_zip_with_blob(id: blob_id, key: exported_key) + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + + blob = ActiveStorage::Blob.find(blob_id) + assert_not_equal exported_key, blob.key + assert_equal 28, blob.key.length + end + + test "import preserves blob metadata" do + blob_id = ActiveRecord::Type::Uuid.generate + + zip = build_zip_with_blob( + id: blob_id, + key: "some-key", + filename: "report.pdf", + content_type: "application/pdf", + byte_size: 12345, + checksum: "abc123checksum" + ) + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + + blob = ActiveStorage::Blob.find(blob_id) + assert_equal "report.pdf", blob.filename.to_s + assert_equal "application/pdf", blob.content_type + assert_equal 12345, blob.byte_size + assert_equal "abc123checksum", blob.checksum + end + + private + def build_zip_with_blob(id:, key:, filename: "test.txt", content_type: "text/plain", byte_size: 32, checksum: "") + tempfile = Tempfile.new([ "test_export", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/active_storage_blobs/#{id}.json", { + id: id, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: byte_size, + checksum: checksum, + content_type: content_type, + created_at: Time.current.iso8601, + filename: filename, + key: key, + metadata: {} + }.to_json) + writer.close + + tempfile.rewind + ZipFile::Reader.new(tempfile) + end +end diff --git a/test/models/account/data_transfer/active_storage/file_record_set_test.rb b/test/models/account/data_transfer/active_storage/file_record_set_test.rb new file mode 100644 index 000000000..4784754ae --- /dev/null +++ b/test/models/account/data_transfer/active_storage/file_record_set_test.rb @@ -0,0 +1,177 @@ +require "test_helper" + +class Account::DataTransfer::ActiveStorage::FileRecordSetTest < ActiveSupport::TestCase + test "import uploads file data to blobs with regenerated keys" do + blob_id = ActiveRecord::Type::Uuid.generate + old_key = "original-key-for-file" + file_content = "hello world file content" + + zip = build_zip_with_blob_and_file(blob_id: blob_id, old_key: old_key, file_content: file_content) + + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).import(from: zip) + + blob = ActiveStorage::Blob.find(blob_id) + assert_not_equal old_key, blob.key + assert_equal file_content, blob.download + end + + test "import handles keys containing path separators" do + blob_id = ActiveRecord::Type::Uuid.generate + old_key = "folder/subfolder/file-key" + file_content = "nested key content" + + zip = build_zip_with_blob_and_file(blob_id: blob_id, old_key: old_key, file_content: file_content) + + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).import(from: zip) + + blob = ActiveStorage::Blob.find(blob_id) + assert_not_equal old_key, blob.key + assert_equal file_content, blob.download + end + + test "import raises IntegrityError for storage file without matching blob metadata" do + blob_id = ActiveRecord::Type::Uuid.generate + old_key = "key-with-metadata" + orphan_key = "orphaned-storage-key" + + zip = build_zip_with_orphaned_storage_file( + blob_id: blob_id, + old_key: old_key, + orphan_key: orphan_key + ) + + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(Current.account).import(from: zip) + + assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).import(from: zip) + end + end + + test "import raises IntegrityError when mapped blob is not found in database" do + blob_id = ActiveRecord::Type::Uuid.generate + old_key = "key-for-missing-blob" + + zip = build_zip_with_blob_and_file(blob_id: blob_id, old_key: old_key, file_content: "data") + + # Import file data WITHOUT importing blob metadata first + assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).import(from: zip) + end + end + + test "check raises IntegrityError for storage file without matching blob metadata" do + blob_id = ActiveRecord::Type::Uuid.generate + old_key = "key-with-metadata" + orphan_key = "orphaned-storage-key" + + zip = build_zip_with_orphaned_storage_file( + blob_id: blob_id, + old_key: old_key, + orphan_key: orphan_key + ) + + assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).check(from: zip) + end + end + + test "import raises IntegrityError for duplicate blob keys in export" do + blob_id_1 = ActiveRecord::Type::Uuid.generate + blob_id_2 = ActiveRecord::Type::Uuid.generate + duplicate_key = "same-key-for-both" + + zip = build_zip_with_duplicate_keys( + blob_id_1: blob_id_1, + blob_id_2: blob_id_2, + key: duplicate_key + ) + + assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + Account::DataTransfer::ActiveStorage::FileRecordSet.new(Current.account).import(from: zip) + end + end + + private + def build_zip_with_blob_and_file(blob_id:, old_key:, file_content:) + tempfile = Tempfile.new([ "test_export", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/active_storage_blobs/#{blob_id}.json", { + id: blob_id, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: file_content.bytesize, + checksum: Digest::MD5.base64digest(file_content), + content_type: "text/plain", + created_at: Time.current.iso8601, + filename: "test.txt", + key: old_key, + metadata: {} + }.to_json) + writer.add_file("storage/#{old_key}", file_content, compress: false) + writer.close + + tempfile.rewind + ZipFile::Reader.new(tempfile) + end + + def build_zip_with_orphaned_storage_file(blob_id:, old_key:, orphan_key:) + tempfile = Tempfile.new([ "test_export", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/active_storage_blobs/#{blob_id}.json", { + id: blob_id, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: 10, + checksum: "", + content_type: "text/plain", + created_at: Time.current.iso8601, + filename: "test.txt", + key: old_key, + metadata: {} + }.to_json) + writer.add_file("storage/#{old_key}", "file data", compress: false) + writer.add_file("storage/#{orphan_key}", "orphan data", compress: false) + writer.close + + tempfile.rewind + ZipFile::Reader.new(tempfile) + end + + def build_zip_with_duplicate_keys(blob_id_1:, blob_id_2:, key:) + tempfile = Tempfile.new([ "test_export", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/active_storage_blobs/#{blob_id_1}.json", { + id: blob_id_1, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: 10, + checksum: "", + content_type: "text/plain", + created_at: Time.current.iso8601, + filename: "file1.txt", + key: key, + metadata: {} + }.to_json) + writer.add_file("data/active_storage_blobs/#{blob_id_2}.json", { + id: blob_id_2, + account_id: ActiveRecord::Type::Uuid.generate, + byte_size: 10, + checksum: "", + content_type: "text/plain", + created_at: Time.current.iso8601, + filename: "file2.txt", + key: key, + metadata: {} + }.to_json) + writer.add_file("storage/#{key}", "file data", compress: false) + writer.close + + tempfile.rewind + ZipFile::Reader.new(tempfile) + end +end diff --git a/test/models/account/import_test.rb b/test/models/account/import_test.rb index 7bf407041..de384b385 100644 --- a/test/models/account/import_test.rb +++ b/test/models/account/import_test.rb @@ -174,6 +174,52 @@ class Account::ImportTest < ActiveSupport::TestCase export_tempfile&.unlink end + test "export and import round-trip preserves blobs and attachments" do + source_account = accounts("37s") + exporter = users(:david) + identity = exporter.identity + + ActiveStorage::Blob.create_and_upload!( + io: StringIO.new("test image data"), + filename: "logo.png", + content_type: "image/png" + ) + + source_blob_count = ActiveStorage::Blob.where(account: source_account).count + source_blob_keys = ActiveStorage::Blob.where(account: source_account).pluck(:key) + + assert_operator source_blob_count, :>, 0 + + export = Account::Export.create!(account: source_account, user: exporter) + export.build + + export_tempfile = Tempfile.new([ "export", ".zip" ]) + export.file.open { |f| FileUtils.cp(f.path, export_tempfile.path) } + + ActiveStorage::Blob.where(account: source_account).delete_all + source_account.destroy! + + target_account = Account.create_with_owner(account: { name: "Import Test" }, owner: { identity: identity, name: exporter.name }) + import = Account::Import.create!(identity: identity, account: target_account) + Current.set(account: target_account) do + import.file.attach(io: File.open(export_tempfile.path), filename: "export.zip", content_type: "application/zip") + end + + import.check + assert_not import.failed? + + import.process + assert import.completed? + + imported_blob = ActiveStorage::Blob.find_by(account: target_account, filename: "logo.png") + assert_not_nil imported_blob + assert_not_includes source_blob_keys, imported_blob.key + assert_equal "test image data", imported_blob.download + ensure + export_tempfile&.close + export_tempfile&.unlink + end + private def account_digest(account) {