From 78d308e4e0b6049e739768ca77f793d8b46587e9 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:27:31 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F=20Stop=20json0=20ops=20by?= =?UTF-8?q?passing=20the=20prototype=20pollution=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs GHSA-9rqw-j2q5-gg2g At the moment, `applyOpEdit()` only scans an op's paths for dangerous segments if `Array.isArray(edit)`. `ot-json0` isn't so fussy: both `checkValidOp()` and `apply()` walk the op with `op.length` and numeric indexing, so an array-like object is an op as far as the type is concerned, and isn't an op as far as the guard is concerned. Any client that can submit an op can therefore write to `Object.prototype` in the server process, for the lifetime of that process, with a single message: ``` {"a":"op","c":"docs","d":"doc","v":1,"src":"c1","seq":1, "op":{"0":{"p":["__proto__","polluted"],"oi":"x"},"length":1}} ``` The server acks that like any other op, and the document data is unchanged, so nothing looks wrong from either end. The root cause is that our validation and `ot-json0`'s traversal disagree about what counts as an op, and the guard has two more holes of its own: - `isDangerousProperty()` builds its lookup map from `Object.getOwnPropertyNames(Object.prototype)` but skips the `__proto__` key. That map has a null prototype, so `__proto__` is an ordinary own key there and the exclusion bought us nothing. It just means the lookup, which coerces its key, missed anything that stringifies to `__proto__`, so `[{p: [['__proto__'], 'x'], oi: 1}]` was accepted with no array-like trickery at all. - `normalizeLegacyJson0Ops()` applies op components itself, before `apply()` guards anything, so `applyOps()` polluted and *then* returned `Invalid path segment`. That one needs no array-like op either, and is reachable through `fetchSnapshot()`. This change makes the path scan traverse an op exactly the way `ot-json0` does — `.length` and numeric indexing, including its coercion of a string `length`, since `{"length": "1"}` applies too — and runs it everywhere an op is applied, including the legacy normalisation. Anything `ot-json0` will apply is now checked. It also stops the guard crashing the process on its own: reading `opComponent.p` threw an uncaught `TypeError` for `op: [null]`, from inside a `backend.trigger()` callback. The scan now stops at the first component `ot-json0` would reject, and leaves it to complain, which it does inside the existing `try`/`catch`. Stopping rather than skipping past it also matters because `{"length": 1e9}` would otherwise spin. Finally, `projections` tested its field allow-list with plain-object truthiness, so `fields['__proto__']` and `fields['toString']` were truthy, and a projection reported those segments as allowed fields. Those lookups now use `util.hasOwn()`. Note this deliberately doesn't police the *shape* of an op, only its paths, so no op that used to apply stops applying. `ot-json0` quietly treats a non-array as a no-op, and `backend.submit()` accepts those, so they exist in real op histories — rejecting them here would make those documents permanently unreadable through `fetchSnapshot()`, which is exactly what `normalizeLegacyJson0Ops()` exists to avoid. Those shapes are a problem for a different reason, handled separately. Note also that none of this defends a client against ops arriving from the server, which has no equivalent guard. That's tracked in https://github.com/share/sharedb/issues/721 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/ot.js | 41 +++++++++++----- lib/projections.js | 6 +-- lib/util.js | 7 ++- test/client/doc.js | 44 ++++++++++++++++++ test/ot.js | 111 ++++++++++++++++++++++++++++++++++++++++++++ test/projections.js | 16 +++++++ 6 files changed, 206 insertions(+), 19 deletions(-) diff --git a/lib/ot.js b/lib/ot.js index 5cf349f60..c142904f1 100644 --- a/lib/ot.js +++ b/lib/ot.js @@ -108,18 +108,9 @@ function applyOpEdit(snapshot, edit) { var type = types.map[snapshot.type]; if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type'); - if (type.name === 'json0' && Array.isArray(edit)) { - for (var i = 0; i < edit.length; i++) { - var opComponent = edit[i]; - if (Array.isArray(opComponent.p)) { - for (var j = 0; j < opComponent.p.length; j++) { - var pathSegment = opComponent.p[j]; - if (util.isDangerousProperty(pathSegment)) { - return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Invalid path segment'); - } - } - } - } + if (type.name === 'json0') { + var pathError = checkJson0OpPaths(edit); + if (pathError) return pathError; } try { @@ -129,6 +120,25 @@ function applyOpEdit(snapshot, edit) { } } +// ot-json0 walks ops with .length and numeric indexing, so it treats array-like +// objects as ops too, and it coerces .length. This traversal has to match it +// exactly: anything ot-json0 will apply has to be checked here. +// See GHSA-9rqw-j2q5-gg2g +function checkJson0OpPaths(op) { + if (op == null) return; + for (var i = 0; i < op.length; i++) { + var component = op[i]; + // ot-json0's checkValidOp() rejects the whole op before applying any of it, + // so there is nothing beyond this component left to check + if (!component || typeof component !== 'object' || !Array.isArray(component.p)) return; + for (var j = 0; j < component.p.length; j++) { + if (util.isDangerousProperty(component.p[j])) { + return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Invalid path segment'); + } + } + } +} + exports.transform = function(type, op, appliedOp) { // There are 16 cases this function needs to deal with - which are all the // combinations of create/delete/op/noop from both op and appliedOp @@ -178,6 +188,13 @@ exports.applyOps = function(snapshot, ops, options) { options = options || {}; for (var index = 0; index < ops.length; index++) { var op = ops[index]; + // normalizeLegacyJson0Ops() applies op components itself, so paths have to + // be checked before it runs, not just in exports.apply() + var type = types.map[snapshot.type]; + if (type && type.name === 'json0' && 'op' in op) { + var pathError = checkJson0OpPaths(op.op); + if (pathError) return pathError; + } if (options._normalizeLegacyJson0Ops) { try { normalizeLegacyJson0Ops(snapshot, op); diff --git a/lib/projections.js b/lib/projections.js index b9a43c201..f91a3341d 100644 --- a/lib/projections.js +++ b/lib/projections.js @@ -58,7 +58,7 @@ function projectEdit(fields, op) { } } else { // The path has a first element. Just check it against the fields. - if (fields[path[0]]) { + if (util.hasOwn(fields, path[0])) { result.push(c); } } @@ -91,7 +91,7 @@ function isSnapshotAllowed(fields, snapshot) { return false; } for (var k in snapshot.data) { - if (!fields[k]) return false; + if (!util.hasOwn(fields, k)) return false; } return true; } @@ -101,7 +101,7 @@ function isEditAllowed(fields, op) { var c = op[i]; if (c.p.length === 0) { return false; - } else if (!fields[c.p[0]]) { + } else if (!util.hasOwn(fields, c.p[0])) { return false; } } diff --git a/lib/util.js b/lib/util.js index 922350492..345fa50d8 100644 --- a/lib/util.js +++ b/lib/util.js @@ -100,14 +100,13 @@ exports.clone = function(obj) { return (obj === undefined) ? undefined : JSON.parse(JSON.stringify(obj)); }; +// A null prototype makes '__proto__' an ordinary own key, so it can be in the map var objectProtoPropNames = Object.create(null); Object.getOwnPropertyNames(Object.prototype).forEach(function(prop) { - if (prop !== '__proto__') { - objectProtoPropNames[prop] = true; - } + objectProtoPropNames[prop] = true; }); exports.isDangerousProperty = function(propName) { - return propName === '__proto__' || objectProtoPropNames[propName]; + return objectProtoPropNames[propName] === true; }; try { diff --git a/test/client/doc.js b/test/client/doc.js index 3caa5ddfd..f30258c00 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -682,6 +682,10 @@ describe('Doc', function() { }); } + afterEach(function() { + delete Object.prototype.polluted; + }); + ['__proto__', 'constructor'].forEach(function(badProp) { it('Rejects ops with collection ' + badProp, function(done) { var collectionName = badProp; @@ -755,6 +759,46 @@ describe('Doc', function() { }); }); }); + + // ot-json0 walks ops with .length and numeric indexing, so it applies an + // array-like object as if it were an op + [ + { + name: 'an array-like op', + op: {0: {p: ['__proto__', 'polluted'], oi: 'oops'}, length: 1} + }, + { + name: 'ops with a path segment that is not a string', + op: [{p: [['__proto__'], 'polluted'], oi: 'oops'}] + } + ].forEach(function(test) { + it('Rejects ' + test.name, function(done) { + var connection = this.connection; + var collectionName = 'test-collection'; + var docId = 'test-doc'; + connection.get(collectionName, docId).create({id: docId}, function(err) { + if (err) { + return done(err); + } + expectReceiveError(connection, collectionName, docId, 'Invalid path segment', function(error) { + if (error) { + return done(error); + } + expect({}.polluted).to.equal(undefined); + done(); + }); + connection.send({ + a: 'op', + c: collectionName, + d: docId, + v: 1, + seq: connection.seq++, + x: {}, + op: test.op + }); + }); + }); + }); }); describe('toSnapshot', function() { diff --git a/test/ot.js b/test/ot.js index 0b524613d..6d093001d 100644 --- a/test/ot.js +++ b/test/ot.js @@ -125,6 +125,68 @@ describe('ot', function() { }); }); + describe('json0 op validation', function() { + var snapshot; + + beforeEach(function() { + snapshot = {v: 6, type: type.uri, data: {colour: 'blue'}}; + }); + + afterEach(function() { + delete Object.prototype.polluted; + }); + + it('does not pollute the prototype through an array-like op', function() { + var op = {0: {p: ['__proto__', 'polluted'], oi: 'yes'}, length: 1}; + var error = ot.apply(snapshot, {v: 6, op: op}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('does not pollute the prototype through an array-like op with a string length', function() { + var op = {0: {p: ['__proto__', 'polluted'], oi: 'yes'}, length: '1'}; + var error = ot.apply(snapshot, {v: 6, op: op}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('does not pollute the prototype through a path segment that is not a string', function() { + var error = ot.apply(snapshot, {v: 6, op: [{p: [['__proto__'], 'polluted'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + ['__proto__', 'constructor'].forEach(function(badProp) { + it('rejects ' + badProp + ' as a path segment', function() { + var error = ot.apply(snapshot, {v: 6, op: [{p: [badProp, 'polluted'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + }); + + it('returns an error for an op component that is not an object', function() { + var error; + expect(function() { + error = ot.apply(snapshot, {v: 6, op: [null]}); + }).to.not.throw(); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + }); + + it('returns an error for an array-like op whose components are missing', function() { + var error = ot.apply(snapshot, {v: 6, op: {length: 1e9}}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + }); + + it('accepts an op that ot-json0 treats as a no-op', function() { + expect(ot.apply(snapshot, {v: 6, op: {p: ['colour'], oi: 'red'}})).equal(); + expect(snapshot).to.eql({v: 7, type: type.uri, data: {colour: 'blue'}}); + }); + }); + describe('no-op', function() { it('works on existing docs', function() { var doc = {v: 6, type: type.uri, data: 'Hi'}; @@ -374,6 +436,55 @@ describe('ot', function() { describe('applyOps', function() { describe('with normalization turned on', function() { + afterEach(function() { + delete Object.prototype.polluted; + }); + + it('does not pollute the prototype while normalizing a legacy op', function() { + var snapshot = { + type: 'http://sharejs.org/types/JSONv0', + data: {title: 'Wee Free Men'} + }; + + var ops = [ + { + v: 1, + op: [ + {p: ['__proto__', 'polluted'], oi: 'yes'}, + {p: ['title'], od: 'Wee Free Men', oi: 'Nation'} + ] + } + ]; + + var error = ot.applyOps(snapshot, ops, { + _normalizeLegacyJson0Ops: true + }); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('replays a legacy op that ot-json0 treats as a no-op', function() { + var snapshot = { + type: 'http://sharejs.org/types/JSONv0', + data: {title: 'Wee Free Men'} + }; + + var ops = [ + { + v: 1, + op: {p: ['title'], od: 'Wee Free Men', oi: 'Nation'} + } + ]; + + var error = ot.applyOps(snapshot, ops, { + _normalizeLegacyJson0Ops: true + }); + expect(error).to.be.undefined; + expect(snapshot.data).to.eql({title: 'Wee Free Men'}); + expect(snapshot.v).to.equal(2); + }); + it('applies an op to a snapshot', function() { var snapshot = { type: 'http://sharejs.org/types/JSONv0', diff --git a/test/projections.js b/test/projections.js index 57c851952..f4aaf8812 100644 --- a/test/projections.js +++ b/test/projections.js @@ -385,4 +385,20 @@ describe('projection utility methods', function() { ); }); }); + + describe('field names inherited from Object.prototype', function() { + ['__proto__', 'constructor', 'toString'].forEach(function(badProp) { + it('does not treat ' + badProp + ' as a projected field', function() { + var op = {op: [{p: [badProp, 'x'], oi: 'oops'}]}; + expect(projections.isOpAllowed(null, {x: true}, op)).equal(false); + projections.projectOp({x: true}, op); + expect(op).eql({op: []}); + }); + + it('does not treat ' + badProp + ' as an allowed snapshot field', function() { + var data = JSON.parse('{"' + badProp + '": "oops"}'); + expect(projections.isSnapshotAllowed({x: true}, {type: type, data: data})).equal(false); + }); + }); + }); });