Implement deep copy for StripeObject and remove marshal/unmarshal

We were previously using a bit of a hack to get a free deep copy
implementation through Ruby's marshaling framework. Lint call this out
as a security problem though, and rightfully so: when combined with
unsanitized user input, unmarshaling can result in very serious security
breaches involving arbitrary code execution.

This patch removes all uses of marshal/unmarshal in favor of
implementing a deep copy method for `StripeObject`. I also reworked some
of the constants around what keys are available for `opts`. I'm still
not completely happy with the results, but I think it's going to need a
slightly larger refactor in order to get somewhere truly good.

There is what could be a breaking change for people doing non-standard
stuff with the library: the opts that we copy with an object are now
whitelisted, so if they were being used to pass around extraneous data,
that might not work as expected anymore. But because this is a contract
that we never committed to, I don't think I'd bump the major version for
change.

.rubocop_todo.yml

@@ -35,10 +35,10 @@ Metrics/LineLength:
 Metrics/MethodLength:
   Max: 47
 
-# Offense count: 1
+# Offense count: 2
 # Configuration parameters: CountComments.
 Metrics/ModuleLength:
-  Max: 290
+  Max: 423
 
 # Offense count: 6
 # Configuration parameters: CountKeywordArgs.
@@ -51,11 +51,6 @@ Metrics/PerceivedComplexity:
   Exclude:
     - 'lib/stripe/stripe_object.rb'
 
-# Offense count: 1
-Security/MarshalLoad:
-  Exclude:
-    - 'lib/stripe/stripe_object.rb'
-
 # Offense count: 2
 Style/ClassVars:
   Exclude:

lib/stripe/api_operations/request.rb

@@ -2,8 +2,6 @@ module Stripe
   module APIOperations
     module Request
       module ClassMethods
-        OPTS_KEYS_TO_PERSIST = Set[:api_key, :api_base, :client, :stripe_account, :stripe_version]
-
         def request(method, url, params = {}, opts = {})
           warn_on_opts_in_params(params)
 
@@ -25,19 +23,16 @@ def request(method, url, params = {}, opts = {})
           # Hash#select returns an array before 1.9
           opts_to_persist = {}
           opts.each do |k, v|
-            opts_to_persist[k] = v if OPTS_KEYS_TO_PERSIST.include?(k)
+            opts_to_persist[k] = v if Util::OPTS_KEYS_TO_PERSIST.include?(k)
           end
 
           [resp, opts_to_persist]
         end
 
         private
 
-        KNOWN_OPTS = Set[:api_key, :idempotency_key, :stripe_account, :stripe_version]
-        private_constant :KNOWN_OPTS
-
         def warn_on_opts_in_params(params)
-          KNOWN_OPTS.each do |opt|
+          Util::OPTS_USER_SPECIFIED.each do |opt|
             if params.key?(opt)
               $stderr.puts("WARNING: #{opt} should be in opts instead of params.")
             end

lib/stripe/stripe_object.rb

@@ -133,20 +133,6 @@ def each(&blk)
       @values.each(&blk)
     end
 
-    def _dump(_level)
-      # The StripeClient instance in @opts is not serializable and is not
-      # really a property of the StripeObject, so we exclude it when
-      # dumping
-      opts = @opts.clone
-      opts.delete(:client)
-      Marshal.dump([@values, opts])
-    end
-
-    def self._load(args)
-      values, opts = Marshal.load(args)
-      construct_from(values, opts)
-    end
-
     # Sets all keys within the StripeObject as unsaved so that they will be
     # included with an update when #serialize_params is called. This method is
     # also recursive, so any StripeObjects contained as values or which are
@@ -304,7 +290,9 @@ def respond_to_missing?(symbol, include_private = false)
     #   remove accessors.
     def initialize_from(values, opts, partial = false)
       @opts = Util.normalize_opts(opts)
-      @original_values = Marshal.load(Marshal.dump(values)) # deep copy
+
+      # the `#send` is here so that we can keep this method private
+      @original_values = self.class.send(:deep_copy, values)
 
       removed = partial ? Set.new : Set.new(@values.keys - values.keys)
       added = Set.new(values.keys - @values.keys)
@@ -404,6 +392,30 @@ def serialize_params_value(value, original, unsaved, force, key: nil)
 
     private
 
+    # Produces a deep copy of the given object including support for arrays,
+    # hashes, and StripeObjects.
+    def self.deep_copy(obj)
+      case obj
+      when Array
+        obj.map { |e| deep_copy(e) }
+      when Hash
+        obj.each_with_object({}) do |(k, v), copy|
+          copy[k] = deep_copy(v)
+          copy
+        end
+      when StripeObject
+        StripeObject.construct_from(
+          deep_copy(obj.instance_variable_get(:@values)),
+          obj.instance_variable_get(:@opts).select do |k, _v|
+            Util::OPTS_COPYABLE.include?(k)
+          end
+        )
+      else
+        obj
+      end
+    end
+    private_class_method :deep_copy
+
     def dirty_value!(value)
       case value
       when Array

