Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

AccessTokenRepository - Valid tokenid handling not present in oauth_access_tokens #70

Closed
andersonluciano opened this issue Nov 26, 2019 · 3 comments
Closed

AccessTokenRepository - Valid tokenid handling not present in oauth_access_tokens #70

andersonluciano opened this issue Nov 26, 2019 · 3 comments
Labels
bug
Milestone
1.2.1

Comments

@andersonluciano
Copy link

Generate a tokenId at "/oauth2/access_token"

'grant_type' => 'client_credentials',
'client_id' => 'client_test',
'client_secret' => 'test',
'scope' => 'test',

Delete the id register from oauth_access_tokens, or recreate the database

Call some resource that will be validated by oAuth, passing the tokenId generated before

it will throw

TypeError raised in file /vendor/zendframework/zend-expressive-authentication-oauth2/src/Repository/Pdo/AccessTokenRepository.php line 118:
Message: array_key_exists() expects parameter 2 to be array, boolean given

I'd paste the isAccessTokenRevoked function with stating return value incorrectly assumes $row is an array, in the case entity doesn't exist in database should return true or throw an exception
@michalbundyra michalbundyra added this to the 1.2.1 milestone Nov 26, 2019
@Xerkus
Copy link
Member

Xerkus commented Nov 26, 2019

zend-expressive-authentication-oauth2/src/Repository/Pdo/AccessTokenRepository.php

Lines 103 to 119 in dd8a7ce

/**
* {@inheritDoc}
*/
public function isAccessTokenRevoked($tokenId)
{
$sth = $this->pdo->prepare(
'SELECT revoked FROM oauth_access_tokens WHERE id = :tokenId'
);
$sth->bindParam(':tokenId', $tokenId);
if (false === $sth->execute()) {
return false;
}
$row = $sth->fetch();
return array_key_exists('revoked', $row) ? (bool) $row['revoked'] : false;
}

Couple points:

  • Failed check for query error if (false === $sth->execute()) should result in exception.
  • Check if token was found is missing and looks like it was confused with query error check.
    • Check if token is found was previously accidentally implemented by array_key_exists('revoked', $row) and it broke after strict types declaration was added. No security issue was introduced.
    • This case is obviously missing test coverage
  • Should missing token result in "token not revoked" (return false), "token revoked" (return true) or exception being thown? Existing code implies false.
    I think "token not revoked" is not a safe result here from security perspective and I don't know if upstream library would like exception for this condition.

@andersonluciano andersonluciano mentioned this issue Nov 26, 2019
Closed
17 tasks
@Xerkus
Copy link
Member

Xerkus commented Nov 26, 2019

After a bit of refresher on league lib:
It uses JWT, so token is validated and expiry time is checked before revocation is checked.
This is an edge case. Missing token condition can be reached if table is purged, some kind of split brain partition or replication delay happened.
Both "token revoked" and exception would work here.

@michalbundyra
Copy link
Member

Solved in #71

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
1.2.1
Development

No branches or pull requests
3 participants
@Xerkus @andersonluciano @michalbundyra