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

Validate expected type of input parameter #1210

Closed marc-mabe closed 3 years ago

marc-mabe commented 3 years ago

fixes #1209

marc-mabe commented 3 years ago

Any update on this? Do you need something from me?

Sephster commented 3 years ago

Thanks @marc-mabe - looks good. We could maybe change this at a later date to have a generic catching of Error classes but this looks to solve the issue at hand. Thanks for submitting this.

JorgenSolli commented 2 years ago

I know this is very late, but I just updated my dependencies only to have my login system break.

Is there a good reason for assuming a username is a string? My application has a login layer that ultimately logs the user in using their user IDs. I'm now typecasting it to string in order to work around this, but I'm still left with the question of why this was introduced.

Obviously hard to think of every scenario possible, but 8.3 actually ended up containing a breaking change. Food for thought I guess.

Sephster commented 2 years ago

I honestly didn't think this would have caused any breaking change. Sorry that this happened for you. The username is retrieved from the request using the getRequestParameter() function which returns either a string or null if the value isn't present. This function is calling the getParsedBody() function from the HTTP Request wrapper. As far as I can tell, this should always return a key value pair of string => string so I didn't think this would cause any issue.

Were you already changing the value to be an int somewhere? Sorry again for any inconvenience caused

JorgenSolli commented 2 years ago

No need to apologize. Shit happens 😄

I didn't dig too deep here, but I can share some code if you're interested in what exactly happened. Note that this is using Laravel (v8) and its Passport (v10) package, which depends on this package.

My implementation looks like this:

$data = [
    'grant_type' => 'password',
    'client_id' => $request->get('client_id'),
    'client_secret' => $request->get('client_secret'),
    'username' => "$user->id", // Needs to be a string lol
    'password' => $request->get('password'),
];

try {
    $loginRequest = Request::create('/oauth/token', 'POST', $data, [], [], $server);
    $loginRequest->headers->set('host', $request->getHttpHost());
    $response = app()->handle($loginRequest);
} ...

This implementation is somewhat unorthodox, I know.

The stacktrace:

[previous exception] [object] (League\\OAuth2\\Server\\Exception\\OAuthServerException(code: 3): The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. at /application/vendor/league/oauth2-server/src/Exception/OAuthServerException.php:142)
[stacktrace]
#0 /application/vendor/league/oauth2-server/src/Grant/PasswordGrant.php(90): League\\OAuth2\\Server\\Exception\\OAuthServerException::invalidRequest('username')
#1 /application/vendor/league/oauth2-server/src/Grant/PasswordGrant.php(56): League\\OAuth2\\Server\\Grant\\PasswordGrant->validateUser(Object(Nyholm\\Psr7\\ServerRequest), Object(Laravel\\Passport\\Bridge\\Client))
#2 /application/vendor/league/oauth2-server/src/AuthorizationServer.php(204): League\\OAuth2\\Server\\Grant\\PasswordGrant->respondToAccessTokenRequest(Object(Nyholm\\Psr7\\ServerRequest), Object(League\\OAuth2\\Server\\ResponseTypes\\BearerTokenResponse), Object(DateInterval))
#3 /application/vendor/laravel/passport/src/Http/Controllers/AccessTokenController.php(65): League\\OAuth2\\Server\\AuthorizationServer->respondToAccessTokenRequest(Object(Nyholm\\Psr7\\ServerRequest), Object(Nyholm\\Psr7\\Response))
#4 /application/vendor/laravel/passport/src/Http/Controllers/HandlesOAuthErrors.php(24): Laravel\\Passport\\Http\\Controllers\\AccessTokenController->Laravel\\Passport\\Http\\Controllers\\{closure}()

Not sure if this helps you understand what and why this happened, but this is what I managed to scrape together before bedtime.