lib/stripe/util.rb

@@ -2,6 +2,22 @@
 
 module Stripe
   module Util
+    # Options that a user is allowed to specify.
+    OPTS_USER_SPECIFIED = Set[
+      :api_key,
+      :idempotency_key,
+      :stripe_account,
+      :stripe_version
+    ].freeze
+
+    # Options that should be copyable from one StripeObject to another
+    # including options that may be internal.
+    OPTS_COPYABLE = (OPTS_USER_SPECIFIED + Set[:api_base]).freeze
+
+    # Options that should be persisted between API requests. This includes
+    # client, which is an object containing an HTTP client to reuse.
+    OPTS_KEYS_TO_PERSIST = (OPTS_USER_SPECIFIED + Set[:client]).freeze
+
     def self.objects_to_ids(h)
       case h
       when APIResource

test/stripe/stripe_object_test.rb

@@ -35,19 +35,75 @@ class StripeObjectTest < Test::Unit::TestCase
       assert_equal "Stripe", obj[:name]
     end
 
-    should "marshal a stripe object correctly" do
-      obj = Stripe::StripeObject.construct_from(
-        { id: 1, name: "Stripe" },
-        api_key: "apikey",
-        # StripeClient is not serializable and is expected to be removed
-        # when marshalling StripeObjects
-        client: StripeClient.active_client
-      )
-      m = Marshal.load(Marshal.dump(obj))
-      assert_equal 1, m.id
-      assert_equal "Stripe", m.name
-      expected_hash = { api_key: "apikey" }
-      assert_equal expected_hash, m.instance_variable_get("@opts")
+    context "#deep_copy" do
+      should "produce a deep copy" do
+        opts = {
+          api_base: Stripe.api_base,
+          api_key: "apikey",
+        }
+        values = {
+          id: 1,
+          name: "Stripe",
+          arr: [
+            StripeObject.construct_from({ id: "index0" }, opts),
+            "index1",
+            2,
+          ],
+          map: {
+            :"0" => StripeObject.construct_from({ id: "index0" }, opts),
+            :"1" => "index1",
+            :"2" => 2,
+          },
+        }
+
+        # it's not good to test methods with `#send` like this, but I've done
+        # it in the interest of trying to keep `.deep_copy` as internal as
+        # possible
+        copy_values = Stripe::StripeObject.send(:deep_copy, values)
+
+        # we can't compare the hashes directly because they have embedded
+        # objects which are different from each other
+        assert_equal values[:id], copy_values[:id]
+        assert_equal values[:name], copy_values[:name]
+
+        assert_equal values[:arr].length, copy_values[:arr].length
+
+        # internal values of the copied StripeObject should be the same
+        # (including opts), but the object itself should be new (hence the
+        # refutation of equality on #object_id)
+        assert_equal values[:arr][0][:id], copy_values[:arr][0][:id]
+        refute_equal values[:arr][0].object_id, copy_values[:arr][0].object_id
+        assert_equal values[:arr][0].instance_variable_get(:@opts),
+                     copy_values[:arr][0].instance_variable_get(:@opts)
+
+        # scalars however, can be compared
+        assert_equal values[:arr][1], copy_values[:arr][1]
+        assert_equal values[:arr][2], copy_values[:arr][2]
+
+        # and a similar story with the hash
+        assert_equal values[:map].keys, copy_values[:map].keys
+        assert_equal values[:map][:"0"][:id], copy_values[:map][:"0"][:id]
+        refute_equal values[:map][:"0"].object_id, copy_values[:map][:"0"].object_id
+        assert_equal values[:map][:"0"].instance_variable_get(:@opts),
+                     copy_values[:map][:"0"].instance_variable_get(:@opts)
+        assert_equal values[:map][:"1"], copy_values[:map][:"1"]
+        assert_equal values[:map][:"2"], copy_values[:map][:"2"]
+      end
+
+      should "not copy a client" do
+        opts = {
+          api_key: "apikey",
+          client: StripeClient.active_client,
+        }
+        values = { id: 1, name: "Stripe" }
+
+        obj = Stripe::StripeObject.construct_from(values, opts)
+        copy_obj = Stripe::StripeObject.send(:deep_copy, obj)
+
+        assert_equal values, copy_obj.instance_variable_get(:@values)
+        assert_equal opts.reject { |k, _v| k == :client },
+                     copy_obj.instance_variable_get(:@opts)
+      end
     end
 
     should "recursively call to_hash on its values" do