Check if session key exists before accessing it #251
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I ran into this issue when trying to set up a basic OAuth flow using this library and Keycloak. The issue was introduced by ce97230 which checks for PKCE support, the way it does this is to call the function getCodeVerifier() which in turn calls getSessionKey() which returns a specified key from _SESSION. This will fail when setting up a client without PKCE because the key will not exist. With this commit a sanity check is introduced which first checks if the key exists in _SESSION before returning it, otherwise just returning false. This should not affect existing library functionality. Signed-off-by: Erik Sjöström <[email protected]>
|
Just a heads up, this broke my implementation, due to this line: OpenID-Connect-PHP/src/OpenIDConnectClient.php Line 1029 in 11785b4 $claims didn't have a nonce (which is optional according to the spec) it would be null and thus the check with getNonce() which calls getSessionKey() would also return null and it would all be fine. With this change null !== false.
This change is probably a good one though. I'd suggest we change the line I linked to so it becomes: !isset($claims->nonce) || $claims->nonce === $this->getNonce()Which also brings it inline with the other checks in this method. I'll put up a pull request with that change shortly if you're happy with it, but let me know otherwise 👍 |
|
Hi. Thank you for your report. This is great if you want to provide a patch! Would you consider writing a small test describing your usecase too? |
jenkoian
pushed a commit
to jenkoian/OpenID-Connect-PHP
that referenced
this pull request
Nov 24, 2021
Nonce is optional according to the open ID spec, so we shouldn't throw an exception if a nonce is null (or not set) within the claims. This was working previously but a change to how session keys are checked here: jumbojett#251 meant that the nonce check was now strict and `null !== false`. This fixes by checking first that the nonce is set before checking it matches the nonce in the session.
|
Done: #280 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I ran into this issue when trying to set up a basic OAuth flow using
this library and Keycloak. The issue was introduced by ce97230 which
checks for PKCE support, the way it does this is to call the function
getCodeVerifier() which in turn calls getSessionKey() which returns a
specified key from _SESSION. This will fail when setting up a client
without PKCE because the key will not exist.
With this commit a sanity check is introduced which first checks if the
key exists in _SESSION before returning it, otherwise just returning
false. This should not affect existing library functionality.
Signed-off-by: Erik Sjöström [email protected]