fix: add XSS sanitization for chat messages#1742
Conversation
|
@jhosepm352-design is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR adds XSS protection to the TicketChat component by introducing a ChangesXSS Sanitization in TicketChat
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Frontend/src/components/shared/TicketChat.jsx (1)
108-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one sanitized payload for both optimistic state and persistence.
Right now only the optimistic message is sanitized (Line [189]), but DB inserts still use raw
content(Line [205], Line [216]). That breaks the dedupe check at Line [108] whenever sanitization changes text, so both temp and persisted copies can remain in the list. It also reintroduces unsanitized content from realtime payloads.Proposed fix
- const content = inputValue.trim(); + const content = inputValue.trim(); + const sanitizedContent = sanitizeMessage(content)?.trim(); + if (!sanitizedContent) return; const currentIsInternal = isInternal; @@ - message: sanitizeMessage(content), + message: sanitizedContent, @@ - content: content + content: sanitizedContent @@ - message: content + message: sanitizedContentAlso applies to: 179-190, 205-217
🤖 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 `@Frontend/src/components/shared/TicketChat.jsx` around lines 108 - 109, The dedupe bug comes from sanitizing only the optimistic message but still persisting/forwarding the raw content, so String(m.id).startsWith('temp-') check against newMessage.message can fail; fix by creating one sanitized payload (e.g., const sanitizedContent or sanitizedPayload) when composing the optimistic message (where newMessage is built) and use that same sanitized value everywhere you persist or emit to realtime/DB (replace raw content usages in the DB insert and realtime-send code paths that currently use content). Ensure filtered, newMessage, and the DB insert / realtime send call sites all reference the single sanitized variable so the in-memory dedupe and persisted record match.
🤖 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 `@Frontend/src/components/shared/TicketChat.jsx`:
- Around line 3-10: The regex-based sanitizeMessage function is insufficient for
XSS; replace it with a proven HTML sanitizer (e.g., DOMPurify or sanitize-html)
and stop ad-hoc regex stripping: update the sanitizeMessage implementation to
import and configure the chosen library (strictly allow only safe
tags/attributes, explicitly disallow event handlers, script/iframe/object, and
normalize case/multiline payloads) and apply the same sanitizer consistently at
both write and read paths; ensure sanitizeMessage (and any server-side
counterpart) uses that shared config, update imports/usages in TicketChat.jsx,
and add/adjust tests to validate common XSS payloads are rejected.
---
Outside diff comments:
In `@Frontend/src/components/shared/TicketChat.jsx`:
- Around line 108-109: The dedupe bug comes from sanitizing only the optimistic
message but still persisting/forwarding the raw content, so
String(m.id).startsWith('temp-') check against newMessage.message can fail; fix
by creating one sanitized payload (e.g., const sanitizedContent or
sanitizedPayload) when composing the optimistic message (where newMessage is
built) and use that same sanitized value everywhere you persist or emit to
realtime/DB (replace raw content usages in the DB insert and realtime-send code
paths that currently use content). Ensure filtered, newMessage, and the DB
insert / realtime send call sites all reference the single sanitized variable so
the in-memory dedupe and persisted record match.
🪄 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: 6b1c0ea9-7e8c-45b0-b7c8-c834cfadbf57
📒 Files selected for processing (1)
Frontend/src/components/shared/TicketChat.jsx
| // Basic XSS sanitization: removes script tags and inline event handlers | ||
| const sanitizeMessage = (text) => { | ||
| if (!text) return text; | ||
| return text | ||
| .replace(/<script.*?>.*?<\/script>/gi, '') | ||
| .replace(/ on\w+="[^"]*"/g, '') | ||
| .replace(/ on\w+='[^']*'/g, ''); | ||
| }; |
There was a problem hiding this comment.
Regex-based sanitizer is too weak to be treated as an XSS boundary.
The current patterns miss common variants (e.g., unquoted on*= handlers, case variants, multiline script payloads), so malicious markup can pass through unchanged. Please replace this with a robust sanitizer strategy (and keep it consistent across write/read paths), rather than relying on ad-hoc regex stripping.
🤖 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 `@Frontend/src/components/shared/TicketChat.jsx` around lines 3 - 10, The
regex-based sanitizeMessage function is insufficient for XSS; replace it with a
proven HTML sanitizer (e.g., DOMPurify or sanitize-html) and stop ad-hoc regex
stripping: update the sanitizeMessage implementation to import and configure the
chosen library (strictly allow only safe tags/attributes, explicitly disallow
event handlers, script/iframe/object, and normalize case/multiline payloads) and
apply the same sanitizer consistently at both write and read paths; ensure
sanitizeMessage (and any server-side counterpart) uses that shared config,
update imports/usages in TicketChat.jsx, and add/adjust tests to validate common
XSS payloads are rejected.
|
Hi @jhosepm352-design! 🙌 Thank you so much for your excellent contribution: "fix: add XSS sanitization for chat messages"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
ritesh-1918
added
gssoc
|
Hi @jhosepm352-design! 🙌 Thank you so much for your excellent contribution: "fix: add XSS sanitization for chat messages"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
|
Merged locally into gssoc branch. Thank you for your GSSoC contribution! 🚀🔥 |
2 participants
Added a defense-in-depth sanitization layer for chat messages to prevent potential XSS attacks via script tags or inline event handlers, addressing the 'XSS in AI chat markdown rendering' security concern.
Summary by CodeRabbit