diff --git a/lib/ot.js b/lib/ot.js index c142904f..4618598b 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 d2cac8d9..afcff7ac 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 e0af1256..8ab84c66 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 f30258c0..332dfc29 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 6d093001..e4e6774a 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'};