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

Explicitly specify utf8 encoding when calculating signature #371

Closed
wants to merge 2 commits into from

Conversation

onevcat
Copy link
Contributor

@onevcat onevcat commented Aug 22, 2017

This is a pull-request for a supporting ticket titled as "Signatures failing when using UTF-8 in charge". (Sorry I am not sure where to find the ticket number)

In some Ubuntu environments, the payload might follows another encoding and even setting the request encoding to 'utf8' will not affect the received data. Explicitly setting the encoding to 'utf8' will ensure that Stripe side and user side using the same encoding when computing signature.

@jlomas-stripe
Copy link
Contributor

Hi @onevcat! Thanks for submitting this! I've fixed the tests in #374 so if you can either wait until that lands and rebase master (or just rebase on that branch right now) so that this PR only includes the one specific change, that would be awesome.

@brandur-stripe should definitely take a peek, but since we're (presumably) always going to send a utf-8 encoded body, hard-coding this where you've got it seems like a reasonable solution to me.

My only concern here is 'will this break folks who don't need this hardcode override', but I'm pretty sure the answer is 'no' given that it passes all the tests, and 'it works on my machine.' ¯\_(ツ)_/¯

@onevcat
Copy link
Contributor Author

onevcat commented Aug 24, 2017

@jlomas-stripe Sure. I will open another p-r for it.

And yes, it might break those ones do not need 'utf8', but I wonder is there a situation they want to use a different encoding? Possibly no.

Another way might be adding an encoding option in the constructEvent API, so we could keep current implementation untouched while give the user (like me) a chance to get rid of struggling with the encoding settings on the server.

Thanks!

@onevcat onevcat closed this Aug 24, 2017
onevcat added a commit to onevcat/stripe-node that referenced this pull request Aug 24, 2017
@onevcat
Copy link
Contributor Author

onevcat commented Aug 24, 2017

Another p-r is opened in #376 , in which only the utf8 changes is contained.

@ob-stripe
Copy link
Contributor

@jlomas-stripe @brandur-stripe FYI, I made a similar change in the Java library a few weeks back: stripe/stripe-java#394.

@brandur-stripe
Copy link
Contributor

Thanks @ob-stripe!

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

Successfully merging this pull request may close these issues.

4 participants