From c269e9bd9baeee4971c0b760b44cf1212ab26fd6 Mon Sep 17 00:00:00 2001
From: Bob Long <robertjflong@gmail.com>
Date: Sat, 7 Mar 2015 13:55:17 +0000
Subject: [PATCH] Safer argument parsing (attempt 2)

---
 lib/stripe/account.rb                |  5 ++++-
 lib/stripe/api_resource.rb           |  2 +-
 lib/stripe/singleton_api_resource.rb |  2 +-
 lib/stripe/util.rb                   | 13 +++++++++++--
 test/stripe/account_test.rb          |  9 +++++++++
 test/stripe/api_resource_test.rb     | 18 +++++++++++++++++-
 test/stripe/util_test.rb             |  5 +++++
 7 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/lib/stripe/account.rb b/lib/stripe/account.rb
index 7f539b8e1..d9e98a5a2 100644
--- a/lib/stripe/account.rb
+++ b/lib/stripe/account.rb
@@ -13,7 +13,8 @@ def url
     end
 
     # @override To make id optional
-    def self.retrieve(id=nil, opts={})
+    def self.retrieve(id=ARGUMENT_NOT_PROVIDED, opts={})
+      id = id.equal?(ARGUMENT_NOT_PROVIDED) ? nil : Util.check_string_argument!(id)
       # Account used to be a singleton, where this method's signature was `(opts={})`.
       # For the sake of not breaking folks who pass in an OAuth key in opts, let's lurkily
       # string match for it.
@@ -31,5 +32,7 @@ def deauthorize(client_id, opts={})
       opts.delete(:api_base) # the api_base here is a one-off, don't persist it
       Util.convert_to_stripe_object(response, opts)
     end
+
+    ARGUMENT_NOT_PROVIDED = Object.new
   end
 end
diff --git a/lib/stripe/api_resource.rb b/lib/stripe/api_resource.rb
index 3c875e00e..79397c633 100644
--- a/lib/stripe/api_resource.rb
+++ b/lib/stripe/api_resource.rb
@@ -25,7 +25,7 @@ def refresh
       refresh_from(response, opts)
     end
 
-    def self.retrieve(id, opts=nil)
+    def self.retrieve(id, opts={})
       opts = Util.normalize_opts(opts)
       instance = self.new(id, opts)
       instance.refresh
diff --git a/lib/stripe/singleton_api_resource.rb b/lib/stripe/singleton_api_resource.rb
index 0b7edcb88..3f8f881f7 100644
--- a/lib/stripe/singleton_api_resource.rb
+++ b/lib/stripe/singleton_api_resource.rb
@@ -12,7 +12,7 @@ def url
     end
 
     def self.retrieve(opts={})
-      instance = self.new(nil, opts)
+      instance = self.new(nil, Util.normalize_opts(opts))
       instance.refresh
       instance
     end
diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb
index 2aa618a2b..3b0f7a6d1 100644
--- a/lib/stripe/util.rb
+++ b/lib/stripe/util.rb
@@ -123,15 +123,24 @@ def self.flatten_params_array(value, calculated_key)
     # Turn this value into an api_key and a set of headers
     def self.normalize_opts(opts)
       case opts
-      when NilClass
-        {}
       when String
         {:api_key => opts}
       when Hash
+        check_api_key!(opts.fetch(:api_key)) if opts.has_key?(:api_key)
         opts.clone
       else
         raise TypeError.new('normalize_opts expects a string or a hash')
       end
     end
+
+    def self.check_string_argument!(key)
+      raise TypeError.new("argument must be a string") unless key.is_a?(String)
+      key
+    end
+
+    def self.check_api_key!(key)
+      raise TypeError.new("api_key must be a string") unless key.is_a?(String)
+      key
+    end
   end
 end
diff --git a/test/stripe/account_test.rb b/test/stripe/account_test.rb
index 1ea1888bc..1defe0044 100644
--- a/test/stripe/account_test.rb
+++ b/test/stripe/account_test.rb
@@ -62,5 +62,14 @@ class AccountTest < Test::Unit::TestCase
       end.returns(test_response({ 'stripe_user_id' => a.id }))
       a.deauthorize('ca_1234', 'sk_test_1234')
     end
+
+    should "reject nil api keys" do
+      assert_raise TypeError do
+        Stripe::Account.retrieve(nil)
+      end
+      assert_raise TypeError do
+        Stripe::Account.retrieve(:api_key => nil)
+      end
+    end
   end
 end
diff --git a/test/stripe/api_resource_test.rb b/test/stripe/api_resource_test.rb
index 7a5783e0f..0c9b0ec14 100644
--- a/test/stripe/api_resource_test.rb
+++ b/test/stripe/api_resource_test.rb
@@ -37,6 +37,15 @@ class ApiResourceTest < Test::Unit::TestCase
       end
     end
 
+    should "using a nil api key should raise an exception" do
+      assert_raises TypeError do
+        Stripe::Customer.all({}, nil)
+      end
+      assert_raises TypeError do
+        Stripe::Customer.all({}, { :api_key => nil })
+      end
+    end
+
     should "specifying api credentials containing whitespace should raise an exception" do
       Stripe.api_key = "key "
       assert_raises Stripe::AuthenticationError do
@@ -454,7 +463,14 @@ class ApiResourceTest < Test::Unit::TestCase
           }
         })
 
-        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[first_name]=Bob&legal_entity[last_name]=').returns(test_response({"id" => "myid"}))
+        @mock.expects(:post).once.with(
+          "#{Stripe.api_base}/v1/accounts/myid",
+          nil,
+          any_of(
+            'legal_entity[first_name]=Bob&legal_entity[last_name]=',
+            'legal_entity[last_name]=&legal_entity[first_name]=Bob'
+          )
+        ).returns(test_response({"id" => "myid"}))
 
         acct.legal_entity = {:first_name => 'Bob'}
         acct.save
diff --git a/test/stripe/util_test.rb b/test/stripe/util_test.rb
index e703cb841..a7c4f91ad 100644
--- a/test/stripe/util_test.rb
+++ b/test/stripe/util_test.rb
@@ -25,5 +25,10 @@ class UtilTest < Test::Unit::TestCase
       symbolized = Stripe::Util.symbolize_names(start)
       assert_equal(finish, symbolized)
     end
+
+    should "normalize_opts should reject nil keys" do
+      assert_raise { Stripe::Util.normalize_opts(nil) }
+      assert_raise { Stripe::Util.normalize_opts(:api_key => nil) }
+    end
   end
 end