Repository navigation
fix: harden the wallet store against tampered and polluted state - #132
Conversation
| /** | ||
| * Property names that would reach Object.prototype when used as a key of a plain object | ||
| */ | ||
| export const RESERVED_OBJECT_KEYS: ReadonlySet<string> = new Set(['__proto__', 'constructor', 'prototype']) |
There was a problem hiding this comment.
RESERVED_OBJECT_KEYS only blocks proto, constructor and prototype, but ??= reads through to any inherited property
I think we can replace the set with a predicate so there is no list to maintain:
export const isReservedObjectKey = (key: string): boolean =>
key in Object.prototype || key === 'prototype'
To be clear, this check will only apply to write attempts from now on, and has no effect on past/created wallets right?
There was a problem hiding this comment.
Dropped RESERVED_OBJECT_KEYS and replaced by isReservedObjectKey
Not only writes, it apply to read too. resolveWalletId is used by the balance and address getters, and the secure-storage read helper validates the identifier as well.
No effect on existing wallets. I loaded a stored blob with a normal wallet plus reserved keys: the app starts, the normal wallet reads fine
There was a problem hiding this comment.
The one caveat is that a wallet already stored under a reserved id (for example constructor) can no longer be read or deleted after this change. In practice, I think this is acceptable since these IDs are unlikely, such an ID would likely corrupt the store before this PR so it's an edge case
| return (state: WalletState | undefined) => { | ||
| if (state) { | ||
| log('🔄 Rehydrating wallet state - resetting loading states') | ||
| state.addresses = {} |
There was a problem hiding this comment.
We need to ensure this doesn't alter the behavior for consumer. Can you test against wdk-examples? What I expect is that all functions behave the same way, it's just a bit slower without the wallet store, but this is only addresses so fetching from the worklet shouldn't be too taxing
There was a problem hiding this comment.
Tested with wdk-examples. Same results after loading. The only difference is one getAddress call per network on first load, then it's cached, and data is undefined on the first render instead of coming from storage
|
Also, this PR addresses 3 different points. Should we make issues to track these points? |
Description
__proto__,constructorandprototypeas wallet id or network name.Motivation and Context
Persisted addresses could be swapped on disk, the encryption key was eligible for Keychain backup while the data it protects was not, and a crafted key could pollute
Object.prototype.Related Issue
Closes #
Type of change