fix(security): bind copilot chat attachment keys to their owner - #6179
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
Reviewed by Cursor Bugbot for commit 175d608. Configure here. |
Greptile SummaryThis PR secures copilot chat attachments by requiring workspace write access and binding storage keys only when they belong to the requesting user and workspace.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the current atomic predicates fix both previously reported ownership races and the permission gate prevents read-only members from reaching attachment metadata writes.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Adds key validation, ownership resolution, storage existence checks, and atomic compare-and-swap predicates that fix the previously reported materialization and cross-chat races. |
| apps/sim/lib/copilot/chat/payload.ts | Restricts attachment tracking to callers whose effective workspace permission satisfies the write requirement. |
| apps/sim/app/api/mothership/local-files/stage/route.ts | Converts storage-key ownership rejection into a deliberate 403 client response. |
| apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts | Adds regression coverage for foreign ownership, archived rows, storage existence, atomic ownership checks, and chat-binding races. |
| apps/sim/lib/copilot/chat/payload.test.ts | Verifies that read or absent permissions cannot track attachments while write and admin permissions can. |
Sequence Diagram
sequenceDiagram
participant Client
participant Chat as Copilot payload/staging
participant Tracker as trackChatUpload
participant DB as workspace_files
participant Storage
Client->>Chat: Submit attachment key
Chat->>Chat: Require write/admin permission
Chat->>Tracker: Bind key to user, workspace, and chat
Tracker->>Tracker: Validate workspace key prefix
Tracker->>DB: Resolve all records for key
alt Existing owned active chat upload
Tracker->>DB: Atomic conditional UPDATE
DB-->>Tracker: Updated row or zero rows
else No prior record
Tracker->>Storage: Verify object exists
Tracker->>DB: INSERT binding
else Foreign, archived, or workspace file
Tracker-->>Chat: Ownership rejection
end
Reviews (4): Last reviewed commit: "refactor(copilot): gate attachment track..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit dc8c8e5. Configure here.
`POST /api/copilot/chat` accepted a client-supplied `fileAttachments[].key` and passed it straight into `trackChatUpload`, which wrote it into the `workspace_files` ownership binding with no permission check and no key-ownership validation. That binding is what `verifyFileAccess` and the Files feature resolve authorization from, so any workspace member — including read-only — could hand in another member's key and have their file re-parented to a private chat: removed from the Files listing, from folders and from download-by-id, and destroyable through the chat-delete FK cascade. The sibling register route already enforced these invariants; the copilot path did not. - `trackChatUpload` now rejects keys that do not address the target workspace, only re-links a chat-upload row the caller already owns (matched by row id, not by raw key), and only mints a new binding when the key has no prior record at all — including soft-deleted ones, which the partial active-key unique index would otherwise let it insert over. - Minting a new binding verifies the object exists in storage, matching `registerUploadedWorkspaceFile`. - `buildCopilotRequestPayload` gates attachment tracking on write/admin, the same grant the upload routes that issue these keys require.
…rite The ownership lookup and the binding UPDATE were separate statements, and the UPDATE matched on the captured row id alone. A concurrent `materialize_file` sets `context='workspace'` and clears `chatId` on that same row, so the tracking write still matched and dragged the saved file back into chat scope — hiding it from every workspace-file listing and re-exposing it to the chat-delete cascade, with materialize's storage-usage increment left stranded. Re-assert every ownership predicate in the UPDATE so the statement is itself the atomic check. The lookup now only decides UPDATE-vs-INSERT and is no longer load-bearing for authorization, which also makes the existing `updated.length === 0` fail-closed branch correct rather than dead.
…ys with 403 Follow-ups from a backward-compatibility audit of the attachment-key hardening. The existence probe is hygiene, not authorization — the key-format and no-prior-record guards already carry that, and a binding to a nonexistent object grants nothing readable. But `headObject` rethrows non-404 provider errors, so a transient 5xx or throttle would drop a legitimate attachment. Only a definitive not-found now rejects; a thrown error logs and proceeds on the ownership guards. This path is reached solely by >50MB multipart uploads, the one flow that persists no metadata row at upload time. The stage route mapped an ownership rejection to a 500. It is a client error; return 403 instead.
Two overlapping chat requests could both observe the same claimable row with `chat_id IS NULL` and both satisfy the update predicate, so the later write silently moved the upload to its own chat — taking over the delete-cascade lifecycle of a file the first chat had already bound. Scope the update to `chat_id IS NULL OR chat_id = <target>` so the statement is a compare-and-swap: the loser matches zero rows and fails closed. The resolver applies the same rule so the update-vs-insert decision stays coherent. This also makes an upload bind to exactly one chat, matching the 409 the sibling `local-files/stage` route already returns for the same case; verified no client flow relinks a key across chats (drafts are per-chat, every retry path replays under a pinned chat id, forking copies blobs to fresh keys).
80ea4ab to
d88c822
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d88c822. Configure here.
…n predicate `permissionSatisfies` is the documented single source of truth for permission comparisons and exists to replace hand-written `=== 'admin' || === 'write'` ladders. `userPermission` is typed `string` for legacy reasons, so narrow it with `isPermissionType` first — an unrecognized value fails the gate instead of ranking below every level. Behavior is unchanged for all three levels. Imported from the dependency-free `/predicates` subpath rather than `/workspace`, which would pull `@sim/db` onto the chat request path.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 175d608. Configure here.
1 participant
Problem
POST /api/copilot/chataccepted a client-suppliedfileAttachments[].keyand passed it straight intotrackChatUpload(), which wrote it into theworkspace_filesownership-binding row with no permission check and no key-ownership validation.workspace_files.keyis the trusted binding thatverifyFileAccess()and the whole workspace Files feature resolve authorization from. The sibling registration path (POST /api/workspaces/[id]/files/register) enforces the invariant explicitly; the copilot attachment path did not.Net effect: a read-only workspace member could take any other member's workspace file, re-bind it to their own private copilot chat, and make it disappear from the workspace — a write/delete capability reached from a read-only grant.
The
UPDATEmatched on(key, workspaceId, deletedAt IS NULL)only. It setcontext='mothership', which removes the row from every workspace-file query (Files tab, folders, download-by-id, restore), and re-parentedchatIdto the attacker's chat — wherecopilot_chats.id ON DELETE CASCADEhard-deletes the victim's row when that chat is deleted. When no active row matched, theINSERTbranch minted a fresh binding claiming a foreign object for the caller's workspace.Fix
trackChatUpload— treats3Keyas untrusted, applying the invariants the direct-upload path already enforces:parseWorkspaceFileKey).workspace_filesrecord at all. Another member's row, acontext='workspace'file, and soft-deleted records are all rejected. (Soft-deleted matters: the active-key unique index is partial ondeleted_at IS NULL, so anINSERTover an archived row would succeed and grant read access to its bytes.)UPDATEre-asserts every ownership predicate rather than trusting the row id captured by the lookup, so the statement itself is the atomic check. A concurrentmaterialize_filesetscontext='workspace'and clearschatIdon the same row; an id-only predicate would drag that saved file back into chat scope.chat_id(IS NULL OR = target). Two overlapping requests both observe an unbound row; only the first wins, and the loser fails closed instead of stealing the upload and its delete-cascade lifecycle. This also makes an upload bind to exactly one chat, matching the 409 the siblinglocal-files/stageroute already returns.headObjectrethrows provider 5xx/throttles, and failing there would drop a legitimate >50MB multipart upload (the only path that reaches this branch).buildCopilotRequestPayload— gate attachment tracking on write/admin via the sharedpermissionSatisfiespredicate, the same grant/api/files/presigned,/api/files/multipartand/api/files/uploadalready require to issue these keys.local-files/stageroute — an ownership rejection is a client error; return 403 instead of 500.Backward compatibility
Audited explicitly for regressions to legitimate users:
insertFileMetadatapassescontextthrough verbatim as'mothership'withchatIdnull, so presigned and the no-cloud-storage fallback land on the UPDATE branch; multipart persists no row and lands on INSERT. Its restore branch also clearsdeleted_at, so re-uploading revives a soft-deleted row before tracking sees it.workspace/{workspaceId}/….sanitizeFileNamecollapses names to[A-Za-z0-9.\-_]and never returns empty, so the pattern holds for unicode, spaces, long names and dotfiles. A pre-feat(tools): added download file tool for onedrive, google drive, and slack; added move email tool for gmail and outlook #1785 prefix-less format exists but is unreachable —trackChatUploadpostdates it and every key is freshly minted in the same request.adminvia an explicit row; org owners/admins are derived admins; the route is session-only (no API-key principal). Areadmember cannot obtain a key in the first place, since all three upload routes already gate on write.workspaceId:chatIdand cleared on submit; every queue/retry/abort-resume path replays under a pinned chat id; no UI attaches an already-uploaded file into a new chat; forking copies blobs to fresh keys.Tests
bunx vitest run lib/uploads lib/copilot app/api/files app/api/mothership app/api/workspaces— 1718 passed. Typecheck clean;bun run check:api-validationpasses.Fifteen new tests cover the ownership contract, the atomicity predicate, the chat compare-and-swap, and the transient-probe tolerance. Each was verified to fail against the unfixed code — reverting the manager turns the ownership/atomicity/CAS tests red, and reverting the gate turns the permission tests red.
Not live-tested. Recommended smoke before production: attach a small file and a >50MB file in a chat and confirm the model reads both, then
materialize_fileone and confirm it appears in Files and the chat still sends. That covers the UPDATE branch, the INSERT +headObjectbranch, and the materialize interaction.