added encrypted bundle caching + escrow key injection#115
added encrypted bundle caching + escrow key injection#115
Conversation
5d50e05 to
1fb0969
Compare
There was a problem hiding this comment.
Pull request overview
Adds encrypted export-bundle caching in the export-and-sign iframe’s localStorage, plus an in-memory “escrow” decryption key injection flow to decrypt bundles and enable signing without persisting the escrow key.
Changes:
- Added
localStoragehelpers for storing/removing encrypted bundles keyed by wallet address. - Added new iframe event handlers for storing encrypted bundles, injecting an escrow decryption bundle, listing stored addresses, clearing stored bundles, and burning the session.
- Added comprehensive Jest coverage for the escrow storage/decryption/signing lifecycle and related helper APIs.
Reviewed changes
Copilot reviewed 4 out of 11 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| shared/turnkey-core.js | Adds shared localStorage helpers to persist encrypted bundles. |
| export-and-sign/src/turnkey-core.js | Re-exports the new shared encrypted-bundle helper APIs through the iframe TKHQ facade. |
| export-and-sign/src/event-handlers.js | Implements new message handlers for encrypted bundle storage + escrow key injection + session/bundle management. |
| export-and-sign/index.test.js | Adds test coverage for new helper APIs and events (store/decrypt/sign/burn/clear/list). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function setEncryptedBundle(address, bundleData) { | ||
| const bundles = getEncryptedBundles() || {}; | ||
| bundles[address] = bundleData; | ||
| window.localStorage.setItem( | ||
| TURNKEY_ENCRYPTED_BUNDLES, | ||
| JSON.stringify(bundles) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Using an untrusted address as an object key allows prototype pollution (e.g., "proto", "constructor", "prototype"), and bundles[address] = ... can mutate the object prototype. Since address ultimately comes from postMessage inputs, this is a concrete risk. Use a Map-like serialization (array of [address, bundleData] entries), or store into an object created via Object.create(null) and explicitly reject dangerous keys before setting/removing. Apply the same protection in removal paths.
f6e33fa to
51d9867
Compare
3bca095 to
e82ee2d
Compare
leeland-turnkey
left a comment
There was a problem hiding this comment.
Left some comments, but overall this is looking great!
| delete inMemoryKeys[address]; | ||
| } else { | ||
| TKHQ.clearAllEncryptedBundles(organizationId); | ||
| inMemoryKeys = {}; |
There was a problem hiding this comment.
This logic removes all orgs right? I guess this use case wouldn't be hit because most users will only be a part of one org. There could be a chance in the future that there are multiple orgs within one company and this will blow away every org's in-memory key.
There was a problem hiding this comment.
This is just clearing in-memory keys, not in local storage. There should only be your current org's keys in memory during each "session" so its fine to wipe those completely 😁
| format: keyFormat, | ||
| expiry: new Date().getTime() + DEFAULT_TTL_MILLISECONDS, | ||
| keypair: cachedKeypair, // Cache the keypair for performance | ||
| keypair: cachedKeypair, |
There was a problem hiding this comment.
How do we clear by address in my first comment? Since there is no organizationId field, is there a way to do org-aware clearing of in-memory keys?
There was a problem hiding this comment.
onClearStoredBundles has an organizationId you must pass in, if the bundle you are trying to delete is not in your org, it will not allow you to delete it
See here:
/**
* Removes a single encrypted bundle by address.
* Only removes the bundle if it belongs to the specified organization.
* @param {string} address - The wallet address to remove
* @param {string} organizationId - Only remove if the bundle belongs to this org
*/
function removeEncryptedBundle(address, organizationId) {
const bundles = getEncryptedBundles();
if (!bundles || !bundles[address]) return;
// Only remove if the bundle belongs to the specified organization
if (bundles[address].organizationId !== organizationId) return;
delete bundles[address];
if (Object.keys(bundles).length === 0) {
window.localStorage.removeItem(TURNKEY_ENCRYPTED_BUNDLES);
} else {
window.localStorage.setItem(
TURNKEY_ENCRYPTED_BUNDLES,
JSON.stringify(bundles)
);
}
}
| ); | ||
|
|
||
| // HPKE-decrypt using the key | ||
| const keyBytes = await HpkeDecrypt({ |
There was a problem hiding this comment.
We have two variables named keyBytes: https://github.com/tkhq/frames/pull/115/changes#diff-24697156a650f90b0b6154cd01ada56112b8581e71d5c8122705f691a4772943R480
This inner keyBytes variable can work because of block scoping, but it could possibly cause confusion as well. I wonder if a rename would be good here?
| ); | ||
| if (!verified) { | ||
| throw new Error( | ||
| `failed to verify enclave signature: ${bundleObj.dataSignature}` |
There was a problem hiding this comment.
I like this change over the old way we did it: https://github.com/tkhq/frames/pull/115/changes#diff-24697156a650f90b0b6154cd01ada56112b8581e71d5c8122705f691a4772943L53
We were exposing the entire bundle which probably was not necessary.
Nice work.
There was a problem hiding this comment.
Copilot pointed that out for me 😅
leeland-turnkey
left a comment
There was a problem hiding this comment.
Everything looks good and the rest of my comments are NIT. LGTM
|
This doesn't make a whole lot of sense to me: instead of exporting the wallet directly encrypted to the iframe, we export a private key to which the wallet is encrypted, and persist the wallet. The result is similar: an export activity has to be performed in both cases, for signing (export private key vs. export wallet). What's the advantage? (generally, the simpler the crypto the better, so this extra layer of persistence/wrapping makes me nervous) |
|
You can find their exact use case in the project doc linked here https://docs.google.com/document/d/16YXWrR75RnRmkM_gURLAzwrEG7FUzK_pF6Z-rQ5g0j8/edit?tab=t.0 |
|
The issue this change is supposed to fix is allowing an important customer to persist a bunch of wallets without having to export them all each session (they need to export many wallets each session). For normal client side signature use you definitely should just use the normal flow (what you described) but in this case, they want tens of export bundles stored in the frames persistent storage for as long as possible So like you mentioned, to initiate signing, instead of having to export and load a 100 wallets (which takes up to 2 mins), you just need to inject the exported key that can decrypt all the bundles stored in the iframe local storage |
Added the ability to store encrypted wallet account export bundles in the
export-and-signiframe's local storage, and decrypt + sign transactions & messages using an injected "escrow" private key export bundle.General Flow:
Now all our encrypted bundles are safely stored encrypted in local storage but decrypted in memory! The escrow private key is never kept in persistent storage and must be re-injected whenever the iframe gets destroyed.
Additional helpers events include:
Sorta open questions:
Quick Demo 🎉
https://www.loom.com/share/5b3c9d5427414629b42e81c9278e5df0
https://www.loom.com/share/3dca52882847484fa8df2738329fc997
Project doc: https://docs.google.com/document/d/16YXWrR75RnRmkM_gURLAzwrEG7FUzK_pF6Z-rQ5g0j8/edit?tab=t.0