mirror of
https://github.com/OAuthSwift/OAuthSwift.git
synced 2026-04-26 12:45:52 +03:00
[GH-ISSUE #401] Double encoded callback url causes issues #258
Labels
No labels
bug
cocoapod
duplicate
enhancement
feature-request
help wanted
help wanted
invalid
pull-request
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/OAuthSwift#258
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @iby on GitHub (Aug 14, 2017).
Original GitHub issue: https://github.com/OAuthSwift/OAuthSwift/issues/401
Description:
I've been using the framework successfully for awhile, but recently it stoped working with Imgur with
The redirect URI provided is missing or does not matcherror. The issue doesn't seem to be related to any particular changes in the framework, but rather to Imgur backend updates. However, oauth works with Postman and Paw, which leads to certain conclusions.Digging deeper the issue seems to be related to double encoded callback url in
OAuth2Swift::authorizemethod. Removing double encoding solves the issue. I believe services take this into account and un-encode the url, which is good and bad, as it makes it easier to work with, but introduces uncertainty.queryStringcomposed inOAuth2Swift::authorizecombines every parameter as is with exception for callback url, which gets encoded. The composed query then gets encoded, which causes url to be double encoded. If query would be composed with every parameter encoded immediately and without re-encoding everything, it would produce a valid query and eliminate the issue.P. S. I see that logic has changed recently, however, the problem occurs in the following
urlQueryEncodedinvocation.OAuth Provider (Twitter, Github, ..):
OAuth Version:
OS (Please fill the version) :
Installation method:
Library version:
Xcode version:
@phimage commented on GitHub (Aug 14, 2017):
someask ask for callback url encoding PR #325
e56fafadc8I make it optional using encodeCallbackURL boolean, issue #339
by default there is no encoding of callback url
@iby commented on GitHub (Aug 14, 2017):
I know. I've seen those. This is great, but not the case. If it's set to true url gets encoded twice, first here, then here. Frankly, if url is not encoded, it doesn't get encoded the second time. Like #339 describes –
slashes ( / ) are being replaced with %252F– when encoded, they should become %2F, but % gets encoded twice, producing %252F.@iby commented on GitHub (Aug 14, 2017):
Happy to make a PR.
@phimage commented on GitHub (Aug 14, 2017):
If you do not encode twice, how the server could decode and know to what url query parameters belong to
If we take this query
x=5&callbackurl=http://toto?y=8&z=4
the z is for callbackurl or main url
If we encode the first time using encodeCallbackURL =true
x=5&callbackurl=http%3A%2F%2Ftoto%3Fy%3D8%26z%3D4
then reencode (callback is encoded two times)
and then the server decode - I think server could know that z is for callbackurl
some server expect callbackurl as last query parameters...
It's difficult to make compatible with a lot of providers because of weird choice...
If you are able to provide a code that work for your provider, please PR.
But If we can keep the current two workflow...encodeCallbackURL=true and encodeCallbackURL=false
Maybe have an another dirty boolean, or make an enum with 3 cases (keep the boolean as compatiblity with deprecated and remapped to the enum)
Maybe I miss something
I must code, make unit test with URL to fully understand, try with some providers to validate that all are compatibles
@iby commented on GitHub (Aug 14, 2017):
No, you are right. I thought of it in the first place, but lost it out of sight with other stuff in mind. But like you say, if we don't encode it at all, the same thing happens – query parameters might get mixed. I'm mostly bummed out by the fact that it used to work… I'll look into this.
@iby commented on GitHub (Aug 15, 2017):
Also tested with my project, everything seems to work! 👌