Fix SECURITY ISSUE 4: Members can forge others’ votes (Low). Bonus: Similar fixes to planning poker too done by xet7.

Thanks to Siam Thanat Hack (STH) and xet7 !

server/lib/tests/cards.methods.tests.js

@@ -0,0 +1,118 @@
+/* eslint-env mocha */
+import { expect } from 'chai';
+import sinon from 'sinon';
+import { Meteor } from 'meteor/meteor';
+import '/models/cards';
+
+// Helpers to access method handlers
+const voteHandler = () => Meteor.server.method_handlers['cards.vote'];
+const pokerVoteHandler = () => Meteor.server.method_handlers['cards.pokerVote'];
+
+// Preserve originals to restore after stubbing
+const origGetCard = ReactiveCache.getCard;
+const origGetBoard = ReactiveCache.getBoard;
+
+describe('cards methods security', function() {
+  let updateStub;
+
+  beforeEach(function() {
+    // Stub collection update to capture modifiers
+    updateStub = sinon.stub(Cards, 'update').returns(1);
+  });
+
+  afterEach(function() {
+    if (updateStub) updateStub.restore();
+    ReactiveCache.getCard = origGetCard;
+    ReactiveCache.getBoard = origGetBoard;
+  });
+
+  describe('cards.vote', function() {
+    it('denies non-member when allowNonBoardMembers=false', function() {
+      const cardId = 'card1';
+      const callerId = 'user-nonmember';
+      const board = { hasMember: id => id === 'someone-else' };
+      const card = { _id: cardId, boardId: 'board1', vote: { allowNonBoardMembers: false } };
+
+      ReactiveCache.getCard = () => card;
+      ReactiveCache.getBoard = () => board;
+
+      const callMethod = () => voteHandler().call({ userId: callerId }, cardId, true);
+      expect(callMethod).to.throw();
+      expect(updateStub.called).to.equal(false);
+    });
+
+    it('allows non-member only for own userId when allowNonBoardMembers=true', function() {
+      const cardId = 'card2';
+      const callerId = 'user-guest';
+      const board = { hasMember: id => id === 'someone-else' };
+      const card = { _id: cardId, boardId: 'board2', vote: { allowNonBoardMembers: true } };
+
+      ReactiveCache.getCard = () => card;
+      ReactiveCache.getBoard = () => board;
+
+      voteHandler().call({ userId: callerId }, cardId, true);
+
+      expect(updateStub.calledOnce).to.equal(true);
+      const [, modifier] = updateStub.getCall(0).args;
+      expect(modifier.$addToSet['vote.positive']).to.equal(callerId);
+      expect(modifier.$pull['vote.negative']).to.equal(callerId);
+      expect(modifier.$set.modifiedAt).to.be.instanceOf(Date);
+      expect(modifier.$set.dateLastActivity).to.be.instanceOf(Date);
+    });
+
+    it('ensures member votes only affect caller userId', function() {
+      const cardId = 'card3';
+      const callerId = 'member1';
+      const otherId = 'member2';
+      const board = { hasMember: id => (id === callerId || id === otherId) };
+      const card = { _id: cardId, boardId: 'board3', vote: { allowNonBoardMembers: false } };
+
+      ReactiveCache.getCard = () => card;
+      ReactiveCache.getBoard = () => board;
+
+      voteHandler().call({ userId: callerId }, cardId, true);
+
+      expect(updateStub.calledOnce).to.equal(true);
+      const [, modifier] = updateStub.getCall(0).args;
+      // Only callerId present in modifier
+      expect(modifier.$addToSet['vote.positive']).to.equal(callerId);
+      expect(modifier.$pull['vote.negative']).to.equal(callerId);
+    });
+  });
+
+  describe('cards.pokerVote', function() {
+    it('denies non-member when allowNonBoardMembers=false', function() {
+      const cardId = 'card4';
+      const callerId = 'nm';
+      const board = { hasMember: id => id === 'someone-else' };
+      const card = { _id: cardId, boardId: 'board4', poker: { allowNonBoardMembers: false } };
+
+      ReactiveCache.getCard = () => card;
+      ReactiveCache.getBoard = () => board;
+
+      const callMethod = () => pokerVoteHandler().call({ userId: callerId }, cardId, 'five');
+      expect(callMethod).to.throw();
+      expect(updateStub.called).to.equal(false);
+    });
+
+    it('allows non-member only for own userId when allowNonBoardMembers=true', function() {
+      const cardId = 'card5';
+      const callerId = 'guest';
+      const board = { hasMember: id => id === 'someone-else' };
+      const card = { _id: cardId, boardId: 'board5', poker: { allowNonBoardMembers: true } };
+
+      ReactiveCache.getCard = () => card;
+      ReactiveCache.getBoard = () => board;
+
+      pokerVoteHandler().call({ userId: callerId }, cardId, 'eight');
+
+      expect(updateStub.calledOnce).to.equal(true);
+      const [, modifier] = updateStub.getCall(0).args;
+      expect(modifier.$addToSet['poker.eight']).to.equal(callerId);
+      // Ensure removal from other buckets includes callerId
+      expect(modifier.$pull['poker.one']).to.equal(callerId);
+      expect(modifier.$set.modifiedAt).to.be.instanceOf(Date);
+      expect(modifier.$set.dateLastActivity).to.be.instanceOf(Date);
+    });
+  });
+});

server/lib/tests/cards.security.tests.js

@@ -0,0 +1,56 @@
+/* eslint-env mocha */
+import { expect } from 'chai';
+import '../utils';
+import '/models/cards';
+
+// Unit tests for canUpdateCard policy (deny direct vote updates)
+describe('cards security', function() {
+  describe(canUpdateCard.name, function() {
+    const userId = 'user1';
+    const board = {
+      hasMember: (id) => id === userId,
+    };
+    const doc = { boardId: 'board1' };
+
+    // Patch ReactiveCache.getBoard for this unit test scope if not defined
+    const origGetBoard = ReactiveCache && ReactiveCache.getBoard;
+    before(function() {
+      if (typeof ReactiveCache === 'object') {
+        ReactiveCache.getBoard = () => board;
+      }
+    });
+    after(function() {
+      if (typeof ReactiveCache === 'object') {
+        ReactiveCache.getBoard = origGetBoard;
+      }
+    });
+
+    it('denies anonymous users', function() {
+      expect(canUpdateCard(null, doc, ['title'])).to.equal(false);
+    });
+
+    it('denies direct vote updates', function() {
+      expect(canUpdateCard(userId, doc, ['vote'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['vote', 'modifiedAt', 'dateLastActivity'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['vote.positive'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['vote.negative'])).to.equal(false);
+    });
+
+    it('denies direct poker updates', function() {
+      expect(canUpdateCard(userId, doc, ['poker'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['poker.one'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['poker.allowNonBoardMembers'])).to.equal(false);
+      expect(canUpdateCard(userId, doc, ['poker.end'])).to.equal(false);
+    });
+
+    it('allows member updates when not touching vote', function() {
+      expect(canUpdateCard(userId, doc, ['title'])).to.equal(true);
+      expect(canUpdateCard(userId, doc, ['description', 'modifiedAt'])).to.equal(true);
+    });
+
+    it('denies non-members even when not touching vote', function() {
+      const nonMemberId = 'user2';
+      expect(canUpdateCard(nonMemberId, doc, ['title'])).to.equal(false);
+    });
+  });
+});

server/lib/tests/index.js

@@ -1,3 +1,5 @@
 import './utils.tests';
 import './users.security.tests';
 import './boards.security.tests';
+import './cards.security.tests';
+import './cards.methods.tests';