Add strict parsing for RequestOptions (#901)

stripe · Mar 24, 2020 · 69185d0 · 69185d0
1 parent 9fd6e5c
commit 69185d0
Show file tree

Hide file tree

Showing 4 changed files with 145 additions and 18 deletions.
diff --git a/lib/BaseStripeClient.php b/lib/BaseStripeClient.php
@@ -126,7 +126,7 @@ public function getFilesBase()
      */
     public function request($method, $path, $params, $opts)
     {
-        $opts = \Stripe\Util\RequestOptions::parse($opts);
+        $opts = \Stripe\Util\RequestOptions::parse($opts, true);
         $baseUrl = $opts->apiBase ?: $this->getApiBase();
         $requestor = new \Stripe\ApiRequestor($this->apiKeyForRequest($opts), $baseUrl);
         list($response, $opts->apiKey) = $requestor->request($method, $path, $params, $opts->headers);

diff --git a/lib/Util/RequestOptions.php b/lib/Util/RequestOptions.php
@@ -2,29 +2,40 @@
 
 namespace Stripe\Util;
 
-use Stripe\Exception;
-
 class RequestOptions
 {
     /**
-     * @var array a list of headers that should be persisted across requests
+     * @var array<string> a list of headers that should be persisted across requests
      */
     public static $HEADERS_TO_PERSIST = [
         'Stripe-Account',
         'Stripe-Version',
     ];
 
+    /** @var array<string, string> */
     public $headers;
+
+    /** @var null|string */
     public $apiKey;
+
+    /** @var null|string */
     public $apiBase;
 
+    /**
+     * @param null|string $key
+     * @param array<string, string> $headers
+     * @param null|string $base
+     */
     public function __construct($key = null, $headers = [], $base = null)
     {
         $this->apiKey = $key;
         $this->headers = $headers;
         $this->apiBase = $base;
     }
 
+    /**
+     * @return array<string, string>
+     */
     public function __debugInfo()
     {
         return [
@@ -38,13 +49,14 @@ public function __debugInfo()
      * Unpacks an options array and merges it into the existing RequestOptions
      * object.
      *
-     * @param null|array|string $options a key => value array
+     * @param null|array|RequestOptions|string $options a key => value array
+     * @param bool $strict when true, forbid string form and arbitrary keys in array form
      *
      * @return RequestOptions
      */
-    public function merge($options)
+    public function merge($options, $strict = false)
     {
-        $other_options = self::parse($options);
+        $other_options = self::parse($options, $strict);
         if (null === $other_options->apiKey) {
             $other_options->apiKey = $this->apiKey;
         }
@@ -71,11 +83,14 @@ public function discardNonPersistentHeaders()
     /**
      * Unpacks an options array into an RequestOptions object.
      *
-     * @param null|array|string $options a key => value array
+     * @param null|array|RequestOptions|string $options a key => value array
+     * @param bool $strict when true, forbid string form and arbitrary keys in array form
+     *
+     * @throws \Stripe\Exception\InvalidArgumentException
      *
      * @return RequestOptions
      */
-    public static function parse($options)
+    public static function parse($options, $strict = false)
     {
         if ($options instanceof self) {
             return $options;
@@ -86,27 +101,46 @@ public static function parse($options)
         }
 
         if (\is_string($options)) {
+            if ($strict) {
+                $message = 'Do not pass a string for request options. If you want to set the '
+                    . 'API key, pass an array like ["api_key" => <apiKey>] instead.';
+
+                throw new \Stripe\Exception\InvalidArgumentException($message);
+            }
+
             return new RequestOptions($options, [], null);
         }
 
         if (\is_array($options)) {
             $headers = [];
             $key = null;
             $base = null;
+
             if (\array_key_exists('api_key', $options)) {
                 $key = $options['api_key'];
+                unset($options['api_key']);
             }
             if (\array_key_exists('idempotency_key', $options)) {
                 $headers['Idempotency-Key'] = $options['idempotency_key'];
+                unset($options['idempotency_key']);
             }
             if (\array_key_exists('stripe_account', $options)) {
                 $headers['Stripe-Account'] = $options['stripe_account'];
+                unset($options['stripe_account']);
             }
             if (\array_key_exists('stripe_version', $options)) {
                 $headers['Stripe-Version'] = $options['stripe_version'];
+                unset($options['stripe_version']);
             }
             if (\array_key_exists('api_base', $options)) {
                 $base = $options['api_base'];
+                unset($options['api_base']);
+            }
+
+            if ($strict && !empty($options)) {
+                $message = 'Got unexpected keys in options array: ' . \implode(', ', \array_keys($options));
+
+                throw new \Stripe\Exception\InvalidArgumentException($message);
             }
 
             return new RequestOptions($key, $headers, $base);
@@ -117,7 +151,7 @@ public static function parse($options)
            . 'per-request options, which must be an array. (HINT: you can set '
            . 'a global apiKey by "Stripe::setApiKey(<apiKey>)")';
 
-        throw new Exception\InvalidArgumentException($message);
+        throw new \Stripe\Exception\InvalidArgumentException($message);
     }
 
     private function redactedApiKey()

diff --git a/tests/Stripe/BaseStripeClientTest.php b/tests/Stripe/BaseStripeClientTest.php
@@ -62,4 +62,26 @@ public function testRequestThrowsIfNoApiKeyInClientAndOpts()
         static::assertNotNull($charge);
         static::assertSame('ch_123', $charge->id);
     }
+
+    public function testRequestThrowsIfOptsIsString()
+    {
+        $this->expectException(\Stripe\Exception\InvalidArgumentException::class);
+        $this->expectExceptionMessageRegExp('#Do not pass a string for request options.#');
+
+        $client = new BaseStripeClient(null, null, MOCK_URL);
+        $charge = $client->request('get', '/v1/charges/ch_123', [], 'foo');
+        static::assertNotNull($charge);
+        static::assertSame('ch_123', $charge->id);
+    }
+
+    public function testRequestThrowsIfOptsIsArrayWithUnexpectedKeys()
+    {
+        $this->expectException(\Stripe\Exception\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Got unexpected keys in options array: foo');
+
+        $client = new BaseStripeClient(null, null, MOCK_URL);
+        $charge = $client->request('get', '/v1/charges/ch_123', [], ['foo' => 'bar']);
+        static::assertNotNull($charge);
+        static::assertSame('ch_123', $charge->id);
+    }
 }
diff --git a/tests/Stripe/Util/RequestOptionsTest.php b/tests/Stripe/Util/RequestOptionsTest.php
@@ -10,28 +10,39 @@ final class RequestOptionsTest extends \PHPUnit\Framework\TestCase
 {
     use \Stripe\TestHelper;
 
-    public function testStringAPIKey()
+    public function testParseString()
     {
         $opts = RequestOptions::parse('foo');
         static::assertSame('foo', $opts->apiKey);
         static::assertSame([], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testNull()
+    public function testParseStringStrict()
+    {
+        $this->expectException(\Stripe\Exception\InvalidArgumentException::class);
+        $this->expectExceptionMessageRegExp('#Do not pass a string for request options.#');
+
+        $opts = RequestOptions::parse('foo', true);
+    }
+
+    public function testParseNull()
     {
         $opts = RequestOptions::parse(null);
         static::assertNull($opts->apiKey);
         static::assertSame([], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testEmptyArray()
+    public function testParseArrayEmpty()
     {
         $opts = RequestOptions::parse([]);
         static::assertNull($opts->apiKey);
         static::assertSame([], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testAPIKeyArray()
+    public function testParseArrayWithAPIKey()
     {
         $opts = RequestOptions::parse(
             [
@@ -40,9 +51,10 @@ public function testAPIKeyArray()
         );
         static::assertSame('foo', $opts->apiKey);
         static::assertSame([], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testIdempotentKeyArray()
+    public function testParseArrayWithIdempotencyKey()
     {
         $opts = RequestOptions::parse(
             [
@@ -51,27 +63,86 @@ public function testIdempotentKeyArray()
         );
         static::assertNull($opts->apiKey);
         static::assertSame(['Idempotency-Key' => 'foo'], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testKeyArray()
+    public function testParseArrayWithAPIKeyAndIdempotencyKey()
     {
         $opts = RequestOptions::parse(
             [
-                'idempotency_key' => 'foo',
                 'api_key' => 'foo',
+                'idempotency_key' => 'foo',
             ]
         );
         static::assertSame('foo', $opts->apiKey);
         static::assertSame(['Idempotency-Key' => 'foo'], $opts->headers);
+        static::assertNull($opts->apiBase);
+    }
+
+    public function testParseArrayWithAPIKeyAndUnexpectedKeys()
+    {
+        $opts = RequestOptions::parse(
+            [
+                'api_key' => 'foo',
+                'foo' => 'bar',
+            ]
+        );
+        static::assertSame('foo', $opts->apiKey);
+        static::assertSame([], $opts->headers);
+        static::assertNull($opts->apiBase);
     }
 
-    public function testWrongType()
+    public function testParseArrayWithAPIKeyAndUnexpectedKeysStrict()
+    {
+        $this->expectException(\Stripe\Exception\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Got unexpected keys in options array: foo');
+
+        $opts = RequestOptions::parse(
+            [
+                'api_key' => 'foo',
+                'foo' => 'bar',
+            ],
+            true
+        );
+    }
+
+    public function testParseArrayWithAPIBase()
+    {
+        $opts = RequestOptions::parse(
+            [
+                'api_base' => 'https://example.com',
+            ]
+        );
+        static::assertNull($opts->apiKey);
+        static::assertSame([], $opts->headers);
+        static::assertSame('https://example.com', $opts->apiBase);
+    }
+
+    public function testParseWrongType()
     {
         $this->expectException(\Stripe\Exception\InvalidArgumentException::class);
 
         $opts = RequestOptions::parse(5);
     }
 
+    public function testMerge()
+    {
+        $baseOpts = RequestOptions::parse(
+            [
+                'api_key' => 'foo',
+                'idempotency_key' => 'foo',
+            ]
+        );
+        $opts = $baseOpts->merge(
+            [
+                'idempotency_key' => 'bar',
+            ]
+        );
+        static::assertSame('foo', $opts->apiKey);
+        static::assertSame(['Idempotency-Key' => 'bar'], $opts->headers);
+        static::assertNull($opts->apiBase);
+    }
+
     public function testDiscardNonPersistentHeaders()
     {
         $opts = RequestOptions::parse(