fix: Separate image alt text from attachment description#40839
Open
dougfabris wants to merge 3 commits into
Open
fix: Separate image alt text from attachment description#40839dougfabris wants to merge 3 commits into
dougfabris wants to merge 3 commits into
Conversation
…y in file attachments
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughThis PR extends Rocket.Chat's attachment system to treat ChangesAttachment Description System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.4.16)apps/meteor/app/lib/server/functions/notifications/email.jsFile contains syntax errors that prevent linting: Line 1: Illegal use of an import declaration outside of a module; Line 2: Illegal use of an import declaration outside of a module; Line 4: Illegal use of an import declaration outside of a module; Line 5: Illegal use of an import declaration outside of a module; Line 6: Illegal use of an import declaration outside of a module; Line 7: Illegal use of an import declaration outside of a module; Line 8: Illegal use of an import declaration outside of a module; Line 9: Illegal use of an import declaration outside of a module; Line 10: Illegal use of an import declaration outside of a module; Line 11: Illegal use of an import declaration outside of a module; Line 24: Illegal use of an export declaration outside of a module; Line 136: Illegal use of an export declaration outside of a module; Line 189: Illegal use of an export declaration outside of a module; Line 196: Illegal use of an export declaration outside of a module 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 #40839 +/- ##
===========================================
- Coverage 70.10% 70.03% -0.08%
===========================================
Files 3337 3337
Lines 123506 123571 +65
Branches 22035 22023 -12
===========================================
- Hits 86584 86540 -44
- Misses 33585 33677 +92
- Partials 3337 3354 +17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
7 issues found across 51 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/livechat/server/lib/sendTranscript.ts">
<violation number="1" location="apps/meteor/app/livechat/server/lib/sendTranscript.ts:119">
P2: `messageContent` is unconditionally replaced with `attachments[0].description || ''`, which can erase the original message text/system content for attachment messages that have no description.</violation>
<violation number="2" location="apps/meteor/app/livechat/server/lib/sendTranscript.ts:120">
P1: Attachment descriptions are inserted into transcript HTML without escaping because `escapeHtml(messageContent)` is called but its return value is ignored, enabling HTML injection in transcript output.
(Based on your team's feedback about ensuring attachment.description is actually escaped/sanitized.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/client/hooks/useDecryptedMessage.ts">
<violation number="1" location="apps/meteor/client/hooks/useDecryptedMessage.ts:23">
P1: Attachment handling now overrides decrypted `msg` when both are present, causing incorrect message text to be shown. Make the attachments branch conditional on `msg` being absent (e.g., `else if`).
(Based on your team's feedback about async flow ordering and overwrite risks in this hook.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts">
<violation number="1" location="apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts:145">
P2: Email body can be unintentionally blank because the new logic dropped the `message.msg` fallback when attachments have no descriptions.</violation>
</file>
<file name="apps/meteor/app/lib/server/methods/updateMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/methods/updateMessage.ts:90">
P1: The new attachment rewrite path can erase existing attachment descriptions when `updateMessage` is called without `msg` (e.g., content-only edits).</violation>
</file>
<file name="apps/meteor/app/lib/server/functions/notifications/email.js">
<violation number="1" location="apps/meteor/app/lib/server/functions/notifications/email.js:83">
P2: Index-based pairing between filtered `files` and unfiltered `attachments` can attach the wrong description to a file.</violation>
</file>
<file name="apps/meteor/client/lib/chats/uploads.ts">
<violation number="1" location="apps/meteor/client/lib/chats/uploads.ts:77">
P2: Encrypted upload metadata is being written to `altText`, which is outside the `IUpload` contract (`description` is the supported field). This can cause encrypted alt text metadata to be ignored by contract-based consumers.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
Comment on lines
+120
to
+121
| escapeHtml(messageContent); | ||
|
|
Contributor
There was a problem hiding this comment.
P1: Attachment descriptions are inserted into transcript HTML without escaping because escapeHtml(messageContent) is called but its return value is ignored, enabling HTML injection in transcript output.
(Based on your team's feedback about ensuring attachment.description is actually escaped/sanitized.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/livechat/server/lib/sendTranscript.ts, line 120:
<comment>Attachment descriptions are inserted into transcript HTML without escaping because `escapeHtml(messageContent)` is called but its return value is ignored, enabling HTML injection in transcript output.
(Based on your team's feedback about ensuring attachment.description is actually escaped/sanitized.) .</comment>
<file context>
@@ -108,14 +108,17 @@ export async function sendTranscript({
if (message.attachments && message.attachments?.length > 0) {
+ messageContent = message.attachments[0].description || '';
+ escapeHtml(messageContent);
+
for await (const attachment of message.attachments) {
</file context>
Suggested change
| escapeHtml(messageContent); | |
| messageContent = escapeHtml(message.attachments[0].description || ''); |
|
|
||
| if (decryptedMsg.attachments && decryptedMsg.attachments.length > 0) { | ||
| setDecryptedMessage(t('Message_with_attachment')); | ||
| if (decryptedMsg.attachments && decryptedMsg.attachments?.length > 0) { |
Contributor
There was a problem hiding this comment.
P1: Attachment handling now overrides decrypted msg when both are present, causing incorrect message text to be shown. Make the attachments branch conditional on msg being absent (e.g., else if).
(Based on your team's feedback about async flow ordering and overwrite risks in this hook.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/hooks/useDecryptedMessage.ts, line 23:
<comment>Attachment handling now overrides decrypted `msg` when both are present, causing incorrect message text to be shown. Make the attachments branch conditional on `msg` being absent (e.g., `else if`).
(Based on your team's feedback about async flow ordering and overwrite risks in this hook.) .</comment>
<file context>
@@ -18,11 +18,14 @@ export const useDecryptedMessage = (message: IMessage): string => {
- if (decryptedMsg.attachments && decryptedMsg.attachments.length > 0) {
- setDecryptedMessage(t('Message_with_attachment'));
+ if (decryptedMsg.attachments && decryptedMsg.attachments?.length > 0) {
+ if (decryptedMsg.attachments[0].description) {
+ setDecryptedMessage(decryptedMsg.attachments[0].description);
</file context>
Suggested change
| if (decryptedMsg.attachments && decryptedMsg.attachments?.length > 0) { | |
| else if (decryptedMsg.attachments?.length > 0) { |
Comment on lines
+90
to
+94
| if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) { | ||
| originalMessage.attachments[0].description = message.msg; | ||
| message.attachments = originalMessage.attachments; | ||
| message.msg = originalMessage.msg; | ||
| } |
Contributor
There was a problem hiding this comment.
P1: The new attachment rewrite path can erase existing attachment descriptions when updateMessage is called without msg (e.g., content-only edits).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/methods/updateMessage.ts, line 90:
<comment>The new attachment rewrite path can erase existing attachment descriptions when `updateMessage` is called without `msg` (e.g., content-only edits).</comment>
<file context>
@@ -86,6 +86,13 @@ export async function executeUpdateMessage(
await canSendMessageAsync(message.rid, { uid: user._id, username: user.username ?? undefined, ...user });
+ // It is possible to have an empty array as the attachments property, so ensure both things exist
+ if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) {
+ originalMessage.attachments[0].description = message.msg;
+ message.attachments = originalMessage.attachments;
</file context>
Suggested change
| if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) { | |
| originalMessage.attachments[0].description = message.msg; | |
| message.attachments = originalMessage.attachments; | |
| message.msg = originalMessage.msg; | |
| } | |
| if ( | |
| typeof message.msg === 'string' && | |
| originalMessage.attachments && | |
| originalMessage.attachments.length > 0 && | |
| originalMessage.attachments[0].description !== undefined | |
| ) { | |
| originalMessage.attachments[0].description = message.msg; | |
| message.attachments = originalMessage.attachments; | |
| message.msg = originalMessage.msg; | |
| } |
Comment on lines
+119
to
+120
| messageContent = message.attachments[0].description || ''; | ||
| escapeHtml(messageContent); |
Contributor
There was a problem hiding this comment.
P2: messageContent is unconditionally replaced with attachments[0].description || '', which can erase the original message text/system content for attachment messages that have no description.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/livechat/server/lib/sendTranscript.ts, line 119:
<comment>`messageContent` is unconditionally replaced with `attachments[0].description || ''`, which can erase the original message text/system content for attachment messages that have no description.</comment>
<file context>
@@ -108,14 +108,17 @@ export async function sendTranscript({
let filesHTML = '';
if (message.attachments && message.attachments?.length > 0) {
+ messageContent = message.attachments[0].description || '';
+ escapeHtml(messageContent);
+
</file context>
Suggested change
| messageContent = message.attachments[0].description || ''; | |
| escapeHtml(messageContent); | |
| const attachmentDescription = message.attachments[0].description; | |
| if (attachmentDescription) { | |
| messageContent = escapeHtml(attachmentDescription); | |
| } |
Comment on lines
+145
to
+149
| const emailText = | ||
| message?.attachments | ||
| ?.map((a) => a.description) | ||
| .filter(Boolean) | ||
| .join('\n\n') || ''; |
Contributor
There was a problem hiding this comment.
P2: Email body can be unintentionally blank because the new logic dropped the message.msg fallback when attachments have no descriptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts, line 145:
<comment>Email body can be unintentionally blank because the new logic dropped the `message.msg` fallback when attachments have no descriptions.</comment>
<file context>
@@ -142,7 +142,11 @@ slashCommands.add({
}
- const emailText = message?.msg || '';
+ const emailText =
+ message?.attachments
+ ?.map((a) => a.description)
</file context>
Suggested change
| const emailText = | |
| message?.attachments | |
| ?.map((a) => a.description) | |
| .filter(Boolean) | |
| .join('\n\n') || ''; | |
| const emailText = | |
| message?.attachments | |
| ?.map((a) => a.description) | |
| .filter(Boolean) | |
| .join('\n\n') || message?.msg || ''; |
| const attachments = message.attachments || []; | ||
| const fileParts = files.map((file, index) => { | ||
| let part = escapeHTML(file.name); | ||
| if (attachments[index]?.description) { |
Contributor
There was a problem hiding this comment.
P2: Index-based pairing between filtered files and unfiltered attachments can attach the wrong description to a file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/functions/notifications/email.js, line 83:
<comment>Index-based pairing between filtered `files` and unfiltered `attachments` can attach the wrong description to a file.</comment>
<file context>
@@ -77,8 +77,13 @@ export async function getEmailContent({ message, user, room }) {
+ const attachments = message.attachments || [];
+ const fileParts = files.map((file, index) => {
+ let part = escapeHTML(file.name);
+ if (attachments[index]?.description) {
+ part += `<br/><br/>${escapeHTML(attachments[index].description)}`;
+ }
</file context>
| altText, | ||
| ...(isEncryptedUpload(upload) && { | ||
| metadataForEncryption: { ...upload.metadataForEncryption, description }, | ||
| metadataForEncryption: { ...upload.metadataForEncryption, altText }, |
Contributor
There was a problem hiding this comment.
P2: Encrypted upload metadata is being written to altText, which is outside the IUpload contract (description is the supported field). This can cause encrypted alt text metadata to be ignored by contract-based consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/uploads.ts, line 77:
<comment>Encrypted upload metadata is being written to `altText`, which is outside the `IUpload` contract (`description` is the supported field). This can cause encrypted alt text metadata to be ignored by contract-based consumers.</comment>
<file context>
@@ -72,9 +72,9 @@ class UploadsStore extends Emitter<{ update: void; [x: `cancelling-${Upload['id'
+ altText,
...(isEncryptedUpload(upload) && {
- metadataForEncryption: { ...upload.metadataForEncryption, description },
+ metadataForEncryption: { ...upload.metadataForEncryption, altText },
}),
};
</file context>
Suggested change
| metadataForEncryption: { ...upload.metadataForEncryption, altText }, | |
| metadataForEncryption: { ...upload.metadataForEncryption, description: altText }, |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts (1)
403-488: ⚡ Quick winAdd a regression test for file attachments containing both
textanddescription.Current additions validate translated
md, but they don’t pin thedescriptionMdsource when both fields are present. A focused case here would prevent the file-caption markdown source from regressing again.🤖 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/lib/parseMessageTextToAstMarkdown.spec.ts` around lines 403 - 488, Add a regression test that creates a file-type attachment object containing both text and description (optionally with translation fields) and asserts parseMessageAttachments(...) returns the description-based markdown in the descriptionMd field (not the text). Specifically, add a test case near the other attachment tests that calls parseMessageAttachments with an attachment where both text and description are set and then expect the returned [0].descriptionMd to strictly equal the parsed AST for the description value (use same Root shape as other tests); reference parseMessageAttachments and the descriptionMd property to locate where to assert.
🤖 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 `@apps/meteor/app/autotranslate/client/lib/autotranslate.ts`:
- Around line 72-80: The new description-translation branch can run after the
attachment.text branch and reuse attachment.translations[language] and
attachment.translations.original, causing both fields to be translated and the
original to be clobbered; update the logic so description and text are mutually
exclusive: if attachment.description exists prefer handling it (and skip text),
or if you choose to run text first then short-circuit and do not execute the
description branch when attachment.text was already processed; adjust checks
around attachment.text, attachment.description, attachment.translations,
language and autoTranslateShowInverse so only one of those branches mutates
attachment.translations.original or attachment.* per pass.
In `@apps/meteor/app/lib/server/methods/updateMessage.ts`:
- Around line 90-94: The update currently unconditionally sets
originalMessage.attachments[0].description = message.msg which can erase
existing captions when the update payload doesn't include text; change the logic
in updateMessage (use the originalMessage and message variables) so you only
overwrite attachments[0].description when message.msg is present (e.g., !==
undefined and not an empty string) in the incoming update; keep the assignment
of message.attachments and the swap of message.msg/message.msg restoration
scoped to that same guard so non-text edits don't clear existing descriptions.
In `@apps/meteor/app/livechat/server/lib/sendTranscript.ts`:
- Line 120: The call to escapeHtml currently discards its return value, leaving
messageContent unescaped and opening an XSS vector; update the sendTranscript
flow so the escaped string is used when building the HTML (e.g., assign the
result of escapeHtml(messageContent) back to messageContent or to a new variable
and use that variable in the template). Locate the escapeHtml invocation and the
HTML interpolation in the sendTranscript function (in sendTranscript.ts) and
replace the discarded call with an assignment so the sanitized value is what
gets inserted into the transcript HTML.
- Around line 119-120: The code currently overwrites messageContent with
message.attachments[0].description for any message; change it to only use the
attachment description when the message is not a system message so
already-formatted/sanitized system messages (the content produced earlier with
DOMPurify) are preserved. Concretely, check the message type/flag that
identifies system messages (e.g., a property like message.system or message.type
=== 'system') before assigning messageContent =
message.attachments[0].description, and keep the existing DOMPurify-sanitized
messageContent for system messages; still call escapeHtml(messageContent) after
deciding which content to use. Ensure you update the logic around
messageContent, message.attachments, and the escapeHtml call so system messages
are never overwritten by attachment descriptions.
In `@apps/meteor/app/slackbridge/server/RocketAdapter.ts`:
- Around line 206-216: The code can produce "undefined <fileName>" when
rocketMessage.msg and attachment.description are both missing; in RocketAdapter
(around getMessageAttachment and the slack.postMessage call) initialize text to
an empty string (e.g., text = rocketMessage.msg || '') and when setting from the
attachment only use attachment.description if present (e.g., if (!text &&
attachment.description) text = attachment.description), then build the msg
payload passed to slack.postMessage (slack.postMessage(..., { ...rocketMessage,
msg: `${text} ${fileName}` })) so it never interpolates undefined.
In `@apps/meteor/client/hooks/useDecryptedMessage.spec.ts`:
- Line 66: The final assertion in the useDecryptedMessage test is synchronous
and races the async decrypt flow; change the immediate
expect(result.current).toBe('Attachment description') to be wrapped in a waitFor
so the assertion waits for state updates from the hook (use the
testing-library's waitFor and assert inside it, e.g. waitFor(() =>
expect(result.current).toBe(...))). Update the test in
useDecryptedMessage.spec.ts where result.current is asserted to use waitFor to
make the check reliable.
In `@apps/meteor/client/lib/parseMessageTextToAstMarkdown.ts`:
- Around line 75-79: The current logic sets attachment.descriptionMd by calling
textToMessageToken(text, parseOptions) which may use attachment.text instead of
the actual attachment.description; update the branch inside the isFileAttachment
block (the conditional that sets attachment.descriptionMd) to call
textToMessageToken(attachment.description, parseOptions) (while preserving the
translated || isEncryptedMessageAttachment(attachment) condition and the
existing fallback to attachment.descriptionMd) so the markdown is parsed from
attachment.description itself.
---
Nitpick comments:
In `@apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts`:
- Around line 403-488: Add a regression test that creates a file-type attachment
object containing both text and description (optionally with translation fields)
and asserts parseMessageAttachments(...) returns the description-based markdown
in the descriptionMd field (not the text). Specifically, add a test case near
the other attachment tests that calls parseMessageAttachments with an attachment
where both text and description are set and then expect the returned
[0].descriptionMd to strictly equal the parsed AST for the description value
(use same Root shape as other tests); reference parseMessageAttachments and the
descriptionMd property to locate where to assert.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4083b201-8379-47d4-9651-8af8a44e63f9
📒 Files selected for processing (51)
apps/meteor/app/autotranslate/client/lib/autotranslate.tsapps/meteor/app/autotranslate/server/autotranslate.tsapps/meteor/app/autotranslate/server/deeplTranslate.tsapps/meteor/app/autotranslate/server/googleTranslate.tsapps/meteor/app/autotranslate/server/msTranslate.tsapps/meteor/app/file-upload/server/methods/sendFileMessage.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/lib/server/methods/updateMessage.tsapps/meteor/app/livechat/server/lib/sendTranscript.tsapps/meteor/app/slackbridge/server/RocketAdapter.tsapps/meteor/app/ui/client/lib/ChatMessages.tsapps/meteor/client/components/message/content/attachments/DefaultAttachment.tsxapps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsxapps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsxapps/meteor/client/components/message/toolbar/useCopyAction.tsapps/meteor/client/components/message/toolbar/useReportMessageAction.tsxapps/meteor/client/hooks/useDecryptedMessage.spec.tsapps/meteor/client/hooks/useDecryptedMessage.tsapps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/lib/chats/flows/processMessageUploads.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/client/lib/normalizeThreadMessage.tsxapps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.tsapps/meteor/client/lib/parseMessageTextToAstMarkdown.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.spec.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/client/views/room/MessageList/hooks/useMessageBody.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerGenericFile.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/useDownloadExportMutation.tsapps/meteor/client/views/room/contextualBar/ExportMessages/useExportMessagesAsPDFMutation.tsxapps/meteor/client/views/room/contextualBar/Threads/hooks/useNormalizedThreadTitleHtml.tsapps/meteor/client/views/room/modals/FileUploadModal/FilePreview.tsxapps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsxapps/meteor/client/views/room/modals/FileUploadModal/ImagePreview.tsxapps/meteor/client/views/room/modals/FileUploadModal/MediaPreview.tsxapps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.tsapps/meteor/server/services/messages/hooks/BeforeSaveMarkdownParser.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveMarkdownParser.tests.tsee/packages/federation-matrix/tests/end-to-end/messaging.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.tspackages/core-typings/src/IMessage/MessageAttachment/Files/ImageAttachmentProps.tspackages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentBase.tspackages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentDefault.ts
Comment on lines
+72
to
+80
| if (attachment.description && attachment.translations && attachment.translations[language]) { | ||
| attachment.translations.original = attachment.description; | ||
|
|
||
| if (autoTranslateShowInverse) { | ||
| attachment.description = attachment.translations.original; | ||
| } else { | ||
| attachment.description = attachment.translations[language]; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent dual-field translation overwrite when both text and description exist.
At Line 72, this new branch can run right after the existing attachment.text branch, and both reuse attachment.translations[language] + attachment.translations.original. For attachments containing both fields, you end up translating both to the same value and overwriting the saved original, which breaks inverse/original toggling.
💡 Suggested fix (make description/text translation mutually exclusive)
- if (attachment.text && attachment.translations && attachment.translations[language]) {
- attachment.translations.original = attachment.text;
-
- if (autoTranslateShowInverse) {
- attachment.text = attachment.translations.original;
- } else {
- attachment.text = attachment.translations[language];
- }
- }
-
- if (attachment.description && attachment.translations && attachment.translations[language]) {
+ if (attachment.description && attachment.translations && attachment.translations[language]) {
attachment.translations.original = attachment.description;
if (autoTranslateShowInverse) {
attachment.description = attachment.translations.original;
} else {
attachment.description = attachment.translations[language];
}
+ } else if (attachment.text && attachment.translations && attachment.translations[language]) {
+ attachment.translations.original = attachment.text;
+
+ if (autoTranslateShowInverse) {
+ attachment.text = attachment.translations.original;
+ } else {
+ attachment.text = attachment.translations[language];
+ }
}Based on learnings and provided context, attachment description is now the display-oriented path and should be preferred without clobbering other fields in the same pass.
🤖 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/app/autotranslate/client/lib/autotranslate.ts` around lines 72 -
80, The new description-translation branch can run after the attachment.text
branch and reuse attachment.translations[language] and
attachment.translations.original, causing both fields to be translated and the
original to be clobbered; update the logic so description and text are mutually
exclusive: if attachment.description exists prefer handling it (and skip text),
or if you choose to run text first then short-circuit and do not execute the
description branch when attachment.text was already processed; adjust checks
around attachment.text, attachment.description, attachment.translations,
language and autoTranslateShowInverse so only one of those branches mutates
attachment.translations.original or attachment.* per pass.
Comment on lines
+90
to
+94
| if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) { | ||
| originalMessage.attachments[0].description = message.msg; | ||
| message.attachments = originalMessage.attachments; | ||
| message.msg = originalMessage.msg; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard attachment-description overwrite to avoid clearing captions on non-text edits.
Lines 90-94 always assign originalMessage.attachments[0].description = message.msg. If message.msg is absent in an update payload, this can wipe an existing description.
Suggested fix
-if (originalMessage.attachments && originalMessage.attachments.length > 0 && originalMessage.attachments[0].description !== undefined) {
+if (
+ originalMessage.attachments &&
+ originalMessage.attachments.length > 0 &&
+ originalMessage.attachments[0].description !== undefined &&
+ typeof message.msg === 'string'
+) {
originalMessage.attachments[0].description = message.msg;
message.attachments = originalMessage.attachments;
message.msg = originalMessage.msg;
}🤖 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/app/lib/server/methods/updateMessage.ts` around lines 90 - 94,
The update currently unconditionally sets
originalMessage.attachments[0].description = message.msg which can erase
existing captions when the update payload doesn't include text; change the logic
in updateMessage (use the originalMessage and message variables) so you only
overwrite attachments[0].description when message.msg is present (e.g., !==
undefined and not an empty string) in the incoming update; keep the assignment
of message.attachments and the swap of message.msg/message.msg restoration
scoped to that same guard so non-text edits don't clear existing descriptions.
Comment on lines
+119
to
+120
| messageContent = message.attachments[0].description || ''; | ||
| escapeHtml(messageContent); |
Contributor
There was a problem hiding this comment.
Preserve system message formatting instead of overwriting with attachment description.
When message.attachments is present, this code unconditionally replaces messageContent with the first attachment's description, even for system messages. System messages (line 111-113) are already formatted and sanitized via DOMPurify and should not be overwritten. This causes system messages to lose their proper formatting in transcripts.
♻️ Proposed fix to preserve system message content
- if (message.attachments && message.attachments?.length > 0) {
+ if (message.attachments && message.attachments?.length > 0 && !messageType?.system) {
messageContent = message.attachments[0].description || '';
messageContent = escapeHtml(messageContent);
🤖 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/app/livechat/server/lib/sendTranscript.ts` around lines 119 -
120, The code currently overwrites messageContent with
message.attachments[0].description for any message; change it to only use the
attachment description when the message is not a system message so
already-formatted/sanitized system messages (the content produced earlier with
DOMPurify) are preserved. Concretely, check the message type/flag that
identifies system messages (e.g., a property like message.system or message.type
=== 'system') before assigning messageContent =
message.attachments[0].description, and keep the existing DOMPurify-sanitized
messageContent for system messages; still call escapeHtml(messageContent) after
deciding which content to use. Ensure you update the logic around
messageContent, message.attachments, and the escapeHtml call so system messages
are never overwritten by attachment descriptions.
|
|
||
| if (message.attachments && message.attachments?.length > 0) { | ||
| messageContent = message.attachments[0].description || ''; | ||
| escapeHtml(messageContent); |
Contributor
There was a problem hiding this comment.
Assign the result of escapeHtml to prevent XSS.
The escapeHtml(messageContent) call discards its return value, leaving messageContent unescaped when interpolated into the HTML template at line 157. This creates an XSS vulnerability when attachment descriptions contain malicious HTML.
🔒 Proposed fix
if (message.attachments && message.attachments?.length > 0) {
messageContent = message.attachments[0].description || '';
- escapeHtml(messageContent);
+ messageContent = escapeHtml(messageContent);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| escapeHtml(messageContent); | |
| if (message.attachments && message.attachments?.length > 0) { | |
| messageContent = message.attachments[0].description || ''; | |
| messageContent = escapeHtml(messageContent); |
🤖 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/app/livechat/server/lib/sendTranscript.ts` at line 120, The call
to escapeHtml currently discards its return value, leaving messageContent
unescaped and opening an XSS vector; update the sendTranscript flow so the
escaped string is used when building the HTML (e.g., assign the result of
escapeHtml(messageContent) back to messageContent or to a new variable and use
that variable in the template). Locate the escapeHtml invocation and the HTML
interpolation in the sendTranscript function (in sendTranscript.ts) and replace
the discarded call with an assignment so the sanitized value is what gets
inserted into the transcript HTML.
Comment on lines
+206
to
216
| let text = rocketMessage.msg; | ||
|
|
||
| const attachment = this.getMessageAttachment(rocketMessage); | ||
| if (attachment) { | ||
| fileName = Meteor.absoluteUrl(attachment.title_link); | ||
| if (!text) { | ||
| text = attachment.description; | ||
| } | ||
| } | ||
|
|
||
| await slack.postMessage(slack.getSlackChannel(rocketMessage.rid), { ...rocketMessage, msg: `${text} ${fileName}` }); |
Contributor
There was a problem hiding this comment.
Guard text before interpolating Slack file-share message.
If rocketMessage.msg is empty and attachment.description is missing, Line 216 sends "undefined ${fileName}". Default text to '' before composing msg.
Suggested patch
- let text = rocketMessage.msg;
+ let text = rocketMessage.msg || '';
const attachment = this.getMessageAttachment(rocketMessage);
if (attachment) {
fileName = Meteor.absoluteUrl(attachment.title_link);
- if (!text) {
- text = attachment.description;
+ if (!text) {
+ text = attachment.description || '';
}
}
- await slack.postMessage(slack.getSlackChannel(rocketMessage.rid), { ...rocketMessage, msg: `${text} ${fileName}` });
+ const msg = text ? `${text} ${fileName}` : fileName;
+ await slack.postMessage(slack.getSlackChannel(rocketMessage.rid), { ...rocketMessage, msg });🤖 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/app/slackbridge/server/RocketAdapter.ts` around lines 206 - 216,
The code can produce "undefined <fileName>" when rocketMessage.msg and
attachment.description are both missing; in RocketAdapter (around
getMessageAttachment and the slack.postMessage call) initialize text to an empty
string (e.g., text = rocketMessage.msg || '') and when setting from the
attachment only use attachment.description if present (e.g., if (!text &&
attachment.description) text = attachment.description), then build the msg
payload passed to slack.postMessage (slack.postMessage(..., { ...rocketMessage,
msg: `${text} ${fileName}` })) so it never interpolates undefined.
| expect(result.current).toBe('E2E_message_encrypted_placeholder'); | ||
| }); | ||
|
|
||
| expect(result.current).toBe('Attachment description'); |
Contributor
There was a problem hiding this comment.
Make the final assertion asynchronous to avoid flaky timing.
Line 66 asserts immediately after an async decrypt flow; this can race state updates. Wrap the final expectation in waitFor.
Suggested patch
- expect(result.current).toBe('Attachment description');
+ await waitFor(() => {
+ expect(result.current).toBe('Attachment description');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(result.current).toBe('Attachment description'); | |
| await waitFor(() => { | |
| expect(result.current).toBe('Attachment description'); | |
| }); |
🤖 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/hooks/useDecryptedMessage.spec.ts` at line 66, The final
assertion in the useDecryptedMessage test is synchronous and races the async
decrypt flow; change the immediate expect(result.current).toBe('Attachment
description') to be wrapped in a waitFor so the assertion waits for state
updates from the hook (use the testing-library's waitFor and assert inside it,
e.g. waitFor(() => expect(result.current).toBe(...))). Update the test in
useDecryptedMessage.spec.ts where result.current is asserted to use waitFor to
make the check reliable.
Comment on lines
+75
to
+79
| if (isFileAttachment(attachment) && attachment.description) { | ||
| attachment.descriptionMd = | ||
| translated || isEncryptedMessageAttachment(attachment) | ||
| ? textToMessageToken(text, parseOptions) | ||
| : (attachment.descriptionMd ?? textToMessageToken(text, parseOptions)); |
Contributor
There was a problem hiding this comment.
Parse descriptionMd from attachment.description, not the generalized text fallback.
On Line 78, descriptionMd is built from text, but text may resolve to attachment.text first. For file attachments that carry both fields, this can generate markdown for the wrong source and display incorrect caption content.
Suggested fix
if (isFileAttachment(attachment) && attachment.description) {
- attachment.descriptionMd =
- translated || isEncryptedMessageAttachment(attachment)
- ? textToMessageToken(text, parseOptions)
- : (attachment.descriptionMd ?? textToMessageToken(text, parseOptions));
+ const descriptionText =
+ (isTranslatedAttachment(attachment) && autoTranslateLanguage && attachment?.translations?.[autoTranslateLanguage]) ||
+ attachment.description;
+ attachment.descriptionMd =
+ translated || isEncryptedMessageAttachment(attachment)
+ ? textToMessageToken(descriptionText, parseOptions)
+ : (attachment.descriptionMd ?? textToMessageToken(descriptionText, parseOptions));
}🤖 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/lib/parseMessageTextToAstMarkdown.ts` around lines 75 -
79, The current logic sets attachment.descriptionMd by calling
textToMessageToken(text, parseOptions) which may use attachment.text instead of
the actual attachment.description; update the branch inside the isFileAttachment
block (the conditional that sets attachment.descriptionMd) to call
textToMessageToken(attachment.description, parseOptions) (while preserving the
translated || isEncryptedMessageAttachment(attachment) condition and the
existing fallback to attachment.descriptionMd) so the markdown is parsed from
attachment.description itself.
| _id: upload.id, | ||
| name: upload.file.name, | ||
| composedMessage: { tmid, msg: currentMsg, fileName: upload.file.name, description: upload.description }, | ||
| composedMessage: { tmid, msg: currentMsg, fileName: upload.file.name, description: upload.altText }, |
There was a problem hiding this comment.
In Rocket.Chat's client-side upload flow, when a user uploads a file in an end-to-end encrypted (E2EE) room, the system can silently fall back to sending the file in plaintext (unencrypted) if the E2E room instance is not yet initialized/ready or if the upload object is not marked as encrypted.
Specifically, in apps/meteor/client/lib/chats/flows/processMessageUploads.ts, the function continueSendingMessage retrieves the E2E room instance using await e2e.getInstanceByRoomId(rid). It then iterates over the files to be uploaded. If e2eRoom is undefined (e.g., due to initialization delays, network issues, or errors fetching keys) or if isEncryptedUpload(upload) evaluates to false, the code executes a fallback block that adds the file to confirmFilesQueue with an unencrypted composedMessage and continues the loop. Finally, the unencrypted files are confirmed and sent to the server via /v1/rooms.mediaConfirm/${rid}/${fileToConfirm._id}.
This fail-open behavior violates the core security guarantee of End-to-End Encryption (E2EE). If E2EE is enabled for a room, any failure to encrypt must result in a hard failure (fail-closed), blocking the transmission and alerting the user, rather than silently transmitting sensitive files in plaintext to the server.
Steps to Reproduce
- Intercept or simulate a delay in E2EE room initialization so that
e2e.getInstanceByRoomId(rid)returnsundefinedor resolves slowly. - Attempt to upload a file in the E2EE room.
- Observe that the client-side code in
continueSendingMessagesilently bypasses encryption and posts the plaintext file details to the server via/v1/rooms.mediaConfirm/${rid}/${fileToConfirm._id}without any warning or error displayed to the user.
Trace
graph TD
subgraph SG0 ["apps/meteor/client/lib/chats/Upload.ts"]
isEncryptedUpload["isEncryptedUpload"]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/client/lib/chats/flows/processMessageUploads.ts"]
getHeightAndWidthFromDataUrl["getHeightAndWidthFromDataUrl"]
getAttachmentForFile["Generates file attachment properties, including encryption metadata and dimensions for image files."]
getEncryptedContent["getEncryptedContent"]
continueSendingMessage{{"Processes queued file uploads and sends confirmation messages to the server."}}
processMessageUploads["processMessageUploads"]
onConfirm["onConfirm"]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/client/lib/e2ee/crypto/aes.ts"]
encrypt["encrypt"]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/client/lib/e2ee/crypto/shared.ts"]
encryptBuffer["encryptBuffer"]
._Rocket.Chat_apps_meteor_client_lib_e2ee_crypto_shared.ts["./Rocket.Chat/apps/meteor/client/lib/e2ee/crypto/shared.ts"]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/client/lib/e2ee/logger.ts"]
isDebugEnabled["isDebugEnabled"]
Logger.constructor["Logger.constructor"]
Logger.span["Logger.span"]
Span.constructor["Span.constructor"]
Span.log["Span.log"]
Span.set["Span.set"]
Span.info["Span.info"]
Span.warn["Span.warn"]
Span.error["Span.error"]
createLogger["createLogger"]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts"]
filterMutation["filterMutation"]
E2ERoom.setState["E2ERoom.setState"]
E2ERoom.isReady["Checks if the E2E room state is 'READY'."]
E2ERoom.shouldConvertSentMessages["Determines if a message being sent should be encrypted based on room state."]
E2ERoom.onRoomKeyReset["Handles room key reset events by clearing session keys and updating state."]
E2ERoom.encryptText["Encrypts text content using AES-CTR and returns the encrypted data."]
E2ERoom.encryptMessageContent["Prepares and serializes message content for E2E encryption."]
._Rocket.Chat_apps_meteor_client_lib_e2ee_rocketchat.e2e.room.ts["Defines constants and symbols for E2E room management including logger and internal state keys."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/client/lib/e2ee/rocketchat.e2e.ts"]
E2E.waitForRoom["Waits for a room to be available in the global room store."]
E2E.getInstanceByRoomId["Retrieves or creates an E2E room instance for a specific room ID."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/client/lib/toast.ts"]
dispatchToastMessage["Dispatches a toast notification message to be displayed in the UI."]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/client/lib/utils/getConfig.ts"]
getConfig["getConfig"]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/lib/utils/getFileExtension.ts"]
getFileExtension["Extracts the file extension from a filename string."]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG10 ["packages/base64/src/base64.ts"]
getChar["getChar"]
newBinary["newBinary"]
encode["Encodes a string or byte array into a Base64-encoded string."]
end
style SG10 fill:#2a2a2a,stroke:#444,color:#aaa
continueSendingMessage --> dispatchToastMessage
continueSendingMessage --> E2ERoom.shouldConvertSentMessages
continueSendingMessage --> E2ERoom.encryptMessageContent
continueSendingMessage --> E2E.getInstanceByRoomId
continueSendingMessage --> isEncryptedUpload
continueSendingMessage --> getEncryptedContent
E2ERoom.shouldConvertSentMessages --> E2ERoom.isReady
E2ERoom.encryptMessageContent --> encode
E2ERoom.encryptMessageContent --> E2ERoom.encryptText
E2E.getInstanceByRoomId --> E2ERoom.onRoomKeyReset
E2E.getInstanceByRoomId --> ._Rocket.Chat_apps_meteor_client_lib_e2ee_rocketchat.e2e.room.ts
E2E.getInstanceByRoomId --> E2E.waitForRoom
getEncryptedContent --> E2ERoom.encryptMessageContent
getEncryptedContent --> getFileExtension
getEncryptedContent --> getAttachmentForFile
encode --> getChar
encode --> newBinary
E2ERoom.encryptText --> encode
E2ERoom.encryptText --> Logger.span
E2ERoom.encryptText --> Span.info
E2ERoom.encryptText --> Span.error
E2ERoom.encryptText --> encrypt
E2ERoom.onRoomKeyReset --> Logger.span
E2ERoom.onRoomKeyReset --> Span.set
E2ERoom.onRoomKeyReset --> Span.info
E2ERoom.onRoomKeyReset --> Span.warn
E2ERoom.onRoomKeyReset --> E2ERoom.setState
._Rocket.Chat_apps_meteor_client_lib_e2ee_rocketchat.e2e.room.ts --> createLogger
getAttachmentForFile --> getFileExtension
getAttachmentForFile --> getHeightAndWidthFromDataUrl
Logger.span --> isDebugEnabled
Logger.span --> Span.constructor
Span.info --> Span.log
Span.error --> Span.log
Span.error --> Span.set
encrypt --> encryptBuffer
encrypt --> ._Rocket.Chat_apps_meteor_client_lib_e2ee_crypto_shared.ts
Span.set --> Span.set
Span.warn --> Span.log
E2ERoom.setState --> Logger.span
E2ERoom.setState --> Span.info
E2ERoom.setState --> Span.error
E2ERoom.setState --> filterMutation
createLogger --> Logger.constructor
isDebugEnabled --> getConfig
processMessageUploads --> continueSendingMessage
onConfirm --> continueSendingMessage
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts
Lines: 126
Severity: high
Vulnerability: E2EE Fail-Open to Plaintext File Upload Transmission when E2E Room is Uninitialized or Upload is Unencrypted
Description:
In Rocket.Chat's client-side upload flow, when a user uploads a file in an end-to-end encrypted (E2EE) room, the system can silently fall back to sending the file in plaintext (unencrypted) if the E2E room instance is not yet initialized/ready or if the upload object is not marked as encrypted.
Specifically, in `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, the function `continueSendingMessage` retrieves the E2E room instance using `await e2e.getInstanceByRoomId(rid)`. It then iterates over the files to be uploaded. If `e2eRoom` is undefined (e.g., due to initialization delays, network issues, or errors fetching keys) or if `isEncryptedUpload(upload)` evaluates to false, the code executes a fallback block that adds the file to `confirmFilesQueue` with an unencrypted `composedMessage` and continues the loop. Finally, the unencrypted files are confirmed and sent to the server via `/v1/rooms.mediaConfirm/${rid}/${fileToConfirm._id}`.
This fail-open behavior violates the core security guarantee of End-to-End Encryption (E2EE). If E2EE is enabled for a room, any failure to encrypt must result in a hard failure (fail-closed), blocking the transmission and alerting the user, rather than silently transmitting sensitive files in plaintext to the server.
Proof of Concept:
1. Intercept or simulate a delay in E2EE room initialization so that `e2e.getInstanceByRoomId(rid)` returns `undefined` or resolves slowly.
2. Attempt to upload a file in the E2EE room.
3. Observe that the client-side code in `continueSendingMessage` silently bypasses encryption and posts the plaintext file details to the server via `/v1/rooms.mediaConfirm/${rid}/${fileToConfirm._id}` without any warning or error displayed to the user.
Affected Code:
- [continueSendingMessage in processMessageUploads.ts](https://github.com/RocketChat/Rocket.Chat/blob/develop/apps/meteor/client/lib/chats/flows/processMessageUploads.ts#L121-L129)
```typescript
let content;
if (!e2eRoom || !isEncryptedUpload(upload)) {
confirmFilesQueue.push({
_id: upload.id,
name: upload.file.name,
composedMessage: { tmid, msg: currentMsg, fileName: upload.file.name, description: upload.altText },
});
continue;
}
```
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
| )} | ||
| {attachment.image_url && ( | ||
| <AttachmentImage {...(attachment.image_dimensions as any)} src={attachment.image_url} alt={attachment.description || ''} /> | ||
| <AttachmentImage {...(attachment.image_dimensions as any)} src={attachment.image_url} alt={attachment.image_alt || ''} /> |
There was a problem hiding this comment.
The Rocket.Chat client does not validate or sanitize the protocol of URLs provided in message attachments (such as author_link and title_link) before rendering them as the href attribute of anchor (<a>) elements.
If an attacker sends a message containing an attachment with a malicious URI scheme (such as javascript:) in the author_link or title_link fields, the client will render a link pointing to that URI. When a victim clicks on the attachment's author name or title, the browser executes the malicious JavaScript payload in the context of the victim's session.
This leads to Stored Cross-Site Scripting (XSS), allowing an attacker to steal session tokens, perform unauthorized actions on behalf of the victim, or compromise the victim's account entirely.
Steps to Reproduce
An attacker can send a message payload with a custom attachment via the Rocket.Chat REST API (e.g., chat.postMessage):
{
"roomId": "GENERAL",
"attachments": [
{
"author_name": "Malicious Author",
"author_link": "javascript:alert(document.domain)",
"title": "Malicious Title",
"title_link": "javascript:alert(document.cookie)"
}
]
}When any user in the channel clicks on "Malicious Author" or "Malicious Title", the JavaScript payload alert(...) will execute.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/emoji/client/emojiParser.ts"]
emojiParser["Parses and replaces emoji shortcodes with rendered emoji elements in HTML content."]
isElement["Type guard to check if a node is an Element."]
isTextNode["Type guard to check if a node is a Text node."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/client/components/MarkdownText.tsx"]
getRegexp["Generates a regular expression for validating URI schemes."]
MarkdownText["Renders Markdown content safely by sanitizing HTML and processing tokens."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/client/components/message/content/Action.tsx"]
Action["Action"]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx"]
applyMarkdownIfRequires["applyMarkdownIfRequires"]
DefaultAttachment{{"Renders a default message attachment with support for Markdown and actions."}}
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/client/components/message/content/attachments/default/ActionAttachmentButton.tsx"]
ActionAttachmentButton["Renders a button for message attachment actions."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/client/components/message/content/attachments/default/ActionAttachtment.tsx"]
ActionAttachment["Renders a group of action buttons for an attachment."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/client/components/message/content/attachments/default/Field.tsx"]
Field["Renders a single field in an attachment."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/client/components/message/content/attachments/default/FieldsAttachment.tsx"]
FieldsAttachment["Renders a collection of fields for an attachment."]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/client/components/message/content/attachments/default/ShortField.tsx"]
ShortField["A UI component that renders a field with a fixed width, used for structured attachment display."]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/client/components/message/content/attachments/default/hooks/usePerformActionMutation.ts"]
usePerformActionMutation["usePerformActionMutation"]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG10 ["apps/meteor/client/components/message/content/attachments/structure/Attachment.tsx"]
Attachment["A container component for message attachments, applying consistent layout and styling."]
end
style SG10 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG11 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthor.tsx"]
AttachmentAuthor["A layout component for displaying the author information of an attachment."]
end
style SG11 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG12 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorAvatar.tsx"]
AttachmentAuthorAvatar["Displays the avatar for an attachment's author."]
end
style SG12 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG13 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentAuthorName.tsx"]
AttachmentAuthorName["Renders the author name of an attachment with truncation."]
end
style SG13 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG14 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentBlock.tsx"]
AttachmentBlock["Provides a styled container block for message attachments with a colored side border."]
end
style SG14 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG15 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentContent.tsx"]
AttachmentContent["Provides a styled content container for attachment details."]
end
style SG15 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG16 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentImage.tsx"]
getDimensions["Calculates the dimensions for an image attachment based on provided limits and aspect ratio."]
AttachmentImage["Renders an image attachment, managing loading states, error retries, and responsive sizing."]
end
style SG16 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG17 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentRow.tsx"]
AttachmentRow["Provides a flex-row container for layout of attachment elements."]
end
style SG17 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG18 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentText.tsx"]
AttachmentText["Renders the text content of an attachment with localization support."]
end
style SG18 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG19 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentThumb.tsx"]
AttachmentThumb["Renders a thumbnail avatar for an attachment."]
end
style SG19 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG20 ["apps/meteor/client/components/message/content/attachments/structure/AttachmentTitle.tsx"]
AttachmentTitle["Renders the title of an attachment with truncation."]
end
style SG20 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG21 ["apps/meteor/client/components/message/content/attachments/structure/image/ImageBox.tsx"]
ImageBox["A styled wrapper component for displaying images within message attachments."]
end
style SG21 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG22 ["apps/meteor/client/components/message/content/attachments/structure/image/Load.tsx"]
Load["A component that renders a placeholder for an image attachment that needs to be manually loaded by the user."]
end
style SG22 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG23 ["apps/meteor/client/components/message/content/attachments/structure/image/Retry.tsx"]
Retry["A component that renders a retry button for failed image attachment loads."]
end
style SG23 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG24 ["apps/meteor/client/components/message/content/collapsible/CollapsibleContent.tsx"]
CollapsibleContent["Displays a toggle button to collapse or expand message content."]
end
style SG24 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG25 ["apps/meteor/client/components/message/hooks/useCollapse.tsx"]
useCollapse["Hook to manage the collapsed state of message attachments."]
end
style SG25 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG26 ["apps/meteor/client/hooks/useExternalLink.ts"]
useExternalLink["Returns a callback to open a URL in a new window/tab securely."]
end
style SG26 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG27 ["apps/meteor/client/lib/errors/InvalidUrlError.ts"]
InvalidUrlError.constructor["InvalidUrlError.constructor"]
end
style SG27 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG28 ["apps/meteor/client/lib/utils/renderMessageEmoji.ts"]
renderMessageEmoji["Renders emoji characters in a message string using the emoji parser."]
end
style SG28 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG29 ["apps/meteor/client/views/room/contexts/ChatContext.ts"]
useChat["Retrieves the current chat API context for interacting with the message composer."]
end
style SG29 fill:#2a2a2a,stroke:#444,color:#aaa
DefaultAttachment --> MarkdownText
DefaultAttachment --> useCollapse
DefaultAttachment --> FieldsAttachment
DefaultAttachment --> ActionAttachment
DefaultAttachment --> AttachmentContent
DefaultAttachment --> AttachmentAuthorAvatar
DefaultAttachment --> AttachmentBlock
DefaultAttachment --> AttachmentTitle
DefaultAttachment --> AttachmentRow
DefaultAttachment --> AttachmentText
DefaultAttachment --> AttachmentThumb
DefaultAttachment --> AttachmentImage
DefaultAttachment --> AttachmentAuthorName
DefaultAttachment --> AttachmentAuthor
DefaultAttachment --> applyMarkdownIfRequires
MarkdownText --> renderMessageEmoji
MarkdownText --> getRegexp
useCollapse --> CollapsibleContent
FieldsAttachment --> Field
FieldsAttachment --> ShortField
ActionAttachment --> useExternalLink
ActionAttachment --> ActionAttachmentButton
AttachmentBlock --> Attachment
AttachmentImage --> ImageBox
AttachmentImage --> Load
AttachmentImage --> Retry
AttachmentImage --> getDimensions
applyMarkdownIfRequires --> MarkdownText
renderMessageEmoji --> emojiParser
CollapsibleContent --> Action
ShortField --> Field
useExternalLink --> InvalidUrlError.constructor
ActionAttachmentButton --> usePerformActionMutation
Load --> ImageBox
Retry --> ImageBox
emojiParser --> isElement
emojiParser --> isTextNode
usePerformActionMutation --> useChat
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx
Lines: 100
Severity: high
Vulnerability: Stored Cross-Site Scripting (XSS) via Unsanitized Attachment Links
Description:
The Rocket.Chat client does not validate or sanitize the protocol of URLs provided in message attachments (such as `author_link` and `title_link`) before rendering them as the `href` attribute of anchor (`<a>`) elements.
If an attacker sends a message containing an attachment with a malicious URI scheme (such as `javascript:`) in the `author_link` or `title_link` fields, the client will render a link pointing to that URI. When a victim clicks on the attachment's author name or title, the browser executes the malicious JavaScript payload in the context of the victim's session.
This leads to Stored Cross-Site Scripting (XSS), allowing an attacker to steal session tokens, perform unauthorized actions on behalf of the victim, or compromise the victim's account entirely.
Proof of Concept:
An attacker can send a message payload with a custom attachment via the Rocket.Chat REST API (e.g., `chat.postMessage`):
```json
{
"roomId": "GENERAL",
"attachments": [
{
"author_name": "Malicious Author",
"author_link": "javascript:alert(document.domain)",
"title": "Malicious Title",
"title_link": "javascript:alert(document.cookie)"
}
]
}
```
When any user in the channel clicks on "Malicious Author" or "Malicious Title", the JavaScript payload `alert(...)` will execute.
Affected Code:
- In [DefaultAttachment.tsx](./Rocket.Chat/apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx), the `author_link` and `title_link` properties from the attachment payload are directly passed to the `href` attribute of the `AttachmentAuthorName` and `AttachmentTitle` components (which render as `<a>` tags):
```typescript
{attachment.author_name && (
<AttachmentAuthor>
{attachment.author_icon && <AttachmentAuthorAvatar url={attachment.author_icon} />}
<AttachmentAuthorName
{...(attachment.author_link && {
is: 'a',
href: attachment.author_link,
target: '_blank',
color: undefined,
})}
>
{attachment.author_name}
</AttachmentAuthorName>
</AttachmentAuthor>
)}
```
and
```typescript
{attachment.title && (
<AttachmentRow>
<AttachmentTitle
{...(attachment.title_link && {
is: 'a',
href: attachment.title_link,
target: '_blank',
color: undefined,
})}
>
{attachment.title}
</AttachmentTitle>{' '}
{collapse}
</AttachmentRow>
)}
```
- In [QuoteAttachment.tsx](./Rocket.Chat/apps/meteor/client/components/message/content/attachments/QuoteAttachment.tsx), the same pattern is used for `author_link`:
```typescript
<AttachmentAuthorName
{...(attachment.author_link && { is: 'a', href: attachment.author_link, target: '_blank', color: 'default' })}
>
{attachment.author_name}
</AttachmentAuthorName>
```
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Comment on lines
+111
to
+121
| let messageContent = messageType?.system | ||
| ? DOMPurify.sanitize(` | ||
| <i>${messageType.text(i18n.cloneInstance({ interpolation: { escapeValue: false } }).t, message)}}</i>`) | ||
| : escapeHtml(message.msg); | ||
|
|
||
| let filesHTML = ''; | ||
|
|
||
| if (message.attachments && message.attachments?.length > 0) { | ||
| messageContent = message.attachments[0].description || ''; | ||
| escapeHtml(messageContent); | ||
|
|
There was a problem hiding this comment.
The sendTranscript function in apps/meteor/app/livechat/server/lib/sendTranscript.ts fails to properly sanitize the description field of message attachments when generating the transcript email. Specifically, the escapeHtml function is called on messageContent on line 120, but the result is not assigned back to messageContent, leaving the variable containing the raw, potentially malicious, input. This raw input is then rendered directly into the HTML of the transcript email on line 157, leading to a Stored Cross-Site Scripting (XSS) vulnerability in the email client of the recipient.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/file-upload/server/lib/FileUpload.ts"]
FileUpload.addExtensionTo["Ensures a file has the correct extension based on its MIME type."]
FileUpload.getStoreByName["Retrieves a file store handler by name."]
FileUpload.getBuffer["Retrieves file content as a Buffer."]
FileUpload.copy["Copies file content to a target stream."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/lib/server/lib/notifyListener.ts"]
notifyOnSettingChanged["notifyOnSettingChanged"]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/app/livechat/server/lib/sendTranscript.ts"]
sendTranscript{{"Generates and sends a conversation transcript via email to the requested address."}}
escapeHtml["escapeHtml"]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/app/mailer/server/api.ts"]
replacekey["Replaces a specific key in a string with a given value using a regular expression."]
translate["Translates a string by replacing language keys with their internationalized values."]
replace["Replaces template variables in a string with values from a provided data object and global settings."]
replaceEscaped["replaceEscaped"]
wrap["Wraps email content with a header and footer template, applying CSS inlining."]
checkAddressFormat["checkAddressFormat"]
sendNoWrap["Sends an email without applying standard wrapping, triggering email-related app events."]
send["Sends a formatted email by replacing template variables and wrapping the content."]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/app/mailer/server/replaceVariables.ts"]
replaceVariables["replaceVariables"]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/app/settings/server/CachedSettings.ts"]
CachedSettings.get["Retrieves the value of a setting from the cache."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/app/utils/server/lib/getTimezone.ts"]
padOffset["padOffset"]
guessTimezoneFromOffset["Guesses timezone from a UTC offset."]
getTimezone["Retrieves the timezone for a user or system default."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/lib/utils/stringUtils.ts"]
makeString["Converts an input to a string, returning an empty string if the input is falsy."]
strLeft["Extracts the substring to the left of the first occurrence of a separator."]
strRightBack["Extracts the substring to the right of the last occurrence of a separator."]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["packages/logger/src/index.ts"]
Logger.debug["Logs a debug-level message."]
Logger.error["Logs an error-level message."]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["packages/message-types/src/MessageTypes.ts"]
MessageTypes.getType["Retrieves the message type configuration for a given message."]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
sendTranscript --> CachedSettings.get
sendTranscript --> Logger.debug
sendTranscript --> Logger.error
sendTranscript --> getTimezone
sendTranscript --> MessageTypes.getType
sendTranscript --> FileUpload.getBuffer
sendTranscript --> replace
sendTranscript --> send
sendTranscript --> escapeHtml
CachedSettings.get --> CachedSettings.get
Logger.debug --> Logger.debug
Logger.error --> Logger.error
getTimezone --> CachedSettings.get
getTimezone --> guessTimezoneFromOffset
FileUpload.getBuffer --> FileUpload.getStoreByName
FileUpload.getBuffer --> FileUpload.copy
replace --> CachedSettings.get
replace --> strLeft
replace --> strRightBack
replace --> replacekey
replace --> translate
replace --> replace
send --> replace
send --> wrap
send --> sendNoWrap
escapeHtml --> replace
guessTimezoneFromOffset --> padOffset
FileUpload.copy --> FileUpload.addExtensionTo
FileUpload.copy --> FileUpload.getStoreByName
FileUpload.copy --> FileUpload.copy
strLeft --> makeString
strRightBack --> makeString
replacekey --> replace
translate --> replaceVariables
wrap --> CachedSettings.get
wrap --> replace
wrap --> replaceEscaped
sendNoWrap --> CachedSettings.get
sendNoWrap --> checkAddressFormat
sendNoWrap --> notifyOnSettingChanged
replaceEscaped --> CachedSettings.get
replaceEscaped --> replace
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/app/livechat/server/lib/sendTranscript.ts
Lines: 111-121
Severity: high
Vulnerability: Stored XSS in Livechat Transcript Email
Description:
The `sendTranscript` function in `apps/meteor/app/livechat/server/lib/sendTranscript.ts` fails to properly sanitize the `description` field of message attachments when generating the transcript email. Specifically, the `escapeHtml` function is called on `messageContent` on line 120, but the result is not assigned back to `messageContent`, leaving the variable containing the raw, potentially malicious, input. This raw input is then rendered directly into the HTML of the transcript email on line 157, leading to a Stored Cross-Site Scripting (XSS) vulnerability in the email client of the recipient.
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Comment on lines
+37
to
40
| const msgText = originalMessage?.attachments?.[0]?.description ?? originalMessage.msg; | ||
| if (msgText === message.msg && !previewUrls && !message.customFields) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Description
A Stored Cross-Site Scripting (XSS) vulnerability exists in the message update functionality. When sending a new message, Rocket.Chat validates message attachments using validateBodyAttachments to ensure that links (such as title_link, author_link, image_url, etc.) do not contain malicious javascript: pseudo-protocol URLs.
However, when a user updates an existing message via the updateMessage DDP method, the executeUpdateMessage function allows the attachments field to be updated without performing any URL validation or sanitization. An attacker can bypass the XSS protections by first sending a benign message and then updating it with a malicious attachment containing javascript: URLs. When other users click on these links in the updated message, arbitrary JavaScript code will execute in their browser context.
Impact
An attacker can execute arbitrary JavaScript code in the browser of any user who views the updated message and interacts with the malicious attachment links. This can lead to session hijacking, credential theft, or unauthorized actions performed on behalf of the victim.
Attacker Model
Any authenticated user who has permission to edit their own messages (which is typically enabled by default) can exploit this vulnerability. No administrative privileges are required.
Steps to Reproduce
- Send a benign message to a channel.
- Call the
updateMessageDDP method with the message ID and a modifiedattachmentspayload:
Meteor.call('updateMessage', {
_id: "<message_id>",
rid: "<room_id>",
attachments: [
{
title: "Click here to win a prize!",
title_link: "javascript:alert(document.domain)"
}
]
});- When another user clicks the attachment title, the payload
alert(document.domain)will execute.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/authorization/server/functions/canAccessRoom.ts"]
._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts["Exports server-side room access authorization functions and defines attributes for room access checks."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/authorization/server/functions/canSendMessage.ts"]
validateRoomMessagePermissionsAsync["Validates if a user has permissions to send a message to a specific room, checking archiving, block status, read-only status, and muting."]
canSendMessageAsync["Checks if a user can send a message to a room by first validating permissions."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/app/authorization/server/functions/hasPermission.ts"]
hasPermissionAsync["Checks if a user has a specific permission within a given scope."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/app/lib/server/methods/updateMessage.ts"]
executeUpdateMessage{{"Validates and processes an update to an existing message, checking permissions and time limits."}}
updateMessage["updateMessage"]
applyAirGappedRestrictionsValidation.arg1["applyAirGappedRestrictionsValidation.arg1"]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/app/settings/server/CachedSettings.ts"]
CachedSettings.get["Retrieves the value of a setting from the cache."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/client/meteor/overrides/userAndUsers.ts"]
Meteor.userId["Overrides Meteor's default userId to use the application's reactive Zustand store."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/client/meteor/user.ts"]
watchUserId["watchUserId"]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/client/meteor/watch.ts"]
watch["watch"]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/server/lib/rooms/roomCoordinator.ts"]
RoomCoordinatorServer.allowMemberAction["RoomCoordinatorServer.allowMemberAction"]
RoomCoordinatorServer.getRoomDirectives["Retrieves directives for a specific room type."]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/server/services/authorization/service.ts"]
Authorization.hasPermission["Checks if a user has a specific permission."]
Authorization.all["Authorization.all"]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
executeUpdateMessage --> CachedSettings.get
executeUpdateMessage --> hasPermissionAsync
executeUpdateMessage --> canSendMessageAsync
executeUpdateMessage --> updateMessage
CachedSettings.get --> CachedSettings.get
hasPermissionAsync --> Authorization.hasPermission
canSendMessageAsync --> validateRoomMessagePermissionsAsync
updateMessage --> Meteor.userId
updateMessage --> executeUpdateMessage
Authorization.hasPermission --> Authorization.all
validateRoomMessagePermissionsAsync --> hasPermissionAsync
validateRoomMessagePermissionsAsync --> ._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts
validateRoomMessagePermissionsAsync --> RoomCoordinatorServer.allowMemberAction
validateRoomMessagePermissionsAsync --> RoomCoordinatorServer.getRoomDirectives
Meteor.userId --> watchUserId
watchUserId --> watch
applyAirGappedRestrictionsValidation.arg1 --> executeUpdateMessage
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/app/lib/server/methods/updateMessage.ts
Lines: 37-95
Severity: high
Vulnerability: Stored XSS via Malicious Attachments in Message Update
Description:
### Description
A Stored Cross-Site Scripting (XSS) vulnerability exists in the message update functionality. When sending a new message, Rocket.Chat validates message attachments using `validateBodyAttachments` to ensure that links (such as `title_link`, `author_link`, `image_url`, etc.) do not contain malicious `javascript:` pseudo-protocol URLs.
However, when a user updates an existing message via the `updateMessage` DDP method, the `executeUpdateMessage` function allows the `attachments` field to be updated without performing any URL validation or sanitization. An attacker can bypass the XSS protections by first sending a benign message and then updating it with a malicious attachment containing `javascript:` URLs. When other users click on these links in the updated message, arbitrary JavaScript code will execute in their browser context.
### Impact
An attacker can execute arbitrary JavaScript code in the browser of any user who views the updated message and interacts with the malicious attachment links. This can lead to session hijacking, credential theft, or unauthorized actions performed on behalf of the victim.
### Attacker Model
Any authenticated user who has permission to edit their own messages (which is typically enabled by default) can exploit this vulnerability. No administrative privileges are required.
Proof of Concept:
1. Send a benign message to a channel.
2. Call the `updateMessage` DDP method with the message ID and a modified `attachments` payload:
```javascript
Meteor.call('updateMessage', {
_id: "<message_id>",
rid: "<room_id>",
attachments: [
{
title: "Click here to win a prize!",
title_link: "javascript:alert(document.domain)"
}
]
});
```
3. When another user clicks the attachment title, the payload `alert(document.domain)` will execute.
Affected Code:
- User calls the `updateMessage` DDP method: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:109)
- `executeUpdateMessage` is invoked: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:16)
- The keys of the updated message are checked against `allowedEditedFields`: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:26-32)
- Since `attachments` is in `allowedEditedFields`, the check passes: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:14)
- No validation (such as `validateBodyAttachments` or `validateMessage` from `sendMessage.ts`) is performed on the updated `attachments` array.
- The message is saved via `updateMessage`: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:98)
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
Comment on lines
+37
to
40
| const msgText = originalMessage?.attachments?.[0]?.description ?? originalMessage.msg; | ||
| if (msgText === message.msg && !previewUrls && !message.customFields) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Description
An authorization bypass vulnerability exists in the message update functionality, allowing users to bypass the message-impersonate permission restriction.
In Rocket.Chat, the message-impersonate permission is required to send messages with a custom alias or avatar. This is enforced during message creation in validateMessage (called by sendMessage). However, when updating an existing message via executeUpdateMessage, the fields alias and avatar are included in allowedEditedFields, but the function fails to verify whether the user has the message-impersonate permission if these fields are modified.
Consequently, any user who is allowed to edit their own messages can bypass this restriction by sending a normal message and immediately updating it to have any custom alias (e.g., "Admin" or "System Bot") and custom avatar, effectively impersonating other users or system services.
Impact
Authenticated users can impersonate administrators, moderators, or system bots within any channel they have access to, potentially leading to highly convincing social engineering attacks, phishing, or spreading misinformation.
Attacker Model
Any authenticated user who has permission to edit their own messages can exploit this vulnerability. No administrative privileges are required.
Steps to Reproduce
- Send a normal message to a channel.
- Call the
updateMessageDDP method with the message ID and a modifiedaliasandavatar:
Meteor.call('updateMessage', {
_id: "<message_id>",
rid: "<room_id>",
alias: "Administrator",
avatar: "https://example.com/admin-avatar.png",
msg: "The system will undergo maintenance in 5 minutes. Please log out."
});- The message will be updated and displayed to other users as if it was sent by "Administrator" with the specified avatar, bypassing the
message-impersonatepermission check.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/authorization/server/functions/canAccessRoom.ts"]
._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts["Exports server-side room access authorization functions and defines attributes for room access checks."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/authorization/server/functions/canSendMessage.ts"]
validateRoomMessagePermissionsAsync["Validates if a user has permissions to send a message to a specific room, checking archiving, block status, read-only status, and muting."]
canSendMessageAsync["Checks if a user can send a message to a room by first validating permissions."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/app/authorization/server/functions/hasPermission.ts"]
hasPermissionAsync["Checks if a user has a specific permission within a given scope."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/app/lib/server/methods/updateMessage.ts"]
executeUpdateMessage{{"Validates and processes an update to an existing message, checking permissions and time limits."}}
updateMessage["updateMessage"]
applyAirGappedRestrictionsValidation.arg1["applyAirGappedRestrictionsValidation.arg1"]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/app/settings/server/CachedSettings.ts"]
CachedSettings.get["Retrieves the value of a setting from the cache."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/client/meteor/overrides/userAndUsers.ts"]
Meteor.userId["Overrides Meteor's default userId to use the application's reactive Zustand store."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/client/meteor/user.ts"]
watchUserId["watchUserId"]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/client/meteor/watch.ts"]
watch["watch"]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/server/lib/rooms/roomCoordinator.ts"]
RoomCoordinatorServer.allowMemberAction["RoomCoordinatorServer.allowMemberAction"]
RoomCoordinatorServer.getRoomDirectives["Retrieves directives for a specific room type."]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/server/services/authorization/service.ts"]
Authorization.hasPermission["Checks if a user has a specific permission."]
Authorization.all["Authorization.all"]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
executeUpdateMessage --> CachedSettings.get
executeUpdateMessage --> hasPermissionAsync
executeUpdateMessage --> canSendMessageAsync
executeUpdateMessage --> updateMessage
CachedSettings.get --> CachedSettings.get
hasPermissionAsync --> Authorization.hasPermission
canSendMessageAsync --> validateRoomMessagePermissionsAsync
updateMessage --> Meteor.userId
updateMessage --> executeUpdateMessage
Authorization.hasPermission --> Authorization.all
validateRoomMessagePermissionsAsync --> hasPermissionAsync
validateRoomMessagePermissionsAsync --> ._Rocket.Chat_apps_meteor_app_authorization_server_functions_canAccessRoom.ts
validateRoomMessagePermissionsAsync --> RoomCoordinatorServer.allowMemberAction
validateRoomMessagePermissionsAsync --> RoomCoordinatorServer.getRoomDirectives
Meteor.userId --> watchUserId
watchUserId --> watch
applyAirGappedRestrictionsValidation.arg1 --> executeUpdateMessage
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/app/lib/server/methods/updateMessage.ts
Lines: 37-95
Severity: medium
Vulnerability: Bypass of message-impersonate Permission via Message Update
Description:
### Description
An authorization bypass vulnerability exists in the message update functionality, allowing users to bypass the `message-impersonate` permission restriction.
In Rocket.Chat, the `message-impersonate` permission is required to send messages with a custom `alias` or `avatar`. This is enforced during message creation in `validateMessage` (called by `sendMessage`). However, when updating an existing message via `executeUpdateMessage`, the fields `alias` and `avatar` are included in `allowedEditedFields`, but the function fails to verify whether the user has the `message-impersonate` permission if these fields are modified.
Consequently, any user who is allowed to edit their own messages can bypass this restriction by sending a normal message and immediately updating it to have any custom alias (e.g., "Admin" or "System Bot") and custom avatar, effectively impersonating other users or system services.
### Impact
Authenticated users can impersonate administrators, moderators, or system bots within any channel they have access to, potentially leading to highly convincing social engineering attacks, phishing, or spreading misinformation.
### Attacker Model
Any authenticated user who has permission to edit their own messages can exploit this vulnerability. No administrative privileges are required.
Proof of Concept:
1. Send a normal message to a channel.
2. Call the `updateMessage` DDP method with the message ID and a modified `alias` and `avatar`:
```javascript
Meteor.call('updateMessage', {
_id: "<message_id>",
rid: "<room_id>",
alias: "Administrator",
avatar: "https://example.com/admin-avatar.png",
msg: "The system will undergo maintenance in 5 minutes. Please log out."
});
```
3. The message will be updated and displayed to other users as if it was sent by "Administrator" with the specified avatar, bypassing the `message-impersonate` permission check.
Affected Code:
- User calls the `updateMessage` DDP method: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:109)
- `executeUpdateMessage` is invoked: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:16)
- The keys of the updated message are checked against `allowedEditedFields`: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:26-32)
- Since `alias` and `avatar` are in `allowedEditedFields`, the check passes: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:14)
- The function checks if the user has `edit-message` permission or owns the message: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:52-61)
- The function does NOT check if the user has the `message-impersonate` permission when `alias` or `avatar` are modified.
- The message is saved via `updateMessage`: [updateMessage.ts](./Rocket.Chat/apps/meteor/app/lib/server/methods/updateMessage.ts:98)
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
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.
Proposed changes (including videos or screenshots)
Introduces a dedicated
image_altfield on image attachments to hold accessibility alternative text, separating it fromdescription, which has been used as a visible caption and it was incorrectly pruned herePreviously the "Alternative text" input from the upload modal was stored in
descriptionand used both as the<img alt>and as a visible caption for old messages. Splitting the concerns keeps alt text accessibility-only and freesdescriptionto render old messages back again.How to test the changes
<img>uses it asalt(inspect element / screen reader)Issue(s)
Closes #40663
Steps to test or reproduce
Further comments
SUP-1039
Summary by CodeRabbit
New Features
Bug Fixes