🥅 Reject json0 ops that ot-json0 can only apply

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

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

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
         });

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() {

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'};