Oauth2-server: Cannot use the client credentials without any redirect URI

[magnetik]

AFAIK when using only the client credentials grant, one can use a client without any redirect URI (as it's not used anywhere in this grant).

Change introduced in #1140 makes it impossible to use such client.

[michanismus]

Same here, very annoying. Downgraded to 8.1.1 to keep my app running...

[paul121]

Rather than requiring a redirect_uri for ALL authorization flows, maybe it should only be required when attempting the Authorization Code flow? And only throw the error for that case?

https://tools.ietf.org/html/rfc6749#section-3.1.2.4

[Sephster]

If a client is registered with the server, there isn't anything native in the package to stop it from using the Authorization Grant which does use the redirect_uri.

The OAuth 2 spec states that when a client registers, it must indicate what type of client it is and give its redirect uris.

Oauth.com goes into more detail about this saying the following:

It is, however, important that you require the developer to register one or more redirect URLs for the application for security purposes.

For these reasons, I believe that allowing clients to register without a redirect_uri wasn't conforming to the spec which is why I made this change.

Apologies if this has caused any inconvenience for you but I believe this is the correct way that the server should be handling client registration.

[michanismus]

https://tools.ietf.org/html/rfc6749#section-4.3

The redirect_uri is useless for this grant type. It would be easy to check the type and test for redirect_uri when it is mandatory.

[Sephster]

It is mandatory regardless of the grant type you are using. We _could_ add in a check but this would go against spec by allowing clients without redirect_uris and I don't believe we should go down this route.

[Spomky]

Hi all,

My 2 cents on this discussion.

The OAuth 2 spec states that when a client registers, it must indicate what type of client it is and give its redirect uris.

It is mandatory regardless of the grant type you are using.

I agree with the fact that the redirect uris are a critical part of the flows.
There are several known attacks that use redirect uris.

But I disagree with you when you write there are mandatory. The section 3.1.2.2 of the specification is clear on that point:

The authorization server SHOULD require all clients to register their
redirection endpoint prior to utilizing the authorization endpoint.

SHOULD, not SHALL or MUST. And it is needed only when hitting the authorization endpoint (the CC flow only needs the token endpoint).
Moreover, it is pointless to register a redirect uri and require it during the flow when it is not used at all.

At last but not least, the new flows introduced since October 2012 do not require any redirections. There are namely the:

• SAML Assertion Grant (RFC7521 and RFC7522)
• JWT Assertion Grant (RFC7521 and RFC7523)
• Device Grant (RFC 8626)

With that being said, I understand that it is not easy to update the library without BC breaks. But from my POV it is a necessary step if you want to be compliant with the specs, want to implement these new grant type flows and smoothly move to OAuth2.1.

[Sephster]

Thanks for your comprehensive response and apologies if I misled. The upcoming oauth 2.1 spec uses SHALL instead of SHOULD. It has been considered best practice to mandate the redirect uri for some time now which is why the oauth site also says it is mandatory. This is the primary reason for my decision.

As an aside, what are the implications for you as developers? What do you need to change to accommodate this? Apologies again for the inconvenience caused

[webclaus]

Hello,

As an aside, what are the implications for you as developers? What do you need to change to accommodate this?

I can give you the simplest example of a scenario from our shop:

We run a SaaS to which our customers can connect their own website using the auth code grant and pull in various content from their account with us. They all use the same client and we don't validate any secret as they have to log in with us before getting back an auth code.

The redirect uri is different for each customer as she needs to be redirected back to her own site to complete the flow, and since the provided redirect uri must match the one registered with the client and it now must exist, we can't support this flow any more :(

[Sephster]

Thanks for the reply @webclaus. I'm sorry this has caused you issues. Your scenario doesn't seem to be a typical setup so is a bit of a head scratcher for me.

Usually the customers website would be acting as the client but it sounds like you have some kind of proxy client if I am understanding correctly?

In this case, when you add a new customer, you should add the redirect URI of the customers website. The risk of not having any listed is that there could be a possibility responses aren't redirected to valid clients which is a security concern.

The downfall of having a single client for multiple customers websites is also that someone could try and redirect a response for one customer to a different customer.

[webclaus]

One example is a WordPress plugin for our service. The flow is as follows:

• Customer installs plugin and hits a button in the admin pages that creates a url for our auth endpoint and initiate the oauth flow.
• Customer logs into our service to access the url she is redirected to (unless already signed in) and grant access to WP.
• Customer gets an auth code and is redirected back to WP where we complete the flow and exchange the auth code for a token.
• Now the WP-site can grab data from the customer's account with us to display on the site.

Using the state parameter should ensure that the redirect uri is not abused to redirect folks elsewhere and regardless, the customer can never authorize an account she does not have access to with us.

Our customers may use this setup to connect multiple websites to the same account with us (wordpress or other types) and asking the customer to register an oauth client for each seems a non-starter.

[bradjones1]

Co-maintainer of the Drupal Simple OAuth module here. This change in 8.2 introduced an uncaught BC break in our module on dependency update; not a huge issue but will be rolling a release soon to pin to ^8.0 < 8.2 in order to preserve the SHOULD behaviour, and the next release can include a note for BC breaking change that this is now a SHALL.

See https://www.drupal.org/project/simple_oauth/issues/3185673#comment-13927649

I'm happy to coordinate in the future with this package's maintainers on similar changes so we stay in sync. Thanks for all the great work on this.

[Sephster]

Thanks for your input @bradjones1. Do you need php 8 support as this is only available for 8.2.

I think because this is causing so many issues, I'm going to revert and move this to the next major version.

I was 50/50 about whether to class this as a BC or a patch because we are fixing a security hole but looks like this is affecting enough people to hold off until a major release. I should have the patch out by the end of the week

[bradjones1]

At the moment the Drupal implementation doesn't require PHP 8.

I think since the spec says SHOULD and this changes it to SHALL, it's more BC break than "security hole," in so far as it's technically still 2.0 compliant, even if it's not a "good idea" to proceed without a redirect URI. That said, rolling a release now to address this on our side though that won't stop users from stumbling upon this if they only update dependencies. Up to you.

[webclaus]

I'll second the point that this seems to be breaking BC - a mere composer update trashed our tests.

So all for postponing the change to a major release....and when that rolls around I'll be the first to bring a PR for an allowEmptyReturnUri() method ;)

[Sephster]

Although this is being changed in 2.1 when that RFC is finalised, it has been common practice for some time now to enforce the redirect URI as mentioned in RFC 6819.

Again it uses the word _should_ but it is much more explicit about the dangers of not doing this. I will push this to v9 for now as the server has been like this for some time and don't want to cause unnecessary issues for people. Thanks very much to @webclaus, @bradjones1 and everyone else for their input.

[magnetik]

It looks like a good way to handle this 👍

Ton of thanks for the lib and your dedication ❤️