diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 46f49f8686a..90be0fae741 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/keyring-api` from `^20.1.0` to `^21.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560)) - Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560)) +- Strip `srpSessionData.token.accessToken` from state logs ([#6553](https://github.com/MetaMask/core/pull/6553)) + - We haven't started using the `includeInStateLogs` metadata yet in clients, so this will have no functional impact. This change brings this metadata into alignment with the hard-coded state log generation performed by clients.today. +- Add dependency on `@metamask/utils` ([#6553](https://github.com/MetaMask/core/pull/6553)) ## [25.0.0] diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index 69db896c5b8..1135aac29ed 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -103,6 +103,7 @@ "@metamask/base-controller": "^8.3.0", "@metamask/snaps-sdk": "^9.0.0", "@metamask/snaps-utils": "^11.0.0", + "@metamask/utils": "^11.8.0", "@noble/ciphers": "^1.3.0", "@noble/hashes": "^1.8.0", "immer": "^9.0.6", diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts index 3b82c86808d..50933b12378 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -20,14 +20,23 @@ const MOCK_ENTROPY_SOURCE_IDS = [ 'MOCK_ENTROPY_SOURCE_ID2', ]; -const mockSignedInState = (): AuthenticationControllerState => { +/** + * Return mock state for the scenario where a user is signed in. + * + * @param options - Options. + * @param options.expiresIn - The timestamp to use for the `expiresIn` token property. + * @returns Mock AuthenticationController state reflecting a signed in user. + */ +const mockSignedInState = ({ + expiresIn = Date.now() + 3600, +}: { expiresIn?: number } = {}): AuthenticationControllerState => { const srpSessionData = {} as Record; MOCK_ENTROPY_SOURCE_IDS.forEach((id) => { srpSessionData[id] = { token: { accessToken: MOCK_OATH_TOKEN_RESPONSE.access_token, - expiresIn: Date.now() + 3600, + expiresIn, obtainedAt: 0, }, profile: { @@ -543,6 +552,8 @@ describe('metadata', () => { const controller = new AuthenticationController({ messenger: createMockAuthenticationMessenger().messenger, metametrics: createMockAuthMetaMetrics(), + // Set `expiresIn` to an arbitrary number so that it stays consistent between test runs + state: mockSignedInState({ expiresIn: 1_000 }), }); expect( @@ -553,41 +564,116 @@ describe('metadata', () => { ), ).toMatchInlineSnapshot(` Object { - "isSignedIn": false, + "isSignedIn": true, } `); }); - it('includes expected state in state logs', () => { - const controller = new AuthenticationController({ - messenger: createMockAuthenticationMessenger().messenger, - metametrics: createMockAuthMetaMetrics(), - }); + describe('includeInStateLogs', () => { + it('includes expected state in state logs, with access token stripped out', () => { + const controller = new AuthenticationController({ + messenger: createMockAuthenticationMessenger().messenger, + metametrics: createMockAuthMetaMetrics(), + // Set `expiresIn` to an arbitrary number so that it stays consistent between test runs + state: mockSignedInState({ expiresIn: 1_000 }), + }); - expect( - deriveStateFromMetadata( + const derivedState = deriveStateFromMetadata( controller.state, controller.metadata, 'includeInStateLogs', - ), - ).toMatchInlineSnapshot(` - Object { - "isSignedIn": false, - } - `); + ); + + expect(derivedState).toMatchInlineSnapshot(` + Object { + "isSignedIn": true, + "srpSessionData": Object { + "MOCK_ENTROPY_SOURCE_ID": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + "MOCK_ENTROPY_SOURCE_ID2": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + }, + } + `); + }); + + it('returns expected state in state logs when srpSessionData is unset', () => { + const controller = new AuthenticationController({ + messenger: createMockAuthenticationMessenger().messenger, + metametrics: createMockAuthMetaMetrics(), + }); + + expect( + deriveStateFromMetadata( + controller.state, + controller.metadata, + 'includeInStateLogs', + ), + ).toMatchInlineSnapshot(` + Object { + "isSignedIn": false, + } + `); + }); }); it('persists expected state', () => { const controller = new AuthenticationController({ messenger: createMockAuthenticationMessenger().messenger, metametrics: createMockAuthMetaMetrics(), + // Set `expiresIn` to an arbitrary number so that it stays consistent between test runs + state: mockSignedInState({ expiresIn: 1_000 }), }); expect( deriveStateFromMetadata(controller.state, controller.metadata, 'persist'), ).toMatchInlineSnapshot(` Object { - "isSignedIn": false, + "isSignedIn": true, + "srpSessionData": Object { + "MOCK_ENTROPY_SOURCE_ID": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + "MOCK_ENTROPY_SOURCE_ID2": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + }, } `); }); @@ -596,6 +682,8 @@ describe('metadata', () => { const controller = new AuthenticationController({ messenger: createMockAuthenticationMessenger().messenger, metametrics: createMockAuthMetaMetrics(), + // Set `expiresIn` to an arbitrary number so that it stays consistent between test runs + state: mockSignedInState({ expiresIn: 1_000 }), }); expect( @@ -606,7 +694,33 @@ describe('metadata', () => { ), ).toMatchInlineSnapshot(` Object { - "isSignedIn": false, + "isSignedIn": true, + "srpSessionData": Object { + "MOCK_ENTROPY_SOURCE_ID": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + "MOCK_ENTROPY_SOURCE_ID2": Object { + "profile": Object { + "identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb", + "metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740", + "profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad", + }, + "token": Object { + "accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", + "expiresIn": 1000, + "obtainedAt": 0, + }, + }, + }, } `); }); diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 0478ff1897d..7e9a62443a8 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -11,6 +11,7 @@ import type { KeyringControllerUnlockEvent, } from '@metamask/keyring-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; +import type { Json } from '@metamask/utils'; import { createSnapPublicKeyRequest, @@ -49,7 +50,29 @@ const metadata: StateMetadata = { usedInUi: true, }, srpSessionData: { - includeInStateLogs: true, + // Remove access token from state logs + includeInStateLogs: (srpSessionData) => { + // Unreachable branch, included just to fix a type error for the case where this property is + // unset. The type gets collapsed to include `| undefined` even though `undefined` is never + // set here, because we don't yet use `exactOptionalPropertyTypes`. + // TODO: Remove branch after enabling `exactOptionalPropertyTypes` + // ref: https://github.com/MetaMask/core/issues/6565 + if (srpSessionData === null || srpSessionData === undefined) { + return null; + } + return Object.entries(srpSessionData).reduce>( + (sanitizedSrpSessionData, [key, value]) => { + const { accessToken: _unused, ...tokenWithoutAccessToken } = + value.token; + sanitizedSrpSessionData[key] = { + ...value, + token: tokenWithoutAccessToken, + }; + return sanitizedSrpSessionData; + }, + {}, + ); + }, persist: true, anonymous: false, usedInUi: true, diff --git a/yarn.lock b/yarn.lock index 3b747dc56b3..3e8f7dd140c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4264,6 +4264,7 @@ __metadata: "@metamask/snaps-controllers": "npm:^14.0.1" "@metamask/snaps-sdk": "npm:^9.0.0" "@metamask/snaps-utils": "npm:^11.0.0" + "@metamask/utils": "npm:^11.8.0" "@noble/ciphers": "npm:^1.3.0" "@noble/hashes": "npm:^1.8.0" "@types/jest": "npm:^27.4.1"