Skip to content

Commit 90fe900

Browse files
committed
fix: Strip accessToken from AuthenticationController state logs
The metadata property was recently added to the `AuthenticationController` in #6470, but we forgot to strip out the `accessToken`. This is currently being stripped from state logs in both clients. The metadata was updated to strip out this token. Additionally, some improvements were added to the metadata snapshot tests to ensure that all properties are represented in the fixture state. The `@metamask/utils` package was added as a dependency because the new metadata state deriver uses `Json` in its signature, so that type is now used in the exported types for this package.
1 parent 29a7354 commit 90fe900

File tree

5 files changed

+143
-17
lines changed

5 files changed

+143
-17
lines changed

packages/base-controller/src/BaseController.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,7 @@ export function deriveStateFromMetadata<
394394
metadata: StateMetadata<ControllerState>,
395395
metadataProperty: keyof StatePropertyMetadata<Json>,
396396
): Record<keyof ControllerState, Json> {
397+
// Cast to keyof ControllerState because `Object.keys` erases index type, returning string
397398
return (Object.keys(state) as (keyof ControllerState)[]).reduce<
398399
Record<keyof ControllerState, Json>
399400
>((derivedState, key) => {

packages/profile-sync-controller/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@
103103
"@metamask/base-controller": "^8.3.0",
104104
"@metamask/snaps-sdk": "^9.0.0",
105105
"@metamask/snaps-utils": "^11.0.0",
106+
"@metamask/utils": "^11.4.2",
106107
"@noble/ciphers": "^1.3.0",
107108
"@noble/hashes": "^1.8.0",
108109
"immer": "^9.0.6",

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

Lines changed: 118 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type { LoginResponse } from '../../sdk';
1414
import { Platform } from '../../sdk';
1515
import { arrangeAuthAPIs } from '../../sdk/__fixtures__/auth';
1616
import { MOCK_USER_PROFILE_LINEAGE_RESPONSE } from '../../sdk/mocks/auth';
17+
import { hasProperty } from '@metamask/utils';
1718

1819
const MOCK_ENTROPY_SOURCE_IDS = [
1920
'MOCK_ENTROPY_SOURCE_ID',
@@ -543,6 +544,7 @@ describe('metadata', () => {
543544
const controller = new AuthenticationController({
544545
messenger: createMockAuthenticationMessenger().messenger,
545546
metametrics: createMockAuthMetaMetrics(),
547+
state: mockSignedInState(),
546548
});
547549

548550
expect(
@@ -553,41 +555,114 @@ describe('metadata', () => {
553555
),
554556
).toMatchInlineSnapshot(`
555557
Object {
556-
"isSignedIn": false,
558+
"isSignedIn": true,
557559
}
558560
`);
559561
});
560562

561-
it('includes expected state in state logs', () => {
562-
const controller = new AuthenticationController({
563-
messenger: createMockAuthenticationMessenger().messenger,
564-
metametrics: createMockAuthMetaMetrics(),
565-
});
563+
describe('includeInStateLogs', () => {
564+
it('includes expected state in state logs, with access token stripped out', () => {
565+
const controller = new AuthenticationController({
566+
messenger: createMockAuthenticationMessenger().messenger,
567+
metametrics: createMockAuthMetaMetrics(),
568+
state: mockSignedInState(),
569+
});
566570

567-
expect(
568-
deriveStateFromMetadata(
571+
const derivedState = deriveStateFromMetadata(
569572
controller.state,
570573
controller.metadata,
571574
'includeInStateLogs',
572-
),
573-
).toMatchInlineSnapshot(`
574-
Object {
575-
"isSignedIn": false,
576-
}
577-
`);
575+
);
576+
577+
expect(derivedState).toMatchInlineSnapshot(`
578+
Object {
579+
"isSignedIn": true,
580+
"srpSessionData": Object {
581+
"MOCK_ENTROPY_SOURCE_ID": Object {
582+
"profile": Object {
583+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
584+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
585+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
586+
},
587+
"token": Object {
588+
"expiresIn": 1757528159922,
589+
"obtainedAt": 0,
590+
},
591+
},
592+
"MOCK_ENTROPY_SOURCE_ID2": Object {
593+
"profile": Object {
594+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
595+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
596+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
597+
},
598+
"token": Object {
599+
"expiresIn": 1757528159922,
600+
"obtainedAt": 0,
601+
},
602+
},
603+
},
604+
}
605+
`);
606+
});
607+
608+
it('returns expected state in state logs when srpSessionData is unset', () => {
609+
const controller = new AuthenticationController({
610+
messenger: createMockAuthenticationMessenger().messenger,
611+
metametrics: createMockAuthMetaMetrics(),
612+
});
613+
614+
expect(
615+
deriveStateFromMetadata(
616+
controller.state,
617+
controller.metadata,
618+
'includeInStateLogs',
619+
),
620+
).toMatchInlineSnapshot(`
621+
Object {
622+
"isSignedIn": false,
623+
}
624+
`);
625+
});
578626
});
579627

580628
it('persists expected state', () => {
581629
const controller = new AuthenticationController({
582630
messenger: createMockAuthenticationMessenger().messenger,
583631
metametrics: createMockAuthMetaMetrics(),
632+
state: mockSignedInState(),
584633
});
585634

586635
expect(
587636
deriveStateFromMetadata(controller.state, controller.metadata, 'persist'),
588637
).toMatchInlineSnapshot(`
589638
Object {
590-
"isSignedIn": false,
639+
"isSignedIn": true,
640+
"srpSessionData": Object {
641+
"MOCK_ENTROPY_SOURCE_ID": Object {
642+
"profile": Object {
643+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
644+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
645+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
646+
},
647+
"token": Object {
648+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
649+
"expiresIn": 1757528159923,
650+
"obtainedAt": 0,
651+
},
652+
},
653+
"MOCK_ENTROPY_SOURCE_ID2": Object {
654+
"profile": Object {
655+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
656+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
657+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
658+
},
659+
"token": Object {
660+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
661+
"expiresIn": 1757528159923,
662+
"obtainedAt": 0,
663+
},
664+
},
665+
},
591666
}
592667
`);
593668
});
@@ -596,6 +671,7 @@ describe('metadata', () => {
596671
const controller = new AuthenticationController({
597672
messenger: createMockAuthenticationMessenger().messenger,
598673
metametrics: createMockAuthMetaMetrics(),
674+
state: mockSignedInState(),
599675
});
600676

601677
expect(
@@ -606,7 +682,33 @@ describe('metadata', () => {
606682
),
607683
).toMatchInlineSnapshot(`
608684
Object {
609-
"isSignedIn": false,
685+
"isSignedIn": true,
686+
"srpSessionData": Object {
687+
"MOCK_ENTROPY_SOURCE_ID": Object {
688+
"profile": Object {
689+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
690+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
691+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
692+
},
693+
"token": Object {
694+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
695+
"expiresIn": 1757528159924,
696+
"obtainedAt": 0,
697+
},
698+
},
699+
"MOCK_ENTROPY_SOURCE_ID2": Object {
700+
"profile": Object {
701+
"identifierId": "da9a9fc7b09edde9cc23cec9b7e11a71fb0ab4d2ddd8af8af905306f3e1456fb",
702+
"metaMetricsId": "561ec651-a844-4b36-a451-04d6eac35740",
703+
"profileId": "f88227bd-b615-41a3-b0be-467dd781a4ad",
704+
},
705+
"token": Object {
706+
"accessToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
707+
"expiresIn": 1757528159924,
708+
"obtainedAt": 0,
709+
},
710+
},
711+
},
610712
}
611713
`);
612714
});

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
KeyringControllerUnlockEvent,
1212
} from '@metamask/keyring-controller';
1313
import type { HandleSnapRequest } from '@metamask/snaps-controllers';
14+
import type { Json } from '@metamask/utils';
1415

1516
import {
1617
createSnapPublicKeyRequest,
@@ -49,7 +50,27 @@ const metadata: StateMetadata<AuthenticationControllerState> = {
4950
usedInUi: true,
5051
},
5152
srpSessionData: {
52-
includeInStateLogs: true,
53+
// Remove access token from state logs
54+
includeInStateLogs: (srpSessionData) => {
55+
// Using non-null assertion here to assert that it's not undefined, because if it was, this
56+
// wouldn't get called.
57+
// The type includes `| undefined` only because we don't yet use `strictOptionalTypes`
58+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
59+
return Object.entries(srpSessionData!).reduce<Record<string, Json>>(
60+
(sanitizedSrpSessionData, [key, value]) => {
61+
const token: Partial<(typeof value)['token']> = {
62+
...value.token,
63+
};
64+
delete token.accessToken;
65+
sanitizedSrpSessionData[key] = {
66+
...value,
67+
token,
68+
};
69+
return sanitizedSrpSessionData;
70+
},
71+
{},
72+
);
73+
},
5374
persist: true,
5475
anonymous: false,
5576
usedInUi: true,

yarn.lock

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4263,6 +4263,7 @@ __metadata:
42634263
"@metamask/snaps-controllers": "npm:^14.0.1"
42644264
"@metamask/snaps-sdk": "npm:^9.0.0"
42654265
"@metamask/snaps-utils": "npm:^11.0.0"
4266+
"@metamask/utils": "npm:^11.4.2"
42664267
"@noble/ciphers": "npm:^1.3.0"
42674268
"@noble/hashes": "npm:^1.8.0"
42684269
"@types/jest": "npm:^27.4.1"

0 commit comments

Comments
 (0)