Merge pull request #61 from mozilla-services/validate-uuids

Fixes #28 - Validate uuids

src/collection.js

@@ -3,7 +3,7 @@
 import { v4 as uuid4 } from "uuid";
 import deepEquals from "deep-eql";
 
-import { attachFakeIDBSymbolsTo, reduceRecords } from "./utils";
+import { attachFakeIDBSymbolsTo, reduceRecords, isUUID4 } from "./utils";
 import { cleanRecord } from "./api";
 
 attachFakeIDBSymbolsTo(typeof global === "object" ? global : window);
@@ -180,6 +180,8 @@ export default class Collection {
           id:      options.synced || options.forceUUID ? record.id : uuid4(),
           _status: options.synced ? "synced" : "created"
         });
+        if (!isUUID4(newRecord.id))
+          reject(new Error(`Invalid UUID: ${newRecord.id}`));
         store.add(newRecord);
         transaction.onerror = event => reject(new Error(event.target.error));
         transaction.oncomplete = event => {
@@ -208,6 +210,8 @@ export default class Collection {
         return Promise.reject(new Error("Record is not an object."));
       if (!record.id)
         return Promise.reject(new Error("Cannot update a record missing id."));
+      if (!isUUID4(record.id))
+        return Promise.reject(new Error(`Invalid UUID: ${record.id}`));
       return this.get(record.id).then(_ => {
         return new Promise((resolve, reject) => {
           var newStatus = "updated";
@@ -247,14 +251,16 @@ export default class Collection {
   }
 
   /**
-   * Retrieve a record by its id from the local database.
+   * Retrieve a record by its uuid from the local database.
    *
    * @param  {String} id
    * @param  {Object} options
    * @return {Promise}
    */
   get(id, options={includeDeleted: false}) {
     return this.open().then(() => {
+      if (!isUUID4(id))
+        throw new Error(`Invalid UUID: ${id}`);
       return new Promise((resolve, reject) => {
         const {transaction, store} = this.prepare();
         const request = store.get(id);
@@ -281,12 +287,14 @@ export default class Collection {
    * - {Boolean} virtual: When set to true, doesn't actually delete the record,
    *                      update its _status attribute to "deleted" instead.
    *
-   * @param  {String} id
-   * @param  {Object} options
+   * @param  {String} id       The record's UUID.
+   * @param  {Object} options  The options object.
    * @return {Promise}
    */
   delete(id, options={virtual: true}) {
     return this.open().then(() => {
+      if (!isUUID4(id))
+        throw new Error(`Invalid UUID: ${id}`);
       // Ensure the record actually exists.
       return this.get(id, {includeDeleted: true}).then(res => {
         if (options.virtual) {

src/utils.js

@@ -1,5 +1,7 @@
 "use strict";
 
+const RE_UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
+
 /**
  * In FakeIndexedDB, symbols are exposed using ``FDB`` prefixes in names.
  * This piece of code will register them with the same names as native API,
@@ -115,3 +117,13 @@ export function partition(array, n) {
     return acc;
   }, []);
 }
+
+/**
+ * Checks if a string is an UUID, according to RFC4122.
+ *
+ * @param  {String} uuid The uuid to validate.
+ * @return {Boolean}
+ */
+export function isUUID4(uuid) {
+  return RE_UUID.test(uuid);
+}

test/collection_test.js

@@ -185,13 +185,19 @@ describe("Collection", () => {
     });
 
     it("should support the forceUUID option", () => {
-      return articles.create({id: 42, title: "foo"}, {forceUUID: true})
+      const testUUID = uuid4();
+      return articles.create({id: testUUID, title: "foo"}, {forceUUID: true})
         .then(result => articles.get(result.data.id))
         .then(res => res.data.id)
-        .should.become(42);
+        .should.become(testUUID);
+    });
+
+    it("should validate record's UUID when provided", () => {
+      return articles.create({id: 42, title: "foo"}, {forceUUID: true})
+        .should.be.rejectedWith(Error, /UUID/);
     });
 
-    it("should reject on transaction error", function() {
+    it("should reject on transaction error", () => {
       sandbox.stub(articles, "prepare").returns({
         store: {add() {}},
         transaction: {
@@ -238,7 +244,7 @@ describe("Collection", () => {
     });
 
     it("should reject updates on a non-existent record", () => {
-      return articles.update({id: "non-existent"})
+      return articles.update({id: uuid4()})
         .should.be.rejectedWith(Error, /not found/);
     });
 
@@ -252,7 +258,12 @@ describe("Collection", () => {
         .should.be.rejectedWith(Error, /missing id/);
     });
 
-    it("should reject on transaction error", function() {
+    it("should validate record's UUID when provided", () => {
+      return articles.update({id: 42})
+        .should.be.rejectedWith(Error, /UUID/);
+    });
+
+    it("should reject on transaction error", () => {
       sandbox.stub(articles, "get").returns(Promise.resolve());
       sandbox.stub(articles, "prepare").returns({
         store: {get() {}, put() {}},
@@ -263,7 +274,7 @@ describe("Collection", () => {
           }
         }
       });
-      return articles.update({id: 1, foo: "bar"})
+      return articles.update({id: uuid4(), foo: "bar"})
         .should.be.rejectedWith(Error, "transaction error");
     });
 
@@ -326,14 +337,19 @@ describe("Collection", () => {
         .should.eventually.eql(article.title);
     });
 
+    it("should validate passed id", () => {
+      return articles.get(42)
+        .should.be.rejectedWith(Error, /UUID/);
+    });
+
     it("should have record status info attached", () => {
       return articles.get(uuid)
         .then(res => res.data._status)
         .should.eventually.eql("created");
     });
 
     it("should reject in case of record not found", () => {
-      return articles.get("non-existent")
+      return articles.get(uuid4())
         .then(res => res.data)
         .should.be.rejectedWith(Error, /not found/);
     });
@@ -354,7 +370,7 @@ describe("Collection", () => {
       // body...
     });
 
-    it("should reject on transaction error", function() {
+    it("should reject on transaction error", () => {
       sandbox.stub(articles, "prepare").returns({
         store: {get() {}},
         transaction: {
@@ -364,7 +380,7 @@ describe("Collection", () => {
           }
         }
       });
-      return articles.get(1)
+      return articles.get(uuid4())
         .should.be.rejectedWith(Error, "transaction error");
     });
 
@@ -383,6 +399,11 @@ describe("Collection", () => {
         .then(result => uuid = result.data.id);
     });
 
+    it("should validate passed id", () => {
+      return articles.delete(42)
+        .should.be.rejectedWith(Error, /UUID/);
+    });
+
     describe("Virtual", () => {
       it("should virtually delete a record", () => {
         return articles.delete(uuid, {virtual: true})
@@ -399,7 +420,7 @@ describe("Collection", () => {
       });
 
       it("should reject on non-existent record", () => {
-        return articles.delete("non-existent", {virtual: true})
+        return articles.delete(uuid4(), {virtual: true})
           .then(res => res.data)
           .should.eventually.be.rejectedWith(Error, /not found/);
       });
@@ -424,13 +445,13 @@ describe("Collection", () => {
       });
 
       it("should reject on non-existent record", () => {
-        return articles.delete("non-existent", {virtual: false})
+        return articles.delete(uuid4(), {virtual: false})
           .then(res => res.data)
           .should.eventually.be.rejectedWith(Error, /not found/);
       });
     });
 
-    it("should reject on transaction error", function() {
+    it("should reject on transaction error", () => {
       sandbox.stub(articles, "get").returns(Promise.resolve());
       sandbox.stub(articles, "prepare").returns({
         store: {delete() {}},
@@ -441,7 +462,7 @@ describe("Collection", () => {
           }
         }
       });
-      return articles.delete(1, {virtual: false})
+      return articles.delete(uuid4(), {virtual: false})
         .should.be.rejectedWith(Error, "transaction error");
     });
   });
@@ -587,14 +608,14 @@ describe("Collection", () => {
       });
     });
 
-    describe("Error handling", function() {
+    describe("Error handling", () => {
       var articles;
 
       beforeEach(() => {
         articles = testCollection();
       });
 
-      it("should reject on transaction error", function() {
+      it("should reject on transaction error", () => {
         sandbox.stub(articles, "prepare").returns({
           store: {openCursor() {return {}}},
           transaction: {
@@ -619,17 +640,24 @@ describe("Collection", () => {
     });
 
     describe("When no conflicts occured", () => {
+      const uuid_1 = uuid4();
+      const uuid_2 = uuid4();
+      const uuid_3 = uuid4();
+      const uuid_4 = uuid4();
+      const uuid_5 = uuid4();
+      const uuid_6 = uuid4();
+
       const localData = [
-        {id: 1, title: "art1"},
-        {id: 2, title: "art2"},
-        {id: 4, title: "art4"},
-        {id: 5, title: "art5"},
+        {id: uuid_1, title: "art1"},
+        {id: uuid_2, title: "art2"},
+        {id: uuid_4, title: "art4"},
+        {id: uuid_5, title: "art5"},
       ];
       const serverChanges = [
-        {id: 2, title: "art2"}, // existing, should simply be marked as synced
-        {id: 3, title: "art3"}, // to be created
-        {id: 4, deleted: true}, // to be deleted
-        {id: 6, deleted: true}, // remotely deleted, missing locally
+        {id: uuid_2, title: "art2"}, // existing, should simply be marked as synced
+        {id: uuid_3, title: "art3"}, // to be created
+        {id: uuid_4, deleted: true}, // to be deleted
+        {id: uuid_6, deleted: true}, // remotely deleted, missing locally
       ];
 
       beforeEach(() => {
@@ -669,23 +697,23 @@ describe("Collection", () => {
         return articles.pullChanges(result)
           .then(res => res.created)
           .should.eventually.become([
-            {id: 3, title: "art3", _status: "synced"}
+            {id: uuid_3, title: "art3", _status: "synced"}
           ]);
       });
 
       it("should resolve with imported updates", () => {
         return articles.pullChanges(result)
           .then(res => res.updated)
           .should.eventually.become([
-            {id: 2, title: "art2", _status: "synced"}
+            {id: uuid_2, title: "art2", _status: "synced"}
           ]);
       });
 
       it("should resolve with imported deletions", () => {
         return articles.pullChanges(result)
           .then(res => res.deleted)
           .should.eventually.become([
-            {id: 4}
+            {id: uuid_4}
           ]);
       });
 
@@ -697,13 +725,13 @@ describe("Collection", () => {
 
       it("should actually import changes into the collection", () => {
         return articles.pullChanges(result)
-          .then(_ => articles.list())
+          .then(_ => articles.list({order: "title"}))
           .then(res => res.data)
           .should.eventually.become([
-            {id: 1, title: "art1", _status: "synced"},
-            {id: 2, title: "art2", _status: "synced"},
-            {id: 3, title: "art3", _status: "synced"},
-            {id: 5, title: "art5", _status: "synced"},
+            {id: uuid_1, title: "art1", _status: "synced"},
+            {id: uuid_2, title: "art2", _status: "synced"},
+            {id: uuid_3, title: "art3", _status: "synced"},
+            {id: uuid_5, title: "art5", _status: "synced"},
           ]);
       });
 
@@ -823,7 +851,7 @@ describe("Collection", () => {
       result = new SyncResultObject();
       return Promise.all([
         articles.create({title: "foo"}),
-        articles.create({id: "fake-uuid", title: "bar"}, {synced: true}),
+        articles.create({id: uuid4(), title: "bar"}, {synced: true}),
       ])
         .then(results => records = results.map(res => res.data));
     });

test/utils_test.js

@@ -9,7 +9,8 @@ import {
   sortObjects,
   filterObjects,
   reduceRecords,
-  partition
+  partition,
+  isUUID4
 } from "../src/utils";
 
 chai.should();
@@ -175,17 +176,34 @@ describe("Utils", () => {
     });
   });
 
-  describe("#partition", function() {
-    it("should chunk array", function() {
+  describe("#partition", () => {
+    it("should chunk array", () => {
       expect(partition([1, 2, 3], 2)).eql([[1, 2], [3]]);
       expect(partition([1, 2, 3], 1)).eql([[1], [2], [3]]);
       expect(partition([1, 2, 3, 4, 5], 3)).eql([[1, 2, 3], [4, 5]]);
       expect(partition([1, 2], 2)).eql([[1, 2]]);
     });
 
-    it("should not chunk array with n<=0", function() {
+    it("should not chunk array with n<=0", () => {
       expect(partition([1, 2, 3], 0)).eql([1, 2, 3]);
       expect(partition([1, 2, 3], -1)).eql([1, 2, 3]);
     });
   });
+
+  describe("#isUUID4", () => {
+    it("should check that a string uses a valid UUID format", () => {
+      expect(isUUID4("110ec58a-a0f2-4ac4-8393-c866d813b8d1")).eql(true);
+    });
+
+    it("should check that a string does not use a valid UUID format", () => {
+      expect(isUUID4("110ex58a-a0f2-4ac4-8393-c866d813b8d1")).eql(false);
+      expect(isUUID4("")).eql(false);
+      expect(isUUID4(null)).eql(false);
+      expect(isUUID4(undefined)).eql(false);
+      expect(isUUID4(42)).eql(false);
+      expect(isUUID4({})).eql(false);
+      expect(isUUID4("00000000-0000-5000-a000-000000000000")).eql(false);
+      expect(isUUID4("00000000-0000-4000-e000-000000000000")).eql(false);
+    });
+  });
 });