Repository: goldibex/targaryen
Pull request #137 (2017-10-23 11:35:28)

fix: regression in rule evaluation of invalid path
Firebase allows RuleDataSnapshot methods to receive invalid path, which PR #132 broke.

This fix allows to traverse DataNode and Ruleset with invalid path and denies read/write operation with invalid path instead, and it keeps denying creating DataNode with invalid keys


Patch:
diff --git a/lib/database/ruleset.js b/lib/database/ruleset.js
index 13a48f3..15d8bad 100644
--- a/lib/database/ruleset.js
+++ b/lib/database/ruleset.js
@@ -120,6 +120,8 @@ class Ruleset {
    */
   tryRead(path, data, now) {
 
+    paths.mustBeValid(path);
+
     const result = results.read(path, data);
     let state = {
       root: data.snapshot('/'),
@@ -336,7 +338,6 @@ class RulesetNode {
    * @return {{child: RulesetNode, wildchildren: object}|void}
    */
   $child(name, wildchildren) {
-    paths.mustBeValid(name);
 
     wildchildren = wildchildren || {};
 
@@ -373,8 +374,6 @@ class RulesetNode {
    */
   $traverse(path, wildchildren, cb) {
 
-    paths.mustBeValid(path);
-
     let currentPath = '';
     let currentRules = this;
 
diff --git a/lib/database/store.js b/lib/database/store.js
index abb06d9..8b3d770 100644
--- a/lib/database/store.js
+++ b/lib/database/store.js
@@ -265,8 +265,6 @@ class DataNode {
    * @return {DataNode}
    */
   $child(path) {
-    paths.mustBeValid(path);
-
     return paths.split(path).reduce(
       (parent, key) => parent[key] || DataNode.null(),
       this


Test patch:
diff --git a/test/spec/lib/database/index.js b/test/spec/lib/database/index.js
index d78b1cc..4680b76 100644
--- a/test/spec/lib/database/index.js
+++ b/test/spec/lib/database/index.js
@@ -546,6 +546,22 @@ describe('database', function() {
       expect(db.read('/a').allowed).to.be.false();
     });
 
+    describe('should fail when reading invalid path', function() {
+
+      ['.', '#', '$', '[', ']'].forEach(char => {
+        const rules = {
+          rules: {
+            '.read': 'true'
+          }
+        };
+        const data = {};
+        const db = database.create(rules, data);
+
+        it(`e.g. using "${char}"`, () => expect(() => db.read(`ab/c${char}d`)).to.throw());
+      });
+
+    });
+
   });
 
   describe('#write', function() {
@@ -895,6 +911,22 @@ describe('database', function() {
       expect(db.write('/a', 2).allowed).to.be.false();
     });
 
+    describe('should fail when writing invalid path', function() {
+
+      ['.', '#', '$', '[', ']'].forEach(char => {
+        const rules = {
+          rules: {
+            '.write': true
+          }
+        };
+        const data = null;
+        const db = database.create(rules, data);
+
+        it(`e.g. using "${char}"`, () => expect(() => db.write(`a/b${char}c`, true)).to.throw());
+      });
+
+    });
+
   });
 
   describe('#update', function() {
@@ -1155,6 +1187,28 @@ describe('database', function() {
       expect(result.info).to.contain('patch was denied.');
     });
 
+    describe('should fail when writing invalid path', function() {
+
+      ['.', '#', '$', '[', ']'].forEach(char => {
+        const rules = {
+          rules: {
+            '.write': true
+          }
+        };
+        const data = null;
+        const db = database.create(rules, data);
+        const patch = {
+          [`a/b${char}c`]: true
+        };
+
+        it(`e.g. using "${char}"`, () => {
+          expect(() => db.update('/', patch)).to.throw();
+          expect(() => db.update(`a/b${char}c`, {d: true})).to.throw();
+        });
+      });
+
+    });
+
   });
 
 });
diff --git a/test/spec/lib/database/ruleset.js b/test/spec/lib/database/ruleset.js
index 3c75453..be84aaa 100644
--- a/test/spec/lib/database/ruleset.js
+++ b/test/spec/lib/database/ruleset.js
@@ -267,15 +267,6 @@ describe('Ruleset', function() {
         expect(child.wildchildren).to.eql({$b: 'foo'});
       });
 
-      describe('should throw when the name includes an invalid character:', function() {
-        const rules = ruleset.create({rules: {'.read': true}});
-
-        ['.', '#', '$', '[', ']'].forEach(char => {
-          it(`e.g. using "${char}"`, () => expect(() => rules.root.$child(`ab/c${char}d`)).to.throw());
-        });
-
-      });
-
     });
 
     describe('#$traverse', function() {
@@ -409,15 +400,6 @@ describe('Ruleset', function() {
         expect(cb).to.have.callCount(2);
       });
 
-      describe('should throw when the path includes an invalid character:', function() {
-        const rules = ruleset.create({rules: {'.read': true}});
-
-        ['.', '#', '$', '[', ']'].forEach(char => {
-          it(`e.g. using "${char}"`, () => expect(() => rules.root.$traverse(`a/b${char}c`, () => {})).to.throw());
-        });
-
-      });
-
     });
 
   });
diff --git a/test/spec/lib/database/store.js b/test/spec/lib/database/store.js
index 1d0a97b..37aab3b 100644
--- a/test/spec/lib/database/store.js
+++ b/test/spec/lib/database/store.js
@@ -383,14 +383,6 @@ describe('store', function() {
       expect(data.$child('foo/bar').$value()).to.equal(null);
     });
 
-    describe('should throw when the path includes an invalid character:', function() {
-
-      ['.', '#', '$', '[', ']'].forEach(char => {
-        it(`e.g. using "${char}"`, () => expect(() => data.$child(`a${char}c`)).to.throw());
-      });
-
-    });
-
   });
 
 });
diff --git a/test/spec/lib/parser/fixtures.json b/test/spec/lib/parser/fixtures.json
index 02d181c..9fbede2 100644
--- a/test/spec/lib/parser/fixtures.json
+++ b/test/spec/lib/parser/fixtures.json
@@ -10,6 +10,9 @@
       "someString": "one",
       "uid": "custom:bob"
     },
+    "uidWithEmail": {
+      "uid": "bob@example.com"
+    },
     "unauth": null
   },
   "tests": [
@@ -1109,6 +1112,48 @@
       "isValid": true,
       "failAtRuntime": false,
       "evaluateTo": true
+    },
+    {
+      "rule": "root.child(\"banned/\" + auth.uid).val() != true",
+      "user": "uidWithEmail",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
+    },
+    {
+      "rule": "root.hasChild(\"banned/\" + auth.uid) == false",
+      "user": "uidWithEmail",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
+    },
+    {
+      "rule": "root.hasChildren([\"banned/\" + auth.uid]) == false",
+      "user": "uidWithEmail",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
+    },
+    {
+      "rule": "root.child(\"banned/bob@example.com\").val() != true",
+      "user": "unauth",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
+    },
+    {
+      "rule": "root.hasChild(\"banned/bob@example.com\") == false",
+      "user": "unauth",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
+    },
+    {
+      "rule": "root.hasChildren([\"banned/bob@example.com\"]) == false",
+      "user": "unauth",
+      "isValid": true,
+      "failAtRuntime": false,
+      "evaluateTo": true
     }
   ]
-}
\ No newline at end of file
+}