Skip to content

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

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

fix(uploads): switch mothership uploads to presigned PUT pattern#4506
waleedlatif1 wants to merge 2 commits intostagingfrom
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
Copy link
Copy Markdown

vercel Bot commented May 8, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 8, 2026 2:00am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 8, 2026

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
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

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

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
Loading

Comments Outside Diff (1)

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

    P1 Multipart 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 thread apps/sim/app/api/files/presigned/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/files/presigned/route.ts
Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 thread apps/sim/app/api/files/presigned/route.ts
Comment thread apps/sim/app/api/files/presigned/route.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

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

Sign up for free to 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