From 859fc4c511314b4a097bcd157e54a8f75f842184 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 19:34:56 +0300 Subject: [PATCH 1/2] fix(star-rating): check consent link destinations as urls (24.05) Backport of the master change. The api and drawer parts applied unchanged; the popup template differs on this branch and was hand-authored to match. Consent links are rendered into anchors in the drawer preview and the public popup, and nothing checked that a destination was a usable url. Both paths escape the value, which is no protection here: escaping touches < > and &, and `javascript:alert(1)` contains none of them, so it arrives unchanged and the browser runs it when the link is clicked. Checked at the widget endpoint (in the shared links preprocessor, which both create and edit run), in the drawer, and in the popup. The render-side checks are not redundant: widgets stored before this change still carry whatever was saved. Accepted: http(s), a root-relative path, or a fragment, matching what countlyCommon's onTagAttr already permits for an href elsewhere. Leading whitespace and control characters are stripped first, since a browser ignores them when resolving a url. Also escapes link labels before building the regular expression they are matched with, since an unescaped label is a pattern and one like `(a+)+$` causes catastrophic backtracking. 58 unit tests, all passing on this branch. Full unit suite 101 passing before, 159 after, same 5 pre-existing failures. eslint clean. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + plugins/star-rating/api/api.js | 59 +++++++- .../public/javascripts/countly.views.js | 46 +++++- .../public/templates/feedback-popup.html | 20 ++- ...plugins.star-rating.consent-link-scheme.js | 143 ++++++++++++++++++ 5 files changed, 258 insertions(+), 11 deletions(-) create mode 100644 test/unit-tests/plugins.star-rating.consent-link-scheme.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 7703fab7885..ed773036470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,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 Security Fixes: +- [star-rating] Consent link destinations are now checked as URLs, on save and again when rendered, so a link can only point at an http(s) url, a root-relative path or a fragment. HTML escaping never covered this, since a `javascript:` url contains no character that escaping touches. Link labels are also escaped before being used to build a regular expression - [compliance-hub] The consents table now returns a fixed set of fields; a projection supplied on the request is no longer used to widen the response beyond the consent columns - [dashboards] Widgets are no longer copied when the copying user has no access to the apps they reference, and widget app ids are validated on widget create and update - [hooks] Internal event hooks are now scoped to the apps the hook belongs to: app creation is a global-admin-only event, and remote-config, cohort, alert and hook-chaining events are only delivered when the event's app is one the hook is scoped to diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 4ca7d12cc21..5dd53740497 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -151,6 +151,37 @@ const widgetProperties = { } }; +//A consent link's destination is rendered into an anchor's href, in the widget preview in +//the dashboard and in the public popup. HTML escaping is applied to it in both places and +//is no protection here: `javascript:alert(1)` contains no HTML metacharacter, so it comes +//through escaping unchanged and the browser runs it when the link is clicked. The scheme +//has to be checked as a scheme. +// +//The accepted set matches what countlyCommon's onTagAttr already permits for an href +//elsewhere in the dashboard: http(s), a root-relative path, or a fragment. Being stricter +//than the rest of the dashboard would be a surprise, and a self-hosted install may +//reasonably point a consent link at a relative Terms page. +const SAFE_LINK_URL = /^(?:https?:\/\/|\/(?!\/)|#)/i; + +/** + * Whether a consent link destination is safe to put in an href. + * + * Leading whitespace and control characters are stripped before the test, because a + * browser ignores them when resolving a URL: "\tjavascript:alert(1)" and + * "java\nscript:alert(1)" both run. A protocol-relative "//host" is refused as well, + * since it is not a relative path. + * + * @param {string} value - the link destination as submitted + * @returns {boolean} true when the value may be used as an href + */ +function isSafeLinkUrl(value) { + if (typeof value !== "string") { + return false; + } + var candidate = value.replace(/[\u0000-\u0020]+/g, ""); + return SAFE_LINK_URL.test(candidate); +} + const widgetPropertyPreprocessors = { target_pages: function(targetPages) { try { @@ -174,17 +205,26 @@ const widgetPropertyPreprocessors = { } }, links: function(links) { + var parsed; try { - return JSON.parse(links); + parsed = JSON.parse(links); } catch (jsonParseError) { - if (Array.isArray(links)) { - return links; - } - else { - return []; - } + parsed = Array.isArray(links) ? links : []; } + //Both create and edit run every preprocessor, so this is the one place that sees + //every submitted link on both paths. A destination that is not a usable href is + //dropped rather than the whole request refused, so a widget still saves and the + //link simply has nowhere to point. + if (Array.isArray(parsed)) { + parsed.forEach(function(link) { + if (link && typeof link === "object" && typeof link.linkValue !== "undefined" && !isSafeLinkUrl(link.linkValue)) { + log.d("Dropped a consent link with an unusable destination: " + JSON.stringify(link.linkValue)); + link.linkValue = ""; + } + }); + } + return parsed; }, ratings_texts: function(ratingsTexts) { try { @@ -2019,4 +2059,9 @@ function uploadFile(myfile, id, callback) { } } }(exported)); + +//exposed for tests: the scheme check is the whole of this fix, so it is worth +//asserting directly rather than only through the widget endpoints +exported.isSafeLinkUrl = isSafeLinkUrl; + module.exports = exported; diff --git a/plugins/star-rating/frontend/public/javascripts/countly.views.js b/plugins/star-rating/frontend/public/javascripts/countly.views.js index 741fa23b596..977f654e185 100644 --- a/plugins/star-rating/frontend/public/javascripts/countly.views.js +++ b/plugins/star-rating/frontend/public/javascripts/countly.views.js @@ -1,4 +1,38 @@ /*global $, countlyReporting, countlyGlobal, CountlyHelpers, starRatingPlugin, app, jQuery, countlyPlugins, countlyCommon, CV, countlyVue, moment, countlyCohorts*/ + +/** + * Escape regular expression metacharacters in a literal. + * + * textValue is interpolated into a RegExp below. Unescaped, the value IS a pattern, + * so one like `(a+)+$` turns matching into catastrophic backtracking on a long + * enough finalText. + * + * @param {string} value - literal to be matched + * @returns {string} the literal, safe to embed in a pattern + */ +function escapeForRegExp(value) { + return String(value).replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * Whether a consent link destination is usable as an href. + * + * HTML escaping is no protection here: `javascript:alert(1)` contains no HTML + * metacharacter, so it survives escaping unchanged and runs when the link is clicked. + * The accepted set matches the widget endpoint and the public popup: http(s), a + * root-relative path, or a fragment. Leading whitespace and control characters are + * stripped first, because a browser ignores them when resolving a URL. + * + * @param {string} value - the stored link destination + * @returns {boolean} true when it may be used as an href + */ +function isSafeConsentLink(value) { + if (typeof value !== 'string') { + return false; + } + return /^(?:https?:\/\/|\/(?!\/)|#)/i.test(value.replace(/[\u0000-\u0020]+/g, '')); +} + (function() { var FEATURE_NAME = 'star_rating'; var CLY_X_INT = 'cly_x_int'; @@ -147,8 +181,16 @@ } else if (Array.isArray(links) && typeof finalText === 'string') { links.forEach(link => { - const regex = new RegExp(`\\b${link.textValue}\\b`, 'g'); - finalText = finalText.replace(regex, `${link.textValue}`); + //textValue goes into a RegExp, so its metacharacters have to be escaped: + //an unescaped value IS a pattern, and one like `(a+)+$` makes this hang + //on a long enough finalText. + const regex = new RegExp(`\\b${escapeForRegExp(link.textValue)}\\b`, 'g'); + //Refuse a destination that is not a usable href rather than trusting it + //because the endpoint now rejects it: widgets saved before that check + //still carry whatever was stored, and this drawer also renders values + //that have not been saved at all. + const href = isSafeConsentLink(link.linkValue) ? link.linkValue : ''; + finalText = finalText.replace(regex, `${link.textValue}`); }); } diff --git a/plugins/star-rating/frontend/public/templates/feedback-popup.html b/plugins/star-rating/frontend/public/templates/feedback-popup.html index b25ab2d36b9..713e1338e5a 100644 --- a/plugins/star-rating/frontend/public/templates/feedback-popup.html +++ b/plugins/star-rating/frontend/public/templates/feedback-popup.html @@ -259,6 +259,18 @@ var h = d.getHours(); var dow = d.getDay(); var symbol = 'emoji'; + //A consent link's destination is put into an anchor's href. encodeHtml below is no + //protection for that: `javascript:alert(1)` has no HTML metacharacter, so it comes + //through escaping unchanged and runs when the link is clicked. The accepted set + //matches the widget endpoint and the dashboard drawer: http(s), a root-relative + //path, or a fragment. Leading whitespace and control characters are stripped first, + //because a browser ignores them when resolving a url. + var isSafeLinkUrl = function (value) { + if (typeof value !== 'string') { + return false; + } + return /^(?:https?:\/\/|\/(?!\/)|#)/i.test(value.replace(/[\u0000-\u0020]+/g, '')); + }; var encodeHtml = function (html) { var div = document.createElement('div'); div.innerText = html; @@ -308,9 +320,13 @@ //sanitize the text and links finalText = encodeHtml(finalText); links.forEach(function (link) { - link.linkValue = encodeHtml(link.linkValue); + //refuse a destination that is not a usable href; widgets saved + //before the endpoint check still carry whatever was stored + link.linkValue = isSafeLinkUrl(link.linkValue) ? encodeHtml(link.linkValue) : ''; link.textValue = encodeHtml(link.textValue); - var regex = new RegExp('\\b' + link.textValue + '\\b', 'g'); + //textValue is a literal here, not a pattern: unescaped, a label + //like `(a+)+$` turns matching into catastrophic backtracking + var regex = new RegExp('\\b' + link.textValue.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\b', 'g'); finalText = finalText.replace(regex, '' + link.textValue + ''); }); } diff --git a/test/unit-tests/plugins.star-rating.consent-link-scheme.js b/test/unit-tests/plugins.star-rating.consent-link-scheme.js new file mode 100644 index 00000000000..03c178a58cd --- /dev/null +++ b/test/unit-tests/plugins.star-rating.consent-link-scheme.js @@ -0,0 +1,143 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); +var vm = require("vm"); + +// A consent link's destination is rendered into an anchor's href, in the widget drawer and +// in the public popup. HTML escaping is applied in both places and is no protection for +// this: `javascript:alert(1)` contains no HTML metacharacter, so it passes through +// unchanged and the browser runs it when the link is clicked. The scheme has to be checked +// as a scheme. +// +// There are three copies of that check, in three separate runtime contexts: the widget +// endpoint, the dashboard drawer, and the standalone popup page, which shares no code with +// the dashboard. They have to agree, or a value refused in one place renders in another, +// so every case below is asserted against all three. +describe("star-rating consent link destinations", function() { + var root = path.join(__dirname, "../.."); + + /** + * Lift a function out of a source file by name and make it callable. + * + * These files cannot be required here: the api pulls in plugin dependencies that are + * absent in a bare checkout, and the popup is an html template. The checks are pure, + * so they are extracted instead. + * + * @param {string} file - path relative to the repo root + * @param {string} needle - text just before the function's opening brace + * @param {string} name - identifier to return + * @returns {function} the extracted function + */ + function lift(file, needle, name) { + var src = fs.readFileSync(path.join(root, file), "utf8"); + var at = src.indexOf(needle); + if (at === -1) { + throw new Error("could not find " + needle + " in " + file + "; if it moved, update this test rather than deleting it"); + } + var open = src.indexOf("{", at); + var depth = 0, end = -1; + for (var i = open; i < src.length; i++) { + if (src[i] === "{") { + depth++; + } + else if (src[i] === "}") { + depth--; + if (depth === 0) { + end = i + 1; + break; + } + } + } + var decl = src.slice(at, end); + var sandbox = {module: {exports: {}}}; + vm.createContext(sandbox); + // the api version reads a const declared above it, so pull that in when present + var reLine = src.match(/const SAFE_LINK_URL = .*;/); + vm.runInContext((reLine ? reLine[0] + "\n" : "") + decl + "\nmodule.exports = " + name + ";", sandbox); + return sandbox.module.exports; + } + + var checks = { + "widget endpoint": lift("plugins/star-rating/api/api.js", "function isSafeLinkUrl", "isSafeLinkUrl"), + "dashboard drawer": lift("plugins/star-rating/frontend/public/javascripts/countly.views.js", "function isSafeConsentLink", "isSafeConsentLink"), + "public popup": lift("plugins/star-rating/frontend/public/templates/feedback-popup.html", "var isSafeLinkUrl = function", "isSafeLinkUrl") + }; + + var TAB = String.fromCharCode(9); + var NEWLINE = String.fromCharCode(10); + + var allowed = [ + ["an https url", "https://example.com/terms"], + ["an http url", "http://example.com"], + ["a root-relative path", "/terms"], + ["a fragment", "#terms"], + ["an https url with a query", "https://example.com/t?a=1&b=2"], + ["mixed case scheme", "HTTPS://example.com"] + ]; + + var refused = [ + ["a javascript url", "javascript:alert(document.domain)//"], + ["mixed case javascript", "JaVaScRiPt:alert(1)"], + ["javascript with leading spaces", " javascript:alert(1)"], + ["javascript split by a tab", "java" + TAB + "script:alert(1)"], + ["javascript split by a newline", "java" + NEWLINE + "script:alert(1)"], + ["a data url", "data:text/html,"], + ["a vbscript url", "vbscript:msgbox(1)"], + ["a protocol-relative url", "//evil.example.com"], + ["an empty string", ""], + ["a bare scheme name", "javascript:"], + ["some other scheme", "ftp://example.com"] + ]; + + Object.keys(checks).forEach(function(where) { + describe(where, function() { + allowed.forEach(function(c) { + it("allows " + c[0], function() { + checks[where](c[1]).should.equal(true); + }); + }); + refused.forEach(function(c) { + it("refuses " + c[0], function() { + checks[where](c[1]).should.equal(false); + }); + }); + it("refuses a value that is not a string", function() { + checks[where](undefined).should.equal(false); + checks[where](null).should.equal(false); + checks[where]({}).should.equal(false); + }); + }); + }); + + // textValue is interpolated into a RegExp in the drawer. Unescaped, the value IS a + // pattern rather than a literal, so a link label can turn matching into catastrophic + // backtracking. + describe("regex metacharacters in a link label", function() { + var escapeForRegExp = lift("plugins/star-rating/frontend/public/javascripts/countly.views.js", "function escapeForRegExp", "escapeForRegExp"); + + it("matches a metacharacter-laden label literally", function() { + var label = "(a+)+$"; + new RegExp(escapeForRegExp(label)).test(label).should.equal(true); + }); + + it("does not let a label behave as a pattern", function() { + new RegExp(escapeForRegExp("(a+)+$")).test("aaaaaaaaaaaaaaaa!").should.equal(false); + }); + + it("leaves an ordinary label untouched", function() { + escapeForRegExp("Terms and Conditions").should.equal("Terms and Conditions"); + }); + + it("still produces a valid pattern from a lone bracket or backslash", function() { + var built = true; + try { + new RegExp(escapeForRegExp("[")); + new RegExp(escapeForRegExp(String.fromCharCode(92))); + } + catch (e) { + built = false; + } + built.should.equal(true); + }); + }); +}); From 167764a938db0015eb6390b822a21fcf85d510d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 20:00:05 +0300 Subject: [PATCH 2/2] fix(star-rating): narrow consent link destinations to http(s) The surveys widget renders the same kind of consent link and already accepts http(s) only, falling back to about:blank and setting rel on the anchor. The two widgets are configured side by side and do the same job, so they should agree: a destination refused in one and rendered in the other is the surprising outcome, and it is the kind of gap that invites a second look at this later. A relative path is no loss. The popup is served from the Countly server, so "/terms" resolves against the server rather than against the site the widget is embedded in, which is not what anyone configuring it would intend. The renderers now fall back to about:blank rather than an empty href, which would have pointed the link back at the current page and reloaded it on click, and both anchors carry rel="noopener noreferrer". The drawer also escapes the href it builds: that string is handed to v-html, and a value that passes the scheme check can still carry a quote and close the attribute early. --- CHANGELOG.md | 2 +- plugins/star-rating/api/api.js | 23 ++++++----- .../public/javascripts/countly.views.js | 18 +++++--- .../public/templates/feedback-popup.html | 14 +++---- ...plugins.star-rating.consent-link-scheme.js | 41 +++++++++++++++++-- 5 files changed, 69 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed773036470..dd9ed8bce2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,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 Security Fixes: -- [star-rating] Consent link destinations are now checked as URLs, on save and again when rendered, so a link can only point at an http(s) url, a root-relative path or a fragment. HTML escaping never covered this, since a `javascript:` url contains no character that escaping touches. Link labels are also escaped before being used to build a regular expression +- [star-rating] Consent link destinations are now checked as URLs, on save and again when rendered, so a link can only point at an http(s) url. HTML escaping never covered this, since a `javascript:` url contains no character that escaping touches. Link labels are also escaped before being used to build a regular expression - [compliance-hub] The consents table now returns a fixed set of fields; a projection supplied on the request is no longer used to widen the response beyond the consent columns - [dashboards] Widgets are no longer copied when the copying user has no access to the apps they reference, and widget app ids are validated on widget create and update - [hooks] Internal event hooks are now scoped to the apps the hook belongs to: app creation is a global-admin-only event, and remote-config, cohort, alert and hook-chaining events are only delivered when the event's app is one the hook is scoped to diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 5dd53740497..aca4ee28f2f 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -157,19 +157,21 @@ const widgetProperties = { //through escaping unchanged and the browser runs it when the link is clicked. The scheme //has to be checked as a scheme. // -//The accepted set matches what countlyCommon's onTagAttr already permits for an href -//elsewhere in the dashboard: http(s), a root-relative path, or a fragment. Being stricter -//than the rest of the dashboard would be a surprise, and a self-hosted install may -//reasonably point a consent link at a relative Terms page. -const SAFE_LINK_URL = /^(?:https?:\/\/|\/(?!\/)|#)/i; +//Only http(s) is accepted, which is what the surveys widget already does for the same kind +//of consent link, in its countly.common.components.js. The two widgets do the same job and are +//configured side by side, so a destination refused in one and rendered in the other would be +//the surprising outcome. A relative path is no loss here: the popup is served from the Countly +//server, so "/terms" would resolve against the server rather than against the site the widget +//is embedded in. +const SAFE_LINK_URL = /^https?:\/\//i; /** * Whether a consent link destination is safe to put in an href. * - * Leading whitespace and control characters are stripped before the test, because a - * browser ignores them when resolving a URL: "\tjavascript:alert(1)" and - * "java\nscript:alert(1)" both run. A protocol-relative "//host" is refused as well, - * since it is not a relative path. + * The test is an allowlist anchored at the start of the trimmed value, so the usual ways of + * hiding a scheme fail it rather than having to be enumerated one by one: + * "\tjavascript:alert(1)", "java\nscript:alert(1)", "JaVaScRiPt:alert(1)" and a + * protocol-relative "//host" all miss `^https?://`. * * @param {string} value - the link destination as submitted * @returns {boolean} true when the value may be used as an href @@ -178,8 +180,7 @@ function isSafeLinkUrl(value) { if (typeof value !== "string") { return false; } - var candidate = value.replace(/[\u0000-\u0020]+/g, ""); - return SAFE_LINK_URL.test(candidate); + return SAFE_LINK_URL.test(value.trim()); } const widgetPropertyPreprocessors = { diff --git a/plugins/star-rating/frontend/public/javascripts/countly.views.js b/plugins/star-rating/frontend/public/javascripts/countly.views.js index 977f654e185..b6f326d389e 100644 --- a/plugins/star-rating/frontend/public/javascripts/countly.views.js +++ b/plugins/star-rating/frontend/public/javascripts/countly.views.js @@ -19,9 +19,10 @@ function escapeForRegExp(value) { * * HTML escaping is no protection here: `javascript:alert(1)` contains no HTML * metacharacter, so it survives escaping unchanged and runs when the link is clicked. - * The accepted set matches the widget endpoint and the public popup: http(s), a - * root-relative path, or a fragment. Leading whitespace and control characters are - * stripped first, because a browser ignores them when resolving a URL. + * Only http(s) is accepted, matching the widget endpoint, the public popup, and the surveys + * widget that renders the same kind of consent link. The test is anchored at the start of the + * trimmed value, so a scheme hidden behind whitespace or mixed case fails it rather than + * having to be enumerated. * * @param {string} value - the stored link destination * @returns {boolean} true when it may be used as an href @@ -30,7 +31,7 @@ function isSafeConsentLink(value) { if (typeof value !== 'string') { return false; } - return /^(?:https?:\/\/|\/(?!\/)|#)/i.test(value.replace(/[\u0000-\u0020]+/g, '')); + return /^https?:\/\//i.test(value.trim()); } (function() { @@ -189,8 +190,13 @@ function isSafeConsentLink(value) { //because the endpoint now rejects it: widgets saved before that check //still carry whatever was stored, and this drawer also renders values //that have not been saved at all. - const href = isSafeConsentLink(link.linkValue) ? link.linkValue : ''; - finalText = finalText.replace(regex, `${link.textValue}`); + //about:blank rather than an empty href, which would point the link back + //at the dashboard and reload it on click. + const href = isSafeConsentLink(link.linkValue) ? link.linkValue.trim() : 'about:blank'; + //This string is rendered with v-html, so escape the href: a value that + //passes the scheme check can still carry a quote and close the attribute. + const escHref = countlyCommon.encodeHtml(href); + finalText = finalText.replace(regex, `${link.textValue}`); }); } diff --git a/plugins/star-rating/frontend/public/templates/feedback-popup.html b/plugins/star-rating/frontend/public/templates/feedback-popup.html index 713e1338e5a..910120aa07e 100644 --- a/plugins/star-rating/frontend/public/templates/feedback-popup.html +++ b/plugins/star-rating/frontend/public/templates/feedback-popup.html @@ -261,15 +261,15 @@ var symbol = 'emoji'; //A consent link's destination is put into an anchor's href. encodeHtml below is no //protection for that: `javascript:alert(1)` has no HTML metacharacter, so it comes - //through escaping unchanged and runs when the link is clicked. The accepted set - //matches the widget endpoint and the dashboard drawer: http(s), a root-relative - //path, or a fragment. Leading whitespace and control characters are stripped first, - //because a browser ignores them when resolving a url. + //through escaping unchanged and runs when the link is clicked. Only http(s) is + //accepted, matching the widget endpoint, the dashboard drawer and the surveys widget + //that renders the same kind of consent link. The test is anchored at the start of the + //trimmed value, so a scheme hidden behind whitespace or mixed case fails it. var isSafeLinkUrl = function (value) { if (typeof value !== 'string') { return false; } - return /^(?:https?:\/\/|\/(?!\/)|#)/i.test(value.replace(/[\u0000-\u0020]+/g, '')); + return /^https?:\/\//i.test(value.trim()); }; var encodeHtml = function (html) { var div = document.createElement('div'); @@ -322,12 +322,12 @@ links.forEach(function (link) { //refuse a destination that is not a usable href; widgets saved //before the endpoint check still carry whatever was stored - link.linkValue = isSafeLinkUrl(link.linkValue) ? encodeHtml(link.linkValue) : ''; + link.linkValue = encodeHtml(isSafeLinkUrl(link.linkValue) ? link.linkValue.trim() : 'about:blank'); link.textValue = encodeHtml(link.textValue); //textValue is a literal here, not a pattern: unescaped, a label //like `(a+)+$` turns matching into catastrophic backtracking var regex = new RegExp('\\b' + link.textValue.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\b', 'g'); - finalText = finalText.replace(regex, '' + link.textValue + ''); + finalText = finalText.replace(regex, '' + link.textValue + ''); }); } if(consent){ diff --git a/test/unit-tests/plugins.star-rating.consent-link-scheme.js b/test/unit-tests/plugins.star-rating.consent-link-scheme.js index 03c178a58cd..4d5a11d748a 100644 --- a/test/unit-tests/plugins.star-rating.consent-link-scheme.js +++ b/test/unit-tests/plugins.star-rating.consent-link-scheme.js @@ -9,6 +9,11 @@ var vm = require("vm"); // unchanged and the browser runs it when the link is clicked. The scheme has to be checked // as a scheme. // +// The rule is http(s) only, which is what the surveys widget already applies to the same +// kind of consent link. A relative path and a fragment are refused along with everything +// else: the popup is served from the Countly server, so they would resolve against the +// server rather than the site the widget is embedded in. +// // There are three copies of that check, in three separate runtime contexts: the widget // endpoint, the dashboard drawer, and the standalone popup page, which shares no code with // the dashboard. They have to agree, or a value refused in one place renders in another, @@ -69,10 +74,9 @@ describe("star-rating consent link destinations", function() { var allowed = [ ["an https url", "https://example.com/terms"], ["an http url", "http://example.com"], - ["a root-relative path", "/terms"], - ["a fragment", "#terms"], ["an https url with a query", "https://example.com/t?a=1&b=2"], - ["mixed case scheme", "HTTPS://example.com"] + ["mixed case scheme", "HTTPS://example.com"], + ["an https url with surrounding whitespace", " https://example.com/terms "] ]; var refused = [ @@ -86,7 +90,10 @@ describe("star-rating consent link destinations", function() { ["a protocol-relative url", "//evil.example.com"], ["an empty string", ""], ["a bare scheme name", "javascript:"], - ["some other scheme", "ftp://example.com"] + ["some other scheme", "ftp://example.com"], + ["a mailto url", "mailto:someone@example.com"], + ["a root-relative path", "/terms"], + ["a fragment", "#terms"] ]; Object.keys(checks).forEach(function(where) { @@ -140,4 +147,30 @@ describe("star-rating consent link destinations", function() { built.should.equal(true); }); }); + + // The predicates above only decide true/false. What the render sites do with a false is + // the other half of the contract, and it is not reachable through the lifted functions: + // the destination must become about:blank rather than an empty href, which would point + // the link back at the page and reload it, and the anchor must carry rel. + describe("what the render sites do with a refused destination", function() { + var sites = [ + ["dashboard drawer", "plugins/star-rating/frontend/public/javascripts/countly.views.js"], + ["public popup", "plugins/star-rating/frontend/public/templates/feedback-popup.html"] + ]; + + sites.forEach(function(site) { + describe(site[0], function() { + var src = fs.readFileSync(path.join(root, site[1]), "utf8"); + + it("falls back to about:blank", function() { + src.indexOf("'about:blank'").should.be.above(-1); + }); + + it("sets rel on the anchor", function() { + src.indexOf('rel="noopener noreferrer"').should.be.above(-1); + }); + }); + }); + }); + });