fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148
fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148himanshuchawla009 wants to merge 7 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
|
@metamaskbot publish-preview |
| vaultData: updatedVaultData, | ||
| pwEncKey, | ||
| }); | ||
|
|
There was a problem hiding this comment.
should we add option to skip proactive renewRefreshToken ?
There was a problem hiding this comment.
good suggestion, would be useful to skip in some cases.
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
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
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).
| // 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. |
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
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.
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
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. |
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.
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.
| revokeToken: newRevokeToken, | ||
| }, | ||
| pwEncKey, | ||
| }); |
There was a problem hiding this comment.
State updated before vault persist, error silently swallowed
Medium Severity
In #rotateRefreshToken, this.update() writes newRefreshToken and newRevokeToken to persisted state before #updateVault re-encrypts the vault with the new revokeToken. If the vault update fails, state holds the new tokens while the vault retains the old revokeToken. Because the caller in #doRefreshAuthTokens swallows this error via .catch(), the inconsistency is silent — no rollback of the state update occurs and #addRefreshTokenToRevokeList is skipped. On next vault unlock, #cachedDecryptedVaultData will contain the old revokeToken while state.revokeToken holds the new one, creating a mismatch that could cause subsequent rotation attempts to use a stale revokeToken.
Additional Locations (1)
3 participants
Explanation
References
Checklist
Note
Medium Risk
Changes token refresh/re-authentication and refresh-token rotation logic in an authentication-adjacent controller, including a breaking removal of the public
renewRefreshTokenAPI. Risk is moderated by extensive test updates but could affect token lifecycle behavior when vault locked/unlocked.Overview
Token refresh now re-authenticates via a stricter private
#reAuthenticatepath (onlyidTokens/accessToken/metadataAccessToken) and, when the vault is unlocked, proactively rotates the refresh/revoke token pair via a new private#rotateRefreshToken(with an opt-outrefreshAuthTokens({ skipRenewRefreshToken })and swallowed rotation errors).Token expiry checks were unified around a new
isTokenNearExpiryhelper: access/metadata tokens refresh when entering the last 10% of lifetime (requiresiat), while node auth tokens use exact expiry;fetchMetadataAccessCredsnow uses the same threshold. Tests and changelog were updated accordingly, including new cases for lock skipping, missingrevokeTokenin vault, proactive rotation behavior, and updated expectations around refresh token mutation.Written by Cursor Bugbot for commit 358e1b9. This will update automatically on new commits. Configure here.