feat: enhance image upload validation across the application #22766
Conversation
|
@Devanshusharma2005 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis change introduces comprehensive image validation across both server-side and client-side code to prevent uploading or serving malicious files disguised as images. A new shared module defines file signature (magic number) constants and utility functions for detecting dangerous file types and validating allowed image formats. Server-side, a Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (1)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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. |
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.
packages/trpc/server/routers/viewer/me/updateProfile.handler.ts
Outdated
Show resolved
Hide resolved
…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
There was a problem hiding this comment.
Would be great if you can check if something like this ensures that images files are still not selectable regardless of switching between All Files <-> Images Files
If there's no way to prevent users from selecting certain file extensions, then we will have to fully rely on the manual validation logic.
On Windows, the file picker always includes an “All Files” option, so users can still manually choose a PDF or any unsupported format, even if we specify accept="image/*". So yeah, it seems like we’ll have to rely on manual validation as the real line of defense here. Maybe I’m missing something though if you find a way that fully enforces it, definitely lmk. |
Yeah I'm aware of it. There's this same behavior on Mac. That's why I suggested explicitly using the file extensions and testing. Lmk if it works! |
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