From cfdb13cbb6c8dc91e70ae06a948f7ed47d300586 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:25:25 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F=20Check=20ops=20from=20th?= =?UTF-8?q?e=20server=20against=20the=20json0=20path=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes https://github.com/share/sharedb/issues/721 `ot.js` refuses to apply a json0 op whose path contains a segment inherited from `Object.prototype`. `Doc` has no equivalent check: `_otApply()` hands op data straight to `this.type.apply()`, so the guard protects the server's own `Object.prototype`, and nothing protects a client's. That matters for documents whose history predates the guard, since a committed `__proto__` op is replayed by every client that fetches the document, and for a hostile or compromised server, or anything else that can put a frame on the socket. The write is silent: `doc.data` is unchanged, no error is emitted, and nothing in the client notices. The type won't complain, so the check has to be explicit. This change adds it to `_otApply()`, which covers remote ops, fixup ops echoed back by the server, and the inverted op on rollback. Every call site already wraps `_otApply()` in a try/catch and hard rollbacks, so throwing fits the existing contract, and the error surfaces the way it does for any other op we can't apply. Locally submitted ops are checked in `_submit()` rather than left to `_otApply()`, because `_pushOp()` runs first, and `_tryCompose()` applies the op to a pending create on the way past. By the time `_otApply()` sees the op, the prototype is already polluted, permanently. Erroring out of `_submit()` also means we call back with the error rather than tearing the document down and refetching it, matching what we already do for an op submitted to an uncreated document. The check reuses the traversal added for GHSA-9rqw-j2q5-gg2g, which walks an op the way `ot-json0` does, so the array-like and non-string path segment bypasses are closed on the client too. It deliberately reuses only the path check, not the shape check: `ot-json0` quietly treats a non-array as a no-op and older versions of ShareDB committed those, so they exist in real op histories, and the client is on the replay side of that line. Note this doesn't clean up documents that already have such an op in their history. Those ops stay in the database and keep failing on every replay. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/client/doc.js | 12 +++++ lib/ot.js | 9 ++++ test/client/doc.js | 117 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/lib/client/doc.js b/lib/client/doc.js index d3713492..8205915c 100644 --- a/lib/client/doc.js +++ b/lib/client/doc.js @@ -1,5 +1,6 @@ var emitter = require('../emitter'); var logger = require('../logger'); +var ot = require('../ot'); var ShareDBError = require('../error'); var types = require('../types'); var util = require('../util'); @@ -599,6 +600,9 @@ Doc.prototype._otApply = function(op, source) { ); } + var pathError = ot.checkOpPathsForType(this.type.name, op); + if (pathError) throw pathError; + // NB: If we need to add another argument to this event, we should consider // the fact that the 'op' event has op.src as its 3rd argument this.emit('before op batch', op.op, source); @@ -765,6 +769,14 @@ Doc.prototype._submit = function(op, source, callback) { } // Try to normalize the op. This removes trailing skip:0's and things like that. if (this.type.normalize) op.op = this.type.normalize(op.op); + + // This has to happen before _pushOp(), because _tryCompose() applies the op + // to a pending create, well before _otApply() gets a chance to check it + var pathError = ot.checkOpPathsForType(this.type.name, op); + if (pathError) { + if (callback) return callback(pathError); + return this.emit('error', pathError); + } } try { diff --git a/lib/ot.js b/lib/ot.js index 4618598b..184117d2 100644 --- a/lib/ot.js +++ b/lib/ot.js @@ -165,6 +165,15 @@ exports.checkOpForType = function(typeName, op) { return checkJson0Op(op.op); }; +// Ops that are only being applied, rather than submitted, get their paths +// checked but not their shape +exports.checkOpPathsForType = function(typeName, op) { + if (!('op' in op)) return; + var type = types.map[typeName]; + if (!type || type.name !== 'json0') return; + return checkJson0OpPaths(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 diff --git a/test/client/doc.js b/test/client/doc.js index 55833ca2..c4ab7b86 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -829,6 +829,123 @@ describe('Doc', function() { }); }); }); + + function expectInvalidPathSegment(error) { + expect(error.code).to.equal(ShareDBError.CODES.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + } + + describe('ops from the server', function() { + var multiComponentOp = [{p: ['baz'], oi: true}, {p: ['__proto__', 'polluted'], oi: 'oops'}]; + var connection; + var doc; + + beforeEach(function(done) { + connection = this.connection; + doc = connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}, done); + }); + + function sendOp(op) { + connection.handleMessage({ + a: 'op', + c: doc.collection, + d: doc.id, + v: 1, + src: 'hostile', + seq: 1, + op: op + }); + } + + [ + { + name: 'ops with a dangerous first path segment', + op: [{p: ['__proto__', 'polluted'], oi: 'oops'}] + }, + { + name: 'ops with a dangerous later path segment', + op: [{p: ['foo', 'constructor'], oi: 'oops'}] + }, + { + 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'}] + }, + { + name: 'multi-component ops', + op: multiComponentOp + } + ].forEach(function(test) { + it('Rejects ' + test.name, function(done) { + doc.on('error', function(error) { + expectInvalidPathSegment(error); + done(); + }); + sendOp(test.op); + }); + }); + + it('does not apply the valid components of a rejected multi-component op', function(done) { + doc.on('error', function() { + expect(doc.data).to.not.have.property('baz'); + done(); + }); + sendOp(multiComponentOp); + }); + }); + + describe('locally submitted ops', function() { + var badOp = [{p: ['__proto__', 'polluted'], oi: 'oops'}]; + + it('rejects an op composed into a pending create', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}); + doc.submitOp(badOp, function(error) { + expectInvalidPathSegment(error); + done(); + }); + }); + + it('rejects an op without sending it to the server', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}, function(error) { + if (error) return done(error); + var calledBack = false; + doc.submitOp(badOp, function(error) { + calledBack = true; + expectInvalidPathSegment(error); + }); + // The server would only reject asynchronously, so calling back + // synchronously is how we know the op never left the client + expect(calledBack).to.equal(true); + done(); + }); + }); + + it('leaves the doc usable after rejecting an op', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + async.series([ + doc.create.bind(doc, {foo: 'bar'}), + function(next) { + doc.submitOp(badOp, function(error) { + expect(error).to.be.instanceOf(Error); + next(); + }); + }, + doc.submitOp.bind(doc, [{p: ['baz'], oi: true}]), + doc.whenNothingPending.bind(doc), + function(next) { + expect(doc.data).to.eql({foo: 'bar', baz: true}); + next(); + } + ], done); + }); + }); }); describe('toSnapshot', function() {