-
Notifications
You must be signed in to change notification settings - Fork 100
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
nonce is optional in callback #102
Comments
In our use case, the iOS developers choose to use the hybrid client-server flow where the client never makes the 'authorize' phase request but instead makes the request using iOS SDK libraries. The app then submits the code to the callback directly. In this flow there is no state and no nonce in the id_token. |
when nonce is not supported, it shouldn't be verified. |
maybe your iOS app is just missing to pass the nonce in the request. |
I'm not the iOS developer but they claim that there is no place to pass one ... and where would they get the nonce from? |
The issue is that in 1.3, it was changed so that: when the nonce is not in the session and not in the id-token it's an error |
when the nonce is not in the session and not in the id-token, this should be true. id_token[:nonce] == stored_nonce |
If that were the only logic then my use case would work and the new spec test would pass; however, that is not what the current implementation is. The current implementation FIRST requires that the id_token[:nonce] value be truthy. https://github.com/nhosoya/omniauth-apple/blob/master/lib/omniauth/strategies/apple.rb#L127-L129 def verify_nonce!(id_token)
invalid_claim! :nonce unless id_token[:nonce] && id_token[:nonce] == stored_nonce
end |
ah, then you need nonce. |
I fixed this by instead of setting the whole session to be same_site: :none through a short lived cookie edit: added a PR for this option as well |
According to my research, the Complicating matters, the callback attempts to verify the Unless there is a way to correct the 'nonce_supported' field, I don't believe the nonce should be validated if it is not present. |
Well, the The probable rejection to that PR is most likely the added |
I believe this to be correct as well. I'm not an iOS developer, but this is what our frontend devs confirmed to me. I don't have any documentation for this though. My guess is that these maintainers only use web REST API and never use the native ios calls so they can't understand why a nonce might be missing. So, two issues.
|
the suggested fix doesn't work for iOS native apps, the method |
In releases before 1.3, the nonce appearing in the id_token was optional and if present was required to be equal to the nonce in the session['omniauth.nonce'] value. That changed in the 1.3 release where the nonce is now required in the id_token and is an error when missing.
My reading of the apple developer docs is that the nonce is an optional feature and may not appear in the id_token. Their wording makes it sounds like the nonce might be useful for locating a user session if the session cookie wasn't received for some reason (like maybe with non-lax cookies and their POST from a different origin).
https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_rest_api/authenticating_users_with_sign_in_with_apple
The nonce can be made to be optional and still enforce the expected value by simply not first requiring that the id_token[:nonce] value exists.
I have a spec test for this path coded up but it doesn't fit very nicely into the current test descriptions and contexts as most of those all force a nonce to be present. But here is a spec test that fails without the patch above and passes with it.
The text was updated successfully, but these errors were encountered: