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

Validating redirect_uri via ValidateURIHandler is a bit weird #267

Open
asiraky opened this issue May 5, 2024 · 0 comments
Open

Validating redirect_uri via ValidateURIHandler is a bit weird #267

asiraky opened this issue May 5, 2024 · 0 comments

Comments

@asiraky
Copy link
Contributor

asiraky commented May 5, 2024

3 issues relating to the redirect_uri:

  1. When an invalid redirect_uri is detected, the framework is still redirecting to the invalid uri with the error message in the url. The RFC states:
Invalid Endpoint

   If an authorization request fails validation due to a missing,
   invalid, or mismatching redirection URI, the authorization server
   SHOULD inform the resource owner of the error and MUST NOT
   automatically redirect the user-agent to the invalid redirection URI.
  1. I think it's kind of weird that the framework wants you to opt in / define a compliant redirect_uri check yourself (one that compares the scheme, authority and path), when the RFC states that the redirect_uri's must be validated against a whitelist when dealing with public clients or when using implicit grant, and nevertheless should be validated against a whitelist for everyone. Given most oauth providers are more often than not dealing with public clients and the fairly strong recommendation to perform validation in any case, why not make it an opt out feature? I may not even have noticed it if I didn't go out of my way to test for it. Some other devs may not even test for it and just assume the framework is implementing standard oauth behaviour.

  2. I believe the callback should not be limited to the pair of redirect_uri's for comparison. What if I want to register multiple uri's? In the context of the callback, I have no way of accessing the already retrieved Client model - which is likely where I would store the multiple uri's. I am willing to make a PR to change the two places where this is called:

if err := m.validateURI(cli.GetDomain(), tgr.RedirectURI); err != nil {

would change to

if err := m.validateURI(cli, tgr.RedirectURI); err != nil {

Thanks

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

No branches or pull requests

1 participant