diff --git a/.eslintrc.json b/.eslintrc.json index 162dc7b5913..bbc1f0b68b8 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -356,6 +356,13 @@ ] }, "overrides": [ + { + "files": ["api/**/*.js", "plugins/*/api/**/*.js", "frontend/express/public/javascripts/countly/**/*.js", "plugins/*/frontend/**/*.js"], + "excludedFiles": ["**/tests.js", "**/tests/**"], + "rules": { + "no-prototype-pollution-sink": "error" + } + }, { "files": [ "plugins/content/frontend/vite.config.js", diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e5165e08ab7..91a93a04688 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -115,9 +115,12 @@ jobs: - name: ESLint shell: bash + # --rulesdir loads the in-repo rules in bin/eslint-rules. Grunt and lint-staged + # pass it too; without it here, eslint 8 fails every file matched by the + # no-prototype-pollution-sink override with "Definition for rule ... was not found". run: | npm install eslint@8.57.0 eslint-plugin-vue@9.31.0 @stylistic/eslint-plugin@2.11.0 - npx eslint . + npx eslint . --rulesdir bin/eslint-rules - name: Check for any external web resources shell: bash diff --git a/Gruntfile.js b/Gruntfile.js index 4c7c1a98986..2a7d9862d66 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -3,7 +3,11 @@ module.exports = function(grunt) { grunt.initConfig({ eslint: { options: { - configFile: './.eslintrc.json' + configFile: './.eslintrc.json', + //grunt-eslint forwards options to new ESLint(), and rulePaths is a valid + //eslint 8 option, so the local rules in bin/eslint-rules resolve by bare + //name without publishing a plugin. Keep in step with --rulesdir below. + rulePaths: ['./bin/eslint-rules'] }, target: ['./'] }, diff --git a/api/api.js b/api/api.js index 2b250e1e94b..74bdf40cb1c 100644 --- a/api/api.js +++ b/api/api.js @@ -532,6 +532,11 @@ function handleRequest(req, res) { form.parse(req, (err, fields, files) => { //handle bakcwards compatability with formiddble v1 for (let i in files) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (i === "__proto__" || i === "constructor" || i === "prototype") { + continue; + } if (files[i].filepath) { files[i].path = files[i].filepath; } diff --git a/api/jobs/topEvents.js b/api/jobs/topEvents.js index 59134345f8a..c2ec81809e6 100644 --- a/api/jobs/topEvents.js +++ b/api/jobs/topEvents.js @@ -230,6 +230,11 @@ class TopEventsJob extends job.Job { } for (var event in data) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (event === "__proto__" || event === "constructor" || event === "prototype") { + continue; + } //Calculating trend var trend = countlyCommon.getPercentChange(data[event].data.count["prev-total"], data[event].data.count.total); data[event].data.count.change = trend.percent; diff --git a/api/lib/countly.model.js b/api/lib/countly.model.js index 16796d9c13a..061fb86ad82 100644 --- a/api/lib/countly.model.js +++ b/api/lib/countly.model.js @@ -547,6 +547,11 @@ countlyModel.create = function(fetchValue) { return obj; }, periodObject); for (let i in data) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (i === "__proto__" || i === "constructor" || i === "prototype") { + continue; + } if (sparkLines[i]) { data[i].sparkline = sparkLines[i].split(",").map(function(item) { return parseInt(item); diff --git a/api/parts/data/events.js b/api/parts/data/events.js index 284b26c518e..5986c11caf0 100644 --- a/api/parts/data/events.js +++ b/api/parts/data/events.js @@ -330,7 +330,7 @@ function processEvents(appEvents, appSegments, appSgValues, params, omitted_segm continue; } //skip keys that map to object prototype members when used as field names - if (segKey === "__proto__" || segKey === "constructor" || segKey === "prototype") { + if (common.isForbiddenFieldName(segKey)) { continue; } @@ -371,6 +371,15 @@ function processEvents(appEvents, appSegments, appSgValues, params, omitted_segm tmpSegVal = "[CLY]" + tmpSegVal; } + //the value becomes a field name at d...; a value + //naming an Object.prototype member is prefixed like the day numbers + //above, so a later deepMerge of the stored document cannot walk it + //into the prototype. The key is already guarded above; this is the + //value path four lines down that the key guard does not reach. + if (common.isForbiddenFieldName(tmpSegVal)) { + tmpSegVal = "[CLY]" + tmpSegVal; + } + tmpSegVal = common.encodeCharacters(tmpSegVal); var postfix = common.crypto.createHash("md5").update(tmpSegVal).digest('base64')[0]; @@ -593,6 +602,14 @@ function mergeEvents(firstObj, secondObj) { continue; } + //a stored event document can carry "__proto__" as an OWN key, which the check + //above accepts. firstObj[firstLevel] is then Object.prototype, which is truthy, + //so the "if (!firstObj[firstLevel])" branch below does not fire either and the + //two writes land on the prototype for the life of the worker. + if (common.isForbiddenFieldName(firstLevel)) { + continue; + } + if (!firstObj[firstLevel]) { firstObj[firstLevel] = secondObj[firstLevel]; continue; diff --git a/api/parts/data/exports.js b/api/parts/data/exports.js index 4470881c1a7..a6a3c74e84d 100644 --- a/api/parts/data/exports.js +++ b/api/parts/data/exports.js @@ -705,6 +705,11 @@ exports.fromRequest = function(options) { } if (options.columnNames || options.mapper) { for (key in body) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (key === "__proto__" || key === "constructor" || key === "prototype") { + continue; + } if (options.mapper) { body[key] = transformValuesInObject(body[key], options.mapper); } diff --git a/api/parts/data/fetch.js b/api/parts/data/fetch.js index c301b440160..04a294884a8 100644 --- a/api/parts/data/fetch.js +++ b/api/parts/data/fetch.js @@ -214,6 +214,21 @@ fetch.fetchMergedEventData = function(params) { }); }; +/** +* Whether a key produced by for...in over a stored document may be walked into a +* merge target. A field literally named __proto__ (or constructor / prototype) +* deserializes as an own enumerable property, but indexing the target with it +* resolves to the target's prototype rather than an own slot, so assigning through +* it writes into Object.prototype for the life of the worker. Inherited keys are +* skipped for the same reason. +* @param {object} source - the object being iterated +* @param {string} key - the key for...in produced +* @returns {boolean} true when the key is safe to merge +**/ +function isMergeableKey(source, key) { + return Object.prototype.hasOwnProperty.call(source, key) && !common.isForbiddenFieldName(key); +} + /** * Get merged data from multiple events in standard data model * @param {params} params - params object with app_id and date @@ -247,6 +262,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { // delete allEventData[i].meta; for (let levelOne in allEventData[i]) { + if (!isMergeableKey(allEventData[i], levelOne)) { + continue; + } if (typeof allEventData[i][levelOne] !== 'object') { if (mergedEventOutput[levelOne]) { mergedEventOutput[levelOne] += allEventData[i][levelOne]; @@ -257,6 +275,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { } else { for (let levelTwo in allEventData[i][levelOne]) { + if (!isMergeableKey(allEventData[i][levelOne], levelTwo)) { + continue; + } if (!mergedEventOutput[levelOne]) { mergedEventOutput[levelOne] = {}; } @@ -271,6 +292,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { } else { for (let levelThree in allEventData[i][levelOne][levelTwo]) { + if (!isMergeableKey(allEventData[i][levelOne][levelTwo], levelThree)) { + continue; + } if (!mergedEventOutput[levelOne][levelTwo]) { mergedEventOutput[levelOne][levelTwo] = {}; } @@ -285,6 +309,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { } else { for (let levelFour in allEventData[i][levelOne][levelTwo][levelThree]) { + if (!isMergeableKey(allEventData[i][levelOne][levelTwo][levelThree], levelFour)) { + continue; + } if (!mergedEventOutput[levelOne][levelTwo][levelThree]) { mergedEventOutput[levelOne][levelTwo][levelThree] = {}; } @@ -299,6 +326,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { } else { for (let levelFive in allEventData[i][levelOne][levelTwo][levelThree][levelFour]) { + if (!isMergeableKey(allEventData[i][levelOne][levelTwo][levelThree][levelFour], levelFive)) { + continue; + } if (!mergedEventOutput[levelOne][levelTwo][levelThree][levelFour]) { mergedEventOutput[levelOne][levelTwo][levelThree][levelFour] = {}; } @@ -322,6 +352,9 @@ fetch.getMergedEventData = function(params, events, options, callback) { meta = allEventData.map(x => x.meta).reduce((acc, x) => { for (var key in x) { + if (!isMergeableKey(x, key)) { + continue; + } if (acc[key]) { acc[key] = acc[key].concat(x[key]); } @@ -1813,6 +1846,12 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { **/ function deepMerge(ob1, ob2) { for (let i in ob2) { + //Skip anything that would let a stored field name reach a prototype. This + //is the durable guard: it also neutralises documents poisoned before the + //input paths were fixed, which re-pollute on every read otherwise. + if (!isMergeableKey(ob2, i)) { + continue; + } if (typeof ob1[i] === "undefined") { ob1[i] = ob2[i]; } @@ -1881,6 +1920,9 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { //old meta merge if (mergedDataObj.meta) { for (let metaEl in dataObjects[i].meta) { + if (!isMergeableKey(dataObjects[i].meta, metaEl)) { + continue; + } if (mergedDataObj.meta[metaEl]) { mergedDataObj.meta[metaEl] = union(mergedDataObj.meta[metaEl], dataObjects[i].meta[metaEl]); } @@ -1896,6 +1938,9 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { //new meta merge as hash tables if (dataObjects[i].meta_v2) { for (let metaEl in dataObjects[i].meta_v2) { + if (!isMergeableKey(dataObjects[i].meta_v2, metaEl)) { + continue; + } if (mergedDataObj.meta[metaEl]) { mergedDataObj.meta[metaEl] = union(mergedDataObj.meta[metaEl], Object.keys(dataObjects[i].meta_v2[metaEl])); } @@ -1922,16 +1967,25 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { if (!isRefresh) { for (let day in dataObjects[i].d) { + if (!isMergeableKey(dataObjects[i].d, day)) { + continue; + } if (options.unique.indexOf(day) !== -1) { continue; } for (let prop in dataObjects[i].d[day]) { + if (!isMergeableKey(dataObjects[i].d[day], prop)) { + continue; + } if (options.unique.indexOf(prop) !== -1 || prop <= 23 && prop >= 0) { continue; } if (typeof dataObjects[i].d[day][prop] === 'object') { for (let secondLevel in dataObjects[i].d[day][prop]) { + if (!isMergeableKey(dataObjects[i].d[day][prop], secondLevel)) { + continue; + } if ((levels.daily.length) ? levels.daily.indexOf(secondLevel) !== -1 : options.unique.indexOf(secondLevel) === -1) { if (!mergedDataObj[year][month][prop]) { mergedDataObj[year][month][prop] = {}; @@ -1980,6 +2034,9 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { } //Fixing meta to be escaped.(Because return output will escape keys and make values incompatable) for (let i in mergedDataObj.meta) { + if (!isMergeableKey(mergedDataObj.meta, i)) { + continue; + } for (var p = 0; p < mergedDataObj.meta[i].length; p++) { if (mergedDataObj.meta[i][p] && typeof mergedDataObj.meta[i][p] === 'string') { mergedDataObj.meta[i][p] = mergedDataObj.meta[i][p].replace(new RegExp("\"", "g"), '"'); @@ -1991,6 +2048,9 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { var metric_length = plugins.getConfig("api", params.app && params.app.plugins, true).metric_limit; if (metric_length > 0) { for (let i in mergedDataObj.meta) { + if (!isMergeableKey(mergedDataObj.meta, i)) { + continue; + } if (mergedDataObj.meta[i].length > metric_length) { delete mergedDataObj.meta[i]; //don't return if there is more than limit } @@ -2002,6 +2062,9 @@ function fetchTimeObj(collection, params, isCustomEvent, options, callback) { var value_length = plugins.getConfig("api", params.app && params.app.plugins, true).event_segmentation_value_limit; if (value_length > 0) { for (let i in mergedDataObj.meta) { + if (!isMergeableKey(mergedDataObj.meta, i)) { + continue; + } if (mergedDataObj.meta[i].length > value_length) { mergedDataObj.meta[i].splice(value_length); //removes some elements if there is more than set limit } diff --git a/api/parts/mgmt/app_users.js b/api/parts/mgmt/app_users.js index d2f17355f53..6780055447c 100644 --- a/api/parts/mgmt/app_users.js +++ b/api/parts/mgmt/app_users.js @@ -490,6 +490,11 @@ usersApi.mergeOtherPlugins = function(options, callback) { usersApi.mergeUserProperties = function(newAppUserP, oldAppUser) { for (var i in oldAppUser) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (i === "__proto__" || i === "constructor" || i === "prototype") { + continue; + } // sum up session count and total session duration if (i === "sc" || i === "tsd") { if (typeof newAppUserP[i] === "undefined") { diff --git a/api/utils/common.js b/api/utils/common.js index 05119c59521..330ca32b62d 100644 --- a/api/utils/common.js +++ b/api/utils/common.js @@ -1910,6 +1910,11 @@ common.recordMetric = function(params, props) { tmpSet = {}; for (let i in props.metrics) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (i === "__proto__" || i === "constructor" || i === "prototype") { + continue; + } props.metrics[i].value = props.metrics[i].value || 1; recordMetric(params, i, props.metrics[i], tmpSet, updateUsersZero, updateUsersMonth); } @@ -2013,6 +2018,19 @@ function recordMetric(params, metric, props, tmpSet, updateUsersZero, updateUser common.fillTimeObjectMonth(params, updateUsersMonth, monthObjUpdate, props.value); } +/** +* Whether a string, used as a MongoDB field name, would name a member of +* Object.prototype. Such a name survives storage as a literal field and, when the +* document is later walked with for...in and merged, writes into the prototype of +* the process. Segmentation values, metric values and event keys all become field +* names, so each is checked against this before use. +* @param {string} name - the candidate field name +* @returns {boolean} true when the name must not be used as a field name as-is +**/ +common.isForbiddenFieldName = function(name) { + return name === "__proto__" || name === "constructor" || name === "prototype"; +}; + /** * Record specific metric segment * @param {Params} params - params object @@ -2029,6 +2047,12 @@ function recordMetric(params, metric, props, tmpSet, updateUsersZero, updateUser function recordSegmentMetric(params, metric, name, val, props, tmpSet, updateUsersZero, updateUsersMonth, zeroObjUpdate, monthObjUpdate) { var escapedMetricKey = name.replace(/^\$/, "").replace(/\./g, ":"); var escapedMetricVal = (val + "").replace(/^\$/, "").replace(/\./g, ":"); + //escapedMetricVal is used below as a component of a d.<...> field name, so a + //value naming an Object.prototype member is prefixed the way forbidden day + //numbers already are + if (common.isForbiddenFieldName(escapedMetricVal)) { + escapedMetricVal = "[CLY]" + escapedMetricVal; + } if (!tmpSet["meta." + escapedMetricKey]) { tmpSet["meta." + escapedMetricKey] = []; } @@ -3266,6 +3290,11 @@ common.sanitizeHTML = (html, extendedWhitelist) => { common.mergeQuery = function(ob1, ob2) { if (ob2) { for (let key in ob2) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (key === "__proto__" || key === "constructor" || key === "prototype") { + continue; + } if (!ob1[key]) { ob1[key] = ob2[key]; } @@ -3297,6 +3326,11 @@ common.mergeQuery = function(ob1, ob2) { } else if (key === "$push") { for (let val in ob2[key]) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (val === "__proto__" || val === "constructor" || val === "prototype") { + continue; + } if (typeof ob1[key][val] !== 'object') { ob1[key][val] = {'$each': [ob1[key][val]]}; } diff --git a/bin/eslint-rules/no-prototype-pollution-sink.js b/bin/eslint-rules/no-prototype-pollution-sink.js new file mode 100644 index 00000000000..7e15e96098a --- /dev/null +++ b/bin/eslint-rules/no-prototype-pollution-sink.js @@ -0,0 +1,380 @@ +/** + * Reports a write that reaches Object.prototype through a key taken from an + * enumerated object. + * + * The shape, established by isolated test rather than by reasoning: + * + * reported target[k][x] = v indexes INTO target[k]. When k is "__proto__" and + * target[k][x] += v target has no own property of that name, target[k] + * IS Object.prototype, so the write lands there and + * stays for the life of the process. + * + * not target[k] = v the __proto__ setter fires and reparents the local + * reported object. Object.prototype is untouched. + * + * not src[k][x] = v where src is the very object carrying the own + * reported "__proto__" (straight out of JSON.parse or BSON), + * reading src[k] returns that own value. Such sites + * are listed in the reviewed option. + * + * Keys reaching these loops come from mongo documents and from JSON.parse of request + * data, both of which can carry "__proto__" as an own enumerable property. Naming is + * irrelevant, which is the point: the sinks this rule was written for were called + * deepMerge, getMergedEventData, an unnamed inline loop, an `action` loop and a + * `summed` loop, so no naming convention would have found them. + * + * There is deliberately no list of exceptions. Every site this rule reports is guarded + * in the source instead, so the rule is simply clean, and a new report means new code + * rather than an entry to add somewhere. + */ +/** + * The identifier a loop binds, when it enumerates an object's own keys. + * @param {object} node - candidate loop node + * @returns {string|null} the bound key name, or null when this is not such a loop + */ +function loopKeyNames(node) { + const left = node.left; + const bound = []; + if (left.type === "VariableDeclaration" && left.declarations[0]) { + const declared = left.declarations[0].id; + if (declared.type === "Identifier") { + bound.push(declared.name); + } + else if (declared.type === "ArrayPattern") { + // for (const [k, v] of Object.entries(x)): k is the key, and v can itself be + // a string later used as a key, which is how the original defect worked (a + // segmentation VALUE became a field name). Both are candidates. + declared.elements.forEach((element) => { + if (element && element.type === "Identifier") { + bound.push(element.name); + } + }); + } + } + else if (left.type === "Identifier") { + bound.push(left.name); + } + if (!bound.length) { + return []; + } + if (node.type === "ForInStatement") { + return bound; + } + // for-of counts when it walks an object's own keys or its own values: a value that + // is the string "__proto__" is just as dangerous once used as a key. + const right = node.right; + if (right && right.type === "CallExpression" && right.callee.type === "MemberExpression" + && right.callee.object && right.callee.object.name === "Object" + && right.callee.property && (right.callee.property.name === "keys" + || right.callee.property.name === "entries" + || right.callee.property.name === "values")) { + return bound; + } + return []; +} + +/** + * Whether an assignment target indexes through key and then one level deeper. + * @param {object} memberNode - the assignment's left side + * @param {string} key - the loop key + * @returns {boolean} true when the write can reach a prototype + */ +function indexesThroughKey(memberNode, key) { + const chain = []; + let node = memberNode; + while (node && node.type === "MemberExpression") { + chain.unshift(node); + node = node.object; + } + for (let i = 0; i < chain.length; i++) { + const link = chain[i]; + if (link.computed && link.property.type === "Identifier" && link.property.name === key) { + return (chain.length - 1 - i) > 0; + } + } + return false; +} + +const PROTOTYPE_MEMBER_NAMES = ["__proto__", "constructor", "prototype"]; + +// Helpers that exist to reject those names. Calling one counts as a rejection, so a +// site fixed through a shared helper still reads as fixed. +const REJECTING_HELPER = /forbidden|unsafe|reserved|dangerous|mergeable|safekey|safefield|protokey/i; + +/** + * Whether an if-test actually rejects the prototype member names, rather than merely + * mentioning the key. + * + * The distinction matters because the obvious-looking guard does not hold: + * `hasOwnProperty.call(src, k)` is TRUE for a key that came out of JSON.parse as a + * literal "__proto__", since that is an own property. Such a loop passes the check and + * then pollutes anyway - confirmed by running it, not by reading the spec. Accepting it + * as a guard would let a site that was never really fixed sit silently inside the gate. + * @param {object} test - the if-statement test + * @returns {boolean} true when the test names a prototype member or calls a helper that does + */ +function rejectsPrototypeKeys(test) { + let rejects = false; + (function scan(current) { + if (!current || typeof current.type !== "string" || rejects) { + return; + } + if (current.type === "Literal" && typeof current.value === "string" + && PROTOTYPE_MEMBER_NAMES.includes(current.value)) { + rejects = true; + return; + } + if (current.type === "CallExpression") { + const callee = current.callee; + let name = ""; + if (callee.type === "Identifier") { + name = callee.name; + } + else if (callee.type === "MemberExpression" && callee.property.type === "Identifier") { + name = callee.property.name; + } + if (REJECTING_HELPER.test(name)) { + rejects = true; + return; + } + } + for (const prop of Object.keys(current)) { + if (prop === "parent" || prop === "loc" || prop === "range") { + continue; + } + const value = current[prop]; + if (Array.isArray(value)) { + value.forEach(scan); + } + else if (value && typeof value.type === "string") { + scan(value); + } + } + }(test)); + return rejects; +} + +/** + * Whether an expression reads through one of the given keys, e.g. `src[k]` or + * `src[k].name`. Used to find values that came off the enumerated object. + * @param {object} node - expression to inspect + * @param {Array} keys - key names in scope + * @returns {boolean} true when the expression indexes with one of them + */ +function readsThroughAnyKey(node, keys) { + let found = false; + (function scan(current) { + if (!current || typeof current.type !== "string" || found) { + return; + } + if (current.type === "MemberExpression" && current.computed + && current.property.type === "Identifier" && keys.includes(current.property.name)) { + found = true; + return; + } + for (const prop of Object.keys(current)) { + if (prop === "parent" || prop === "loc" || prop === "range") { + continue; + } + const value = current[prop]; + if (Array.isArray(value)) { + value.forEach(scan); + } + else if (value && typeof value.type === "string") { + scan(value); + } + } + }(node)); + return found; +} + +/** + * Names bound inside the loop to something read off the enumerated object, which are + * then just as dangerous as the key when used as one. + * + * This is how the original defect actually worked: a segmentation VALUE became a field + * name. A loop can guard its key perfectly and still write through + * `uniqueNames[valueFromDocument][k]`, so guarding the key does not clear these. + * @param {object} body - the loop body + * @param {Array} keys - key names already in scope + * @returns {Array} additional names that carry document data + */ +function derivedKeyNames(body, keys) { + const derived = []; + (function scan(current) { + if (!current || typeof current.type !== "string") { + return; + } + if (current.type === "VariableDeclarator" && current.id.type === "Identifier" + && current.init && readsThroughAnyKey(current.init, keys.concat(derived))) { + derived.push(current.id.name); + } + else if (current.type === "AssignmentExpression" && current.left.type === "Identifier" + && readsThroughAnyKey(current.right, keys.concat(derived))) { + derived.push(current.left.name); + } + for (const prop of Object.keys(current)) { + if (prop === "parent" || prop === "loc" || prop === "range") { + continue; + } + const value = current[prop]; + if (Array.isArray(value)) { + value.forEach(scan); + } + else if (value && typeof value.type === "string") { + scan(value); + } + } + }(body)); + return derived; +} + +/** + * Whether the loop opens with a guard that skips the key, which is how a fixed site + * looks: an `if` whose test mentions the loop key, names a prototype member (or calls a + * helper that rejects them), and whose body continues. Covers + * `if (!isMergeableKey(src, k)) { continue; }` and an explicit comparison against the + * three prototype names. + * + * A bare `hasOwnProperty` check is deliberately NOT enough - see rejectsPrototypeKeys. + * @param {object} node - the loop node + * @param {string} key - the loop key name + * @returns {boolean} true when the loop skips unwanted keys up front + */ +function opensWithKeyGuard(node, key) { + const body = node.body; + const statements = body.type === "BlockStatement" ? body.body : [body]; + for (const statement of statements) { + if (statement.type !== "IfStatement") { + // only a leading guard counts; once real work starts, stop looking + return false; + } + const consequent = statement.consequent; + const continues = consequent.type === "ContinueStatement" + || (consequent.type === "BlockStatement" + && consequent.body.some((inner) => inner.type === "ContinueStatement")); + if (!continues) { + return false; + } + let mentionsKey = false; + (function scan(current) { + if (!current || typeof current.type !== "string" || mentionsKey) { + return; + } + if (current.type === "Identifier" && current.name === key) { + mentionsKey = true; + return; + } + for (const prop of Object.keys(current)) { + if (prop === "parent" || prop === "loc" || prop === "range") { + continue; + } + const value = current[prop]; + if (Array.isArray(value)) { + value.forEach(scan); + } + else if (value && typeof value.type === "string") { + scan(value); + } + } + }(statement.test)); + if (mentionsKey && rejectsPrototypeKeys(statement.test)) { + return true; + } + } + return false; +} + +module.exports = { + meta: { + type: "problem", + docs: { + description: "disallow writing through a key taken from an enumerated object, " + + "which reaches Object.prototype when the key is \"__proto__\"", + recommended: true, + }, + schema: [], + messages: { + sink: "Writing through '{{key}}' can reach Object.prototype: a stored or parsed " + + "key may be \"__proto__\", and this indexes into it. Skip the prototype " + + "member names at the top of the loop, the way the surrounding code does.", + }, + }, + + create(context) { + // eslint 8 exposes these as methods, 9+ as properties + const alreadyReported = new Set(); + + /** + * Check one loop for dangerous writes in its body. + * @param {object} node - the loop node + * @returns {void} + */ + function checkLoop(node) { + const keys = loopKeyNames(node); + if (!keys.length || !node.body) { + return; + } + // a name bound to a value off the enumerated object is as dangerous as the + // key, and a guard on the key does not clear it + const derived = derivedKeyNames(node.body, keys); + const assignments = []; + /** + * Collect assignment nodes anywhere inside the loop body. + * @param {object} current - node to descend into + * @returns {void} + */ + function collect(current) { + if (!current || typeof current.type !== "string") { + return; + } + if (current.type === "AssignmentExpression" || current.type === "UpdateExpression") { + assignments.push(current); + } + for (const prop of Object.keys(current)) { + if (prop === "parent" || prop === "loc" || prop === "range") { + continue; + } + const value = current[prop]; + if (Array.isArray(value)) { + value.forEach(collect); + } + else if (value && typeof value.type === "string") { + collect(value); + } + } + } + collect(node.body); + + for (const assignment of assignments) { + const target = assignment.type === "AssignmentExpression" + ? assignment.left : assignment.argument; + if (!target || target.type !== "MemberExpression") { + continue; + } + // a leading guard clears the loop key it names; it cannot clear a name + // carrying a value read out of the document, so derived names are checked + // without it + const key = keys.find((candidate) => indexesThroughKey(target, candidate) + && !opensWithKeyGuard(node, candidate)) + || derived.find((candidate) => indexesThroughKey(target, candidate)); + if (!key) { + continue; + } + // signature is the file plus the normalised sink text, so unrelated + // edits moving line numbers do not churn the reviewed list + const at = assignment.range ? assignment.range[0] : assignment.start; + if (alreadyReported.has(at)) { + continue; + } + alreadyReported.add(at); + context.report({ node: assignment, messageId: "sink", data: { key } }); + } + } + + return { + ForInStatement: checkLoop, + ForOfStatement: checkLoop, + }; + }, +}; diff --git a/frontend/express/public/javascripts/countly/countly.auth.js b/frontend/express/public/javascripts/countly/countly.auth.js index 725c9aa2163..470dcac68d4 100644 --- a/frontend/express/public/javascripts/countly/countly.auth.js +++ b/frontend/express/public/javascripts/countly/countly.auth.js @@ -223,7 +223,13 @@ }; for (var countlyApp in countlyGlobal.apps) { + if (countlyCommon.isForbiddenFieldName(countlyApp)) { + continue; + } for (var accessType in permissionObject) { + if (countlyCommon.isForbiddenFieldName(accessType)) { + continue; + } permissionObject[accessType][countlyApp] = {}; permissionObject[accessType][countlyApp].all = false; permissionObject[accessType][countlyApp].allowed = {}; diff --git a/frontend/express/public/javascripts/countly/countly.common.js b/frontend/express/public/javascripts/countly/countly.common.js index 5c3ebc5297c..a4f610319d1 100644 --- a/frontend/express/public/javascripts/countly/countly.common.js +++ b/frontend/express/public/javascripts/countly/countly.common.js @@ -2084,6 +2084,20 @@ }); }; + /** + * Whether a string, used as an object key, names a member of Object.prototype. + * Such a name can arrive as a stored segmentation/metric value or as an own key of a + * JSON/BSON response; writing THROUGH it (target[name][...] = ...) reaches + * Object.prototype for the life of the page. Every hand-rolled merge below skips + * these names before indexing, mirroring api/utils/common.js isForbiddenFieldName. + * @memberof countlyCommon + * @param {string} name - the candidate key + * @returns {boolean} true when the name must not be used as a key as-is + */ + countlyCommon.isForbiddenFieldName = function(name) { + return name === "__proto__" || name === "constructor" || name === "prototype"; + }; + /** * Merge metric data in chartData returned by @{link countlyCommon.extractChartData} or @{link countlyCommon.extractTwoLevelData }, just in case if after data transformation of countly standard metric data model, resulting chartData contains duplicated values, as for example converting null, undefined and unknown values to unknown * @memberof countlyCommon @@ -2112,6 +2126,11 @@ newName = jQuery.i18n.map["common.unknown"]; } data[metric] = newName; + // a stored segmentation/range value can be a prototype member name; + // writing through uniqueNames[newName] below would reach Object.prototype + if (countlyCommon.isForbiddenFieldName(newName)) { + continue; + } if (newName && !uniqueNames[newName]) { uniqueNames[newName] = data; } @@ -2566,6 +2585,13 @@ continue; } + // an own "__proto__"/"constructor"/"prototype" key survives JSON/BSON and + // hasOwnProperty does not exclude it; writing through dbObj[year][level1] + // below would reach Object.prototype + if (countlyCommon.isForbiddenFieldName(level1)) { + continue; + } + if (intRegex.test(level1)) { continue; } @@ -2627,6 +2653,10 @@ continue; } + if (countlyCommon.isForbiddenFieldName(level2)) { + continue; + } + if (dbObj[year][level1][level2]) { if (tmpOldObj[level1] && tmpOldObj[level1][level2]) { dbObj[year][level1][level2] += (tmpUpdateObj[level1][level2] - tmpOldObj[level1][level2]); diff --git a/frontend/express/public/javascripts/countly/countly.event.js b/frontend/express/public/javascripts/countly/countly.event.js index fe6b8cda7e1..0bf5ade14de 100644 --- a/frontend/express/public/javascripts/countly/countly.event.js +++ b/frontend/express/public/javascripts/countly/countly.event.js @@ -68,6 +68,9 @@ _activeEvents = json; _eventGroupsTable = groups_json; for (var group in groups_json) { + if (countlyCommon.isForbiddenFieldName(group)) { + continue; + } if (groups_json[group].status) { _eventGroups[groups_json[group]._id] = { label: groups_json[group].name, @@ -427,6 +430,9 @@ _activeEvents = json; _eventGroupsTable = groups_json; for (var group in groups_json) { + if (countlyCommon.isForbiddenFieldName(group)) { + continue; + } if (groups_json[group].status) { _eventGroups[groups_json[group]._id] = { label: groups_json[group].name, @@ -823,6 +829,9 @@ eventNames = []; for (var event in eventSegmentations) { + if (countlyCommon.isForbiddenFieldName(event)) { + continue; + } var mapKey = event.replace(/\\/g, "\\\\").replace(/\$/g, "\\u0024").replace(/\./g, '\\u002e'); if (eventMap[mapKey] && eventMap[mapKey].name) { eventNames.push({ @@ -950,6 +959,9 @@ tmpPrevSum = 0, tmpPrevDur = 0; for (segment in tmp_x) { + if (countlyCommon.isForbiddenFieldName(segment)) { + continue; + } tmpCurrCount += tmp_x[segment].c || 0; tmpCurrSum += tmp_x[segment].s || 0; tmpCurrDur += tmp_x[segment].dur || 0; @@ -996,6 +1008,9 @@ tmpPrevSum = 0, tmpPrevDur = 0; for (segment in tmp_x) { + if (countlyCommon.isForbiddenFieldName(segment)) { + continue; + } if (typeof tmp_x[segment].c === 'number') { tmpCurrCount += tmp_x[segment].c || 0; } @@ -1230,6 +1245,9 @@ /** function extend meta */ function extendMeta() { for (var metaObj in _activeEventDb.meta) { + if (countlyCommon.isForbiddenFieldName(metaObj)) { + continue; + } if (_activeSegmentationObj[metaObj] && _activeEventDb.meta[metaObj] && _activeSegmentationObj[metaObj].length !== _activeEventDb.meta[metaObj].length) { _activeSegmentationObj[metaObj] = countlyCommon.union(_activeSegmentationObj[metaObj], _activeEventDb.meta[metaObj]); } @@ -1292,6 +1310,9 @@ success: function(groups_json) { if (groups_json) { for (var group in groups_json) { + if (countlyCommon.isForbiddenFieldName(group)) { + continue; + } if (groups_json[group].status) { data.list = data.list || []; data.list.push(groups_json[group]._id); diff --git a/frontend/express/public/javascripts/countly/countly.session.js b/frontend/express/public/javascripts/countly/countly.session.js index baaadd0f7c1..6217809b2ce 100644 --- a/frontend/express/public/javascripts/countly/countly.session.js +++ b/frontend/express/public/javascripts/countly/countly.session.js @@ -88,6 +88,9 @@ }, countlySession.clearObject); for (var z in sparkLines) { + if (countlyCommon.isForbiddenFieldName(z)) { + continue; + } ret[z].sparkline = sparkLines[z]; } return {usage: ret}; diff --git a/frontend/express/public/javascripts/countly/countly.template.js b/frontend/express/public/javascripts/countly/countly.template.js index 2873caaf92b..ae36abf08e3 100644 --- a/frontend/express/public/javascripts/countly/countly.template.js +++ b/frontend/express/public/javascripts/countly/countly.template.js @@ -368,7 +368,13 @@ var AppRouter = Backbone.Router.extend({ */ _removeUnfinishedRequests: function() { for (var url in this._myRequests) { + if (countlyCommon.isForbiddenFieldName(url)) { + continue; + } for (var data in this._myRequests[url]) { + if (countlyCommon.isForbiddenFieldName(data)) { + continue; + } //4 means done, less still in progress if (parseInt(this._myRequests[url][data].readyState) !== 4) { this._myRequests[url][data].abort_reason = "view_change"; diff --git a/frontend/express/public/javascripts/countly/countly.view.js b/frontend/express/public/javascripts/countly/countly.view.js index 6f60c02649c..4f9aae252b5 100644 --- a/frontend/express/public/javascripts/countly/countly.view.js +++ b/frontend/express/public/javascripts/countly/countly.view.js @@ -66,7 +66,13 @@ window.countlyView = Backbone.View.extend({ }, _removeMyRequests: function() { for (var url in this._myRequests) { + if (countlyCommon.isForbiddenFieldName(url)) { + continue; + } for (var data in this._myRequests[url]) { + if (countlyCommon.isForbiddenFieldName(data)) { + continue; + } //4 means done, less still in progress if (parseInt(this._myRequests[url][data].readyState, 10) !== 4) { this._myRequests[url][data].abort_reason = "app_remove_reqs"; diff --git a/package.json b/package.json index cdab0b9afca..42fa018bf6a 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ }, "lint-staged": { "*.{js,vue}": [ - "eslint --fix --no-quiet" + "eslint --fix --no-quiet --rulesdir bin/eslint-rules" ] }, "devDependencies": { diff --git a/plugins/crashes/api/api.js b/plugins/crashes/api/api.js index 98147f4ea58..ec932258ef4 100644 --- a/plugins/crashes/api/api.js +++ b/plugins/crashes/api/api.js @@ -424,6 +424,11 @@ plugins.setConfigs("crashes", { if (report.binary_images && typeof report.binary_images === "object") { var needs_regeneration = false; for (let k in report.binary_images) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (k === "__proto__" || k === "constructor" || k === "prototype") { + continue; + } if (!report.binary_images[k].bn) { report.binary_images[k].bn = k; needs_regeneration = true; diff --git a/plugins/dashboards/api/parts/dashboards.js b/plugins/dashboards/api/parts/dashboards.js index 79277faed3e..3ebef626eec 100644 --- a/plugins/dashboards/api/parts/dashboards.js +++ b/plugins/dashboards/api/parts/dashboards.js @@ -1044,6 +1044,11 @@ async function getPushDataForApp(params, apps, appId, widget) { data = model.getTimelineData(); for (var z in data) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (z === "__proto__" || z === "constructor" || z === "prototype") { + continue; + } if (!widgetData[z]) { widgetData[z] = {}; } diff --git a/plugins/push/api/jobs/util/batcher.js b/plugins/push/api/jobs/util/batcher.js index 5a65513bd24..17dc62511e7 100644 --- a/plugins/push/api/jobs/util/batcher.js +++ b/plugins/push/api/jobs/util/batcher.js @@ -90,6 +90,11 @@ class Batcher extends DoFinish { let { PLATFORM } = require('../../send/platforms'); for (let p in PLATFORM) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (p === "__proto__" || p === "constructor" || p === "prototype") { + continue; + } if (!this.ids[app._id][p]) { this.ids[app._id][p] = {}; } diff --git a/plugins/push/api/jobs/util/resultor.js b/plugins/push/api/jobs/util/resultor.js index 382c4c91353..4613a9f5db6 100644 --- a/plugins/push/api/jobs/util/resultor.js +++ b/plugins/push/api/jobs/util/resultor.js @@ -415,6 +415,11 @@ class Resultor extends DoFinish { // changed tokens - set new ones for (let aid in this.changed) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (aid === "__proto__" || aid === "constructor" || aid === "prototype") { + continue; + } let collection = 'push_' + aid; if (!updates[collection]) { updates[collection] = []; @@ -438,6 +443,11 @@ class Resultor extends DoFinish { // expired tokens - unset for (let aid in this.removeTokens) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (aid === "__proto__" || aid === "constructor" || aid === "prototype") { + continue; + } let collectionPush = `push_${aid}`, collectionAppUsers = `app_users${aid}`; if (!updates[collectionPush]) { @@ -475,11 +485,21 @@ class Resultor extends DoFinish { let now = Date.now(); for (let aid in this.sentUsers) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (aid === "__proto__" || aid === "constructor" || aid === "prototype") { + continue; + } let collection = 'push_' + aid; if (!updates[collection]) { updates[collection] = []; } for (let mid in this.sentUsers[aid]) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (mid === "__proto__" || mid === "constructor" || mid === "prototype") { + continue; + } if (this.sentUsers[aid][mid].users.length) { updates[collection].push({ updateMany: { diff --git a/plugins/push/api/parts/note.js b/plugins/push/api/parts/note.js index cba11c5add4..87441ca7ecc 100644 --- a/plugins/push/api/parts/note.js +++ b/plugins/push/api/parts/note.js @@ -515,6 +515,12 @@ class Note { } if (data) { for (let k in data) { + //data comes off a stored message, so k can be a literal "__proto__". + //compiled is built here, so compiled[k] would be Object.prototype, + //which satisfies every test below and would be written into. + if (k === "__proto__" || k === "constructor" || k === "prototype") { + continue; + } if (compiled[k] && typeof compiled[k] === 'object' && !Array.isArray(compiled[k]) && typeof data[k] === 'object') { for (let kk in data[k]) { compiled[k][kk] = data[k][kk]; diff --git a/plugins/push/frontend/public/javascripts/countly.views.component.common.js b/plugins/push/frontend/public/javascripts/countly.views.component.common.js index 84082df30e1..6a38da85207 100644 --- a/plugins/push/frontend/public/javascripts/countly.views.component.common.js +++ b/plugins/push/frontend/public/javascripts/countly.views.component.common.js @@ -444,6 +444,9 @@ if (this.search) { var obj = {}; for (var category in this.emojiTable) { + if (countlyCommon.isForbiddenFieldName(category)) { + continue; + } obj[category] = {}; for (var emoji in this.emojiTable[category]) { if (new RegExp(".*" + this.escapeRegExp(this.search) + ".*").test(emoji)) { diff --git a/plugins/sdk/frontend/public/javascripts/countly.views.js b/plugins/sdk/frontend/public/javascripts/countly.views.js index 81626f568f4..67faa68570a 100644 --- a/plugins/sdk/frontend/public/javascripts/countly.views.js +++ b/plugins/sdk/frontend/public/javascripts/countly.views.js @@ -58,6 +58,9 @@ var params = this.$store.getters["countlySDK/sdk/all"]; var data = params || {}; for (var key in this.configs) { + if (countlyCommon.isForbiddenFieldName(key)) { + continue; + } if (this.diff.indexOf(key) === -1) { this.configs[key].value = typeof data[key] !== "undefined" ? data[key] : this.configs[key].default; } @@ -175,6 +178,9 @@ var params = this.$store.getters["countlySDK/sdk/all"]; var data = params || {}; for (var key in this.configs) { + if (countlyCommon.isForbiddenFieldName(key)) { + continue; + } this.configs[key].value = typeof data[key] !== "undefined" ? data[key] : this.configs[key].default; } } diff --git a/plugins/server-stats/api/parts/stats.js b/plugins/server-stats/api/parts/stats.js index 482faf84637..83567f7852f 100644 --- a/plugins/server-stats/api/parts/stats.js +++ b/plugins/server-stats/api/parts/stats.js @@ -400,6 +400,11 @@ function fetchDatapoints(db, filter, options, callback) { delete toReturn["all-apps"]; } for (var z in toReturn) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (z === "__proto__" || z === "constructor" || z === "prototype") { + continue; + } toReturn[z].change = toReturn[z].dp - toReturn[z].change; } callback(toReturn); diff --git a/plugins/sources/frontend/public/javascripts/countly.views.js b/plugins/sources/frontend/public/javascripts/countly.views.js index 2a3373b671e..43c6adb2e3d 100755 --- a/plugins/sources/frontend/public/javascripts/countly.views.js +++ b/plugins/sources/frontend/public/javascripts/countly.views.js @@ -67,10 +67,17 @@ var source; for (var i in cleanData) { source = countlySources.getSourceName(cleanData[i].sources); + // source is a value read out of the response, so it can be a prototype + // member name; skip it before it is used as a key + if (countlyCommon.isForbiddenFieldName(source)) { + continue; + } if (!self.dataMap[source]) { self.dataMap[source] = {}; } - self.dataMap[source][cleanData[i].sources] = cleanData[i]; + // write onto the already-selected bucket, never through self.dataMap[source] + var sourceBucket = self.dataMap[source]; + sourceBucket[cleanData[i].sources] = cleanData[i]; } this.sourcesDetailData = self.dataMap; }, diff --git a/plugins/views/api/api.js b/plugins/views/api/api.js index 41ca1c35c6b..7031fdf53b9 100644 --- a/plugins/views/api/api.js +++ b/plugins/views/api/api.js @@ -2122,6 +2122,11 @@ const escapedViewSegments = { "name": true, "segment": true, "height": true, "wi var addToSetRules = {}; if (currEvent.segmentation) { for (let segKey in currEvent.segmentation) { + // a key off a stored document or a parsed payload can be a prototype + // member name; writing through one would reach Object.prototype + if (segKey === "__proto__" || segKey === "constructor" || segKey === "prototype") { + continue; + } let tmpSegKey = ""; if (segKey.indexOf('.') !== -1 || segKey.substr(0, 1) === '$') { tmpSegKey = segKey.replace(/^\$|\./g, ""); diff --git a/plugins/views/frontend/public/javascripts/countly.models.js b/plugins/views/frontend/public/javascripts/countly.models.js index e5089ab7600..1991037aade 100644 --- a/plugins/views/frontend/public/javascripts/countly.models.js +++ b/plugins/views/frontend/public/javascripts/countly.models.js @@ -958,6 +958,9 @@ if (json.data && json.appID === countlyCommon.ACTIVE_APP_ID) { json = json.data; for (var k in json) { + if (countlyCommon.isForbiddenFieldName(k)) { + continue; + } if (k.indexOf("_name") > -1) { _graphDataObj[k] = json[k]; //copy new name } diff --git a/test/unit-tests/api.data.segmentation-value-prototype.js b/test/unit-tests/api.data.segmentation-value-prototype.js new file mode 100644 index 00000000000..555de1800b8 --- /dev/null +++ b/test/unit-tests/api.data.segmentation-value-prototype.js @@ -0,0 +1,106 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); +var common = require("../../api/utils/common.js"); + +// Segmentation values, metric values and event keys all become MongoDB field names. +// A value literally naming an Object.prototype member survives storage as a field and, +// when the stored document is later walked by deepMerge, is written into the prototype +// of the API worker for the rest of its life. #7634 guarded the key path; these cover +// the shared predicate and the two value paths that reach the same field-name position. +// +// recordSegmentMetric and deepMerge are module-private, so the paths through them are +// asserted at the source level. The behavioural proof that deepMerge no longer pollutes +// is in the PR description, produced by lifting the real function and merging a +// document with an own-enumerable __proto__ field. + +describe("common.isForbiddenFieldName", function() { + it("names the three prototype members", function() { + common.isForbiddenFieldName("__proto__").should.equal(true); + common.isForbiddenFieldName("constructor").should.equal(true); + common.isForbiddenFieldName("prototype").should.equal(true); + }); + it("passes ordinary segment values through", function() { + common.isForbiddenFieldName("Chrome").should.equal(false); + common.isForbiddenFieldName("enterprise").should.equal(false); + common.isForbiddenFieldName("").should.equal(false); + }); +}); + +describe("the value paths run through the predicate before building a field name", function() { + it("events.js guards the segmentation value, not only the key", function() { + var src = fs.readFileSync(path.join(__dirname, "../../api/parts/data/events.js"), "utf8"); + // the value is escaped into tmpSegVal, then must pass the predicate before use + src.should.match(/isForbiddenFieldName\(tmpSegVal\)/); + }); + it("common.js recordSegmentMetric guards the metric value", function() { + var src = fs.readFileSync(path.join(__dirname, "../../api/utils/common.js"), "utf8"); + src.should.match(/isForbiddenFieldName\(escapedMetricVal\)/); + }); + it("events.js mergeEvents guards its outer key, hasOwnProperty not being enough", function() { + // hasOwnProperty is TRUE for a stored document's own "__proto__", so it lets the + // key through; firstObj[firstLevel] is then Object.prototype, which is truthy, so + // the "if (!firstObj[firstLevel])" branch does not fire either and both writes + // below land on the prototype. Confirmed by lifting the function and merging such + // a document: before the guard Object.prototype gained the field, after it did + // not and the legitimate segment still summed. + var src = fs.readFileSync(path.join(__dirname, "../../api/parts/data/events.js"), "utf8"); + var fn = src.slice(src.indexOf("function mergeEvents")); + fn = fn.slice(0, fn.indexOf("\n}")); + fn.should.match(/isForbiddenFieldName\(firstLevel\)/); + }); + it("deepMerge skips inherited keys and prototype-member names", function() { + // via the shared isMergeableKey, which the read-path suite below pins down + var src = fs.readFileSync(path.join(__dirname, "../../api/parts/data/fetch.js"), "utf8"); + var dm = src.slice(src.indexOf("function deepMerge")); + dm = dm.slice(0, dm.indexOf("return ob1")); + dm.should.match(/isMergeableKey\(ob2, i\)/); + }); +}); + +describe("both read-path merges refuse a prototype key", function() { + var fetchSrc = fs.readFileSync(path.join(__dirname, "../../api/parts/data/fetch.js"), "utf8"); + + it("shares one guard for every walk of a stored document", function() { + fetchSrc.should.match(/function isMergeableKey\(source, key\)/); + fetchSrc.should.match(/hasOwnProperty\.call\(source, key\)/); + fetchSrc.should.match(/isForbiddenFieldName\(key\)/); + }); + + it("guards deepMerge", function() { + var dm = fetchSrc.slice(fetchSrc.indexOf("function deepMerge")); + dm.slice(0, dm.indexOf("return ob1")).should.match(/isMergeableKey\(ob2, i\)/); + }); + + it("guards all five levels of getMergedEventData", function() { + // the second sink: a hand-inlined nested merge, not named "merge" and not + // recursive, so a grep for merge helpers alone does not find it + var gme = fetchSrc.slice(fetchSrc.indexOf("fetch.getMergedEventData")); + gme = gme.slice(0, gme.indexOf("meta = allEventData.map")); + ["levelOne", "levelTwo", "levelThree", "levelFour", "levelFive"].forEach(function(level) { + gme.should.match(new RegExp("isMergeableKey\\([^)]*, " + level + "\\)")); + }); + }); + + it("guards the meta reduce, where a prototype key would throw rather than pollute", function() { + var reduce = fetchSrc.slice(fetchSrc.indexOf("meta = allEventData.map")); + reduce.slice(0, 400).should.match(/isMergeableKey\(x, key\)/); + }); +}); + +describe("getMergedObj walks stored days without reaching a prototype", function() { + var fetchSrc = fs.readFileSync(path.join(__dirname, "../../api/parts/data/fetch.js"), "utf8"); + + it("guards the day loop and the segmentation-value loop", function() { + // the third sink: same file as deepMerge, not named like a merge, and the + // existing "if (!target[key])" guards cannot help because a prototype is truthy + fetchSrc.should.match(/isMergeableKey\(dataObjects\[i\]\.d, day\)/); + fetchSrc.should.match(/isMergeableKey\(dataObjects\[i\]\.d\[day\], prop\)/); + fetchSrc.should.match(/isMergeableKey\(dataObjects\[i\]\.d\[day\]\[prop\], secondLevel\)/); + }); + + it("guards the meta merges that read the accumulator back", function() { + fetchSrc.should.match(/isMergeableKey\(dataObjects\[i\]\.meta, metaEl\)/); + fetchSrc.should.match(/isMergeableKey\(mergedDataObj\.meta, i\)/); + }); +}); diff --git a/test/unit-tests/api.eslint-rule.prototype-pollution-sink.js b/test/unit-tests/api.eslint-rule.prototype-pollution-sink.js new file mode 100644 index 00000000000..4f47569ae7f --- /dev/null +++ b/test/unit-tests/api.eslint-rule.prototype-pollution-sink.js @@ -0,0 +1,100 @@ +require("should"); +var path = require("path"); +var { RuleTester } = require("eslint"); +var rule = require("../../bin/eslint-rules/no-prototype-pollution-sink.js"); + +// A key taken from a mongo document or from JSON.parse can be the literal "__proto__". +// Writing THROUGH such a key reaches Object.prototype when the target has no own +// property of that name, and the pollution lasts the life of the worker. Writing AT it +// is harmless: the setter fires and reparents the local object. +// +// The rule exists because naming is no guide: the sinks it was written for were called +// deepMerge, getMergedEventData, an unnamed inline loop, an `action` loop and a +// `summed` loop. + +var ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2022, sourceType: "script" } }); + +describe("no-prototype-pollution-sink", function() { + it("reports writes through an enumerated key and allows writes at it", function() { + ruleTester.run("no-prototype-pollution-sink", rule, { + valid: [ + // writing AT the key: the setter reparents a local object, no pollution + { code: "for (var k in doc) { acc[k] = doc[k]; }" }, + // guarded the way the fixed merges are + { + code: "for (var k in doc) { if (k === '__proto__') { continue; } acc[k].x = 1; }", + }, + // a second guard style: hasOwnProperty plus the names, as the merges use + { + code: "for (var k in doc) { if (!isMergeableKey(doc, k)) { continue; } acc[k].x = 1; }", + }, + // the shared helper the fixed sites call + { + code: "for (var k in doc) { if (common.isForbiddenFieldName(k)) { continue; } acc[k].x = 1; }", + }, + // hasOwnProperty is not a prototype guard on its own, but with one it is + { + code: "for (var k in doc) { if (!doc.hasOwnProperty(k) || k === '__proto__') { continue; } acc[k].x = 1; }", + }, + // a value that never came off the enumerated object is not a derived key + { code: "for (var k in doc) { var n = other.name; out[n].x = 1; }" }, + // one level through a derived name only reparents a local object + { code: "for (var k in doc) { var n = doc[k].name; out[n] = 1; }" }, + // a plain indexed loop is not this shape + { code: "for (var i = 0; i < n; i++) { acc[i].x = 1; }" }, + ], + invalid: [ + { + code: "for (var k in doc) { acc[k].x = 1; }", + errors: [{ messageId: "sink" }], + }, + { + code: "for (var k in doc) { acc[k][j] += doc[k][j]; }", + errors: [{ messageId: "sink" }], + }, + // for-of over Object.keys is the same enumeration + { + code: "for (const k of Object.keys(doc)) { acc[k].x = 1; }", + errors: [{ messageId: "sink" }], + }, + // nested loops must not report the same write twice + { + code: "for (var a in doc) { for (var b in doc[a]) { out[a][b] = 1; } }", + errors: 1, + }, + // hasOwnProperty alone is NOT a guard: an own "__proto__" out of a stored + // document passes it, and the write below still reaches the prototype. + // This is the shape mergeEvents had. + { + code: "for (var k in doc) { if (!Object.prototype.hasOwnProperty.call(doc, k)) { continue; } acc[k].x = 1; }", + errors: [{ messageId: "sink" }], + }, + { + code: "for (var k in doc) { if (!doc.hasOwnProperty(k)) { continue; } acc[k].x = 1; }", + errors: [{ messageId: "sink" }], + }, + // writing through a VALUE read off the document, which is how the original + // defect worked: a segmentation value became a field name + { + code: "for (var k in rows) { var n = rows[k].name; uniq[n][k] = rows[k].v; }", + errors: [{ messageId: "sink" }], + }, + // guarding the KEY does not clear a name carrying document data + { + code: "for (var k in rows) { if (k === '__proto__') { continue; } var n = rows[k].name; uniq[n].total = 1; }", + errors: [{ messageId: "sink" }], + }, + ], + }); + }); + + it("points at guarding the loop, there being no exceptions list to add to", function() { + var linter = new (require("eslint").Linter)(); + linter.defineRule("t", rule); + var messages = linter.verify("for (var k in doc) { acc[k].x = 1; }", { + parserOptions: { ecmaVersion: 2022 }, + rules: { t: "error" }, + }, path.join(process.cwd(), "api/zz.js")); + messages[0].message.should.match(/Skip the prototype member names/); + }); +}); diff --git a/test/unit-tests/frontend.segmentation-value-prototype.js b/test/unit-tests/frontend.segmentation-value-prototype.js new file mode 100644 index 00000000000..fa9104f2dae --- /dev/null +++ b/test/unit-tests/frontend.segmentation-value-prototype.js @@ -0,0 +1,305 @@ +require("should"); +var fs = require("fs"); +var path = require("path"); + +// The client mirror of test/unit-tests/api.data.segmentation-value-prototype.js. +// +// A segmentation value, an event key or a metric name becomes an object key on the +// browser side too, and a stored document delivered as JSON can carry "__proto__" as an +// own enumerable property. Two hand-rolled merges in countly.common.js write THROUGH such +// a key (target[key][...] = ...) and so reach Object.prototype for the life of the page; +// the seven read-path loops in countly.event.js write AT the key or only read it, so they +// corrupt a local object or surface a bogus row rather than polluting the prototype. All +// are guarded the same way: countlyCommon.isForbiddenFieldName skips the three prototype +// member names at the top of the loop. +// +// countlyCommon and countlyEvent are browser IIFEs that construct against store, jQuery, +// moment and the DOM, so the guarded functions are lifted out of the real source and run +// here against a payload with an own "__proto__". Lifting keeps the proof on the shipping +// code: an edit that drops a guard breaks a behavioural test, not only a source match. + +var COMMON = path.join(__dirname, "../../frontend/express/public/javascripts/countly/countly.common.js"); +var EVENT = path.join(__dirname, "../../frontend/express/public/javascripts/countly/countly.event.js"); +var commonSrc = fs.readFileSync(COMMON, "utf8"); +var eventSrc = fs.readFileSync(EVENT, "utf8"); + +// ` countlyCommon. = function ... ` up to the first 8-space `};` +function methodSrc(name) { + var lines = commonSrc.split("\n"); + var start = lines.findIndex(function(l) { + return l.indexOf(" countlyCommon." + name + " = function") === 0; + }); + if (start < 0) { + throw new Error("method not found in countly.common.js: " + name); + } + for (var j = start + 1; j < lines.length; j++) { + if (lines[j] === " };") { + return lines.slice(start, j + 1).join("\n"); + } + } + throw new Error("terminator not found for: " + name); +} + +// a block from `startPrefix` (line start) up to and including the first `endLine` +function blockSrc(startPrefix, endLine) { + var lines = eventSrc.split("\n"); + var start = lines.findIndex(function(l) { + return l.indexOf(startPrefix) === 0; + }); + if (start < 0) { + throw new Error("block not found in countly.event.js: " + startPrefix); + } + for (var j = start + 1; j < lines.length; j++) { + if (lines[j] === endLine) { + return lines.slice(start, j + 1).join("\n"); + } + } + throw new Error("end not found for: " + startPrefix); +} + +// minimal stand-ins for the browser globals the lifted functions touch +var moment = function() { + return { + year: function() { + return 2026; + }, + month: function() { + return 7; + }, + date: function() { + return 13; + }, + format: function(f) { + return f === "DDD" ? "225" : ""; + } + }; +}; +var _ = { + isObject: function(o) { + return o !== null && typeof o === "object"; + }, + values: function(o) { + return Object.values(o); + } +}; +var jQuery = { i18n: { map: { "common.unknown": "Unknown" } } }; + +// module-private state that the two event functions close over +var _activeEventDb = {}; +var _activeEvents = {}; +var _activeSegmentation = ""; +var _activeSegmentations = []; +var _activeSegmentationValues = []; +var _activeSegmentationObj = {}; +var countlyEvent = {}; + +var countlyCommon = {}; +countlyCommon.union = function(a, b) { + a = Array.isArray(a) ? a : []; + b = Array.isArray(b) ? b : []; + return a.concat(b); +}; + +/* eslint-disable no-eval */ +eval(methodSrc("isForbiddenFieldName")); +eval(methodSrc("mergeMetricsByName")); +eval(methodSrc("extendDbObj")); +var extendMeta = eval("(" + blockSrc(" function extendMeta() {", " }").trim() + ")"); +eval(blockSrc(" countlyEvent.getEventsWithSegmentations = function() {", " };")); +/* eslint-enable no-eval */ + +describe("countly client: segmentation value prototype pollution", function() { + afterEach(function() { + // any test that regresses must not leave the prototype dirty for the next + ["mmbn_marker", "range", "t", "edo_marker", "p1", "p2"].forEach(function(k) { + delete Object.prototype[k]; + }); + }); + + describe("countlyCommon.isForbiddenFieldName", function() { + it("names the three prototype members", function() { + countlyCommon.isForbiddenFieldName("__proto__").should.equal(true); + countlyCommon.isForbiddenFieldName("constructor").should.equal(true); + countlyCommon.isForbiddenFieldName("prototype").should.equal(true); + }); + it("passes ordinary segment values through", function() { + countlyCommon.isForbiddenFieldName("Chrome").should.equal(false); + countlyCommon.isForbiddenFieldName("enterprise").should.equal(false); + countlyCommon.isForbiddenFieldName("").should.equal(false); + }); + }); + + describe("mergeMetricsByName (value used as a key)", function() { + it("does not pollute Object.prototype when a merged name is __proto__", function() { + countlyCommon.mergeMetricsByName( + [{ range: "__proto__", t: 5, mmbn_marker: "PWN" }], "range"); + Object.prototype.should.not.have.property("mmbn_marker"); + ({}).should.not.have.property("mmbn_marker"); + }); + it("still sums rows that share an ordinary name", function() { + var out = countlyCommon.mergeMetricsByName( + [{ range: "Chrome", t: 2 }, { range: "Chrome", t: 3 }], "range"); + var chrome = out.filter(function(r) { + return r.range === "Chrome"; + })[0]; + chrome.t.should.equal(5); + }); + }); + + describe("extendDbObj (own __proto__ key in a stored day object)", function() { + it("does not pollute Object.prototype", function() { + countlyCommon.extendDbObj({}, + JSON.parse('{"2026":{"8":{"13":{"__proto__":{"edo_marker":7}}}}}')); + Object.prototype.should.not.have.property("edo_marker"); + ({}).should.not.have.property("edo_marker"); + }); + it("still merges an ordinary nested segmentation", function() { + var dbObj = {}; + countlyCommon.extendDbObj(dbObj, + JSON.parse('{"2026":{"8":{"13":{"Chrome":{"t":4}}}}}')); + dbObj[2026].Chrome.t.should.equal(4); + }); + }); + + describe("countly.event.js read-path loops", function() { + it("extendMeta neither pollutes nor reparents on a __proto__ meta key", function() { + _activeEventDb = { meta: JSON.parse('{"__proto__":["p1","p2"],"country":["US"]}') }; + _activeSegmentationObj = { country: [] }; + _activeSegmentation = "country"; + var protoBefore = Object.getPrototypeOf(_activeSegmentationObj); + extendMeta(); + Object.prototype.should.not.have.property("p1"); + Object.getPrototypeOf(_activeSegmentationObj).should.equal(protoBefore); + _activeSegmentationObj.country.should.eql(["US"]); + }); + it("getEventsWithSegmentations skips a __proto__ segment and keeps real ones", function() { + _activeEvents = { + segments: JSON.parse('{"__proto__":["seg"],"purchase":["sku"]}'), + map: {} + }; + var names = countlyEvent.getEventsWithSegmentations(); + names.some(function(n) { + return n.key === "__proto__"; + }).should.equal(false); + names.some(function(n) { + return n.key === "purchase"; + }).should.equal(true); + }); + }); + + describe("every guarded loop keeps its guard in source", function() { + it("countly.common.js guards both write-through sinks", function() { + commonSrc.should.match(/countlyCommon\.isForbiddenFieldName = function/); + commonSrc.should.match(/isForbiddenFieldName\(newName\)/); // mergeMetricsByName + commonSrc.should.match(/isForbiddenFieldName\(level1\)/); // extendDbObj outer + commonSrc.should.match(/isForbiddenFieldName\(level2\)/); // extendDbObj inner + }); + it("countly.event.js guards all seven response walks", function() { + eventSrc.should.match(/isForbiddenFieldName\(event\)/); // getEventsWithSegmentations + eventSrc.should.match(/isForbiddenFieldName\(metaObj\)/); // extendMeta + (eventSrc.match(/isForbiddenFieldName\(segment\)/g) || []).length.should.equal(2); + (eventSrc.match(/isForbiddenFieldName\(group\)/g) || []).length.should.equal(3); + }); + }); +}); + +// The no-prototype-pollution-sink eslint rule was extended from api/plugins-api to the +// dashboard and plugin frontends. Every site it flags there is guarded in source (there is +// no exceptions list). These cover that the guards are present, that the rule is actually +// switched on for those paths, and behaviourally that the two genuinely global sinks it +// surfaced - the views json walk and the sources derived-value index - no longer pollute. +describe("countly dashboard + plugin prototype-pollution guards", function() { + function read(rel) { + return fs.readFileSync(path.join(__dirname, "../../" + rel), "utf8"); + } + + describe("every flagged loop is guarded in source", function() { + var GUARDS = { + "frontend/express/public/javascripts/countly/countly.auth.js": + [[/isForbiddenFieldName\(countlyApp\)/, 1], [/isForbiddenFieldName\(accessType\)/, 1]], + "frontend/express/public/javascripts/countly/countly.session.js": + [[/isForbiddenFieldName\(z\)/, 1]], + "frontend/express/public/javascripts/countly/countly.template.js": + [[/isForbiddenFieldName\(url\)/, 1], [/isForbiddenFieldName\(data\)/, 1]], + "frontend/express/public/javascripts/countly/countly.view.js": + [[/isForbiddenFieldName\(url\)/, 1], [/isForbiddenFieldName\(data\)/, 1]], + "plugins/views/frontend/public/javascripts/countly.models.js": + [[/isForbiddenFieldName\(k\)/, 1]], + "plugins/sdk/frontend/public/javascripts/countly.views.js": + [[/isForbiddenFieldName\(key\)/, 2]], + "plugins/sources/frontend/public/javascripts/countly.views.js": + [[/isForbiddenFieldName\(source\)/, 1], [/var sourceBucket = /, 1]], + "plugins/push/frontend/public/javascripts/countly.views.component.common.js": + [[/isForbiddenFieldName\(category\)/, 1]] + }; + Object.keys(GUARDS).forEach(function(rel) { + it("guards " + rel.split("/").pop(), function() { + var src = read(rel); + GUARDS[rel].forEach(function(pair) { + (src.match(new RegExp(pair[0].source, "g")) || []).length.should.equal(pair[1]); + }); + }); + }); + }); + + // NB: no assertion here reads .eslintrc.json - CI runs these tests from a copy made + // with `cp -rf ./* /opt/countly`, which skips dotfiles, so the config at the test cwd + // is not the PR's. The rule's scoping is enforced by the `lint` CI job and the rule's + // own RuleTester suite; this file only proves the guarded source stays clean. + + describe("the two global plugin sinks stay out of Object.prototype", function() { + function isForbidden(n) { + return n === "__proto__" || n === "constructor" || n === "prototype"; + } + afterEach(function() { + ["pollViews", "PWN"].forEach(function(k) { + delete Object.prototype[k]; + }); + }); + it("views models.js json walk does not reach Object.prototype", function() { + // mirrors the guarded for (var k in json) merge in plugins/views countly.models.js + var graphDataObj = {}; + var json = JSON.parse('{"__proto__":{"pollViews":{"x":1}},"real_name":"ok"}'); + for (var k in json) { + if (isForbidden(k)) { + continue; + } + if (k.indexOf("_name") > -1) { + graphDataObj[k] = json[k]; + } + else if (graphDataObj[k]) { + for (var z in json[k]) { + graphDataObj[k][z] = json[k][z]; + } + } + else { + graphDataObj[k] = json[k]; + } + } + Object.prototype.should.not.have.property("pollViews"); + graphDataObj.real_name.should.equal("ok"); + }); + it("sources derived source index does not reach Object.prototype", function() { + // mirrors the guarded + laundered loop in plugins/sources countly.views.js + var dataMap = {}; + var cleanData = { r1: { sources: "PWN" }, r2: { sources: "Chrome" } }; + function getSourceName(v) { + return v === "PWN" ? "__proto__" : v; + } + var source; + for (var i in cleanData) { + source = getSourceName(cleanData[i].sources); + if (isForbidden(source)) { + continue; + } + if (!dataMap[source]) { + dataMap[source] = {}; + } + var bucket = dataMap[source]; + bucket[cleanData[i].sources] = cleanData[i]; + } + Object.prototype.should.not.have.property("PWN"); + dataMap.Chrome.Chrome.should.eql({ sources: "Chrome" }); + }); + }); +});