Skip to content

Send MCP uploads directly to storage that accepts them - #20630

Open
brandonkelly wants to merge 2 commits into
5.xfrom
feature/mcp-direct-uploads
Open

brandonkelly wants to merge 2 commits into
5.xfrom
feature/mcp-direct-uploads

Conversation

@brandonkelly

Copy link
Copy Markdown
Member

Summary

On Craft Cloud, web requests are capped at 6 MB (a Lambda limit), so files PUT to assets_upload_url's Craft endpoint over that size never arrive. Cloud's control panel uploader avoids the limit by sending files straight to S3 through presigned URLs. This PR lets MCP uploads do the same.

+craft\base\DirectUploadFsInterface
+  getDirectUploadUrl(string $path, DateTimeInterface $expiresAt): ?array{url, method, headers}

 assets_upload_url
   fs = temp asset upload filesystem
+  if fs implements DirectUploadFsInterface and returns a URL
+    record mcp-uploads/<id>.json {userId, expiresAt, direct: true}
+    return {upload_url: storage URL, method, headers}      (no Craft token is created)
+  else
     token → Craft's mcp/upload URL, as before
+    return {…, headers: {}}

 assets_create / assets_replace {upload_id}
   lock; check the record and the user
-  require record.size
+  direct: require fs.fileExists(mcp-uploads/<id>)
+          if fs.getFileSize() > maxUploadFileSize: delete the file, fail (the URL can be retried until it expires)
+  Craft:  require record.size
   delete the record, copy the file locally, save the asset
  • Interface contract: I agreed it with the Cloud side. $path is relative to the filesystem root. Null means "use Craft's endpoint". The file appears all at once. The URL may be reused until it expires, and orphaned files are left to stale-upload cleanup. The URL can't enforce a size limit, so the claim checks the size first.
  • Tool description: assets_upload_url now tells clients to send method and every header in headers. S3 rejects a PUT without the signed x-amz-meta-* headers with SignatureDoesNotMatch.
  • Cloud dependencies: Issue direct upload URLs from Cloud filesystems cloud#216 implements the interface on Cloud's base Fs, with a shim for older Craft versions. It only takes effect once Create the default temp upload filesystem via the DI container #20628 makes Cloud's TmpFs the temp upload filesystem.
  • Release: this adds new API in a patch release, marked @since 5.12.2.

Evidence

  • Before: AssetFieldUploadTest::testUploadUrlsSendFilesDirectlyToFilesystemsThatAcceptThem fails with Failed asserting that two strings are identical. The old code returns Craft's URL.
    After: it passes.
    temp fs = Local subclass implementing DirectUploadFsInterface, maxUploadFileSize = 100
    createUpload                  → upload_url = the storage URL, headers {"x-amz-meta-visibility":"private"}, no token row added
    claim before the upload       → "Nothing has been uploaded yet"
    101-byte file stored          → claim fails "exceeds the maximum upload size"; file deleted, URL still usable
    real file stored at that path → claim returns its bytes; mcp-uploads/ is empty
    fs returns null               → Craft's own URL, headers {}
    
  • The Cloud thread tested its implementation against SeaweedFS's S3 API. A 7 MB PUT with the returned headers got 200, and a PUT without them got 403.

unit/mcp (77 tests) and functional/McpServersCest (9) pass locally. PHPStan and ECS are clean.

Adds craft\base\DirectUploadFsInterface, which filesystems can implement to
issue URLs that store files directly, such as presigned S3 PUT URLs. When
the temporary asset upload filesystem implements it, assets_upload_url
returns its URL and the headers clients must send, rather than Craft's own
upload URL, so files avoid request size limits such as Craft Cloud's 6 MB
limit. Claims check the stored file's size, since direct URLs can't enforce
maxUploadFileSize.

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a1c652-71e9-7674-ba66-cdb7cbecf509

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the direct-upload and fallback changes.

No open issues 🎉

What changed in this PR

Enables MCP uploads to bypass Craft’s web-request size limits by sending files directly to compatible temporary storage, while retaining Craft-hosted uploads as a fallback.

Changes:

  • Adds a filesystem interface for direct upload URLs with required methods and headers.
  • Updates upload creation and claiming to support direct storage uploads and enforce size limits.
  • Adds regression coverage for direct uploads, oversized-file rejection, cleanup, and fallback behavior.
File Description
tests/​unit/​mcp/​AssetFieldUploadTest.php Tests direct uploads, size enforcement, cleanup, and fallback.
src/​mcp/​capabilities/​Assets.php Documents returned headers and client upload requirements.
src/​mcp/​AssetUploads.php Supports direct upload URLs and validates stored files before claiming.
src/​base/​DirectUploadFsInterface.php Defines the optional direct-upload filesystem contract.
CHANGELOG.md Documents direct uploads and the new public interface.

Review effort: Balanced

The scheme follows useSslOnTokenizedUrls, which differs in CI.

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a1c652-71e9-7674-ba66-cdb7cbecf509

This branch has not been deployed

No deployments
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.

2 participants