From 30b35bdbc5015cd8baded000c6a1a9f6458f4be0 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:06:44 +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 three 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 buys us nothing. It just means the lookup, which coerces its key, misses anything that stringifies to `__proto__`, so `[{p: [['__proto__'], 'x'], oi: 1}]` was accepted with no array-like trickery at all. - `op: [null]` threw a `TypeError` out of the guard itself, from inside a `backend.trigger()` callback, which takes the process down. - `normalizeLegacyJson0Ops()` applies op components itself, before `apply()` guards anything, so `applyOps()` polluted and *then* returned `Invalid path segment`. 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` — and uses it everywhere an op is applied, including the legacy normalisation. Anything `ot-json0` will apply is now checked. Submitted ops are additionally held to the shape `ot-json0` documents, and that happens in `submit-request` rather than in `ot.apply()`. It has to be before the op reaches any type function, because `$fixup()` in `apply` middleware composes the op, and `ot-json0`'s `compose()` throws on an array-like, which is another way to take the process down. It can't go in `checkOp()`, which runs before we have the snapshot and so doesn't know the document's type. Note the shape check deliberately doesn't apply to ops we've already committed. `ot-json0` quietly treats a non-array as a no-op, and `backend.submit()` accepted those until now, so they exist in real op histories — three of our own tests submitted them. Rejecting them on replay would make those documents permanently unreadable through `fetchSnapshot()`, which is exactly what `normalizeLegacyJson0Ops()` exists to avoid. The path check, not the shape check, is what closes the pollution; the shape check is ingress hardening. 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()`. This does change `backend.submit()`: a json0 op that isn't an array used to commit as a silent no-op and bump the version, and is now rejected with `ERR_OT_OP_BADLY_FORMED`. Clients are unaffected, since `Doc._submit()` runs `type.normalize()` first, which wraps a bare component into an array. 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 | 67 +++++++++++++++---- lib/projections.js | 6 +- lib/submit-request.js | 6 ++ lib/util.js | 7 +- test/backend.js | 6 +- test/client/doc.js | 74 +++++++++++++++++++++ test/ot.js | 149 ++++++++++++++++++++++++++++++++++++++++++ test/projections.js | 16 +++++ 8 files changed, 309 insertions(+), 22 deletions(-) diff --git a/lib/ot.js b/lib/ot.js index 5cf349f60..4618598bc 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,51 @@ 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'); + } + } + } +} + +// Submitted ops are held to the shape ot-json0 documents, so that nothing +// array-like can be committed in the first place. Ops that are already +// committed only get their paths checked, since ops that ot-json0 quietly +// treats as no-ops were committable by older versions of ShareDB +function checkJson0Op(op) { + if (!Array.isArray(op)) { + return new ShareDBError(ERROR_CODE.ERR_OT_OP_BADLY_FORMED, 'json0 op must be an array'); + } + + for (var i = 0; i < op.length; i++) { + var component = op[i]; + if (!component || typeof component !== 'object' || !Array.isArray(component.p)) { + return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Missing path'); + } + } + + return checkJson0OpPaths(op); +} + +exports.checkOpForType = function(typeName, op) { + if (!('op' in op)) return; + var type = types.map[typeName]; + if (!type || type.name !== 'json0') return; + return checkJson0Op(op.op); +}; + 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 +214,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/submit-request.js b/lib/submit-request.js index d2cac8d99..afcff7aca 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -109,6 +109,12 @@ SubmitRequest.prototype.submit = function(callback) { request.snapshot = snapshot; request._addSnapshotMeta(); + // The type is only known once we have the snapshot, so this is the earliest + // we can validate the op against it. It has to happen before the op reaches + // any type function, including through $fixup() in the apply middleware + var opError = ot.checkOpForType(snapshot.type, op); + if (opError) return callback(opError); + if (op.v == null) { if (op.create && snapshot.type && op.src) { // If the document was already created by another op, we will return a 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/backend.js b/test/backend.js index e0af12565..8ab84c662 100644 --- a/test/backend.js +++ b/test/backend.js @@ -207,7 +207,7 @@ describe('Backend', function() { title: '1984', author: 'George Orwell' }); - var op = {op: {p: ['publication'], oi: 1949}}; + var op = {op: [{p: ['publication'], oi: 1949}]}; stream.on('data', function(data) { expect(data.op).to.eql(op.op); done(); @@ -245,7 +245,7 @@ describe('Backend', function() { done(); }); - var op = {op: {p: ['publicationYear'], oi: 1949}}; + var op = {op: [{p: ['publicationYear'], oi: 1949}]}; backend.submit(agent, 'books', '1984', op, null, function(error) { if (error) done(error); }); @@ -262,7 +262,7 @@ describe('Backend', function() { done(); }); - var op = {op: {p: ['publicationYear'], oi: 1949}}; + var op = {op: [{p: ['publicationYear'], oi: 1949}]}; backend.submit(agent, 'books', '1984', op, null, function() { // Swallow the error }); diff --git a/test/client/doc.js b/test/client/doc.js index 3caa5ddfd..55833ca22 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,76 @@ describe('Doc', function() { }); }); }); + + [ + { + name: 'an array-like op', + op: {0: {p: ['__proto__', 'polluted'], oi: 'oops'}, length: 1}, + error: 'json0 op must be an array' + }, + { + name: 'ops with a path segment that is not a string', + op: [{p: [['__proto__'], 'polluted'], oi: 'oops'}], + error: 'Invalid path segment' + }, + { + name: 'ops with a component that is not an object', + op: [null], + error: 'Missing path' + } + ].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, test.error, 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 + }); + }); + }); + }); + + it('Rejects an array-like op before the apply middleware can fix it up', function(done) { + var connection = this.connection; + var collectionName = 'test-collection'; + var docId = 'test-doc'; + this.backend.use('apply', function(request, next) { + if ('op' in request.op) request.$fixup([{p: ['fixed'], oi: true}]); + next(); + }); + connection.get(collectionName, docId).create({id: docId}, function(err) { + if (err) { + return done(err); + } + expectReceiveError(connection, collectionName, docId, 'json0 op must be an array', done); + connection.send({ + a: 'op', + c: collectionName, + d: docId, + v: 1, + seq: connection.seq++, + x: {}, + op: {0: {p: ['colour'], oi: 'red'}, length: 1} + }); + }); + }); }); describe('toSnapshot', function() { diff --git a/test/ot.js b/test/ot.js index 0b524613d..e4e6774a4 100644 --- a/test/ot.js +++ b/test/ot.js @@ -125,6 +125,106 @@ 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('checkOpForType', function() { + it('rejects an array-like json0 op', function() { + var op = {op: {0: {p: ['colour'], oi: 'red'}, length: 1}}; + var error = ot.checkOpForType(type.uri, op); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_BADLY_FORMED); + }); + + it('rejects a json0 op that is not an array', function() { + var error = ot.checkOpForType(type.uri, {op: {p: ['colour'], oi: 'red'}}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_BADLY_FORMED); + }); + + it('rejects a json0 op component that is not an object', function() { + var error = ot.checkOpForType(type.uri, {op: [null]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Missing path'); + }); + + it('rejects a dangerous path segment', function() { + var error = ot.checkOpForType(type.uri, {op: [{p: ['__proto__', 'x'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + }); + + it('accepts a valid json0 op', function() { + expect(ot.checkOpForType(type.uri, {op: [{p: ['colour'], oi: 'red'}]})).equal(); + }); + + it('leaves ops for other types alone', function() { + expect(ot.checkOpForType(presenceType.uri, {op: {index: 0, value: 'hi'}})).equal(); + }); + + it('leaves creates and deletes alone', function() { + expect(ot.checkOpForType(type.uri, {create: {type: type.uri}})).equal(); + expect(ot.checkOpForType(type.uri, {del: true})).equal(); + }); + }); + describe('no-op', function() { it('works on existing docs', function() { var doc = {v: 6, type: type.uri, data: 'Hi'}; @@ -374,6 +474,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); + }); + }); + }); });