fix(uploads): switch mothership uploads to presigned PUT pattern#4506
fix(uploads): switch mothership uploads to presigned PUT pattern#4506waleedlatif1 wants to merge 2 commits intostagingfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The presigned upload API ( Reviewed by Cursor Bugbot for commit ebf97a2. Configure here. |
Greptile SummaryThis PR migrates copilot/mothership file attachments from a server-proxied multipart POST to presigned PUT URLs via
Confidence Score: 3/5Two correctness issues need fixes before merging: the presigned route permission check is weaker than the multipart route, and large mothership files routed through multipart will be stored under the wrong key prefix. The presigned mothership branch checks only permission === null, granting upload access to read-only workspace members. Separately, when a mothership file exceeds 50 MB and goes through the multipart path, no custom key is set for the mothership context so initiateS3MultipartUpload falls back to a kb/-prefixed key, inconsistent with the workspace-scoped key used for smaller files. apps/sim/app/api/files/presigned/route.ts (permission guard) and apps/sim/app/api/files/multipart/route.ts (missing custom key generation for mothership in the multipart initiate path).
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/files/presigned/route.ts | Adds mothership branch to presigned route; permission check uses === null instead of requiring write/admin, allowing read-only members to upload. |
| apps/sim/lib/uploads/client/direct-upload.ts | Widens context union to include mothership; files >50 MB routed to multipart will get a kb/ key prefix instead of the workspace-scoped key used by the presigned-PUT path. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/copilot/components/user-input/hooks/use-file-attachments.ts | Cleanly migrates copilot/mothership uploads from server-proxied POST to presigned PUT via runUploadStrategy, with a correct FALLBACK_REQUIRED fallback for local dev. |
| apps/sim/lib/api/contracts/storage-transfer.ts | Adds mothership to the validUploadTypes enum; straightforward contract update with no issues. |
Sequence Diagram
sequenceDiagram
participant Client as useFileAttachments
participant Presigned as /api/files/presigned
participant S3 as Cloud Storage
participant Multipart as /api/files/multipart
participant Fallback as /api/files/upload
Client->>Client: file selected
alt "file <= 50 MB presigned PUT path"
Client->>Presigned: "POST type=mothership workspaceId"
Presigned->>Presigned: validatePermission validateFileType
Presigned->>Presigned: generateWorkspaceFileKey
Presigned-->>Client: presignedUrl fileInfo
Client->>S3: PUT file direct
S3-->>Client: 200 OK
else "file > 50 MB multipart path"
Client->>Multipart: "POST action=initiate context=mothership"
Note over Multipart: customKey not set for mothership defaults to kb/ prefix
Multipart-->>Client: uploadId key uploadToken
Client->>Multipart: "POST action=get-part-urls"
Multipart-->>Client: presignedUrls
loop per chunk
Client->>S3: PUT chunk
end
Client->>Multipart: "POST action=complete"
Multipart-->>Client: path key
else FALLBACK_REQUIRED local dev
Client->>Fallback: POST multipart form-data
Fallback-->>Client: fileInfo
end
Client->>Client: update attachedFiles state
Comments Outside Diff (1)
-
apps/sim/lib/uploads/client/direct-upload.ts, line 548-556 (link)Multipart path stores mothership files under
kb/key prefixWhen a mothership file exceeds
LARGE_FILE_THRESHOLD(50 MB),runUploadStrategycallsuploadViaMultipartwithcontext: 'mothership'. The multipart route'sinitiateaction only sets acustomKeyforcontext === 'workspace'; for mothership it leavescustomKeyundefined.initiateS3MultipartUploadfalls through to the defaultkb/prefix key. The resulting key has akb/prefix but is written to the workspace S3 bucket.inferContextFromKeyhas nomothership/case and would throw or misidentify the file as 'knowledge-base' for any downstream operation that resolves context from the key alone.
Reviews (2): Last reviewed commit: "fix(uploads): drop unreachable size guar..." | Re-trigger Greptile
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ebf97a2. Configure here.
|
Closing — superseded by #4509, which bundles this mothership migration with profile-picture, workspace-logo, workflow file-upload, and execution-trigger migrations. The bot review comments here have been carried forward and addressed in #4509 (commit 56b90ef): tightened mothership permission to write/admin, fixed multipart route to generate the proper customKey for mothership and execution contexts (was defaulting to kb/... prefix for files >50 MB). |

Summary
POST /api/files/uploadto presigned PUT URLs viarunUploadStrategy, fixing intermittent "missing final boundary while parsing FormData" errors on large uploads (e.g. 24 MB docx)mothershipbranch to/api/files/presignedreusinggenerateWorkspaceFileKey,MAX_WORKSPACE_FILE_SIZE, workspace permission check, andvalidateFileTyperunUploadStrategycontext to includemothershipso the existing >50 MB multipart ladder works unchanged/api/files/uploadmothership branch as the local-devFALLBACK_REQUIREDtarget (mirrors knowledge-base)Type of Change
Testing
Tested manually.
direct-upload.test.ts,presigned/route.test.ts,track-chat-upload.test.tsall pass.bun run check:api-validation:strictpasses.tsc --noEmitclean.Checklist