Skip to content

Commit f234cee

Browse files
authored
fix: resolve AuthenticationController 401 errors from token caching bugs (#8144)
## Explanation ⚠️ This is a breaking change, and clients will need to be updated around the E2E setup area. Needed changes are in the test-drive PRs below. Extension test-drive PR: MetaMask/metamask-extension#40711 ## References Related to https://consensyssoftware.atlassian.net/browse/MUL-1549 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [x] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **High Risk** > Touches authentication session validation and token retrieval behavior (including breaking test/mock contract), which can affect login/coalescing and any consumers relying on cached tokens. > > **Overview** > Fixes profile auth/token caching edge-cases that could lead to **stale bearer tokens (401s)**. > > `profile-sync-controller` now validates cached login sessions by decoding the JWT `exp` claim (rejecting expired/malformed/non-JWT tokens) and resolves `undefined` `entropySourceId` to the *primary* SRP ID (cached across calls and cleared on sign-out), eliminating duplicate logins caused by `undefined` vs explicit primary IDs; `getUserProfileLineage` is updated to accept an optional `entropySourceId` end-to-end. > > `profile-metrics-controller` moves `AuthenticationController:getBearerToken` acquisition inside the retry execution so each retry fetches a fresh token. E2E/test mocks are updated to wrap mock identifiers in JWT-shaped tokens and provide `getE2EIdentifierFromJwt` to extract the original identifier. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 884180c. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 19fdf2a commit f234cee

File tree

15 files changed

+466
-93
lines changed

15 files changed

+466
-93
lines changed

eslint-suppressions.json

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1365,7 +1365,7 @@
13651365
"count": 2
13661366
},
13671367
"@typescript-eslint/prefer-nullish-coalescing": {
1368-
"count": 2
1368+
"count": 1
13691369
}
13701370
},
13711371
"packages/profile-sync-controller/src/controllers/authentication/__fixtures__/mockServices.ts": {
@@ -1605,11 +1605,6 @@
16051605
"count": 1
16061606
}
16071607
},
1608-
"packages/profile-sync-controller/src/sdk/utils/validate-login-response.test.ts": {
1609-
"@typescript-eslint/explicit-function-return-type": {
1610-
"count": 1
1611-
}
1612-
},
16131608
"packages/profile-sync-controller/src/shared/encryption/cache.ts": {
16141609
"@typescript-eslint/explicit-function-return-type": {
16151610
"count": 1

packages/profile-metrics-controller/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- Move bearer token acquisition inside the retry loop in `ProfileMetricsService.submitMetrics` so each retry attempt fetches a fresh token instead of reusing a potentially stale one ([#8144](https://github.com/MetaMask/core/pull/8144))
13+
1014
## [3.0.2]
1115

1216
### Changed

packages/profile-metrics-controller/src/ProfileMetricsService.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,11 @@ export class ProfileMetricsService {
198198
* @returns The response from the API.
199199
*/
200200
async submitMetrics(data: ProfileMetricsSubmitMetricsRequest): Promise<void> {
201-
const authToken = await this.#messenger.call(
202-
'AuthenticationController:getBearerToken',
203-
data.entropySourceId ?? undefined,
204-
);
205201
await this.#policy.execute(async () => {
202+
const authToken = await this.#messenger.call(
203+
'AuthenticationController:getBearerToken',
204+
data.entropySourceId ?? undefined,
205+
);
206206
const url = new URL(`${this.#baseURL}/profile/accounts`);
207207
const localResponse = await this.#fetch(url, {
208208
method: 'PUT',

packages/profile-sync-controller/CHANGELOG.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2020

2121
### Changed
2222

23+
- **BREAKING:** Add client-side JWT `exp` claim validation to prevent stale cached tokens from being returned ([#8144](https://github.com/MetaMask/core/pull/8144))
24+
- `validateLoginResponse` now decodes the JWT `exp` claim and rejects tokens that have actually expired, regardless of client-side TTL tracking (`obtainedAt`/`expiresIn`)
25+
- Non-JWT access tokens are now rejected as invalid. In production this has no effect (access tokens are always JWTs from the OIDC server), but E2E test mocks that use raw identifier strings as access tokens must be updated. `getMockAuthAccessTokenResponse` now wraps identifiers in a JWT; consumers should use `getE2EIdentifierFromJwt` (newly exported) to extract the identifier from the bearer token in mock servers.
2326
- **BREAKING:** Standardize names of `AuthenticationController` and `UserStorageController` messenger action types ([#7976](https://github.com/MetaMask/core/pull/7976/))
2427
- All existing types for messenger actions have been renamed so they end in `Action` (e.g. `AuthenticationControllerPerformSignIn` -> `AuthenticationControllerPerformSignInAction`). You will need to update imports appropriately.
2528
- This change only affects the types. The action type strings themselves have not changed, so you do not need to update the list of actions you pass when initializing `AuthenticationController` and `UserStorageController` messengers.
2629

30+
### Fixed
31+
32+
- Fix `AuthenticationController` silently discarding tokens when `entropySourceId` is `undefined` ([#8144](https://github.com/MetaMask/core/pull/8144))
33+
- `getBearerToken`, `getSessionProfile`, and `getUserProfileLineage` now resolve `undefined` `entropySourceId` to the primary SRP entropy source ID via the message-signing snap before delegating to the auth SDK
34+
- This also eliminates a login deduplication race condition where `getBearerToken(undefined)` and `getBearerToken("primary-srp-id")` would trigger two independent OIDC logins for the same identity
35+
- Update `getUserProfileLineage` to accept an optional `entropySourceId` parameter ([#8144](https://github.com/MetaMask/core/pull/8144))
36+
2737
## [27.1.0]
2838

2939
### Changed

packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts

Lines changed: 93 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,95 @@ describe('AuthenticationController', () => {
355355
expect.any(Error),
356356
);
357357
});
358+
359+
it('resolves undefined entropySourceId to primary and stores token', async () => {
360+
const metametrics = createMockAuthMetaMetrics();
361+
const { messenger, mockSnapGetAllPublicKeys } =
362+
createMockAuthenticationMessenger();
363+
arrangeAuthAPIs();
364+
365+
const controller = new AuthenticationController({
366+
messenger,
367+
metametrics,
368+
});
369+
370+
const result = await controller.getBearerToken();
371+
expect(result).toBe(MOCK_OATH_TOKEN_RESPONSE.access_token);
372+
373+
expect(mockSnapGetAllPublicKeys).toHaveBeenCalled();
374+
expect(controller.state.isSignedIn).toBe(true);
375+
expect(
376+
controller.state.srpSessionData?.[MOCK_ENTROPY_SOURCE_IDS[0]],
377+
).toBeDefined();
378+
});
379+
380+
it('returns the same token for undefined and explicit primary entropySourceId', async () => {
381+
const metametrics = createMockAuthMetaMetrics();
382+
const { messenger } = createMockAuthenticationMessenger();
383+
const originalState = mockSignedInState();
384+
const controller = new AuthenticationController({
385+
messenger,
386+
state: originalState,
387+
metametrics,
388+
});
389+
390+
const resultUndefined = await controller.getBearerToken();
391+
const resultExplicit = await controller.getBearerToken(
392+
MOCK_ENTROPY_SOURCE_IDS[0],
393+
);
394+
expect(resultUndefined).toBe(resultExplicit);
395+
});
396+
397+
it('caches the primary entropySourceId resolution across calls', async () => {
398+
const metametrics = createMockAuthMetaMetrics();
399+
const { messenger, mockSnapGetAllPublicKeys } =
400+
createMockAuthenticationMessenger();
401+
const originalState = mockSignedInState();
402+
const controller = new AuthenticationController({
403+
messenger,
404+
state: originalState,
405+
metametrics,
406+
});
407+
408+
await controller.getBearerToken();
409+
await controller.getBearerToken();
410+
await controller.getBearerToken();
411+
412+
// getAllPublicKeys should only be called once despite multiple getBearerToken calls
413+
expect(mockSnapGetAllPublicKeys).toHaveBeenCalledTimes(1);
414+
});
415+
416+
it('throws when snap returns no entropy sources', async () => {
417+
const metametrics = createMockAuthMetaMetrics();
418+
const { messenger, mockSnapGetAllPublicKeys } =
419+
createMockAuthenticationMessenger();
420+
mockSnapGetAllPublicKeys.mockResolvedValue([]);
421+
422+
const controller = new AuthenticationController({
423+
messenger,
424+
metametrics,
425+
});
426+
427+
await expect(controller.getBearerToken()).rejects.toThrow(
428+
'No entropy sources found from snap',
429+
);
430+
});
431+
432+
it('throws when primary entropy source ID is undefined', async () => {
433+
const metametrics = createMockAuthMetaMetrics();
434+
const { messenger, mockSnapGetAllPublicKeys } =
435+
createMockAuthenticationMessenger();
436+
mockSnapGetAllPublicKeys.mockResolvedValue([[undefined, 'MOCK_KEY']]);
437+
438+
const controller = new AuthenticationController({
439+
messenger,
440+
metametrics,
441+
});
442+
443+
await expect(controller.getBearerToken()).rejects.toThrow(
444+
'Primary entropy source ID is undefined',
445+
);
446+
});
358447
});
359448

360449
describe('getSessionProfile', () => {
@@ -657,7 +746,7 @@ describe('metadata', () => {
657746
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
658747
},
659748
"token": {
660-
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
749+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjQxMDI0NDQ4MDB9.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
661750
"expiresIn": 1000,
662751
"obtainedAt": 0,
663752
},
@@ -669,7 +758,7 @@ describe('metadata', () => {
669758
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
670759
},
671760
"token": {
672-
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
761+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjQxMDI0NDQ4MDB9.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
673762
"expiresIn": 1000,
674763
"obtainedAt": 0,
675764
},
@@ -704,7 +793,7 @@ describe('metadata', () => {
704793
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
705794
},
706795
"token": {
707-
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
796+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjQxMDI0NDQ4MDB9.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
708797
"expiresIn": 1000,
709798
"obtainedAt": 0,
710799
},
@@ -716,7 +805,7 @@ describe('metadata', () => {
716805
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
717806
},
718807
"token": {
719-
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
808+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjQxMDI0NDQ4MDB9.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
720809
"expiresIn": 1000,
721810
"obtainedAt": 0,
722811
},

packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,8 @@ export class AuthenticationController extends BaseController<
141141

142142
#isUnlocked = false;
143143

144+
#cachedPrimaryEntropySourceId?: string;
145+
144146
readonly #keyringController = {
145147
setupLockedStateSubscriptions: () => {
146148
const { isUnlocked } = this.messenger.call('KeyringController:getState');
@@ -219,43 +221,33 @@ export class AuthenticationController extends BaseController<
219221
async #getLoginResponseFromState(
220222
entropySourceId?: string,
221223
): Promise<LoginResponse | null> {
222-
if (entropySourceId) {
223-
if (!this.state.srpSessionData?.[entropySourceId]) {
224-
return null;
225-
}
226-
return this.state.srpSessionData[entropySourceId];
227-
}
228-
229-
const primarySrpLoginResponse = Object.values(
230-
this.state.srpSessionData || {},
231-
)?.[0];
232-
233-
if (!primarySrpLoginResponse) {
224+
const resolvedId =
225+
entropySourceId ?? (await this.#getPrimaryEntropySourceId());
226+
if (!this.state.srpSessionData?.[resolvedId]) {
234227
return null;
235228
}
236-
237-
return primarySrpLoginResponse;
229+
return this.state.srpSessionData[resolvedId];
238230
}
239231

240232
async #setLoginResponseToState(
241233
loginResponse: LoginResponse,
242234
entropySourceId?: string,
243235
) {
236+
const resolvedId =
237+
entropySourceId ?? (await this.#getPrimaryEntropySourceId());
244238
const metaMetricsId = await this.#metametrics.getMetaMetricsId();
245239
this.update((state) => {
246-
if (entropySourceId) {
247-
state.isSignedIn = true;
248-
if (!state.srpSessionData) {
249-
state.srpSessionData = {};
250-
}
251-
state.srpSessionData[entropySourceId] = {
252-
...loginResponse,
253-
profile: {
254-
...loginResponse.profile,
255-
metaMetricsId,
256-
},
257-
};
240+
state.isSignedIn = true;
241+
if (!state.srpSessionData) {
242+
state.srpSessionData = {};
258243
}
244+
state.srpSessionData[resolvedId] = {
245+
...loginResponse,
246+
profile: {
247+
...loginResponse.profile,
248+
metaMetricsId,
249+
},
250+
};
259251
});
260252
}
261253

@@ -265,6 +257,29 @@ export class AuthenticationController extends BaseController<
265257
}
266258
}
267259

260+
async #getPrimaryEntropySourceId(): Promise<string> {
261+
if (this.#cachedPrimaryEntropySourceId) {
262+
return this.#cachedPrimaryEntropySourceId;
263+
}
264+
const allPublicKeys = await this.#snapGetAllPublicKeys();
265+
266+
if (allPublicKeys.length === 0) {
267+
throw new Error(
268+
'#getPrimaryEntropySourceId - No entropy sources found from snap',
269+
);
270+
}
271+
272+
const primaryId = allPublicKeys[0][0];
273+
if (!primaryId) {
274+
throw new Error(
275+
'#getPrimaryEntropySourceId - Primary entropy source ID is undefined',
276+
);
277+
}
278+
279+
this.#cachedPrimaryEntropySourceId = primaryId;
280+
return this.#cachedPrimaryEntropySourceId;
281+
}
282+
268283
public async performSignIn(): Promise<string[]> {
269284
this.#assertIsUnlocked('performSignIn');
270285

@@ -282,6 +297,7 @@ export class AuthenticationController extends BaseController<
282297
}
283298

284299
public performSignOut(): void {
300+
this.#cachedPrimaryEntropySourceId = undefined;
285301
this.update((state) => {
286302
state.isSignedIn = false;
287303
state.srpSessionData = undefined;
@@ -297,7 +313,9 @@ export class AuthenticationController extends BaseController<
297313

298314
public async getBearerToken(entropySourceId?: string): Promise<string> {
299315
this.#assertIsUnlocked('getBearerToken');
300-
return await this.#auth.getAccessToken(entropySourceId);
316+
const resolvedId =
317+
entropySourceId ?? (await this.#getPrimaryEntropySourceId());
318+
return await this.#auth.getAccessToken(resolvedId);
301319
}
302320

303321
/**
@@ -312,12 +330,18 @@ export class AuthenticationController extends BaseController<
312330
entropySourceId?: string,
313331
): Promise<UserProfile> {
314332
this.#assertIsUnlocked('getSessionProfile');
315-
return await this.#auth.getUserProfile(entropySourceId);
333+
const resolvedId =
334+
entropySourceId ?? (await this.#getPrimaryEntropySourceId());
335+
return await this.#auth.getUserProfile(resolvedId);
316336
}
317337

318-
public async getUserProfileLineage(): Promise<UserProfileLineage> {
338+
public async getUserProfileLineage(
339+
entropySourceId?: string,
340+
): Promise<UserProfileLineage> {
319341
this.#assertIsUnlocked('getUserProfileLineage');
320-
return await this.#auth.getUserProfileLineage();
342+
const resolvedId =
343+
entropySourceId ?? (await this.#getPrimaryEntropySourceId());
344+
return await this.#auth.getUserProfileLineage(resolvedId);
321345
}
322346

323347
public isSignedIn(): boolean {

0 commit comments

Comments
 (0)