From 9ee361d0a4a69e5a3f342f379e540b7de862bba3 Mon Sep 17 00:00:00 2001 From: himanshu Date: Mon, 9 Mar 2026 22:41:18 +0800 Subject: [PATCH 1/9] fix(seedless-onboarding): update tests for optional refreshToken and 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 --- .../src/SeedlessOnboardingController.test.ts | 216 ++++++++++++++---- .../src/SeedlessOnboardingController.ts | 143 +++++++++--- 2 files changed, 284 insertions(+), 75 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 2c87ed22f4b..8a5b1166064 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4673,6 +4673,10 @@ describe('SeedlessOnboardingController', () => { // Unlock controller first await controller.submitPassword(OLD_PASSWORD); + // Capture before the call — proactive renewRefreshToken inside + // refreshAuthTokens will rotate state.refreshToken afterwards. + const originalRefreshToken = controller.state.refreshToken; + const recoverEncKeySpy = jest.spyOn(toprfClient, 'recoverEncKey'); const encryptorSpy = jest.spyOn(encryptor, 'encryptWithDetail'); @@ -4713,7 +4717,7 @@ describe('SeedlessOnboardingController', () => { // Verify that getNewRefreshToken was called expect(mockRefreshJWTToken).toHaveBeenCalledWith({ connection: controller.state.authConnection, - refreshToken: controller.state.refreshToken, + refreshToken: originalRefreshToken, }); // Verify that recoverEncKey was called twice (once failed, once succeeded) @@ -5192,6 +5196,10 @@ describe('SeedlessOnboardingController', () => { async ({ controller, toprfClient, mockRefreshJWTToken }) => { await controller.submitPassword(MOCK_PASSWORD); + // Capture before the call — proactive renewRefreshToken inside + // refreshAuthTokens will rotate state.refreshToken afterwards. + const originalRefreshToken = controller.state.refreshToken; + // Mock authenticate for token refresh jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, @@ -5202,7 +5210,7 @@ describe('SeedlessOnboardingController', () => { expect(mockRefreshJWTToken).toHaveBeenCalledWith({ connection: controller.state.authConnection, - refreshToken: controller.state.refreshToken, + refreshToken: originalRefreshToken, }); expect(toprfClient.authenticate).toHaveBeenCalledWith({ @@ -5560,7 +5568,7 @@ describe('SeedlessOnboardingController', () => { ); }); - it('should use live state.refreshToken in authenticate when token was rotated concurrently', async () => { + it('should not pass refreshToken to authenticate during token refresh', async () => { await withController( { state: getMockInitialControllerState({ @@ -5568,55 +5576,123 @@ describe('SeedlessOnboardingController', () => { }), }, async ({ controller, mockRefreshJWTToken }) => { - const originalRefreshToken = controller.state.refreshToken; - const rotatedRefreshToken = 'rotated-refresh-token-from-renew'; + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); - let resolveRefresh!: () => void; - const refreshBarrier = new Promise((resolve) => { - resolveRefresh = resolve; + // Spy on the controller's own authenticate method to verify that + // refreshToken is intentionally omitted. Passing it here would risk + // overwriting a token that renewRefreshToken rotated concurrently. + const authenticateSpy = jest + .spyOn(controller, 'authenticate') + .mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + await controller.refreshAuthTokens(); + + expect(authenticateSpy).not.toHaveBeenCalledWith( + expect.objectContaining({ refreshToken: expect.anything() }), + ); + }, + ); + }); + + it('should proactively call renewRefreshToken after vault update when vault is unlocked', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + vault: encryptedMockVault, + vaultEncryptionKey, + vaultEncryptionSalt, + }), + }, + async ({ + controller, + toprfClient, + mockRenewRefreshToken, + mockRefreshJWTToken, + }) => { + await controller.submitPassword(MOCK_PASSWORD); + + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, }); - mockRefreshJWTToken.mockImplementation(async () => { - await refreshBarrier; - return { - idTokens: ['newIdToken'], - metadataAccessToken: 'mock-metadata-access-token', - accessToken, - }; + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, }); - // Spy on the controller's own authenticate method — this is the - // call that receives the refreshToken parameter. The inner - // toprfClient.authenticate call does not carry refreshToken. - const authenticateSpy = jest - .spyOn(controller, 'authenticate') - .mockResolvedValue(); + await controller.refreshAuthTokens(); - // Start the refresh — it is now blocked at the HTTP call. - const refreshPromise = controller.refreshAuthTokens(); + expect(mockRenewRefreshToken).toHaveBeenCalled(); + }, + ); + }); - // Simulate renewRefreshToken writing a new token to state while the - // HTTP call is in-flight. refreshAuthTokens holds no controller lock, - // so these two can genuinely overlap in production. - // @ts-expect-error Accessing protected method for testing - controller.update((state) => { - state.refreshToken = rotatedRefreshToken; + it('should not call renewRefreshToken when vault is locked', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient, mockRenewRefreshToken }) => { + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, }); - // Release the HTTP call and let refreshAuthTokens complete. - resolveRefresh(); - await refreshPromise; - - // authenticate must have received the live state value, not the - // value captured at the start of the method. Using the stale - // captured value would overwrite the newer token in state, causing - // all subsequent refreshes to 401 once the old token is revoked. - expect(authenticateSpy).toHaveBeenCalledWith( - expect.objectContaining({ refreshToken: rotatedRefreshToken }), - ); - expect(authenticateSpy).not.toHaveBeenCalledWith( - expect.objectContaining({ refreshToken: originalRefreshToken }), + await controller.refreshAuthTokens(); + + expect(mockRenewRefreshToken).not.toHaveBeenCalled(); + }, + ); + }); + + it('should swallow renewRefreshToken errors and still resolve when vault is unlocked', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + vault: encryptedMockVault, + vaultEncryptionKey, + vaultEncryptionSalt, + }), + }, + async ({ + controller, + toprfClient, + mockRenewRefreshToken, + mockRefreshJWTToken, + }) => { + await controller.submitPassword(MOCK_PASSWORD); + + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); + + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + // Make the proactive renewal fail — the error is caught and logged. + mockRenewRefreshToken.mockRejectedValueOnce( + new Error('renewal failed'), ); + + // refreshAuthTokens must still resolve even when renewal fails. + expect(await controller.refreshAuthTokens()).toBeUndefined(); }, ); }); @@ -6262,6 +6338,62 @@ describe('SeedlessOnboardingController', () => { }, ); }); + + it('should work without a password when vault is already unlocked', async () => { + const mockToprfEncryptor = createMockToprfEncryptor(); + const MOCK_ENCRYPTION_KEY = + mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); + const MOCK_PASSWORD_ENCRYPTION_KEY = + mockToprfEncryptor.derivePwEncKey(MOCK_PASSWORD); + const MOCK_AUTH_KEY_PAIR = + mockToprfEncryptor.deriveAuthKeyPair(MOCK_PASSWORD); + + const mockResult = await createMockVault( + MOCK_ENCRYPTION_KEY, + MOCK_PASSWORD_ENCRYPTION_KEY, + MOCK_AUTH_KEY_PAIR, + MOCK_PASSWORD, + MOCK_REVOKE_TOKEN, + ); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + vault: mockResult.encryptedMockVault, + vaultEncryptionKey: mockResult.vaultEncryptionKey, + vaultEncryptionSalt: mockResult.vaultEncryptionSalt, + }), + }, + async ({ controller, mockRenewRefreshToken }) => { + // Unlock the vault first so #cachedDecryptedVaultData is populated. + await controller.submitPassword(MOCK_PASSWORD); + + // Call without a password — should use cached vault data directly. + await controller.renewRefreshToken(); + + expect(mockRenewRefreshToken).toHaveBeenCalledWith({ + connection: controller.state.authConnection, + revokeToken: MOCK_REVOKE_TOKEN, + }); + }, + ); + }); + + it('should throw VaultError when vault is locked and no password is provided', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller }) => { + await expect(controller.renewRefreshToken()).rejects.toThrow( + SeedlessOnboardingControllerErrorMessage.VaultError, + ); + }, + ); + }); }); describe('revokePendingRefreshTokens', () => { diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 56df91704de..aa341bcd87d 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -503,7 +503,9 @@ export class SeedlessOnboardingController< * @param params.userId - user email or id from Social login * @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 - Optional refresh token issued during OAuth login. When provided it is + * written to state. Omit when calling from token-refresh paths (e.g. `#doRefreshAuthTokens`) so + * that a concurrent `renewRefreshToken` rotation is never silently overwritten. * @param params.revokeToken - revoke token for revoking refresh token and get new refresh token and new revoke token. * @param params.accessToken - Access token for pairing with profile sync auth service and to access other services. * @param params.metadataAccessToken - Metadata access token for accessing the metadata service before the vault is created or unlocked. @@ -519,7 +521,7 @@ export class SeedlessOnboardingController< userId: string; groupedAuthConnectionId?: string; socialLoginEmail?: string; - refreshToken: string; + refreshToken?: string; revokeToken?: string; skipLock?: boolean; }): Promise { @@ -553,7 +555,12 @@ export class SeedlessOnboardingController< state.authConnection = authConnection; state.socialLoginEmail = socialLoginEmail; state.metadataAccessToken = metadataAccessToken; - state.refreshToken = refreshToken; + // Only write refreshToken when a new value is explicitly provided (i.e. during + // OAuth login). Token-refresh paths omit this field so that a concurrent + // renewRefreshToken rotation is never silently overwritten. + if (refreshToken) { + state.refreshToken = refreshToken; + } if (revokeToken) { // Temporarily store revoke token & access token in state for later vault creation state.revokeToken = revokeToken; @@ -2161,15 +2168,13 @@ export class SeedlessOnboardingController< try { const { idTokens, accessToken, metadataAccessToken } = res; - // re-authenticate with the new id tokens to set new node auth tokens - // NOTE: here we can't provide the `revokeToken` value to the `authenticate` method because `refreshAuthTokens` method can be called when the wallet (vault) is locked - // NOTE: use this.state.refreshToken (current value) rather than the - // `refreshToken` captured at the start of this method. If renewRefreshToken() - // ran concurrently while our HTTP call was in-flight, it will have already - // updated state.refreshToken to the new token. Using the captured (now-stale) - // value would overwrite that newer token and revert the state, causing the - // old token to be used for subsequent refreshes — and once that old token is - // revoked (15 s after the renewal), every subsequent refresh will return 401. + // Re-authenticate with the new id tokens to set new node auth tokens. + // NOTE: revokeToken is intentionally omitted — refreshAuthTokens can be + // called while the vault is locked, so we never have access to it here. + // 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. await this.authenticate({ idTokens, accessToken, @@ -2178,7 +2183,6 @@ export class SeedlessOnboardingController< authConnectionId: this.state.authConnectionId, groupedAuthConnectionId: this.state.groupedAuthConnectionId, userId: this.state.userId, - refreshToken: this.state.refreshToken, skipLock: true, }); @@ -2194,6 +2198,18 @@ export class SeedlessOnboardingController< vaultData: updatedVaultData, pwEncKey, }); + + // Proactively rotate the refresh token now that we have vault access. + // skipLock: true prevents deadlock when #doRefreshAuthTokens is invoked + // from within a #withControllerLock context via #executeWithTokenRefresh. + await this.renewRefreshToken(undefined, { skipLock: true }).catch( + (error) => { + log( + 'Error renewing refresh token after successful JWT refresh', + error, + ); + }, + ); } } catch (error) { log('Error refreshing node auth tokens', error); @@ -2211,22 +2227,66 @@ export class SeedlessOnboardingController< * and also updates the vault with the new revoke token. * This method is to be called after user is authenticated. * - * @param password - The password to encrypt the vault. + * When the vault is already unlocked (e.g. called from `#doRefreshAuthTokens` + * after a JWT refresh), `password` may be omitted — the cached vault data is + * used directly, avoiding a redundant and expensive password-based vault + * decryption. Pass `skipLock: true` in that case to prevent a deadlock when + * the caller is already executing inside `#withControllerLock`. + * + * @param password - The password to decrypt/re-encrypt the vault. Required + * when the vault is locked; may be omitted when the vault is already unlocked. + * @param options - Optional flags. + * @param options.skipLock - Skip acquiring `#withControllerLock`. Use when + * the caller already holds the lock or when the lock must not be re-acquired + * (e.g. from inside `#doRefreshAuthTokens` which can run within a locked + * operation via `#executeWithTokenRefresh`). * @returns A Promise that resolves to void. */ - async renewRefreshToken(password: string): Promise { - return await this.#withControllerLock(async () => { + async renewRefreshToken( + password?: string, + options?: { skipLock?: boolean }, + ): Promise { + const doRenew = async (): Promise => { this.#assertIsAuthenticatedUser(this.state); - const { refreshToken, vaultEncryptionKey } = this.state; - const { - toprfEncryptionKey: rawToprfEncryptionKey, - toprfPwEncryptionKey: rawToprfPwEncryptionKey, - toprfAuthKeyPair: rawToprfAuthKeyPair, - revokeToken, - } = await this.#unlockVaultAndGetVaultData({ - password, - encryptionKey: vaultEncryptionKey, - }); + const { refreshToken } = this.state; + + let rawToprfEncryptionKey: Uint8Array; + let rawToprfPwEncryptionKey: Uint8Array; + let rawToprfAuthKeyPair: KeyPair; + let revokeToken: string; + // Captured when vault is already unlocked; used to build the updated + // vault data without a password re-encryption round-trip. + let unlockedVaultData: DeserializedVaultData | undefined; + + if (this.#isUnlocked && this.#cachedDecryptedVaultData) { + // Vault is already unlocked — use cached vault data directly to avoid + // a redundant (and expensive) password-based decryption round-trip. + unlockedVaultData = this.#cachedDecryptedVaultData; + ({ + toprfEncryptionKey: rawToprfEncryptionKey, + toprfPwEncryptionKey: rawToprfPwEncryptionKey, + toprfAuthKeyPair: rawToprfAuthKeyPair, + revokeToken, + } = unlockedVaultData); + } else { + if (!password) { + // Vault is locked and no password provided — cannot proceed. + throw new SeedlessOnboardingError( + SeedlessOnboardingControllerErrorMessage.VaultError, + { + cause: new Error('Vault is locked and no password provided'), + }, + ); + } + ({ + toprfEncryptionKey: rawToprfEncryptionKey, + toprfPwEncryptionKey: rawToprfPwEncryptionKey, + toprfAuthKeyPair: rawToprfAuthKeyPair, + revokeToken, + } = await this.#unlockVaultAndGetVaultData({ + password, + })); + } const { newRevokeToken, newRefreshToken } = await this.#renewRefreshToken( { @@ -2243,19 +2303,36 @@ export class SeedlessOnboardingController< state.refreshToken = newRefreshToken; }); - await this.#createNewVaultWithAuthData({ - password, - rawToprfEncryptionKey, - rawToprfPwEncryptionKey, - rawToprfAuthKeyPair, - }); + if (password) { + await this.#createNewVaultWithAuthData({ + password, + rawToprfEncryptionKey, + rawToprfPwEncryptionKey, + rawToprfAuthKeyPair, + }); + } else if (unlockedVaultData) { + // Vault was already unlocked — re-encrypt in-place using the + // existing vault encryption key (no password KDF needed). + await this.#updateVault({ + vaultData: { + ...unlockedVaultData, + revokeToken: newRevokeToken, + }, + pwEncKey: rawToprfPwEncryptionKey, + }); + } // add the old refresh token to the list to be revoked later when possible this.#addRefreshTokenToRevokeList({ refreshToken, revokeToken, }); } - }); + }; + + if (options?.skipLock) { + return await doRenew(); + } + return await this.#withControllerLock(doRenew); } /** From d905738c692b47d9a41fa34d81de2c9ae8dad92f Mon Sep 17 00:00:00 2001 From: himanshu Date: Tue, 10 Mar 2026 15:01:59 +0800 Subject: [PATCH 2/9] feat(seedless-onboarding): proactive 90% lifetime token expiry check 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. --- .../CHANGELOG.md | 4 + .../src/SeedlessOnboardingController.test.ts | 180 +++++++++++++++++- .../src/SeedlessOnboardingController.ts | 30 ++- 3 files changed, 202 insertions(+), 12 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 64ce4032900..ab96994ba1f 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Token expiry checks now use a 90% lifetime threshold for proactive refresh: access tokens and metadata access tokens are refreshed when less than 10% of their lifetime remains (requires `iat`), while node auth tokens fall back to exact-expiry checking ([#8139](https://github.com/MetaMask/core/pull/8139)) + ## [8.1.0] ### Changed diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 8a5b1166064..299882c9530 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4407,6 +4407,58 @@ describe('SeedlessOnboardingController', () => { }, ); }); + + it('should return true when node auth token is past its expiry', async () => { + const expiredToken = createMockNodeAuthToken({ + exp: Math.floor(Date.now() / 1000) - 1, + }); + await withController( + { + state: { + ...getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + // assertIsAuthenticatedUser requires ≥ 3 tokens; only the first is checked + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS.map((token, i) => ({ + ...token, + authToken: i === 0 ? expiredToken : token.authToken, + })), + }, + }, + async ({ controller }) => { + jest.spyOn(controller, 'checkNodeAuthTokenExpired').mockRestore(); + + const isExpired = controller.checkNodeAuthTokenExpired(); + expect(isExpired).toBe(true); + }, + ); + }); + + it('should return false when node auth token has not yet reached its expiry', async () => { + const validToken = createMockNodeAuthToken({ + exp: Math.floor(Date.now() / 1000) + 7000, + }); + await withController( + { + state: { + ...getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + // assertIsAuthenticatedUser requires ≥ 3 tokens; only the first is checked + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS.map((token, idx) => ({ + ...token, + authToken: idx === 0 ? validToken : token.authToken, + })), + }, + }, + async ({ controller }) => { + jest.spyOn(controller, 'checkNodeAuthTokenExpired').mockRestore(); + + const isExpired = controller.checkNodeAuthTokenExpired(); + expect(isExpired).toBe(false); + }, + ); + }); }); describe('checkIsPasswordOutdated with token refresh', () => { @@ -5771,8 +5823,11 @@ describe('SeedlessOnboardingController', () => { describe('checkMetadataAccessTokenExpired', () => { it('should return false if metadata access token is not expired', async () => { - const futureExp = Math.floor(Date.now() / 1000) + 3600; // 1 hour from now - const validToken = createMockJWTToken({ exp: futureExp }); + const now = Math.floor(Date.now() / 1000); + const validToken = createMockJWTToken({ + iat: now, + exp: now + 3600, // 1 hour lifetime, 0% elapsed + }); await withController( { @@ -5794,8 +5849,11 @@ describe('SeedlessOnboardingController', () => { }); it('should return true if metadata access token is expired', async () => { - const pastExp = Math.floor(Date.now() / 1000) - 3600; // 1 hour ago - const expiredToken = createMockJWTToken({ exp: pastExp }); + const now = Math.floor(Date.now() / 1000); + const expiredToken = createMockJWTToken({ + iat: now - 7200, + exp: now - 3600, // issued 2 h ago, expired 1 h ago + }); await withController( { @@ -5816,6 +5874,58 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should return true when 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; 9 001 s have elapsed → just past the 90 % mark + const tokenAt90Percent = createMockJWTToken({ + iat: now - 9001, + exp: now + 999, + }); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + metadataAccessToken: tokenAt90Percent, + }), + }, + async ({ controller }) => { + jest + .spyOn(controller, 'checkMetadataAccessTokenExpired') + .mockRestore(); + + const result = controller.checkMetadataAccessTokenExpired(); + expect(result).toBe(true); + }, + ); + }); + + it('should return false when less than 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; only 3 000 s have elapsed → well below the 90 % mark + const tokenBelow90Percent = createMockJWTToken({ + iat: now - 3000, + exp: now + 7000, + }); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + metadataAccessToken: tokenBelow90Percent, + }), + }, + async ({ controller }) => { + jest + .spyOn(controller, 'checkMetadataAccessTokenExpired') + .mockRestore(); + + const result = controller.checkMetadataAccessTokenExpired(); + expect(result).toBe(false); + }, + ); + }); + it('should return true if user is not authenticated', async () => { await withController(async ({ controller }) => { // Restore the original implementation to test the real logic @@ -5849,8 +5959,11 @@ describe('SeedlessOnboardingController', () => { describe('checkAccessTokenExpired', () => { it('should return false if access token is not expired', async () => { - const futureExp = Math.floor(Date.now() / 1000) + 3600; // 1 hour from now - const validToken = createMockJWTToken({ exp: futureExp }); + const now = Math.floor(Date.now() / 1000); + const validToken = createMockJWTToken({ + iat: now, + exp: now + 3600, // 1 hour lifetime, 0% elapsed + }); await withController( { @@ -5870,8 +5983,11 @@ describe('SeedlessOnboardingController', () => { }); it('should return true if access token is expired', async () => { - const pastExp = Math.floor(Date.now() / 1000) - 3600; // 1 hour ago - const expiredToken = createMockJWTToken({ exp: pastExp }); + const now = Math.floor(Date.now() / 1000); + const expiredToken = createMockJWTToken({ + iat: now - 7200, + exp: now - 3600, // issued 2 h ago, expired 1 h ago + }); await withController( { @@ -5890,6 +6006,54 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should return true when 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; 9 001 s have elapsed → just past the 90 % mark + const tokenAt90Percent = createMockJWTToken({ + iat: now - 9001, + exp: now + 999, + }); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + accessToken: tokenAt90Percent, + }), + }, + async ({ controller }) => { + jest.spyOn(controller, 'checkAccessTokenExpired').mockRestore(); + + const result = controller.checkAccessTokenExpired(); + expect(result).toBe(true); + }, + ); + }); + + it('should return false when less than 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; only 3 000 s have elapsed → well below the 90 % mark + const tokenBelow90Percent = createMockJWTToken({ + iat: now - 3000, + exp: now + 7000, + }); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + accessToken: tokenBelow90Percent, + }), + }, + async ({ controller }) => { + jest.spyOn(controller, 'checkAccessTokenExpired').mockRestore(); + + const result = controller.checkAccessTokenExpired(); + expect(result).toBe(false); + }, + ); + }); + it('should return true if user is not authenticated', async () => { await withController(async ({ controller }) => { // Restore the original implementation to test the real logic diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index aa341bcd87d..fccf90321ae 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -2543,8 +2543,8 @@ export class SeedlessOnboardingController< const firstAuthToken = nodeAuthTokens[0]?.authToken; // node auth token is base64 encoded json object const decodedToken = decodeNodeAuthToken(firstAuthToken); - // check if the token is expired - return decodedToken.exp < Date.now() / 1000; + // Node auth tokens do not carry a reliable iat field — use exact expiry. + return isTokenNearExpiry(decodedToken.exp); } /** @@ -2558,7 +2558,7 @@ export class SeedlessOnboardingController< const { metadataAccessToken } = this.state; // assertIsAuthenticatedUser will throw if metadataAccessToken is missing const decodedToken = decodeJWTToken(metadataAccessToken as string); - return decodedToken.exp < Math.floor(Date.now() / 1000); + return isTokenNearExpiry(decodedToken.exp, decodedToken.iat); } catch { return true; // Consider unauthenticated user as having expired tokens } @@ -2578,13 +2578,35 @@ export class SeedlessOnboardingController< return true; // Consider missing token as expired } const decodedToken = decodeJWTToken(accessToken); - return decodedToken.exp < Math.floor(Date.now() / 1000); + return isTokenNearExpiry(decodedToken.exp, decodedToken.iat); } catch { return true; // Consider unauthenticated user as having expired tokens } } } +/** + * Determine whether a token should be proactively refreshed. + * + * When `iat` is provided: returns `true` when less than 10% of the token's + * lifetime remains (i.e. we are in the last 10% before expiry). + * When `iat` is omitted (e.g. node auth tokens): returns `true` when the token + * is already expired. + * + * @param exp - Token expiration time in seconds (Unix epoch). + * @param iat - Optional issued-at time in seconds (Unix epoch). Required for 10% threshold. + * @returns True if the token should be refreshed. + */ +function isTokenNearExpiry(exp: number, iat?: number): boolean { + const now = Date.now() / 1000; + if (iat === undefined) { + return now >= exp; + } + const remaining = exp - now; + const lifetime = exp - iat; + return remaining <= 0.1 * lifetime; +} + /** * Lock the given mutex before executing the given function, * and release it after the function is resolved or after an From f3503dd74656cbaa40c495470783bae1bfad0e1f Mon Sep 17 00:00:00 2001 From: himanshu Date: Tue, 10 Mar 2026 15:38:11 +0800 Subject: [PATCH 3/9] chore(seedless-onboarding): expand [Unreleased] changelog for PR #8148 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 --- packages/seedless-onboarding-controller/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index ab96994ba1f..5353dbbb7e3 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -9,7 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Token expiry checks now use a 90% lifetime threshold for proactive refresh: access tokens and metadata access tokens are refreshed when less than 10% of their lifetime remains (requires `iat`), while node auth tokens fall back to exact-expiry checking ([#8139](https://github.com/MetaMask/core/pull/8139)) +- `authenticate` now accepts an optional `refreshToken` parameter — it is only written to state when provided, preventing a concurrent `renewRefreshToken` rotation from being silently overwritten when called from token-refresh paths ([#8148](https://github.com/MetaMask/core/pull/8148)) +- `#doRefreshAuthTokens` now proactively calls `renewRefreshToken` after a successful JWT refresh (without requiring a password) to keep the refresh/revoke token pair fresh ([#8148](https://github.com/MetaMask/core/pull/8148)) +- `renewRefreshToken` now accepts an optional `password` parameter and a `skipLock` option — when the vault is already unlocked the cached vault data is used directly, avoiding a redundant vault decryption; when the vault is locked a password is still required ([#8148](https://github.com/MetaMask/core/pull/8148)) +- Token expiry checks now use a 90% lifetime threshold for proactive refresh: access tokens and metadata access tokens are refreshed when less than 10% of their lifetime remains (requires `iat`), while node auth tokens fall back to exact-expiry checking ([#8148](https://github.com/MetaMask/core/pull/8148)) ## [8.1.0] From ac6a62a00733d889da5c84ae17f6f0c5a0a5c41d Mon Sep 17 00:00:00 2001 From: himanshu Date: Wed, 11 Mar 2026 14:21:08 +0800 Subject: [PATCH 4/9] refactor(seedless-onboarding): apply PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../CHANGELOG.md | 11 +- .../src/SeedlessOnboardingController.test.ts | 288 ++++++++++-------- .../src/SeedlessOnboardingController.ts | 255 ++++++++-------- 3 files changed, 306 insertions(+), 248 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 5353dbbb7e3..e959724da05 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -9,11 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `authenticate` now accepts an optional `refreshToken` parameter — it is only written to state when provided, preventing a concurrent `renewRefreshToken` rotation from being silently overwritten when called from token-refresh paths ([#8148](https://github.com/MetaMask/core/pull/8148)) -- `#doRefreshAuthTokens` now proactively calls `renewRefreshToken` after a successful JWT refresh (without requiring a password) to keep the refresh/revoke token pair fresh ([#8148](https://github.com/MetaMask/core/pull/8148)) -- `renewRefreshToken` now accepts an optional `password` parameter and a `skipLock` option — when the vault is already unlocked the cached vault data is used directly, avoiding a redundant vault decryption; when the vault is locked a password is still required ([#8148](https://github.com/MetaMask/core/pull/8148)) +- `authenticate` now accepts an optional `refreshToken` parameter — it is only written to state when provided, so token-refresh paths cannot silently overwrite a concurrently-rotated refresh token ([#8148](https://github.com/MetaMask/core/pull/8148)) +- Token refresh now uses a dedicated private `#reAuthenticate` method that only accepts the tokens returned by the JWT-refresh service (`idTokens`, `accessToken`, `metadataAccessToken`), making the re-authentication path stricter than the initial `authenticate` call ([#8148](https://github.com/MetaMask/core/pull/8148)) +- After a successful JWT refresh the controller proactively rotates the refresh/revoke token pair (when the vault is unlocked) via a new private `#rotateRefreshToken` method ([#8148](https://github.com/MetaMask/core/pull/8148)) +- `refreshAuthTokens` now accepts an optional `skipRenewRefreshToken` flag to opt out of the proactive token rotation ([#8148](https://github.com/MetaMask/core/pull/8148)) - Token expiry checks now use a 90% lifetime threshold for proactive refresh: access tokens and metadata access tokens are refreshed when less than 10% of their lifetime remains (requires `iat`), while node auth tokens fall back to exact-expiry checking ([#8148](https://github.com/MetaMask/core/pull/8148)) +### Removed + +- Removed public `renewRefreshToken` method — refresh token rotation is now handled automatically by the controller after a successful JWT refresh ([#8148](https://github.com/MetaMask/core/pull/8148)) + ## [8.1.0] ### Changed diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 299882c9530..c3f40e701ae 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -1033,6 +1033,33 @@ describe('SeedlessOnboardingController', () => { ); }); }); + + it('should skip the controller lock when skipLock is true', async () => { + await withController(async ({ controller, toprfClient }) => { + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + const authResult = await controller.authenticate({ + idTokens, + authConnectionId, + userId, + authConnection, + socialLoginEmail, + refreshToken, + revokeToken, + accessToken, + metadataAccessToken, + skipLock: true, + }); + + expect(authResult.isNewUser).toBe(false); + expect(controller.state.isSeedlessOnboardingUserAuthenticated).toBe( + true, + ); + }); + }); }); describe('checkPasswordOutdated', () => { @@ -2578,6 +2605,47 @@ describe('SeedlessOnboardingController', () => { }, ); }); + + it('should throw InvalidRevokeToken if vault data is missing revokeToken', async () => { + const mockToprfEncryptor = createMockToprfEncryptor(); + const encKey = mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); + const pwEncKey = mockToprfEncryptor.derivePwEncKey(MOCK_PASSWORD); + const authKeyPair = mockToprfEncryptor.deriveAuthKeyPair(MOCK_PASSWORD); + const encryptor = createMockVaultEncryptor(); + + // Build a vault without the revokeToken field. + const vaultPayload = JSON.stringify({ + toprfEncryptionKey: bytesToBase64(encKey), + toprfPwEncryptionKey: bytesToBase64(pwEncKey), + toprfAuthKeyPair: JSON.stringify({ + sk: `0x${authKeyPair.sk.toString(16)}`, + pk: bytesToBase64(authKeyPair.pk), + }), + accessToken, + // revokeToken intentionally omitted + }); + + const { vault: vaultWithoutRevokeToken, exportedKeyString } = + await encryptor.encryptWithDetail(MOCK_PASSWORD, vaultPayload); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + vault: vaultWithoutRevokeToken, + vaultEncryptionKey: exportedKeyString, + vaultEncryptionSalt: JSON.parse(vaultWithoutRevokeToken).salt, + }), + }, + async ({ controller }) => { + await expect( + controller.submitPassword(MOCK_PASSWORD), + ).rejects.toThrow( + SeedlessOnboardingControllerErrorMessage.InvalidRevokeToken, + ); + }, + ); + }); }); describe('verifyPassword', () => { @@ -5620,25 +5688,23 @@ describe('SeedlessOnboardingController', () => { ); }); - it('should not pass refreshToken to authenticate during token refresh', async () => { + it('should re-authenticate using only the new idTokens from the JWT refresh', async () => { await withController( { state: getMockInitialControllerState({ withMockAuthenticatedUser: true, }), }, - async ({ controller, mockRefreshJWTToken }) => { + async ({ controller, toprfClient, mockRefreshJWTToken }) => { + const newIdTokens = ['newIdToken']; mockRefreshJWTToken.mockResolvedValueOnce({ - idTokens: ['newIdToken'], + idTokens: newIdTokens, metadataAccessToken: 'mock-metadata-access-token', accessToken, }); - // Spy on the controller's own authenticate method to verify that - // refreshToken is intentionally omitted. Passing it here would risk - // overwriting a token that renewRefreshToken rotated concurrently. - const authenticateSpy = jest - .spyOn(controller, 'authenticate') + const toprfAuthSpy = jest + .spyOn(toprfClient, 'authenticate') .mockResolvedValue({ nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, isNewUser: false, @@ -5646,13 +5712,51 @@ describe('SeedlessOnboardingController', () => { await controller.refreshAuthTokens(); - expect(authenticateSpy).not.toHaveBeenCalledWith( - expect.objectContaining({ refreshToken: expect.anything() }), + // #reAuthenticate passes only idTokens to the TOPRF client — + // refreshToken and revokeToken are structurally absent. + expect(toprfAuthSpy).toHaveBeenCalledWith( + expect.objectContaining({ idTokens: newIdTokens }), ); }, ); }); + it('should skip proactive token rotation when skipRenewRefreshToken is true', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + vault: encryptedMockVault, + vaultEncryptionKey, + vaultEncryptionSalt, + }), + }, + async ({ + controller, + toprfClient, + mockRenewRefreshToken, + mockRefreshJWTToken, + }) => { + await controller.submitPassword(MOCK_PASSWORD); + + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); + + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + await controller.refreshAuthTokens({ skipRenewRefreshToken: true }); + + expect(mockRenewRefreshToken).not.toHaveBeenCalled(); + }, + ); + }); + it('should proactively call renewRefreshToken after vault update when vault is unlocked', async () => { await withController( { @@ -6374,11 +6478,11 @@ describe('SeedlessOnboardingController', () => { }); }); - describe('renewRefreshToken', () => { + describe('#rotateRefreshToken', () => { const MOCK_PASSWORD = 'mock-password'; const MOCK_REVOKE_TOKEN = 'newRevokeToken'; - it('should successfully renew refresh token and update vault', async () => { + it('should call the renewal service with connection and revokeToken from vault', async () => { const mockToprfEncryptor = createMockToprfEncryptor(); const MOCK_ENCRYPTION_KEY = mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); @@ -6404,62 +6508,36 @@ describe('SeedlessOnboardingController', () => { vaultEncryptionSalt: mockResult.vaultEncryptionSalt, }), }, - async ({ controller, mockRenewRefreshToken }) => { - await controller.renewRefreshToken(MOCK_PASSWORD); + async ({ + controller, + toprfClient, + mockRenewRefreshToken, + mockRefreshJWTToken, + }) => { + // Unlock vault so #cachedDecryptedVaultData is populated. + await controller.submitPassword(MOCK_PASSWORD); - expect(mockRenewRefreshToken).toHaveBeenCalledWith({ - connection: controller.state.authConnection, - revokeToken: controller.state.revokeToken, + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, }); - }, - ); - }); - - it('should throw error if revoke token is missing', async () => { - const mockToprfEncryptor = createMockToprfEncryptor(); - const MOCK_ENCRYPTION_KEY = - mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); - const MOCK_PASSWORD_ENCRYPTION_KEY = - mockToprfEncryptor.derivePwEncKey(MOCK_PASSWORD); - const MOCK_AUTH_KEY_PAIR = - mockToprfEncryptor.deriveAuthKeyPair(MOCK_PASSWORD); - - // Create vault data without revoke token manually - const encryptor = createMockVaultEncryptor(); - const serializedKeyData = JSON.stringify({ - toprfEncryptionKey: bytesToBase64(MOCK_ENCRYPTION_KEY), - toprfPwEncryptionKey: bytesToBase64(MOCK_PASSWORD_ENCRYPTION_KEY), - toprfAuthKeyPair: JSON.stringify({ - sk: `0x${MOCK_AUTH_KEY_PAIR.sk.toString(16)}`, - pk: bytesToBase64(MOCK_AUTH_KEY_PAIR.pk), - }), - // Intentionally omit revokeToken - accessToken, - }); - const { vault: encryptedMockVault, exportedKeyString } = - await encryptor.encryptWithDetail(MOCK_PASSWORD, serializedKeyData); + await controller.refreshAuthTokens(); - await withController( - { - state: getMockInitialControllerState({ - withMockAuthenticatedUser: true, - vault: encryptedMockVault, - vaultEncryptionKey: exportedKeyString, - vaultEncryptionSalt: JSON.parse(encryptedMockVault).salt, - }), - }, - async ({ controller }) => { - await expect( - controller.renewRefreshToken(MOCK_PASSWORD), - ).rejects.toThrow( - SeedlessOnboardingControllerErrorMessage.InvalidRevokeToken, - ); + expect(mockRenewRefreshToken).toHaveBeenCalledWith({ + connection: controller.state.authConnection, + revokeToken: MOCK_REVOKE_TOKEN, + }); }, ); }); - it('should not queue old token for revocation when vault creation fails', async () => { + it('should not queue old token for revocation when vault update fails', async () => { const mockToprfEncryptor = createMockToprfEncryptor(); const MOCK_ENC_KEY = mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); const MOCK_PW_ENC_KEY = mockToprfEncryptor.derivePwEncKey(MOCK_PASSWORD); @@ -6483,15 +6561,37 @@ describe('SeedlessOnboardingController', () => { vaultEncryptionSalt: mockResult.vaultEncryptionSalt, }), }, - async ({ controller, encryptor }) => { - // Force vault re-encryption to fail so #createNewVaultWithAuthData throws. + async ({ controller, toprfClient, encryptor, mockRefreshJWTToken }) => { + // Unlock vault so #cachedDecryptedVaultData is populated. + await controller.submitPassword(MOCK_PASSWORD); + + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + // Let the first vault update (new accessToken) succeed; fail the + // second (new revokeToken inside #rotateRefreshToken). + const originalEncryptWithKey = + encryptor.encryptWithKey.bind(encryptor); + let vaultUpdateCallCount = 0; jest .spyOn(encryptor, 'encryptWithKey') - .mockRejectedValueOnce(new Error('Storage full')); + .mockImplementation(async (...args) => { + vaultUpdateCallCount += 1; + if (vaultUpdateCallCount === 2) { + throw new Error('Storage full'); + } + return originalEncryptWithKey(...args); + }); - await expect( - controller.renewRefreshToken(MOCK_PASSWORD), - ).rejects.toThrow('Storage full'); + // refreshAuthTokens still resolves — the rotation error is swallowed. + await controller.refreshAuthTokens(); // The old token must NOT be in pendingToBeRevokedTokens. Queueing it // before confirming the new vault was persisted would schedule the @@ -6502,62 +6602,6 @@ describe('SeedlessOnboardingController', () => { }, ); }); - - it('should work without a password when vault is already unlocked', async () => { - const mockToprfEncryptor = createMockToprfEncryptor(); - const MOCK_ENCRYPTION_KEY = - mockToprfEncryptor.deriveEncKey(MOCK_PASSWORD); - const MOCK_PASSWORD_ENCRYPTION_KEY = - mockToprfEncryptor.derivePwEncKey(MOCK_PASSWORD); - const MOCK_AUTH_KEY_PAIR = - mockToprfEncryptor.deriveAuthKeyPair(MOCK_PASSWORD); - - const mockResult = await createMockVault( - MOCK_ENCRYPTION_KEY, - MOCK_PASSWORD_ENCRYPTION_KEY, - MOCK_AUTH_KEY_PAIR, - MOCK_PASSWORD, - MOCK_REVOKE_TOKEN, - ); - - await withController( - { - state: getMockInitialControllerState({ - withMockAuthenticatedUser: true, - vault: mockResult.encryptedMockVault, - vaultEncryptionKey: mockResult.vaultEncryptionKey, - vaultEncryptionSalt: mockResult.vaultEncryptionSalt, - }), - }, - async ({ controller, mockRenewRefreshToken }) => { - // Unlock the vault first so #cachedDecryptedVaultData is populated. - await controller.submitPassword(MOCK_PASSWORD); - - // Call without a password — should use cached vault data directly. - await controller.renewRefreshToken(); - - expect(mockRenewRefreshToken).toHaveBeenCalledWith({ - connection: controller.state.authConnection, - revokeToken: MOCK_REVOKE_TOKEN, - }); - }, - ); - }); - - it('should throw VaultError when vault is locked and no password is provided', async () => { - await withController( - { - state: getMockInitialControllerState({ - withMockAuthenticatedUser: true, - }), - }, - async ({ controller }) => { - await expect(controller.renewRefreshToken()).rejects.toThrow( - SeedlessOnboardingControllerErrorMessage.VaultError, - ); - }, - ); - }); }); describe('revokePendingRefreshTokens', () => { diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index fccf90321ae..332bd2418af 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -2122,15 +2122,24 @@ export class SeedlessOnboardingController< * already in-progress the returned promise resolves when that request settles * rather than firing a duplicate request with the same token. * + * @param options - Optional flags. + * @param options.skipRenewRefreshToken - When `true`, the proactive + * refresh-token rotation that normally runs after a successful vault update + * is skipped. Useful in contexts where the caller will handle rotation + * separately or wants to avoid the extra network round-trip. * @returns A promise that resolves when the tokens have been refreshed. */ - async refreshAuthTokens(): Promise { + async refreshAuthTokens(options?: { + skipRenewRefreshToken?: boolean; + }): Promise { if (this.#pendingRefreshPromise) { return this.#pendingRefreshPromise; } - this.#pendingRefreshPromise = this.#doRefreshAuthTokens().finally(() => { - this.#pendingRefreshPromise = undefined; - }); + this.#pendingRefreshPromise = this.#doRefreshAuthTokens(options).finally( + () => { + this.#pendingRefreshPromise = undefined; + }, + ); return this.#pendingRefreshPromise; } @@ -2138,8 +2147,14 @@ export class SeedlessOnboardingController< * Internal implementation of token refresh. Called exclusively by * `refreshAuthTokens` which gates concurrent access via * `#pendingRefreshPromise`. + * + * @param options - Optional flags forwarded from `refreshAuthTokens`. + * @param options.skipRenewRefreshToken - Skip the proactive refresh-token + * rotation after a successful vault update. */ - async #doRefreshAuthTokens(): Promise { + async #doRefreshAuthTokens(options?: { + skipRenewRefreshToken?: boolean; + }): Promise { this.#assertIsAuthenticatedUser(this.state); const { refreshToken } = this.state; @@ -2168,22 +2183,12 @@ export class SeedlessOnboardingController< try { const { idTokens, accessToken, metadataAccessToken } = res; - // Re-authenticate with the new id tokens to set new node auth tokens. - // NOTE: revokeToken is intentionally omitted — refreshAuthTokens can be - // called while the vault is locked, so we never have access to it here. - // 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. - await this.authenticate({ + // Re-authenticate with the refreshed id tokens to update node auth + // tokens, accessToken, and metadataAccessToken in state. + await this.#reAuthenticate({ idTokens, accessToken, metadataAccessToken, - authConnection: this.state.authConnection, - authConnectionId: this.state.authConnectionId, - groupedAuthConnectionId: this.state.groupedAuthConnectionId, - userId: this.state.userId, - skipLock: true, }); // update the vault with new access token if wallet is unlocked @@ -2199,17 +2204,16 @@ export class SeedlessOnboardingController< pwEncKey, }); - // Proactively rotate the refresh token now that we have vault access. - // skipLock: true prevents deadlock when #doRefreshAuthTokens is invoked - // from within a #withControllerLock context via #executeWithTokenRefresh. - await this.renewRefreshToken(undefined, { skipLock: true }).catch( - (error) => { + // Proactively rotate the refresh token now that we have vault access, + // unless the caller has opted out. + if (!options?.skipRenewRefreshToken) { + await this.#rotateRefreshToken().catch((error) => { log( - 'Error renewing refresh token after successful JWT refresh', + 'Error rotating refresh token after successful JWT refresh', error, ); - }, - ); + }); + } } } catch (error) { log('Error refreshing node auth tokens', error); @@ -2223,116 +2227,121 @@ export class SeedlessOnboardingController< } /** - * Renew the refresh token - get new refresh token and new revoke token - * and also updates the vault with the new revoke token. - * This method is to be called after user is authenticated. + * Re-authenticate the user using freshly issued tokens from a JWT refresh. * - * When the vault is already unlocked (e.g. called from `#doRefreshAuthTokens` - * after a JWT refresh), `password` may be omitted — the cached vault data is - * used directly, avoiding a redundant and expensive password-based vault - * decryption. Pass `skipLock: true` in that case to prevent a deadlock when - * the caller is already executing inside `#withControllerLock`. + * Unlike the public `authenticate` method, this variant is called exclusively + * from token-refresh paths where the user's identity has already been + * established. It only accepts the tokens returned by the JWT-refresh + * service (`idTokens`, `accessToken`, `metadataAccessToken`) and reads all + * other auth-connection details from the existing controller state. * - * @param password - The password to decrypt/re-encrypt the vault. Required - * when the vault is locked; may be omitted when the vault is already unlocked. - * @param options - Optional flags. - * @param options.skipLock - Skip acquiring `#withControllerLock`. Use when - * the caller already holds the lock or when the lock must not be re-acquired - * (e.g. from inside `#doRefreshAuthTokens` which can run within a locked - * operation via `#executeWithTokenRefresh`). - * @returns A Promise that resolves to void. + * `refreshToken` and `revokeToken` are intentionally absent — token-refresh + * paths must not touch `state.refreshToken` directly; that field is managed + * exclusively by `#rotateRefreshToken`. + * + * @param params - Tokens issued by the JWT-refresh service. + * @param params.idTokens - New node id tokens. + * @param params.accessToken - New access token. + * @param params.metadataAccessToken - New metadata access token. + * @returns A promise that resolves to the authentication result. */ - async renewRefreshToken( - password?: string, - options?: { skipLock?: boolean }, - ): Promise { - const doRenew = async (): Promise => { - this.#assertIsAuthenticatedUser(this.state); - const { refreshToken } = this.state; + async #reAuthenticate(params: { + idTokens: string[]; + accessToken: string; + metadataAccessToken: string; + }): Promise { + const { idTokens, accessToken, metadataAccessToken } = params; + const { + authConnection, + authConnectionId, + groupedAuthConnectionId, + userId, + socialLoginEmail, + } = this.state; - let rawToprfEncryptionKey: Uint8Array; - let rawToprfPwEncryptionKey: Uint8Array; - let rawToprfAuthKeyPair: KeyPair; - let revokeToken: string; - // Captured when vault is already unlocked; used to build the updated - // vault data without a password re-encryption round-trip. - let unlockedVaultData: DeserializedVaultData | undefined; + try { + const authenticationResult = await this.toprfClient.authenticate({ + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + authConnectionId: authConnectionId!, + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + userId: userId!, + idTokens, + groupedAuthConnectionId, + }); - if (this.#isUnlocked && this.#cachedDecryptedVaultData) { - // Vault is already unlocked — use cached vault data directly to avoid - // a redundant (and expensive) password-based decryption round-trip. - unlockedVaultData = this.#cachedDecryptedVaultData; - ({ - toprfEncryptionKey: rawToprfEncryptionKey, - toprfPwEncryptionKey: rawToprfPwEncryptionKey, - toprfAuthKeyPair: rawToprfAuthKeyPair, - revokeToken, - } = unlockedVaultData); - } else { - if (!password) { - // Vault is locked and no password provided — cannot proceed. - throw new SeedlessOnboardingError( - SeedlessOnboardingControllerErrorMessage.VaultError, - { - cause: new Error('Vault is locked and no password provided'), - }, - ); - } - ({ - toprfEncryptionKey: rawToprfEncryptionKey, - toprfPwEncryptionKey: rawToprfPwEncryptionKey, - toprfAuthKeyPair: rawToprfAuthKeyPair, - revokeToken, - } = await this.#unlockVaultAndGetVaultData({ - password, - })); - } + this.update((state) => { + state.nodeAuthTokens = authenticationResult.nodeAuthTokens; + state.authConnection = authConnection; + state.authConnectionId = authConnectionId; + state.groupedAuthConnectionId = groupedAuthConnectionId; + state.userId = userId; + state.socialLoginEmail = socialLoginEmail; + state.metadataAccessToken = metadataAccessToken; + state.accessToken = accessToken; + assertIsSeedlessOnboardingUserAuthenticated(state); + state.isSeedlessOnboardingUserAuthenticated = true; + }); - const { newRevokeToken, newRefreshToken } = await this.#renewRefreshToken( + return authenticationResult; + } catch (error) { + log('Error re-authenticating user', error); + throw new SeedlessOnboardingError( + SeedlessOnboardingControllerErrorMessage.AuthenticationError, { - connection: this.state.authConnection, - revokeToken, + cause: error, }, ); + } + } - if (newRevokeToken && newRefreshToken) { - this.update((state) => { - // set new revoke token in state temporarily for persisting in vault - state.revokeToken = newRevokeToken; - // set new refresh token to persist in state - state.refreshToken = newRefreshToken; - }); + /** + * Rotate the refresh token — fetch a new refresh/revoke token pair from the + * auth service and persist the new revoke token in the vault. + * + * This method is private and is called exclusively from `#doRefreshAuthTokens` + * after a successful JWT refresh, at which point the vault is guaranteed to + * be unlocked and `#cachedDecryptedVaultData` is populated. + * + * @returns A Promise that resolves to void. + */ + async #rotateRefreshToken(): Promise { + this.#assertIsAuthenticatedUser(this.state); + // Callers are responsible for ensuring the vault is unlocked before + // invoking this method. This assertion is a safety net only. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const vaultData = this.#cachedDecryptedVaultData!; + const { refreshToken } = this.state; + const { toprfPwEncryptionKey: pwEncKey, revokeToken } = vaultData; - if (password) { - await this.#createNewVaultWithAuthData({ - password, - rawToprfEncryptionKey, - rawToprfPwEncryptionKey, - rawToprfAuthKeyPair, - }); - } else if (unlockedVaultData) { - // Vault was already unlocked — re-encrypt in-place using the - // existing vault encryption key (no password KDF needed). - await this.#updateVault({ - vaultData: { - ...unlockedVaultData, - revokeToken: newRevokeToken, - }, - pwEncKey: rawToprfPwEncryptionKey, - }); - } - // add the old refresh token to the list to be revoked later when possible - this.#addRefreshTokenToRevokeList({ - refreshToken, - revokeToken, - }); - } - }; + const { newRevokeToken, newRefreshToken } = await this.#renewRefreshToken({ + connection: this.state.authConnection, + revokeToken, + }); - if (options?.skipLock) { - return await doRenew(); + if (newRevokeToken && newRefreshToken) { + this.update((state) => { + // set new revoke token in state temporarily for persisting in vault + state.revokeToken = newRevokeToken; + // set new refresh token to persist in state + state.refreshToken = newRefreshToken; + }); + + // Vault was already unlocked — re-encrypt in-place using the + // existing vault encryption key (no password KDF needed). + await this.#updateVault({ + vaultData: { + ...vaultData, + revokeToken: newRevokeToken, + }, + pwEncKey, + }); + + // add the old refresh token to the list to be revoked later when possible + this.#addRefreshTokenToRevokeList({ + refreshToken, + revokeToken, + }); } - return await this.#withControllerLock(doRenew); } /** From 71ca37769528f27ed7d2b5d81a3ee694fdbd4a5f Mon Sep 17 00:00:00 2001 From: himanshu Date: Wed, 11 Mar 2026 14:35:19 +0800 Subject: [PATCH 5/9] fix(seedless-onboarding): address cursor bot review comments - 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 --- .../CHANGELOG.md | 1 - .../src/SeedlessOnboardingController.ts | 23 ++++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index e959724da05..cf7fd6b2d76 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -9,7 +9,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `authenticate` now accepts an optional `refreshToken` parameter — it is only written to state when provided, so token-refresh paths cannot silently overwrite a concurrently-rotated refresh token ([#8148](https://github.com/MetaMask/core/pull/8148)) - Token refresh now uses a dedicated private `#reAuthenticate` method that only accepts the tokens returned by the JWT-refresh service (`idTokens`, `accessToken`, `metadataAccessToken`), making the re-authentication path stricter than the initial `authenticate` call ([#8148](https://github.com/MetaMask/core/pull/8148)) - After a successful JWT refresh the controller proactively rotates the refresh/revoke token pair (when the vault is unlocked) via a new private `#rotateRefreshToken` method ([#8148](https://github.com/MetaMask/core/pull/8148)) - `refreshAuthTokens` now accepts an optional `skipRenewRefreshToken` flag to opt out of the proactive token rotation ([#8148](https://github.com/MetaMask/core/pull/8148)) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 332bd2418af..3ea9b2e4044 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -463,9 +463,10 @@ export class SeedlessOnboardingController< ); } - // Check if token is expired and refresh if needed + // Use the same 90%-lifetime threshold as checkMetadataAccessTokenExpired + // so both code paths agree on when a refresh is needed. const decodedToken = decodeJWTToken(metadataAccessToken); - if (decodedToken.exp < Math.floor(Date.now() / 1000)) { + if (isTokenNearExpiry(decodedToken.exp, decodedToken.iat)) { // Token is expired, refresh it await this.refreshAuthTokens(); @@ -503,7 +504,7 @@ export class SeedlessOnboardingController< * @param params.userId - user email or id from Social login * @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 - Optional refresh token issued during OAuth login. When provided it is + * @param params.refreshToken - refresh token issued during OAuth login. When provided it is * written to state. Omit when calling from token-refresh paths (e.g. `#doRefreshAuthTokens`) so * that a concurrent `renewRefreshToken` rotation is never silently overwritten. * @param params.revokeToken - revoke token for revoking refresh token and get new refresh token and new revoke token. @@ -521,7 +522,7 @@ export class SeedlessOnboardingController< userId: string; groupedAuthConnectionId?: string; socialLoginEmail?: string; - refreshToken?: string; + refreshToken: string; revokeToken?: string; skipLock?: boolean; }): Promise { @@ -555,12 +556,7 @@ export class SeedlessOnboardingController< state.authConnection = authConnection; state.socialLoginEmail = socialLoginEmail; state.metadataAccessToken = metadataAccessToken; - // Only write refreshToken when a new value is explicitly provided (i.e. during - // OAuth login). Token-refresh paths omit this field so that a concurrent - // renewRefreshToken rotation is never silently overwritten. - if (refreshToken) { - state.refreshToken = refreshToken; - } + state.refreshToken = refreshToken; if (revokeToken) { // Temporarily store revoke token & access token in state for later vault creation state.revokeToken = revokeToken; @@ -2611,8 +2607,13 @@ function isTokenNearExpiry(exp: number, iat?: number): boolean { if (iat === undefined) { return now >= exp; } - const remaining = exp - now; const lifetime = exp - iat; + // Guard against malformed tokens where iat >= exp (zero or negative lifetime). + // Fall back to exact-expiry check so bad tokens are always considered stale. + if (lifetime <= 0) { + return now >= exp; + } + const remaining = exp - now; return remaining <= 0.1 * lifetime; } From 73d482b5b5322174ae1c47751f68e5d0be9b0cae Mon Sep 17 00:00:00 2001 From: himanshu Date: Wed, 11 Mar 2026 14:52:31 +0800 Subject: [PATCH 6/9] fix(seedless-onboarding): don't coalesce refreshAuthTokens calls with 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. --- .../src/SeedlessOnboardingController.ts | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 3ea9b2e4044..586c65ab063 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -2128,15 +2128,26 @@ export class SeedlessOnboardingController< async refreshAuthTokens(options?: { skipRenewRefreshToken?: boolean; }): Promise { - if (this.#pendingRefreshPromise) { + // Only coalesce concurrent calls that use default options. A caller with + // explicit options must not silently inherit a different in-flight + // request's behaviour (e.g. a skipRenewRefreshToken:true call must not be + // served by an in-flight request that will perform rotation). + const hasExplicitOptions = options?.skipRenewRefreshToken !== undefined; + if (!hasExplicitOptions && this.#pendingRefreshPromise) { return this.#pendingRefreshPromise; } - this.#pendingRefreshPromise = this.#doRefreshAuthTokens(options).finally( - () => { + + const promise = this.#doRefreshAuthTokens(options).finally(() => { + if (this.#pendingRefreshPromise === promise) { this.#pendingRefreshPromise = undefined; - }, - ); - return this.#pendingRefreshPromise; + } + }); + + if (!hasExplicitOptions) { + this.#pendingRefreshPromise = promise; + } + + return promise; } /** From 358e1b9d48e9e30497acad43ff9f0c0c2f4292d6 Mon Sep 17 00:00:00 2001 From: himanshu Date: Wed, 11 Mar 2026 15:04:00 +0800 Subject: [PATCH 7/9] chore(seedless-onboarding): mark breaking change in changelog --- .../seedless-onboarding-controller/CHANGELOG.md | 2 +- .../src/SeedlessOnboardingController.ts | 15 +++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index cf7fd6b2d76..e77914b21ff 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed -- Removed public `renewRefreshToken` method — refresh token rotation is now handled automatically by the controller after a successful JWT refresh ([#8148](https://github.com/MetaMask/core/pull/8148)) +- **BREAKING:** Removed public `renewRefreshToken` method — refresh token rotation is now handled automatically by the controller after a successful JWT refresh ([#8148](https://github.com/MetaMask/core/pull/8148)) ## [8.1.0] diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 586c65ab063..2757210dfe5 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -2128,24 +2128,19 @@ export class SeedlessOnboardingController< async refreshAuthTokens(options?: { skipRenewRefreshToken?: boolean; }): Promise { - // Only coalesce concurrent calls that use default options. A caller with - // explicit options must not silently inherit a different in-flight - // request's behaviour (e.g. a skipRenewRefreshToken:true call must not be - // served by an in-flight request that will perform rotation). - const hasExplicitOptions = options?.skipRenewRefreshToken !== undefined; - if (!hasExplicitOptions && this.#pendingRefreshPromise) { + // we should skip the refresh if a refresh is already in progress + // to avoid concurrent requests with the same refresh token + // it applies to both requests with or without the skipRenewRefreshToken flag + if (this.#pendingRefreshPromise) { return this.#pendingRefreshPromise; } - const promise = this.#doRefreshAuthTokens(options).finally(() => { if (this.#pendingRefreshPromise === promise) { this.#pendingRefreshPromise = undefined; } }); - if (!hasExplicitOptions) { - this.#pendingRefreshPromise = promise; - } + this.#pendingRefreshPromise = promise; return promise; } From 0574664f3470aabb6d01a58f3f552aaa3f12faf3 Mon Sep 17 00:00:00 2001 From: himanshu Date: Thu, 12 Mar 2026 19:00:24 +0800 Subject: [PATCH 8/9] refactor(seedless-onboarding): remove skipRenewRefreshToken, fix comments 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 --- .../CHANGELOG.md | 1 - .../src/SeedlessOnboardingController.test.ts | 170 +++++++++++++++--- .../src/SeedlessOnboardingController.ts | 97 +++++----- .../src/constants.ts | 1 + 4 files changed, 196 insertions(+), 73 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index e77914b21ff..f464a90585f 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -11,7 +11,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Token refresh now uses a dedicated private `#reAuthenticate` method that only accepts the tokens returned by the JWT-refresh service (`idTokens`, `accessToken`, `metadataAccessToken`), making the re-authentication path stricter than the initial `authenticate` call ([#8148](https://github.com/MetaMask/core/pull/8148)) - After a successful JWT refresh the controller proactively rotates the refresh/revoke token pair (when the vault is unlocked) via a new private `#rotateRefreshToken` method ([#8148](https://github.com/MetaMask/core/pull/8148)) -- `refreshAuthTokens` now accepts an optional `skipRenewRefreshToken` flag to opt out of the proactive token rotation ([#8148](https://github.com/MetaMask/core/pull/8148)) - Token expiry checks now use a 90% lifetime threshold for proactive refresh: access tokens and metadata access tokens are refreshed when less than 10% of their lifetime remains (requires `iat`), while node auth tokens fall back to exact-expiry checking ([#8148](https://github.com/MetaMask/core/pull/8148)) ### Removed diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index c3f40e701ae..08bcaa27172 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -5721,7 +5721,7 @@ describe('SeedlessOnboardingController', () => { ); }); - it('should skip proactive token rotation when skipRenewRefreshToken is true', async () => { + it('should proactively call renewRefreshToken after vault update when vault is unlocked', async () => { await withController( { state: getMockInitialControllerState({ @@ -5750,14 +5750,34 @@ describe('SeedlessOnboardingController', () => { isNewUser: false, }); - await controller.refreshAuthTokens({ skipRenewRefreshToken: true }); + await controller.refreshAuthTokens(); + + expect(mockRenewRefreshToken).toHaveBeenCalled(); + }, + ); + }); + + it('should not call renewRefreshToken when vault is locked', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient, mockRenewRefreshToken }) => { + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ + nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, + isNewUser: false, + }); + + await controller.refreshAuthTokens(); expect(mockRenewRefreshToken).not.toHaveBeenCalled(); }, ); }); - it('should proactively call renewRefreshToken after vault update when vault is unlocked', async () => { + it('should swallow renewRefreshToken errors and still resolve when vault is unlocked', async () => { await withController( { state: getMockInitialControllerState({ @@ -5786,21 +5806,39 @@ describe('SeedlessOnboardingController', () => { isNewUser: false, }); - await controller.refreshAuthTokens(); + // Make the proactive renewal fail — the error is caught and logged. + mockRenewRefreshToken.mockRejectedValueOnce( + new Error('renewal failed'), + ); - expect(mockRenewRefreshToken).toHaveBeenCalled(); + // refreshAuthTokens must still resolve even when renewal fails. + expect(await controller.refreshAuthTokens()).toBeUndefined(); }, ); }); - it('should not call renewRefreshToken when vault is locked', async () => { + it('should queue old refresh token for revocation after successful rotation', async () => { await withController( { state: getMockInitialControllerState({ withMockAuthenticatedUser: true, + vault: encryptedMockVault, + vaultEncryptionKey, + vaultEncryptionSalt, }), }, - async ({ controller, toprfClient, mockRenewRefreshToken }) => { + async ({ controller, toprfClient, mockRefreshJWTToken }) => { + await controller.submitPassword(MOCK_PASSWORD); + + const originalRefreshToken = controller.state.refreshToken; + const originalRevokeToken = controller.state.revokeToken; + + mockRefreshJWTToken.mockResolvedValueOnce({ + idTokens: ['newIdToken'], + metadataAccessToken: 'mock-metadata-access-token', + accessToken, + }); + jest.spyOn(toprfClient, 'authenticate').mockResolvedValue({ nodeAuthTokens: MOCK_NODE_AUTH_TOKENS, isNewUser: false, @@ -5808,12 +5846,21 @@ describe('SeedlessOnboardingController', () => { await controller.refreshAuthTokens(); - expect(mockRenewRefreshToken).not.toHaveBeenCalled(); + // After successful rotation the old token pair should be queued + // for background revocation. + expect(controller.state.pendingToBeRevokedTokens).toStrictEqual( + expect.arrayContaining([ + { + refreshToken: originalRefreshToken, + revokeToken: originalRevokeToken, + }, + ]), + ); }, ); }); - it('should swallow renewRefreshToken errors and still resolve when vault is unlocked', async () => { + it('should update state.refreshToken after successful rotation', async () => { await withController( { state: getMockInitialControllerState({ @@ -5823,14 +5870,11 @@ describe('SeedlessOnboardingController', () => { vaultEncryptionSalt, }), }, - async ({ - controller, - toprfClient, - mockRenewRefreshToken, - mockRefreshJWTToken, - }) => { + async ({ controller, toprfClient, mockRefreshJWTToken }) => { await controller.submitPassword(MOCK_PASSWORD); + const originalRefreshToken = controller.state.refreshToken; + mockRefreshJWTToken.mockResolvedValueOnce({ idTokens: ['newIdToken'], metadataAccessToken: 'mock-metadata-access-token', @@ -5842,13 +5886,13 @@ describe('SeedlessOnboardingController', () => { isNewUser: false, }); - // Make the proactive renewal fail — the error is caught and logged. - mockRenewRefreshToken.mockRejectedValueOnce( - new Error('renewal failed'), - ); + await controller.refreshAuthTokens(); - // refreshAuthTokens must still resolve even when renewal fails. - expect(await controller.refreshAuthTokens()).toBeUndefined(); + // #rotateRefreshToken writes the new refresh token to state. + expect(controller.state.refreshToken).not.toBe( + originalRefreshToken, + ); + expect(controller.state.refreshToken).toBe('newRefreshToken'); }, ); }); @@ -5923,6 +5967,63 @@ describe('SeedlessOnboardingController', () => { expect(controller.refreshAuthTokens).toHaveBeenCalled(); }); + + it('should call refreshAuthTokens when 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; 9 500 s have elapsed → past the 90 % mark + const nearExpiryToken = createMockJWTToken({ + iat: now - 9500, + exp: now + 500, + }); + const { messenger } = mockSeedlessOnboardingMessenger(); + const controller = new SeedlessOnboardingController({ + messenger, + encryptor: createMockVaultEncryptor(), + refreshJWTToken: jest.fn(), + revokeRefreshToken: jest.fn(), + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + metadataAccessToken: nearExpiryToken, + }), + renewRefreshToken: jest.fn(), + }); + + jest.spyOn(controller, 'refreshAuthTokens').mockResolvedValue(); + + await controller.fetchMetadataAccessCreds(); + + expect(controller.refreshAuthTokens).toHaveBeenCalled(); + }); + + it('should not call refreshAuthTokens when less than 90% of token lifetime has elapsed', async () => { + const now = Math.floor(Date.now() / 1000); + // 10 000 s lifetime; only 3 000 s have elapsed → well below 90 % + const freshToken = createMockJWTToken({ + iat: now - 3000, + exp: now + 7000, + }); + const { messenger } = mockSeedlessOnboardingMessenger(); + const controller = new SeedlessOnboardingController({ + messenger, + encryptor: createMockVaultEncryptor(), + refreshJWTToken: jest.fn(), + revokeRefreshToken: jest.fn(), + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + metadataAccessToken: freshToken, + }), + renewRefreshToken: jest.fn(), + }); + + jest.spyOn(controller, 'refreshAuthTokens').mockResolvedValue(); + + const result = await controller.fetchMetadataAccessCreds(); + + expect(controller.refreshAuthTokens).not.toHaveBeenCalled(); + expect(result).toStrictEqual({ + metadataAccessToken: freshToken, + }); + }); }); describe('checkMetadataAccessTokenExpired', () => { @@ -6030,6 +6131,33 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should return true when token has zero lifetime (iat === exp)', async () => { + const now = Math.floor(Date.now() / 1000); + // Zero lifetime: iat === exp. Falls back to exact-expiry check; + // exp is in the past so returns true. + const malformedToken = createMockJWTToken({ + iat: now - 10, + exp: now - 10, + }); + + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + metadataAccessToken: malformedToken, + }), + }, + async ({ controller }) => { + jest + .spyOn(controller, 'checkMetadataAccessTokenExpired') + .mockRestore(); + + const result = controller.checkMetadataAccessTokenExpired(); + expect(result).toBe(true); + }, + ); + }); + it('should return true if user is not authenticated', async () => { await withController(async ({ controller }) => { // Restore the original implementation to test the real logic diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 2757210dfe5..e4ccca1a66b 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -467,7 +467,7 @@ export class SeedlessOnboardingController< // so both code paths agree on when a refresh is needed. const decodedToken = decodeJWTToken(metadataAccessToken); if (isTokenNearExpiry(decodedToken.exp, decodedToken.iat)) { - // Token is expired, refresh it + // Token is near expiry or already expired, refresh it await this.refreshAuthTokens(); // Get the new token after refresh @@ -504,9 +504,7 @@ export class SeedlessOnboardingController< * @param params.userId - user email or id from Social login * @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 issued during OAuth login. When provided it is - * written to state. Omit when calling from token-refresh paths (e.g. `#doRefreshAuthTokens`) so - * that a concurrent `renewRefreshToken` rotation is never silently overwritten. + * @param params.refreshToken - Refresh token issued during OAuth login. Written to state when provided. * @param params.revokeToken - revoke token for revoking refresh token and get new refresh token and new revoke token. * @param params.accessToken - Access token for pairing with profile sync auth service and to access other services. * @param params.metadataAccessToken - Metadata access token for accessing the metadata service before the vault is created or unlocked. @@ -2118,23 +2116,16 @@ export class SeedlessOnboardingController< * already in-progress the returned promise resolves when that request settles * rather than firing a duplicate request with the same token. * - * @param options - Optional flags. - * @param options.skipRenewRefreshToken - When `true`, the proactive - * refresh-token rotation that normally runs after a successful vault update - * is skipped. Useful in contexts where the caller will handle rotation - * separately or wants to avoid the extra network round-trip. * @returns A promise that resolves when the tokens have been refreshed. */ - async refreshAuthTokens(options?: { - skipRenewRefreshToken?: boolean; - }): Promise { - // we should skip the refresh if a refresh is already in progress - // to avoid concurrent requests with the same refresh token - // it applies to both requests with or without the skipRenewRefreshToken flag + async refreshAuthTokens(): Promise { + // Coalesce concurrent calls to avoid issuing parallel HTTP requests + // with the same refresh token. if (this.#pendingRefreshPromise) { return this.#pendingRefreshPromise; } - const promise = this.#doRefreshAuthTokens(options).finally(() => { + + const promise = this.#doRefreshAuthTokens().finally(() => { if (this.#pendingRefreshPromise === promise) { this.#pendingRefreshPromise = undefined; } @@ -2149,14 +2140,8 @@ export class SeedlessOnboardingController< * Internal implementation of token refresh. Called exclusively by * `refreshAuthTokens` which gates concurrent access via * `#pendingRefreshPromise`. - * - * @param options - Optional flags forwarded from `refreshAuthTokens`. - * @param options.skipRenewRefreshToken - Skip the proactive refresh-token - * rotation after a successful vault update. */ - async #doRefreshAuthTokens(options?: { - skipRenewRefreshToken?: boolean; - }): Promise { + async #doRefreshAuthTokens(): Promise { this.#assertIsAuthenticatedUser(this.state); const { refreshToken } = this.state; @@ -2206,16 +2191,18 @@ export class SeedlessOnboardingController< pwEncKey, }); - // Proactively rotate the refresh token now that we have vault access, - // unless the caller has opted out. - if (!options?.skipRenewRefreshToken) { - await this.#rotateRefreshToken().catch((error) => { - log( - 'Error rotating refresh token after successful JWT refresh', - error, - ); - }); - } + // Proactively rotate the refresh token now that we have vault access. + await this.#rotateRefreshToken().catch((error) => { + // Rotation failure is intentionally non-fatal: the JWT refresh + // itself succeeded and the caller should not be blocked. + // However the user is now operating with a stale refresh token + // that may be revoked server-side, so log prominently. + log( + 'Failed to rotate refresh token after successful JWT refresh. ' + + 'The user may be logged out when the old token is revoked.', + error, + ); + }); } } catch (error) { log('Error refreshing node auth tokens', error); @@ -2308,28 +2295,27 @@ export class SeedlessOnboardingController< */ async #rotateRefreshToken(): Promise { this.#assertIsAuthenticatedUser(this.state); - // Callers are responsible for ensuring the vault is unlocked before - // invoking this method. This assertion is a safety net only. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const vaultData = this.#cachedDecryptedVaultData!; + + const vaultData = this.#cachedDecryptedVaultData; + // Safety net: the caller (#doRefreshAuthTokens) already guards with + // `this.#isUnlocked && this.#cachedDecryptedVaultData`, so this branch + // is unreachable in normal flow. + /* istanbul ignore if */ + if (!vaultData) { + throw new SeedlessOnboardingError( + SeedlessOnboardingControllerErrorMessage.VaultLocked, + ); + } const { refreshToken } = this.state; const { toprfPwEncryptionKey: pwEncKey, revokeToken } = vaultData; - const { newRevokeToken, newRefreshToken } = await this.#renewRefreshToken({ connection: this.state.authConnection, revokeToken, }); if (newRevokeToken && newRefreshToken) { - this.update((state) => { - // set new revoke token in state temporarily for persisting in vault - state.revokeToken = newRevokeToken; - // set new refresh token to persist in state - state.refreshToken = newRefreshToken; - }); - - // Vault was already unlocked — re-encrypt in-place using the - // existing vault encryption key (no password KDF needed). + // 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({ vaultData: { ...vaultData, @@ -2338,6 +2324,11 @@ export class SeedlessOnboardingController< pwEncKey, }); + this.update((state) => { + state.revokeToken = newRevokeToken; + state.refreshToken = newRefreshToken; + }); + // add the old refresh token to the list to be revoked later when possible this.#addRefreshTokenToRevokeList({ refreshToken, @@ -2559,9 +2550,11 @@ export class SeedlessOnboardingController< } /** - * Check if the current metadata access token is expired. + * Check if the current metadata access token should be refreshed. + * Returns true when the token is expired or when less than 10% of its + * lifetime remains (proactive refresh). * - * @returns True if the metadata access token is expired, false otherwise. + * @returns True if the metadata access token should be refreshed, false otherwise. */ public checkMetadataAccessTokenExpired(): boolean { try { @@ -2576,10 +2569,12 @@ export class SeedlessOnboardingController< } /** - * Check if the current access token is expired. + * Check if the current access token should be refreshed. + * Returns true when the token is expired or when less than 10% of its + * lifetime remains (proactive refresh). * When the vault is locked, the access token is not accessible, so we return false. * - * @returns True if the access token is expired, false otherwise. + * @returns True if the access token should be refreshed, false otherwise. */ public checkAccessTokenExpired(): boolean { try { diff --git a/packages/seedless-onboarding-controller/src/constants.ts b/packages/seedless-onboarding-controller/src/constants.ts index 4ceb62c3930..7338ec17bb8 100644 --- a/packages/seedless-onboarding-controller/src/constants.ts +++ b/packages/seedless-onboarding-controller/src/constants.ts @@ -26,6 +26,7 @@ export enum SecretMetadataVersion { export enum SeedlessOnboardingControllerErrorMessage { ControllerLocked = `${controllerName} - The operation cannot be completed while the controller is locked.`, + VaultLocked = `${controllerName} - The operation cannot be completed while the vault is locked.`, AuthenticationError = `${controllerName} - Authentication error`, MissingAuthUserInfo = `${controllerName} - Missing authenticated user information`, FailedToPersistOprfKey = `${controllerName} - Failed to persist OPRF key`, From d3b0814a768bc6bb32f2acb9644cad9f61256a45 Mon Sep 17 00:00:00 2001 From: himanshu Date: Thu, 12 Mar 2026 19:08:15 +0800 Subject: [PATCH 9/9] code comments updated --- .../src/SeedlessOnboardingController.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index e4ccca1a66b..aa31c4180e7 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -2287,9 +2287,7 @@ export class SeedlessOnboardingController< * Rotate the refresh token — fetch a new refresh/revoke token pair from the * auth service and persist the new revoke token in the vault. * - * This method is private and is called exclusively from `#doRefreshAuthTokens` - * after a successful JWT refresh, at which point the vault is guaranteed to - * be unlocked and `#cachedDecryptedVaultData` is populated. + * This method should be called after a successful JWT refresh. * * @returns A Promise that resolves to void. */ @@ -2299,7 +2297,8 @@ export class SeedlessOnboardingController< const vaultData = this.#cachedDecryptedVaultData; // Safety net: the caller (#doRefreshAuthTokens) already guards with // `this.#isUnlocked && this.#cachedDecryptedVaultData`, so this branch - // is unreachable in normal flow. + // is unreachable in normal flow. This check is added to satisfy the linter and + // to make sure any future caller of this method does not bypass the check. /* istanbul ignore if */ if (!vaultData) { throw new SeedlessOnboardingError(