feat: pass file metadata to checkUpload and bind ContentLength in presigned URLs#35
feat: pass file metadata to checkUpload and bind ContentLength in presigned URLs#35ginzlabs wants to merge 3 commits into
Conversation
…signed URLs Extend the upload flow to forward file size and content type information through the entire chain, enabling server-side validation and S3-level enforcement of upload constraints. Changes: - R2.generateUploadUrl() accepts optional contentLength/contentType, bound into the PutObjectCommand so the presigned URL signature enforces exact file size - checkUpload callback receives optional fileInfo as a third parameter, allowing users to implement custom validation (size limits, file type checks, per-user quotas, etc.) - generateUploadUrl mutation accepts optional fileSize/contentType args - React, Svelte, and Vue hooks automatically pass file.size and file.type - Updated tests for all three frameworks - Example app demonstrates 1MB file size limit with client-side fast check, server-side ConvexError validation, and user-friendly error display Fully backward compatible - all new parameters are optional. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR propagates client-side file metadata (size and MIME type) through the upload flow: Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client UI
participant Lib as Frontend lib (useUploadFile)
participant Server as Convex Mutation (generateUploadUrl)
participant R2 as R2 Upload Service
Client->>Lib: select file (file.size, file.type)
Lib->>Server: generateUploadUrl({ fileSize, contentType })
Server->>Server: validate args (fileSize non-negative)
Server->>Server: call checkUpload(ctx, bucket, { size, type })
Server-->>Lib: return upload URL (includes ContentLength/Type metadata)
Lib->>R2: upload file to URL with Content-Length/Type
R2-->>Lib: upload result (key)
Lib->>Server: syncMetadata(key, ...)
Server-->>Client: final success / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@example/src/App.tsx`:
- Line 59: The line creating const file from event.target.files![0] can throw if
the file picker was cancelled; update the change handler in App.tsx to guard
against null/empty file lists by checking event.target.files (or using optional
chaining and length check) and return early or handle the no-file case before
accessing [0]; ensure the rest of the handler (the logic that uses file) only
runs when a valid File object is present.
🧹 Nitpick comments (3)
src/client/index.ts (2)
396-416: Consider validatingfileSizeis a positive integer before passing it asContentLength.
v.optional(v.number())permits negative values, zero, fractional numbers,NaN, andInfinity. Passing any of these asContentLengthin thePutObjectCommandwill either produce an invalid presigned URL or be rejected by S3/R2 at signing time with an opaque error. A simple guard in the handler (or a more restrictive validator) would surface a clearer error to the caller.Proposed validation in the handler
handler: async (ctx, args) => { + if ( + args.fileSize !== undefined && + (!Number.isInteger(args.fileSize) || args.fileSize < 0) + ) { + throw new Error("fileSize must be a non-negative integer"); + } if (opts?.checkUpload) {
426-429: Note:syncMetadatacallscheckUploadwithoutfileInfo.This is correct since
syncMetadatadoesn't receive file metadata, and the parameter is optional. However, this means an upload flow still callscheckUploadtwice: once withfileInfo(ingenerateUploadUrl) and once without (insyncMetadata). If acheckUploadimplementation requiresfileInfo.sizeto be present (e.g., throws when missing), it will blocksyncMetadata. This is worth documenting or noting in thecheckUploadJSDoc so consumers write defensive checks (as the example does withfileInfo?.size).example/src/App.tsx (1)
53-53: MoveMAX_FILE_SIZEoutside the component.It's a constant and doesn't depend on props or state. Declaring it at module scope avoids re-creation on every render and makes it easier to share/import if needed.
- Guard against null/empty file list when file picker is cancelled - Validate fileSize is a non-negative integer before presigning - Move MAX_FILE_SIZE constant to module scope - Document that checkUpload is called without fileInfo from syncMetadata Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/index.ts`:
- Around line 417-422: The call to opts?.checkUpload currently always passes an
object { size: args.fileSize, type: args.contentType } even when both fields are
undefined, causing truthy false-positives for callers that test if (fileInfo);
modify the invocation in the block that calls opts?.checkUpload so it constructs
a fileInfo variable (e.g. const fileInfo = (args.fileSize !== undefined ||
args.contentType !== undefined) ? { size: args.fileSize, type: args.contentType
} : undefined) and then call await opts.checkUpload(ctx, this.config.bucket,
fileInfo) so fileInfo is only passed when at least one field is defined
(referencing opts?.checkUpload, ctx, this.config.bucket, args.fileSize,
args.contentType).
🧹 Nitpick comments (2)
example/src/App.tsx (2)
81-86:error.data as stringwill render poorly if the server throws a non-string payload.If
ConvexErroris ever thrown with a structureddatavalue (e.g., an object), this cast will display[object Object]in the error banner. A small guard would make this more resilient:Suggested improvement
const message = error instanceof ConvexError - ? (error.data as string) + ? String(error.data) : "Failed to upload file";
134-140: File input value is never reset — re-selecting the same file won't triggeronChange.After an upload (successful or failed), the
<input>retains its last value. If the user picks the identical file again, most browsers won't fireonChange. Resetting the input at the end ofhandleUploadavoids this:One way to fix
} finally { setUploadProgress(null); + event.target.value = ""; }(Move
eventinto the closure or use a ref to the input element.)
- Only pass fileInfo to checkUpload when at least one field is defined - Use String(error.data) instead of type cast for resilient error display - Reset file input after upload attempt to allow re-selecting same file Co-authored-by: Cursor <cursoragent@cursor.com>
|
Late review here - thanks for this! Would need to think a bit more on how to approach. Two issues:
Part of the problem is how checkUpload() is reused between syncMetadata and generateUploadUrl, we may need to provide separate checks for those, or differentiate between an auth check and input/file validation check. |
2 participants
Summary
checkUploadcallback with an optionalfileInfoparameter ({ size?, type? }), enabling users to implement custom upload validation (file size limits, type restrictions, per-user quotas, etc.)ContentLengthandContentTypein presigned PUT URLs viaPutObjectCommand, so R2 cryptographically enforces exact file size at the S3 protocol levelfile.sizeandfile.typeto thegenerateUploadUrlmutationMotivation
Currently there is no way to enforce upload size limits through the R2 component. The
checkUploadcallback only receives(ctx, bucket)with no information about the file being uploaded.This is a common need in practice — I've had to manually implement file size validation on top of the R2 component across multiple projects. Passing file metadata through the existing
checkUploadcallback is a natural extension that saves every user from reimplementing this themselves, while theContentLengthbinding in the presigned URL provides S3-level cryptographic enforcement for free.Changes
Client library (
src/client/index.ts):R2.generateUploadUrl()accepts optional{ contentLength, contentType }— included inPutObjectCommandso the presigned URL signature covers the exact file sizecheckUploadtype extended with optional third parameterfileInfo?: { size?, type? }generateUploadUrlmutation accepts optionalfileSize/contentTypeargs, forwarded to both the callback and the presigned URLHooks (
src/react/,src/svelte/,src/vue/):useUploadFilehooks passfile.sizeandfile.typeautomaticallyExample app:
checkUploaddemonstrates a 1MB limit usingConvexErrorApp.tsxadds client-side validation for instant feedback and error displayBackward compatibility
Fully backward compatible — all new parameters are optional. Existing
checkUploadcallbacks that only take(ctx, bucket)continue to work unchanged.Test plan
fileSize/contentTypeare forwardednpm run test)npm run build,npm run typecheck)Content-Lengthin the presigned URL doesn't match the actual file (tampered +1 byte upload returns 403)Summary by CodeRabbit
New Features
Bug Fixes