✨(fullstack) add opt-in end-to-end encryption for transfers#14
Open
Nastaliss wants to merge 1 commit into
Open
✨(fullstack) add opt-in end-to-end encryption for transfers#14Nastaliss wants to merge 1 commit into
Nastaliss wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughAdds E2E transfer support across backend metadata, browser encryption, service-worker downloads, and transfer UI. It also removes the dev-only authentication bypass setting and route. ChangesEnd-to-end encrypted transfer flow
Dev auth bypass cleanup
Sequence Diagram(s)sequenceDiagram
participant DownloadView
participant e2eServiceWorker
participant handleDownload
participant DownloadFileView
participant S3
DownloadView->>e2eServiceWorker: registerE2eKey(...)
DownloadView->>handleDownload: request /_dl/{token}/{fileId}
handleDownload->>DownloadFileView: GET /download?as=json
DownloadFileView-->>handleDownload: {"url": presigned URL}
handleDownload->>S3: fetch presigned ciphertext URL
S3-->>handleDownload: ciphertext stream
handleDownload-->>DownloadView: plaintext attachment response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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/backend/core/api/serializers.py`:
- Around line 374-376: The key_fragment field in the serializer is accepting
arbitrary non-blank strings even though it must be URL-safe base64; add
validation in the serializer path that handles key_fragment so malformed values
are rejected before link generation, and apply the same check in the email-mode
flow referenced by the serializer methods that build these links. Use the
existing key_fragment field definition and the related validation/serialization
logic in this module to locate where to enforce the URL-safe base64 contract.
- Around line 237-281: The validate() method in the serializer currently checks
E2E presence flags but still allows inconsistent size metadata to slip through;
add validation in validate() for attrs.size, attrs.plaintext_size, and
attrs.encryption_chunk_size so they agree before persistence. Use the existing
validate() logic in the serializer to reject mismatches when e2e_encrypted is
true, and ensure complete_upload cannot accept a transfer whose stored
plaintext/chunk metadata would not match the actual upload.
In `@src/backend/core/api/viewsets/download.py`:
- Around line 238-239: The presigned-url JSON branch in the download viewset
returns a signed S3 URL in the response body without preventing caching. Update
the response created in the `request.query_params.get("as") == "json"` path so
it includes `Cache-Control: no-store`, using the existing response handling in
the download endpoint to ensure browsers and proxies do not retain the
direct-download credential.
In `@src/backend/core/api/viewsets/draft.py`:
- Around line 579-588: The invitation enqueue path in draft.py is passing the
E2E key fragment into Celery via send_recipient_invitations_task.delay, which
puts sensitive key material into the broker payload. Update the
transaction.on_commit callback and send_recipient_invitations_task invocation so
only non-sensitive transfer context is enqueued, and have the task fetch or
derive any needed data from server-side state instead of receiving key_fragment.
Use the existing key_fragment handling around transfer.e2e_encrypted as the spot
to remove the serialized secret from the task payload.
- Around line 137-175: Reject server-side imports when the draft is E2E
encrypted by updating the draft handling in the add-file flow around
TransferDraft and _get_locked_draft: if data requests or reuses an e2e_encrypted
draft and the upload comes from source_url/import rather than client-side
encrypted bytes, raise a ValidationError instead of continuing. Keep the
existing encryption-mode and encryption_chunk_size locking checks, and add an
explicit guard in the upload/import path so e2e_encrypted drafts can only accept
client-encrypted uploads, preventing complete_upload from trusting a plaintext
backend import.
In `@src/backend/core/tests/test_api_drafts.py`:
- Around line 689-702: Update the E2E draft-file tests around _add_e2e_file to
cover more than the single-chunk happy path: add cases for files larger than
CHUNK (for example CHUNK + 1) so multi-chunk behavior is exercised, and add
negative tests that submit inconsistent size and encryption_chunk_size metadata
to verify rejection. Use _add_e2e_file, CHUNK, OVERHEAD, and the add-file flow
in the draft API tests to locate the affected helpers and expand the assertions
accordingly.
In `@src/frontend/public/sw.js`:
- Line 29: The service-worker registry in REGISTRY currently retains decrypted
E2E transfer keys indefinitely, so update the service worker flow to clear them
when no longer needed. Add a TTL to entries and/or ensure the download page
sends an e2e-unregister message when it is finished, and wire the unregister
handling into the existing registry access paths so stale keys are removed
reliably.
- Around line 47-54: The e2e registration flow in sw.js only sends the
“e2e-register-ack” after registerKey succeeds, so failures from
crypto.subtle.importKey leave the page waiting indefinitely. Update the
data.type === "e2e-register" handling to catch rejections from registerKey(data)
and always notify event.source with an explicit failure acknowledgment or error
payload so the UI can switch to its error state. Keep the success ack behavior
unchanged, and make sure the failure path uses the same postMessage mechanism on
event.source.
In `@src/frontend/src/features/transfers/api/useTransferDraft.ts`:
- Around line 837-845: The finalize request in useTransferDraft must not include
the E2E key fragment in the `/finalize/` payload, because that leaks the
decryption key to the backend. Update the finalizeBody construction near the
fragment handling so it never adds key_fragment, and instead keep finalize
metadata key-free for the finalize flow; if email delivery needs the fragment,
handle that out-of-band or restrict E2E email to a flow that does not send the
key to the server.
- Around line 517-520: The generated transfer key fragment is only stored in
React state, so the submit flow can read a stale null before the state update
lands. Update the registerFile path to also write the fragment into a
synchronous ref (for example, the same pattern used by e2eKeyRef.current), and
change submit to read the fragment from that ref instead of e2eKeyFragment. Use
the existing registerFile and submit logic in useTransferDraft to keep the key
fragment available immediately for the finalizeBody payload.
In `@src/frontend/src/features/transfers/components/_transfers.scss`:
- Around line 845-849: Add the missing empty line before the existing comment in
the transfers stylesheet so it satisfies the
scss/double-slash-comment-empty-line-before rule; update the spacing in the
block around the gap declaration and the surrounding comment near the transfers
header styles so the comment is preceded by a blank line.
In `@src/frontend/src/features/transfers/components/DownloadView.tsx`:
- Around line 50-91: The download effect is re-reading window.location.hash on
every rerun, so after history.replaceState strips the fragment the effect can
incorrectly fall back to no-key. Update DownloadView’s useEffect to capture the
fragment once and keep it stable across reruns, e.g. store it in a ref or state
before clearing the URL, and then reuse that preserved value when calling
registerE2eKey instead of reading location.hash again. Ensure the existing e2e
state transitions in setE2eState still work from the retained fragment.
In `@src/frontend/src/features/transfers/components/TransferDetail.tsx`:
- Around line 101-105: The footer “Copy link” CTA in TransferDetail is still
shown as actionable even when e2e_encrypted transfers intentionally have an
empty downloadUrl, so it should be hidden or disabled when downloadUrl is blank.
Update the footer rendering logic in TransferDetail to gate the public-link
action on downloadUrl (or match the per-file disabled state), and keep the
existing copyLink() handler from being presented as available when it can only
no-op.
In `@src/frontend/src/features/transfers/upload/e2eCrypto.ts`:
- Around line 116-122: The ciphertextSize helper currently returns 0 for empty
inputs, but the upload flow still produces one encrypted empty part via
e2eCrypto, so the predicted size is too small. Update ciphertextSize to treat
plaintextSize === 0 as a single encrypted chunk that includes
CRYPTO_OVERHEAD_PER_CHUNK, and keep the existing chunk-based calculation
consistent with the upload path used by e2eCrypto and the uploader’s backend
size validation.
- Around line 75-85: encryptChunk and decryptChunk currently authenticate only
the ciphertext, so chunks can be swapped or replayed without detection. Update
the e2eCrypto helpers to accept additionalData and pass it into the AES-GCM
encrypt/decrypt calls, then update the useTransferDraft call sites to build
deterministic AAD from the file ID and part number for each chunk. Keep the API
change consistent across the encryptChunk/decryptChunk symbols so every chunk is
bound to its file and position.
In `@src/frontend/src/features/transfers/upload/e2eServiceWorker.ts`:
- Around line 33-45: The controller wait and the service-worker ack wait in
e2eServiceWorker can hang forever, so bound both with explicit timeouts and
rejection paths. Update the controllerchange/setup promise and the post-message
ack flow to reject after a reasonable timeout instead of only resolving, and
make sure the exported setup/handshake logic surfaces those failures so
DownloadView can recover. Use the existing e2eServiceWorker async flow and any
helper around controllerchange / ack handling as the place to add the timeout
and error propagation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4904718-9bc9-42d2-87ef-42440db0056e
📒 Files selected for processing (30)
README.mddocs/S3.mdsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/download.pysrc/backend/core/api/viewsets/draft.pysrc/backend/core/authentication/dev_bypass.pysrc/backend/core/authentication/urls.pysrc/backend/core/migrations/0006_e2e_encryption_fields.pysrc/backend/core/models.pysrc/backend/core/services/email.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_api_downloads.pysrc/backend/core/tests/test_api_drafts.pysrc/backend/transferts/settings.pysrc/frontend/caddy/Caddyfilesrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/sw.jssrc/frontend/src/features/api/types.tssrc/frontend/src/features/transfers/api/useTransferDraft.tssrc/frontend/src/features/transfers/components/DownloadView.tsxsrc/frontend/src/features/transfers/components/TransferDetail.tsxsrc/frontend/src/features/transfers/components/TransferForm.tsxsrc/frontend/src/features/transfers/components/TransferSuccess.tsxsrc/frontend/src/features/transfers/components/_transfers.scsssrc/frontend/src/features/transfers/upload/MultipartUploader.tssrc/frontend/src/features/transfers/upload/e2eCrypto.test.tssrc/frontend/src/features/transfers/upload/e2eCrypto.tssrc/frontend/src/features/transfers/upload/e2eServiceWorker.tssrc/frontend/src/routes/_app/confirm/$id.tsx
💤 Files with no reviewable changes (3)
- src/backend/core/authentication/urls.py
- src/backend/core/authentication/dev_bypass.py
- src/backend/transferts/settings.py
Comment on lines
+374
to
+376
| key_fragment = serializers.CharField( | ||
| required=False, allow_blank=True, default="", max_length=128 | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate key_fragment against the URL-safe base64 contract.
The field is documented as URL-safe base64, but any non-blank string is accepted in email mode. Reject malformed fragments before emailing links recipients cannot decode.
Proposed validation
if mode != SharingMode.EMAIL and attrs.get("key_fragment"):
raise serializers.ValidationError(
{"key_fragment": "Only allowed in email mode."}
)
+ fragment = attrs.get("key_fragment", "")
+ if fragment and not all(
+ ("A" <= c <= "Z") or ("a" <= c <= "z") or ("0" <= c <= "9") or c in "-_"
+ for c in fragment
+ ):
+ raise serializers.ValidationError(
+ {"key_fragment": "Must be URL-safe base64."}
+ )
return attrsAlso applies to: 389-396
🤖 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/backend/core/api/serializers.py` around lines 374 - 376, The key_fragment
field in the serializer is accepting arbitrary non-blank strings even though it
must be URL-safe base64; add validation in the serializer path that handles
key_fragment so malformed values are rejected before link generation, and apply
the same check in the email-mode flow referenced by the serializer methods that
build these links. Use the existing key_fragment field definition and the
related validation/serialization logic in this module to locate where to enforce
the URL-safe base64 contract.
Comment on lines
+238
to
+239
| if request.query_params.get("as") == "json": | ||
| return Response({"url": url}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mark the presigned-url JSON response as non-cacheable.
This branch puts the signed S3 URL in the response body; add Cache-Control: no-store so browsers/proxies do not retain direct-download credentials.
Suggested fix
if request.query_params.get("as") == "json":
- return Response({"url": url})
+ response = Response({"url": url})
+ response["Cache-Control"] = "no-store"
+ return response📝 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
| if request.query_params.get("as") == "json": | |
| return Response({"url": url}) | |
| if request.query_params.get("as") == "json": | |
| response = Response({"url": url}) | |
| response["Cache-Control"] = "no-store" | |
| return response |
🤖 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/backend/core/api/viewsets/download.py` around lines 238 - 239, The
presigned-url JSON branch in the download viewset returns a signed S3 URL in the
response body without preventing caching. Update the response created in the
`request.query_params.get("as") == "json"` path so it includes `Cache-Control:
no-store`, using the existing response handling in the download endpoint to
ensure browsers and proxies do not retain the direct-download credential.
Comment on lines
+137
to
+175
| # E2E params are honoured here only; subsequent add-file calls | ||
| # ignore them — the mode is locked the moment the draft exists. | ||
| draft = models.TransferDraft.objects.create( | ||
| owner=request.user, | ||
| e2e_encrypted=data.get("e2e_encrypted", False), | ||
| encryption_chunk_size=data.get("encryption_chunk_size"), | ||
| ) | ||
| else: | ||
| draft = self._get_locked_draft(draft_id) | ||
|
|
||
| # The draft's E2E mode is locked at creation; reject mismatched | ||
| # follow-up calls instead of letting a plaintext file slip into | ||
| # an encrypted draft (or vice versa). | ||
| if data.get("e2e_encrypted", False) != draft.e2e_encrypted: | ||
| raise drf.exceptions.ValidationError( | ||
| { | ||
| "e2e_encrypted": ( | ||
| "Cannot change encryption mode once a draft has " | ||
| "been started." | ||
| ) | ||
| } | ||
| ) | ||
| # Same lock on chunk size: the recipient's SW computes part | ||
| # boundaries from one constant value across all files in the | ||
| # transfer. A follow-up call that ships a different value | ||
| # would silently de-sync the boundaries. | ||
| if ( | ||
| draft.e2e_encrypted | ||
| and data["encryption_chunk_size"] != draft.encryption_chunk_size | ||
| ): | ||
| raise drf.exceptions.ValidationError( | ||
| { | ||
| "encryption_chunk_size": ( | ||
| "Cannot change chunk size once a draft has " | ||
| "been started." | ||
| ) | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject server-side imports on E2E drafts.
This endpoint can create or reuse an E2E draft and still accept a source_url file, but the import path below runs on the backend and has no client key to encrypt with. That lets a crafted request produce an e2e_encrypted=True draft whose bytes were uploaded server-side in plaintext, and the later complete_upload path will still skip AV because it trusts the draft flag.
Suggested fix
with transaction.atomic():
if draft_id is None:
+ if data.get("source_url") and data.get("e2e_encrypted", False):
+ raise drf.exceptions.ValidationError(
+ {
+ "source_url": (
+ "Drive imports are not supported for "
+ "end-to-end encrypted drafts."
+ )
+ }
+ )
# First drop of the session — open a fresh draft. No cumulative
# guards: count=1 and total_size = this single file's size,
# which the serializer already bounded to the per-file limit
# (and per-file ≤ total by invariant).
# E2E params are honoured here only; subsequent add-file calls
@@
else:
draft = self._get_locked_draft(draft_id)
+ if data.get("source_url") and draft.e2e_encrypted:
+ raise drf.exceptions.ValidationError(
+ {
+ "source_url": (
+ "Drive imports are not supported for "
+ "end-to-end encrypted drafts."
+ )
+ }
+ )📝 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
| # E2E params are honoured here only; subsequent add-file calls | |
| # ignore them — the mode is locked the moment the draft exists. | |
| draft = models.TransferDraft.objects.create( | |
| owner=request.user, | |
| e2e_encrypted=data.get("e2e_encrypted", False), | |
| encryption_chunk_size=data.get("encryption_chunk_size"), | |
| ) | |
| else: | |
| draft = self._get_locked_draft(draft_id) | |
| # The draft's E2E mode is locked at creation; reject mismatched | |
| # follow-up calls instead of letting a plaintext file slip into | |
| # an encrypted draft (or vice versa). | |
| if data.get("e2e_encrypted", False) != draft.e2e_encrypted: | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "e2e_encrypted": ( | |
| "Cannot change encryption mode once a draft has " | |
| "been started." | |
| ) | |
| } | |
| ) | |
| # Same lock on chunk size: the recipient's SW computes part | |
| # boundaries from one constant value across all files in the | |
| # transfer. A follow-up call that ships a different value | |
| # would silently de-sync the boundaries. | |
| if ( | |
| draft.e2e_encrypted | |
| and data["encryption_chunk_size"] != draft.encryption_chunk_size | |
| ): | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "encryption_chunk_size": ( | |
| "Cannot change chunk size once a draft has " | |
| "been started." | |
| ) | |
| } | |
| ) | |
| if data.get("source_url") and data.get("e2e_encrypted", False): | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "source_url": ( | |
| "Drive imports are not supported for " | |
| "end-to-end encrypted drafts." | |
| ) | |
| } | |
| ) | |
| # First drop of the session — open a fresh draft. No cumulative | |
| # guards: count=1 and total_size = this single file's size, | |
| # which the serializer already bounded to the per-file limit | |
| # (and per-file ≤ total by invariant). | |
| # E2E params are honoured here only; subsequent add-file calls | |
| # ignore them — the mode is locked the moment the draft exists. | |
| draft = models.TransferDraft.objects.create( | |
| owner=request.user, | |
| e2e_encrypted=data.get("e2e_encrypted", False), | |
| encryption_chunk_size=data.get("encryption_chunk_size"), | |
| ) | |
| else: | |
| draft = self._get_locked_draft(draft_id) | |
| if data.get("source_url") and draft.e2e_encrypted: | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "source_url": ( | |
| "Drive imports are not supported for " | |
| "end-to-end encrypted drafts." | |
| ) | |
| } | |
| ) | |
| # The draft's E2E mode is locked at creation; reject mismatched | |
| # follow-up calls instead of letting a plaintext file slip into | |
| # an encrypted draft (or vice versa). | |
| if data.get("e2e_encrypted", False) != draft.e2e_encrypted: | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "e2e_encrypted": ( | |
| "Cannot change encryption mode once a draft has " | |
| "been started." | |
| ) | |
| } | |
| ) | |
| # Same lock on chunk size: the recipient's SW computes part | |
| # boundaries from one constant value across all files in the | |
| # transfer. A follow-up call that ships a different value | |
| # would silently de-sync the boundaries. | |
| if ( | |
| draft.e2e_encrypted | |
| and data["encryption_chunk_size"] != draft.encryption_chunk_size | |
| ): | |
| raise drf.exceptions.ValidationError( | |
| { | |
| "encryption_chunk_size": ( | |
| "Cannot change chunk size once a draft has " | |
| "been started." | |
| ) | |
| } | |
| ) |
🤖 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/backend/core/api/viewsets/draft.py` around lines 137 - 175, Reject
server-side imports when the draft is E2E encrypted by updating the draft
handling in the add-file flow around TransferDraft and _get_locked_draft: if
data requests or reuses an e2e_encrypted draft and the upload comes from
source_url/import rather than client-side encrypted bytes, raise a
ValidationError instead of continuing. Keep the existing encryption-mode and
encryption_chunk_size locking checks, and add an explicit guard in the
upload/import path so e2e_encrypted drafts can only accept client-encrypted
uploads, preventing complete_upload from trusting a plaintext backend import.
Comment on lines
+579
to
+588
| # E2E + email: the key fragment travels via Celery kwarg | ||
| # to the send task. Not persisted on Transfer — once emails | ||
| # are sent it lives only in the recipients' inboxes. | ||
| key_fragment = ( | ||
| metadata.get("key_fragment", "") if transfer.e2e_encrypted else "" | ||
| ) | ||
| transaction.on_commit( | ||
| lambda: send_recipient_invitations_task.delay(str(transfer.id)) | ||
| lambda fragment=key_fragment: send_recipient_invitations_task.delay( | ||
| str(transfer.id), key_fragment=fragment | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not enqueue the decryption key in Celery.
Line 586 serializes key_fragment into the task payload. In this stack that means the backend stores the E2E key in the Celery/Redis hop before any email is sent, which materially widens the trust boundary and contradicts the “not persisted” guarantee described around this flow.
🤖 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/backend/core/api/viewsets/draft.py` around lines 579 - 588, The
invitation enqueue path in draft.py is passing the E2E key fragment into Celery
via send_recipient_invitations_task.delay, which puts sensitive key material
into the broker payload. Update the transaction.on_commit callback and
send_recipient_invitations_task invocation so only non-sensitive transfer
context is enqueued, and have the task fetch or derive any needed data from
server-side state instead of receiving key_fragment. Use the existing
key_fragment handling around transfer.e2e_encrypted as the spot to remove the
serialized secret from the task payload.
Comment on lines
+50
to
+91
| useEffect(() => { | ||
| if (!transfer.e2e_encrypted) return; | ||
| const fragment = window.location.hash.replace(/^#/, ""); | ||
| if (!fragment) { | ||
| setE2eState("no-key"); | ||
| return; | ||
| } | ||
| const chunkSize = transfer.encryption_chunk_size; | ||
| if (!chunkSize) { | ||
| setE2eState("error"); | ||
| return; | ||
| } | ||
| // Remove the key from the visible URL: shoulder-surfing, browser | ||
| // history, copy-from-address-bar all stop leaking it. The page keeps | ||
| // the fragment in-memory (passed to the SW below) so downloads still | ||
| // work — the only thing the user loses by stripping is the ability to | ||
| // recover the key by refreshing the tab. | ||
| try { | ||
| window.history.replaceState(null, "", window.location.pathname); | ||
| } catch { | ||
| // replaceState can throw under exotic sandboxing (about:blank parents, | ||
| // some embedded webviews); the visible URL stays as-is, which is a | ||
| // degradation but not a blocker for downloading. | ||
| } | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| const sw = await ensureE2eServiceWorker(); | ||
| if (!sw) { | ||
| if (!cancelled) setE2eState("error"); | ||
| return; | ||
| } | ||
| await registerE2eKey(sw, token, fragment, transfer.files, chunkSize); | ||
| if (!cancelled) setE2eState("ready"); | ||
| } catch { | ||
| if (!cancelled) setE2eState("error"); | ||
| } | ||
| })(); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [transfer.e2e_encrypted, transfer.encryption_chunk_size, transfer.files, token]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the decryption fragment stable after stripping the URL hash.
Line 52 rereads window.location.hash, but Line 68 clears it. If transfer.files gets a new array reference and the effect reruns, the page flips to no-key and disables downloads even though the key was already present.
Suggested fix
+ const e2eFragmentRef = useRef<string | null>(null);
+
useEffect(() => {
if (!transfer.e2e_encrypted) return;
- const fragment = window.location.hash.replace(/^`#/`, "");
+ const fragment =
+ e2eFragmentRef.current ??
+ new URL(initialUrlRef.current || window.location.href).hash.replace(/^`#/`, "");
if (!fragment) {
setE2eState("no-key");
return;
}
+ e2eFragmentRef.current = fragment;🤖 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/frontend/src/features/transfers/components/DownloadView.tsx` around lines
50 - 91, The download effect is re-reading window.location.hash on every rerun,
so after history.replaceState strips the fragment the effect can incorrectly
fall back to no-key. Update DownloadView’s useEffect to capture the fragment
once and keep it stable across reruns, e.g. store it in a ref or state before
clearing the URL, and then reuse that preserved value when calling
registerE2eKey instead of reading location.hash again. Ensure the existing e2e
state transitions in setE2eState still work from the retained fragment.
Comment on lines
+75
to
+85
| export async function encryptChunk( | ||
| key: CryptoKey, | ||
| plaintext: BufferSource, | ||
| ): Promise<Uint8Array> { | ||
| const iv = crypto.getRandomValues(new Uint8Array(IV_BYTES)); | ||
| const ct = new Uint8Array( | ||
| await crypto.subtle.encrypt( | ||
| { name: "AES-GCM", iv: asBufferSource(iv) }, | ||
| key, | ||
| plaintext, | ||
| ), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify every encrypt/decrypt call site can supply the same file/part context.
rg -n -C4 '\b(encryptChunk|decryptChunk)\s*\(' src/frontend/src/features/transfersRepository: suitenumerique/transfers
Length of output: 7508
Bind each ciphertext chunk to its file and part context.
Verification confirms encryptChunk and decryptChunk authenticate only the ciphertext bytes. The search of call sites in src/frontend/src/features/transfers/api/useTransferDraft.ts shows chunks are encrypted without passing additionalData (AAD) to bind the operation to file metadata (file ID, part number). Without this binding, an attacker or storage glitch could swap or replay valid chunks from the same key context without triggering a GCM authentication failure, potentially resulting in undetected data corruption or stream reordering.
Update the API signatures to accept additionalData and construct a deterministic byte sequence at the call site containing the file ID and part number.
Proposed API change
export async function encryptChunk(
key: CryptoKey,
plaintext: BufferSource,
+ additionalData: BufferSource,
): Promise<Uint8Array> {
const iv = crypto.getRandomValues(new Uint8Array(IV_BYTES));
const ct = new Uint8Array(
await crypto.subtle.encrypt(
- { name: "AES-GCM", iv: asBufferSource(iv) },
+ { name: "AES-GCM", iv: asBufferSource(iv), additionalData },
key,
plaintext,
),
);
@@
export async function decryptChunk(
key: CryptoKey,
chunk: Uint8Array,
+ additionalData: BufferSource,
): Promise<Uint8Array> {
- // ...
const plain = await crypto.subtle.decrypt(
- { name: "AES-GCM", iv: asBufferSource(iv) },
+ { name: "AES-GCM", iv: asBufferSource(iv), additionalData },
key,
asBufferSource(chunk),
);🤖 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/frontend/src/features/transfers/upload/e2eCrypto.ts` around lines 75 -
85, encryptChunk and decryptChunk currently authenticate only the ciphertext, so
chunks can be swapped or replayed without detection. Update the e2eCrypto
helpers to accept additionalData and pass it into the AES-GCM encrypt/decrypt
calls, then update the useTransferDraft call sites to build deterministic AAD
from the file ID and part number for each chunk. Keep the API change consistent
across the encryptChunk/decryptChunk symbols so every chunk is bound to its file
and position.
Comment on lines
+116
to
+122
| export function ciphertextSize( | ||
| plaintextSize: number, | ||
| chunkSize: number, | ||
| ): number { | ||
| if (plaintextSize <= 0) return 0; | ||
| const chunks = Math.ceil(plaintextSize / chunkSize); | ||
| return plaintextSize + chunks * CRYPTO_OVERHEAD_PER_CHUNK; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Account for encrypted empty files.
Line 120 returns 0 for plaintextSize === 0, but the uploader still encrypts one empty part, which uploads IV + tag bytes. That under-declares the S3 object size and can fail the backend size check for zero-byte E2E files.
📏 Proposed size fix
export function ciphertextSize(
plaintextSize: number,
chunkSize: number,
): number {
- if (plaintextSize <= 0) return 0;
- const chunks = Math.ceil(plaintextSize / chunkSize);
+ if (plaintextSize < 0) throw new Error("Invalid plaintext size");
+ if (chunkSize <= 0) throw new Error("Invalid chunk size");
+ const chunks = Math.max(1, Math.ceil(plaintextSize / chunkSize));
return plaintextSize + chunks * CRYPTO_OVERHEAD_PER_CHUNK;
}📝 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
| export function ciphertextSize( | |
| plaintextSize: number, | |
| chunkSize: number, | |
| ): number { | |
| if (plaintextSize <= 0) return 0; | |
| const chunks = Math.ceil(plaintextSize / chunkSize); | |
| return plaintextSize + chunks * CRYPTO_OVERHEAD_PER_CHUNK; | |
| export function ciphertextSize( | |
| plaintextSize: number, | |
| chunkSize: number, | |
| ): number { | |
| if (plaintextSize < 0) throw new Error("Invalid plaintext size"); | |
| if (chunkSize <= 0) throw new Error("Invalid chunk size"); | |
| const chunks = Math.max(1, Math.ceil(plaintextSize / chunkSize)); | |
| return plaintextSize + chunks * CRYPTO_OVERHEAD_PER_CHUNK; | |
| } |
🤖 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/frontend/src/features/transfers/upload/e2eCrypto.ts` around lines 116 -
122, The ciphertextSize helper currently returns 0 for empty inputs, but the
upload flow still produces one encrypted empty part via e2eCrypto, so the
predicted size is too small. Update ciphertextSize to treat plaintextSize === 0
as a single encrypted chunk that includes CRYPTO_OVERHEAD_PER_CHUNK, and keep
the existing chunk-based calculation consistent with the upload path used by
e2eCrypto and the uploader’s backend size validation.
Comment on lines
+33
to
+45
| if (!navigator.serviceWorker.controller) { | ||
| await new Promise<void>((resolve) => { | ||
| navigator.serviceWorker.addEventListener( | ||
| "controllerchange", | ||
| () => resolve(), | ||
| { once: true }, | ||
| ); | ||
| // Belt-and-suspenders: if claim has already happened, the event | ||
| // won't fire — re-check after a short tick. | ||
| setTimeout(() => { | ||
| if (navigator.serviceWorker.controller) resolve(); | ||
| }, 100); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the service-worker waits so setup can fail visibly.
Both waits can hang forever: Line 42 only resolves if a controller exists after 100ms, and Line 75 waits for an ack with no timeout/error path. DownloadView catches failures, but these promises never reject, so users stay stuck with disabled download buttons.
Suggested fix
+const SERVICE_WORKER_READY_TIMEOUT_MS = 3000;
+const REGISTER_ACK_TIMEOUT_MS = 5000;
+
export async function ensureE2eServiceWorker(): Promise<ServiceWorker | null> {
@@
if (!navigator.serviceWorker.controller) {
- await new Promise<void>((resolve) => {
+ const controlled = await new Promise<boolean>((resolve) => {
navigator.serviceWorker.addEventListener(
"controllerchange",
- () => resolve(),
+ () => resolve(true),
{ once: true },
);
- // Belt-and-suspenders: if claim has already happened, the event
- // won't fire — re-check after a short tick.
setTimeout(() => {
- if (navigator.serviceWorker.controller) resolve();
- }, 100);
+ resolve(Boolean(navigator.serviceWorker.controller));
+ }, SERVICE_WORKER_READY_TIMEOUT_MS);
});
+ if (!controlled) return null;
}
@@
- await new Promise<void>((resolve) => {
+ await new Promise<void>((resolve, reject) => {
+ const timeout = window.setTimeout(() => {
+ navigator.serviceWorker.removeEventListener("message", listener);
+ reject(new Error("Timed out waiting for E2E service worker registration ack."));
+ }, REGISTER_ACK_TIMEOUT_MS);
const listener = (event: MessageEvent) => {
if (
event.data &&
event.data.type === "e2e-register-ack" &&
event.data.token === token
) {
navigator.serviceWorker.removeEventListener("message", listener);
+ window.clearTimeout(timeout);
resolve();
}
};Also applies to: 75-94
🤖 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/frontend/src/features/transfers/upload/e2eServiceWorker.ts` around lines
33 - 45, The controller wait and the service-worker ack wait in e2eServiceWorker
can hang forever, so bound both with explicit timeouts and rejection paths.
Update the controllerchange/setup promise and the post-message ack flow to
reject after a reasonable timeout instead of only resolving, and make sure the
exported setup/handshake logic surfaces those failures so DownloadView can
recover. Use the existing e2eServiceWorker async flow and any helper around
controllerchange / ack handling as the place to add the timeout and error
propagation.
Nastaliss
force-pushed
the
feat/e2e-encryption
branch
from
b817fec to
0ab0111
Compare
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation