regression: slash command replies show HTML entities in names - #41983
Open
abhinavkrin wants to merge 1 commit into
Open
regression: slash command replies show HTML entities in names#41983abhinavkrin wants to merge 1 commit into
abhinavkrin wants to merge 1 commit into
Conversation
PRs #41667 and #41736 replaced the sprintf postprocessor with i18next's named interpolation. The server i18n instance never sets `interpolation.escapeValue`, so it defaults to `true`, while the client provider sets it to `false`. Values interpolated server-side are now HTML escaped, and since these strings are delivered as chat message bodies the entities are shown to the user verbatim, e.g. `/create a&b` replies with "The channel `#a&b` already exists." Opt out of escaping at the 23 server call sites whose keys moved from `%s` to named interpolation in those two PRs. This restores the pre-migration output exactly and leaves every other server interpolation untouched. Escaping is not load bearing at any of these sites: - 21 go to `notify.ephemeralMessage`, which parses the string into a message-parser AST rendered by gazzodown as React elements, so React escapes the text and no raw HTML sink is involved. - `exportRoomMessagesToFile` already escapes with `escapeHTML()` at the sink, added by #40802, which is unchanged here. That call site had been double escaping since #41736. - The 8 `help` sites interpolate hardcoded keyboard shortcuts with no HTML-special characters, so they are a no-op either way and are included only to keep the rule uniform. No changeset: both migration PRs are unreleased, so this is a regression within the same cycle. Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used📓 Path-based instructions (2)The main Rocket.Chat Meteor application resides in `apps/meteor/`; place its application code there rather than in other monorepo areas.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc) Files:
🧠 Learnings (2)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
🔇 Additional comments (16)
WalkthroughThe changes add ChangesServer i18n interpolation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized server-side change restores literal names in slash-command and related replies without altering client behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Errors 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
KevLehman
approved these changes
Aug 27, 2026
KevLehman
left a comment
KevLehman
left a comment
Member
There was a problem hiding this comment.
A test would be nice but we can create a UI test on a later task
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.8.0 #41983 +/- ##
================================================
Coverage ? 69.33%
================================================
Files ? 4254
Lines ? 168642
Branches ? 30066
================================================
Hits ? 116934
Misses ? 46526
Partials ? 5182
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
2 participants
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)
#41667 and #41736 replaced the
i18next-sprintf-postprocessor(which never escaped) with i18next's native named interpolation (which escapes by default).The server i18n instance never sets
interpolation.escapeValue, so it falls back to the i18next default oftrue, while the client provider explicitly setsfalse:interpolation.escapeValueapps/meteor/server/lib/i18n.tstrueapps/meteor/client/providers/TranslationProvider.tsxfalsepackages/livechat/src/i18next.tsfalseThe regression is therefore confined to server-side calls. Because these strings are delivered as ordinary chat message bodies, nothing decodes the entities and the user reads them literally:
Scope
40 translation keys moved from
%sto{{...}}across the two PRs, spanning 67 call sites. 33 are client-side and unaffected. Of the 34 server-side sites, 4 (SlackBridge) were already opted out in #41736 itself, leaving 23 call sites patched here withinterpolation: { escapeValue: false }.This is deliberately scoped to sites whose behaviour actually changed. A global
escapeValue: falseon the server instance would be the tidier fix, but it is a workspace-wide policy change and does not belong on a release branch — see Further comments.Escaping is not load bearing at any patched site
notify.ephemeralMessage.listeners.module.tsparses the string into a message-parser AST which gazzodown renders as React elements (PlainSpanemits{text}), so React escapes it. NodangerouslySetInnerHTMLanywhere on that path.exportRoomMessagesToFile, thewmcase) flows intoexportMessageObject, which already callsescapeHTML()at the sink. That control was added by fix: escape HTML in exported data #40802 and is untouched here; at the time it landedWelcomestill used%sand was not escaped by i18n, so this restores exactly the input it was written to receive. The site had been double escaping only since chore: replace positional translation parameters with named interpolation #41736.helpsites interpolate hardcoded keyboard shortcuts such asShift (or Ctrl) + ESC, which contain no HTML-special characters. They are a no-op either way and are included only so the rule stays uniform and greppable.I also confirmed the two client call sites that do reach
dangerouslySetInnerHTML(UrlChangeModal.tsx,OAuthGroupPage.tsx) both wrap inDOMPurify.sanitize(), and are on the client instance whereescapeValuewas alreadyfalsebefore and after the migration. Untouched, unaffected.Verification
Rendered every patched key through the real pre-migration stack (
i18next+i18next-sprintf-postprocessor) and the post-patch stack, across values includinga&b,o'brien&co,<b>x</b>anda"b:tsc --noEmit --skipLibCheckexits clean, prettier passes, eslint reports 0 errors on the changed files.Issue(s)
Closes: CORE-2645
Steps to test or reproduce
No admin configuration change is required:
/join #a&b(any non-existent name containing&,<,>,"or').The channel `#a&b` does not exist.After:The channel `#a&b` does not exist.For the case reported in CORE-2645, which needs a widened validation setting:
UTF8_Channel_Names_Validationto[0-9a-zA-Z&_.-]+and save.a&b./create a&bin any room.The channel `#a&b` already exists.After:The channel `#a&b` already exists.Further comments
No changeset. Both migration PRs are unreleased, so this is a regression fixed within the same cycle.
Why not a global
escapeValue: falseon the server instance? It would fix all 23 at once and prevent recurrence, and I audited the blast radius: only 65 of the 212 server-sidei18n.tcalls actually interpolate, and just one key genuinely depends on escaping (UserDataDownload_EmailBody, whose<a href="{{download_link}}">receivesSite_Url+ aRandom.id(), so admin-controlled only). That makes it a good change fordevelop, but it is a workspace-wide behaviour change and too broad for a release branch. Worth a follow-up ticket.Three adjacent issues found while tracing this, all pre-existing and deliberately left alone:
i18n.cloneInstance({ interpolation: { escapeValue: false } })silently ignores the override in i18next 23.4.9, making the opt-out atsendTranscript.ts:113dead code. The per-call form used in this PR does work.notifications/message/email.jsrunsescapeHTML()onuserName/roomNamebefore handing them to i18n, which escapes again, so offline notification emails renderTom & Jerryfor any real name containing&. The same shape exists inmention.module.tsand in the other 11 cases of theexportRoomMessagesToFileswitch. Unlike thewmcase, those keys already used{{...}}before both migration PRs, so they are not regressions from this cycle and a fix for them would need its own changeset.help/server.tsreadssettings.get('language')with a lowercase id; the setting isLanguage.CORE-2645