security: encode and sanitize values at frontend HTML sinks (24.05) - #7971
security: encode and sanitize values at frontend HTML sinks (24.05)#7971ar2rsawseen wants to merge 8 commits into
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>
| // text, so a stored message cannot introduce active markup when the editor is populated. | ||
| var PUSH_MESSAGE_EDITOR_XSS_OPTIONS = { | ||
| whiteList: { | ||
| span: ["class", "id", "contenteditable", "data-user-property-label", "data-user-property-value", "data-user-property-fallback"] |
There was a problem hiding this comment.
[P2] Preserve the token's type attribute
The same component creates each personalization span with data-user-property-type and updates it when the user selects a property type, but this allowlist omits that attribute. Every reset() therefore strips the type from stored token markup; the next editor change emits innerHTML without it and can persist the lossy representation. Include data-user-property-type in the allowed span attributes (or remove the attribute from the token contract everywhere if it is truly obsolete).
There was a problem hiding this comment.
Confirmed and added in 9c24332.
The component sets it in two places — on creation (addEmptyUserProperty) and on every type change (setUserPropertyValue) — and the allowlist named the other three data attributes but not this one, so each sanitize pass dropped it and the following innerHTML emit persisted the lossier markup.
Checked the rest of the token contract while I was there: data-user-property-label, -value and -fallback, plus id, class and contenteditable, were all already allowed. Nothing else was missing.
On the test: rather than restating the list, it derives the expectation from the component. It reads the allowlist out of the source and the attribute names out of the setAttribute calls — in the component and in countly.models.js, whose getUserPropertyElement rebuilds tokens when a saved message is opened and goes through the same sanitizer — then 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 matters because a silently stripped attribute fails quietly. It pins the other direction too: no on* handler, nothing that can carry a URL, no element but the span. It fails against the previous state with data-user-property-type is set on a token but sanitized away.
One thing worth knowing, on your "or remove it everywhere if it is truly obsolete". I checked: it is written and never read. Those two setAttribute calls are the only occurrences of data-user-property-type in countly-server, countly-platform and countly-enterprise-plugins — no getAttribute, no CSS selector, no template. The property type is carried in the model (userPropertyDto[key].t), and getUserPropertyElement does not set the attribute at all, so a message reopened from storage already lacks it.
I still took the first option. Allowlisting a data- attribute costs nothing and cannot execute, whereas retiring an attribute from the token contract is a frontend decision that does not belong in an XSS-hardening PR. Flagging it here so it can be retired deliberately if you want it gone.
…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.
1 participant
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