Checking the URI before matching

[eugene-borovov]

A redirect URI without scheme causes an error.

[boesing]

Oh, haven't seen this before raising my issue.
This relates to #1239

[Spomky]

URNs shall be accepted too. See example at https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-app-registration

The URN urn:ietf:wg:oauth:2.0:oob won't pass this validation.

[eugene-borovov]

I fixed the isLoopbackUri check and remove scheme restrictions. So URNs and private URI schemes and redirect path should pass. The host and/or path are required for the correct URL.

[eugene-borovov]

https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2

The redirection endpoint URI MUST be an absolute URI as defined by
[RFC3986] Section 4.3. The endpoint URI MAY include an
"application/x-www-form-urlencoded" formatted (per Appendix B) query
component ([RFC3986] Section 3.4), which MUST be retained when adding
additional query parameters. The endpoint URI MUST NOT include a
fragment component.

So scheme is required.

[Sephster]

I see Microsoft mentioning urns but don't see anything in the official specs about urn support. Just custom app schemes. We always follow the official RFCs so I'm not concerned with urns at this time but if someone can find info in an oauth RFC that specifies we should support urns, we will of course include this.

[Spomky]

Actually, URNs are more likely used for native apps.
See also the Google Documentation at https://developers.google.com/youtube/v3/live/guides/auth/installed-apps

[eugene-borovov]

https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2

The redirection endpoint URI MUST be an absolute URI as defined by [RFC3986] Section 4.3.

https://datatracker.ietf.org/doc/html/rfc3986#section-1.1.3

A URI can be further classified as a locator, a name, or both. The term "Uniform Resource Locator" (URL) refers to the subset of URIs that, in addition to identifying a resource, provide a means of locating the resource by describing its primary access mechanism (e.g., its network "location"). The term "Uniform Resource Name" (URN) has been used historically to refer to both URIs under the "urn" scheme [RFC2141], which are required to remain globally unique and persistent even when the resource ceases to exist or becomes unavailable, and to any other URI with the properties of a name.

https://datatracker.ietf.org/doc/html/rfc3986#section-3

The following are two example URIs and their component parts:

     foo://example.com:8042/over/there?name=ferret#nose
     \_/   \______________/\_________/ \_________/ \__/
      |           |            |            |        |
   scheme     authority       path        query   fragment
      |   _____________________|__
     / \ /                        \
     urn:example:animal:ferret:nose

So URNs can also be redirect URIs.

[Sephster]

I'm not sure this is more common but would happily be proved wrong. As I said before though, I would really want to see something about URNs in the Oauth specs before committing to this. If it is an official implementation, recommended by the IETF then we should support this. Is there any spec out there specifying this?

[Sephster]

Thanks @eugene-borovov and @Spomky for clarifying this

[eugene-borovov]

By the way, the main purpose of this PR is to fix the isLoopbackUri method and add some verification for redirect URI. The URN discussion is a side effect of the too strict validation rules that I gave initially.

[Spomky]

[...] , I would really want to see something about URNs in the Oauth specs before committing to this.

The RFC6749 spec always refers to URIs. URIs include URLs and URNs so from my POV there is no reason to exclude them.
https://en.wikipedia.org/wiki/Uniform_Resource_Identifier

[Sephster]

Thank you for this PR @eugene-borovov and I'm sorry I didn't get a chance to push this through. Since this was submitted a newer PR was proposed (#1274) which I think provides a more elegant solution as it relies on the league/uri package to handle parsing. For this reason, I'm going to close this PR but wanted to thank you for trying to resolve the issue.

[eugene-borovov]

@Sephster #1274 resolves only loopback URI. This PR resolves all redirect URI validation. These are different branches of the validate algorithm. This PR allows to use URN as redirect URI in addition.

[Sephster]

thanks for the quick reply @eugene-borovov. I added in some further tests to main, based on yours, to ensure we are able to accept custom schemes and urns. I think that the solution that has been merged does ensure all valid uris are accepted because it is using the league/uri package. If an invalid uri is passed (poorly formed rather than not pre-registered), the library will throw an exception. Thank you again for your efforts here and sorry I didn't merge it this time.

[eugene-borovov]

@Sephster I`d suggested to use URI validation in constructor. So on loopback analyze step you have valid URI and test only the host. Also this prevents using no-URI strings as redirect_uri as in the issue #1239.