Merge pull request #2678 from basecamp/verify-import-models

Validate polymorphic types in import against allowlist

app/models/account/data_transfer/manifest.rb

@@ -26,20 +26,42 @@ class Account::DataTransfer::Manifest
       [
         Account::DataTransfer::AccountRecordSet.new(account),
         Account::DataTransfer::UserRecordSet.new(account),
-        *build_record_sets(::User::Settings, ::Tag, ::Board, ::Column),
+        *build_record_sets(
+          ::User::Settings,
+          ::Tag,
+          ::Board,
+          ::Column
+        ),
         Account::DataTransfer::EntropyRecordSet.new(account),
         *build_record_sets(
-          ::Board::Publication, ::Webhook, ::Access, ::Card, ::Comment, ::Step,
-          ::Assignment, ::Tagging, ::Closure, ::Card::Goldness, ::Card::NotNow,
-          ::Card::ActivitySpike, ::Watch, ::Pin, ::Reaction, ::Mention,
-          ::Filter, ::Webhook::DelinquencyTracker, ::Event,
-          ::Notification, ::Notification::Bundle, ::Webhook::Delivery
+          ::Board::Publication,
+          ::Webhook,
+          ::Access,
+          ::Card,
+          ::Comment,
+          ::Step,
+          ::Assignment,
+          ::Tagging,
+          ::Closure,
+          ::Card::Goldness,
+          ::Card::NotNow,
+          ::Card::ActivitySpike,
+          ::Watch,
+          ::Pin,
+          ::Reaction,
+          ::Mention,
+          ::Filter,
+          ::Webhook::DelinquencyTracker,
+          ::Event,
+          ::Notification,
+          ::Notification::Bundle,
+          ::Webhook::Delivery
         ),
         Account::DataTransfer::ActiveStorage::BlobRecordSet.new(account),
         *build_record_sets(::ActiveStorage::Attachment),
         Account::DataTransfer::ActionText::RichTextRecordSet.new(account),
         Account::DataTransfer::ActiveStorage::FileRecordSet.new(account)
-      ]
+      ].then { set_importable_model_names(it) }
     end
 
     def build_record_sets(*models)
@@ -47,4 +69,10 @@ class Account::DataTransfer::Manifest
         Account::DataTransfer::RecordSet.new(account: account, model: model)
       end
     end
+
+    def set_importable_model_names(record_sets)
+      model_names = record_sets.filter_map { |record_set| record_set.model&.name }
+      record_sets.each { |record_set| record_set.importable_model_names = model_names }
+      record_sets
+    end
 end

app/models/account/data_transfer/record_set.rb

@@ -4,12 +4,14 @@ class Account::DataTransfer::RecordSet
 
   IMPORT_BATCH_SIZE = 100
 
+  attr_accessor :importable_model_names
   attr_reader :account, :model, :attributes
 
-  def initialize(account:, model:, attributes: nil)
+  def initialize(account:, model:, attributes: nil, importable_model_names: nil)
     @account = account
     @model = model
     @attributes = (attributes || model.column_names).map(&:to_s)
+    @importable_model_names = importable_model_names || [ model.name ]
   end
 
   def export(to:, start: nil)
@@ -113,7 +115,7 @@ class Account::DataTransfer::RecordSet
     def check_association_doesnt_exist(data, association, associated_id)
       if association.polymorphic?
         type_column = association.foreign_type.to_s
-        associated_class = data[type_column].constantize
+        associated_class = verify_model_type(data[type_column])
       else
         associated_class = association.klass
       end
@@ -123,6 +125,14 @@ class Account::DataTransfer::RecordSet
       end
     end
 
+    def verify_model_type(type_name)
+      if importable_model_names.include?(type_name)
+        type_name.constantize
+      else
+        raise IntegrityError, "Unrecognized model type: #{type_name}"
+      end
+    end
+
     def skip_to(file_list, last_id)
       index = file_list.index(last_id)
 

test/models/account/data_transfer/action_text/rich_text_record_set_test.rb

@@ -30,6 +30,7 @@ class Account::DataTransfer::ActionText::RichTextRecordSetTest < ActiveSupport::
     reader = ZipFile::Reader.new(tempfile)
 
     record_set = Account::DataTransfer::ActionText::RichTextRecordSet.new(importing_account)
+    record_set.importable_model_names = %w[ ActionText::RichText Card ]
 
     error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
       record_set.check(from: reader)

test/models/account/data_transfer/record_set_test.rb

@@ -0,0 +1,93 @@
+require "test_helper"
+
+class Account::DataTransfer::RecordSetTest < ActiveSupport::TestCase
+  setup do
+    @importable_model_names = %w[ Card Board Event ]
+  end
+
+  test "check rejects polymorphic type not in the importable models allowlist" do
+    event_data = build_event_data(eventable_type: "Identity")
+
+    record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Event, importable_model_names: @importable_model_names)
+
+    error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
+      record_set.check(from: build_reader(dir: "events", data: event_data))
+    end
+
+    assert_match(/unrecognized.*type/i, error.message)
+  end
+
+  test "check rejects nonexistent polymorphic type" do
+    event_data = build_event_data(eventable_type: "Nonexistent::ClassName")
+
+    record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Event, importable_model_names: @importable_model_names)
+
+    error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
+      record_set.check(from: build_reader(dir: "events", data: event_data))
+    end
+
+    assert_match(/unrecognized.*type/i, error.message)
+  end
+
+  test "check rejects non-ActiveRecord class used as polymorphic type" do
+    event_data = build_event_data(eventable_type: "ActiveSupport::BroadcastLogger")
+
+    record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Event, importable_model_names: @importable_model_names)
+
+    error = assert_raises(Account::DataTransfer::RecordSet::IntegrityError) do
+      record_set.check(from: build_reader(dir: "events", data: event_data))
+    end
+
+    assert_match(/unrecognized.*type/i, error.message)
+  end
+
+  test "check accepts polymorphic type in the importable models allowlist" do
+    event_data = build_event_data(eventable_type: "Card")
+
+    record_set = Account::DataTransfer::RecordSet.new(account: importing_account, model: Event, importable_model_names: @importable_model_names)
+
+    assert_nothing_raised do
+      record_set.check(from: build_reader(dir: "events", data: event_data))
+    end
+  end
+
+  private
+    def importing_account
+      @importing_account ||= Account.create!(name: "Importing Account", external_account_id: 99999999)
+    end
+
+    def build_event_data(eventable_type:)
+      {
+        "id" => "test_event_id_12345678901234",
+        "account_id" => "nonexistent_account_id_1234567",
+        "board_id" => "nonexistent_board_id_12345678",
+        "creator_id" => "nonexistent_user_id_123456789",
+        "eventable_type" => eventable_type,
+        "eventable_id" => "nonexistent_id_1234567890123",
+        "action" => "created",
+        "particulars" => "{}",
+        "created_at" => Time.current.iso8601,
+        "updated_at" => Time.current.iso8601
+      }
+    end
+
+    def build_reader(dir:, data:)
+      tempfile = Tempfile.new([ "import_test", ".zip" ])
+      tempfile.binmode
+
+      writer = ZipFile::Writer.new(tempfile)
+      writer.add_file("data/#{dir}/#{data['id']}.json", data.to_json)
+      writer.close
+      tempfile.rewind
+
+      @tempfiles ||= []
+      @tempfiles << tempfile
+
+      ZipFile::Reader.new(tempfile)
+    end
+
+    def teardown
+      @tempfiles&.each { |f| f.close; f.unlink }
+      @importing_account&.destroy
+    end
+end