feat!: add serialized keyring data to state#8009
Conversation
| }, | ||
| keyrings: { | ||
| includeInStateLogs: true, | ||
| includeInStateLogs: false, |
There was a problem hiding this comment.
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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Cast to `string[]` here is safe here because `accounts` has no nullish | ||
| // values, and `normalize` returns `string` unless given a nullish value | ||
| accounts: accounts.map(normalize) as string[], | ||
| data: cloneDeep(await keyring.serialize()), |
There was a problem hiding this comment.
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.
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.
| // Cast to `string[]` here is safe here because `accounts` has no nullish | ||
| // values, and `normalize` returns `string` unless given a nullish value | ||
| accounts: accounts.map(normalize) as string[], | ||
| data: cloneDeep(await keyring.serialize()), |
There was a problem hiding this comment.
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.
| }, | ||
| keyrings: { | ||
| includeInStateLogs: true, | ||
| includeInStateLogs: false, |
There was a problem hiding this comment.
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.
1 participant
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.