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

Non-URL redirection URIs fail validation (e.g., for native apps with custom schemes) #1273

Closed
bradjones1 opened this issue Mar 16, 2022 · 4 comments · Fixed by #1274
Closed

Comments

@bradjones1
Copy link
Contributor

I have a native app which performs OAuth2 with, among other clients, a native app.

The redirection URI is something like native.app.reverse.dns://, which is a valid URI but is not a URL.

Spec references:
https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2
https://datatracker.ietf.org/doc/html/rfc3986#section-4.3

The culprit is parse_url(), which as the name implies is specific to URLs. Further FWIW the PHP docs say:

This function is not meant to validate the given URL, it only breaks it up into the parts listed below...

Which is more or less what we're doing here.

To be truly spec-compliant, we must allow and match against URIs, not just the subset which are URLs.

This being a League package, good thing there's https://github.com/thephpleague/uri, which advertises URI validation and manipulation. Let's use that.

Also this affects the Drupal implementation, since Drupal core assumes URIs are URLs, despite the various interfaces being called UriWhatever.

@bradjones1
Copy link
Contributor Author

Potentially related #1039

@bradjones1
Copy link
Contributor Author

Refs #1188 (comment), though that and its related PR still use parse_url() which I think is incorrect.

@Sephster
Copy link
Member

Thanks @bradjones1 and thanks for the PR. I will take a look at this tomorrow

@victorbalssa
Copy link

Hello,
same problem here.
I have a native application with scheme:// as redirect_uri,
Thank you @bradjones1 💪🏻

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

Successfully merging a pull request may close this issue.

3 participants