search

thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.51k stars 1.12k forks source link

Checking the URI before matching #1237

Closed eugene-borovov closed 2 years ago

eugene-borovov commented 3 years ago

A redirect URI without scheme causes an error.

boesing commented 3 years ago

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

Spomky commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Thanks @eugene-borovov and @Spomky for clarifying this

eugene-borovov commented 3 years ago

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 commented 3 years ago

[...] , 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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/thephpleague/oauth2-server/issues/1239.