feat: enhance image upload validation across the application#22766
feat: enhance image upload validation across the application#22766anikdhabal merged 54 commits intocalcom:mainfrom
Conversation
|
@Devanshusharma2005 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds shared image validation constants/utilities and new validators: a server-side 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
keithwillcode
added
the
community-interns
Graphite Automations"Add community label" took an action on this PR • (07/28/25)1 label was added to this PR based on Keith Williams's automation. |
![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
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (1)
120-157: Remove redundant format validation aftervalidateBase64ImageSame issue as in
updateProfile.handler.ts- the base64 prefix check is redundant aftervalidateBase64Imageand creates a format support mismatch.Apply the same fix to remove redundant validation:
if (input.avatar) { // Comprehensive validation using magic numbers const validation = validateBase64Image(input.avatar); if (!validation.isValid) { throw new TRPCError({ code: "BAD_REQUEST", message: `Invalid avatar image: ${validation.error}`, }); } - // Additional check for supported base64 formats (after validation) - if ( - input.avatar.startsWith("data:image/png;base64,") || - input.avatar.startsWith("data:image/jpeg;base64,") || - input.avatar.startsWith("data:image/jpg;base64,") || - input.avatar.startsWith("data:image/svg+xml;base64,") - ) { + if (input.avatar === "") { + data.avatarUrl = null; + } else { try { const avatar = await resizeBase64Image(input.avatar); data.avatarUrl = await uploadAvatar({ avatar, userId: user.id, }); } catch (error) { throw new TRPCError({ code: "BAD_REQUEST", message: error instanceof Error ? error.message : "Failed to upload avatar", }); } - } else if (input.avatar === "") { - data.avatarUrl = null; - } else { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Unsupported image format. Please use PNG, JPEG, or SVG.", - }); } }packages/ui/components/image-uploader/ImageUploader.tsx (1)
94-220: Extract duplicatedvalidateImageFilefunctionThis validation function is identical to the one in
BannerUploader.tsx. Both components should share a common validation utility.See the suggestion in
BannerUploader.tsxreview for extracting this to a shared utility file.
🧹 Nitpick comments (4)
packages/lib/server/imageValidation.ts (2)
123-132: Consider optimizing SVG content scanning for performance.Converting the entire buffer to UTF-8 string for content scanning could be expensive for large SVG files. Consider scanning the first few KB or using a streaming approach for better performance.
- const textContent = buffer.toString("utf8"); + // Only scan first 10KB for dangerous patterns to improve performance + const scanLength = Math.min(buffer.length, 10 * 1024); + const textContent = buffer.subarray(0, scanLength).toString("utf8");
175-191: Simplify the JPEG format handling logic.The function works correctly but has redundant logic in the JPEG validation section.
// Handle JPEG variations if ( - (detectedFormat === "jpeg" && (expectedFormat === "jpg" || expectedFormat === "jpeg")) || - (expectedFormat === "jpeg" && detectedFormat === "jpeg") + (detectedFormat === "jpeg" && (expectedFormat === "jpg" || expectedFormat === "jpeg")) ) { return true; }The second condition is redundant since
expectedFormat === "jpeg" && detectedFormat === "jpeg"is already covered by the direct comparison on line 190.packages/trpc/server/routers/viewer/organizations/update.handler.ts (2)
165-199: LGTM: Comprehensive banner validation and error handlingThe banner validation implementation is thorough with proper TRPC error handling. The try-catch block ensures robust error handling during upload operations.
Consider removing the format checking after validation (lines 175-199) since
validateBase64Imagealready validates the format. The current approach adds redundancy:if (input.banner) { // Validate the banner image data const validation = validateBase64Image(input.banner); if (!validation.isValid) { throw new TRPCError({ code: "BAD_REQUEST", message: `Invalid banner image: ${validation.error}`, }); } - if ( - input.banner.startsWith("data:image/png;base64,") || - input.banner.startsWith("data:image/jpeg;base64,") || - input.banner.startsWith("data:image/jpg;base64,") || - input.banner.startsWith("data:image/svg+xml;base64,") - ) { try { const banner = await resizeBase64Image(input.banner, { maxSize: 1500 }); data.bannerUrl = await uploadLogo({ logo: banner, teamId: currentOrgId, isBanner: true, }); } catch (error) { throw new TRPCError({ code: "BAD_REQUEST", message: error instanceof Error ? error.message : "Failed to upload banner", }); } - } else { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Unsupported banner format. Please use PNG, JPEG, or SVG.", - }); - } }
204-236: LGTM: Consistent logo validation patternThe logo validation follows the same comprehensive pattern as the banner validation, maintaining consistency in the codebase.
Same optimization opportunity applies here - the format checking after validation is redundant since
validateBase64Imagealready handles format validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/api/v1/pages/api/users/[userId]/_patch.ts(2 hunks)apps/web/app/api/avatar/[uuid]/route.ts(3 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/app-store/_utils/oauth/updateProfilePhotoGoogle.ts(2 hunks)packages/lib/server/avatar.ts(2 hunks)packages/lib/server/imageValidation.ts(1 hunks)packages/trpc/server/routers/viewer/me/updateProfile.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/update.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts(2 hunks)packages/ui/components/image-uploader/BannerUploader.tsx(1 hunks)packages/ui/components/image-uploader/ImageUploader.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/lib/server/avatar.tspackages/app-store/_utils/oauth/updateProfilePhotoGoogle.tsapps/api/v1/pages/api/users/[userId]/_patch.tsapps/web/app/api/avatar/[uuid]/route.tspackages/trpc/server/routers/viewer/organizations/updateUser.handler.tspackages/trpc/server/routers/viewer/me/updateProfile.handler.tspackages/ui/components/image-uploader/BannerUploader.tsxpackages/trpc/server/routers/viewer/organizations/update.handler.tspackages/ui/components/image-uploader/ImageUploader.tsxpackages/lib/server/imageValidation.ts
🧠 Learnings (3)
apps/web/app/api/avatar/[uuid]/route.ts (1)
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to backend/**/*.{ts,tsx} : For Prisma queries: Only select data you need.
apps/web/public/static/locales/en/common.json (1)
Learnt from: bandhan-majumder
PR: #22359
File: packages/lib/server/locales/en/common.json:1336-1339
Timestamp: 2025-07-14T16:31:45.233Z
Learning: When making localization changes for new features, it's often safer to add new strings rather than modify existing ones to avoid breaking existing functionality that depends on the original strings. This approach allows for feature-specific customization while maintaining backward compatibility.
packages/ui/components/image-uploader/ImageUploader.tsx (1)
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
🧬 Code Graph Analysis (6)
packages/app-store/_utils/oauth/updateProfilePhotoGoogle.ts (1)
packages/lib/server/imageValidation.ts (1)
validateBase64Image(47-141)
apps/web/app/api/avatar/[uuid]/route.ts (2)
packages/lib/server/imageValidation.ts (1)
validateBase64Image(47-141)packages/lib/constants.ts (2)
AVATAR_FALLBACK(91-91)WEBAPP_URL(12-18)
packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts (2)
packages/lib/server/imageValidation.ts (1)
validateBase64Image(47-141)packages/lib/server/avatar.ts (1)
uploadAvatar(8-39)
packages/trpc/server/routers/viewer/me/updateProfile.handler.ts (2)
packages/lib/server/imageValidation.ts (1)
validateBase64Image(47-141)packages/lib/server/avatar.ts (1)
uploadAvatar(8-39)
packages/ui/components/image-uploader/BannerUploader.tsx (1)
packages/lib/server/imageValidation.ts (1)
validateImageFile(146-170)
packages/ui/components/image-uploader/ImageUploader.tsx (1)
packages/lib/server/imageValidation.ts (1)
validateImageFile(146-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Detect changes
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (16)
packages/lib/server/imageValidation.ts (5)
6-10: LGTM! Well-defined interface.The interface is properly structured with appropriate optional properties and clear naming conventions.
15-34: LGTM! Comprehensive file signature definitions.The magic numbers are correctly defined and well-organized. The separation between allowed image formats and dangerous blocked formats enhances security and readability.
39-42: LGTM! Solid helper function implementation.The bounds checking and byte-by-byte comparison logic is correct and handles edge cases appropriately.
47-141: Excellent security-first validation approach!The function correctly prioritizes checking dangerous formats before valid ones, implements comprehensive SVG security scanning, and handles edge cases well.
146-170: LGTM! Well-structured file validation with appropriate checks.The function correctly implements layered validation (MIME type, size, magic numbers) and efficiently reuses the base64 validation logic for consistency.
apps/web/public/static/locales/en/common.json (1)
3420-3428: No functional issues – double-check parity in other locale filesThe new image-validation strings are well-formed and correctly inserted above the sentinel line, so this file remains valid JSON.
To avoid “missing translation” warnings at runtime, please add the same keys to the other locale JSON files (e.g.de,es, etc.) or confirm that a fallback mechanism is in place.packages/lib/server/avatar.ts (3)
6-6: LGTM: Import added for validation utilityThe import of
validateBase64Imageis appropriately placed and necessary for the validation logic.
9-13: LGTM: Early validation prevents processing invalid imagesThe validation is correctly placed before expensive operations like UUID generation and image processing. The error message includes validation details, which is helpful for debugging.
50-54: LGTM: Consistent validation pattern applied to logo uploadsThe same validation pattern is consistently applied to the
uploadLogofunction, maintaining code consistency and security standards.packages/app-store/_utils/oauth/updateProfilePhotoGoogle.ts (2)
6-6: LGTM: Import added for OAuth image validationThe import of
validateBase64Imageis correctly added for validating OAuth profile images.
26-31: LGTM: Proper validation and error handling for OAuth imagesThe validation correctly prevents processing of invalid images from Google OAuth. The error logging and early return pattern is appropriate for handling validation failures without disrupting the OAuth flow.
apps/api/v1/pages/api/users/[userId]/_patch.ts (1)
6-6: LGTM: Import added for API avatar validationThe import of
validateBase64Imageis correctly added for validating avatar uploads in the API endpoint.apps/web/app/api/avatar/[uuid]/route.ts (3)
9-9: LGTM: Import added for stored image validationThe import of
validateBase64Imageis correctly added for validating stored avatar images before serving.
50-57: Excellent security enhancement: Validation of stored imagesThis is a great security improvement that validates stored images before serving them. The approach of redirecting to a fallback avatar instead of serving potentially dangerous content is the right defensive strategy.
85-89: LGTM: Comprehensive security headersThe security headers are well-chosen and comprehensive:
X-Content-Type-Options: nosniffprevents MIME type confusion attacksContent-Disposition: inlineensures proper handlingX-Frame-Options: DENYprevents clickjackingContent-Security-Policyrestricts resource loading to prevent XSSThese headers significantly enhance the security posture when serving avatar images.
packages/trpc/server/routers/viewer/organizations/update.handler.ts (1)
6-6: LGTM: Import added for organization image validationThe import of
validateBase64Imageis correctly added for validating banner and logo images in organization updates.
…euse in BannerUploader and ImageUploader
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/api/v1/pages/api/users/[userId]/_patch.ts(2 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/trpc/server/routers/viewer/me/updateProfile.handler.ts(2 hunks)packages/ui/components/image-uploader/BannerUploader.tsx(2 hunks)packages/ui/components/image-uploader/ImageUploader.tsx(2 hunks)packages/ui/components/image-uploader/imageValidation.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/public/static/locales/en/common.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui/components/image-uploader/BannerUploader.tsx
- apps/api/v1/pages/api/users/[userId]/_patch.ts
- packages/trpc/server/routers/viewer/me/updateProfile.handler.ts
- packages/ui/components/image-uploader/ImageUploader.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/ui/components/image-uploader/imageValidation.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/image-uploader/imageValidation.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.918Z
Learning: For large pull requests (>500 lines changed or >10 files touched), advise splitting into smaller, focused PRs by feature, layer, dependency chain, or file/module
🔇 Additional comments (5)
packages/ui/components/image-uploader/imageValidation.ts (5)
1-7: Function signature and documentation look good.The async function signature with proper typing and internationalization support is well-designed. The JSDoc comment effectively describes the function's purpose.
8-12: File size validation is implemented correctly.The 5MB limit with decimal-based calculation (5 * 1000000) is appropriate for user-facing applications and aligns with common file size representations.
14-17: MIME type validation provides a good first-level check.This basic MIME type validation is appropriate as an initial filter, though it's correctly supplemented by magic number validation since MIME types can be easily spoofed.
85-126: Image format validation is comprehensive and accurate.The magic number validation for PNG, JPEG, GIF, WEBP, BMP, and ICO formats is correctly implemented. The byte signatures are accurate and the logic properly validates legitimate image formats.
128-133: Error handling is robust and follows best practices.The try-catch block appropriately handles potential exceptions during file processing, and returning false on error ensures safe fallback behavior.
…Devanshusharma2005/cal into fix/image-upload-validation-fix
There was a problem hiding this comment.
Great work @Devanshusharma2005 , haven't tested it yet but looks promising.
The code needs some refactoring and cleanup. Left comments
Also can you whip up a few tests for the validateBase64Image function, devin can probably help here
packages/trpc/server/routers/viewer/organizations/update.handler.ts
Outdated
Show resolved
Hide resolved
|
@Devanshusharma2005 Mind checking the recent Coderabbit comments? Pls check it's made sense or not |
yeah looking into it. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/ui/components/image-uploader/imageValidation.ts (3)
60-76: Use raw head for binary signatures; keep trimmed for text.
Using trimmed bytes for binary formats accepts files with leading whitespace that real decoders will reject. Introduce a raw head slice and use it for PDF/ZIP/EXE and image binaries. This aligns with prior guidance.const arrayBuffer = await file.arrayBuffer(); const uint8Array = new Uint8Array(arrayBuffer); @@ - const strippedData = stripBomAndWhitespace(uint8Array); + const strippedData = stripBomAndWhitespace(uint8Array); @@ - const trimmedHead = strippedData.slice(0, 512); + const trimmedHead = strippedData.slice(0, 512); + // Raw head for binary-signature checks (no trimming) + const head = uint8Array.subarray(0, Math.min(uint8Array.length, 64));
81-91: Binary “dangerous file” checks should use raw head.
Prepending whitespace shouldn’t change binary magic numbers.- if (matchesSignature(trimmedHead, FILE_SIGNATURES.PDF)) { + if (matchesSignature(head, FILE_SIGNATURES.PDF)) { return { isValid: false, error: "pdf_files_cannot_be_uploaded_as_images" }; } - if (matchesSignature(trimmedHead, FILE_SIGNATURES.ZIP)) { + if (matchesSignature(head, FILE_SIGNATURES.ZIP)) { return { isValid: false, error: "zip_files_cannot_be_uploaded_as_images" }; } - if (matchesSignature(trimmedHead, FILE_SIGNATURES.EXECUTABLE)) { + if (matchesSignature(head, FILE_SIGNATURES.EXECUTABLE)) { return { isValid: false, error: "executable_files_cannot_be_uploaded_as_images" }; }
93-103: Image binary signature checks should use raw head.
Ensures headers are at byte 0 as required by format specs. Fix WebP offset too.- const isValidImage = - matchesSignature(trimmedHead, FILE_SIGNATURES.PNG) || - matchesSignature(trimmedHead, FILE_SIGNATURES.JPEG_FF_D8_FF) || - matchesSignature(trimmedHead, FILE_SIGNATURES.GIF87a) || - matchesSignature(trimmedHead, FILE_SIGNATURES.GIF89a) || - matchesSignature(trimmedHead, FILE_SIGNATURES.BMP) || - matchesSignature(trimmedHead, FILE_SIGNATURES.ICO) || - (matchesSignature(trimmedHead, FILE_SIGNATURES.WEBP) && - trimmedHead.length >= 12 && - matchesSignature(trimmedHead.slice(8), FILE_SIGNATURES.WEBP_SIGNATURE)) || + const isValidImage = + matchesSignature(head, FILE_SIGNATURES.PNG) || + matchesSignature(head, FILE_SIGNATURES.JPEG_FF_D8_FF) || + matchesSignature(head, FILE_SIGNATURES.GIF87a) || + matchesSignature(head, FILE_SIGNATURES.GIF89a) || + matchesSignature(head, FILE_SIGNATURES.BMP) || + matchesSignature(head, FILE_SIGNATURES.ICO) || + (matchesSignature(head, FILE_SIGNATURES.WEBP) && + head.length >= 12 && + matchesSignature(head.slice(8), FILE_SIGNATURES.WEBP_SIGNATURE)) || (() => { const fullText = new TextDecoder("utf-8", { fatal: false }).decode(strippedData); const svgTrimmedHead = createTrimmedTextHead(fullText);
🧹 Nitpick comments (2)
packages/ui/components/image-uploader/imageValidation.ts (2)
119-123: Consider mapping unexpected errors to telemetry.
Optional: surface a breadcrumb/metric (without toasts) to observe validation failures in the wild.
52-123: Add tests for header strictness and text-substring checks.
Please add cases: (1) PNG/GIF/WebP with leading whitespace → invalid; (2) HTML/JS with comments before tags → blocked; (3) SVG with/without dangerous content; (4) WebP RIFF/WEBP offset at 8.I can draft test vectors if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/ui/components/image-uploader/imageValidation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/ui/components/image-uploader/imageValidation.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/image-uploader/imageValidation.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/ui/components/image-uploader/imageValidation.ts
🧬 Code graph analysis (1)
packages/ui/components/image-uploader/imageValidation.ts (1)
packages/lib/imageValidationConstants.ts (3)
matchesSignature(32-35)FILE_SIGNATURES(9-27)containsDangerousSVGContent(54-56)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
packages/ui/components/image-uploader/imageValidation.ts (5)
1-5: Imports look good; shared constants + helpers correctly referenced.
7-7: Good default; still overridable via param.
Exported MAX size with overridable param keeps callers flexible.
9-26: Robust BOM/whitespace trimming.
Covers UTF‑8 BOM and ASCII whitespace (incl. VT/FF). Zero-copy could use subarray, but fine as-is.
52-58: Confirm i18n key name.
“image_size_limit_exceed” reads grammatically off; verify it matches locale bundle keys (and usages) before shipping.
103-114: SVG branch is solid.
Signature + dangerous-content scan on full text is appropriate.
volnei
left a comment
volnei
left a comment
There was a problem hiding this comment.
I love code, really... but I wondering if we can use a library like this one. https://www.npmjs.com/package/image-validator
What do you think?
|
|
||
| const validation = validateBase64Image(data); | ||
| if (!validation.isValid) { | ||
| console.error(`Invalid image data found in database for objectKey ${objectKey}: ${validation.error}`); |
There was a problem hiding this comment.
Can you remove this console.log?
| "pdf_files_cannot_be_uploaded_as_images": "PDF files cannot be uploaded as images", | ||
| "only_image_files_allowed": "Only image files are allowed", | ||
| "html_files_cannot_be_uploaded_as_images": "HTML files cannot be uploaded as images", | ||
| "script_files_cannot_be_uploaded_as_images": "Script files cannot be uploaded as images", | ||
| "zip_files_cannot_be_uploaded_as_images": "ZIP files cannot be uploaded as images", | ||
| "executable_files_cannot_be_uploaded_as_images": "Executable files cannot be uploaded as images", | ||
| "failed_to_validate_image_file": "Failed to validate image file", | ||
| "invalid_image_file_format": "Invalid image file format", |
There was a problem hiding this comment.
Can we use something more generic like: "unsuported_files": {{type}} can be uploaded as image?
The reason is avoiding to have multiple "same" label and need to include more when we have new types being managed.
Pull request was converted to draft
- Add generic 'unsupported_file_type' translation key with {{type}} interpolation
- Replace hardcoded file type error messages with reusable i18n pattern
- Update imageValidation interfaces to support errorKey and errorParams
- Refactor server-side and client-side validation to use new pattern
- Remove individual translation keys for each file type (PDF, HTML, Script, ZIP, Executable)
- Add new translation keys for other validation errors (SVG, base64, empty data, etc.)
- Type-safe error handling with proper interpolation support
Benefits:
- Single reusable translation pattern reduces duplication
- Easier to maintain and localize
- Consistent error messaging across file types
- Add MAX_BANNER_SIZE constant (5MB) to shared constants file - Update BannerUploader to use shared constant instead of inline value - Update ImageUploader to handle new errorKey/errorParams pattern - Maintain backward compatibility with existing error handling Note: Pre-existing linting warnings in constants.ts and BannerUploader.tsx are unrelated to these changes and were present before this refactor.
- Update all dangerous file type tests to expect errorKey and errorParams instead of hardcoded error messages - Add comprehensive tests for new error format validation pattern - Add tests for backward compatibility with error field - Add tests for MAX_BANNER_SIZE constant integration - Verify that unsupported file types now return generic 'unsupported_file_type' key with type interpolation - Ensure all existing functionality continues to work with enhanced error handling All 42 tests passing ✅
Devanshusharma2005
force-pushed
the
fix/image-upload-validation-fix
branch
from
d206e97 to
b223904
Compare
- Update all dangerous file type tests to expect errorKey and errorParams - Change hardcoded error messages to i18n keys (unsupported_file_type, svg_contains_dangerous_content, etc.) - Update edge case tests to use new error keys (empty_image_data, invalid_base64_format, unrecognized_image_format) - Add comprehensive tests for new error format validation pattern - Add tests for backward compatibility with error field - Verify that unsupported file types return generic 'unsupported_file_type' key with type interpolation All 26 server-side tests passing ✅ Complements client-side test updates for complete test coverage
dosubot
bot
added
the
🧹 Improvements
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/api/avatar/[uuid]/route.ts (2)
56-68: Don’t mutate state in a GET handler (DB write on GET)Converting SVG to PNG and persisting it via prisma.avatar.update in a GET breaks HTTP semantics, complicates caching, and can introduce race conditions under load.
Apply this diff to avoid writes here; do conversion on-the-fly and persist during upload/update flows instead:
// Convert SVG to PNG if needed and update the database if (data.startsWith("data:image/svg+xml;base64,")) { const pngData = await convertSvgToPng(data); - await prisma.avatar.update({ - where: { objectKey }, - data: { data: pngData }, - }); img = pngData; } else { img = data; }If you want deduped storage, move the persistence to the place that first stores the avatar (e.g., upload/update handler) or a background job.
74-89: Incorrect decoding and content-type; rejects valid non-PNG/JPEG imagesOnly stripping PNG/JPEG prefixes means valid GIF/WEBP/BMP/ICO (allowed by validation) will fail decoding and 302 to fallback. Also, Content-Type is always set to image/png even for JPEG, etc.
Apply this diff to robustly parse the data URL, derive content type, and decode correctly:
- const decoded = img.toString().replace("data:image/png;base64,", "").replace("data:image/jpeg;base64,", ""); - const imageResp = Buffer.from(decoded, "base64"); + const match = /^data:(image\/[a-zA-Z0-9.+-]+);base64,(.*)$/i.exec(String(img)); + if (!match) { + const url = new URL(AVATAR_FALLBACK, WEBAPP_URL).toString(); + return NextResponse.redirect(url, 302); + } + const contentType = match[1].toLowerCase(); + const imageResp = Buffer.from(match[2], "base64"); @@ - "Content-Type": "image/png", + "Content-Type": contentType, "Content-Length": imageResp.length.toString(), "Cache-Control": "max-age=86400",
♻️ Duplicate comments (3)
packages/ui/components/image-uploader/BannerUploader.tsx (1)
16-16: Duplication resolved via shared validatorSwitching to validateImageFile from the shared utility addresses earlier duplication concerns.
packages/ui/components/image-uploader/ImageUploader.tsx (1)
16-16: Shared validator import LGTMThis keeps the avatar path consistent with banner validation rules.
packages/ui/components/image-uploader/imageValidation.ts (1)
82-99: Split raw head vs trimmed head; use raw head for binary signatures.Binary magic numbers (PDF/ZIP/EXE) should be checked on the untrimmed head; keep trimmed head for text-only checks. This aligns with earlier feedback.
Apply this diff:
- const trimmedHead = strippedData.slice(0, 512); + const HEADER_BYTES = 4096; // small, fixed cap to avoid scanning entire file + const head = uint8Array.subarray(0, HEADER_BYTES); + const trimmedHead = strippedData.subarray(0, HEADER_BYTES); @@ - if (matchesSignature(trimmedHead, FILE_SIGNATURES.PDF)) { + if (matchesSignature(head, FILE_SIGNATURES.PDF)) { return { isValid: false, errorKey: "unsupported_file_type", errorParams: { type: "PDF" } }; } @@ - if (matchesSignature(trimmedHead, FILE_SIGNATURES.ZIP)) { + if (matchesSignature(head, FILE_SIGNATURES.ZIP)) { return { isValid: false, errorKey: "unsupported_file_type", errorParams: { type: "ZIP" } }; } @@ - if (matchesSignature(trimmedHead, FILE_SIGNATURES.EXECUTABLE)) { + if (matchesSignature(head, FILE_SIGNATURES.EXECUTABLE)) { return { isValid: false, errorKey: "unsupported_file_type", errorParams: { type: "Executable" } }; }
🧹 Nitpick comments (14)
packages/lib/constants.ts (1)
71-73: Good extraction; confirm naming vs usage scopeConstantizing the 5MB limit addresses the earlier “make it a constant” nit. Given it’s also used by avatar/image uploads, consider a more generic name (e.g., MAX_IMAGE_FILE_SIZE) or an alias to avoid “banner”-specific semantics creeping into non-banner flows.
apps/web/app/api/avatar/[uuid]/route.ts (2)
50-55: Fail-closed redirect on invalid data is fine; consider minimal loggingRedirecting to AVATAR_FALLBACK on invalid images is safe. Optionally add structured logging (rate-limited) for ops visibility without leaking PII.
82-87: Security headers on binary image responses have limited effectX-Frame-Options and CSP are ignored by browsers for non-HTML resources. They don’t hurt, but consider trimming to essentials and adding CORP if needed.
Proposed minimal set:
- X-Content-Type-Options: nosniff
- Cache-Control: max-age=86400
- Cross-Origin-Resource-Policy: same-site (or same-origin), depending on embedding needs
packages/ui/components/image-uploader/BannerUploader.tsx (2)
97-110: Validation flow looks solid; unify messaging fields (optional)Logic is correct. For consistency, consider returning only errorKey (+ params) from the validator and dropping plain error to simplify callers.
187-187: Accept list alignment (minor)The explicit MIME list is good. Optionally add image/vnd.microsoft.icon alongside image/x-icon for broader ICO picker compatibility.
packages/ui/components/image-uploader/ImageUploader.tsx (3)
7-7: Minor: naming driftUsing MAX_BANNER_SIZE for avatars works but reads oddly. Either rely on the validator’s default max (omit the arg) or import a generic MAX_IMAGE_FILE_SIZE alias.
93-113: Validation + UX handling look goodAsync validation, toast, and input reset are correct and user-friendly. See note above on using the validator’s default max to avoid repeating the size limit here.
175-175: Accept list alignment (minor)Same as BannerUploader: consider also image/vnd.microsoft.icon; image/jpg is redundant with image/jpeg (harmless).
packages/lib/server/imageValidation.ts (1)
12-18: Consider narrowing detectedFormat to a union typeType it as "PNG" | "JPEG" | "GIF" | "BMP" | "ICO" | "WEBP" | "SVG" | "PDF" | "HTML" | "Script" | "ZIP" | "Executable" | "Unknown" for better exhaustiveness in callers.
packages/ui/components/image-uploader/imageValidation.ts (5)
10-27: Handle UTF‑16/UTF‑32 BOM to avoid false negatives for HTML/SVG.
stripBomAndWhitespaceonly recognizes UTF‑8 BOM. UTF‑16/UTF‑32 HTML/SVG (rare but possible) will decode poorly in later checks and be misclassified as generic “invalid format.” Prefer decoding text with BOM-aware logic for text-based detection paths.Apply this within-range diff to keep the zero-copy behavior:
- return data.slice(start); + return data.subarray(start);And add a BOM-aware decoder (outside this range) and use it where you decode:
// Place near top of file function decodeTextWithBom(bytes: Uint8Array): string { // UTF-32 BE/LE if (bytes.length >= 4) { if (bytes[0] === 0x00 && bytes[1] === 0x00 && bytes[2] === 0xfe && bytes[3] === 0xff) return new TextDecoder("utf-32be", { fatal: false }).decode(bytes.subarray(4)); if (bytes[0] === 0xff && bytes[1] === 0xfe && bytes[2] === 0x00 && bytes[3] === 0x00) return new TextDecoder("utf-32le", { fatal: false }).decode(bytes.subarray(4)); } // UTF-16 BE/LE if (bytes.length >= 2) { if (bytes[0] === 0xfe && bytes[1] === 0xff) return new TextDecoder("utf-16be", { fatal: false }).decode(bytes.subarray(2)); if (bytes[0] === 0xff && bytes[1] === 0xfe) return new TextDecoder("utf-16le", { fatal: false }).decode(bytes.subarray(2)); } // Fallback to UTF-8 (with/without BOM) const off = bytes.length >= 3 && bytes[0] === 0xef && bytes[1] === 0xbb && bytes[2] === 0xbf ? 3 : 0; return new TextDecoder("utf-8", { fatal: false }).decode(bytes.subarray(off)); }Then replace TextDecoder usages accordingly (see comments below for exact diffs).
29-43: Use BOM-aware decoder and consider scanning a slightly larger text head.
- Decode with
decodeTextWithBomto handle UTF‑16/32 text correctly.- 512 bytes is fine, but bumping to 2–4 KiB reduces start-only false negatives with negligible cost for 5 MB max files.
Apply this diff:
- const text = new TextDecoder("utf-8", { fatal: false }).decode(data).toLowerCase(); + const text = decodeTextWithBom(data).toLowerCase();
101-121: Minor perf nits: favor subarray, minimize encoders/decoders, and gate SVG decode.
- Use
subarrayto avoid copies.- Instantiate decoders once or reuse helper.
- Decode full text for SVG only if the header looks texty.
Apply this diff:
- (matchesSignature(trimmedHead, FILE_SIGNATURES.WEBP) && + (matchesSignature(trimmedHead, FILE_SIGNATURES.WEBP) && trimmedHead.length >= 12 && matchesSignature(trimmedHead.slice(8), FILE_SIGNATURES.WEBP_SIGNATURE)) || (() => { - const fullText = new TextDecoder("utf-8", { fatal: false }).decode(strippedData); + // Quick precheck: if head starts with '<' (after trimming), do the text path + if (!(trimmedHead.length > 0 && trimmedHead[0] === 0x3c)) return false; + const fullText = decodeTextWithBom(strippedData); - const svgTrimmedHead = createTrimmedTextHead(fullText); + const svgTrimmedHead = createTrimmedTextHead(fullText);
65-66: Return structured i18n errors and include the limit.Keep API consistent: prefer
errorKey/errorParamseverywhere; includemaxBytesso UIs can render MB/size.Apply this diff:
- return { isValid: false, error: "image_size_limit_exceed" }; + return { + isValid: false, + errorKey: "image_size_limit_exceed", + errorParams: { maxBytes: maxFileSize } + };Please verify downstream code paths (components showing toasts) handle
errorKeyuniformly.
82-87: Optional: scan a slightly larger text head.512 bytes can miss
<html>/<script>after long comments. 2–4 KiB keeps it fast and safer. You already cap file size at 5 MB.(Adopted in the earlier HEAD diff via
HEADER_BYTES = 4096.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/web/app/api/avatar/[uuid]/route.ts(3 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/lib/constants.ts(1 hunks)packages/lib/server/imageValidation.test.ts(1 hunks)packages/lib/server/imageValidation.ts(1 hunks)packages/ui/components/image-uploader/BannerUploader.tsx(4 hunks)packages/ui/components/image-uploader/ImageUploader.tsx(4 hunks)packages/ui/components/image-uploader/imageValidation.test.ts(1 hunks)packages/ui/components/image-uploader/imageValidation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/public/static/locales/en/common.json
- packages/lib/server/imageValidation.test.ts
- packages/ui/components/image-uploader/imageValidation.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/constants.tsapps/web/app/api/avatar/[uuid]/route.tspackages/lib/server/imageValidation.tspackages/ui/components/image-uploader/imageValidation.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/constants.tspackages/ui/components/image-uploader/BannerUploader.tsxpackages/ui/components/image-uploader/ImageUploader.tsxapps/web/app/api/avatar/[uuid]/route.tspackages/lib/server/imageValidation.tspackages/ui/components/image-uploader/imageValidation.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/constants.tspackages/ui/components/image-uploader/BannerUploader.tsxpackages/ui/components/image-uploader/ImageUploader.tsxapps/web/app/api/avatar/[uuid]/route.tspackages/lib/server/imageValidation.tspackages/ui/components/image-uploader/imageValidation.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/ui/components/image-uploader/BannerUploader.tsxpackages/ui/components/image-uploader/ImageUploader.tsx
🧬 Code graph analysis (5)
packages/ui/components/image-uploader/BannerUploader.tsx (2)
packages/ui/components/image-uploader/imageValidation.ts (1)
validateImageFile(55-131)packages/lib/constants.ts (1)
MAX_BANNER_SIZE(72-72)
packages/ui/components/image-uploader/ImageUploader.tsx (2)
packages/ui/components/image-uploader/imageValidation.ts (1)
validateImageFile(55-131)packages/lib/constants.ts (1)
MAX_BANNER_SIZE(72-72)
apps/web/app/api/avatar/[uuid]/route.ts (2)
packages/lib/server/imageValidation.ts (1)
validateBase64Image(23-132)packages/lib/constants.ts (2)
AVATAR_FALLBACK(95-95)WEBAPP_URL(12-18)
packages/lib/server/imageValidation.ts (1)
packages/lib/imageValidationConstants.ts (4)
isValidBase64(69-71)matchesSignature(32-35)FILE_SIGNATURES(9-27)containsDangerousSVGContent(54-56)
packages/ui/components/image-uploader/imageValidation.ts (2)
packages/lib/constants.ts (1)
MAX_BANNER_SIZE(72-72)packages/lib/imageValidationConstants.ts (3)
matchesSignature(32-35)FILE_SIGNATURES(9-27)containsDangerousSVGContent(54-56)
🔇 Additional comments (4)
apps/web/app/api/avatar/[uuid]/route.ts (1)
9-9: Server-side validation import is appropriateBringing validateBase64Image into the route is the right move to guard against picker bypasses and content-type spoofing.
packages/ui/components/image-uploader/BannerUploader.tsx (1)
7-7: Good: shared constant for size limitImporting MAX_BANNER_SIZE aligns with prior feedback to avoid hardcoding.
packages/lib/server/imageValidation.ts (1)
38-84: Rule set balance looks goodBlocking PDF/HTML/Script/ZIP/Executable and allowing common image types is appropriate. SVG danger checks are a necessary guard.
packages/ui/components/image-uploader/imageValidation.ts (1)
55-131: Solid layered validation; approach LGTM.Good short-circuit order, BOM/whitespace trimming, and SVG safety check gated behind binary signature failures. Nice.
|
@volnei ^^ |
Did I just build a validation library in our codebase from scratch. Haha 😂. |
7 participants
What does this PR do?
Enhancing Image upload validation.
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Screen.Recording.2025-07-28.170817.mp4
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist