fix(scheduled): enforce XSS sanitization & Cloudinary validation for scheduled messages#584
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR closes a stored-XSS vulnerability and attachment-validation gap in scheduled messages. A new ChangesScheduled Message Sanitization and Attachment Validation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/controllers/scheduledMessage.controller.js (1)
190-194:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSecurity gap:
updateScheduledMessagebypasses XSS sanitization and attachment validation.The PR adds sanitization and validation to
scheduleMessage, butupdateScheduledMessagedirectly assignsmessage,image, andaudiowithout:
- Sanitizing
messagewithxss()- Validating
image/audioattachments- Uploading validated attachments to Cloudinary
An attacker can schedule a benign message, then PATCH it to inject malicious XSS or oversized/invalid base64. While the scheduler re-sanitizes at delivery time, the stored
ScheduledMessageand API responses (getScheduledMessage,getScheduledMessages) will return unsanitized content that clients may render in previews.🔒 Proposed fix
// Update fields if provided - if (message !== undefined) scheduledMessage.message = message; - if (image !== undefined) scheduledMessage.image = image; - if (audio !== undefined) scheduledMessage.audio = audio; + if (message !== undefined) scheduledMessage.message = xss(message.trim()); + + if (image !== undefined) { + if (image) { + const validation = validateImageAttachment(image); + if (!validation.isValid) { + return res.status(400).json({ message: validation.error }); + } + const result = await cloudinary.uploader.upload(image); + scheduledMessage.image = result.secure_url; + } else { + scheduledMessage.image = ""; + } + } + + if (audio !== undefined) { + if (audio) { + const validation = validateAudioAttachment(audio); + if (!validation.isValid) { + return res.status(400).json({ message: validation.error }); + } + const result = await cloudinary.uploader.upload(audio, { resource_type: "auto" }); + scheduledMessage.audio = result.secure_url; + } else { + scheduledMessage.audio = ""; + } + } + if (replyTo !== undefined) scheduledMessage.replyTo = replyTo;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/controllers/scheduledMessage.controller.js` around lines 190 - 194, The updateScheduledMessage handler currently assigns message, image, and audio directly and must be changed to reuse the same sanitization/validation/upload flow used by scheduleMessage: when message is provided run it through xss() before assigning to scheduledMessage.message; when image or audio are provided validate them using the same attachment validation function (the validator used by scheduleMessage), reject the update on invalid/oversized/base64 data, and for valid attachments upload them to Cloudinary using the same upload helper so scheduledMessage.image/audio store the uploaded URL/metadata (not raw base64); ensure any helper returns used in scheduleMessage are invoked here so stored and returned ScheduledMessage data remain sanitized and validated.
🧹 Nitpick comments (1)
backend/src/lib/messageScheduler.js (1)
85-89: 💤 Low valueConsider using
sanitizedMessagein push notification payload for consistency.The notification body uses
scheduledMsg.message(the raw DB value) instead ofsanitizedMessage. While push notifications aren't rendered as HTML, using the sanitized version maintains consistency and prevents any edge cases if the notification is later displayed in an HTML context.const notificationPayload = { title: "New message", - body: scheduledMsg.message || "(Message with attachment)", + body: sanitizedMessage || "(Message with attachment)", icon: "/icon.png", };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/lib/messageScheduler.js` around lines 85 - 89, The push notification payload currently sets body using scheduledMsg.message; update the notificationPayload construction to use the already-computed sanitizedMessage instead (replace scheduledMsg.message with sanitizedMessage) so the notification body is consistent with sanitized output; locate this in messageScheduler.js where notificationPayload is created and change the body field and any related comment to reference sanitizedMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/attachmentValidator.js`:
- Around line 8-12: The validators call .match() on their input and will throw
if base64Str is null/undefined or not a string; update validateImageAttachment
and validateAudioAttachment to defensively check that the input is a non-empty
string (e.g. if (typeof base64Str !== 'string' || !base64Str) return { isValid:
false, error: "Invalid file format structure or corrupt payload." }; ) before
performing the regex match so they return the same error object instead of
throwing a TypeError.
In `@backend/src/lib/messageScheduler.js`:
- Around line 27-29: The image/audio validation early-returns leave
scheduledMsg.status as "pending", causing permanent retries; update both
validation failure paths (the image validation branch around the check using
validation.isValid and the audio validation branch noted in the comment) to set
scheduledMsg.status = "failed" (or "cancelled" if your model uses that), set a
failureReason (e.g., validation.error or validation.error.message), and persist
the change (e.g., await scheduledMsg.save() or ScheduledMessage.updateOne({_id:
scheduledMsg._id}, {status: "failed", failureReason: ...})). Ensure both
branches mirror each other so failed validations are excluded from future
runScheduler runs. If the ScheduledMessage schema lacks status="failed" or
failureReason, update the schema accordingly.
---
Outside diff comments:
In `@backend/src/controllers/scheduledMessage.controller.js`:
- Around line 190-194: The updateScheduledMessage handler currently assigns
message, image, and audio directly and must be changed to reuse the same
sanitization/validation/upload flow used by scheduleMessage: when message is
provided run it through xss() before assigning to scheduledMessage.message; when
image or audio are provided validate them using the same attachment validation
function (the validator used by scheduleMessage), reject the update on
invalid/oversized/base64 data, and for valid attachments upload them to
Cloudinary using the same upload helper so scheduledMessage.image/audio store
the uploaded URL/metadata (not raw base64); ensure any helper returns used in
scheduleMessage are invoked here so stored and returned ScheduledMessage data
remain sanitized and validated.
---
Nitpick comments:
In `@backend/src/lib/messageScheduler.js`:
- Around line 85-89: The push notification payload currently sets body using
scheduledMsg.message; update the notificationPayload construction to use the
already-computed sanitizedMessage instead (replace scheduledMsg.message with
sanitizedMessage) so the notification body is consistent with sanitized output;
locate this in messageScheduler.js where notificationPayload is created and
change the body field and any related comment to reference sanitizedMessage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02ef6a5b-3e3e-47d7-968e-b24f2ef328bf
📒 Files selected for processing (3)
backend/src/controllers/scheduledMessage.controller.jsbackend/src/lib/attachmentValidator.jsbackend/src/lib/messageScheduler.js
1 participant
This PR resolves #563.
Scheduled messages were previously stored without XSS sanitization and without Cloudinary validation for image/audio attachments. The changes add XSS sanitisation, enforce attachment MIME and size validation, upload attachments to Cloudinary, and refactor validation logic into a shared module. This brings the scheduled-message flow in line with the live-message security standards.
Summary by CodeRabbit
New Features
Bug Fixes