Skip to content

[fix][push] render message text as text in the notification preview - #7892

Open
ar2rsawseen wants to merge 1 commit into
masterfrom
fix/push-preview-message-rendering
Open

[fix][push] render message text as text in the notification preview#7892
ar2rsawseen wants to merge 1 commit into
masterfrom
fix/push-preview-message-rendering

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

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:

// countly.models.js
var messageInHTMLString = this.decodeHtml(message);      // then placeholders spliced in
...
htmlTitle.innerHTML = content;                            // parsed back out here

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 messagePers and titlePers are 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 through innerText and 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: getUserPropertyElement builds it with setAttribute and innerText and serialises with outerHTML, so the label, key and fallback were already escaped by the serialiser.

Verification

Compared against the pre-change source, in a real DOM:

  • the pre-change source does produce the element, which confirms the harness reaches the code path
  • after the change no element and no event-handler attribute are produced, and the value shows as the text that was typed
  • preview components are byte-identical across a single placeholder mid-text, one at the start, two placeholders, adjacent placeholders, text containing an ampersand, and a message with no personalization at all

node --check and npx eslint clean 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant