From 62416698590c665b370318c7266573eb792bf69c Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:28:05 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A5=85=20Reject=20json0=20ops=20that=20`o?= =?UTF-8?q?t-json0`=20can=20only=20apply?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ot-json0`'s `apply()` quietly treats an op that isn't a real array as a no-op: it walks the op with `op.length` and numeric indexing, so a bare component or a plain number iterates zero times and the snapshot comes back untouched. `backend.submit()` has always accepted those, so they commit, bump the version and get published, having changed nothing. Its `compose()` and `invert()` are not so relaxed, though. Both assume a real array, and throw on anything else: ``` compose({p: ['x'], oi: 1}, fixup) -> dest.push is not a function compose({0: {…}, length: 1}, fixup) -> dest.push is not a function compose([null], fixup) -> reading 'p' of null invert({p: ['x'], oi: 1}) -> op.slice is not a function ``` `$fixup()` composes, and it's called from `apply` middleware, where a throw is uncaught. So every one of those shapes is a remote crash on any server that fixes ops up, needing nothing more than a raw socket frame — no ShareDB client sends them, since `Doc._submit()` runs `type.normalize()` first, but nothing stops a hand-written one. This change checks the shape of a submitted op as well as its paths, in `submit-request` rather than in `ot.apply()`. It has to happen before the op reaches any type function, and it can't go in `checkOp()`, which runs before we have the snapshot and so doesn't know the document's type. Ops we've already committed are deliberately left alone, since rejecting a historical no-op would make that document unreadable through `fetchSnapshot()`. This is an API change: a json0 op that isn't an array used to commit as a silent no-op, and is now rejected with `ERR_OT_OP_BADLY_FORMED`. That seems worth it, since the behaviour being removed is a crash for fixup users and a no-op for everyone else — but it is why three of our own tests in `test/backend.js` needed their ops wrapping, which is a fair signal that the shape is easy to write by accident. Clients are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/ot.js | 26 ++++++++++++++++++ lib/submit-request.js | 6 +++++ test/backend.js | 6 ++--- test/client/doc.js | 61 +++++++++++++++++++++++++++++++++++++++---- test/ot.js | 38 +++++++++++++++++++++++++++ 5 files changed, 129 insertions(+), 8 deletions(-) diff --git a/lib/ot.js b/lib/ot.js index c142904f1..4618598bc 100644 --- a/lib/ot.js +++ b/lib/ot.js @@ -139,6 +139,32 @@ function checkJson0OpPaths(op) { } } +// 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 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/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 f30258c00..332dfc29a 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -760,16 +760,21 @@ 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} + 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'}] + 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) { @@ -780,7 +785,7 @@ describe('Doc', function() { if (err) { return done(err); } - expectReceiveError(connection, collectionName, docId, 'Invalid path segment', function(error) { + expectReceiveError(connection, collectionName, docId, test.error, function(error) { if (error) { return done(error); } @@ -799,6 +804,52 @@ describe('Doc', function() { }); }); }); + + // ot-json0's apply() quietly ignores an op that isn't an array, but its + // compose() and invert() throw on one. $fixup() composes, and it is called + // from middleware, so the throw is uncaught and takes the process down + [ + { + name: 'an array-like op', + op: {0: {p: ['colour'], oi: 'red'}, length: 1}, + error: 'json0 op must be an array' + }, + { + name: 'a bare op component', + op: {p: ['colour'], oi: 'red'}, + error: 'json0 op must be an array' + }, + { + name: 'an op component that is not an object', + op: [null], + error: 'Missing path' + } + ].forEach(function(test) { + it('Rejects ' + test.name + ' 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, test.error, 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 6d093001d..e4e6774a4 100644 --- a/test/ot.js +++ b/test/ot.js @@ -187,6 +187,44 @@ describe('ot', function() { }); }); + 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'};