fix unauthorized fk change (#1540)

lib/relation-definition.js

@@ -108,6 +108,19 @@ function bindRelationMethods(relation, relationMethod, definition) {
   });
 };
 
+function preventFkOverride(inst, data, fkProp) {
+  if (!fkProp) return undefined;
+  if (data[fkProp] !== undefined && !idEquals(data[fkProp], inst[fkProp])) {
+    var err = new Error(g.f(
+      'Cannot override foreign key %s from %s to %s',
+      fkProp,
+      inst[fkProp],
+      data[fkProp])
+    );
+  }
+  return err;
+}
+
 /**
  * Relation definition class.  Use to define relationships between models.
  * @param {Object} definition
@@ -853,10 +866,15 @@ HasMany.prototype.updateById = function(fkId, data, options, cb) {
     options = {};
   }
   cb = cb || utils.createPromiseCallback();
+  var fk = this.definition.keyTo;
+
   this.findById(fkId, options, function(err, inst) {
     if (err) {
       return cb && cb(err);
     }
+    // Ensure Foreign Key cannot be changed!
+    var fkErr = preventFkOverride(inst, data, fk);
+    if (fkErr) return cb(fkErr);
     inst.updateAttributes(data, options, cb);
   });
   return cb.promise;
@@ -1356,8 +1374,13 @@ BelongsTo.prototype.update = function(targetModelData, options, cb) {
   }
   cb = cb || utils.createPromiseCallback();
   var definition = this.definition;
+  var fk = definition.keyTo;
+
   this.fetch(options, function(err, inst) {
     if (inst instanceof ModelBaseClass) {
+      // Ensures Foreign Key cannot be changed!
+      var fkErr = preventFkOverride(inst, targetModelData, fk);
+      if (fkErr) return cb(fkErr);
       inst.updateAttributes(targetModelData, options, cb);
     } else {
       cb(new Error(g.f('{{BelongsTo}} relation %s is empty', definition.name)));
@@ -1744,7 +1767,9 @@ HasOne.prototype.update = function(targetModelData, options, cb) {
   var fk = this.definition.keyTo;
   this.fetch(function(err, targetModel) {
     if (targetModel instanceof ModelBaseClass) {
-      delete targetModelData[fk];
+      // Ensures Foreign Key cannot be changed!
+      var fkErr = preventFkOverride(targetModel, targetModelData, fk);
+      if (fkErr) return cb(fkErr);
       targetModel.updateAttributes(targetModelData, options, cb);
     } else {
       cb(new Error(g.f('{{HasOne}} relation %s is empty', definition.name)));

test/relations.test.js

@@ -88,6 +88,22 @@ describe('relations', function() {
         });
       });
 
+      it('should not update FK', function(done) {
+        Book.create(function(err, book) {
+          book.chapters.create({name: 'chapter 1'}, function(err, c) {
+            if (err) return done(err);
+            should.exist(c);
+            c.bookId.should.eql(book.id);
+            c.name.should.eql('chapter 1');
+            book.chapters.updateById(c.id, {name: 'chapter 0', bookId: 10}, function(err, cc) {
+              should.exist(err);
+              err.message.should.startWith('Cannot override foreign key');
+              done();
+            });
+          });
+        });
+      });
+
       it('should create record on scope with promises', function(done) {
         Book.create()
         .then (function(book) {
@@ -2607,6 +2623,17 @@ describe('relations', function() {
       });
     });
 
+    it('should not update related item FK on scope', function(done) {
+      Item.findById(itemId, function(e, todo) {
+        if (e) return done(e);
+        todo.list.update({id: 10}, function(err, list) {
+          should.exist(err);
+          err.message.should.startWith('Cannot override foreign key');
+          done();
+        });
+      });
+    });
+
     it('should get related item on scope', function(done) {
       Item.findById(itemId, function(e, todo) {
         todo.list(function(err, list) {
@@ -2919,6 +2946,18 @@ describe('relations', function() {
       });
     });
 
+    it('should not update the related item FK on scope', function(done) {
+      Supplier.findById(supplierId, function(err, supplier) {
+        if (err) return done(err);
+        should.exist(supplier);
+        supplier.account.update({supplierName: 'Supplier A', supplierId: 10}, function(err, acct) {
+          should.exist(err);
+          err.message.should.containEql('Cannot override foreign key');
+          done();
+        });
+      });
+    });
+
     it('should update the related item on scope with promises', function(done) {
       Supplier.findById(supplierId)
       .then(function(supplier) {
@@ -2932,7 +2971,7 @@ describe('relations', function() {
       .catch(done);
     });
 
-    it('should ignore the foreign key in the update', function(done) {
+    it('should error trying to change the foreign key in the update', function(done) {
       Supplier.create({name: 'Supplier 2'}, function(e, supplier) {
         var sid = supplier.id;
         Supplier.findById(supplierId, function(e, supplier) {
@@ -2941,7 +2980,23 @@ describe('relations', function() {
           supplier.account.update({supplierName: 'Supplier A',
               supplierId: sid},
             function(err, act) {
-              should.not.exist(e);
+              should.exist(err);
+              err.message.should.startWith('Cannot override foreign key');
+              done();
+            });
+        });
+      });
+    });
+
+    it('should update the related item on scope with same foreign key', function(done) {
+      Supplier.create({name: 'Supplier 2'}, function(err, supplier) {
+        Supplier.findById(supplierId, function(err, supplier) {
+          if (err) return done(err);
+          should.exist(supplier);
+          supplier.account.update({supplierName: 'Supplier A',
+            supplierId: supplierId},
+            function(err, act) {
+              if (err) return done(err);
               act.supplierName.should.equal('Supplier A');
               act.supplierId.should.eql(supplierId);
               done();