fix: special characters not escaped in Omnichannel queue side panel message preview - #41595
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: db0d46b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe omnichannel queue side panel now HTML-escapes visitor names in message previews and includes a patch changeset for ChangesOmnichannel preview escaping
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41595 +/- ##
===========================================
- Coverage 68.75% 68.74% -0.01%
===========================================
Files 4150 4150
Lines 159416 159416
Branches 27997 27969 -28
===========================================
- Hits 109609 109596 -13
- Misses 44629 44650 +21
+ Partials 5178 5170 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx (1)
33-34: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for escaped visitor names.
Cover a name such as
<img src=x onerror=alert(1)>and verify the generated preview contains escaped HTML. This protects thedangerouslySetInnerHTMLboundary from future regressions.🤖 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 `@apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx` around lines 33 - 34, Add a regression test covering the preview construction in InquireSidePanelItem, using a visitor name such as <img src=x onerror=alert(1)> and asserting the generated preview contains its escaped HTML representation before dangerouslySetInnerHTML renders it.
🤖 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.
Nitpick comments:
In
`@apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx`:
- Around line 33-34: Add a regression test covering the preview construction in
InquireSidePanelItem, using a visitor name such as <img src=x onerror=alert(1)>
and asserting the generated preview contains its escaped HTML representation
before dangerouslySetInnerHTML renders it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f217a66-c130-4e60-9758-75ff91767f59
📒 Files selected for processing (2)
.changeset/shy-actors-jump.mdapps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx
🧠 Learnings (3)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/shy-actors-jump.md
🔇 Additional comments (2)
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx (1)
3-3: LGTM!.changeset/shy-actors-jump.md (1)
1-5: LGTM!
|
/patch |
|
Pull request #41817 added to Project: "Patch 8.7.1" |
|
/backport 8.6.2 |
|
Pull request #41825 added to Project: "Patch 8.4.6" |
|
/backport 8.3.8 |
|
Pull request #41826 added to Project: "Patch 8.3.8" |
|
/backport 8.2.8 |
|
Pull request #41840 added to Project: "Patch 8.2.8" |
|
/backport 8.1.8 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
|
/backport 8.1.8 |
|
Pull request #41841 added to Project: "Patch 8.1.8" |
|
/backport 7.10.15 |
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
|
/backport 7.10.15 |
|
Pull request #41852 added to Project: "Patch 7.10.15" |
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/VLN-531
Steps to test or reproduce
Further comments
Summary by CodeRabbit