diff --git a/app/models/account/data_transfer/account_record_set.rb b/app/models/account/data_transfer/account_record_set.rb index b7869eb73..a5151f05f 100644 --- a/app/models/account/data_transfer/account_record_set.rb +++ b/app/models/account/data_transfer/account_record_set.rb @@ -31,7 +31,7 @@ class Account::DataTransfer::AccountRecordSet < Account::DataTransfer::RecordSet account_data = load(files.first) join_code_data = account_data.delete("join_code") - account.update!(name: account_data.fetch("name")) + account.update!(name: account_data.fetch("name"), cards_count: account_data.fetch("cards_count", 0)) account.join_code.update!(join_code_data.slice("usage_count", "usage_limit")) account.join_code.update(code: join_code_data.fetch("code")) end diff --git a/app/models/account/data_transfer/action_text_rich_text_record_set.rb b/app/models/account/data_transfer/action_text/rich_text_record_set.rb similarity index 53% rename from app/models/account/data_transfer/action_text_rich_text_record_set.rb rename to app/models/account/data_transfer/action_text/rich_text_record_set.rb index fa1a23cb9..01e046dbe 100644 --- a/app/models/account/data_transfer/action_text_rich_text_record_set.rb +++ b/app/models/account/data_transfer/action_text/rich_text_record_set.rb @@ -1,4 +1,4 @@ -class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer::RecordSet +class Account::DataTransfer::ActionText::RichTextRecordSet < Account::DataTransfer::RecordSet ATTRIBUTES = %w[ account_id body @@ -11,16 +11,16 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer ].freeze def initialize(account) - super(account: account, model: ActionText::RichText) + super(account: account, model: ::ActionText::RichText) end private def records - ActionText::RichText.where(account: account) + ::ActionText::RichText.where(account: account) end def export_record(rich_text) - data = rich_text.as_json.merge("body" => convert_sgids_to_gids(rich_text.body)) + data = rich_text.as_json.merge("body" => transform_body_for_export(rich_text.body)) zip.add_file "data/action_text_rich_texts/#{rich_text.id}.json", data.to_json end @@ -31,11 +31,11 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer def import_batch(files) batch_data = files.map do |file| data = load(file) - data["body"] = convert_gids_to_sgids(data["body"]) + data["body"] = transform_body_for_import(data["body"]) data.slice(*ATTRIBUTES).merge("account_id" => account.id) end - ActionText::RichText.insert_all!(batch_data) + ::ActionText::RichText.insert_all!(batch_data) end def check_record(file_path) @@ -54,11 +54,16 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer check_associations_dont_exist(data) end - def convert_sgids_to_gids(content) + def transform_body_for_export(content) return nil if content.blank? + html = convert_sgids_to_gids(content) + relativize_urls(html) + end + + def convert_sgids_to_gids(content) content.send(:attachment_nodes).each do |node| - sgid = SignedGlobalID.parse(node["sgid"], for: ActionText::Attachable::LOCATOR_NAME) + sgid = SignedGlobalID.parse(node["sgid"], for: ::ActionText::Attachable::LOCATOR_NAME) record = begin sgid&.find @@ -75,11 +80,35 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer content.fragment.source.to_html end - def convert_gids_to_sgids(html) - return html if html.blank? + def relativize_urls(html) + host = Rails.application.routes.default_url_options[:host] + return html unless host fragment = Nokogiri::HTML.fragment(html) + fragment.css("a[href]").each do |link| + uri = URI.parse(link["href"]) rescue nil + + if uri.respond_to?(:host) && uri.host == host + link["href"] = uri.path + link["href"] += "?#{uri.query}" if uri.query + link["href"] += "##{uri.fragment}" if uri.fragment + end + end + + fragment.to_html + end + + def transform_body_for_import(body) + return body if body.blank? + + Nokogiri::HTML.fragment(body) + .then { convert_gids_to_sgids(it) } + .then { replace_account_slugs(it) } + .to_html + end + + def convert_gids_to_sgids(fragment) fragment.css("action-text-attachment[gid]").each do |node| gid = GlobalID.parse(node["gid"]) @@ -97,6 +126,20 @@ class Account::DataTransfer::ActionTextRichTextRecordSet < Account::DataTransfer end end - fragment.to_html + fragment + end + + def replace_account_slugs(fragment) + fragment.css("a[href]").each do |link| + match = link["href"].match(AccountSlug::PATH_INFO_MATCH) + + if match + path = match.post_match.presence || "/" + valid_path = Rails.application.routes.recognize_path(path) rescue nil + link["href"] = "#{account.slug}#{path}" if valid_path + end + end + + fragment end end 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 similarity index 55% rename from app/models/account/data_transfer/active_storage_blob_record_set.rb rename to app/models/account/data_transfer/active_storage/blob_record_set.rb index 10ee326ad..1637768f3 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 @@ -1,9 +1,9 @@ -class Account::DataTransfer::ActiveStorageBlobRecordSet < Account::DataTransfer::RecordSet +class Account::DataTransfer::ActiveStorage::BlobRecordSet < Account::DataTransfer::RecordSet def initialize(account) super( account: account, - model: ActiveStorage::Blob, - attributes: ActiveStorage::Blob.column_names - %w[service_name] + model: ::ActiveStorage::Blob, + attributes: ::ActiveStorage::Blob.column_names - %w[service_name] ) end @@ -13,7 +13,7 @@ class Account::DataTransfer::ActiveStorageBlobRecordSet < Account::DataTransfer: data = load(file) data.slice(*attributes).merge( "account_id" => account.id, - "service_name" => ActiveStorage::Blob.service.name + "service_name" => ::ActiveStorage::Blob.service.name ) end diff --git a/app/models/account/data_transfer/blob_file_record_set.rb b/app/models/account/data_transfer/active_storage/file_record_set.rb similarity index 72% rename from app/models/account/data_transfer/blob_file_record_set.rb rename to app/models/account/data_transfer/active_storage/file_record_set.rb index 4bbfcba10..15d359353 100644 --- a/app/models/account/data_transfer/blob_file_record_set.rb +++ b/app/models/account/data_transfer/active_storage/file_record_set.rb @@ -1,11 +1,11 @@ -class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSet +class Account::DataTransfer::ActiveStorage::FileRecordSet < Account::DataTransfer::RecordSet def initialize(account) - super(account: account, model: ActiveStorage::Blob) + super(account: account, model: ::ActiveStorage::Blob) end private def records - ActiveStorage::Blob.where(account: account) + ::ActiveStorage::Blob.where(account: account) end def export_record(blob) @@ -23,7 +23,7 @@ class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSe def import_batch(files) files.each do |file| key = File.basename(file) - blob = ActiveStorage::Blob.find_by(key: key, account: account) + blob = ::ActiveStorage::Blob.find_by(key: key, account: account) next unless blob zip.read(file) do |stream| @@ -35,7 +35,7 @@ class Account::DataTransfer::BlobFileRecordSet < Account::DataTransfer::RecordSe def check_record(file_path) key = File.basename(file_path) - unless zip.exists?("data/active_storage_blobs/#{key}.json") || ActiveStorage::Blob.exists?(key: key, account: account) + 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 end diff --git a/app/models/account/data_transfer/manifest.rb b/app/models/account/data_transfer/manifest.rb index b5976e3d5..2159926d5 100644 --- a/app/models/account/data_transfer/manifest.rb +++ b/app/models/account/data_transfer/manifest.rb @@ -35,10 +35,10 @@ class Account::DataTransfer::Manifest ::Filter, ::Webhook::DelinquencyTracker, ::Event, ::Notification, ::Notification::Bundle, ::Webhook::Delivery ), - Account::DataTransfer::ActiveStorageBlobRecordSet.new(account), + Account::DataTransfer::ActiveStorage::BlobRecordSet.new(account), *build_record_sets(::ActiveStorage::Attachment), - Account::DataTransfer::ActionTextRichTextRecordSet.new(account), - Account::DataTransfer::BlobFileRecordSet.new(account) + Account::DataTransfer::ActionText::RichTextRecordSet.new(account), + Account::DataTransfer::ActiveStorage::FileRecordSet.new(account) ] end diff --git a/app/models/account/import.rb b/app/models/account/import.rb index 14ee376ea..50ec3769d 100644 --- a/app/models/account/import.rb +++ b/app/models/account/import.rb @@ -48,6 +48,7 @@ class Account::Import < ApplicationRecord end add_importer_to_all_access_boards + reconcile_cards_count reconcile_account_storage mark_completed @@ -78,6 +79,10 @@ class Account::Import < ApplicationRecord ImportMailer.failed(self).deliver_later end + def reconcile_cards_count + account.update_column :cards_count, [ account.cards_count, account.cards.maximum(:number).to_i ].max + end + def add_importer_to_all_access_boards importer = account.users.find_by!(identity: identity) diff --git a/test/models/account/data_transfer/action_text/rich_text_record_set_test.rb b/test/models/account/data_transfer/action_text/rich_text_record_set_test.rb new file mode 100644 index 000000000..a28935a3e --- /dev/null +++ b/test/models/account/data_transfer/action_text/rich_text_record_set_test.rb @@ -0,0 +1,181 @@ +require "test_helper" + +class Account::DataTransfer::ActionText::RichTextRecordSetTest < ActiveSupport::TestCase + test "check rejects ActionText record referencing existing card in another account" do + importing_account = Account.create!(name: "Importing Account", external_account_id: 99999999) + + victim_card = cards(:logo) + assert_not_equal importing_account.id, victim_card.account_id, "Card must belong to a different account" + + # Create a malicious ActionText record that points to the victim's card + malicious_action_text_data = { + "id" => "malicious_action_text_id_12345", + "account_id" => importing_account.id, + "record_type" => "Card", + "record_id" => victim_card.id, + "name" => "description", + "body" => "

