Skip to content

fix: Strip accessToken from AuthenticationController state logs #6553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Gudahtt merged 9 commits into from
Sep 12, 2025
Merged

fix: Strip accessToken from AuthenticationController state logs #6553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc06fb9
fix: Strip `accessToken` from `AuthenticationController` state logs
Gudahtt Sep 10, 2025
1d6855d
Update changelog
Gudahtt Sep 10, 2025
50627c6
Remove accidentally committed comment
Gudahtt Sep 10, 2025
8e9fac0
Fix test failures and lint error
Gudahtt Sep 10, 2025
c7d38a4
Fix another lint error
Gudahtt Sep 10, 2025
8cc1bc2
Make state deriver a bit more concise
Gudahtt Sep 11, 2025
fb418b9
Safer handling of null property value
Gudahtt Sep 11, 2025
32bfec7
Update changelog
Gudahtt Sep 11, 2025
04098a8
Update utils to match version used elsewhere (it was updated in a rec…
Gudahtt Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/profile-sync-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
1 change: 1 addition & 0 deletions packages/profile-sync-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
150 changes: 132 additions & 18 deletions ...s/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, LoginResponse>;

MOCK_ENTROPY_SOURCE_IDS.forEach((id) => {
srpSessionData[id] = {
token: {
accessToken: MOCK_OATH_TOKEN_RESPONSE.access_token,
expiresIn: Date.now() + 3600,
expiresIn,
obtainedAt: 0,
},
profile: {
Expand Down Expand Up @@ -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 }),
Copy link
Contributor

@fabiobozzo fabiobozzo Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, consider extracting MOCK_EXPIRES_IN = 1_000

Copy link
Member Author

@Gudahtt Gudahtt Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost did this, but in the end I figured it was better to have the actual mock value directly in the test, because the value itself is what we're looking for in the inline snapshot.

Usually in tests with common mocks, the mock values are less relevant. But it was useful for readability in this case.

});

expect(
Expand All @@ -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,
},
},
},
}
`);
});
Expand All @@ -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(
Expand All @@ -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,
},
},
},
}
`);
});
Expand Down
25 changes: 24 additions & 1 deletion packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -49,7 +50,29 @@ const metadata: StateMetadata<AuthenticationControllerState> = {
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<Record<string, Json>>(
(sanitizedSrpSessionData, [key, value]) => {
const { accessToken: _unused, ...tokenWithoutAccessToken } =
value.token;
sanitizedSrpSessionData[key] = {
...value,
token: tokenWithoutAccessToken,
};
return sanitizedSrpSessionData;
},
{},
);
},
persist: true,
anonymous: false,
usedInUi: true,
Expand Down
1 change: 1 addition & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading