security: encode and sanitize values at frontend HTML sinks - #7970
Open
ar2rsawseen wants to merge 13 commits into
Open
security: encode and sanitize values at frontend HTML sinks#7970ar2rsawseen wants to merge 13 commits into
ar2rsawseen wants to merge 13 commits into
Conversation
…oltip The graph-note hover tooltip builds its content as an HTML string that includes the application name from countlyGlobal, whose values are raw at runtime, and renders it via tipsy html:true. Encode the application name with countlyCommon.encodeHtml so it renders as text; the other values in the tooltip are API-encoded or i18n. Display is 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 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>
…ad of html
The populator template and environment delete confirmations rendered their
body with v-html, but the value bound there is a localized sentence with a name
substituted into a {0} placeholder. Both names are html-decoded on the way in,
so the escaping the api applied no longer held by the time they were rendered.
Text interpolation removes the sink instead of filtering what reaches it.
Checked every assignment to this dialog object and every populator locale file
that supplies these strings, in all 26 languages present: none contain tags, so
nothing renders differently.
Left the plugins plugin's dependency confirmation on v-html deliberately. Its
strings do carry markup: plugins.confirm has a <br/><br/> in all 24 translated
locale files, even though the default locale no longer has it. The values
interpolated into that one 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) The dashboard serialises the exposed countlyGlobal object into an inline <script> block (dashboard.html: <script><%- javascript %></script>). 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 of those 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 (the primitive branch, replacing the exact-match "</script>"/"<!--" replaces) and object keys (escape_js_string). < parses back to "<", so every value read from the exposed object is unchanged. This continues the value-preserving direction of the previous fix (aa33b31, which removed the old HTML-entity escape_html that corrupted values); it does not reintroduce entity escaping. Also render the active-app name with .text() instead of .html() in countly.template.js (line 2406), an independent DOM sink for the same value. Verified: no raw "<" survives for any breakout spelling in values or keys; and an eval round-trip of the serialiser output reproduces the input object byte-for-byte and matches the previous serialiser's output (no dashboard-visible change). Reported through the security bug bounty programme (received 2026-08-17). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 19, 2026
Closed
ar2rsawseen
commented
Aug 24, 2026
# Conflicts: # CHANGELOG.md
…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.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
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.
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 is placed into an HTML sink, leaving normal display unchanged. Replaces #7966, #7968, #7961, #7954 and #7949.
countly.common.js).innerHTMLon the editor's contenteditable, allowing only the user-property token span and its attributes (countly.views.component.common.js).test/unit-tests/plugins.compliance-hub.actions-escaping.js) guarding both directions.environment_detail.html,populator.html).<is escaped when serializing the exposedcountlyGlobalobject into the inline page script, and the active-app name is rendered with.text()instead of.html()(express-expose.js,countly.template.js).Verified:
node --checkand eslint on all changed JS; the compliance-hub escaping unit test passes (5/5).🤖 Generated with Claude Code