From 181f837d8cbae96bdf9dcbd31beaa3653c2c0285 Mon Sep 17 00:00:00 2001 From: Lauri Ojansivu Date: Mon, 29 Dec 2025 16:47:11 +0200 Subject: [PATCH] Security Fix 5: Read-only roles can still update cards. Thanks to Joshua Rogers of joshua.hu, Twitter MegaManSec ! --- models/boards.js | 2 +- models/cards.js | 14 +++++++------- models/lists.js | 6 +++--- models/swimlanes.js | 6 +++--- server/authentication.js | 8 ++++++++ server/lib/utils.js | 2 +- 6 files changed, 23 insertions(+), 15 deletions(-) diff --git a/models/boards.js b/models/boards.js index c0a760ea6..d36574f81 100644 --- a/models/boards.js +++ b/models/boards.js @@ -2408,7 +2408,7 @@ if (Meteor.isServer) { */ JsonRoutes.add('PUT', '/api/boards/:boardId/labels', function(req, res) { const id = req.params.boardId; - Authentication.checkBoardAccess(req.userId, id); + Authentication.checkBoardWriteAccess(req.userId, id); try { if (req.body.hasOwnProperty('label')) { const board = ReactiveCache.getBoard(id); diff --git a/models/cards.js b/models/cards.js index 7bb486e6b..2d7c20c26 100644 --- a/models/cards.js +++ b/models/cards.js @@ -3936,7 +3936,7 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( const newSwimlaneId = req.body.newSwimlaneId; const newListId = req.body.newListId; let updated = false; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); if (req.body.title) { // Basic client-side validation - server will handle full sanitization @@ -4292,8 +4292,8 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( ); } if (newBoardId && newSwimlaneId && newListId) { - // Validate destination board access - Authentication.checkBoardAccess(req.userId, newBoardId); + // Validate destination board write access + Authentication.checkBoardWriteAccess(req.userId, newBoardId); // Validate that the destination list exists and belongs to the destination board const destList = ReactiveCache.getList({ @@ -4409,7 +4409,7 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( const paramBoardId = req.params.boardId; const paramListId = req.params.listId; const paramCardId = req.params.cardId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const card = ReactiveCache.getCard(paramCardId); Cards.direct.remove({ @@ -4485,7 +4485,7 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( const paramListId = req.params.listId; const paramCustomFieldId = req.params.customFieldId; const paramCustomFieldValue = req.body.value; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const card = ReactiveCache.getCard({ _id: paramCardId, listId: paramListId, @@ -4541,7 +4541,7 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( const paramBoardId = req.params.boardId; const paramCardId = req.params.cardId; const paramListId = req.params.listId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const card = ReactiveCache.getCard({ _id: paramCardId, listId: paramListId, @@ -4580,7 +4580,7 @@ JsonRoutes.add('GET', '/api/boards/:boardId/cards_count', function( const paramBoardId = req.params.boardId; const paramCardId = req.params.cardId; const paramListId = req.params.listId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const card = ReactiveCache.getCard({ _id: paramCardId, listId: paramListId, diff --git a/models/lists.js b/models/lists.js index 84828a791..95820f03b 100644 --- a/models/lists.js +++ b/models/lists.js @@ -685,7 +685,7 @@ if (Meteor.isServer) { JsonRoutes.add('POST', '/api/boards/:boardId/lists', function(req, res) { try { const paramBoardId = req.params.boardId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const board = ReactiveCache.getBoard(paramBoardId); const id = Lists.insert({ title: req.body.title, @@ -731,7 +731,7 @@ if (Meteor.isServer) { const paramBoardId = req.params.boardId; const paramListId = req.params.listId; let updated = false; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const list = ReactiveCache.getList({ _id: paramListId, @@ -871,7 +871,7 @@ if (Meteor.isServer) { try { const paramBoardId = req.params.boardId; const paramListId = req.params.listId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); Lists.remove({ _id: paramListId, boardId: paramBoardId }); JsonRoutes.sendResult(res, { code: 200, diff --git a/models/swimlanes.js b/models/swimlanes.js index 07cce2807..f57ec2ff1 100644 --- a/models/swimlanes.js +++ b/models/swimlanes.js @@ -545,7 +545,7 @@ if (Meteor.isServer) { JsonRoutes.add('POST', '/api/boards/:boardId/swimlanes', function(req, res) { try { const paramBoardId = req.params.boardId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const board = ReactiveCache.getBoard(paramBoardId); const id = Swimlanes.insert({ @@ -581,7 +581,7 @@ if (Meteor.isServer) { try { const paramBoardId = req.params.boardId; const paramSwimlaneId = req.params.swimlaneId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); const board = ReactiveCache.getBoard(paramBoardId); const swimlane = ReactiveCache.getSwimlane({ _id: paramSwimlaneId, @@ -626,7 +626,7 @@ if (Meteor.isServer) { try { const paramBoardId = req.params.boardId; const paramSwimlaneId = req.params.swimlaneId; - Authentication.checkBoardAccess(req.userId, paramBoardId); + Authentication.checkBoardWriteAccess(req.userId, paramBoardId); Swimlanes.remove({ _id: paramSwimlaneId, boardId: paramBoardId }); JsonRoutes.sendResult(res, { code: 200, diff --git a/server/authentication.js b/server/authentication.js index 474de6e25..5b7ad2ee6 100644 --- a/server/authentication.js +++ b/server/authentication.js @@ -60,6 +60,14 @@ Meteor.startup(() => { Authentication.checkAdminOrCondition(userId, normalAccess); }; + // Helper function. Will throw an error if the user does not have write access to the board (excludes read-only users). + Authentication.checkBoardWriteAccess = function(userId, boardId) { + Authentication.checkLoggedIn(userId); + const board = ReactiveCache.getBoard(boardId); + const writeAccess = board.members.some(e => e.userId === userId && e.isActive && !e.isNoComments && !e.isCommentOnly && !e.isWorker && !e.isReadOnly && !e.isReadAssignedOnly); + Authentication.checkAdminOrCondition(userId, writeAccess); + }; + if (Meteor.isServer) { if ( process.env.ORACLE_OIM_ENABLED === 'true' || diff --git a/server/lib/utils.js b/server/lib/utils.js index b194bb246..bd6037b59 100644 --- a/server/lib/utils.js +++ b/server/lib/utils.js @@ -13,7 +13,7 @@ allowIsAnyBoardMember = function(userId, boards) { }; allowIsBoardMemberCommentOnly = function(userId, board) { - return board && board.hasMember(userId) && !board.hasCommentOnly(userId); + return board && board.hasMember(userId) && !board.hasReadOnly(userId) && !board.hasReadAssignedOnly(userId) && !board.hasNoComments(userId); }; allowIsBoardMemberNoComments = function(userId, board) {