security: encode and sanitize values at frontend HTML sinks (24.05) - #7971
Merged
Conversation
…oltip Backport of #7966 to release.24.05. The graph-note tooltip builds an HTML string including the application name from countlyGlobal (raw at runtime) and renders it via tipsy html:true. Encode it with countlyCommon.encodeHtml so it renders as text. Display unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…contenteditable The push message editor set the composed message as innerHTML on a live contenteditable. Sanitize that content with countlyCommon.encodeSomeHtml, allowing only the user-property token span (and the attributes it relies on: class, id, contenteditable, data-user-property-*) and escaping any other markup to inert text. The message body is user text and the token element is the only legitimate markup, so display is unchanged for normal messages; the token id is preserved so the editor's per-token event wiring keeps working. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…istory actions column Backport of the master change. The export/purge history datatable builds an html string in onReady and the template renders it, so every value interpolated into it has to be html-safe before it gets there. Values taken from the row arrive through common.returnOutput, which escape_html_entities has already escaped, so they are deliberately left alone: escaping them a second time would surface the entities literally in the ui. One value in that string comes from countlyGlobal instead. That object is serialized into the dashboard by express-expose, whose escaping is for the javascript string context and is value-preserving by design, so the api's html escaping never applied to it. It is now escaped where it is interpolated. Adds test/unit-tests/plugins.compliance-hub.actions-escaping.js, which loads the real module in a sandbox and exercises the actual onReady builder. It pins both directions: a value carrying markup is neutralized, and an already-escaped api value is not double-escaped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ete confirmations
Backport of the master change, adjusted for this branch: the two populator
delete confirmations need different fixes here, because their localized strings
differ from master's.
Both dialogs substitute a name that was html-decoded on the way in, so the
escaping the api applied no longer held by the time it was rendered.
- the template delete confirmation carries no markup in any of the 26 locale
files, so its body is now text interpolation, which removes the sink
- the environment delete confirmation cannot do that on this branch. Its string
is "Are you sure you want to delete <b>{0}</b> environment?", so the body has
to stay v-html and the name is escaped where it is substituted instead. On
master the same string has no markup, which is why that branch converts the
template and this one does not.
Left the plugins plugin's dependency confirmation on v-html, as on master:
plugins.confirm carries a <br/><br/> in all translated locale files, and the
values interpolated into it are plugin titles from package metadata rather than
anything a dashboard account can write.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…S via app name) Backport of #7949 to release.24.05. The dashboard serialises the exposed countlyGlobal object into an inline <script> block. The serialiser only neutralised the exact sequence "</script>", but the HTML tokeniser also ends a script element at "</script >", "</script/>" and other whitespace/slash spellings, so an application name containing one broke out of the script block. An app admin of a single app could store such a name; any global admin who then loaded the dashboard (which lists every app) executed the attacker's markup in their own session, escalating an app-admin account to global-admin control. Escape every "<" as < in both serialisation paths (string values and object keys). < parses back to "<", so every value read from the exposed object is unchanged; verified by an eval round-trip that reproduces the input object identically and matches the previous serialiser output. Also render the active-app name with .text() instead of .html() in countly.template.js, an independent DOM sink for the same value. Reported through the security bug bounty programme (received 2026-08-17). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ent version Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 19, 2026
Closed
ar2rsawseen
commented
Aug 24, 2026
…itizer The component sets data-user-property-type on every personalization token, once when the token is created and again whenever the property type changes, and the allowlist did not name it. Every sanitize pass therefore dropped it, and the next editor change emitted innerHTML without it, persisting the lossier markup. Added. Nothing else in the token contract was missing: the other three data attributes, and the id, class and contenteditable the token needs, were all already there. The test derives the expectation rather than restating the list. It reads the allowlist out of the component and the attribute names out of the setAttribute calls in both the component and countly.models.js, which rebuilds tokens when a saved message is opened, and requires every one of them to survive. So the next attribute added to a token has to be allowlisted or the suite says so - which is the failure mode here, since nothing about a silently stripped attribute is loud. It also pins the other direction: no event handler attribute and nothing that can carry a url, and no element but the span. Worth knowing separately: the attribute is written and never read. Two setAttribute calls in this file are the only occurrences of data-user-property-type in countly-server, countly-platform and countly-enterprise-plugins - the property type is carried in the model (userPropertyDto[key].t), and getUserPropertyElement, which rebuilds a token from a stored message, does not set it. Allowlisting it costs nothing and is the conservative half of the review note; retiring it from the token contract is a frontend decision that does not belong in this PR.
The changelog is generated from PR and commit titles later, so an entry written by hand here is duplicated work at best. It is also the single worst file in this wave for conflicts: every merge to the base appends a line, which re-conflicts every open branch that also appends one. Eight of the sixteen conflicts across these security PRs today were this file and nothing else, and two of them came back within the hour. Only the lines this branch added are removed - the file is otherwise the base's, and the change here was purely additive, so nothing else moves.
Cookiezaurs
approved these changes
Aug 27, 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
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.
Backport of #7970 to
release.24.05. Consolidates the frontend HTML-sink hardening work into one PR; each change routes an attacker-influenceable value through the right encoder/sanitizer at the point it enters an HTML sink, leaving normal display unchanged. Replaces #7967, #7969, #7962, #7965 and #7950.countly.common.js).innerHTMLon the editor's contenteditable, allowing only the user-property token span and its attributes.<when serializingcountlyGlobalinto the inline page script; the active-app name is rendered with.text()instead of.html().Verified:
node --checkand eslint on all changed JS; compliance-hub escaping unit test passes (5/5).🤖 Generated with Claude Code