fix: Strip `accessToken` from `AuthenticationController` state logs #6553

Gudahtt · 2025-09-10T18:25:52Z

Explanation

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.

References

This is an amendment to #6470

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Gudahtt · 2025-09-10T18:27:11Z

packages/profile-sync-controller/CHANGELOG.md

It didn't seem relevant to mention stripping out accessToken here, since it's not really a change from the previous release (the state log metadata isn't released yet).

I did mention the new dependency though

🤦 That quick, it was released. I'll have to add something now.

OK, updated again

mcmire

Makes sense!

mathieuartu

LGTM! Thanks!

fabiobozzo · 2025-09-11T09:48:16Z

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

+      // wouldn't get called.
+      // The type includes `| undefined` only because we don't yet use `strictOptionalTypes`
+      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+      return Object.entries(srpSessionData!).reduce<Record<string, Json>>(


Suggested change

return Object.entries(srpSessionData!).reduce<Record<string, Json>>(

if (!srpSessionData) return {};

return Object.entries(srpSessionData).reduce<Record<string, Json>>(

safer?

Yeah, I almost did this as well. But then we'd have another untested branch in the code that can't be reached. It would be an error if this ever returned an empty object, despite it matching the type (i.e. it's still never supposed to happen).

Here's the version I almost used:

if (!srpSessionData) { return srpSessionData; }

This version was "safer" because if we ever updated this property to allow null, it would still work. Whereas my version would fail.

But.... this didn't work, because then it gave a type error about returning undefined which isn't valid JSON.

We do plan on enabling strictOptionalTypes, and that would eliminate the need for a non-null assertion here.

Actually... now that I'm saying that outloud, I think something like this would be best:

if (!srpSessionData === null || srpSessionData === undefined) { return null; }

This branch eliminates the type error, and it still works if we later update this property to allow null. It's still an unreachable branch, but that seems OK. I'll go with this option.

Done here: f29d7d1

fabiobozzo · 2025-09-11T09:49:10Z

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

+          const token: Partial<(typeof value)['token']> = {
+            ...value.token,
+          };
+          delete token.accessToken;


Isn't that the same as

const { accessToken, ...token } = value.token;

?

Hmmm. Yes, it is. Admittedly I coped this directly from the pre-existing implementation (here) and made minimal changes to address type errors and lint/formatting.

I do like your suggestion here much better, but I'm tempted to leave this as-is just because it's approved already.

I took the suggestion since I was making another change that would dismiss reviews anyway. Updated here: 65d23cb

fabiobozzo · 2025-09-11T09:49:46Z

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

      messenger: createMockAuthenticationMessenger().messenger,
      metametrics: createMockAuthMetaMetrics(),
+      // Set `expiresIn` to an arbitrary number so that it stays consistent between test runs
+      state: mockSignedInState({ expiresIn: 1_000 }),


Nit, consider extracting MOCK_EXPIRES_IN = 1_000

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.

fabiobozzo

LGTM

cryptodev-2s

LGTM!

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.

…ent PR)

## Explanation Release for `@metamask/profile-sync-controller` & `@metamask/multichain-account-service`. ```md ## [25.1.0] ### Changed - Use deferred promises for encryption/decryption KDF operations ([#6736](#6736)) - That will prevent duplicate KDF operations from being computed if one with the same options is already in progress. - For operations that already completed, we use the already existing cache. - Bump `@metamask/utils` from `^11.8.0` to `^11.8.1` ([#6708](#6708)) - Bump `@metamask/keyring-api` from `^20.1.0` to `^21.0.0` ([#6560](#6560)) - Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](#6560)) - Strip `srpSessionData.token.accessToken` from state logs ([#6553](#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](#6553)) - Bump `@metamask/base-controller` from `^8.3.0` to `^8.4.0` ([#6632](#6632)) ``` ## References  ## 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/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes  --- > [!NOTE] > Releases core 589.0.0, publishing @metamask/profile-sync-controller 25.1.0 and @metamask/multichain-account-service 1.3.0, and bumps dependent packages and changelogs. > > - **Release**: `@metamask/core-monorepo` to `589.0.0`. > - **Packages**: > - `@metamask/profile-sync-controller@25.1.0` > - Use deferred promises for KDF operations; update deps and strip `srpSessionData.token.accessToken` from state logs. > - `@metamask/multichain-account-service@1.3.0` > - Add `{Btc, Trx}AccountProvider`; update compare links. > - **Deps Bumped**: > - Update references to `@metamask/profile-sync-controller` to `^25.1.0` in `account-tree-controller`, `notification-services-controller`, `subscription-controller`, and lockfile. > - Update references to `@metamask/multichain-account-service` to `^1.3.0` in `assets-controllers`, `account-tree-controller`, and lockfile. > - **Changelogs**: > - Add Unreleased note for `assets-controllers` reflecting `multichain-account-service` bump. > - Add `1.3.0` section and links in `multichain-account-service`. > - Add `25.1.0` section and links in `profile-sync-controller`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 88351ca. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>  --------- Co-authored-by: Antonio Regadas <antonio.regadas@consensys.net>

Gudahtt commented Sep 10, 2025

View reviewed changes

Gudahtt mentioned this pull request Sep 10, 2025

feat: Add new metadata properties to accounts controllers #6470

Merged

4 tasks

Gudahtt marked this pull request as ready for review September 10, 2025 19:07

Gudahtt force-pushed the strip-access-token-from-state-logs branch from 846e6ad to e77d26b Compare September 10, 2025 19:07

Gudahtt requested review from a team as code owners September 10, 2025 19:07

mcmire previously approved these changes Sep 10, 2025

View reviewed changes

owencraston previously approved these changes Sep 10, 2025

View reviewed changes

mathieuartu previously approved these changes Sep 11, 2025

View reviewed changes

fabiobozzo reviewed Sep 11, 2025

View reviewed changes

fabiobozzo previously approved these changes Sep 11, 2025

View reviewed changes

Gudahtt dismissed stale reviews from fabiobozzo, mathieuartu, owencraston, and mcmire via 65d23cb September 11, 2025 12:56

Gudahtt force-pushed the strip-access-token-from-state-logs branch from f29d7d1 to c90a0ff Compare September 11, 2025 13:14

cryptodev-2s previously approved these changes Sep 11, 2025

View reviewed changes

Gudahtt dismissed cryptodev-2s’s stale review via 6358e0e September 11, 2025 14:57

Gudahtt force-pushed the strip-access-token-from-state-logs branch from 6358e0e to 2352606 Compare September 11, 2025 14:57

Gudahtt added 8 commits September 12, 2025 15:42

Update changelog

1d6855d

Remove accidentally committed comment

50627c6

Fix test failures and lint error

8e9fac0

Fix another lint error

c7d38a4

Make state deriver a bit more concise

8cc1bc2

Safer handling of null property value

fb418b9

Update changelog

32bfec7

Gudahtt force-pushed the strip-access-token-from-state-logs branch from 2352606 to 32bfec7 Compare September 12, 2025 18:12

Gudahtt enabled auto-merge (squash) September 12, 2025 18:12

Update utils to match version used elsewhere (it was updated in a rec…

04098a8

…ent PR)

fabiobozzo approved these changes Sep 12, 2025

View reviewed changes

Gudahtt merged commit 8c743e6 into main Sep 12, 2025
239 checks passed

Gudahtt deleted the strip-access-token-from-state-logs branch September 12, 2025 19:06

-      return Object.entries(srpSessionData!).reduce<Record<string, Json>>(
+      if (!srpSessionData) return {};
+      return Object.entries(srpSessionData).reduce<Record<string, Json>>(

Uh oh!

fix: Strip accessToken from AuthenticationController state logs #6553

fix: Strip accessToken from AuthenticationController state logs #6553

Uh oh!

Conversation

Gudahtt commented Sep 10, 2025

Explanation

References

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

mathieuartu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiobozzo left a comment

Choose a reason for hiding this comment

Uh oh!

cryptodev-2s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix: Strip `accessToken` from `AuthenticationController` state logs #6553

fix: Strip `accessToken` from `AuthenticationController` state logs #6553