Skip to content

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

Merged
ashfame merged 6 commits into
mainfrom
nonce_id_token_fix
Sep 23, 2022
Merged

Copy over nonce from parsed POST vars to parsed GET vars as a temp fix#36
ashfame merged 6 commits into
mainfrom
nonce_id_token_fix

Conversation

@ashfame

@ashfame ashfame commented Sep 21, 2022

Copy link
Copy Markdown
Member

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 requested review from akirk and psrpinto September 21, 2022 07:48
@ashfame ashfame self-assigned this Sep 21, 2022

@psrpinto psrpinto left a comment

Copy link
Copy Markdown
Member

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!

Comment thread src/Http/Handlers/AuthorizeHandler.php Outdated
Comment thread templates/authenticate/form.php Outdated
@akirk

akirk commented Sep 21, 2022

Copy link
Copy Markdown
Member

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

@ashfame

ashfame commented Sep 22, 2022

Copy link
Copy Markdown
Member Author

@psrpinto @akirk Please take another look! Thanks!

@ashfame ashfame requested a review from psrpinto September 22, 2022 05:53

@psrpinto psrpinto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@ashfame

ashfame commented Sep 23, 2022

Copy link
Copy Markdown
Member Author

@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