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

Copy over nonce from parsed POST vars to parsed GET vars as a temp fix #36

Merged
merged 6 commits into from
Sep 23, 2022

Conversation

ashfame
Copy link
Member

@ashfame ashfame commented Sep 21, 2022

Currently bshaffer oauth library has a bug when POST is used on AuthorizeEndpoint along with nonce (optional parameter) which fails to set the nonce in id_token

This PR switches the form submit method to GET when taking user consent and this fixes the issue.

Fixes #35

The reason why this issue would only break for new users is because of sticky consent functionality the parameters would get passed as GET parameters, hence not uncovering the issue with bshaffer oauth library.

Additionally, I have also already created a PR to the library with the fix.

Currently bshaffer oauth library has a bug when POST is used on AuthorizeEndpoint along with nonce
(optional parameter) which fails to set the nonce in id_token
@ashfame ashfame self-assigned this Sep 21, 2022
Copy link
Member

@psrpinto psrpinto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for also opening a PR to the library!

src/Http/Handlers/AuthorizeHandler.php Outdated Show resolved Hide resolved
templates/authenticate/form.php Outdated Show resolved Hide resolved
@akirk
Copy link
Member

akirk commented Sep 21, 2022

Could we also work around the library issue by just doing a $_GET['nonce'] = $_REQUEST['nonce'] before invoking the library?

@ashfame
Copy link
Member Author

ashfame commented Sep 22, 2022

@psrpinto @akirk Please take another look! Thanks!

Copy link
Member

@psrpinto psrpinto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ashfame
Copy link
Member Author

ashfame commented Sep 23, 2022

@akirk Going to merge this but happy to address any remarks, if there are any, later on.

@ashfame ashfame merged commit e55e4e0 into main Sep 23, 2022
@ashfame ashfame deleted the nonce_id_token_fix branch September 23, 2022 04:36
@ashfame ashfame changed the title switch to GET method when taking user consent Copy over nonce from parsed POST vars to parsed GET vars as a temp fix Sep 23, 2022
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.

nonce missing in id_token at first SSO attempt
3 participants