From bb19829e18765e1c6123e15c4290dbbc9085df7f 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=92=A5=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) -> reading 'p' of undefined 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 takes down any server that fixes ops up, needing nothing more than a single socket frame: ``` {"a":"op","c":"docs","d":"doc","v":1,"src":"c1","seq":1, "op":{"p":["colour"],"oi":"red"}} ``` No ShareDB client sends that, since `Doc._submit()` runs `type.normalize()` first, but nothing stops a hand-written one. The disagreement is really `ot-json0`'s, and it's now recorded upstream in https://github.com/ottypes/json0/issues/52. Upstream isn't going to fix it for us, though. `apply()` was already tightened in ottypes/json0#40, but that's still unreleased: the plan settled on in ottypes/json0#42 was a breaking `json0` v2 with the stricter checks on by default, keeping the type URI, and leaving ShareDB to shim historic ops — which we did, in #494. That release hasn't happened. Even when it does, it won't help here. `$fixup()` is called from the `apply` middleware, which `submit-request` triggers *before* it calls `ot.apply()`, so a badly formed op reaches `compose()` while a stricter `apply()` is still waiting its turn. Nor can we just catch the throw: middleware calls `$fixup()` synchronously, so catching would mean wrapping every middleware call, swallowing everyone's errors to fix one type's, and it still wouldn't stop the no-op commit. So this change makes the strict/lenient split on our side, which is where the two populations of ops can be told apart anyway. Newly submitted ops are checked in `submit-request`: 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()`. `checkSubmittedOpForType()` sits alongside the `checkOpPathsForType()` added for GHSA-9rqw-j2q5-gg2g, and shares its traversal, so the two can't drift apart on what counts as an op component. It's named for the ops it's valid on: it demands a real array, which is only fair on an op that reaches the type exactly as it arrived. `_otApply()` keeps checking only paths, for the same reason `applyOps()` does: it replays committed ops, and a document whose history contains one of these has to stay readable. `Doc` needs no equivalent shape check, because `normalize()` already turns everything it accepts into a real op before the paths are checked. It does need the one thing it was missing: that call sat outside the `try`/`catch`, so a type that throws while normalizing threw straight out of `submitOp()` instead of calling back — `doc.submitOp([null])` being the json0 case. Wrapping it costs nothing and isn't json0 specific. What the catch does with what it caught matters too, since a type can throw anything at all. `.message` is only there to read if the thrown value is an object, and `null.message` would throw straight back out of the catch we just added, so a non-object falls back to the value itself. A `ShareDBError` is passed through as it is: it already carries a code, and giving the error a code is the only reason to wrap it. 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 otherwise unaffected: everything `doc.submitOp()` used to accept, it still accepts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/client/doc.js | 15 ++- lib/ot.js | 53 ++++++++-- lib/submit-request.js | 6 ++ test/backend.js | 6 +- test/client/doc.js | 241 +++++++++++++++++++++++++++++++++++++----- test/ot.js | 38 +++++++ 6 files changed, 321 insertions(+), 38 deletions(-) diff --git a/lib/client/doc.js b/lib/client/doc.js index 1f36a8257..d8a0f3b07 100644 --- a/lib/client/doc.js +++ b/lib/client/doc.js @@ -768,7 +768,20 @@ Doc.prototype._submit = function(op, source, callback) { return this.emit('error', err); } // 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); + if (this.type.normalize) { + try { + op.op = this.type.normalize(op.op); + } catch (error) { + // Otherwise a type that throws on a badly formed op throws out of + // submitOp() rather than calling back + var normalizeError = error instanceof ShareDBError ? error : new ShareDBError( + ERROR_CODE.ERR_OT_OP_BADLY_FORMED, + (error && error.message) || String(error) + ); + if (callback) return callback(normalizeError); + return this.emit('error', normalizeError); + } + } // This has to happen before _pushOp(), because _tryCompose() applies the op // to a pending create, well before _otApply() gets a chance to check it diff --git a/lib/ot.js b/lib/ot.js index 7537b12b9..507aa2848 100644 --- a/lib/ot.js +++ b/lib/ot.js @@ -120,6 +120,18 @@ function applyOpEdit(snapshot, edit) { } } +function isJson0OpComponent(component) { + return !!component && typeof component === 'object' && Array.isArray(component.p); +} + +function checkJson0ComponentPath(component) { + for (var i = 0; i < component.p.length; i++) { + if (util.isDangerousProperty(component.p[i])) { + return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Invalid path segment'); + } + } +} + // 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. @@ -127,25 +139,50 @@ function applyOpEdit(snapshot, edit) { 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'); - } + if (!isJson0OpComponent(op[i])) return; + var pathError = checkJson0ComponentPath(op[i]); + if (pathError) return pathError; + } +} + +// Only for ops arriving at submit-request, which reach ot-json0 exactly as +// they came off the wire, with nothing to normalize them on the way. Ops that +// are already committed can only have their paths checked, since ops that +// ot-json0 quietly treats as no-ops were committable by older versions of +// ShareDB +function checkSubmittedJson0Op(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++) { + if (!isJson0OpComponent(op[i])) { + return new ShareDBError(ERROR_CODE.ERR_OT_OP_BADLY_FORMED, 'Missing path'); } + var pathError = checkJson0ComponentPath(op[i]); + if (pathError) return pathError; } } +function isJson0(type) { + if (typeof type === 'string') type = types.map[type]; + return !!type && type.name === 'json0'; +} + exports.checkOpPathsForType = function(type, op) { if (!('op' in op)) return; - if (typeof type === 'string') type = types.map[type]; - if (!type || type.name !== 'json0') return; + if (!isJson0(type)) return; return checkJson0OpPaths(op.op); }; +exports.checkSubmittedOpForType = function(type, op) { + if (!('op' in op)) return; + if (!isJson0(type)) return; + return checkSubmittedJson0Op(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..c4f2cd472 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.checkSubmittedOpForType(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 ad025dfd3..0d1added0 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -657,31 +657,31 @@ describe('Doc', function() { }); }); - describe('errors on ops that could cause prototype corruption', function() { - function expectReceiveError( - connection, - collectionName, - docId, - expectedError, - done - ) { - connection.on('receive', function(request) { - var message = request.data; - if (message.c === collectionName && message.d === docId) { - if ('error' in message) { - request.data = null; // Stop further processing of the message - if (message.error.message === expectedError) { - return done(); - } else { - return done('Unexpected ShareDB error: ' + message.error.message); - } + function expectReceiveError( + connection, + collectionName, + docId, + expectedError, + done + ) { + connection.on('receive', function(request) { + var message = request.data; + if (message.c === collectionName && message.d === docId) { + if ('error' in message) { + request.data = null; // Stop further processing of the message + if (message.error.message === expectedError) { + return done(); } else { - return done('Expected error on ' + collectionName + '.' + docId + ' but got no error'); + return done('Unexpected ShareDB error: ' + message.error.message); } + } else { + return done('Expected error on ' + collectionName + '.' + docId + ' but got no error'); } - }); - } + } + }); + } + describe('errors on ops that could cause prototype corruption', function() { afterEach(function() { delete Object.prototype.polluted; }); @@ -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); } @@ -911,6 +916,31 @@ describe('Doc', function() { }); }); + // normalize() runs before the paths are checked, and it takes a bare + // component as well as an op, fills in a missing path, and branches on + // Array.isArray() — so these all reach the check as a real op + [ + {name: 'a bare op component', op: {p: ['__proto__', 'polluted'], oi: 'oops'}}, + {name: 'an op that is also shaped like a component', op: (function() { + var op = [{p: ['__proto__', 'polluted'], oi: 'oops'}]; + op.p = []; + return op; + })()}, + {name: 'an op with a pathless component before a bad one', op: [ + {p: [], od: {foo: 'bar'}, oi: {foo: 'bar'}}, + {p: ['__proto__', 'polluted'], oi: 'oops'} + ]} + ].forEach(function(test) { + it('rejects ' + test.name + ' composed into a pending create', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}); + doc.submitOp(test.op, function(error) { + expectInvalidPathSegment(error); + done(); + }); + }); + }); + it('leaves the doc usable after rejecting an op', function(done) { var doc = this.connection.get('test-collection', 'test-doc'); async.series([ @@ -932,6 +962,165 @@ 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 + describe('errors on badly formed json0 ops', function() { + [ + { + 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('locally submitted ops', function() { + // json0's normalize() throws on a component that isn't an object, and it + // runs before the op is checked, so this used to come straight out of + // submitOp() rather than through the callback + it('rejects an op component that is not an object 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([null], function(error) { + calledBack = true; + expect(error.code).to.equal(ShareDBError.CODES.ERR_OT_OP_BADLY_FORMED); + }); + // 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('emits an error for an op component that is not an object with no callback', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}, function(error) { + if (error) return done(error); + doc.on('error', function(error) { + expect(error.code).to.equal(ShareDBError.CODES.ERR_OT_OP_BADLY_FORMED); + done(); + }); + doc.submitOp([null]); + }); + }); + + // A type can throw anything at all, and reading .message off a thrown + // null would throw out of the catch, which is what the catch is for + [ + { + name: 'a string', + thrown: 'not an op', + code: ShareDBError.CODES.ERR_OT_OP_BADLY_FORMED, + message: 'not an op' + }, + { + name: 'null', + thrown: null, + code: ShareDBError.CODES.ERR_OT_OP_BADLY_FORMED, + message: 'null' + }, + { + name: 'a ShareDBError', + thrown: new ShareDBError('ERR_CUSTOM_TYPE_ERROR', 'Custom type error'), + code: 'ERR_CUSTOM_TYPE_ERROR', + message: 'Custom type error' + } + ].forEach(function(test) { + it('reports ' + test.name + ' thrown by normalize()', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + doc.create({foo: 'bar'}, function(error) { + if (error) return done(error); + sinon.stub(json0, 'normalize').callsFake(function() { + throw test.thrown; + }); + doc.submitOp([{p: ['foo'], od: 'bar'}], function(error) { + expect(error.code).to.equal(test.code); + expect(error.message).to.equal(test.message); + done(); + }); + }); + }); + }); + + // normalize() accepts more than the server does, and everything it + // accepts has to stay submittable + [ + {name: 'a bare op component', op: {p: ['baz'], oi: true}}, + {name: 'an op whose component has no path', op: [{od: {foo: 'bar'}, oi: {foo: 'bar', baz: true}}]}, + {name: 'a bare component with no path', op: {od: {foo: 'bar'}, oi: {foo: 'bar', baz: true}}} + ].forEach(function(test) { + it('accepts ' + test.name + ', which json0 normalizes into an op', function(done) { + var doc = this.connection.get('test-collection', 'test-doc'); + async.series([ + doc.create.bind(doc, {foo: 'bar'}), + doc.submitOp.bind(doc, test.op), + doc.whenNothingPending.bind(doc), + function(next) { + expect(doc.data).to.eql({foo: 'bar', baz: true}); + next(); + } + ], 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([null], 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() { var doc; beforeEach(function(done) { diff --git a/test/ot.js b/test/ot.js index 6d093001d..a823da39e 100644 --- a/test/ot.js +++ b/test/ot.js @@ -187,6 +187,44 @@ describe('ot', function() { }); }); + describe('checkSubmittedOpForType', function() { + it('rejects an array-like json0 op', function() { + var op = {op: {0: {p: ['colour'], oi: 'red'}, length: 1}}; + var error = ot.checkSubmittedOpForType(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.checkSubmittedOpForType(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.checkSubmittedOpForType(type.uri, {op: [null]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_BADLY_FORMED); + expect(error.message).to.equal('Missing path'); + }); + + it('rejects a dangerous path segment', function() { + var error = ot.checkSubmittedOpForType(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.checkSubmittedOpForType(type.uri, {op: [{p: ['colour'], oi: 'red'}]})).equal(); + }); + + it('leaves ops for other types alone', function() { + expect(ot.checkSubmittedOpForType(presenceType.uri, {op: {index: 0, value: 'hi'}})).equal(); + }); + + it('leaves creates and deletes alone', function() { + expect(ot.checkSubmittedOpForType(type.uri, {create: {type: type.uri}})).equal(); + expect(ot.checkSubmittedOpForType(type.uri, {del: true})).equal(); + }); + }); + describe('no-op', function() { it('works on existing docs', function() { var doc = {v: 6, type: type.uri, data: 'Hi'};