Skip to content

Added remove keysStore secret into emailVault controller#2341

Merged
JIOjosBG merged 19 commits into
v2from
email-vault-remove-key-store-secret
May 18, 2026
Merged

Added remove keysStore secret into emailVault controller#2341
JIOjosBG merged 19 commits into
v2from
email-vault-remove-key-store-secret

Conversation

@stojnovsky

@stojnovsky stojnovsky commented Apr 28, 2026

Copy link
Copy Markdown
Member

@JIOjosBG JIOjosBG self-requested a review May 7, 2026 15:02
@JIOjosBG

JIOjosBG commented May 8, 2026

Copy link
Copy Markdown
Member

/security-review

@JIOjosBG JIOjosBG marked this pull request as ready for review May 8, 2026 15:42
@JIOjosBG

JIOjosBG commented May 8, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown

Preparing review...

@JIOjosBG JIOjosBG requested review from jordan-enev and removed request for JIOjosBG May 8, 2026 15:49
@JIOjosBG

Copy link
Copy Markdown
Member

/review

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
📝 TODO sections

⚡ Recommended focus areas for review

State Leak

If an exception is thrown during #removeKeyStoreSecret (for example, the relayer call rejects or the keystore throws), #isRemovingSecret is never reset to false because the method lacks a try/finally block. This permanently leaves the controller stuck in the RemovingSecret state until the extension is restarted.

  this.#isRemovingSecret = true
  const keyStoreUid = await this.#keyStore.getKeyStoreUid()
  result = await this.#emailVault.removeKeyStoreSecretFromRelayer(
    email,
    magicKey.key,
    keyStoreUid
  )
  await this.#keyStore.removeSecret(RECOVERY_SECRET_ID)
} else
  this.emitError({
    message: 'Email key not confirmed',
    level: 'minor',
    sendCrashReport: false,
    error: new Error('removeKeyStoreSecret: not confirmed magic link key')
  })

if (result) {
  await this.#getEmailVaultInfo(email, 'setup')
} else {
  this.emitError({
    level: 'minor',
    message: 'Error upload keyStore to email vault',
    error: new Error('error removing keyStore secret from email vault')
  })
}

this.#isRemovingSecret = false
this.emitUpdate()
Inconsistent Local Deletion

this.#keyStore.removeSecret(RECOVERY_SECRET_ID) is executed unconditionally immediately after the relayer call, even when removeKeyStoreSecretFromRelayer returns false. If cloud removal fails, the local recovery secret is still deleted, leaving the user's local state inconsistent and potentially preventing recovery.

if (magicKey?.key) {
  this.#isRemovingSecret = true
  const keyStoreUid = await this.#keyStore.getKeyStoreUid()
  result = await this.#emailVault.removeKeyStoreSecretFromRelayer(
    email,
    magicKey.key,
    keyStoreUid
  )
  await this.#keyStore.removeSecret(RECOVERY_SECRET_ID)
} else
  this.emitError({
    message: 'Email key not confirmed',
    level: 'minor',
    sendCrashReport: false,
    error: new Error('removeKeyStoreSecret: not confirmed magic link key')
  })
Wrong Error Message

The failure branch in #removeKeyStoreSecret emits the error message "Error upload keyStore to email vault", which is copy-pasted from the upload flow. This misleading message makes debugging removal failures harder.

this.emitError({
  level: 'minor',
  message: 'Error upload keyStore to email vault',
  error: new Error('error removing keyStore secret from email vault')
})

@JIOjosBG JIOjosBG merged commit d44673f into v2 May 18, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants