Merge pull request #87 from dinoboff/fix-null-value-handling

Fix handling of arguments with wrong type in snapshot and string methods.
goldibex · Nov 15, 2016 · feea9cf · feea9cf
2 parents b81e30d + fb37ac5
commit feea9cf
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 5 deletions.
diff --git a/lib/database.js b/lib/database.js
@@ -74,7 +74,7 @@ class Database {
    * @return {RuleDataSnapshot}
    */
   snapshot(path) {
-    return new RuleDataSnapshot(this.root, path);
+    return new RuleDataSnapshot(this.root, path || '/');
   }
 
   /**
@@ -178,7 +178,8 @@ class RuleDataSnapshot {
    * @param  {string}   path Path to the data location
    */
   constructor(root, path) {
-    this[rootKey] = root;
+    // Snapshot with invalid path should reference to null.
+    this[rootKey] = path == null ? store.create(null) : root;
     this[pathKey] = paths.trim(path);
   }
 
@@ -228,7 +229,11 @@ class RuleDataSnapshot {
    * @return {RuleDataSnapshot}
    */
   child(childPath) {
-    const newPath = paths.join(this[pathKey], childPath);
+    if (typeof childPath !== 'string') {
+      throw new Error(`${childPath} should be a string.`);
+    }
+
+    const newPath = childPath ? paths.join(this[pathKey], childPath) : null;
 
     return new RuleDataSnapshot(this[rootKey], newPath);
   }
@@ -255,7 +260,11 @@ class RuleDataSnapshot {
    * @return {boolean}
    */
   hasChild(path) {
-    return this.child(path).exists();
+    if (typeof path !== 'string') {
+      throw new Error(`${path} should be a string.`);
+    }
+
+    return Boolean(path) && this.child(path).exists();
   }
 
   /**

diff --git a/lib/parser/string-methods.js b/lib/parser/string-methods.js
@@ -6,17 +6,37 @@ const replaceall = require('replaceall');
 module.exports = {
 
   contains(str, substr) {
+    if (typeof substr !== 'string') {
+      throw new Error(`${substr} is not a string.`);
+    }
+
     return str.indexOf(substr) !== -1;
   },
+
   beginsWith(str, substr) {
+    if (typeof substr !== 'string') {
+      throw new Error(`${substr} is not a string.`);
+    }
+
     return str.indexOf(substr) === 0;
   },
+
   endsWith(str, substr) {
+    if (typeof substr !== 'string') {
+      throw new Error(`${substr} is not a string.`);
+    }
+
     return str.indexOf(substr) + substr.length === str.length;
   },
+
   replace(str, substr, replacement) {
+    if (typeof substr !== 'string' || typeof replacement !== 'string') {
+      throw new Error(`${substr} is not a string.`);
+    }
+
     return replaceall(substr, replacement, str);
   },
+
   matches(str, regex) {
     return regex.test(str);
   }

diff --git a/test/spec/lib/parser/rule.js b/test/spec/lib/parser/rule.js
@@ -136,6 +136,91 @@ const ruleEvaluationTests = [{
   wildchildren: [],
   scope: {auth: {}},
   result: true
+}, {
+  rule: 'root.child("foo").child(auth.foo).val() != null',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").child(auth.foo).val() == null',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").child(auth.foo).exists()',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").child(auth.foo).exists() == false',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").hasChild(auth.foo)',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").hasChild(auth.foo) == false',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").hasChildren([auth.foo])',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: 'root.child("foo").hasChildren([auth.foo]) == false',
+  wildchildren: [],
+  scope: {auth: null, root: database.snapshot('/', {foo: {bar: true}})},
+  willThrow: true
+}, {
+  rule: '"foo".contains(auth.foo)',
+  wildchildren: [],
+  scope: {auth: null},
+  willThrow: true
+}, {
+  rule: '"foo1".contains(auth.foo)',
+  wildchildren: [],
+  scope: {auth: {foo: 1}},
+  willThrow: true
+}, {
+  rule: '"foo".beginsWith(auth.foo)',
+  wildchildren: [],
+  scope: {auth: null},
+  willThrow: true
+}, {
+  rule: '"1foo".beginsWith(auth.foo)',
+  wildchildren: [],
+  scope: {auth: {foo: 1}},
+  willThrow: true
+}, {
+  rule: '"foo".endsWith(auth.foo)',
+  wildchildren: [],
+  scope: {auth: null},
+  willThrow: true
+}, {
+  rule: '"foo1".endsWith(auth.foo)',
+  wildchildren: [],
+  scope: {auth: {foo: 1}},
+  willThrow: true
+}, {
+  rule: '"foo".replace(auth.foo, "bar") == "foo"',
+  wildchildren: [],
+  scope: {auth: null},
+  willThrow: true
+}, {
+  rule: '"foo1".replace(auth.foo, "bar") == "foobar"',
+  wildchildren: [],
+  scope: {auth: {foo: 1}},
+  willThrow: true
+}, {
+  rule: '"foobar".replace("bar", auth.foo) == "foo1"',
+  wildchildren: [],
+  scope: {auth: {foo: 1}},
+  willThrow: true
 }];
 
 describe('Rule', function() {
@@ -180,7 +265,7 @@ describe('Rule', function() {
             const rule = new Rule(ruleTest.rule, ruleTest.wildchildren);
 
             rule.evaluate(ruleTest.scope, ruleTest.skipOnNoValue);
-          }).to.throw();
+          }, ruleTest.rule).to.throw();
 
         } else {