[fix][core] keep segmentation values out of Object.prototype - #7923
Open
ar2rsawseen wants to merge 11 commits into
Open
[fix][core] keep segmentation values out of Object.prototype#7923ar2rsawseen wants to merge 11 commits into
ar2rsawseen wants to merge 11 commits into
Conversation
An event segmentation value arriving on the unauthenticated /i endpoint becomes a MongoDB field name at d.<day>.<value>.<metric>. A value of "__proto__" survived the transforms (no $, no ., not a day number) and stored a literal __proto__ field. On read, deepMerge in fetch.js walks the document with for...in, and because the driver returns __proto__ as an own enumerable property, merged into ob1[i] where ob1[i] resolves to Object.prototype. That writes into the worker's prototype for the rest of its life, so every subsequent read, for every app the worker serves, returns corrupted numbers. The daily api:topEvents job triggers the read across all apps on its own. #7634 guarded the segmentation KEY. This is the VALUE path four lines down, plus two more places the same class reaches, plus the sink: - common.isForbiddenFieldName, one predicate for the three prototype-member names, now also used by the key guard #7634 hard-coded. - events.js: the segmentation value is prefixed with [CLY] when it names a prototype member, the way forbidden day numbers already are. - common.js recordSegmentMetric: the same for the metric value, which builds an identical d.<...> field name. - fetch.js deepMerge: skips inherited keys and the three names outright. This is the durable guard, and the only one that also neutralises documents poisoned before the input paths were fixed, since those re-pollute on every read. Verified by lifting the real deepMerge and merging a document with an own-enumerable __proto__ field: before, Object.prototype.c/.s are set process-wide; after, they stay undefined and a legitimate merge still sums (Chrome.c 3+3 = 6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getMergedEventData walks stored event documents through five nested for...in levels and merges them with mergedEventOutput[l1][l2][l3][l4][l5] += ... . It is the same defect as deepMerge and the same reachability: a segmentation value of "__proto__" is a key at one of those levels, indexing the target with it resolves to Object.prototype rather than an own slot, and the guard on each level is "if (!mergedEventOutput[...])", which a prototype is truthy for, so it is never replaced before the assignment writes through it. This was missed on the first pass. It is not named like a merge helper and does not recurse, so both a search for merge functions and a search for recursive walkers skip it; only a search for nested bracket assignment finds it. Fixing deepMerge alone left the read path exploitable from any already-poisoned document, which is exactly what the report said the sink fix was for. - isMergeableKey, one guard shared by every walk of a stored document in this file: own-property check plus common.isForbiddenFieldName. deepMerge now uses it too, so the file has a single rule rather than two spellings of it. - All five levels of getMergedEventData guarded. - The meta reduce guarded as well. A prototype key there does not pollute, since it assigns whole values, but acc[key].concat would be called on Object.prototype and throw, failing the read. Verified by lifting the real loop and running it in this realm, not a vm context, whose separate Object.prototype hides the result: before, Object.prototype.c/.s are set process-wide from one merged document; after they stay undefined and the legitimate merge still accumulates (Chrome.c 3+3 = 6). Confirmed on master, 24.05 and platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pe keys
An AST scan for the sink shape, rather than for functions named like merges, found
that fixing deepMerge and getMergedEventData was not enough.
getMergedObj walks a stored document's days and segmentation values and writes
mergedDataObj[year][month][prop][secondLevel]. Same defect, same reachability, and
the "if (!mergedDataObj[year][month][prop])" guard cannot help: for a "__proto__"
key that expression is Object.prototype, which is truthy, so it is never replaced
before the assignment writes through it. Proven on the real loop: before, one merged
document sets Object.prototype.c/.s process-wide; after, they stay undefined and the
legitimate segment still merges.
Guarded here: the day loop, the segmentation-value loop, the metric loop, both meta
merges and the three walks of mergedDataObj.meta, all through the isMergeableKey
helper this file already uses.
The same scan surfaced two more, in different files and with no shared naming:
- plugins/users: `action` comes from JSON.parse(user_details), and userDetails is a
locally built object, so userDetails["__proto__"] is the prototype and the
existing truthiness check passes for it. Reachable from sdk ingestion.
- plugins/drill: `summed[key] = summed[key] || {}` keeps Object.prototype rather
than replacing it when key is "__proto__", and the next line writes onto it.
Both verified by lifting the real code and observing the pollution before the guard.
Neighbouring sites where the target is the very object carrying the own __proto__
are deliberately left alone: reading such a key returns the own value, which was
confirmed by test rather than assumed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the standalone scanner with a real lint rule, so the check runs wherever eslint runs: in CI, in editors, and in the pre-commit hook. bin/eslint-rules/no-prototype-pollution-sink.js reports a write that indexes THROUGH a key taken from an enumerated object, target[k][x] = v, which reaches Object.prototype when k is "__proto__" and the target has no own property of that name. It does not report target[k] = v: there the setter fires and reparents a local object, which was confirmed by test rather than assumed. Two things make it usable rather than noisy: - It recognises a fix. A loop that opens with an `if` mentioning the key and continuing is treated as guarded, so guarding a site quietens the rule instead of forcing an entry in a list of exceptions. Without this the only way to silence a correctly fixed site would be to record it as "reviewed", which is the wrong record to leave behind. The trade is that the rule trusts such a guard without proving the test is sufficient; that is stated in the rule's header. - Sites that are safe for a different reason, typically the target being the very object that carries the own __proto__, are listed in bin/eslint-rules/prototype-pollution-reviewed.json. The rule loads that itself, so both an eslintrc and a flat config need only "error". The message prints the exact signature to add, so recording one is a copy-paste. Wiring differs per repo and the configs are not interchangeable: - eslintrc, eslint 8: rulePaths in the Gruntfile, since grunt-eslint forwards options to new ESLint(), plus --rulesdir for the lint-staged CLI invocation. Both paths verified. - flat config, eslint 10: the rule registered as a local plugin in eslint.config.mjs, scoped to api/** and plugins/*/api/**, where keys come from mongo and from JSON.parse of request payloads. Enterprise plugins have no eslint of their own and are linted through the core submodule, so they inherit this. Tests: RuleTester over the reporting and non-reporting shapes, including for-of over Object.keys, a guarded loop, and that nested loops report a write once rather than once per enclosing key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… in the rule
Follow-up from triaging the sites the rule had recorded rather than reviewed. Each was
checked against two questions, both settled by test rather than by reading: can the
enumerated object carry an own "__proto__" (only BSON and JSON.parse produce one, a
JS assignment never does), and is the write target a different object from the one being
enumerated.
Fixed, each with the shape reproduced first:
- push parts/note.js: `data` comes off a stored message and `compiled` is built in the
function, so compiled["__proto__"] is the prototype and satisfies every test in the
existing guard chain. Verified: the unguarded shape sets Object.prototype.leak.
- drill mapped/eventMeta: `z` is a key off ingested data while mapped and eventMeta are
built locally, so `if (!mapped[groups[k]][z])` and `eventMeta[...][z] || {}` both keep
Object.prototype rather than replacing it. Verified: sets a prototype property from the
poisoned array's elements while a legitimate key does not.
- drill result/meta_up: same, and typeof result[i][j] is "object" for a prototype rather
than "undefined", so that initialiser does not fire either.
Confirmed safe, with the reason recorded rather than assumed:
- topEvents, countly.model, exports body, revenue, flows, and the users
user_details[prop][key] writes: the target is the enumerated object itself, so
indexing it with an own "__proto__" returns that own value.
- dashboards widgetData and the push resultor accumulators: the enumerated object is
built in JS with plain assignment, which cannot create an own "__proto__", so its
for...in never yields one.
- cohorts newQuery: the writes sit inside `else if (key === '$or' || key === '$and')`,
which is an effective allowlist. The rule cannot see an enclosing equality test, only
a leading guard, so this stays a recorded exception.
The rule now also treats a value used as a key as dangerous: Object.values, and the
value element of an Object.entries destructuring. That is the original defect's own
shape, a segmentation VALUE becoming a field name, and it was outside the rule's view.
Measured: zero new findings in either repo, so the gap closes at no cost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rule shipped with a file recording the sites it flagged, so that it would only fail on new ones. That file was generated, not reviewed, while describing itself as reviewed, and shipping a list like that invites trusting a judgement nobody made. So there is no list any more. Every loop the rule reports is guarded in the source instead: it skips the three prototype member names before doing anything else. The rule takes no options, and its output is simply empty. A report now means new code rather than an entry to add somewhere. Guards were inserted at every flagged loop, all of which have block bodies, and each repo verified afterwards: the rule reports zero, eslint is clean on every touched file, and the unit suites pass. Loops guarded per repo: countly-server master 17, release 24.05 16, platform 49, enterprise master 28, enterprise 24.05 27. Skipping these keys is the behaviour we want regardless of whether a given site could be reached: a segment, metric or property literally named __proto__ is not data anyone wants aggregated, and the alternative at each site was to reason about whether that particular target had an own property of that name. Several of those judgements were subtle enough to be worth not relying on, which is the same reason the report asked for the sink to be fixed rather than only the input. The rule's message now says to guard the loop, since that is the only remedy it offers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Gruntfile and lint-staged both point eslint at bin/eslint-rules, but the CI step runs a bare `npx eslint .`, so no-prototype-pollution-sink is referenced in .eslintrc.json without ever being loaded. eslint 8 treats that as an error per file and the step fails on all 216 files matched by the override. Verified on this branch: `npx eslint .` exits 1 with 216 "Definition for rule 'no-prototype-pollution-sink' was not found"; with --rulesdir it exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…of them hid
The rule had two false negatives, both confirmed by running the shapes rather than by
reading them, and the first was hiding a live sink in this very PR.
1. opensWithKeyGuard accepted any leading if/continue that merely mentioned the key,
including a bare hasOwnProperty check. That check does NOT stop a prototype key: a
document out of JSON.parse or BSON carries "__proto__" as an OWN property, so
hasOwnProperty returns true and the loop body runs anyway. A guard now has to name a
prototype member or call a helper that rejects them (isForbiddenFieldName,
isMergeableKey and the like still read as fixed).
2. The rule only tracked the loop key, so a write through a value READ off the enumerated
object was invisible - which is exactly how the original defect worked, a segmentation
value becoming a field name. Names bound inside the loop to something read through the
key are now tracked too, and a guard on the key deliberately does not clear them,
because guarding a key says nothing about a value.
Turning (1) on surfaced one site in this repo: mergeEvents in api/parts/data/events.js.
It opens with hasOwnProperty, which passes an own "__proto__", and its second guard
`if (!firstObj[firstLevel])` cannot fire either, because firstObj["__proto__"] is
Object.prototype and truthy. Both writes below then land on the prototype for the life
of the worker.
Proven by lifting the function and merging {"__proto__": {"pollutedCount": 42}}:
before, ({}).pollutedCount became 42 process-wide; after, it stays undefined and a
legitimate segment still sums (1 + 3 = 4). Guarded with the same isForbiddenFieldName
the other sites in this PR use.
Only firstLevel needs it. secondLevel writes one level into firstObj[firstLevel], where
the __proto__ setter just reparents that local object.
Verified: `npx eslint . --rulesdir bin/eslint-rules` exits 0, and the rule suite is 18
passing, including the two shapes above and the guard styles that must keep passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
countlyCommon.extendDbObj and mergeMetricsByName write THROUGH a key or value taken from a stored document, so a segmentation value or an own "__proto__" field survives as a field name and reaches Object.prototype for the life of the dashboard page. Add a countlyCommon.isForbiddenFieldName helper (the client mirror of api/utils/common.js) and skip the three prototype member names at the top of both merges and of the seven countly.event.js response walks. Adds test/unit-tests/frontend.segmentation-value-prototype.js, which lifts the real functions out of the browser IIFE and runs them on an own-__proto__ payload: the two common.js merges no longer touch Object.prototype and the event loops no longer reparent a local object or surface a bogus "__proto__" row, while ordinary segments still merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…inds Turn no-prototype-pollution-sink on for frontend/express/public/javascripts/ countly and plugins/*/frontend and guard every site it reports, keeping the rule's no-exceptions contract. Two were latent global sinks: the views plugin walked an API response with for (var k in json) and wrote through _graphDataObj[k][z], and the sources plugin indexed dataMap through a source name derived from the response. Both now skip prototype member names, the sources write going onto an already-selected bucket so the derived value index no longer reaches a prototype. The remaining reports (the auth app/permission maps, the session sparkline and the _myRequests walks, the sdk config and push emoji loops) iterate objects the code builds itself, so the guard is a cheap no-op that documents the invariant and keeps the rule clean. Extends test/unit-tests/frontend.segmentation-value-prototype.js with the guard-presence checks, the eslintrc scope check, and a behavioural proof that the two global sinks no longer pollute Object.prototype. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| } | ||
|
|
||
| // minimal stand-ins for the browser globals the lifted functions touch | ||
| var moment = function() { |
| return Object.values(o); | ||
| } | ||
| }; | ||
| var jQuery = { i18n: { map: { "common.unknown": "Unknown" } } }; |
The frontend prototype-pollution test asserted that .eslintrc.json scopes the sink rule to the dashboard, but test-api-core runs from a copy made with `cp -rf ./* /opt/countly`, which skips dotfiles, so the config at the test cwd is a base checkout without the override — the assertion fails there while passing locally. The rule's scoping is already enforced by the `lint` CI job and the rule's own RuleTester suite, so drop the redundant config read. The file keeps its guard-presence and behavioural checks, which only read copied (non-dotfile) source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What was wrong
An event segmentation value on the unauthenticated
/iendpoint becomes a MongoDB field name atd.<day>.<value>.<metric>. A value of__proto__passed every transform — no$, no., not one of the day numbers inforbiddenSegValues— and stored a document with a literal__proto__field.On read,
deepMergeinfetch.jswalks the document withfor...in. The driver returns__proto__as an own enumerable property, so the loop yields it, and:merges the stored object into
Object.prototypeof the worker. That persists for the worker's whole life, so every later read, for every app it serves, returns corrupted numbers. The dailyapi:topEventsjob triggers the read across all apps unprompted.#7634guarded the segmentation key. This is the value path four lines down, which that guard never reached, plus two more sites of the same class and the sink itself.What changed
common.isForbiddenFieldName— one predicate for__proto__/constructor/prototype, now also used by the key guard#7634hard-coded, so the whole class reads through one function.events.js— the segmentation value is prefixed with[CLY]when it names a prototype member, exactly as forbidden day numbers already are.common.jsrecordSegmentMetric— the same for the metric value, which builds an identicald.<...>field name.fetch.jsdeepMerge— skips inherited keys (hasOwnProperty) and the three names. This is the durable guard: it also neutralises documents already poisoned in existing databases, which the input guards alone cannot, since those re-pollute on every read.Verified against the unpatched code
The real
deepMergewas lifted and fed a document with an own-enumerable__proto__field, the way the mongo driver deserializes one:Unit test
test/unit-tests/api.data.segmentation-value-prototype.js: the exported predicate, plus source-level assertions that the two value paths run through it and thatdeepMergeskips inherited and prototype-member keys. 5 cases, all failing against the unpatched files.Scope
The corruption is in the running process, not the stored data — the database is never wrong, and a poisoned document re-pollutes each fresh worker on read until the code is fixed. Property names are confined to the
common.dbMapmetrics and the values are numbers, so this is data-integrity corruption, not RCE.Second sink, found after the first commit
getMergedEventDatain the same file walks stored event documents through five nestedfor...inlevels and merges withmergedEventOutput[l1][l2][l3][l4][l5] += …. Same defect, same reachability: a segmentation value is a key at those levels, and each level's guard isif (!mergedEventOutput[…]), which a prototype is truthy for, so it is never replaced before the assignment writes through it.Fixing
deepMergealone therefore left the read path exploitable from any already-poisoned document, which defeats the point of the sink fix.Worth recording why it was missed: it is not named like a merge helper and it does not recurse, so both a search for merge functions and a search for recursive walkers skip it. Only a search for nested bracket assignment finds it.
isMergeableKeyis now the single guard for every walk of a stored document in this file, anddeepMergeuses it too.metareduce, where a prototype key would call.concatonObject.prototypeand throw rather than pollute.Proof, on all three targets:
The harness runs the lifted loop in the host realm rather than a
vmcontext. A vm has its ownObject.prototype, so pollution inside it is invisible to a probe outside and the test passes either way; my first attempt at this made exactly that mistake.Client / dashboard side (added in this PR)
The same class exists in the dashboard: a segmentation value, event key or metric name becomes an object key in the browser too, and an API response delivered as JSON can carry
__proto__as an own enumerable property. Two hand-rolled merges incountly.common.jswrite through such a key and reachObject.prototypefor the life of the page.countlyCommon.isForbiddenFieldName— the browser mirror of the server predicate.mergeMetricsByName— pollutes through a data value index (uniqueNames[newName][k],newNamea segmentation/range value); the value is now skipped before use.extendDbObj— walks a fetched day document withfor (var level1 in …); the existinghasOwnPropertydoes not exclude an own__proto__, so both levels now skip the prototype names.for…inwalks incountly.event.js(readers / write-AT loops), guarded for consistency.Proof, real functions lifted out of the browser IIFE and fed an own-
__proto__payload:The eslint rule, extended to the dashboard
no-prototype-pollution-sinkwas scoped toapi/**+plugins/*/api/**. It is now also switched on forfrontend/express/public/javascripts/countly/**andplugins/*/frontend/**, and every site it reports there is guarded in source — keeping the no-exceptions contract. It surfaced two genuinely global sinks the manual review had not named:countly.models.js—for (var k in json)over an API response, writing through_graphDataObj[k][z].countly.views.js— an index through asourcename derived from the response; fixed by skipping the prototype names and writing onto an already-selected bucket, so the derived value no longer sits mid-chain in the assignment target.The remaining reports (auth app / permission maps, the session sparkline walk, the two
_myRequestswalks, the sdk config and push emoji loops) iterate objects the code builds itself, so the guard is a cheap no-op that keeps the rule clean. eslint reports 0 across api + plugins-api + frontend + plugins-frontend.Proof, the two global plugin sinks:
Unit test
test/unit-tests/frontend.segmentation-value-prototype.js: the predicate, the two liftedcommon.jssinks and two liftedevent.jsfunctions run on an own-__proto__payload, plus guard-presence and eslint-scope assertions and a behavioural proof for the two plugin sinks. 21 cases.