[fix][push] render message text as text in the notification preview - #7892
Merged
Conversation
The details view builds its phone-style preview by splicing personalization elements into the message and handing the result to innerHTML, so it can walk the nodes back out and turn each into a component. The message text travels inside that string, so a tag in the message was parsed as markup and its handlers ran. The decode in front of it is not the mistake and cannot simply go. The indexes in messagePers and titlePers are offsets into the message as it was typed, and the api escapes on output, so the string has to be decoded before those offsets line up with anything. Removing the decode would misplace every element that follows an escaped character. So the placeholders now go in as opaque tokens, the whole string is escaped, and the tokens are swapped for their elements afterwards. Only markup generated here survives the escape. The offset arithmetic is untouched: a token stands in for an element and is measured the same way, so every position it computes is the position it computed before. Escaping is a plain string replacement rather than countlyCommon.encodeHtml, which round-trips through innerText and would fold newlines into <br>, changing both the text and the offsets. This covers the title as well as the message, since both go through buildMessageText. The element markup itself was already safe, built with setAttribute and innerText and serialized with outerHTML. Verified against the pre-change source: the payload no longer produces an element or an event-handler attribute and instead shows as the text that was typed, and the preview components for ordinary messages are byte-identical across single, leading, multiple and adjacent placeholders, text containing an ampersand, and messages with no personalization at all.
ar2rsawseen
commented
Aug 24, 2026
ar2rsawseen
commented
Aug 24, 2026
…ceholder token uncollidable Answers both review findings on the preview builder. **A notification without personalization skipped the escaping entirely.** buildMessageText returned early before any of it ran, and the result goes to innerHTML in getPreviewMessageComponentsList. It happened to be safe for a stored notification, because the api escapes what it returns - but the preview also renders the editor's live state, and text typed into the editor has been through no escaping at all. That path now makes the same round trip the personalization path makes: decode, then escape. Not escape alone, which is what the note suggested: the stored message is already escaped, so escaping it again would show a user <b> where they wrote something else. Decoding first makes the result exactly one level of escaping whichever way the message arrived, and it is idempotent for the stored case. **The placeholder token could be typed.** @@CLY_PERS_0@@ in the message body was indistinguishable from the token this function inserts, so an author's literal text became an extra personalization span, or vanished when the index was out of range - and the edit state carries that corruption back on save. The token now carries a per call nonce, which cannot be guessed or typed. Length is not constrained here, since the arithmetic measures the token it actually inserted, so none of the offset handling changes. Checked the round trip against both shapes rather than reasoning about it: an api-escaped message comes back byte-identical, and a raw typed tag comes back neutralized.
cihadtekin
approved these changes
Aug 26, 2026
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
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.
The push details view builds its phone-style preview by splicing personalization elements into the message and handing the result to
innerHTML, so it can walk the child nodes back out and turn each into a component:The message text travels inside that string, so a tag in the message was parsed as markup rather than shown as text, and its event handlers ran.
Why the decode has to stay
It looks like the obvious thing to remove, but it is load-bearing. The keys in
messagePersandtitlePersare character offsets into the message as it was typed, and the API escapes on output, so&arrives as five characters instead of one. Decoding is what puts the string back into the coordinate space those offsets refer to. Drop it and every element after an escaped character lands in the wrong place.That also rules out the simpler-looking
textContent = content, which would remove the markup the walk exists to find and take the personalization chips with it.What changed
Placeholders now go in as opaque tokens, the whole string is escaped, and the tokens are swapped for their elements afterwards. Only markup generated by this code survives the escape.
The offset arithmetic is untouched — a token stands in for an element and is measured the same way, so every position it computes is the position it computed before. Escaping is a plain string replacement rather than
countlyCommon.encodeHtml, which round-trips throughinnerTextand would fold newlines into<br>, changing both the text and those offsets.This covers the title as well as the message, since both go through
buildMessageText. The element markup itself was already safe:getUserPropertyElementbuilds it withsetAttributeandinnerTextand serialises withouterHTML, so the label, key and fallback were already escaped by the serialiser.Verification
Compared against the pre-change source, in a real DOM:
node --checkandnpx eslintclean on all three branches. Not covered by an automated test in-repo: the push frontend has no DOM test harness, and adding one is out of scope here.