Injected content from attacker

", + "created_at" => Time.current.iso8601, + "updated_at" => Time.current.iso8601 + } + + tempfile = Tempfile.new([ "malicious_import", ".zip" ]) + tempfile.binmode + + writer = ZipFile::Writer.new(tempfile) + writer.add_file("data/action_text_rich_texts/#{malicious_action_text_data['id']}.json", malicious_action_text_data.to_json) + writer.close + tempfile.rewind + + reader = ZipFile::Reader.new(tempfile) + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(importing_account) + + error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do + record_set.check(from: reader) + end + + assert_match(/references existing record.*Card.*#{victim_card.id}/i, error.message) + ensure + tempfile&.close + tempfile&.unlink + importing_account&.destroy + end + + test "transform_body_for_import skips GIDs belonging to another account" do + victim_tag = tags(:web) + attacker_account = accounts(:initech) + assert_not_equal attacker_account.id, victim_tag.account_id + + cross_tenant_gid = victim_tag.to_global_id.to_s + html = %() + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(attacker_account) + result = record_set.send(:transform_body_for_import, html) + + assert_no_match(/sgid=/, result, "Cross-tenant GID must not be converted to SGID") + assert_match(/gid=/, result, "Original GID should remain unconverted") + end + + test "transform_body_for_import converts GIDs belonging to the same account" do + own_tag = tags(:web) + own_account = accounts(:"37s") + assert_equal own_account.id, own_tag.account_id + + same_account_gid = own_tag.to_global_id.to_s + html = %() + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(own_account) + result = record_set.send(:transform_body_for_import, html) + + assert_match(/sgid=/, result, "Same-account GID should be converted to SGID") + assert_no_match(/ gid=/, result, "GID should be removed after SGID conversion") + end + + test "transform_body_for_import handles non-existent record GIDs gracefully" do + nonexistent_gid = "gid://fizzy/Tag/00000000000000000000000000" + html = %() + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s")) + result = record_set.send(:transform_body_for_import, html) + + assert_no_match(/sgid=/, result, "Non-existent record should not produce SGID") + end + + test "replace_account_slugs rewrites relative URLs with account slug" do + target_account = accounts(:"37s") + source_slug = "9999999" + target_slug = AccountSlug.encode(target_account.external_account_id) + + html = %(

