💥 Reject json0 ops that ot-json0 can only apply

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

lib/ot.js

@@ -120,32 +120,69 @@ 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.
 // See GHSA-9rqw-j2q5-gg2g
 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

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

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