Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix encoding of nested parameters in multipart requests #624

Merged
merged 1 commit into from
Apr 5, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries

For multipart requests, we don't encode the parameters ourselves and instead pass them directly to curl:

$opts[CURLOPT_POSTFIELDS] = $hasFile ? $params : Util\Util::encodeParameters($params);

However, curl doesn't know how to encode arrays and encodes them as Array regardless of their contents. For instance this request:

$file = \Stripe\File::create([
  "file" => new \CurlFile("/Users/ob/test.txt"),
  "purpose" => "identity_document",
  "file_link_data" => ["create" => true],
]);

would be encoded as:

--------------------------9f9f472a73222896
Content-Disposition: form-data; name="file"; filename="/Users/ob/test.txt"
Content-Type: application/octet-stream

hello

--------------------------9f9f472a73222896
Content-Disposition: form-data; name="purpose"

identity_document
--------------------------9f9f472a73222896
Content-Disposition: form-data; name="file_link_data"

Array
--------------------------9f9f472a73222896--

which of course causes the request to fail.

This PR fixes this by manually flattening the parameters before passing them to curl. It's unfortunately difficult to write tests for this because we can't mock curl, but I've verified that requests are encoded correctly even with deeply nested parameters. E.g. with this patch, the following request:

$file = \Stripe\File::create([
  "file" => new \CurlFile("/Users/ob/test.txt"),
  "purpose" => "identity_document",
  "file_link_data" => [
    "create" => true,
    "expires_at" => 1234567890,
    "metadata" => [
      "key1" => "value1",
      "key2" => "value2",
    ],
  ],
]);

is encoded as:

--------------------------344ee1cb94395191
Content-Disposition: form-data; name="file"; filename="/Users/ob/test.txt"
Content-Type: application/octet-stream

hello

--------------------------344ee1cb94395191
Content-Disposition: form-data; name="purpose"

identity_document
--------------------------344ee1cb94395191
Content-Disposition: form-data; name="file_link_data[create]"

true
--------------------------344ee1cb94395191
Content-Disposition: form-data; name="file_link_data[expires_at]"

1234567890
--------------------------344ee1cb94395191
Content-Disposition: form-data; name="file_link_data[metadata][key1]"

value1
--------------------------344ee1cb94395191
Content-Disposition: form-data; name="file_link_data[metadata][key2]"

value2
--------------------------344ee1cb94395191--

@ob-stripe
Copy link
Contributor Author

Of course my elegant one-liner doesn't work on PHP 5.4 😢

@ob-stripe ob-stripe force-pushed the ob-fix-multipart-nested-params-encoding branch from 137adbc to 873d129 Compare April 5, 2019 06:39
@ob-stripe ob-stripe force-pushed the ob-fix-multipart-nested-params-encoding branch from 873d129 to eeebf90 Compare April 5, 2019 06:41
@remi-stripe
Copy link
Contributor

This looks fine to me especially as you explicitly tested manually but deferring to @brandur-stripe to approve

@remi-stripe remi-stripe removed their assignment Apr 5, 2019
Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix OB! Looks good to me as well.

@ob-stripe
Copy link
Contributor Author

Thanks both!

@ob-stripe ob-stripe merged commit f513348 into master Apr 5, 2019
@ob-stripe ob-stripe deleted the ob-fix-multipart-nested-params-encoding branch April 5, 2019 17:08
@ob-stripe
Copy link
Contributor Author

Released as 6.31.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants