[PB-6470]: feature/encrypt and decrypt the attachments mail#60
Merged
Conversation
Deploying mail-web with
|
| Latest commit: |
4013c68
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc87990f.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-encrypt-attachment.mail-web-ea0.pages.dev |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements encrypted attachment handling end-to-end: refactors compose-side hooks to manage per-session attachment encryption keys, introduces NetworkService for encrypted upload/download with retry logic, modernizes UploadManager to use session-scoped instances, extends mail decryption to expose envelope data for key recovery, and integrates encrypted downloads in the mail preview. ChangesEncrypted attachments pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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 |
jzunigax2
reviewed
Jun 8, 2026
Comment on lines
+199
to
+212
| /** | ||
| * Encrypt the session key for the attachments | ||
| * @param sessionKey - The session key to encrypt | ||
| * @param email - The email of the user to encrypt for | ||
| * @param userPubKey - The user public key of the user to encrypt for | ||
| * @returns - The hybrid ciphertext and encrypted key | ||
| */ | ||
| async encryptAttachmentSessionKey(sessionKey: Uint8Array<ArrayBufferLike>, email: string, userPubKey: string) { | ||
| const enc = await encryptKeysHybrid(sessionKey, { | ||
| email, | ||
| publicHybridKey: base64ToUint8Array(userPubKey), | ||
| }); | ||
| return { hybridCiphertext: enc.hybridCiphertext, encryptedKey: enc.encryptedKey }; | ||
| } |
Contributor
There was a problem hiding this comment.
this function is not called anywhere
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/mail-encryption/index.test.ts (1)
169-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the parse fixture to match the current encryption envelope contract.
This fixture omits
attachmentWrappedKeys, so the parse test does not validate the full envelope shape used by current encrypted attachments flow.Proposed fix
const block = { version: 'v1' as const, encryptedText: 'et', encryptedPreview: 'ep', wrappedKeys: [{ hybridCiphertext: 'h', encryptedKey: 'k' }], + attachmentWrappedKeys: [{ hybridCiphertext: 'ah', encryptedKey: 'ak' }], };🤖 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/mail-encryption/index.test.ts` around lines 169 - 176, The test fixture for parseEncryptionBlock is missing the attachmentWrappedKeys field required by the current envelope contract; update the expected `block` object in the test (used with `ENCRYPTED_EMAIL_PREFIX` and parsed by `mailEncryption.parseEncryptionBlock`) to include an `attachmentWrappedKeys` property (e.g., an empty array or appropriate wrapped key entries) so the parsed shape matches the full encryption envelope used by encrypted attachments.
🤖 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/useAttachments.test.ts`:
- Around line 44-58: Move the test-suite mock-restoration call into the setup
block: remove vi.restoreAllMocks() from afterEach and add vi.restoreAllMocks()
at the start of beforeEach (before any stubbing/resetting like vi.stubGlobal,
enqueue.mockReset, retryFn.mockReset, etc.) so mocks are restored before each
test is configured; keep vi.unstubAllGlobals() in afterEach if needed for global
cleanup.
In `@src/features/mail/components/mail-preview/preview/index.tsx`:
- Around line 44-55: The Preview component currently performs crypto key
derivation and async download side effects in its onDownload handler; extract
that logic into a new hook (e.g., usePreviewDownload) so Preview is render-only.
Create usePreviewDownload(mailId, envelope) that encapsulates
useAttachmentsSessionKey, any key derivation, and the async
NetworkService.instance.download orchestration, and returns a stable onDownload
callback plus any loading/error states and the attachmentsSessionKey if needed.
Replace the in-component onDownload and the useAttachmentsSessionKey call in
Preview with the hook's returned values and pass only render-ready
callbacks/data (sanitizedBody, attachments, subject, onDownload) into the
Preview component. Ensure the hook handles errors and side effects and that
Preview contains no async logic or crypto work.
- Around line 51-55: The call to NetworkService.instance.download currently
passes attachmentsSessionKey! which can be null and causes failures; add a
null-safe guard before invoking NetworkService.instance.download (inside the
preview component/function that performs the download) so that if
attachmentsSessionKey is null/undefined you skip or defer the download and
render a safe placeholder or retry logic instead of calling download with an
invalid value; only call NetworkService.instance.download with a validated
Uint8Array attachmentsSessionKey, and use the existing mailId and attachment
objects when the key is present.
In `@src/hooks/mail/useAttachmentsSessionKey.test.tsx`:
- Around line 23-29: The test setup resets mocks in afterEach but policy
requires restoring mocks at the start of each test; move vi.restoreAllMocks()
into the beforeEach block so it runs before
useMailKeysMock.mockReturnValue(KEYPAIR) (i.e., update the beforeEach containing
useMailKeysMock to first call vi.restoreAllMocks()), and remove
vi.restoreAllMocks() from afterEach, ensuring useMailKeysMock and other mocks
are reset before Arrange.
In `@src/services/mail-encryption/index.ts`:
- Around line 180-182: The decryptAttachmentsSessionKey method should validate
envelope.attachmentWrappedKeys before calling trialDecryptKey so malformed
envelopes raise an EnvelopeDecryptionError instead of a raw runtime error;
inside decryptAttachmentsSessionKey (the function shown) check that
envelope.attachmentWrappedKeys is present and is an array (and optionally
non-empty/contains expected entries), and if the check fails throw new
EnvelopeDecryptionError with a clear message, otherwise call and return
this.trialDecryptKey(envelope.attachmentWrappedKeys, keypair).
In `@src/services/network/index.ts`:
- Line 5: Replace the relative parent import for MailEncryptionService with the
repository path alias: update the import of MailEncryptionService (currently
"import { MailEncryptionService } from '../mail-encryption'") to use the
alias-based path (e.g. "`@/services/mail-encryption`") so internal modules under
src/ use the `@/`* alias; ensure the imported symbol name MailEncryptionService
remains unchanged.
- Line 51: The returned object currently sets name using "name ?? fileName",
which always selects the required input "name" and ignores a server-provided
canonical filename; change the fallback order so the server-provided "fileName"
is preferred when present (i.e., set the returned name to fileName ?? name) by
updating the return statement that currently returns { blob, name: name ??
fileName } to use the reversed fallback; keep the symbols blob, name, and
fileName intact.
In `@src/services/network/network.test.ts`:
- Around line 46-53: The test suite restores mocks in afterEach but policy
requires restoring before each test; update the setup so vi.restoreAllMocks() is
called at the start of the beforeEach block (before uploadAttachment.mockReset()
and encryptAttachment.mockClear()) to ensure a clean slate; locate the
beforeEach/afterEach in network.test.ts and move the restore call into
beforeEach (and remove it from afterEach) while keeping the
uploadAttachment.mockReset and encryptAttachment.mockClear calls after the
restore.
- Line 3: The import statement for NetworkService uses a relative path './index'
instead of the project's path alias. Replace the relative import path with the
appropriate `@/`* alias to align with the project's coding guidelines for internal
module imports in the NetworkService import statement.
In `@src/services/upload-manager/index.ts`:
- Around line 20-29: The UploadManager class in src/services breaks the service
singleton contract by requiring runtime constructor parameters; fix it by either
(A) turning UploadManager into a true singleton: make the constructor private,
add public static readonly instance = new UploadManager(), add an
init(sessionKey: Uint8Array, callbacks: UploadManagerCallbacks) method to set
session-scoped values (and update any code using new UploadManager(...) to call
UploadManager.instance.init(...)), and keep existing methods (process, q, jobs)
referencing the instance fields; or (B) move this class out of src/services into
a session-scoped module (e.g., src/lib or src/session) so it can remain
constructed per-session without violating the services contract — choose one
approach and update imports/usages accordingly (referencing class UploadManager,
constructor, process, jobs, q, and UploadManager.instance when applying option
A).
- Line 3: Replace the parent-relative import of NetworkService with the project
alias import pattern: update the import statement that currently imports
NetworkService (from '../network') to use the '`@/`...' alias (e.g., import {
NetworkService } from '`@/`...') so it conforms to the internal import rule for
src/**/*.{ts,tsx}; ensure the imported symbol name remains NetworkService and
that any exported path matches the network service module's alias entry.
- Around line 74-79: After a successful upload the job is not removed from the
jobs map causing stale state and allowing retry(id) for completed uploads;
update the success path in the upload handler to clear job.canceler (as in the
catch) and remove the job from this.jobs (e.g., this.jobs.delete(job.id)) before
calling this.callbacks.onSuccess(job.id, result.blobId) so completed entries are
not retained; keep the existing cancel/errored behavior unchanged.
In `@src/services/upload-manager/upload-manager.test.ts`:
- Line 2: Update the test import to use the repository internal-alias path
instead of a relative path: replace the current import of UploadManager and
UploadManagerCallbacks from './index' with the alias import that begins with
'`@/`...' so the file imports UploadManager and type UploadManagerCallbacks via
the internal alias (matching other src imports and repository conventions).
- Around line 26-32: Move the global mock restoration into the test setup by
calling vi.restoreAllMocks() inside the beforeEach and remove the
vi.restoreAllMocks() call from afterEach; specifically, update the beforeEach
block to call vi.restoreAllMocks() (then upload.mockReset() can run afterward)
and delete the afterEach block that currently calls vi.restoreAllMocks(),
referencing the existing beforeEach, afterEach, upload.mockReset, and
vi.restoreAllMocks symbols to locate the changes.
---
Outside diff comments:
In `@src/services/mail-encryption/index.test.ts`:
- Around line 169-176: The test fixture for parseEncryptionBlock is missing the
attachmentWrappedKeys field required by the current envelope contract; update
the expected `block` object in the test (used with `ENCRYPTED_EMAIL_PREFIX` and
parsed by `mailEncryption.parseEncryptionBlock`) to include an
`attachmentWrappedKeys` property (e.g., an empty array or appropriate wrapped
key entries) so the parsed shape matches the full encryption envelope used by
encrypted attachments.
🪄 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: 9032a5f9-f6c8-4b75-8a67-ba9a804ca7c1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
package.jsonsrc/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.tssrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/index.tsxsrc/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/index.tsxsrc/features/mail/components/mail-preview/preview/index.tsxsrc/hooks/mail/useAttachmentsSessionKey.test.tsxsrc/hooks/mail/useAttachmentsSessionKey.tssrc/hooks/mail/useDecryptedMail.test.tsxsrc/hooks/mail/useDecryptedMail.tssrc/services/mail-encryption/index.test.tssrc/services/mail-encryption/index.tssrc/services/network/index.tssrc/services/network/network.test.tssrc/services/sdk/mail/mail.service.test.tssrc/services/upload-manager/index.tssrc/services/upload-manager/upload-manager.test.tssrc/types/mail/upload-manager/index.ts
💤 Files with no reviewable changes (1)
- src/types/mail/upload-manager/index.ts
Comment on lines
+44
to
+55
| const Preview = ({ mailId, subject, body, attachments, envelope }: PreviewProps) => { | ||
| const { translate } = useTranslationContext(); | ||
| const sanitizedBody = purify.sanitize(body); | ||
| const attachmentsSessionKey = useAttachmentsSessionKey(mailId, envelope ?? null); | ||
|
|
||
| const onDownload = async (attachment: EmailAttachment) => { | ||
| try { | ||
| const { data, contentType } = await MailService.instance.downloadAttachment( | ||
| mailId, | ||
| attachment.blobId, | ||
| attachment.name, | ||
| attachment.type, | ||
| ); | ||
| const blob = new Blob([data], { type: contentType || attachment.type }); | ||
| const { blob, name } = await NetworkService.instance.download({ | ||
| ...attachment, | ||
| mailId: mailId, | ||
| attachmentsSessionKey: attachmentsSessionKey!, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move preview download orchestration into a hook, keep component render-only.
This component now owns crypto key derivation and async download side effects. Extract this into a use<...> hook and pass render-ready callbacks/data into the component. As per coding guidelines, “Components must be dumb; hooks should contain state/effects/event handlers.”
🤖 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/features/mail/components/mail-preview/preview/index.tsx` around lines 44
- 55, The Preview component currently performs crypto key derivation and async
download side effects in its onDownload handler; extract that logic into a new
hook (e.g., usePreviewDownload) so Preview is render-only. Create
usePreviewDownload(mailId, envelope) that encapsulates useAttachmentsSessionKey,
any key derivation, and the async NetworkService.instance.download
orchestration, and returns a stable onDownload callback plus any loading/error
states and the attachmentsSessionKey if needed. Replace the in-component
onDownload and the useAttachmentsSessionKey call in Preview with the hook's
returned values and pass only render-ready callbacks/data (sanitizedBody,
attachments, subject, onDownload) into the Preview component. Ensure the hook
handles errors and side effects and that Preview contains no async logic or
crypto work.
Source: Coding guidelines
| import type { UploadAttachmentResponse } from '@internxt/sdk/dist/mail/types'; | ||
| import type { UploadAttachmentCallbacks, UploadAttachmentTask, UploadHandle } from '@/types/mail/upload-manager'; | ||
| import type { RequestCanceler } from '@internxt/sdk/dist/shared/http/types'; | ||
| import { NetworkService } from '../network'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use alias import for the internal network service.
Line 3 should import via @/* instead of a parent-relative path.
Proposed fix
-import { NetworkService } from '../network';
+import { NetworkService } from '`@/services/network`';As per coding guidelines, src/**/*.{ts,tsx} requires internal imports to use the @/* alias.
🤖 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/upload-manager/index.ts` at line 3, Replace the parent-relative
import of NetworkService with the project alias import pattern: update the
import statement that currently imports NetworkService (from '../network') to
use the '`@/`...' alias (e.g., import { NetworkService } from '`@/`...') so it
conforms to the internal import rule for src/**/*.{ts,tsx}; ensure the imported
symbol name remains NetworkService and that any exported path matches the
network service module's alias entry.
Source: Coding guidelines
Comment on lines
+20
to
29
| export class UploadManager { | ||
| private readonly jobs: Map<string, UploadJob> = new Map(); | ||
| private readonly q: QueueObject<UploadJob>; | ||
|
|
||
| /** | ||
| * The retry method is used to restart an upload that has failed | ||
| * @param id - The ID of the upload task | ||
| * @param callbacks - The callbacks to call when the upload is complete (success or error) | ||
| * @returns - void | ||
| */ | ||
| retry(id: string, callbacks: UploadAttachmentCallbacks): void { | ||
| const existing = this.tasks.get(id); | ||
| if (!existing) return; | ||
| this.cancelTask(existing); | ||
| this.enqueueTask(id, existing.file, callbacks); | ||
| constructor( | ||
| private readonly sessionKey: Uint8Array, | ||
| private readonly callbacks: UploadManagerCallbacks, | ||
| ) { | ||
| this.q = queue<UploadJob>(this.process, UPLOAD_CONCURRENCY); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
UploadManager in src/services breaks the repo singleton service contract.
This service is constructed directly with runtime parameters and has no public static readonly instance. Either restore a singleton entrypoint or move this session-scoped orchestrator out of src/services to align with architecture constraints.
As per coding guidelines, services in src/services/**/*.ts must follow public static readonly instance = new ServiceName() and be consumed via ServiceName.instance.method().
🤖 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/upload-manager/index.ts` around lines 20 - 29, The UploadManager
class in src/services breaks the service singleton contract by requiring runtime
constructor parameters; fix it by either (A) turning UploadManager into a true
singleton: make the constructor private, add public static readonly instance =
new UploadManager(), add an init(sessionKey: Uint8Array, callbacks:
UploadManagerCallbacks) method to set session-scoped values (and update any code
using new UploadManager(...) to call UploadManager.instance.init(...)), and keep
existing methods (process, q, jobs) referencing the instance fields; or (B) move
this class out of src/services into a session-scoped module (e.g., src/lib or
src/session) so it can remain constructed per-session without violating the
services contract — choose one approach and update imports/usages accordingly
(referencing class UploadManager, constructor, process, jobs, q, and
UploadManager.instance when applying option A).
Source: Coding guidelines
| import { MailService } from '@/services/sdk/mail'; | ||
| import type { UploadAttachmentResponse } from '@internxt/sdk/dist/mail/types'; | ||
| import { AxiosResponseError } from '@internxt/sdk/dist/shared/types/errors'; | ||
| import { UploadManager, type UploadManagerCallbacks } from './index'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the internal alias import in this test file.
Line 2 should import through @/* to match repository import conventions.
Proposed fix
-import { UploadManager, type UploadManagerCallbacks } from './index';
+import { UploadManager, type UploadManagerCallbacks } from '`@/services/upload-manager`';As per coding guidelines, all src/**/*.{ts,tsx} files must use @/* for internal imports.
📝 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.
Suggested change
| import { UploadManager, type UploadManagerCallbacks } from './index'; | |
| import { UploadManager, type UploadManagerCallbacks } from '`@/services/upload-manager`'; |
🤖 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/upload-manager/upload-manager.test.ts` at line 2, Update the
test import to use the repository internal-alias path instead of a relative
path: replace the current import of UploadManager and UploadManagerCallbacks
from './index' with the alias import that begins with '`@/`...' so the file
imports UploadManager and type UploadManagerCallbacks via the internal alias
(matching other src imports and repository conventions).
Source: Coding guidelines
jzunigax2
approved these changes
Jun 8, 2026
|
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
2 participants
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.
Upload the attachment encrypted with a session key (which is encrypted with the pub key of each of the recipients) and decrypt the attachment before download it by using the session key (which is decrypted with the privkey of the user recipient).