fix(security): harden auth and sharing subsystems#267
Merged
Conversation
Address 4 findings from security review: - L1: /shares/lookup always returns 200 (eliminates pubkey enumeration) - L4: Add pagination (limit/offset) to share listing endpoints - M5: Surface notification toast when reWrapForRecipients fails - H-08: Encrypt device private key at rest in IndexedDB (HKDF + AES-256-GCM) Previously fixed: M-07 (scoped JWT), M-11 (Rust Debug redaction). Known limitation: M1 (plaintext itemName in share record) documented. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 68f538dc75d7
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds paginated shares endpoints and DTOs, updates server and client signatures to return { shares, total }, introduces a LookupUserResponse DTO, adds a frontend notification store and toast, persists device private keys encrypted with a vault-derived wrapping key, and updates callsites/tests to use pagination and notifications. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as "Shares Controller"
participant Service as "Shares Service"
participant Repo as "Repository / DB"
Client->>Controller: GET /shares/received?limit=50&offset=0
Controller->>Service: getReceivedShares(userId, 50, 0)
Service->>Repo: findAndCount(where, skip=0, take=50)
Repo-->>Service: [shares[], total]
Service-->>Controller: { shares: Share[], total: number }
Controller-->>Client: 200 PaginatedReceivedSharesDto
sequenceDiagram
participant Service as "Share Service"
participant Store as "Notification Store"
participant Toast as "NotificationToast"
participant User as "User"
Service->>Service: reWrapForRecipients()
alt some recipients failed
Service->>Store: addNotification('warning', "X recipients failed")
Store-->>Toast: notifications updated
Toast->>User: render toast (auto-dismiss 8s)
User->>Toast: click dismiss or timeout
Toast->>Store: dismissNotification(id)
else all succeeded
Service-->>Store: (no notification)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements four hardening improvements to the auth and sharing subsystems as part of a security review:
- L1 fix:
/shares/lookupnow always returns HTTP 200 with{ exists: boolean }instead of 200/404, eliminating public key enumeration via response code differences. - L4 fix: Adds
limit/offsetpagination to/shares/receivedand/shares/sent, with validated defaults (limit=50, max=100). - M5 fix: Adds a Zustand-backed
NotificationToastcomponent that surfaces user-visible warnings whenreWrapForRecipientsfails. - H-08 fix: Device Ed25519 private key is now encrypted with AES-256-GCM (HKDF-derived from vault private key) before being stored in IndexedDB, with a migration path that regenerates the keypair for v1 (plaintext) entries.
Changes:
- API + OpenAPI spec updated for paginated shares endpoints and new lookup/pagination DTOs
- New Zustand notification store +
NotificationToastcomponent - Device identity module now encrypts the private key at rest;
getOrCreateDeviceIdentitynow accepts optionalvaultPrivateKey - All callers of
fetchReceivedShares,fetchSentShares, andreWrapForRecipientsupdated to the new signatures
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/api-client/openapi.json |
Adds pagination params for received/sent shares, removes 404 from lookup, adds new DTO schemas |
apps/api/src/shares/shares.controller.ts |
Wires pagination params; changes lookupUser to always return 200+boolean |
apps/api/src/shares/shares.service.ts |
getReceivedShares/getSentShares use findAndCount with take/skip |
apps/api/src/shares/dto/pagination.dto.ts |
New PaginationQueryDto (limit 1–100, offset ≥0) + paginated response DTOs |
apps/api/src/shares/dto/lookup-user-response.dto.ts |
New LookupUserResponseDto |
apps/api/src/shares/shares.controller.spec.ts |
Tests updated for paginated response shape and new lookup behavior |
apps/api/src/shares/shares.service.spec.ts |
Tests updated for findAndCount and paginated results |
apps/web/src/stores/notification.store.ts |
New Zustand notification store |
apps/web/src/components/NotificationToast.tsx |
New toast component with auto-dismiss |
apps/web/src/components/layout/AppShell.tsx |
Mounts NotificationToast in both layout branches |
apps/web/src/lib/clear-user-stores.ts |
Clears notification store on logout |
apps/web/src/services/share.service.ts |
Pagination in fetch functions; reWrapForRecipients returns failedRecipients; ensureFreshSentShares uses limit=1000 |
apps/web/src/lib/device/identity.ts |
HKDF-derived wrapping key; v1→v2 migration; vaultPrivateKey param added |
apps/web/src/hooks/useAuth.ts |
Passes vaultPrivateKey to getOrCreateDeviceIdentity |
apps/web/src/hooks/useDeviceApproval.ts |
Passes vaultPrivateKey to getOrCreateDeviceIdentity in approve/deny handlers |
apps/web/src/hooks/useFolderMutations.ts |
Shows warning toast on re-wrap failure |
apps/web/src/hooks/useFileOperations.ts |
Shows warning toast on re-wrap failure |
apps/web/src/hooks/useSharedNavigation.ts |
Destructures { shares } from paginated response |
apps/web/src/components/file-browser/ShareDialog.tsx |
Uses limit: 100 for fetching sent shares |
apps/web/src/api/shares/shares.ts |
Updated Orval-generated client for paginated + lookup changes |
apps/web/src/api/models/*.ts |
New generated model files |
.planning/todos/done/*.md |
Todo items moved to done |
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/file-browser/ShareDialog.tsx (1)
195-205:⚠️ Potential issue | 🟠 MajorLookup flow still expects 404, but API now returns
{ exists: boolean }with 200.This branch won’t show
user not foundreliably anymore and defers failure tocreateShare.Suggested fix (check `exists` explicitly)
- try { - // Verify recipient is a registered user - await sharesControllerLookupUser({ publicKey: key }); - } catch (err: unknown) { - const status = (err as { status?: number }).status; - if (status === 404) { - setError('user not found'); - } else { - setError('lookup failed, please try again'); - } - setIsSharing(false); - return; - } + try { + // Verify recipient is a registered user + const lookup = await sharesControllerLookupUser({ publicKey: key }); + if (!lookup.exists) { + setError('user not found'); + setIsSharing(false); + return; + } + } catch { + setError('lookup failed, please try again'); + setIsSharing(false); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/file-browser/ShareDialog.tsx` around lines 195 - 205, The lookup no longer returns 404 but a 200 payload with { exists: boolean }, so change the flow in ShareDialog to inspect the lookup response instead of relying on an error status: call sharesControllerLookupUser({ publicKey: key }) and if the returned object has exists === false call setError('user not found'), setIsSharing(false) and return; only proceed to createShare when exists === true; remove the status-based 404 check in the catch block (keep a generic error handler to setError('lookup failed, please try again') and setIsSharing(false) for unexpected exceptions).
🧹 Nitpick comments (2)
apps/web/src/components/NotificationToast.tsx (1)
6-16: Consider consolidating duplicate color maps.
borderColorsandlabelColorsare identical. You could use a single shared map to reduce duplication.♻️ Suggested consolidation
-const borderColors: Record<Notification['type'], string> = { - info: 'var(--color-green-primary)', - warning: 'var(--color-warning)', - error: 'var(--color-error)', -}; - -const labelColors: Record<Notification['type'], string> = { +const typeColors: Record<Notification['type'], string> = { info: 'var(--color-green-primary)', warning: 'var(--color-warning)', error: 'var(--color-error)', };Then use
typeColors[n.type]for both border and label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/NotificationToast.tsx` around lines 6 - 16, borderColors and labelColors in NotificationToast.tsx are identical; replace them with a single shared map (e.g., typeColors) keyed by Notification['type'] and use that single map wherever both borderColors and labelColors are referenced (update any usages like borderColors[n.type] and labelColors[n.type] to typeColors[n.type]); ensure the new map preserves the same keys and values and update imports/exports if either map was exported.apps/api/src/shares/shares.controller.spec.ts (1)
143-157: Add a pagination pass-through test forgetSentShares(parity with received).Received path validates
(userId, limit, offset)forwarding; sent path should assert the same contract.Suggested test addition
describe('getSentShares', () => { const pagination = { limit: 50, offset: 0 }; it('should return paginated shares with recipientPublicKey', async () => { mockSharesService.getSentShares.mockResolvedValue({ shares: [mockShare], total: 1 }); const result = await controller.getSentShares(mockReq, pagination); expect(result.shares).toHaveLength(1); expect(result.total).toBe(1); expect(result.shares[0].shareId).toBe(shareId); expect(result.shares[0].recipientPublicKey).toBe(recipientPublicKey); expect(result.shares[0].itemType).toBe('folder'); expect(result.shares[0].itemName).toBe('My Folder'); }); + + it('should pass pagination params to service', async () => { + mockSharesService.getSentShares.mockResolvedValue({ shares: [], total: 0 }); + await controller.getSentShares(mockReq, { limit: 10, offset: 20 }); + expect(mockSharesService.getSentShares).toHaveBeenCalledWith(userId, 10, 20); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 143 - 157, Add a test asserting that controller.getSentShares forwards pagination and userId to the shares service: call controller.getSentShares(mockReq, pagination) and verify mockSharesService.getSentShares was called with mockReq.user.userId (or equivalent userId), pagination.limit and pagination.offset; keep existing assertions on returned shares (shares/total) and use the same symbols getSentShares, controller, mockSharesService.getSentShares, mockReq, and pagination to locate where to add this parity test with the received path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 124-129: The current call to sharesControllerGetSentShares({
limit: 100 }) only fetches a single page and can miss recipients; change the
logic in the ShareDialog code that uses sharesControllerGetSentShares (and the
SentShare result handling that filters by ipnsName) to paginate until no more
pages are returned: repeatedly call sharesControllerGetSentShares using the
API's pagination mechanism (e.g., nextCursor/offset/page token or incrementing
offset with limit) accumulating response.shares, break when the response
indicates no more data (empty page or missing next token), respect the cancelled
flag on each loop iteration, then perform the .filter((s) => s.ipnsName ===
ipnsName) on the full accumulated list so revoke/manage UI sees all recipients.
In `@apps/web/src/hooks/useDeviceApproval.ts`:
- Around line 366-367: The code currently passes vaultKeypair?.privateKey into
getOrCreateDeviceIdentity which allows creating a new ephemeral device identity
when vault state is missing; instead, check for a present vaultKeypair and its
privateKey before calling getOrCreateDeviceIdentity and fail fast (throw or
return an error) if absent so approve/deny actions cannot silently create
ephemeral identities. Update both usages (the one referencing vaultKeypair and
getOrCreateDeviceIdentity and the other occurrence at the later call) to
validate useAuthStore.getState().vaultKeypair and its privateKey, and only call
getOrCreateDeviceIdentity when a real privateKey exists; otherwise surface a
clear error/early return so respondedByDeviceId attribution remains stable.
In `@apps/web/src/hooks/useSharedNavigation.ts`:
- Line 122: The current call in useSharedNavigation (const { shares } = await
fetchReceivedShares()) only loads the first page (default 50); change the logic
to page through fetchReceivedShares until no more pages and aggregate all share
items before updating state: call fetchReceivedShares repeatedly (using its
pagination token/parameters) or adjust it to accept page params, accumulate
results into a single shares array, then set state with the combined array;
target the useSharedNavigation function and the fetchReceivedShares
call/response handling where shares is destructured.
In `@apps/web/src/lib/device/identity.ts`:
- Around line 89-114: The IndexedDB payload fetched for KEYPAIR_KEY is being
cast without validation and can feed malformed data into
deriveDeviceWrappingKey/crypto.subtle.decrypt; before calling
deriveDeviceWrappingKey or decrypt, explicitly validate that val is an object,
val.version === STORAGE_VERSION, and that val.publicKey, val.iv, and
val.encryptedPrivateKey are arrays (or typed arrays) of expected numeric bytes
(and non-empty/expected length), and if any check fails return null (or reject)
so decryption never runs on bad input; also guard the Uint8Array conversions so
they only run after these checks.
In `@apps/web/src/services/share.service.ts`:
- Around line 189-191: The call that uses fetchSentShares(1000) violates the
backend max page size and can break rotation; replace the single oversized fetch
with a safe pagination loop: repeatedly call fetchSentShares with the backend
max (100) and the appropriate paging parameter (offset, pageToken, or page
number as supported by fetchSentShares) until no more shares are returned,
aggregate all pages into one array, then call
useShareStore.getState().setSentShares(allShares); ensure you do not pass 1000
anywhere and handle empty/partial pages and errors from fetchSentShares during
the loop.
In `@packages/api-client/openapi.json`:
- Around line 1210-1230: The OpenAPI schemas for pagination and count fields
(parameters named "limit" and "offset" and any response/count properties
documented as type "number") must be changed to integer types to match backend
validation; update each schema where "type": "number" is used for
pagination/counts to "type": "integer" (optionally add "format": "int32" and
keep existing "default" values) for the "limit" and "offset" query parameters
and for any corresponding response/count properties in the same file so
generated clients no longer send decimals.
- Around line 1321-1334: The OpenAPI spec for the /shares/lookup operation is
missing a 400 response for invalid publicKey input; update
packages/api-client/openapi.json under the /shares/lookup operation (the
responses object that currently lists 200 and 401) to add a 400 response
describing "Bad Request" (invalid publicKey) and reference an appropriate error
schema (or create one) consistent with the controller behavior in
apps/api/src/shares/shares.controller.ts where BadRequestException is thrown;
ensure the new response entry is named "400" with a brief description and a
content/application/json schema that matches the API's error DTO so generated
clients will handle the 400 case.
---
Outside diff comments:
In `@apps/web/src/components/file-browser/ShareDialog.tsx`:
- Around line 195-205: The lookup no longer returns 404 but a 200 payload with {
exists: boolean }, so change the flow in ShareDialog to inspect the lookup
response instead of relying on an error status: call
sharesControllerLookupUser({ publicKey: key }) and if the returned object has
exists === false call setError('user not found'), setIsSharing(false) and
return; only proceed to createShare when exists === true; remove the
status-based 404 check in the catch block (keep a generic error handler to
setError('lookup failed, please try again') and setIsSharing(false) for
unexpected exceptions).
---
Nitpick comments:
In `@apps/api/src/shares/shares.controller.spec.ts`:
- Around line 143-157: Add a test asserting that controller.getSentShares
forwards pagination and userId to the shares service: call
controller.getSentShares(mockReq, pagination) and verify
mockSharesService.getSentShares was called with mockReq.user.userId (or
equivalent userId), pagination.limit and pagination.offset; keep existing
assertions on returned shares (shares/total) and use the same symbols
getSentShares, controller, mockSharesService.getSentShares, mockReq, and
pagination to locate where to add this parity test with the received path.
In `@apps/web/src/components/NotificationToast.tsx`:
- Around line 6-16: borderColors and labelColors in NotificationToast.tsx are
identical; replace them with a single shared map (e.g., typeColors) keyed by
Notification['type'] and use that single map wherever both borderColors and
labelColors are referenced (update any usages like borderColors[n.type] and
labelColors[n.type] to typeColors[n.type]); ensure the new map preserves the
same keys and values and update imports/exports if either map was exported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5b4c376-8b82-473b-863b-8b2a141aa417
📒 Files selected for processing (29)
.planning/todos/done/2026-02-15-security-review-medium-term-fixes.md.planning/todos/done/2026-02-21-phase14-security-review-deferred.mdapps/api/src/shares/dto/lookup-user-response.dto.tsapps/api/src/shares/dto/pagination.dto.tsapps/api/src/shares/shares.controller.spec.tsapps/api/src/shares/shares.controller.tsapps/api/src/shares/shares.service.spec.tsapps/api/src/shares/shares.service.tsapps/web/src/api/custom-instance.tsapps/web/src/api/models/index.tsapps/web/src/api/models/lookupUserResponseDto.tsapps/web/src/api/models/paginatedReceivedSharesDto.tsapps/web/src/api/models/paginatedSentSharesDto.tsapps/web/src/api/models/sharesControllerGetReceivedSharesParams.tsapps/web/src/api/models/sharesControllerGetSentSharesParams.tsapps/web/src/api/shares/shares.tsapps/web/src/components/NotificationToast.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/components/layout/AppShell.tsxapps/web/src/hooks/useAuth.tsapps/web/src/hooks/useDeviceApproval.tsapps/web/src/hooks/useFileOperations.tsapps/web/src/hooks/useFolderMutations.tsapps/web/src/hooks/useSharedNavigation.tsapps/web/src/lib/clear-user-stores.tsapps/web/src/lib/device/identity.tsapps/web/src/services/share.service.tsapps/web/src/stores/notification.store.tspackages/api-client/openapi.json
- Paginate all share fetches (API max 100/page) instead of limit=1000 - Use role="alert" + aria-live="assertive" for warning/error toasts - Consolidate duplicate borderColors/labelColors into single typeColors map - Fail fast in approver/deny when vault keypair unavailable - Validate IndexedDB payload shape before decrypting device key - Add 400 response to /shares/lookup OpenAPI spec - Use integer types for pagination params and total counts in OpenAPI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 29de1d88b4dc
…tion test - ShareDialog lookup now checks `lookup.exists` instead of catching 404 (API always returns 200 after L1 fix) - Add pagination pass-through test for getSentShares controller Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e24bba172048
Owner
Author
|
Addressing remaining review items from CodeRabbit review: Outside-diff-range comment (ShareDialog.tsx:195-205 — lookup still expects 404): Nitpick (controller.spec.ts — missing getSentShares pagination test): |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/services/share.service.ts (1)
292-330:⚠️ Potential issue | 🟡 MinorDeduplicate failed recipients before returning.
failedRecipientsis currently appended per failed share. If one recipient is covered by multiple shares, UI warnings can over-count affected recipients.💡 Proposed fix
- const failedRecipients: string[] = []; + const failedRecipients = new Set<string>(); ... - failedRecipients.push(share.recipientPublicKey); + failedRecipients.add(share.recipientPublicKey); ... - return { failedRecipients }; + return { failedRecipients: Array.from(failedRecipients) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/share.service.ts` around lines 292 - 330, failedRecipients may contain duplicates if the same recipient appears in multiple coveringShares; update the logic in the re-wrap loop (the block using coveringShares, wrapKey, addShareKeys) to deduplicate before returning by using a Set (or filtering unique values) when collecting push operations or just convert failedRecipients to a unique array before the return; ensure you still push share.recipientPublicKey on catch but return Array.from(new Set(failedRecipients)) (or equivalent) so the function returns unique recipient values.
🧹 Nitpick comments (1)
apps/web/src/components/NotificationToast.tsx (1)
35-41: Optional: avoid repeated linear scans when pruning timers.You can reduce cleanup from repeated
some(...)checks by precomputing active IDs once per effect run.♻️ Proposed refactor
- // Clean up timers for removed notifications - for (const [id, timer] of timers) { - if (!notifications.some((n) => n.id === id)) { - clearTimeout(timer); - timers.delete(id); - } - } + // Clean up timers for removed notifications + const activeIds = new Set(notifications.map((n) => n.id)); + for (const [id, timer] of timers) { + if (!activeIds.has(id)) { + clearTimeout(timer); + timers.delete(id); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/NotificationToast.tsx` around lines 35 - 41, Precompute the set of active notification IDs once per effect and use it to prune timers instead of calling notifications.some(...) for each timer; specifically, build a Set (from notifications.map(n => n.id)) and then iterate timers with for (const [id, timer] of timers) { if (!activeIds.has(id)) { clearTimeout(timer); timers.delete(id); } }, referencing the existing timers, notifications, id, timer, clearTimeout and timers.delete symbols to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/lib/device/identity.ts`:
- Around line 213-218: The call to saveDeviceKeypair is awaited inside
getOrCreateDeviceIdentity which causes the function to throw if
IndexedDB/encryption fails; wrap the saveDeviceKeypair(...) call in a try/catch
so write/encryption errors are caught and do not abort identity creation, and
ensure the function still returns the newly generated keypair from
generateDeviceKeypair when persistence fails (log or processLogger.warn the
caught error). Locate generateDeviceKeypair, saveDeviceKeypair and
getOrCreateDeviceIdentity to implement a non-fatal fallback: attempt to persist,
catch and log the error, then continue returning the generated keypair.
---
Outside diff comments:
In `@apps/web/src/services/share.service.ts`:
- Around line 292-330: failedRecipients may contain duplicates if the same
recipient appears in multiple coveringShares; update the logic in the re-wrap
loop (the block using coveringShares, wrapKey, addShareKeys) to deduplicate
before returning by using a Set (or filtering unique values) when collecting
push operations or just convert failedRecipients to a unique array before the
return; ensure you still push share.recipientPublicKey on catch but return
Array.from(new Set(failedRecipients)) (or equivalent) so the function returns
unique recipient values.
---
Nitpick comments:
In `@apps/web/src/components/NotificationToast.tsx`:
- Around line 35-41: Precompute the set of active notification IDs once per
effect and use it to prune timers instead of calling notifications.some(...) for
each timer; specifically, build a Set (from notifications.map(n => n.id)) and
then iterate timers with for (const [id, timer] of timers) { if
(!activeIds.has(id)) { clearTimeout(timer); timers.delete(id); } }, referencing
the existing timers, notifications, id, timer, clearTimeout and timers.delete
symbols to locate where to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c3fba07-24f1-4a03-9892-ad6f35e2b06f
📒 Files selected for processing (13)
apps/api/src/shares/dto/pagination.dto.tsapps/api/src/shares/shares.controller.tsapps/web/src/api/models/paginatedReceivedSharesDto.tsapps/web/src/api/models/paginatedSentSharesDto.tsapps/web/src/api/models/sharesControllerGetReceivedSharesParams.tsapps/web/src/api/models/sharesControllerGetSentSharesParams.tsapps/web/src/components/NotificationToast.tsxapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/hooks/useDeviceApproval.tsapps/web/src/hooks/useSharedNavigation.tsapps/web/src/lib/device/identity.tsapps/web/src/services/share.service.tspackages/api-client/openapi.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/src/api/models/paginatedSentSharesDto.ts
- apps/web/src/hooks/useSharedNavigation.ts
- apps/api/src/shares/dto/pagination.dto.ts
- apps/web/src/api/models/sharesControllerGetReceivedSharesParams.ts
- apps/web/src/api/models/sharesControllerGetSentSharesParams.ts
…ation Wrap saveDeviceKeypair in try/catch so identity creation succeeds even if IndexedDB persistence fails (e.g. incognito mode). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b19f546ec5be
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
Prevents unnecessary API calls after component unmount during multi-page share fetching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 6ab41206d086
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/shares/shares.controller.spec.ts (1)
144-163: Optional: add an explicit empty-result test for sent shares.Lines 146-163 cover happy-path pagination and parameter forwarding. For parity with
getReceivedShares, consider asserting{ shares: [], total: 0 }forgetSentSharestoo.Proposed test addition
describe('getSentShares', () => { const pagination = { limit: 50, offset: 0 }; @@ it('should pass pagination params to service', async () => { mockSharesService.getSentShares.mockResolvedValue({ shares: [], total: 0 }); await controller.getSentShares(mockReq, { limit: 10, offset: 20 }); expect(mockSharesService.getSentShares).toHaveBeenCalledWith(userId, 10, 20); }); + + it('should return empty array when no sent shares', async () => { + mockSharesService.getSentShares.mockResolvedValue({ shares: [], total: 0 }); + const result = await controller.getSentShares(mockReq, pagination); + expect(result.shares).toEqual([]); + expect(result.total).toBe(0); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/shares/shares.controller.spec.ts` around lines 144 - 163, Add a new unit test that asserts getSentShares returns an explicit empty result ({ shares: [], total: 0 }) and that controller.getSentShares forwards pagination parameters to mockSharesService.getSentShares; specifically, call controller.getSentShares(mockReq, { limit: 10, offset: 20 }) after mocking mockSharesService.getSentShares.mockResolvedValue({ shares: [], total: 0 }) and assert the returned value or that mockSharesService.getSentShares was called with (userId, 10, 20) to mirror the getReceivedShares empty-result parity and ensure parameter forwarding from controller.getSentShares.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/lib/device/identity.ts`:
- Around line 193-200: getOrCreateDeviceIdentity currently silently returns an
ephemeral keypair when vaultPrivateKey is omitted; change its API to a
discriminated-union input (e.g., { vaultPrivateKey: Uint8Array } | { ephemeral:
true }) so callers must opt in to ephemeral behavior, update the function body
to switch on the discriminant and preserve existing behavior for the vault path,
and update call sites that intentionally want ephemeral keys (useMfa.ts at
useMfa and useDeviceApproval.ts at useDeviceApproval) to pass the explicit {
ephemeral: true } variant while other callers pass { vaultPrivateKey } so
accidental omission no longer rotates deviceId; also update the JSDoc above
getOrCreateDeviceIdentity to describe the new union parameters.
---
Nitpick comments:
In `@apps/api/src/shares/shares.controller.spec.ts`:
- Around line 144-163: Add a new unit test that asserts getSentShares returns an
explicit empty result ({ shares: [], total: 0 }) and that
controller.getSentShares forwards pagination parameters to
mockSharesService.getSentShares; specifically, call
controller.getSentShares(mockReq, { limit: 10, offset: 20 }) after mocking
mockSharesService.getSentShares.mockResolvedValue({ shares: [], total: 0 }) and
assert the returned value or that mockSharesService.getSentShares was called
with (userId, 10, 20) to mirror the getReceivedShares empty-result parity and
ensure parameter forwarding from controller.getSentShares.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cf44b8d-ff86-4b30-82fa-25d224d5864a
📒 Files selected for processing (4)
apps/api/src/shares/shares.controller.spec.tsapps/web/src/components/file-browser/ShareDialog.tsxapps/web/src/hooks/useSharedNavigation.tsapps/web/src/lib/device/identity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/useSharedNavigation.ts
Codecov needs coverage data on the base branch (main) to compute PR diffs and post coverage comments. The CI workflow only triggered on pull_request, so no baseline was ever uploaded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e9f7e2c1c467
Codecov needs coverage data on main to compute PR diffs. Rather than re-running the full CI on push to main, cache the coverage artifact from the PR CI run and re-upload it in a lightweight job. - ci.yml: revert push trigger, add coverage artifact upload step - codecov-base.yml: new workflow on push to main that downloads the cached artifact and uploads to codecov Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d109f435fc30
Replace optional `vaultPrivateKey?` parameter with a discriminated union
type (`{ mode: 'persisted', vaultPrivateKey }` | `{ mode: 'ephemeral' }`)
so callers must explicitly choose ephemeral mode. This prevents accidental
key rotation from forgetting to pass the vault key.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 3041e592ee03
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
+ Coverage 46.31% 48.18% +1.87%
==========================================
Files 106 110 +4
Lines 8266 8997 +731
Branches 591 642 +51
==========================================
+ Hits 3828 4335 +507
- Misses 4271 4490 +219
- Partials 167 172 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…d edge cases - Error propagation: NotFoundException, ForbiddenException, ConflictException for createShare, getShareKeys, addShareKeys, revokeShare, hideShare, updateShareEncryptedKey, and completeRotation - Response shape: verify internal fields (sharerId, recipientId, etc.) not leaked - Edge cases: empty results, multiple items, null/undefined publicKey, wrong prefix, too-long keys, case-insensitive hex Total shares controller tests: 18 → 46 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 9f51bf05c2e5
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codecov-base.yml:
- Around line 53-54: The base Codecov upload currently only includes the API and
crypto reports/flags (the files line referencing apps/api/coverage/lcov.info and
packages/crypto/coverage/lcov.info and the flags line noting "api,crypto");
update the upload step to also include the desktop coverage file (e.g.,
packages/desktop/coverage/lcov.info) and add the "desktop" flag to the flags
list so the base-branch upload contains api, crypto, and desktop to match
project.default.flags in codecov.yml.
- Around line 12-22: The workflow job "upload-coverage" is missing an explicit
Actions API permission; add a permissions block for that job with "actions:
read" so the gh api calls for listing workflow runs and downloading artifacts
run deterministically; update the "upload-coverage" job (job name
upload-coverage) to include a permissions: actions: read entry scoped to the
job.
In `@apps/web/src/lib/device/identity.ts`:
- Around line 227-236: The saveDeviceKeypair failure currently swallows errors
and leaves no session-stable identity; modify the catch block after
generateDeviceKeypair/saveDeviceKeypair to store the generated keypair in a
module-scoped session fallback (e.g., sessionFallbackKeypair) so subsequent
persisted-mode calls in the same session reuse it; ensure functions that load or
create identity (where generateDeviceKeypair and saveDeviceKeypair are used)
first check and return sessionFallbackKeypair when IndexedDB persistence failed,
and set sessionFallbackKeypair = { publicKey: keypair.publicKey, privateKey:
keypair.privateKey } inside the catch so the identity remains stable for the
session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ed4c0d2-b685-4c8d-83a5-88f3115e00df
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/codecov-base.ymlapps/api/src/shares/shares.controller.spec.tsapps/web/src/hooks/useAuth.tsapps/web/src/hooks/useDeviceApproval.tsapps/web/src/hooks/useMfa.tsapps/web/src/lib/device/identity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/hooks/useAuth.ts
…entity fallback - Add explicit `permissions: actions: read` to codecov-base.yml for deterministic behavior when calling Actions API endpoints - Add module-scoped session fallback in device identity so repeated persisted-mode calls return the same keypair when IndexedDB is unavailable, preventing deviceId churn within a session Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 94fe8778a376
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/lib/device/identity.ts`:
- Around line 180-181: The module-scoped variable sessionFallback is leaking a
device key across vaults; change the implementation to scope fallback state to
the active vault/context (e.g., replace sessionFallback with a Map keyed by
activeVaultId or move it into the React context/Zustand store) and update any
callers (e.g., the loadDeviceIdentity/null-load path) to: 1) look up the
fallback by current vault id, 2) validate the fallback belongs to the current
persisted request/account before returning, and 3) ensure the fallback is fully
cleared on logout or vault-switch to prevent cross-account reuse.
- Around line 223-253: Concurrent callers can race in the persisted create path
(generateDeviceKeypair, saveDeviceKeypair, loadDeviceKeypair) causing multiple
different keypairs to be created; serialize this with a shared in-process
lock/promise so only one creator performs generate+save and others wait and then
return the saved canonical identity. Implement a module-scoped variable (e.g.,
creatingDeviceKeypairPromise) that is set when entering the creation branch;
other callers await it; inside the promise perform
loadDeviceKeypair(vaultPrivateKey) again (to short-circuit if another writer
completed), otherwise generateDeviceKeypair(), attempt saveDeviceKeypair(...,
vaultPrivateKey) and set sessionFallback only on save failure, then resolve with
the final canonical identity (use deriveDeviceId on the returned publicKey);
always clear the creatingDeviceKeypairPromise in finally so future calls
proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eff4e217-3fc2-41ac-89b8-99b74f11fc77
📒 Files selected for processing (2)
.github/workflows/codecov-base.ymlapps/web/src/lib/device/identity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/codecov-base.yml
…urrent calls - Scope sessionFallback to vault key hash so a different user in the same tab cannot receive a stale device identity from a previous login - Deduplicate concurrent persisted-mode calls via shared in-flight promise so only one keypair is generated and saved to IndexedDB - Extract resolvePersistedIdentity() for clean separation of concerns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: c5e71c9330a8
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 32 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
…validation - Scope persistedInFlight promise to vault key hash so concurrent calls with different vault keys don't return the wrong identity - Tighten encrypted private key validation from length > 0 to length >= 48 (32-byte Ed25519 seed + 16-byte AES-GCM auth tag) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: be85ed171786
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 32 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
apps/web/src/hooks/useSharedNavigation.ts:143
- After the
whileloop exits (all pages collected), the calls touseShareStore.getState().setReceivedShares(shares)(line 135) andsetSharedItems(items)(line 143) are not guarded by acancelledcheck. If the component unmounts after the last page fetch but before these lines execute,setSharedItemswill attempt to update state on an unmounted component. Aif (cancelled) return;guard should be placed immediately after the while loop (before line 135) to match the pattern used inside the loop.
useShareStore.getState().setReceivedShares(shares);
const items: SharedListItem[] = shares.map((share) => ({
share,
children: null,
folderKey: null,
}));
setSharedItems(items);
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This was referenced Mar 21, 2026
Closed
This was referenced Mar 24, 2026
Merged
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
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
/shares/lookupalways returns 200 — eliminates public key enumeration via 200/404 differencelimit/offset) to/shares/receivedand/shares/sentendpointsreWrapForRecipientsfails (new Zustand store + component)Previously fixed (not in this PR): M-07 (scoped JWT), M-11 (Rust Debug redaction).
Known limitation: M1 (plaintext
itemNamein share record) documented as acceptable.Test plan
tsc --noEmit)pnpm api:generatetests/e2e/tests/share-workflow.spec.ts(if available)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements