-
Notifications
You must be signed in to change notification settings - Fork 11
Fix/disable hashed factorkey #224
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: master
Are you sure you want to change the base?
Conversation
updateSession if session id exist
himanshuchawla009
left a comment
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.
this PR changes a lot of other things which are not well documented in PR description. Please describe the reasoning behind the changes for better review.
src/mpcCoreKit.ts
Outdated
| } | ||
| } catch (err) { | ||
| log.warn("failed to authorize session", err); | ||
| log.warn("failed to authorize session please use new", err); |
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.
new wht?
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: Double Login Vulnerability in JWT Authentication
The loginWithJWT method is missing the double-login prevention check that was added to loginWithOAuth. According to the PR description, both login methods should check if this.state.factorKey exists to prevent double login, but only loginWithOAuth has this guard. This allows users to call loginWithJWT multiple times or after already being logged in, potentially causing state corruption.
src/mpcCoreKit.ts#L404-L415
mpc-core-kit/src/mpcCoreKit.ts
Lines 404 to 415 in 5d11a85
| public async loginWithJWT(params: JWTLoginParams): Promise<void> { | |
| this.checkReady(); | |
| const { prefetchTssPublicKeys = 1 } = params; | |
| if (prefetchTssPublicKeys > 3) { | |
| throw CoreKitError.prefetchValueExceeded(`The prefetch value '${prefetchTssPublicKeys}' exceeds the maximum allowed limit of 3.`); | |
| } | |
| const { verifier, verifierId, idToken, importTssKey, registerExistingSFAKey } = params; | |
| this.torusSp.verifierName = verifier; | |
| this.torusSp.verifierId = verifierId; | |
Motivation and Context
Jira Link:
Description
Fix disableHashedFactorkey feature
device factorkey is not set when disableHashedFactoryKey flag is set.
this is causing user lose the device factorkey during signup.
reinitialization - save deviceFactorkey on device if available
Fix atomic sync for
backupMetadataShare
deleteMetadataShareBackup
Add checking for already log-in state in the beginning of loginWithJWT to prevent double login
Add checking for valid factor key during InputFactorKey via Metadata's FactorEncs instead of depend only on checking for metadata linking
update tests
update tests cases
How has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Note
Adds strict factor key validation and double-login guards, persists device factor for disableHashedFactorKey, wraps metadata transitions in atomic sync, and updates tests (incl. ed25519 export/import).
inputFactorKeyviametadata.factorEncsinstead of relying only on metadata linking.backupMetadataShareanddeleteMetadataShareBackupinatomicSync(with conditional auto-commit).KEY_NOT_FOUNDandSHARE_DELETEDfor metadata checks; minor log/message tweaks.login-disableHashedFactorKey.spec.tscovering device-factor persistence, signing, wallet index, and pre-sign hooks.ed25519.spec.ts: add export/import seed test, session manager toggles, and ids.factors.spec.tsandimportRecovery.spec.ts: add signing helpers, recovery/import paths, storage/session adjustments, and test data updates.securityQuestion.spec.tsandsetup.ts(init params, emails).Written by Cursor Bugbot for commit ccff02e. This will update automatically on new commits. Configure here.