Skip to content

CachedKeySet does not check HTTP response status code #499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
croensch opened this issue Apr 14, 2023 · 3 comments · Fixed by #508
Closed

CachedKeySet does not check HTTP response status code #499

croensch opened this issue Apr 14, 2023 · 3 comments · Fixed by #508

Comments

@croensch
Copy link

croensch commented Apr 14, 2023

The CachedKeySet uses an PSR-18 HTTP client, sending a request and immediately decoding the answer - regardless of the response status code.

I tested a couple of failure scenarios around the connection but a working connection that produces an error is not checked. It leads to the UnexpectedValueException('"keys" member must exist in the JWK Set'). I propose a new UnexpectedValueException("HTTP ...").

@croensch
Copy link
Author

A Client MUST NOT treat a well-formed HTTP request or HTTP response as an error condition. For example, response status codes in the 400 and 500 range MUST NOT cause an exception and MUST be returned to the Calling Library as normal.
https://www.php-fig.org/psr/psr-18/#error-handling

@bshaffer
Copy link
Collaborator

Thank you for this! I was working off my understanding of Guzzle, which throws exceptions by default.

@bshaffer
Copy link
Collaborator

So the suggested change would be here

https://github.com/firebase/php-jwt/blob/e94e7353302b0c11ec3cfff7180cd0b1743975d2/src/CachedKeySet.php#L179-L181C49

This could be changed to something like:

$request = $this->httpFactory->createRequest('GET', $this->jwksUri);
$jwksResponse = $this->httpClient->sendRequest($request);
if ($jwksResponse->getStatusCode() !== 200) {
    throw new UnexpectedValueException((string) $jwksResponse->getBody(), $jwksResponse->getStatusCode());
}
$this->keySet = $this->formatJwksForCache((string) $jwksResponse->getBody());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants