-
Notifications
You must be signed in to change notification settings - Fork 104
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
feat: make nonce handling configurable #111
base: master
Are you sure you want to change the base?
Conversation
OmniAuth and its plugins are Rack middleware and do not require Rails. I see that there are some rails specific extensions being used in this PR which will break when Rails is not available. |
@btalbot Thanks for pointing that out. I'll look into ways to interact with cookies without the rails intermediate layer. But as the rails dependency is only required in testing would it be fine to keep it as dev dependency only? |
Seems like the best way to ensure that rails extensions are not present is to not include them in any dependency; otherwise, how can you be sure? |
@bvogel thanks for your work on this. spent 2 hours trying to debug this issue and finally found this. I hope this gets merged 🙏 |
works for me! currently I see no way to effectively use the gem without this addition |
@@ -105,7 +123,7 @@ def verify_claims!(id_token) | |||
verify_aud!(id_token) | |||
verify_iat!(id_token) | |||
verify_exp!(id_token) | |||
verify_nonce!(id_token) if id_token[:nonce_supported] | |||
verify_nonce!(id_token) if id_token[:nonce_supported] && options[:nonce] != :ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, have you considered applying the suggestion made by @btalbot in #102 and just avoid verifying a blank nonce
?
verify_nonce!(id_token) if id_token[:nonce_supported] && options[:nonce] != :ignore | |
verify_nonce!(id_token) if id_token[:nonce_supported] && id_token[:nonce].present? |
If so then may I ask why you decided not to use this solution ? It seems to work for me and requires a lot less changes in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this would pass even if nonce
is required but nor returned, which I would consider a security risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, what I meant was to update verify_nonce
like this
def verify_nonce!(id_token)
invalid_claim! :nonce unless id_token[:nonce] == stored_nonce
end
From my understanding stored_nonce
will always be defined when the nonce is required and will not be defined when it's not so only checking that the token's nonce equals the stored_nonce would result in either checking whether "nonce is optional" or "nonce is required and it matches", wouldn't it ?
This PR will introduce a individual handling of the nonce validation that is significantly hindered by Apple with using a POST callback.
Added specs, README too.
fixes #102 and
fixes #103
Just reopening #107 with an additional fix.
See all discussion over there.