-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
RFC8252#section-7.3 Loopback Interface Redirection #400
Conversation
authorize_helper_test.go
Outdated
expected: "http://127.0.0.1:1024", | ||
}, | ||
{ | ||
client: &DefaultClient{RedirectURIs: []string{"http://127.0.0.1"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this should fail because the whitelisted redirect URL does not have the /cb
path segment. The spec only talks about randomly assigned ports but does not leave room for variable paths, if I understand correctly. Otherwise, this looks pretty good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds right. I had to change some bits because of that. Using url parse now to extract the paths and compare them.
You rock, thank you for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last change :)
authorize_helper.go
Outdated
// Loopback redirect URIs use the "http" scheme and are constructed with | ||
// the loopback IP literal and whatever port the client is listening on. | ||
func isMatchingRedirectURI(uri string, haystack []string) bool { | ||
requested, err := url.Parse(strings.ToLower(uri)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are now using url.Parse, toLower is no longer required and actually might cause issues for servers that have case sensitive paths. I think toLower should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requested, err := url.Parse(strings.ToLower(uri)) | |
requested, err := url.Parse(uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fixed it. (Had to add another fix, with the fix above test 8 would have failed).
Thanks for merging!
Thanks, same to you! |
I released a new tag for this so this can now be added to hydra |
This implements support for a dynamic port request from an oauth2 client.
RFC reference: https://tools.ietf.org/html/rfc8252#section-7.3
Related issue
#284
ory/hydra#1732
Proposed changes
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments
The RFC (https://tools.ietf.org/html/rfc8252#section-7.3) states an URI like
http://127.0.0.1:{port}/{path}
andhttp://[::1]:{port}/{path}
. I am not 100% sure if this is a must or just a regex example.I have implemented this in fosil that it is now possible to register:
... and if a registered client has one or both of these redirect uri's a client may request a dynamic port onto the loopback interface. For example http://127.0.0.1:9988 or with a path http://127.0.0.1:9988/callback. Note as with this PR, if both IPv4 and IPv6 should be supported for redirect loopback uris the clients needs to be registered with both.
If the RFC requests that the redirect URI must exactly be
http://127.0.0.1:{port}/{path}
then more code needs to be changed, for example the client registration in hydra would also fail with this (invalid) uri.