[PB-6479]: feature/encrypt email for externals#61
Open
xabg2 wants to merge 1 commit into
Open
Conversation
|
📝 WalkthroughWalkthroughThe PR introduces support for encrypted delivery to external mail recipients by distinguishing Internxt users (using their public keys) from external recipients (using a server public key). The EncryptionState type is renamed from encrypted/cleartext to internxt/external, the send pipeline always builds encryption blocks while setting differentiated delivery modes, and comprehensive tests validate both paths. ChangesExternal Recipients Encryption Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/compose-message/hooks/useComposeSend.test.ts (1)
86-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd blank-line separation between AAA sections.
As per coding guidelines, tests should use blank-line separation between Arrange, Act, and Assert sections. This improves readability by visually distinguishing test phases.
Example for lines 86-99:
♻️ Proposed refactor
test('When the active domains have not resolved, then the send is blocked', async () => { mocks.activeDomains = undefined; const { result, onSent } = renderSend({ toRecipients: [recipient('bob@inxt.me')] }); + expect(result.current.encryptionState).toBe('unknown'); + await act(async () => { await result.current.send(); }); + expect(show).toHaveBeenCalledWith(expect.objectContaining({ text: 'errors.mail.encryptionUnavailable' })); expect(mocks.sendEmail).not.toHaveBeenCalled(); expect(onSent).not.toHaveBeenCalled(); });Apply similar blank-line separation to all test cases in this file.
Also applies to: 101-112, 127-142, 144-171, 189-222, 224-241
🤖 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.test.ts` around lines 86 - 99, Update the test "When the active domains have not resolved, then the send is blocked" (and the other listed tests) to insert a blank line between the Arrange, Act and Assert sections so each phase is visually separated: e.g. separate the setup (mocks.activeDomains = undefined; const { result, onSent } = renderSend(...); expect(result.current.encryptionState)...), the act (await act(async () => { await result.current.send(); });), and the assertions (expect(show)... expect(mocks.sendEmail)... expect(onSent)...). Apply the same blank-line separation to the other tests referenced (lines 101-112, 127-142, 144-171, 189-222, 224-241) so Arrange/Act/Assert blocks are clearly separated around calls to renderSend, act, send, and subsequent expect assertions.Source: Coding guidelines
src/services/network/index.ts (1)
40-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the optional-decryption behavior explicit in the method contract.
At Line 46,
attachmentsSessionKeyis required (Uint8Array), so Line 50’s guard is ineffective for typed callers and decryption still always executes. If this path must support non-encrypted payloads, the parameter should be optional and decryption should only run when a key is actually provided.Suggested patch
async download({ mailId, blobId, name, type, attachmentsSessionKey, }: { mailId: string; blobId: string; name: string; type: string; - attachmentsSessionKey: Uint8Array; + attachmentsSessionKey?: Uint8Array; }): Promise<{ blob: Blob; name: string }> { const { data, contentType, fileName } = await MailService.instance.downloadAttachment(mailId, blobId, name, type); let payload = new Uint8Array(data) as BlobPart; - if (attachmentsSessionKey) { + if (attachmentsSessionKey?.length) { payload = (await decryptSymmetrically(attachmentsSessionKey, new Uint8Array(data))) as BlobPart; } const blob = new Blob([payload], { type: contentType || type });🤖 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/services/network/index.ts` around lines 40 - 53, The attachmentsSessionKey parameter is currently typed as required Uint8Array but the code conditionally checks it, so change the method signature/destructured param type to attachmentsSessionKey?: Uint8Array (or Uint8Array | undefined) and update any callers to pass undefined when no key is available; keep the guard if (attachmentsSessionKey) before calling decryptSymmetrically so decryption only runs when a key is provided, and ensure payload is constructed from either the raw data or the decrypted result (use new Uint8Array(data) for the non-encrypted path and the decrypted Uint8Array for the encrypted path) while preserving the returned Blob creation and types.
🤖 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.
Outside diff comments:
In `@src/components/compose-message/hooks/useComposeSend.test.ts`:
- Around line 86-99: Update the test "When the active domains have not resolved,
then the send is blocked" (and the other listed tests) to insert a blank line
between the Arrange, Act and Assert sections so each phase is visually
separated: e.g. separate the setup (mocks.activeDomains = undefined; const {
result, onSent } = renderSend(...); expect(result.current.encryptionState)...),
the act (await act(async () => { await result.current.send(); });), and the
assertions (expect(show)... expect(mocks.sendEmail)... expect(onSent)...). Apply
the same blank-line separation to the other tests referenced (lines 101-112,
127-142, 144-171, 189-222, 224-241) so Arrange/Act/Assert blocks are clearly
separated around calls to renderSend, act, send, and subsequent expect
assertions.
In `@src/services/network/index.ts`:
- Around line 40-53: The attachmentsSessionKey parameter is currently typed as
required Uint8Array but the code conditionally checks it, so change the method
signature/destructured param type to attachmentsSessionKey?: Uint8Array (or
Uint8Array | undefined) and update any callers to pass undefined when no key is
available; keep the guard if (attachmentsSessionKey) before calling
decryptSymmetrically so decryption only runs when a key is provided, and ensure
payload is constructed from either the raw data or the decrypted result (use new
Uint8Array(data) for the non-encrypted path and the decrypted Uint8Array for the
encrypted path) while preserving the returned Blob creation and types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a6249b5-1075-4867-b3eb-6d6c0b182af0
📒 Files selected for processing (7)
.env.examplesrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/index.tsxsrc/services/config/index.tssrc/services/network/index.tssrc/types/config/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support for encrypting emails sent to external recipients using the server's public key. With this approach, the server acts as the recipient of the encrypted payload, receiving the encrypted email and attachments, decrypting them on the server side, and then delivering the content to the final recipients.
Currently, when an email includes both external recipients and Internxt users, the backend sends the email through the external-recipient flow, meaning it is processed as encrypted for all recipients. In a future improvement, recipients should be grouped by type (external recipients vs. Internxt users) so that each group can receive the email through the appropriate delivery mechanism: server-side processing for external recipients and the standard Internxt flow for Internxt users.