-
-
Notifications
You must be signed in to change notification settings - Fork 275
Comments
feat!: add serialized keyring data to state#8009
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keyrings array has historically provided useful insights into state logs. If we want to keep this array exposed, we can add an separate serializedKeyrings state property where we can store serialized keyring objects safely, instead of having a data property in the objects included in keyrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serialized keyring secrets exposed via controller state
High Severity
displayForKeyring now puts await keyring.serialize() into state.keyrings[*].data, and keyrings is marked usedInUi: true. Since keyring serialize() commonly includes sensitive material (e.g., mnemonics/private keys), this exposes secrets to any consumer of KeyringController:getState / KeyringController:stateChange (including UI code).
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should worry about KeyringController consumers being able to read secrets, because that's already possible via withKeyring. But perhaps we should consider restricting this property in the UI, to avoid accidental leaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
State updates now repeatedly serialize keyrings
Medium Severity
displayForKeyring now calls keyring.serialize() (and cloneDeep) when building state.keyrings. This makes common operations that refresh state (e.g., unlock, vault updates) perform full keyring serialization even when only accounts/type/metadata are needed, which can significantly increase latency for large keyrings or expensive serializers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyrings removed from state logs unexpectedly
Medium Severity
Setting keyrings.includeInStateLogs to false removes keyrings from deriveStateFromMetadata(..., 'includeInStateLogs'), reducing observability in state logs. This can break existing diagnostics/support workflows that relied on seeing the (non-sensitive) keyrings array shape/types in logs, especially since the PR discussion notes historical reliance on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #8009 (comment)
Gudahtt
commented
Feb 24, 2026
@metamaskbot publish-preview
Gudahtt
commented
Feb 24, 2026
(we just changed the preview build workflow, I just wanted to test that it still works)
Preview builds have been published. See these instructions for more information about preview builds.
Expand for full list of packages and versions.
{
"@metamask-previews/account-tree-controller": "4.1.1-preview-3d4d0d0ef",
"@metamask-previews/accounts-controller": "36.0.1-preview-3d4d0d0ef",
"@metamask-previews/address-book-controller": "7.0.1-preview-3d4d0d0ef",
"@metamask-previews/ai-controllers": "0.1.0-preview-3d4d0d0ef",
"@metamask-previews/analytics-controller": "1.0.0-preview-3d4d0d0ef",
"@metamask-previews/analytics-data-regulation-controller": "0.0.0-preview-3d4d0d0ef",
"@metamask-previews/announcement-controller": "8.0.0-preview-3d4d0d0ef",
"@metamask-previews/app-metadata-controller": "2.0.0-preview-3d4d0d0ef",
"@metamask-previews/approval-controller": "8.0.0-preview-3d4d0d0ef",
"@metamask-previews/assets-controller": "2.0.2-preview-3d4d0d0ef",
"@metamask-previews/assets-controllers": "100.0.2-preview-3d4d0d0ef",
"@metamask-previews/base-controller": "9.0.0-preview-3d4d0d0ef",
"@metamask-previews/bridge-controller": "67.1.1-preview-3d4d0d0ef",
"@metamask-previews/bridge-status-controller": "67.0.1-preview-3d4d0d0ef",
"@metamask-previews/build-utils": "3.0.4-preview-3d4d0d0ef",
"@metamask-previews/chain-agnostic-permission": "1.4.0-preview-3d4d0d0ef",
"@metamask-previews/claims-controller": "0.4.2-preview-3d4d0d0ef",
"@metamask-previews/client-controller": "1.0.0-preview-3d4d0d0ef",
"@metamask-previews/compliance-controller": "1.0.0-preview-3d4d0d0ef",
"@metamask-previews/composable-controller": "12.0.0-preview-3d4d0d0ef",
"@metamask-previews/connectivity-controller": "0.1.0-preview-3d4d0d0ef",
"@metamask-previews/controller-utils": "11.19.0-preview-3d4d0d0ef",
"@metamask-previews/core-backend": "6.0.0-preview-3d4d0d0ef",
"@metamask-previews/delegation-controller": "2.0.1-preview-3d4d0d0ef",
"@metamask-previews/earn-controller": "11.1.1-preview-3d4d0d0ef",
"@metamask-previews/eip-5792-middleware": "2.1.0-preview-3d4d0d0ef",
"@metamask-previews/eip-7702-internal-rpc-middleware": "0.1.0-preview-3d4d0d0ef",
"@metamask-previews/eip1193-permission-middleware": "1.0.3-preview-3d4d0d0ef",
"@metamask-previews/ens-controller": "19.0.3-preview-3d4d0d0ef",
"@metamask-previews/error-reporting-service": "3.0.1-preview-3d4d0d0ef",
"@metamask-previews/eth-block-tracker": "15.0.1-preview-3d4d0d0ef",
"@metamask-previews/eth-json-rpc-middleware": "23.1.0-preview-3d4d0d0ef",
"@metamask-previews/eth-json-rpc-provider": "6.0.0-preview-3d4d0d0ef",
"@metamask-previews/foundryup": "1.0.1-preview-3d4d0d0ef",
"@metamask-previews/gas-fee-controller": "26.0.3-preview-3d4d0d0ef",
"@metamask-previews/gator-permissions-controller": "2.0.0-preview-3d4d0d0ef",
"@metamask-previews/json-rpc-engine": "10.2.2-preview-3d4d0d0ef",
"@metamask-previews/json-rpc-middleware-stream": "8.0.8-preview-3d4d0d0ef",
"@metamask-previews/keyring-controller": "25.1.0-preview-3d4d0d0ef",
"@metamask-previews/logging-controller": "7.0.1-preview-3d4d0d0ef",
"@metamask-previews/message-manager": "14.1.0-preview-3d4d0d0ef",
"@metamask-previews/messenger": "0.3.0-preview-3d4d0d0ef",
"@metamask-previews/multichain-account-service": "7.0.0-preview-3d4d0d0ef",
"@metamask-previews/multichain-api-middleware": "1.2.7-preview-3d4d0d0ef",
"@metamask-previews/multichain-network-controller": "3.0.4-preview-3d4d0d0ef",
"@metamask-previews/multichain-transactions-controller": "7.0.1-preview-3d4d0d0ef",
"@metamask-previews/name-controller": "9.0.0-preview-3d4d0d0ef",
"@metamask-previews/network-controller": "30.0.0-preview-3d4d0d0ef",
"@metamask-previews/network-enablement-controller": "4.1.2-preview-3d4d0d0ef",
"@metamask-previews/notification-services-controller": "22.0.0-preview-3d4d0d0ef",
"@metamask-previews/permission-controller": "12.2.0-preview-3d4d0d0ef",
"@metamask-previews/permission-log-controller": "5.0.0-preview-3d4d0d0ef",
"@metamask-previews/perps-controller": "0.0.0-preview-3d4d0d0ef",
"@metamask-previews/phishing-controller": "16.3.0-preview-3d4d0d0ef",
"@metamask-previews/polling-controller": "16.0.3-preview-3d4d0d0ef",
"@metamask-previews/preferences-controller": "22.1.0-preview-3d4d0d0ef",
"@metamask-previews/profile-metrics-controller": "3.0.1-preview-3d4d0d0ef",
"@metamask-previews/profile-sync-controller": "27.1.0-preview-3d4d0d0ef",
"@metamask-previews/ramps-controller": "9.0.0-preview-3d4d0d0ef",
"@metamask-previews/rate-limit-controller": "7.0.0-preview-3d4d0d0ef",
"@metamask-previews/remote-feature-flag-controller": "4.0.0-preview-3d4d0d0ef",
"@metamask-previews/sample-controllers": "4.0.3-preview-3d4d0d0ef",
"@metamask-previews/seedless-onboarding-controller": "8.0.0-preview-3d4d0d0ef",
"@metamask-previews/selected-network-controller": "26.0.3-preview-3d4d0d0ef",
"@metamask-previews/shield-controller": "5.0.1-preview-3d4d0d0ef",
"@metamask-previews/signature-controller": "39.0.4-preview-3d4d0d0ef",
"@metamask-previews/storage-service": "1.0.0-preview-3d4d0d0ef",
"@metamask-previews/subscription-controller": "6.0.0-preview-3d4d0d0ef",
"@metamask-previews/transaction-controller": "62.18.0-preview-3d4d0d0ef",
"@metamask-previews/transaction-pay-controller": "16.0.0-preview-3d4d0d0ef",
"@metamask-previews/user-operation-controller": "41.0.3-preview-3d4d0d0ef"
}
Uh oh!
There was an error while loading. Please reload this page.
Explanation
A draft proposal to provide a way to sychronously access read-only serialized keyrings via KeyringController state, with no mutex needed
References
Checklist
Note
Medium Risk
Touches keyring state shape and logging behavior; consumers may break if they assume
KeyringObjecthas onlytype/accounts/metadata, and serialized data in memory increases sensitivity if mishandled.Overview
BREAKING:
KeyringObject(and thusKeyringControllerState.keyrings) now includes adata: Jsonfield containing a deep-cloned result ofkeyring.serialize(), so serialized keyring data is synchronously available in controller state.To reduce sensitive logging,
keyringsis no longer included in state logs metadata. Tests and changelog are updated to assert the newdatafield and to use a mock keyring builder in the "existing metadata" unlock test; unlock state restoration now assigns updated keyrings with a TS workaround for deepJsoninstantiation.Written by Cursor Bugbot for commit 3d4d0d0. This will update automatically on new commits. Configure here.