feat: accounts sync between the mobile app and the extension - #2625
feat: accounts sync between the mobile app and the extension#2625sonytooo wants to merge 10 commits into
Conversation
Adds the ground work for syncing accounts between the Ambire mobile app and the browser extension over animated QR codes. - libs/accountsSync: the transported payload (existing types only), its serialization and a strict parser for the scanned bytes - keystore: exportForSync collects the selected keys, the seeds they were derived from and the password wrapped main key, all still encrypted; importFromSync unwraps the other device's main key with its password and re-encrypts everything with the local one - libs/keystore: decryptMainKeyWithSecret, the inverse of encryptMainKeyWithSecret, now also used by the GCM unlock path - addSeed can preserve the id a seed had on the other device, so synced keys keep pointing to the seed they were derived from Fixes a bug where the isReadyToStoreKeys setter added the queued internal and external keys in parallel; both persist the same storage entry, so one of the two sets was lost. They are now added sequentially, together with the seeds queued by a sync that happened before the device password was set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- accounts: getAccountsForSync returns the records of the selected accounts - main: exportAccountsForSync collects them together with the keys controlling them and sends the payload to the UI over the one time data channel, so it never lands in persisted state - main: importAccountsFromSync parses the scanned data, hands the keys to the keystore and only then adds the accounts, so a wrong password of the other device leaves nothing behind Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The UI awaits the export payload and the import result through the existing one time data channel (requestId), instead of watching a transient status, which mobile can collapse before the UI renders it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- accounts with no keys, accounts controlled by a hardware wallet, re-syncing the same accounts and syncing keys whose seed the device already has - exportForSync refuses to export keys or seeds that are still on the old encryption, so it fails on the device that can explain it instead of on the other one - lock() drops the queued keys and seeds, as they hold decrypted material while waiting for the device password during onboarding Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| getAccountsForSync(addrs: Account['addr'][]): Account[] { | ||
| return this.accounts.filter((account) => addrs.includes(account.addr)) | ||
| } | ||
|
|
There was a problem hiding this comment.
imo we don't need a dedicated method for this as it's a simple filter that is also not sync specific
| const isIdTaken = !!id && this.#keystoreSeeds.some((s) => s.id === id) | ||
| const newEntry: StoredKeystoreSeed = { | ||
| id: generateUuid(), | ||
| id: isIdTaken || !id ? generateUuid() : id, |
There was a problem hiding this comment.
do we really need to check for a seed with the same id? #findStoredSeed already checks for the same seed content (perhaps we can also add a quick id check there too) and generateUuid will never generate a colliding uuid
| /** | ||
| * Adds the seeds and keys that were queued while the keystore was not ready to store | ||
| * them yet (e.g. accounts synced from another device during onboarding, where the | ||
| * device password is set afterwards). | ||
| * | ||
| * Everything runs sequentially, because internal and external keys are persisted | ||
| * under the same storage key and would otherwise overwrite each other. | ||
| */ | ||
| async #addQueuedKeysAndSeeds() { | ||
| const seedsToAdd = this.#seedsToAddOnKeystoreReady | ||
| const internalKeysToAdd = this.#internalKeysToAddOnKeystoreReady | ||
| const externalKeysToAdd = this.#externalKeysToAddOnKeystoreReady | ||
| this.#seedsToAddOnKeystoreReady = [] | ||
| this.#internalKeysToAddOnKeystoreReady = [] | ||
| this.#externalKeysToAddOnKeystoreReady = [] | ||
|
|
||
| try { | ||
| // Seeds first, so the keys can keep pointing to the seed they were derived from | ||
| for (const seed of seedsToAdd) { | ||
| await this.#addSeed(seed) | ||
| } | ||
| await this.#addKeys(internalKeysToAdd) | ||
| await this.#addKeysExternallyStored(externalKeysToAdd) | ||
| } catch (error: any) { | ||
| this.emitError({ | ||
| level: 'major', | ||
| message: | ||
| 'Something went wrong when saving your keys. Please try again or contact support if the problem persists.', | ||
| error: error instanceof Error ? error : new Error('keystore: failed to add queued keys') | ||
| }) | ||
| } finally { | ||
| this.emitUpdate() | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice one 👏 Perhaps we can also emitUpdate only once at the end? (pass emitUpdate=false to all private methods)
| const entropy = await decryptWithKey(exportedMainKey, seed.seed) | ||
| const seedPassphrase = seed.seedPassphrase | ||
| ? new TextDecoder().decode(await decryptWithKey(exportedMainKey, seed.seedPassphrase)) | ||
| : null | ||
|
|
||
| syncedSeedIds[seed.id] = await this.#storeSyncedSeed({ | ||
| id: seed.id, | ||
| seed: reconstructSeedFromEntropy(entropy, seedPassphrase), |
There was a problem hiding this comment.
should we reuse this logic in some way?
| // The exporting device's main key. Kept in this scope only, never persisted. | ||
| const exportedMainKey = await this.#decryptMainKeyWithSecret(secretKey, secret.aesEncrypted) | ||
|
|
There was a problem hiding this comment.
Technically it does get persisted in memory because we import it with extractable=true. I don't know why but I 'm a bit worried to have the main key of a device transferred, even encrypted. The reason is that changing the password of the device cannot defend it from a leaked main key. Can't we derive a new key to reencrypt the payload and then use it to decrypt in the new device (instead of leaking the original main key). This shouldn't really be a concern because users will be syncing between their own devices, just sharing so we can discuss it.
| "events": "^3.3.0", | ||
| "hash-wasm": "^4.12.0", | ||
| "js-yaml": "^4.1.0", | ||
| "pako": "^2.1.0", |
There was a problem hiding this comment.
Just fyi https://dev.to/parsajiravand/youre-importing-pako-to-gzip-data-compressionstream-does-it-natively-4d7b. I don't have a strong opinion
| for (const seed of seedsToAdd) { | ||
| await this.#addSeed(seed) | ||
| } |
| if (secret.aesEncrypted.cipherType !== CIPHER) | ||
| throw new Error('keystore: synced main key is not encrypted with GCM') | ||
|
|
||
| const secretKey = await deriveSecret(this.#scryptAdapter, password, secret.scryptParams.salt) |
There was a problem hiding this comment.
Why do we pass only scryptParams.salt? The function should read the secret's params as this will silently break if the extension and mobile app have different configurations
2 participants
Business logic for syncing selected accounts and their keys between the Ambire mobile app and the browser extension over animated QR codes. Platform-agnostic — the same controller methods serve both products.
Added
libs/accountsSync—AccountsSyncPayloadv1 ({ v, secret, accounts, keys, seeds }) plusserializeAccountsSyncPayload/parseAccountsSyncPayload. The payload is gzipped (pako) before it is hex encoded for the UR frames, because JSON full of hex strings compresses 3x or better, and inflation is capped at 2MB so a hostile QR code can't balloon the background's memory.KeystoreController.exportForSync(keyAddrs, includeSeeds)— returns thepasswordsecret entry, the selectedStoredKeys and (optionally) the recovery phrases they were derived from, all read as stored. Nothing is decrypted, so it works while locked; refuses when the device has nopasswordsecret.KeystoreController.importFromSync(payload, password)— derives the other device's main key from its password, decrypts the seeds and private keys and re-encrypts them under the local main key through the existing#addSeed/#addKeys/#addKeysExternallyStored. Wrong password reuses the existing silentIncorrect passwordpath.AccountsController.getAccountsForSync(addrs)— the selectedAccountrecords, preferences and all.MainController.exportAccountsForSync/importAccountsFromSync— onewithStatus-wrapped method per direction. Export ships the payload through the one-time-data channel (never persisted state); import parses and unlocks the key material before#updateAccounts, so a wrong password adds neither keys nor accounts. Both accept an optional trailingrequestIdand reply through it, so the UI can await the result.Changed / fixed along the way
decryptMainKeyWithSecretextracted intolibs/keystore(the exact inverse of the existingencryptMainKeyWithSecret) and reused by#unlockWithSecretGCM, so the unwrap + wrong-password mapping exists once.#addSeedtakes an optionalidand returns the stored id, so a synced seed keeps the id the other device gave it and the synced keys'meta.fromSeedIdkeeps resolving.isReadyToStoreKeyssetter fired#addKeysand#addKeysExternallyStoredin parallelvoidcalls, and both rewrite the wholekeystoreKeysstorage entry — whichever finished last dropped the other's keys. No existing flow queued both at once; syncing does. They now flush sequentially through#addQueuedKeysAndSeeds(seeds included), which clears the queues, reports failures viaemitErrorinstead of an unhandled rejection, and alwaysemitUpdates.#seedsToAddOnKeystoreReady, next to the two existing key queues, so an import that happens before the device password is set still lands.Security notes
passwordsecret entry travel.passwordsecret is exported;biometricsand email-vault secrets are device-bound and excluded.scryptN=131072 is the cost barrier, and both designs warn that the QR code carries sensitive information.Tests
accountsSync.test.ts, 7 new cases inkeystore.test.tsand a newmainAccountsSync.test.tswith two full device-to-device round trips: subset export, signer works on the importing device, preferences travel, onboarding order (import → set password → keys land), wrong password imports nothing, foreign QR data imports nothing, empty selection exports nothing, double sync doesn't duplicate.