-
Notifications
You must be signed in to change notification settings - Fork 11
fix: checkIfFactorKeyValid, check in factorEncs #225
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
base: fix/disableHashedFactorkey
Are you sure you want to change the base?
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.
Bug: NewUser Flag Malfunction on Reset/Init Rehydrate
The resetState method doesn't reset the newUser flag to false. According to the comment, newUser should only be true during new user sign up and should be false after reinit or rehydration. Since resetState is called during init and logout, the flag will incorrectly remain true after these operations for users who were initially new users.
src/mpcCoreKit.ts#L1406-L1413
mpc-core-kit/src/mpcCoreKit.ts
Lines 1406 to 1413 in d28e51c
| private resetState(): void { | |
| this.ready = false; | |
| this.tkey = null; | |
| this.torusSp = null; | |
| this.storageLayer = null; | |
| this.state = { accountIndex: 0 }; | |
| } |
d28e51c to
856f3af
Compare
| assert.strictEqual(instance.status, COREKIT_STATUS.LOGGED_IN); | ||
|
|
||
|
|
||
| const deviceFactorPub = getPubKeyPoint(deviceFactorKeyBN); |
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.
Bug: Test uses wrong import and missing curve parameter
The test imports getPubKeyPoint from @tkey/common-types but all source code imports it from @tkey/tss and calls it with two arguments: the key and factorKeyCurve. The test call getPubKeyPoint(deviceFactorKeyBN) only passes one argument. When this Point is passed to deleteFactor, it's compared against Points created using factorKeyCurve. Using a different function or missing the curve parameter will cause incorrect Point comparisons, making the test unreliable. The factorKeyCurve is already imported in the test file but not used with getPubKeyPoint.
Additional Locations (1)
| throw Error("should not be able to deleted input factor"); | ||
| } catch (e) { | ||
| assert(e instanceof Error); | ||
| } |
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.
Bug: Test assertion passes even when test should fail
The test's catch block uses assert(e instanceof Error) which will pass for both the expected error from inputFactorKey (when it correctly rejects a deleted factor) AND the manually thrown error on line 234 (when inputFactorKey incorrectly succeeds). If inputFactorKey fails to reject the deleted factor key, the test will still pass because the fallback throw Error(...) is also caught and satisfies the instanceof Error check. This masks potential regressions in the production code.
| throw Error("should not be able to deleted input factor"); | ||
| } catch (e) { | ||
| assert(e instanceof Error); | ||
| } |
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.
Bug: Test fails silently when expected error not thrown
The test's error handling logic is flawed. The catch block checks assert(e instanceof Error), which passes for both the expected error from inputFactorKey AND the manually thrown error at line 234. If inputFactorKey succeeds unexpectedly, the test throws "should not be able to deleted input factor", catches it, and the assertion still passes since that error is also an Error instance. This masks test failures - the test will pass even when the deleted factor key is incorrectly accepted.
Motivation and Context
Jira Link:
Description
tkey.metadata.factorEncswhich will reduce the metadata call and avoid dirty deleted factorkey metadata.Previously we check for valid factorkey by checking the metadata availability. This could cause issue if user deleted share with factor pub only but not factorkey where it delete the metadata.
How has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Note
Validate factor keys using
metadata.factorEncs, introducenewUserlifecycle flag, and extend tests to cover MFA device factor replacement.tkey.metadata.factorEncsincheckIfFactorKeyValidandinputFactorKeybefore metadata reads.public newUserflag; set inhandleNewUser/handleExistingUser, reset inresetState.getPubKeyPointand capturebrowserFactorfor reuse.DEVICEfactor, switch active factor, delete old device factor, and assert old factor no longer works.Written by Cursor Bugbot for commit 826e944. This will update automatically on new commits. Configure here.