Conversation
…proactive renewal - Replace stale concurrent-rotation test with assertion that authenticate never receives refreshToken from the token-refresh path - Add tests verifying renewRefreshToken is called proactively after vault update when vault is unlocked, and skipped when locked - Add test covering the renewRefreshToken error-swallow path in refreshAuthTokens (catch block log call) - Add renewRefreshToken tests for vault-unlocked (no password) path and vault-locked-no-password error path - Fix two existing tests that captured state.refreshToken after the call; proactive renewal now rotates the token, so capture it before the call
Token expiry helpers now use a 90% lifetime threshold: - Access/metadata tokens refresh when <10% lifetime remains (uses iat) - Node auth tokens fall back to exact-expiry check (no reliable iat) Introduces module-level isTokenNearExpiry(exp, iat?) helper used by checkNodeAuthTokenExpired, checkMetadataAccessTokenExpired, and checkAccessTokenExpired.
Cover all source changes introduced in this branch: - authenticate() optional refreshToken param - proactive renewRefreshToken after JWT refresh in #doRefreshAuthTokens - renewRefreshToken vault-unlock path (optional password + skipLock) - 90% lifetime threshold for proactive token expiry checks
Contributor
Author
|
@metamaskbot publish-preview |
ieow
reviewed
Mar 10, 2026
| vaultData: updatedVaultData, | ||
| pwEncKey, | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
should we add option to skip proactive renewRefreshToken ?
Contributor
Author
There was a problem hiding this comment.
good suggestion, would be useful to skip in some cases.
ieow
reviewed
Mar 10, 2026
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
Contributor
There was a problem hiding this comment.
should we consider to allow renewRefreshToken happend only when the seedless controller vault is unlocked?
we can always call submitPassword before renewRefreshToken if there are scenario where the vault is locked and we have password
Contributor
There was a problem hiding this comment.
Agree with this. We can get rid of password from the params and rely on the cached encryptionKey only (which is only available when the vault is unlocked).
ieow
reviewed
Mar 10, 2026
| // NOTE: refreshToken is intentionally omitted — renewRefreshToken is the | ||
| // sole owner of state.refreshToken. Passing it here would risk overwriting | ||
| // a token that renewRefreshToken rotated concurrently during the async | ||
| // toprfClient.authenticate() call below. |
Contributor
There was a problem hiding this comment.
is it cleaner if we have re-authenticate function which only accept
idTokens,
accessToken,
metadataAccessToken,
Instead making the authenticate accept different input structure type
Contributor
There was a problem hiding this comment.
Yes, I think refactor the authenticate would be better, so that we can make stricter validation, i.e. first time authentication must provide accessToken and revokeToken while re-authenticate may not.
lwin-kyaw
reviewed
Mar 10, 2026
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
Contributor
There was a problem hiding this comment.
I noticed we call this method internally, inside #doRefreshAuthTokens() so, I think we can make this private already.
Clients should not decide when to call this method, imo.
| // NOTE: refreshToken is intentionally omitted — renewRefreshToken is the | ||
| // sole owner of state.refreshToken. Passing it here would risk overwriting | ||
| // a token that renewRefreshToken rotated concurrently during the async | ||
| // toprfClient.authenticate() call below. |
Contributor
There was a problem hiding this comment.
Yes, I think refactor the authenticate would be better, so that we can make stricter validation, i.e. first time authentication must provide accessToken and revokeToken while re-authenticate may not.
- Make renewRefreshToken private (#rotateRefreshToken): vault-unlock only,
no password param — callers must unlock first via submitPassword
- Add #reAuthenticate private method for JWT-refresh re-auth path, accepting
only {idTokens, accessToken, metadataAccessToken} from the refresh service
- Add skipRenewRefreshToken option to refreshAuthTokens to allow callers to
opt out of the proactive rotation
- Update tests: renewRefreshToken describe converted to #rotateRefreshToken
(tested via refreshAuthTokens), add skipRenewRefreshToken test, add
skipLock branch coverage for authenticate
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
- fetchMetadataAccessCreds: use isTokenNearExpiry(exp, iat) instead of exact expiry so it is consistent with checkMetadataAccessTokenExpired - isTokenNearExpiry: guard against malformed tokens where iat >= exp (zero or negative lifetime) by falling back to exact-expiry check
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
… explicit options Callers passing explicit options (e.g. skipRenewRefreshToken: true) must not be served by an in-flight request that was started with different options. Only default-options calls (no explicit flags) share the pending promise; explicit-options calls each get their own independent request.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
ieow
reviewed
Mar 12, 2026
| * | ||
| * @returns A Promise that resolves to void. | ||
| */ | ||
| async #rotateRefreshToken(): Promise<void> { |
Contributor
There was a problem hiding this comment.
Just to confirm the private behaviour
with this private, the parent cannot rotateRefreshToken (renewRefreshToken), it can now only revokePendingToken.
…ents and logging - Remove redundant `skipRenewRefreshToken` option from `refreshAuthTokens` since the `#cachedDecryptedVaultData` guard already prevents rotation when the vault is locked - Use project `log()` instead of `console.error` for rotation failure - Fix stale JSDoc on `authenticate` that referenced removed internal usage - Update `checkMetadataAccessTokenExpired` and `checkAccessTokenExpired` JSDoc to mention the 90% proactive refresh threshold - Add `VaultLocked` error constant with defensive guard in `#rotateRefreshToken` - Remove related tests for the deleted option
ieow
reviewed
Mar 16, 2026
| if (newRevokeToken && newRefreshToken) { | ||
| // Persist the new revoke token to the vault first. Only update state after | ||
| // the vault write succeeds, so state and vault stay in sync if the write fails. | ||
| await this.#updateVault({ |
Contributor
There was a problem hiding this comment.
info - the vault is not persisted yet after await.
It broadcast state:change message only.
same message is broadcasted during the addRefreshTokenToRevokeList
ieow
reviewed
Mar 16, 2026
| await this.authenticate({ | ||
| // Re-authenticate with the refreshed id tokens to update node auth | ||
| // tokens, accessToken, and metadataAccessToken in state. | ||
| await this.#reAuthenticate({ |
Contributor
There was a problem hiding this comment.
nit: access token is in the non-vault state,
do we still need it in the vault data?
check line 2183
Contributor
There was a problem hiding this comment.
Hello, accessToken is not persisted in plain state, we only persist in vault. That's why we need it.
Contributor
There was a problem hiding this comment.
if it is not persisted in the plain state, how does the check for password outdated works when the wallet is locked / app reopen ?
Contributor
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Expose rotateRefreshToken as a public method so callers can trigger refresh token rotation directly. Update tests to exercise the method via its public API and add coverage for edge cases.
Contributor
|
@metamaskbot publish-preview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| refreshToken, | ||
| revokeToken, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Public rotateRefreshToken lost controller lock protection
Medium Severity
The old renewRefreshToken was wrapped in this.#withControllerLock(), preventing concurrent execution with other locked operations like authenticate or submitPassword. The new rotateRefreshToken has no lock protection but remains public (the CHANGELOG says it "can also be called directly"). An external caller invoking rotateRefreshToken while refreshAuthTokens is in-flight could cause both to read the same revokeToken from the vault and the same refreshToken from state, leading to a double-rotation race: duplicate HTTP renewal calls, last-writer-wins on vault and state updates, and the same old token pair queued for revocation twice.
Contributor
There was a problem hiding this comment.
@himanshuchawla009 , should we add the pendingRequest variable like we did for the refresh access token ( refreshToken )
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
|
@metamaskbot publish-preview |
…esh wrapper assertPasswordInSync was called outside executeWithTokenRefresh in addNewSecretData and changePassword, causing fetchAuthPubKey to send expired nodeAuthTokens to the backend (FailedToFetchAuthPubKey errors in Sentry). Moving it inside the callback ensures tokens are refreshed before the call and retried on expiry.
lwin-kyaw
reviewed
Mar 18, 2026
| * @param params.groupedAuthConnectionId - Optional grouped authConnectionId to be used for the authenticate request. | ||
| * @param params.socialLoginEmail - The user email from Social login. | ||
| * @param params.refreshToken - refresh token for refreshing expired nodeAuthTokens. | ||
| * @param params.refreshToken - Refresh token issued during OAuth login. Written to state when provided. |
Contributor
There was a problem hiding this comment.
I think we can make revokeToken as required after we refactor re-authenticate logic to #reAuthenticate
Contributor
Author
There was a problem hiding this comment.
reauthenticate function can be called before unlock when revokeToken is not available, so don't think it can be required.
Contributor
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
ieow
reviewed
Mar 18, 2026
| }); | ||
|
|
||
| const performBackup = async (): Promise<void> => { | ||
| await this.#assertPasswordInSync({ |
Contributor
There was a problem hiding this comment.
why do we move this.#assertPasswordInSync in side the performBackup/attemptChangePassword ?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
References
Checklist
Note
Medium Risk
Touches authentication/token-refresh logic and changes refresh-token lifecycle (including a breaking API removal), which could impact session continuity if edge cases are missed.
Overview
Updates
refreshAuthTokensto re-authenticate via a new private#reAuthenticatethat only accepts JWT-refresh outputs, and (when the vault is unlocked) proactively rotates the refresh/revoke token pair via new publicrotateRefreshTokenwhile queuing the old pair for later revocation.Token expiry checks now use a proactive 90%-lifetime threshold for
accessToken/metadataAccessToken(via newisTokenNearExpiry), while node auth tokens use exact expiry;fetchMetadataAccessCredsaligns with this logic.Fixes token-refresh ordering in
addNewSecretData/changePasswordby moving#assertPasswordInSyncinside the token-refresh wrapper, adds a newVaultLockederror, removes the publicrenewRefreshTokenAPI, and refreshes/expands tests to cover the new behaviors.Written by Cursor Bugbot for commit 25d2d45. This will update automatically on new commits. Configure here.