From d37336f255b3c79ada9f4dac062792b783673691 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 13:30:36 +0300 Subject: [PATCH] fix(dashboard): show chart tooltip labels as text, and decode keys like the api does Two related things, both about a value ending up on screen as what it actually is. sanitizeHtml, used for every value interpolated into a chart tooltip template, encoded its input and then immediately unescaped it. Those two are inverses, so it returned the input unchanged and a label containing markup went into the template as markup rather than being shown as text. That was not the original intent. The call sites escaped correctly until the unescape was added, to stop Countly's own key substitutions ($ for a leading $, . for dots) from being shown to the user as literal character references. Both goals hold if the value is normalised first and escaped last, so that is what it now does: undo the html escaping the api applies, undo the key substitutions, then escape once. A label reads as its real text and is never treated as markup. The second part came out of the first. There are two implementations of the key substitution pair, the api one that produces every value in the database and the dashboard one that consumes them, and they had drifted. The dashboard decoder undid only $ and . while the api encoder also substitutes NUL and the api decoder also accepts the url encoded forms, so ▯, &#36; and &#46; reached callers still encoded. Its own comment already promised the url encoded forms, so this looks like an incomplete copy rather than a decision. It went unnoticed because the result is usually interpolated into html, where the browser resolves the leftovers anyway; it shows through anywhere that is not html. All 25 callers of the dashboard decoder consume api-produced data, and the dashboard encoder has no callers at all, so the pair is now aligned with the api in both directions. One visible consequence: a value containing NUL now shows the character itself, which is invisible, where it previously showed the placeholder glyph the browser resolved ▯ into. NUL in a key or segment value is pathological, and the alternative was leaving one of the two decoders knowingly incomplete. Tests: 13 over the tooltip helper, 4 of which fail without this change, and 30 asserting the two substitution pairs agree on behaviour rather than on source text, 6 of which fail without it. The suite has no DOM, so encodeHtml, which is implemented with innerText, is substituted by its documented effect on element content; the composition order and the decode step are the real source, lifted out of the files. A key that already looks encoded is not round-trippable, which is a property of the scheme rather than of either implementation, so that is asserted as the shared behaviour it is instead of being asserted away. Full unit suite: 185 passing before, 228 after, same 2 pre-existing failures (Countly Request, network dependent). eslint clean. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + .../javascripts/countly/countly.common.js | 19 ++- .../javascripts/countly/vue/components/vis.js | 16 +- .../frontend.api.key-escaping-parity.js | 136 ++++++++++++++++ test/unit-tests/frontend.vis.tooltip-label.js | 152 ++++++++++++++++++ 5 files changed, 318 insertions(+), 6 deletions(-) create mode 100644 test/unit-tests/frontend.api.key-escaping-parity.js create mode 100644 test/unit-tests/frontend.vis.tooltip-label.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ace8a79ae..10eda89f7ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ Fixes: - [hooks] Internal event hooks are validated on save: an unknown event type is rejected, and an event that names a cohort, hook or alert must name one belonging to the hook's own apps - [events] Fix sum chart tooltip displaying the raw floating-point value instead of a rounded number +- [core] Chart tooltips now show label text as text, so a label is never treated as markup, and the dashboard undoes the same key substitutions the api applies (`$`, `.`, `▯` and their url encoded forms) instead of only two of them - [events] Fixed event descriptions (and custom names / count-sum-dur labels) not showing on the Events page for events whose key contains special characters (`.`, `$`, `\`, `&`, `<`, `>`, `"`, `'`) Enterprise Fixes: diff --git a/frontend/express/public/javascripts/countly/countly.common.js b/frontend/express/public/javascripts/countly/countly.common.js index 5c3ebc5297c..5779daf7f61 100644 --- a/frontend/express/public/javascripts/countly/countly.common.js +++ b/frontend/express/public/javascripts/countly/countly.common.js @@ -311,23 +311,34 @@ }; /** - * Encode value to be passed to db as key, encoding $ symbol to $ if it is first and all . (dot) symbols to . in the string + * Encode value to be passed to db as key, encoding $ symbol to $ if it is first, all . (dot) symbols to . and NUL to ▯ in the string + * + * Mirrors the encoder in api/lib/countly.common.js. Keys are substituted because + * mongo will not accept $ or . in a key name, and the two encoders have to agree + * on the substitution set or the decoder below cannot undo all of it. * @memberof countlyCommon * @param {string} str - value to encode * @returns {string} encoded string */ countlyCommon.encode = function(str) { - return str.replace(/^\$/g, "$").replace(/\./g, '.'); + return str.replace(/^\$/g, "$").replace(/\./g, '.').replace(/\u0000/g, "▯"); }; /** - * Decode value from db, decoding first $ to $ and all . to . (dots). Decodes also url encoded values as &#36;. + * Decode value from db, decoding first $ to $, all . to . (dots) and ▯ back to NUL. Decodes also url encoded values as &#36; and &#46;. + * + * What this receives was encoded by the api, not by countlyCommon.encode above, so + * the set it has to undo is the api's. It used to handle only $ and . even though + * this comment already promised the url encoded forms, so ▯, &#36; and + * &#46; reached callers still encoded. That went unnoticed because the result + * is usually interpolated into html, where the browser resolves the leftovers as + * character references anyway; it shows through anywhere that is not html. * @memberof countlyCommon * @param {string} str - value to decode * @returns {string} decoded string */ countlyCommon.decode = function(str) { - return str.replace(/^$/g, "$").replace(/./g, '.'); + return str.replace(/^$/g, "$").replace(/^&#36;/g, '$').replace(/./g, '.').replace(/&#46;/g, '.').replace(/▯/g, '\u0000'); }; /** diff --git a/frontend/express/public/javascripts/countly/vue/components/vis.js b/frontend/express/public/javascripts/countly/vue/components/vis.js index 45b7ce503d5..b80c4edcdaf 100644 --- a/frontend/express/public/javascripts/countly/vue/components/vis.js +++ b/frontend/express/public/javascripts/countly/vue/components/vis.js @@ -1278,10 +1278,22 @@ return options; }, + //Tooltip templates are built by concatenating into an html string, so a value + //interpolated into one has to be escaped, and escaping has to be the last + //thing done to it. This used to encode and then immediately unescape, which + //returns the input unchanged, so a label containing markup was passed through + //as markup. That was not the original intent: the sites below escaped + //correctly until the unescape was added to stop Countly's own key escaping + //($ for a leading $, . for dots) from being shown to the user as + //literal character references. + // + //Both are satisfied by normalising first and escaping once: undo the html + //escaping the api applies, undo the key substitutions mongo forces on us, + //then escape. A label reads as its real text and can no longer open a tag. sanitizeHtml: function(value) { if (value) { - value = countlyCommon.encodeHtml(value); - return countlyCommon.unescapeHtml(value); + var text = countlyCommon.unescapeHtml(String(value)); + return countlyCommon.encodeHtml(countlyCommon.decode(text)); } return value; } diff --git a/test/unit-tests/frontend.api.key-escaping-parity.js b/test/unit-tests/frontend.api.key-escaping-parity.js new file mode 100644 index 00000000000..edd9f2bbbef --- /dev/null +++ b/test/unit-tests/frontend.api.key-escaping-parity.js @@ -0,0 +1,136 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); +var apiCommon = require("../../api/lib/countly.common.js"); + +// Mongo will not accept $ or . in a key, so keys are substituted on the way in and undone +// on the way out. There are two implementations of that pair: the api one, which produces +// every value in the database, and the dashboard one, which consumes them. +// +// They had drifted. The dashboard decoder undid only $ and . while the api encoder also +// substitutes NUL, and the api decoder also accepts the url encoded forms, so ▯, +// &#36; and &#46; reached the dashboard still encoded. That normally hides itself, +// because the result is interpolated into html and the browser resolves the leftovers as +// character references, but it surfaces anywhere that is not html, and it made a tooltip +// sanitizer look like it needed to undo html escaping when it did not. +// +// This asserts the two agree on behaviour rather than on source text, so reformatting one +// of them does not fail the test but changing what either substitutes does. +describe("key escaping parity between the api and the dashboard", function() { + var frontendCommonPath = path.join(__dirname, "../../frontend/express/public/javascripts/countly/countly.common.js"); + + /** + * Lift a single function body out of a source file and make it callable. + * + * The dashboard file cannot be required here: it expects a browser. Only these two + * functions are needed and both are pure string transforms, so they are extracted + * rather than pulling in a DOM implementation for the whole suite. + * + * @param {string} src - file contents + * @param {string} marker - text that immediately precedes the function's brace + * @returns {function} the extracted function + */ + function liftFunction(src, marker) { + var at = src.indexOf(marker); + if (at === -1) { + throw new Error("could not find " + marker + "; if it was renamed, update this test rather than deleting it"); + } + var open = src.indexOf("{", at); + var depth = 0; + for (var i = open; i < src.length; i++) { + if (src[i] === "{") { + depth++; + } + else if (src[i] === "}") { + depth--; + if (depth === 0) { + /*eslint-disable no-new-func*/ + return new Function("str", "return (function(str)" + src.slice(open, i + 1) + ")(str);"); + /*eslint-enable no-new-func*/ + } + } + } + throw new Error("unbalanced braces after " + marker); + } + + var frontendSrc = fs.readFileSync(frontendCommonPath, "utf8"); + var frontendEncode = liftFunction(frontendSrc, "countlyCommon.encode = function"); + var frontendDecode = liftFunction(frontendSrc, "countlyCommon.decode = function"); + + var NUL = String.fromCharCode(0); + + var keys = [ + "plain_key", + "price.usd", + "$price", + "a.b.c", + "$", + ".", + "x" + NUL + "y", + "mixed$.value", + "trailing.", + "" + ]; + + // A key whose text already looks like a substitution is not round-trippable, since + // nothing distinguishes it from an encoded dot. Kept out of the round trip list and + // asserted on its own below: it is a property of the scheme, shared by both + // implementations, not something either side gets wrong. + var LOOKS_ENCODED = "already.encoded"; + + describe("encode", function() { + keys.forEach(function(k) { + it("agrees with the api for " + JSON.stringify(k), function() { + frontendEncode(k).should.equal(apiCommon.encode(k)); + }); + }); + }); + + describe("decode", function() { + var encoded = [ + "price.usd", + "$price", + "price&#46;usd", + "&#36;price", + "x▯y", + "nothing_to_undo", + "" + ]; + encoded.forEach(function(e) { + it("agrees with the api for " + JSON.stringify(e), function() { + frontendDecode(e).should.equal(apiCommon.decode(e)); + }); + }); + }); + + describe("round trip", function() { + keys.forEach(function(k) { + it("returns the original key for " + JSON.stringify(k), function() { + // the substitutions exist to survive a trip through mongo, so encoding + // and decoding has to be lossless on both sides + frontendDecode(frontendEncode(k)).should.equal(k); + apiCommon.decode(apiCommon.encode(k)).should.equal(k); + }); + }); + }); + + describe("a key that already looks encoded", function() { + it("is not round-trippable, and both sides are wrong about it identically", function() { + // documenting the limitation rather than asserting it away: the point of this + // suite is that the two implementations agree, and here they do + frontendEncode(LOOKS_ENCODED).should.equal(apiCommon.encode(LOOKS_ENCODED)); + frontendDecode(frontendEncode(LOOKS_ENCODED)).should.equal("already.encoded"); + apiCommon.decode(apiCommon.encode(LOOKS_ENCODED)).should.equal("already.encoded"); + }); + }); + + describe("the substitutions the dashboard used to leave behind", function() { + it("undoes the NUL placeholder", function() { + frontendDecode("x▯y").should.equal("x" + NUL + "y"); + }); + it("undoes the url encoded dollar and dot forms", function() { + frontendDecode("&#36;price").should.equal("$price"); + frontendDecode("price&#46;usd").should.equal("price.usd"); + }); + }); +}); diff --git a/test/unit-tests/frontend.vis.tooltip-label.js b/test/unit-tests/frontend.vis.tooltip-label.js new file mode 100644 index 00000000000..dd89d29042e --- /dev/null +++ b/test/unit-tests/frontend.vis.tooltip-label.js @@ -0,0 +1,152 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); + +// Chart tooltips are built by concatenating values into an html string, so whatever is +// interpolated has to be escaped, and the escaping has to be the last thing done to it. +// +// sanitizeHtml used to encode and then immediately unescape, which returns the input +// unchanged, so a label containing markup went into the template as markup. The order is +// the whole point, which is what this pins down. +// +// The suite has no DOM, and countlyCommon.encodeHtml is implemented as +// `div.innerText = value; return div.innerHTML`. Its documented effect on element content +// is to escape & < >, so that is substituted here. Everything else under test, the +// composition order and the decode step, is the real source lifted out of the files. +describe("chart tooltip label rendering", function() { + var visPath = path.join(__dirname, "../../frontend/express/public/javascripts/countly/vue/components/vis.js"); + var commonPath = path.join(__dirname, "../../frontend/express/public/javascripts/countly/countly.common.js"); + + /** + * Lift a function body out of a source file and make it callable. + * @param {string} src - file contents + * @param {string} marker - text immediately preceding the function's brace + * @param {string} arg - the function's parameter name + * @returns {function} the extracted function + */ + function lift(src, marker, arg) { + var at = src.indexOf(marker); + if (at === -1) { + throw new Error("could not find " + marker + "; if it moved, update this test rather than deleting it"); + } + var open = src.indexOf("{", at); + var depth = 0; + for (var i = open; i < src.length; i++) { + if (src[i] === "{") { + depth++; + } + else if (src[i] === "}") { + depth--; + if (depth === 0) { + /*eslint-disable no-new-func*/ + return new Function("countlyCommon", arg, + "return (function(" + arg + ")" + src.slice(open, i + 1) + ")(" + arg + ");"); + /*eslint-enable no-new-func*/ + } + } + } + throw new Error("unbalanced braces after " + marker); + } + + var visSrc = fs.readFileSync(visPath, "utf8"); + var commonSrc = fs.readFileSync(commonPath, "utf8"); + + var rawSanitize = lift(visSrc, "sanitizeHtml: function", "value"); + var rawDecode = lift(commonSrc, "countlyCommon.decode = function", "str"); + var rawUnescape = lift(commonSrc, "countlyCommon.unescapeHtml = function", "htmlStr"); + + var countlyCommon = { + decode: function(s) { + return rawDecode(null, s); + }, + unescapeHtml: function(s) { + return rawUnescape(null, s); + }, + encodeHtml: function(h) { + return String(h).replace(/&/g, "&").replace(//g, ">"); + } + }; + + /** + * Call the lifted sanitizeHtml with the stubbed countlyCommon. + * @param {any} value - the label value + * @returns {any} sanitized value + */ + function sanitize(value) { + return rawSanitize(countlyCommon, value); + } + + /** + * Whether an html string would open a tag when parsed as element content. + * + * A character reference is resolved into text during tokenization, so only a literal + * `<` followed by a name or a slash can start one. + * + * @param {string} html - the html string + * @returns {boolean} true when the string contains a tag + */ + function opensATag(html) { + return (/<[a-zA-Z/]/).test(String(html)); + } + + /** + * What a browser shows for an html string in element content. + * @param {string} html - the html string + * @returns {string} the visible text + */ + function shows(html) { + // one left-to-right pass, so a reference produced by decoding an earlier one is + // not decoded again: "&lt;" has to come out as "<", not as "<" + var named = {lt: "<", gt: ">", quot: "\"", amp: "&", apos: "'"}; + return String(html).replace(/&(lt|gt|quot|amp|apos|#\d+);?/g, function(match, entity) { + if (entity.charAt(0) === "#") { + return String.fromCharCode(parseInt(entity.slice(1), 10)); + } + return named[entity]; + }); + } + + describe("values that must not become markup", function() { + [ + ["a decoded segment value, which is how the events chart supplies them", ""], + ["a value still carrying the api's escaping", "<img src=x onerror=alert(1)>"], + ["a closing tag", ""], + ["an unquoted attribute payload", ""] + ].forEach(function(c) { + it("does not open a tag for " + c[0], function() { + opensATag(sanitize(c[1])).should.equal(false); + }); + }); + + it("shows the markup to the user as text instead of running it", function() { + shows(sanitize("")).should.equal(""); + }); + }); + + describe("values that must still read normally", function() { + [ + ["a dotted key", "price.usd", "price.usd"], + ["a dollar-prefixed key", "$price", "$price"], + ["a url encoded dotted key", "price&#46;usd", "price.usd"], + ["an ampersand from the api", "A & B", "A & B"], + ["quotes from the api", "say "hi"", "say \"hi\""], + ["plain text", "checkout_button", "checkout_button"] + ].forEach(function(c) { + it("shows " + c[0] + " as " + JSON.stringify(c[2]), function() { + shows(sanitize(c[1])).should.equal(c[2]); + }); + }); + }); + + describe("values that are not strings", function() { + it("passes falsy values straight through, as before", function() { + (sanitize(0) === 0).should.equal(true); + (sanitize("") === "").should.equal(true); + (sanitize(null) === null).should.equal(true); + (sanitize(undefined) === undefined).should.equal(true); + }); + it("returns a number as a string, as before, since callers isNaN the result", function() { + sanitize(1234).should.equal("1234"); + }); + }); +});