Skip to content

fix(uploads): switch mothership uploads to presigned PUT pattern - #4506

Closed
waleedlatif1 wants to merge 2 commits into
stagingfrom
waleedlatif1/file-download-boundary
Closed

fix(uploads): switch mothership uploads to presigned PUT pattern#4506
waleedlatif1 wants to merge 2 commits into
stagingfrom
waleedlatif1/file-download-boundary

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Migrate mothership/copilot file attachments from server-proxied multipart POST /api/files/upload to presigned PUT URLs via runUploadStrategy, fixing intermittent "missing final boundary while parsing FormData" errors on large uploads (e.g. 24 MB docx)
  • Add mothership branch to /api/files/presigned reusing generateWorkspaceFileKey, MAX_WORKSPACE_FILE_SIZE, workspace permission check, and validateFileType
  • Widen runUploadStrategy context to include mothership so the existing >50 MB multipart ladder works unchanged
  • Keep the legacy /api/files/upload mothership branch as the local-dev FALLBACK_REQUIRED target (mirrors knowledge-base)

Type of Change

  • Bug fix

Testing

Tested manually. direct-upload.test.ts, presigned/route.test.ts, track-chat-upload.test.ts all pass. bun run check:api-validation:strict passes. tsc --noEmit clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercelBot commented May 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
docsSkippedSkippedMay 8, 2026 2:00am

Request Review

@cursor

cursorBot commented May 8, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes the upload path for workspace-scoped (mothership) attachments, adding new presigned-URL and permission/validation logic; mistakes could break uploads or allow/deny access incorrectly. Limited blast radius but touches file storage and authz checks.

Overview
Mothership (workspace) file attachments now upload via presigned PUT URLs instead of server-proxied multipart form uploads. The copilot workspace UI switches to runUploadStrategy with a local-dev fallback to POST /api/files/upload only when cloud storage is unavailable.

The presigned upload API (/api/files/presigned) adds a mothership upload type that requires workspaceId, checks workspace permissions, validates file type, and uses a workspace-derived storage key/metadata. API contracts and the direct-upload client types are updated to recognize mothership as a valid upload context.

Reviewed by Cursor Bugbot for commit ebf97a2. Configure here.

@greptile-apps

greptile-appsBot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR migrates copilot/mothership file attachments from a server-proxied multipart POST to presigned PUT URLs via runUploadStrategy, fixing the "missing final boundary" FormData errors on large uploads. A local-dev fallback path is preserved via FALLBACK_REQUIRED.

  • /api/files/presigned/route.ts — adds a mothership branch that validates workspace membership, file type, and generates a workspace/{workspaceId}/... key for presigned PUT uploads (< 50 MB).
  • use-file-attachments.ts — replaces the inline fetch('/api/files/upload') with runUploadStrategy + a DirectUploadError FALLBACK_REQUIRED catch block for local dev.
  • direct-upload.ts / storage-transfer.ts — widen the context type union and schema enum to include 'mothership', enabling the existing > 50 MB multipart ladder for that context.

Confidence Score: 3/5

Two 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).

Security Review

  • Authorization bypass in apps/sim/app/api/files/presigned/route.ts (mothership branch, line 134): The permission check permission === null only blocks non-members. Users with 'read'-only workspace access can obtain a presigned upload URL and write files into the workspace. The peer multipart route correctly requires 'write' or 'admin'.

Important Files Changed

FilenameOverview
apps/sim/app/api/files/presigned/route.tsAdds 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.tsWidens 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.tsCleanly 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.tsAdds 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
Loading

Comments Outside Diff (1)

  1. apps/sim/lib/uploads/client/direct-upload.ts, line 548-556 (link)

    P1Multipart path stores mothership files under kb/ key prefix

    When a mothership file exceeds LARGE_FILE_THRESHOLD (50 MB), runUploadStrategy calls uploadViaMultipart with context: 'mothership'. The multipart route's initiate action only sets a customKey for context === 'workspace'; for mothership it leaves customKey undefined. initiateS3MultipartUpload falls through to the default kb/ prefix key. The resulting key has a kb/ prefix but is written to the workspace S3 bucket. inferContextFromKey has no mothership/ 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

Comment threadapps/sim/app/api/files/presigned/route.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

Comment threadapps/sim/app/api/files/presigned/route.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment threadapps/sim/app/api/files/presigned/route.ts
Comment threadapps/sim/app/api/files/presigned/route.ts
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

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).

@waleedlatif1
waleedlatif1 deleted the waleedlatif1/file-download-boundary branch May 9, 2026 22:05
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@waleedlatif1