-
Notifications
You must be signed in to change notification settings - Fork 983
security: encode and sanitize values at frontend HTML sinks #7970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ar2rsawseen
wants to merge
6
commits into
master
Choose a base branch
from
security/xss-sink-hardening
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a24ea05
security(core): HTML-encode the application name in the graph note to…
ar2rsawseen 937fbc8
security(push): sanitize message-editor content before it enters the …
ar2rsawseen a6a8dbf
[security][compliance-hub] escape interpolated values in the export h…
ar2rsawseen 9af2b50
[security][core] render populator confirm dialog bodies as text inste…
ar2rsawseen c387c01
security(core): escape "<" in the res.expose script island (stored XS…
ar2rsawseen d1fde1d
docs(changelog): add compliance-hub and populator security entries
ar2rsawseen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
132 changes: 132 additions & 0 deletions
132
test/unit-tests/plugins.compliance-hub.actions-escaping.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| var should = require("should"); | ||
| var fs = require("fs"); | ||
| var path = require("path"); | ||
| var vm = require("vm"); | ||
|
|
||
| // The export/purge history datatable builds an HTML string in onReady and the template renders it | ||
| // with v-html (plugins/compliance-hub/frontend/public/templates/exportHistory.html). Everything | ||
| // interpolated into that string therefore has to be HTML-safe already. | ||
| // | ||
| // Two opposite mistakes are possible and this file guards both directions: | ||
| // | ||
| // 1. NOT escaping the app name. It is read from countlyGlobal, which express-expose serializes | ||
| // into the dashboard's inline script island. That serializer escapes for the JavaScript string | ||
| // context only and is deliberately value-preserving, so the value arrives raw. An app admin can | ||
| // set their own app's name, and a global admin's dashboard lists every app, so an unescaped name | ||
| // is a stored cross-user XSS. | ||
| // 2. Escaping the values that came from the API. Those already went through | ||
| // common.escape_html_entities in common.returnOutput, so escaping them again would render the | ||
| // entities literally in the UI. | ||
| var SRC = path.resolve(__dirname, "../../plugins/compliance-hub/frontend/public/javascripts/countly.models.js"); | ||
|
|
||
| /** | ||
| * Load countly.models.js in a sandbox and hand back the onReady callbacks it registers, | ||
| * keyed by data-table name. | ||
| * @param {object} apps - the countlyGlobal.apps map the module should see | ||
| * @returns {object} map of resource name to its onReady function | ||
| */ | ||
| function loadResources(apps) { | ||
| var resources = {}; | ||
| var noop = function() {}; | ||
| // matches countlyCommon.encodeHtml, which is `div.innerText = x; return div.innerHTML`: | ||
| // the text-node serializer escapes &, < and > and leaves quotes alone. | ||
| var encodeHtml = function(html) { | ||
| return (html + "").replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">"); | ||
| }; | ||
| var sandbox = { | ||
| window: {}, | ||
| countlyCommon: { | ||
| encodeHtml: encodeHtml, | ||
| formatTimeAgoText: function() { | ||
| return { text: "just now" }; | ||
| }, | ||
| getDescendantProp: noop, | ||
| API_PARTS: { data: { r: "/o" } }, | ||
| ACTIVE_APP_ID: "5f1a2b3c4d5e6f0011223344", | ||
| periodObj: {}, | ||
| getPeriodForAjax: function() { | ||
| return "30days"; | ||
| } | ||
| }, | ||
| CountlyHelpers: { createMetricModel: noop }, | ||
| jQuery: { i18n: { map: { "systemlogs.for-app": "For app", "systemlogs.for-appuser": "For app user", "systemlogs.action.export": "Data exported" } } }, | ||
| CV: { | ||
| i18n: function(k) { | ||
| return k; | ||
| } | ||
| }, | ||
| countlyGlobal: { apps: apps }, | ||
| countlyTaskManager: {}, | ||
| countlyVue: { | ||
| vuex: { | ||
| ServerDataTable: function(name, cfg) { | ||
| resources[name] = cfg.onReady; | ||
| return { name: name }; | ||
| }, | ||
| Module: function() { | ||
| return {}; | ||
| }, | ||
| MutationsFor: noop, | ||
| ActionsFor: noop | ||
| } | ||
| } | ||
| }; | ||
| sandbox.global = sandbox; | ||
| vm.createContext(sandbox); | ||
| vm.runInContext(fs.readFileSync(SRC, "utf8"), sandbox, { filename: SRC }); | ||
| return resources; | ||
| } | ||
|
|
||
| describe("compliance-hub export history actions escaping", function() { | ||
| var APP_ID = "5f1a2b3c4d5e6f0011223344"; | ||
|
|
||
| /** | ||
| * Run the export-history onReady over a single row. | ||
| * @param {string} appName - the app name countlyGlobal should carry | ||
| * @param {object} i - the row's "i" payload | ||
| * @returns {string} the built actions HTML | ||
| */ | ||
| function actionsFor(appName, i) { | ||
| var apps = {}; | ||
| apps[APP_ID] = { name: appName }; | ||
| var onReady = loadResources(apps).exportHistoryDataResource; | ||
| should.exist(onReady); | ||
| var rows = onReady({}, [{ a: "export", ts: 0, i: i || { app_id: APP_ID } }]); | ||
| return rows[0].actions; | ||
| } | ||
|
|
||
| it("escapes an app name that carries a tag", function(done) { | ||
| var actions = actionsFor('<img src=x onerror="alert(1)">'); | ||
| actions.indexOf("<img").should.equal(-1); | ||
| actions.should.containEql("<img src=x onerror="alert(1)">".replace(/"/g, '"')); | ||
| done(); | ||
| }); | ||
|
|
||
| it("escapes an app name that closes the surrounding tag", function(done) { | ||
| var actions = actionsFor("</p><script>alert(1)</script>"); | ||
| actions.indexOf("<script").should.equal(-1); | ||
| actions.indexOf("</script>").should.equal(-1); | ||
| done(); | ||
| }); | ||
|
|
||
| it("leaves no raw angle bracket from the app name", function(done) { | ||
| var actions = actionsFor("<svg onload=alert(1)>"); | ||
| // the only markup left must be the <p> wrappers this builder emits itself | ||
| actions.replace(/<\/?p[^>]*>/g, "").indexOf("<").should.equal(-1); | ||
| done(); | ||
| }); | ||
|
|
||
| it("keeps an ordinary app name readable", function(done) { | ||
| var actions = actionsFor("My Application"); | ||
| actions.should.containEql("For app: My Application"); | ||
| done(); | ||
| }); | ||
|
|
||
| it("does not double-escape values the API already escaped", function(done) { | ||
| // returnOutput turns ' into '; escaping again would surface "&#39;" in the UI | ||
| var actions = actionsFor("My Application", { app_id: APP_ID, appuser_id: "user's-id" }); | ||
| actions.should.containEql("user's-id"); | ||
| actions.indexOf("&#39;").should.equal(-1); | ||
| done(); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Preserve the token's type attribute
The same component creates each personalization span with
data-user-property-typeand updates it when the user selects a property type, but this allowlist omits that attribute. Everyreset()therefore strips the type from stored token markup; the next editor change emitsinnerHTMLwithout it and can persist the lossy representation. Includedata-user-property-typein the allowed span attributes (or remove the attribute from the token contract everywhere if it is truly obsolete).