From 3e9fef2927a16c7b3baafd4257bc6b6ca510e424 Mon Sep 17 00:00:00 2001
From: Alec Gibson <aleckgibson@gmail.com>
Date: Mon, 16 Jul 2018 16:02:32 +0100
Subject: [PATCH] Throw when array and object deletion values do not match
 current version

This change adds validation to array and object deletion. If the values
provided in either `ld` or `od` do not match the current value, then
`apply` will `throw`. It will also throw if `oi` overwrites an existing
value without providing `od`.

The primary motivation of this change is to ensure that all submitted
ops remain reversible. At the moment, the following series of ops is
possible:

  - start with `{ foo: 'bar' }`
  - `apply` this op: `{ p: ['foo'], oi: 'baz' }`
  - ...resulting in `{ foo: 'baz' }`
  - `invert` the previous op: `{ p: ['foo'], od: 'baz' }`
  - and `apply` the inverted op: `{}`

When I apply, invert and apply, I should always wind up where I started,
but in this case I clearly do not.

By ensuring that the `od` matches the current value, we make sure that
all ops remain reversible.

Deep equality adds a dependency on [`fast-deep-equal`][1].

[1]: https://github.com/epoberezkin/fast-deep-equal
---
 lib/json0.js      | 14 ++++++++++----
 package.json      |  4 +++-
 test/json0.coffee | 21 +++++++++++++++++----
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/lib/json0.js b/lib/json0.js
index dc3a405..91afda0 100644
--- a/lib/json0.js
+++ b/lib/json0.js
@@ -1,3 +1,5 @@
+var deepEqual = require('fast-deep-equal');
+
 /*
  This is the implementation of the JSON OT type.
 
@@ -181,7 +183,8 @@ json.apply = function(snapshot, op) {
     // List replace
     else if (c.li !== void 0 && c.ld !== void 0) {
       json.checkList(elem);
-      // Should check the list element matches c.ld
+      if (!deepEqual(elem[key], c.ld))
+        throw new Error('List deletion value does not match current value')
       elem[key] = c.li;
     }
 
@@ -194,7 +197,8 @@ json.apply = function(snapshot, op) {
     // List delete
     else if (c.ld !== void 0) {
       json.checkList(elem);
-      // Should check the list element matches c.ld here too.
+      if (!deepEqual(elem[key], c.ld))
+        throw new Error('List deletion value does not match current value')
       elem.splice(key,1);
     }
 
@@ -214,7 +218,8 @@ json.apply = function(snapshot, op) {
     else if (c.oi !== void 0) {
       json.checkObj(elem);
 
-      // Should check that elem[key] == c.od
+      if (!deepEqual(elem[key], c.od))
+        throw new Error('Object deletion value does not match current value')
       elem[key] = c.oi;
     }
 
@@ -222,7 +227,8 @@ json.apply = function(snapshot, op) {
     else if (c.od !== void 0) {
       json.checkObj(elem);
 
-      // Should check that elem[key] == c.od
+      if (!deepEqual(elem[key], c.od))
+        throw new Error('Object deletion value does not match current value')
       delete elem[key];
     }
 
diff --git a/package.json b/package.json
index b6c9df6..5dc685b 100644
--- a/package.json
+++ b/package.json
@@ -6,7 +6,9 @@
   "directories": {
     "test": "test"
   },
-  "dependencies": {},
+  "dependencies": {
+    "fast-deep-equal": "^2.0.1"
+  },
   "devDependencies": {
     "ot-fuzzer": "^1.0.0",
     "mocha": "^1.20.1",
diff --git a/test/json0.coffee b/test/json0.coffee
index 531f76e..0f13bd7 100644
--- a/test/json0.coffee
+++ b/test/json0.coffee
@@ -67,10 +67,14 @@ genTests = (type) ->
 
   # Strings should be handled internally by the text type. We'll just do some basic sanity checks here.
   describe 'string', ->
-    describe '#apply()', -> it 'works', ->
-      assert.deepEqual 'abc', type.apply 'a', [{p:[1], si:'bc'}]
-      assert.deepEqual 'bc', type.apply 'abc', [{p:[0], sd:'a'}]
-      assert.deepEqual {x:'abc'}, type.apply {x:'a'}, [{p:['x', 1], si:'bc'}]
+    describe '#apply()', ->
+      it 'works', ->
+        assert.deepEqual 'abc', type.apply 'a', [{p:[1], si:'bc'}]
+        assert.deepEqual 'bc', type.apply 'abc', [{p:[0], sd:'a'}]
+        assert.deepEqual {x:'abc'}, type.apply {x:'a'}, [{p:['x', 1], si:'bc'}]
+
+      it 'throws when the deletion target does not match', ->
+        assert.throws -> type.apply 'abc', [{p:[0], sd:'x'}]
 
     describe '#transform()', ->
       it 'splits deletes', ->
@@ -127,6 +131,10 @@ genTests = (type) ->
         assert.deepEqual ['a', 'b', 'c'], type.apply ['b', 'a', 'c'], [{p:[1], lm:0}]
         assert.deepEqual ['a', 'b', 'c'], type.apply ['b', 'a', 'c'], [{p:[0], lm:1}]
 
+      it 'throws when the deletion target does not match', ->
+        assert.throws -> type.apply ['a', 'b', 'c'], [{p:[0], ld: 'x'}]
+        assert.throws -> type.apply ['a', 'b', 'c'], [{p:[0], li: 'd', ld: 'x'}]
+
       ###
       'null moves compose to nops', ->
         assert.deepEqual [], type.compose [], [{p:[3],lm:3}]
@@ -389,6 +397,11 @@ genTests = (type) ->
       assert.deepEqual [], type.transform [{p:['k'], od:'x'}], [{p:['k'], od:'x'}], 'left'
       assert.deepEqual [], type.transform [{p:['k'], od:'x'}], [{p:['k'], od:'x'}], 'right'
 
+    it 'throws when the deletion target does not match', ->
+      assert.throws -> type.apply {x:'a'}, [{p:['x'], od: 'b'}]
+      assert.throws -> type.apply {x:'a'}, [{p:['x'], oi: 'c', od: 'b'}]
+      assert.throws -> type.apply {x:'a'}, [{p:['x'], oi: 'b'}]
+
   describe 'randomizer', ->
     @timeout 20000
     @slow 6000