See card 42

) + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account) + result = record_set.send(:transform_body_for_import, html) + + assert_includes result, "/#{target_slug}/cards/42" + assert_not_includes result, source_slug + end + + test "replace_account_slugs leaves absolute URLs alone" do + target_account = accounts(:"37s") + + html = %(

See board

) + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account) + result = record_set.send(:transform_body_for_import, html) + + assert_includes result, "https://fizzy.app/9999999/boards/7" + end + + test "replace_account_slugs leaves plain text alone" do + target_account = accounts(:"37s") + + html = "

Nothing to rewrite here

" + + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(target_account) + result = record_set.send(:transform_body_for_import, html) + + assert_equal html, result + end + + test "relativize_urls strips instance host from absolute URLs" do + with_default_url_host("fizzy.example.com") do + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s")) + + html = %(

See card

) + result = record_set.send(:relativize_urls, html) + + assert_includes result, %(/123/cards/42) + assert_not_includes result, "fizzy.example.com" + end + end + + test "relativize_urls preserves query and fragment" do + with_default_url_host("fizzy.example.com") do + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s")) + + html = %(

link

) + result = record_set.send(:relativize_urls, html) + + assert_includes result, "/123/cards/42?tab=comments#comment_1" + assert_not_includes result, "fizzy.example.com" + end + end + + test "relativize_urls leaves external URLs alone" do + with_default_url_host("fizzy.example.com") do + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s")) + + html = %(

