fix(e-signer): file upload error handling #737
Conversation
|
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. 📝 WalkthroughWalkthroughReduced server and client upload limits to 20MB, added client-side file size validation with a FileSizeError class, improved upload error handling to map HTTP 413 responses into FileSizeError, and updated the upload page to present clearer size-related error messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Store as File Store
participant API as Backend API
User->>Browser: Select file & trigger upload
Browser->>Store: uploadFile(file)
Store->>Store: Compare file.size to MAX_FILE_SIZE (20MB)
alt Exceeds client limit
Store->>Browser: Throw FileSizeError (client-side)
Browser->>User: Display size violation message
else Within client limit
Store->>API: POST file (multipart/form-data)
API->>API: Multer enforces 20MB limit
alt Exceeds server limit
API-->>Store: HTTP 413 Payload Too Large
Store->>Store: Build FileSizeError from server data
Store->>Browser: Throw FileSizeError
Browser->>User: Display server-provided size error
else Upload success
API-->>Store: 200 OK
Store->>Browser: Refresh file list
Browser->>User: File available for signing
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Fix all issues with AI agents
In `@platforms/esigner-api/src/controllers/FileController.ts`:
- Around line 5-8: The current multer setup buffers uploads in memory (const
upload = multer(..., storage: multer.memoryStorage())) with a 1GB limit which
risks exhausting process memory under concurrent uploads; change to a safer
approach by replacing multer.memoryStorage() with multer.diskStorage() and
stream writes to the final destination in the upload handling code (or add
per-user quota and concurrency controls around the upload middleware), or if
intentional add a clear comment above the upload declaration documenting the
risk and listing mitigating infrastructure (load‑balancer limits, container
memory limits) and/or implement upload concurrency limits around the upload
middleware; refer to the upload constant and the use of multer.memoryStorage()
when making the change.
In `@platforms/esigner-api/src/index.ts`:
- Around line 64-65: The body-parser limits are set to an excessively large 1gb
via express.json(...) and express.urlencoded(...), which risks memory
exhaustion; lower these limits to a reasonable size (e.g., 10-50MB) by updating
the express.json and express.urlencoded calls in index.ts to a smaller limit and
leave multer’s file-size limits to handle multipart uploads (adjust multer
configuration separately if larger file uploads are required). Ensure you change
the limit values referenced in the express.json and express.urlencoded calls and
verify multer remains responsible for multipart/form-data file size enforcement.
In `@platforms/esigner/src/routes/`(protected)/files/new/+page.svelte:
- Line 5: The import line currently brings in unused symbols `files` and
`fetchFiles`; remove those two from the import so only `uploadFile` and
`FileSizeError` are imported (i.e., change the import that references `files,
fetchFiles, uploadFile, FileSizeError` to import just `uploadFile` and
`FileSizeError`) to eliminate dead imports and related lint warnings.
🧹 Nitpick comments (1)
platforms/esigner/src/lib/stores/files.ts (1)
105-120: HTTP 413 handling looks correct, but type assertion could be cleaner.The error handling correctly detects 413 responses and extracts server-provided size information. However, using axios's built-in types would be more robust.
Optional: Use axios types for cleaner error handling
- } catch (err: unknown) { - // Handle HTTP 413 Payload Too Large - if (err && typeof err === 'object' && 'response' in err) { - const axiosError = err as { response?: { status?: number; data?: { error?: string; maxSize?: number; fileSize?: number } } }; - if (axiosError.response?.status === 413) { + } catch (err: unknown) { + // Handle HTTP 413 Payload Too Large + import { isAxiosError } from 'axios'; + if (isAxiosError(err) && err.response?.status === 413) { + const data = err.response.data as { error?: string; maxSize?: number; fileSize?: number } | undefined;Note:
isAxiosErrorwould need to be imported at the top of the file.
| const upload = multer({ | ||
| limits: { fileSize: 10 * 1024 * 1024 }, // 10MB limit | ||
| limits: { fileSize: 1024 * 1024 * 1024 }, // 1GB limit | ||
| storage: multer.memoryStorage(), | ||
| }); |
There was a problem hiding this comment.
Memory storage with 1GB limit poses a resource exhaustion risk.
Using multer.memoryStorage() with a 1GB limit means entire files are buffered in memory. Concurrent uploads of large files could exhaust server memory and crash the process.
Consider one of these alternatives for production:
- Use disk storage (
multer.diskStorage()) and stream to the final destination - Implement upload size quotas per user
- Add concurrency limits on uploads
If this tradeoff is intentional for simplicity, consider adding a comment documenting the risk and any mitigating infrastructure (e.g., load balancer limits, container memory limits).
🤖 Prompt for AI Agents
In `@platforms/esigner-api/src/controllers/FileController.ts` around lines 5 - 8,
The current multer setup buffers uploads in memory (const upload = multer(...,
storage: multer.memoryStorage())) with a 1GB limit which risks exhausting
process memory under concurrent uploads; change to a safer approach by replacing
multer.memoryStorage() with multer.diskStorage() and stream writes to the final
destination in the upload handling code (or add per-user quota and concurrency
controls around the upload middleware), or if intentional add a clear comment
above the upload declaration documenting the risk and listing mitigating
infrastructure (load‑balancer limits, container memory limits) and/or implement
upload concurrency limits around the upload middleware; refer to the upload
constant and the use of multer.memoryStorage() when making the change.
| import { goto } from '$app/navigation'; | ||
| import { isAuthenticated } from '$lib/stores/auth'; | ||
| import { uploadFile } from '$lib/stores/files'; | ||
| import { files, fetchFiles, uploadFile, FileSizeError } from '$lib/stores/files'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\b(files|fetchFiles)\b' 'platforms/esigner/src/routes/(protected)/files/new/+page.svelte' | grep -v 'import'Repository: MetaState-Prototype-Project/prototype
Length of output: 1067
Remove unused imports files and fetchFiles.
The imports files and fetchFiles are not used in this file. Only uploadFile and FileSizeError are referenced. Remove the unused imports from line 5.
🤖 Prompt for AI Agents
In `@platforms/esigner/src/routes/`(protected)/files/new/+page.svelte at line 5,
The import line currently brings in unused symbols `files` and `fetchFiles`;
remove those two from the import so only `uploadFile` and `FileSizeError` are
imported (i.e., change the import that references `files, fetchFiles,
uploadFile, FileSizeError` to import just `uploadFile` and `FileSizeError`) to
eliminate dead imports and related lint warnings.
sosweetham
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@platforms/esigner-api/src/controllers/FileController.ts`:
- Around line 5-7: Add a global error-handling middleware after all routes to
catch multer file-size errors: detect errors that are instances of MulterError
(or have error.code === 'LIMIT_FILE_SIZE') produced by the upload middleware
(the upload constant using multer.memoryStorage()), and respond with HTTP 413
and a clear message; non-multer errors should be passed to the next handler or
handled normally. Ensure the middleware is registered after route registration
so LIMIT_FILE_SIZE errors thrown during middleware preprocessing are converted
to 413 responses.
* fix(e-signer): file upload error handling and coherent limits with file manager * fix: readd svelte markup * fix: coderabbit suggestions * fix: limit to 20 megs * fix: express req body size 20M
Description of change
fixes file upload error message on esigner
Issue Number
closes #713
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.