fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148
Open
himanshuchawla009 wants to merge 10 commits intomainfrom
Open
fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148himanshuchawla009 wants to merge 10 commits intomainfrom
himanshuchawla009 wants to merge 10 commits intomainfrom
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
|
@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
|
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 |
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
Changes token refresh/rotation behavior and removes the public
renewRefreshTokenAPI (breaking), which can affect authentication stability if edge cases around vault lock state or token lifetime are missed. Test coverage was updated/expanded, but the changes touch security-sensitive token handling paths.Overview
Token refresh now re-authenticates via a new private
#reAuthenticatepath that only accepts JWT-refresh outputs, andrefreshAuthTokensproactively callsrotateRefreshToken(when the vault is unlocked) to rotate the refresh/revoke pair without failing the refresh flow if rotation fails.Token expiry checks for
accessToken/metadataAccessTokenswitch to a proactive 90%-lifetime threshold viaisTokenNearExpiry(with node auth tokens using exactexp), andfetchMetadataAccessCredsis aligned to the same logic. The publicrenewRefreshTokenmethod is removed (breaking) in favor ofrotateRefreshToken, with a newVaultLockederror and extensive test updates to cover the new behaviors and edge cases.Written by Cursor Bugbot for commit fe5e409. This will update automatically on new commits. Configure here.