3021: Use Marshal dump/load to save @old_attributes. Otherwise, hashes

in @old_attributes can contain references to objects that can be
mutated by callers, which could result in the new attributes being
logged incorrectly as old_attributes.

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 38a46de09acbc7736e320e72ff6d6d2a6be62efc..308da7fa11576acc00066bb47de5592cf2543f96 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -523,8 +523,8 @@ class ArvadosModel < ActiveRecord::Base
   end
 
   def log_start_state
-    @old_attributes = attributes
-    @old_logged_attributes = logged_attributes
+    @old_attributes = Marshal.load(Marshal.dump(attributes))
+    @old_logged_attributes = Marshal.load(Marshal.dump(logged_attributes))
   end
 
   def log_change(event_type)

diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb
index 0c85d4c6e4b9a8e2744268a22ace33a8613e2655..d6b76fc6057633e28f8518aa9765a28ab121e3e2 100644 (file)
--- a/services/api/test/unit/log_test.rb
+++ b/services/api/test/unit/log_test.rb
@@ -94,6 +94,20 @@ class LogTest < ActiveSupport::TestCase
     end
   end
 
+  test "old_attributes preserves values deep inside a hash" do
+    set_user_from_auth :active
+    it = specimens(:owned_by_active_user)
+    it.properties = {'foo' => {'bar' => ['baz', 'qux', {'quux' => 'bleat'}]}}
+    it.save!
+    @log_count += 1
+    it.properties['foo']['bar'][2]['quux'] = 'blert'
+    it.save!
+    assert_logged it, :update do |props|
+      assert_equal 'bleat', props['old_attributes']['properties']['foo']['bar'][2]['quux']
+      assert_equal 'blert', props['new_attributes']['properties']['foo']['bar'][2]['quux']
+    end
+  end
+
   test "destroying an authorization makes a log" do
     set_user_from_auth :admin_trustedclient
     auth = api_client_authorizations(:spectator)