Skip to content
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

Validate state before ID Token request #447

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ricklambrechts
Copy link
Contributor

@ricklambrechts ricklambrechts commented Sep 30, 2024

There was always an ID Token request to the idP, before the state was checked.

I think it is better to perform the state check first to prevent flooding the idP with incorrect requests.

Neither the OAuth 2.0 documentation or the OpenID spec says something about checking the state before requesting the ID Token. Only that we should validate it and we do this so, that is not a problem.

Also added a test to check if the properties are set correctly on authenticate and if the code and session matches.
Needed to mock the getState for PHP 7.1, looks to me like something with the session state is not working great in PHP 7.1.

PS: If you want, I can create a seperate function for the state check with the unset state so that we reuse code instead of the same duplicated state check,

PS: for the sometimes failing invalid-bad-iat test, would it be an option to update the 301 value to 302 ? Or just rerun the action?

List of common tasks a pull request require complete

  • Changelog entry is added or the pull request don't alter library's functionality

@ricklambrechts ricklambrechts force-pushed the validate-state-before-token-request branch from e4b82fc to 7ef4516 Compare September 30, 2024 12:15
@ricklambrechts ricklambrechts force-pushed the validate-state-before-token-request branch 2 times, most recently from 8edf8d4 to 2dde092 Compare September 30, 2024 15:10
@ricklambrechts ricklambrechts force-pushed the validate-state-before-token-request branch from 2dde092 to 9e04095 Compare September 30, 2024 15:35
@ricklambrechts ricklambrechts marked this pull request as ready for review September 30, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant