fix(oauth): sort parameters in a standard way as per the specs #721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the signature is now calculated a bit differently to match other implementations. BREAKING CHANGE: existing OAuth applications may encounter some issues leading to `invalid_signature` 401 errors from Magento due to the removal of natural sorting for parameters when generating the signature
colinmollenhour
approved these changes
Jun 13, 2019
|
Thanks for the feedback. |
|
Yes, thus my approval. :) It is hard to say if anyone is relying on this incorrect behavior but as this hasn't been reported yet I would say the chances are very slim and it is better to fix it and make it correct than leave it incorrect. |
|
Awesome. Thanks for your quick feedback! |
|
Hi! It's been a few months since @colinmollenhour's approval. Would it be possible to have a 2nd review for this PR please? It is a blocking issue for headless implementations of M1 (that has to be patched in userland code for now) (ping @sreichel @Flyingmana) |
Flyingmana
approved these changes
Nov 3, 2019
|
Thank you very much! 🎉 |
edannenberg
pushed a commit
to edannenberg/magento-lts
that referenced
this pull request
Aug 20, 2020
BREAKING CHANGE: existing OAuth applications may encounter some issues leading to `invalid_signature` 401 errors from Magento due to the removal of natural sorting for parameters when generating the signature
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi folks!
We have detected what seems to be a flaw in the way the OAuth signature is calculated to ensure the received request is correct.
From our understanding it seems to be a bug, but I’d really appreciate if someone more used to this standard could have a look :)
The symptom is: 401 errors with an
invalid_signatureerror for requests containing numerical indexes.Example:
Given the following params:
limit=>21filter[0][attribute]=>skufilter[0][in][0]=>msj000filter[0][in][1]=>msj001filter[0][in][2]=>msj002filter[0][in][3]=>msj000xsfilter[0][in][4]=>msj000xlfilter[0][in][5]=>msj003xsfilter[0][in][6]=>msj003xlfilter[0][in][7]=>msj006filter[0][in][8]=>msj007filter[0][in][9]=>msj008filter[0][in][10]=>msj006xlfilter[0][in][11]=>msj006xsMagento will use
natsortto sort them before generating the signature. The order will thus be:filter[0][attribute]=>skufilter[0][in][0]=>msj000filter[0][in][1]=>msj001filter[0][in][2]=>msj002filter[0][in][3]=>msj000xsfilter[0][in][4]=>msj000xlfilter[0][in][5]=>msj003xsfilter[0][in][6]=>msj003xlfilter[0][in][7]=>msj006filter[0][in][8]=>msj007filter[0][in][9]=>msj008filter[0][in][10]=>msj006xl<------filter[0][in][11]=>msj006xs<------limit=>21The library we use in nodeJS uses a standard sort (see https://github.com/ddo/oauth-1.0a/blob/d2ec1e2820ff60e99da4444deae630d8eac31d91/oauth-1.0a.js#L366) and will generate a signature with the following order:
filter[0][attribute]=>skufilter[0][in][0]=>msj000filter[0][in][1]=>msj001filter[0][in][10]=>msj006xl<-------filter[0][in][11]=>msj006xs<-------filter[0][in][2]=>msj002filter[0][in][3]=>msj000xsfilter[0][in][4]=>msj000xlfilter[0][in][5]=>msj003xsfilter[0][in][6]=>msj003xlfilter[0][in][7]=>msj006filter[0][in][8]=>msj007filter[0][in][9]=>msj008limit=>21As a consequence, signatures will not match…
Resources
Our researches led us to the OAuth spec which says:
From our understanding it seems that the Zend_Oauth is the wrong implementation…
Also, it seems that other implementations such as https://github.com/thephpleague/oauth1-client/blob/master/src/Client/Signature/HmacSha1Signature.php#L73 are also sorting parameters as the node library linked above.
Breaking change
This IS a breaking change, also I am not sure whether you think it could be merged or not in its current state.
Tests
This is my first contribution here, so I am relying on the CI process to see if it breaks things. I could add tests later if you think it is necessary.
Please let me know if you need further information.