Fix 355 - Don't duplicate ? if qs in redirect url #356
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
Instead of appending a ? or # (implicit), parse the query string and fragment for parameters. Add our parameters and rebuild the redirect uri This should hopfully be more robust to redirect_uri's containing query parametes / fragment parameters One thing that it will not account for is if the fragment doesn't contain a parameter, but a single argument. That would be lost. But I'm not sure if you could legally combine those in a fragment? eg: https://example.com/#anchor¶m=value This also adds tests for AuthorizeError.create_uri and fixes updates the test_missing_nonce test to account for this change.
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.
Instead of appending a ? or # (implicit), parse the query string and fragment for parameters. Add our parameters and rebuild the redirect uri. This also changes
%20to+in query strings (since it was originallyquoteed and now I'm usingurlencode. No double dipping.)This should hopefully be more robust to
redirect_uri'scontaining query parameters / fragment parameters.One thing that it will not account for is if the fragment doesn't contain a parameter, but a single argument. That would be lost. But I'm not sure if you could legally combine those in a fragment? eg:
https://example.com/#anchor¶m=valueThis also adds tests for
AuthorizeError.create_uriand fixes updates the test_missing_nonce test to account for this change.