fix(scheduled): enforce XSS sanitization & Cloudinary validation for scheduled messages#585
Open
harrshita123 wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR closes a security gap where scheduled messages bypassed XSS sanitization, attachment MIME/size validation, and Cloudinary upload. A new ChangesScheduled Message Security: XSS Sanitization & Cloudinary Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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.
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)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStatus filter excludes the new
"failed"status.The schema now includes
"failed"as a valid status (added inscheduledMessage.model.jsline 11), but this filter array doesn't include it. Users cannot query their failed scheduled messages.🐛 Proposed fix
- if (status && ["pending", "sent", "cancelled"].includes(status)) { + if (status && ["pending", "sent", "cancelled", "failed"].includes(status)) { query.status = status; }🤖 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 99 - 101, The status filter array in the if statement (which checks if the status is in ["pending", "sent", "cancelled"]) does not include the new "failed" status that was added to the schema in scheduledMessage.model.js. Add "failed" to this status array so that users can query their failed scheduled messages. Update the array to include all four valid statuses: "pending", "sent", "cancelled", and "failed".
🧹 Nitpick comments (3)
backend/src/lib/attachmentValidator.js (2)
8-27: 💤 Low valueError message hardcodes size limit despite configurable parameter.
Line 24 hardcodes "5MB" but
maxSizeBytesis a parameter. If a caller passes a different limit, the error message will be misleading.💡 Suggested fix
- if (binarySizeEstimate > maxSizeBytes) { - return { isValid: false, error: "File boundary limit exceeded. Image size must be under 5MB." }; - } + if (binarySizeEstimate > maxSizeBytes) { + const maxMB = (maxSizeBytes / (1024 * 1024)).toFixed(0); + return { isValid: false, error: `File boundary limit exceeded. Image size must be under ${maxMB}MB.` }; + }🤖 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/attachmentValidator.js` around lines 8 - 27, The error message in the validateImageAttachment function hardcodes "5MB" as the file size limit, but the function accepts a configurable maxSizeBytes parameter. When the file size check fails, calculate the actual limit in megabytes from the maxSizeBytes parameter and include that dynamic value in the error message instead of hardcoding "5MB", so the error message accurately reflects the actual limit enforced for each function call.
29-54: 💤 Low valueSame hardcoded limit issue for audio validation.
Line 51 hardcodes "10MB" but
maxSizeBytesis configurable. Apply the same fix as for image validation.🤖 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/attachmentValidator.js` around lines 29 - 54, The validateAudioAttachment function has a hardcoded "10MB" string in its error message when the audio size check fails, but the maxSizeBytes parameter is configurable. Replace the hardcoded "10MB" limit text in the error message with a dynamic value calculated from the maxSizeBytes parameter by converting bytes to megabytes, so the error message accurately reflects whatever size limit is actually being enforced.backend/src/lib/messageScheduler.js (1)
91-106: ⚖️ Poor tradeoffUnhandled errors leave message in
pendingstate indefinitely.The outer catch (lines 104-106) logs the error but doesn't update the message status. While this allows retries for transient failures (e.g., network issues), permanent failures (e.g., Cloudinary credential issues) would cause infinite retry loops every 30 seconds.
Consider adding retry limits or marking as failed after repeated attempts.
🤖 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 91 - 106, The outer catch block logs errors from processing the scheduled message but doesn't update the message status, causing permanent failures to retry indefinitely every 30 seconds. Add retry limit logic in the outer catch block: track the number of retry attempts (consider adding a retryCount field to scheduledMsg), and after a maximum number of retries (e.g., 5 attempts), update the scheduledMsg status to "failed" using the same database update pattern used elsewhere in the code, rather than leaving it in "pending" state indefinitely. For transient failures within the retry limit, the message should continue to be retried on the next scheduler cycle.
🤖 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.
Outside diff comments:
In `@backend/src/controllers/scheduledMessage.controller.js`:
- Around line 99-101: The status filter array in the if statement (which checks
if the status is in ["pending", "sent", "cancelled"]) does not include the new
"failed" status that was added to the schema in scheduledMessage.model.js. Add
"failed" to this status array so that users can query their failed scheduled
messages. Update the array to include all four valid statuses: "pending",
"sent", "cancelled", and "failed".
---
Nitpick comments:
In `@backend/src/lib/attachmentValidator.js`:
- Around line 8-27: The error message in the validateImageAttachment function
hardcodes "5MB" as the file size limit, but the function accepts a configurable
maxSizeBytes parameter. When the file size check fails, calculate the actual
limit in megabytes from the maxSizeBytes parameter and include that dynamic
value in the error message instead of hardcoding "5MB", so the error message
accurately reflects the actual limit enforced for each function call.
- Around line 29-54: The validateAudioAttachment function has a hardcoded "10MB"
string in its error message when the audio size check fails, but the
maxSizeBytes parameter is configurable. Replace the hardcoded "10MB" limit text
in the error message with a dynamic value calculated from the maxSizeBytes
parameter by converting bytes to megabytes, so the error message accurately
reflects whatever size limit is actually being enforced.
In `@backend/src/lib/messageScheduler.js`:
- Around line 91-106: The outer catch block logs errors from processing the
scheduled message but doesn't update the message status, causing permanent
failures to retry indefinitely every 30 seconds. Add retry limit logic in the
outer catch block: track the number of retry attempts (consider adding a
retryCount field to scheduledMsg), and after a maximum number of retries (e.g.,
5 attempts), update the scheduledMsg status to "failed" using the same database
update pattern used elsewhere in the code, rather than leaving it in "pending"
state indefinitely. For transient failures within the retry limit, the message
should continue to be retried on the next scheduler cycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40bd8a44-6e74-4a40-bdf4-932f5e1be0c3
📒 Files selected for processing (4)
backend/src/controllers/scheduledMessage.controller.jsbackend/src/lib/attachmentValidator.jsbackend/src/lib/messageScheduler.jsbackend/src/models/scheduledMessage.model.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR replaces #584, which could not be reopened because the submitting fork repository was deleted and recreated.
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
Release Notes
New Features
Bug Fixes