-
-
Notifications
You must be signed in to change notification settings - Fork 949
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
fix: TOTP invalid code and/or 500 error #2960
Conversation
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.
Great find, thank you!
c6359dc
to
5d1a5b4
Compare
@aeneasr I fixed the failing assert.Equal(t, "{}", gjson.GetBytes(actualFlow.InternalContext, ... to assert.Empty(t, gjson.GetBytes(actualFlow.InternalContext, ... I haven't looked too deep into it, but my initial guess is that the test was relying on the buggy behavior, where the internal context was not reset properly. There is already an assertion like this (using |
574b78c
to
2d74082
Compare
And now I also fixed |
2d74082
to
c2e068b
Compare
Codecov Report
@@ Coverage Diff @@
## master #2960 +/- ##
==========================================
- Coverage 77.59% 76.51% -1.08%
==========================================
Files 310 308 -2
Lines 19265 19181 -84
==========================================
- Hits 14948 14677 -271
- Misses 3180 3402 +222
+ Partials 1137 1102 -35
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
2af25f4
to
0785c0a
Compare
And now some snapshot comparisons fail in WebAuthn tests, even though they passed before 😞 . |
0785c0a
to
a9c2c3c
Compare
Yeah sorry about that, we had some breaks on master :( Should all be fixed now |
a9c2c3c
to
f79b028
Compare
f79b028
to
acadd14
Compare
Fixes a 500 error if TOTP is removed and then re-added in the same flow.
It also fixes another related bug, that can be reproduced with https://github.com/ory/kratos-selfservice-ui-react-nextjs:
The reason is that because the
InternalContext
is not reset properly inPostSettingsHook
, the flow will have a new QR code and secret key (UI
is reset), but the internal context will keep the old TOTP secret.In the case of the original issue,
InternalContextKeyURL
is deleted fromInternalContext
when TOTP is removed, and since it's not set properly inPostSettingsHook
, when we try to re-add TOTP, we get 500, becauseInternalContextKeyURL
is missing.Related issue(s)
#2680
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments
WebAuthn is most likely affected by the same bug, but I didn't any tests there. I wasn't able to run WebAuthn tests locally, because of this error: