[_]: refactor: update encryption handling and address management in email components#71
Conversation
jzunigax2
commented
Jul 14, 2026
jzunigax2
commented
- Enhanced the encryption logic to include recipient addresses for better key management, removing trial decryption logic.
- Updated the versioning of encryption blocks from v1 to v2 across various components and tests.
- Implemented checks to prevent sending encrypted emails with Bcc recipients.
- Adjusted related tests to reflect changes in encryption handling and recipient address management.
…components - Enhanced the encryption logic to include recipient addresses for better key management. - Updated the versioning of encryption blocks from v1 to v2 across various components and tests. - Implemented checks to prevent sending encrypted emails with Bcc recipients, ensuring compliance with encryption protocols. - Adjusted related tests to reflect changes in encryption handling and recipient address management.
📝 WalkthroughWalkthroughThe mail encryption envelope was migrated to v2 with recipient-labeled wrapped keys, separate preview encryption, and embedded attachment session keys. Decryption consumers now supply the current address. Encrypted Internxt messages with Bcc are blocked with localized warnings. ChangesEncrypted mail v2
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying mail-web with
|
| Latest commit: |
4b7bbf4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9b922ee2.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://refactor-remove-trial-decryp.mail-web-ea0.pages.dev |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-message/hooks/useComposeSend.ts`:
- Around line 96-100: Update the send function to check the result of
handleInheritAttachments immediately after awaiting it, and return early when it
is undefined before dispatching the email; preserve normal sending when
attachment recovery succeeds.
In `@src/hooks/mail/useAttachmentsSessionKey.ts`:
- Around line 20-24: Update the cache guard and cache entries in the effect
within useAttachmentsSessionKey so cached results are keyed by the normalized
address in addition to mailId, preventing reuse across account switches; include
envelope and keypair identity as cache inputs where available. Preserve existing
success and failure handling, and add a regression test covering an address
change that must not reuse the previous cached entry.
In `@src/hooks/mail/useDecryptedPreviews.ts`:
- Around line 9-21: Update decryptPendingPreviews to start all
decryptSummaryPreview operations concurrently and await them with Promise.all,
then populate the resolved record while preserving each summary.id mapping and
existing error-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72a0ff4c-3765-491a-8c8f-bdb5ef6db84f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonsrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/hooks/useDraftMessage.test.tssrc/components/compose-message/index.tsxsrc/hooks/mail/useAttachmentsSessionKey.test.tsxsrc/hooks/mail/useAttachmentsSessionKey.tssrc/hooks/mail/useDecryptedPreviews.test.tsxsrc/hooks/mail/useDecryptedPreviews.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/mail-encryption/index.test.tssrc/services/mail-encryption/index.tssrc/services/sdk/mail/mail.service.test.ts
|
|
||
| if (pendingInherited.length > 0) { | ||
| const senderKeysForAttachments = MailKeysService.instance.getCurrentKeys(); | ||
| if (!senderKeysForAttachments) { | ||
| const senderAddress = MailKeysService.instance.getCurrentAddress(); | ||
| if (!senderKeysForAttachments || !senderAddress) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Abort email dispatch if inherited attachments fail to process.
When handleInheritAttachments encounters missing keys, missing addresses, or decryption errors, it displays an error toast and returns undefined. However, the send function executes const attachmentsToSend = await handleInheritAttachments(); and immediately proceeds without verifying if the result is undefined.
This causes the email to be silently sent without its attachments if processing fails. To prevent this data loss, verify the return value in the send function and abort the operation if the attachment recovery failed.
// In `send` (around line 183):
const attachmentsToSend = await handleInheritAttachments();
if (!attachmentsToSend) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useComposeSend.ts` around lines 96 -
100, Update the send function to check the result of handleInheritAttachments
immediately after awaiting it, and return early when it is undefined before
dispatching the email; preserve normal sending when attachment recovery
succeeds.
| const [cache, setCache] = useState<Record<string, CachedKey>>({}); | ||
|
|
||
| useEffect(() => { | ||
| if (!mailId || !envelope || !keypair) return; | ||
| if (!mailId || !envelope || !keypair || !address) return; | ||
| if (cache[mailId]) return; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Key the cache by all decryption inputs, not only mailId.
When the address changes, the effect reruns but immediately accepts the previous address’s cached entry. This can return a stale cross-account session key—or retain a prior failure. Include at least the normalized address, and ideally envelope/keypair identity, in the cache entry and add an address-switch regression test.
Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/mail/useAttachmentsSessionKey.ts` around lines 20 - 24, Update the
cache guard and cache entries in the effect within useAttachmentsSessionKey so
cached results are keyed by the normalized address in addition to mailId,
preventing reuse across account switches; include envelope and keypair identity
as cache inputs where available. Preserve existing success and failure handling,
and add a regression test covering an address change that must not reuse the
previous cached entry.
| const decryptPendingPreviews = async ( | ||
| pending: Summary[], | ||
| keypair: HybridKeyPair, | ||
| address: string, | ||
| ): Promise<Record<string, string>> => { | ||
| const resolved: Record<string, string> = {}; | ||
| for (const summary of pending) { | ||
| try { | ||
| resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview(summary.encryption!, keypair); | ||
| resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview( | ||
| summary.encryption!, | ||
| keypair, | ||
| address, | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Process preview decryptions concurrently.
Currently, the loop processes preview decryptions sequentially. Since WebCrypto operations are asynchronous and offloaded to the browser's crypto thread, using Promise.all to decrypt them concurrently will significantly improve performance when rendering a view with multiple encrypted previews.
♻️ Proposed refactor
-const decryptPendingPreviews = async (
- pending: Summary[],
- keypair: HybridKeyPair,
- address: string,
-): Promise<Record<string, string>> => {
- const resolved: Record<string, string> = {};
- for (const summary of pending) {
- try {
- resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview(
- summary.encryption!,
- keypair,
- address,
- );
- } catch (error) {
- console.error('Failed to decrypt mail preview', { mailId: summary.id, error });
- }
- }
- return resolved;
-};
+const decryptPendingPreviews = async (
+ pending: Summary[],
+ keypair: HybridKeyPair,
+ address: string,
+): Promise<Record<string, string>> => {
+ const resolved: Record<string, string> = {};
+ await Promise.all(
+ pending.map(async (summary) => {
+ try {
+ resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview(
+ summary.encryption!,
+ keypair,
+ address,
+ );
+ } catch (error) {
+ console.error('Failed to decrypt mail preview', { mailId: summary.id, error });
+ }
+ })
+ );
+ return resolved;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const decryptPendingPreviews = async ( | |
| pending: Summary[], | |
| keypair: HybridKeyPair, | |
| address: string, | |
| ): Promise<Record<string, string>> => { | |
| const resolved: Record<string, string> = {}; | |
| for (const summary of pending) { | |
| try { | |
| resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview(summary.encryption!, keypair); | |
| resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview( | |
| summary.encryption!, | |
| keypair, | |
| address, | |
| ); | |
| const decryptPendingPreviews = async ( | |
| pending: Summary[], | |
| keypair: HybridKeyPair, | |
| address: string, | |
| ): Promise<Record<string, string>> => { | |
| const resolved: Record<string, string> = {}; | |
| await Promise.all( | |
| pending.map(async (summary) => { | |
| try { | |
| resolved[summary.id] = await MailEncryptionService.instance.decryptSummaryPreview( | |
| summary.encryption!, | |
| keypair, | |
| address, | |
| ); | |
| } catch (error) { | |
| console.error('Failed to decrypt mail preview', { mailId: summary.id, error }); | |
| } | |
| }) | |
| ); | |
| return resolved; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/mail/useDecryptedPreviews.ts` around lines 9 - 21, Update
decryptPendingPreviews to start all decryptSummaryPreview operations
concurrently and await them with Promise.all, then populate the resolved record
while preserving each summary.id mapping and existing error-handling behavior.
2 participants
