search

thephpleague / oauth2-server

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

Make validateAuthorizationCode protected #1206

Closed NikoGrano closed 3 years ago

NikoGrano commented 3 years ago

By making this method protected, developer can override checks. For example more complex redirect_uri validation checks require you to override huge amount code, when only overriding this function would be enough.

eugene-borovov commented 3 years ago

Duplicate #1200

Sephster commented 3 years ago

@NikoGrano can you give an example of the kind of checks you need? As @eugene-borovov notes, we've been asked about this before and I've been reluctant to allow this as the use cases are normally very specific for the requester and could make the server vulnerable if carried out improperly.

To change this, we'd really need to see a mass use case to warrant such a change

NikoGrano commented 3 years ago

There is usecases where you might allow multiple redirect URIs for same client or URI with changing parts. These cannot be checked using just !==. However, I do disagree with implementation of this PR. Instead of changing this whole funtion from private to protected you might want consider introducing new protected function for URI validation.

So, create new function from these lines which would be protected by default.

Sephster commented 3 years ago

This would introduce a security concern though. The client can and does have multiple redirect URIs registered against it but when a request comes in, it must either specify the redirect URI it wants to use or if absent, we will use the primary default redirect URI. The specs are pretty clear on this aspect which is why it is implemented in this manner.

I will close this PR for now as I don't think this is something we will change in the library in the near future. Thank you very much for your time and effort with this though and sorry we aren't going to pursue it at this time

NikoGrano commented 3 years ago

I dont think its anyways job of this library force to make validation with ! == either. I cannot see security issue with this as its up to the developer to make sure his implementation is secured correctly.

Maintaining fork to have support for RegEx based validation of redirect URIs is pure pain. Afterall, only those who really dive deep into the code will override the function for their needs.

Its still not impossible to override this check with current version, it just causes more and more stuff being overriden and not being updated if the library updates. I think that's even bigger security concern.