link

) + result = record_set.send(:relativize_urls, html) + + assert_includes result, "https://github.com/some/repo" + end + end + + test "relativize_urls is a no-op when host is not configured" do + with_default_url_host(nil) do + record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(accounts(:"37s")) + + html = %(

link

) + result = record_set.send(:relativize_urls, html) + + assert_includes result, "https://fizzy.example.com/123/cards/42" + end + end + + private + def with_default_url_host(host) + options = Rails.application.routes.default_url_options + had_key = options.key?(:host) + original = options[:host] + options[:host] = host + yield + ensure + if had_key + options[:host] = original + else + options.delete(:host) + end + end +end diff --git a/test/models/account/data_transfer/action_text_rich_text_record_set_test.rb b/test/models/account/data_transfer/action_text_rich_text_record_set_test.rb deleted file mode 100644 index 14a7aeb9c..000000000 --- a/test/models/account/data_transfer/action_text_rich_text_record_set_test.rb +++ /dev/null @@ -1,84 +0,0 @@ -require "test_helper" - -class Account::DataTransfer::ActionTextRichTextRecordSetTest < ActiveSupport::TestCase - test "check rejects ActionText record referencing existing card in another account" do - importing_account = Account.create!(name: "Importing Account", external_account_id: 99999999) - - victim_card = cards(:logo) - assert_not_equal importing_account.id, victim_card.account_id, "Card must belong to a different account" - - # Create a malicious ActionText record that points to the victim's card - malicious_action_text_data = { - "id" => "malicious_action_text_id_12345", - "account_id" => importing_account.id, - "record_type" => "Card", - "record_id" => victim_card.id, - "name" => "description", - "body" => "

Injected content from attacker

", - "created_at" => Time.current.iso8601, - "updated_at" => Time.current.iso8601 - } - - tempfile = Tempfile.new([ "malicious_import", ".zip" ]) - tempfile.binmode - - writer = ZipFile::Writer.new(tempfile) - writer.add_file("data/action_text_rich_texts/#{malicious_action_text_data['id']}.json", malicious_action_text_data.to_json) - writer.close - tempfile.rewind - - reader = ZipFile::Reader.new(tempfile) - - record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(importing_account) - - error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do - record_set.check(from: reader) - end - - assert_match(/references existing record.*Card.*#{victim_card.id}/i, error.message) - ensure - tempfile&.close - tempfile&.unlink - importing_account&.destroy - end - - test "convert_gids_to_sgids skips GIDs belonging to another account" do - victim_tag = tags(:web) - attacker_account = accounts(:initech) - assert_not_equal attacker_account.id, victim_tag.account_id - - cross_tenant_gid = victim_tag.to_global_id.to_s - html = %() - - record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(attacker_account) - result = record_set.send(:convert_gids_to_sgids, html) - - assert_no_match(/sgid=/, result, "Cross-tenant GID must not be converted to SGID") - assert_match(/gid=/, result, "Original GID should remain unconverted") - end - - test "convert_gids_to_sgids converts GIDs belonging to the same account" do - own_tag = tags(:web) - own_account = accounts(:"37s") - assert_equal own_account.id, own_tag.account_id - - same_account_gid = own_tag.to_global_id.to_s - html = %() - - record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(own_account) - result = record_set.send(:convert_gids_to_sgids, html) - - assert_match(/sgid=/, result, "Same-account GID should be converted to SGID") - assert_no_match(/ gid=/, result, "GID should be removed after SGID conversion") - end - - test "convert_gids_to_sgids handles non-existent record GIDs gracefully" do - nonexistent_gid = "gid://fizzy/Tag/00000000000000000000000000" - html = %() - - record_set = Account::DataTransfer::ActionTextRichTextRecordSet.new(accounts(:"37s")) - result = record_set.send(:convert_gids_to_sgids, html) - - assert_no_match(/sgid=/, result, "Non-existent record should not produce SGID") - end -end diff --git a/test/models/account/import_test.rb b/test/models/account/import_test.rb index a33cc32c2..7bf407041 100644 --- a/test/models/account/import_test.rb +++ b/test/models/account/import_test.rb @@ -62,6 +62,36 @@ class Account::ImportTest < ActiveSupport::TestCase export_tempfile&.unlink end + test "import reconciles cards count so new cards get correct numbers" do + source_account = accounts("37s") + exporter = users(:david) + identity = exporter.identity + max_card_number = source_account.cards.maximum(:number) + + 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) } + + 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 + import.process + + target_account.reload + assert_operator target_account.cards_count, :>=, max_card_number + ensure + export_tempfile&.close + export_tempfile&.unlink + end + test "check sets no failure_reason for unexpected errors" do import = Account::Import.create!(identity: identities(:david), account: Account.create!(name: "Import Test"))