Dragging a checklist item on one's own board is no longer reported as a cross-board attack.

Dragging a checklist item within its card sets cardId and checklistId
to where the item already is. The ChecklistBleed deny rule
(GHSA-gv8h-5p3p-6hx7) checked only that the destination board was
writable, so for a comment-only member or a Worker - whose own board is
one they cannot write to - an ordinary drag tripped the high-severity
canary checklist-item.cross-board-move and named them in Admin Panel /
Problems / Security as if they had attacked another board.

The board the document is already on is no destination now. That board
is the one of its SOURCE CARD, which the allow rule decides on - not
the document's own denormalized boardId, as the reporter's patch read
it: the insert hooks kept a boardId the client sent, so an item on the
attacker's card could carry a victim board's id and would have passed
as staying while moved onto a victim card. Those hooks (checklists,
checklist items, and now text notes on the server) set boardId from the
card always. A move to another unwritable board, and an unknown
destination, are still denied and still reported.

Thanks to Raymond-Voogt and xet7 !

Fixes #6813.

models/cardTextNotes.js

@@ -102,6 +102,17 @@ CardTextNotes.before.insert((userId, doc) => {
   }
 });
 
+// #6813: on the server the board is ALWAYS the card's, not what the client
+// sent - a text note on its own card could otherwise carry another board's id
+// (published to that board, and mistaken for being on it).
+if (Meteor.isServer) {
+  CardTextNotes.before.insert(async (userId, doc) => {
+    if (!doc.cardId) return;
+    const card = await ReactiveCache.getCard(doc.cardId);
+    if (card && card.boardId) doc.boardId = card.boardId;
+  });
+}
+
 if (Meteor.isServer) {
   const { ensureIndex } = require('/server/lib/mongoStartup');
   Meteor.startup(async () => {

server/lib/tests/checklistbleed.security.tests.js

@@ -7,6 +7,7 @@ import Checklists from '/models/checklists';
 import {
   denyCrossBoardMoveByCard,
   denyCrossBoardMoveByChecklistItem,
+  sourceBoardOfCardChild,
 } from '/server/lib/utils';
 
 // Security regression test for GHSA-gv8h-5p3p-6hx7 (ChecklistBleed):
@@ -89,5 +90,40 @@ describe('ChecklistBleed — checklist/item cross-board move deny rules (GHSA-gv
       const denied = await denyCrossBoardMoveByChecklistItem(ATTACKER, { $set: { checklistId: 'attackerChecklist' } });
       expect(denied).to.equal(false);
     });
+
+    // #6813: a comment-only member or Worker reordering items on their own
+    // card - the drag sets cardId/checklistId to where the item already is. The
+    // allow rule decides that; the cross-board canary must not fire.
+    it('does not call a drag within the item\'s own board a cross-board move (#6813)', async function() {
+      const source = await sourceBoardOfCardChild({ cardId: 'victimCard' });
+      expect(source).to.equal('victimBoard');
+      const denied = await denyCrossBoardMoveByChecklistItem(ATTACKER,
+        { $set: { cardId: 'victimCard', checklistId: 'victimChecklist' } }, source);
+      expect(denied).to.equal(false);
+    });
+
+    it('still DENIES a move off the item\'s own board to an unwritable one (#6813)', async function() {
+      const denied = await denyCrossBoardMoveByChecklistItem(ATTACKER,
+        { $set: { checklistId: 'victimChecklist' } }, await sourceBoardOfCardChild({ cardId: 'attackerCard' }));
+      expect(denied).to.equal(true);
+    });
+
+    // The source is the item's CARD's board, never its own boardId: an item on
+    // the attacker's card carrying the victim board's id is still a move into
+    // the victim board.
+    it('still DENIES a move onto a victim card for an item whose own boardId names the victim board (#6813)', async function() {
+      const forged = { cardId: 'attackerCard', boardId: 'victimBoard' };
+      const source = await sourceBoardOfCardChild(forged);
+      expect(source).to.equal('sourceBoard');
+      const denied = await denyCrossBoardMoveByChecklistItem(ATTACKER,
+        { $set: { cardId: 'victimCard', checklistId: 'victimChecklist' } }, source);
+      expect(denied).to.equal(true);
+    });
+
+    it('skips nothing when the source card is gone (#6813)', async function() {
+      expect(await sourceBoardOfCardChild({ cardId: 'doesNotExist' })).to.equal(null);
+      const denied = await denyCrossBoardMoveByChecklistItem(ATTACKER, { $set: { cardId: 'victimCard' } }, null);
+      expect(denied).to.equal(true);
+    });
   });
 });

server/lib/utils.js

@@ -114,11 +114,34 @@ export async function denyCrossBoardMove(userId, modifier) {
 // Shared destination gate for the checklist deny rules below: returns true to
 // DENY when boardId names a board the caller cannot write to. An unknown/missing
 // board id also denies, matching denyCrossBoardMove's fail-closed behavior.
-async function denyMoveToUnwritableBoard(userId, boardId) {
+// #6813: the board the document is already on is not a destination. Dragging
+// a checklist item within its card sets cardId and checklistId to where the
+// item already is; for a comment-only member or a Worker their own board is "a
+// board they cannot write to", so that ordinary drag tripped the cross-board
+// canary and named them in Admin Panel / Problems as if they had attacked
+// another board. Staying on the board is no cross-board move, and whether the
+// caller may edit there is the allow rule's question, unchanged.
+//
+// `sourceBoardId` must be the board of the document's SOURCE CARD - what the
+// allow rule decides on (sourceBoardOfCardChild below) - never the document's
+// own denormalized boardId: that one a client could set on insert, and an item
+// on its own card carrying a victim board's id would then pass as "staying"
+// while being moved onto a victim card.
+async function denyMoveToUnwritableBoard(userId, boardId, sourceBoardId) {
   if (typeof boardId !== 'string' || !boardId) return false;
+  if (typeof sourceBoardId === 'string' && sourceBoardId && boardId === sourceBoardId) return false;
   return !allowIsBoardMemberWithWriteAccess(userId, await Boards.findOneAsync(boardId));
 }
 
+// The board of the card a checklist, checklist item or text note is attached
+// to - the source the allow rules check write access on (canEditCardOrLinkedCard
+// on doc.cardId). Null when the card is gone: then nothing is skipped.
+export async function sourceBoardOfCardChild(doc) {
+  if (!doc || typeof doc.cardId !== 'string' || !doc.cardId) return null;
+  const card = await Cards.findOneAsync(doc.cardId, { fields: { boardId: 1 } });
+  return (card && card.boardId) || null;
+}
+
 // Security (GHSA-gv8h-5p3p-6hx7): Checklists and ChecklistItems are attached to a
 // card and carry a denormalized boardId. They are MOVED between cards by setting a
 // new cardId (and, for items, a new checklistId) in a direct collection update; a
@@ -134,16 +157,16 @@ async function denyMoveToUnwritableBoard(userId, boardId) {
 // Legitimate moves (destination writable) and same-card edits are unaffected, and
 // server-side Meteor methods bypass allow/deny entirely, so the trusted
 // moveChecklist method still works.
-export async function denyCrossBoardMoveByCard(userId, modifier) {
+export async function denyCrossBoardMoveByCard(userId, modifier, sourceBoardId) {
   const set = modifier && modifier.$set;
   if (!set) return false;
   // Direct boardId change (defense in depth, same as denyCrossBoardMove).
-  if (await denyMoveToUnwritableBoard(userId, set.boardId)) return true;
+  if (await denyMoveToUnwritableBoard(userId, set.boardId, sourceBoardId)) return true;
   // cardId change: resolve the destination card's board.
   if (typeof set.cardId === 'string' && set.cardId) {
     const card = await Cards.findOneAsync(set.cardId);
     if (!card) return true;
-    if (await denyMoveToUnwritableBoard(userId, card.boardId)) return true;
+    if (await denyMoveToUnwritableBoard(userId, card.boardId, sourceBoardId)) return true;
   }
   return false;
 }
@@ -151,15 +174,15 @@ export async function denyCrossBoardMoveByCard(userId, modifier) {
 // As denyCrossBoardMoveByCard, plus the ChecklistItem-only checklistId move: an
 // item can be re-parented to another checklist, which (via that checklist's card)
 // may live on a board the caller cannot write to. Returns true to DENY.
-export async function denyCrossBoardMoveByChecklistItem(userId, modifier) {
-  if (await denyCrossBoardMoveByCard(userId, modifier)) return true;
+export async function denyCrossBoardMoveByChecklistItem(userId, modifier, sourceBoardId) {
+  if (await denyCrossBoardMoveByCard(userId, modifier, sourceBoardId)) return true;
   const set = modifier && modifier.$set;
   if (set && typeof set.checklistId === 'string' && set.checklistId) {
     const checklist = await Checklists.findOneAsync(set.checklistId);
     if (!checklist) return true;
     const card = await Cards.findOneAsync(checklist.cardId);
     if (!card) return true;
-    if (await denyMoveToUnwritableBoard(userId, card.boardId)) return true;
+    if (await denyMoveToUnwritableBoard(userId, card.boardId, sourceBoardId)) return true;
   }
   return false;
 }

server/models/checklistItems.js

@@ -21,8 +21,11 @@ const { deferSyncItemRecording, deferSyncChecklistRecording } = require('/server
 // moved to another card (its move helper sets a new cardId), so the board
 // publication can filter checklist items with a single reactive cursor on
 // boardId and still see items on newly added cards. Server-authoritative.
+// #6813: ALWAYS from the card, not only when missing - a client's own boardId
+// would otherwise stay, and a document on its own card could carry another
+// board's id (published to that board, and mistaken for being on it).
 ChecklistItems.before.insert(async (userId, doc) => {
-  if (!doc.boardId && doc.cardId) {
+  if (doc.cardId) {
     const card = await ReactiveCache.getCard(doc.cardId);
     if (card) {
       doc.boardId = card.boardId;

server/models/checklists.js

@@ -78,8 +78,11 @@ Meteor.methods({
 // single reactive cursor on boardId and still see checklists on newly added
 // cards. Server-authoritative: the board publication runs on the server, so the
 // server document carrying boardId is what matters.
+// #6813: ALWAYS from the card, not only when missing - a client's own boardId
+// would otherwise stay, and a document on its own card could carry another
+// board's id (published to that board, and mistaken for being on it).
 Checklists.before.insert(async (userId, doc) => {
-  if (!doc.boardId && doc.cardId) {
+  if (doc.cardId) {
     const card = await ReactiveCache.getCard(doc.cardId);
     if (card) {
       doc.boardId = card.boardId;

server/permissions/cardTextNotes.js

@@ -1,6 +1,6 @@
 import Cards from '/models/cards';
 import CardTextNotes from '/models/cardTextNotes';
-import { denyCrossBoardMoveByCard } from '/server/lib/utils';
+import { denyCrossBoardMoveByCard, sourceBoardOfCardChild } from '/server/lib/utils';
 import { tripCanaryDeny } from '/server/lib/canary';
 import { canEditCardOrLinkedCard } from '/server/lib/linkedCardPermission';
 
@@ -27,8 +27,9 @@ CardTextNotes.allow({
 // caller cannot write to.
 CardTextNotes.deny({
   async update(userId, doc, fieldNames, modifier) {
-    if (!(await denyCrossBoardMoveByCard(userId, modifier))) return false;
+    // #6813: the source board from the source card, as the allow rule.
+    if (!(await denyCrossBoardMoveByCard(userId, modifier, await sourceBoardOfCardChild(doc)))) return false;
     return tripCanaryDeny('cardTextNote.cross-board-move', { userId });
   },
-  fetch: [],
+  fetch: ['cardId'],
 });

server/permissions/checklistItems.js

@@ -1,7 +1,7 @@
 import Boards from '/models/boards';
 import Cards from '/models/cards';
 import ChecklistItems from '/models/checklistItems';
-import { denyCrossBoardMoveByChecklistItem } from '/server/lib/utils';
+import { denyCrossBoardMoveByChecklistItem, sourceBoardOfCardChild } from '/server/lib/utils';
 import { tripCanaryDeny } from '/server/lib/canary';
 import { canEditCardOrLinkedCard } from '/server/lib/linkedCardPermission';
 const { workerMayToggleChecklistItem } = require('/models/lib/workerChecklistItemToggle');
@@ -55,8 +55,9 @@ ChecklistItems.deny({
 
 ChecklistItems.deny({
   async update(userId, doc, fieldNames, modifier) {
-    if (!(await denyCrossBoardMoveByChecklistItem(userId, modifier))) return false;
+    // #6813: the source board from the source card, as the allow rule.
+    if (!(await denyCrossBoardMoveByChecklistItem(userId, modifier, await sourceBoardOfCardChild(doc)))) return false;
     return tripCanaryDeny('checklist-item.cross-board-move', { userId });
   },
-  fetch: [],
+  fetch: ['cardId'],
 });

server/permissions/checklists.js

@@ -1,6 +1,6 @@
 import Cards from '/models/cards';
 import Checklists from '/models/checklists';
-import { denyCrossBoardMoveByCard } from '/server/lib/utils';
+import { denyCrossBoardMoveByCard, sourceBoardOfCardChild } from '/server/lib/utils';
 import { tripCanaryDeny } from '/server/lib/canary';
 import { canEditCardOrLinkedCard } from '/server/lib/linkedCardPermission';
 
@@ -27,8 +27,9 @@ Checklists.allow({
 // card). Deny any move whose destination board the caller cannot write to.
 Checklists.deny({
   async update(userId, doc, fieldNames, modifier) {
-    if (!(await denyCrossBoardMoveByCard(userId, modifier))) return false;
+    // #6813: the source board from the source card, as the allow rule.
+    if (!(await denyCrossBoardMoveByCard(userId, modifier, await sourceBoardOfCardChild(doc)))) return false;
     return tripCanaryDeny('checklist.cross-board-move', { userId });
   },
-  fetch: [],
+  fetch: ['cardId'],
 });

tests/checklistSameBoardCanary6813.test.cjs

@@ -0,0 +1,115 @@
+'use strict';
+
+// #6813: dragging a checklist item within its own card sets cardId and
+// checklistId to where the item already is; for a comment-only member or a
+// Worker their own board is "a board they cannot write to", so the ordinary
+// drag tripped ChecklistBleed's cross-board canary and named them in Admin
+// Panel / Problems as an attacker. The board the document is already on is
+// not a destination now - and that board is its SOURCE CARD's board, the one
+// the allow rule decides on, never the document's own boardId, which a client
+// could set on insert (the reporter's patch read doc.boardId: an item on the
+// attacker's card carrying the victim board's id would have passed as
+// "staying" while moved onto a victim card).
+// Run: node tests/checklistSameBoardCanary6813.test.cjs
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+
+const root = path.join(__dirname, '..');
+const read = p => fs.readFileSync(path.join(root, p), 'utf8');
+let passed = 0;
+const pending = [];
+function test(name, fn) { pending.push([name, fn]); }
+
+// The real functions of server/lib/utils.js, run against stubs.
+const utils = read('server/lib/utils.js');
+const slice = (from, to) => utils.slice(utils.indexOf(from), utils.indexOf(to, utils.indexOf(from) + 1));
+const src = [
+  slice('async function denyMoveToUnwritableBoard(', '\n}\n') + '\n}\n',
+  slice('export async function sourceBoardOfCardChild(', '\n}\n') + '\n}\n',
+  slice('export async function denyCrossBoardMoveByCard(', '\n}\n') + '\n}\n',
+  slice('export async function denyCrossBoardMoveByChecklistItem(', '\n}\n') + '\n}\n',
+].join('\n').replace(/export async function/g, 'async function');
+
+const boards = {
+  own: { writers: ['writer'] },         // the commenter's own board: they cannot write
+  victim: { writers: ['victimOwner'] },
+  attacker: { writers: ['commenter', 'writer'] },
+};
+const cards = { ownCard: { boardId: 'own' }, victimCard: { boardId: 'victim' }, attackerCard: { boardId: 'attacker' } };
+const checklists = { ownChecklist: { cardId: 'ownCard' }, victimChecklist: { cardId: 'victimCard' } };
+const make = new Function('Boards', 'Cards', 'Checklists', 'allowIsBoardMemberWithWriteAccess',
+  `${src}\nreturn { denyMoveToUnwritableBoard, sourceBoardOfCardChild, denyCrossBoardMoveByCard, denyCrossBoardMoveByChecklistItem };`);
+const f = make(
+  { findOneAsync: async id => boards[id] || null },
+  { findOneAsync: async id => cards[id] || null },
+  { findOneAsync: async id => checklists[id] || null },
+  (userId, board) => Boolean(board && board.writers.includes(userId)),
+);
+
+test('the reporter\'s drag: a commenter reordering items on their own card is no cross-board move', async () => {
+  const doc = { cardId: 'ownCard', checklistId: 'ownChecklist' };
+  const source = await f.sourceBoardOfCardChild(doc);
+  assert.strictEqual(source, 'own');
+  assert.strictEqual(await f.denyCrossBoardMoveByChecklistItem('commenter', { $set: { cardId: 'ownCard', checklistId: 'ownChecklist' } }, source), false);
+  assert.strictEqual(await f.denyCrossBoardMoveByCard('commenter', { $set: { cardId: 'ownCard' } }, source), false, 'a checklist or text note too');
+});
+
+test('negative: a move off the board to one the caller cannot write to is still denied', async () => {
+  const source = await f.sourceBoardOfCardChild({ cardId: 'attackerCard' });
+  assert.strictEqual(await f.denyCrossBoardMoveByChecklistItem('commenter', { $set: { checklistId: 'victimChecklist' } }, source), true);
+  assert.strictEqual(await f.denyCrossBoardMoveByChecklistItem('commenter', { $set: { cardId: 'victimCard' } }, source), true);
+  assert.strictEqual(await f.denyCrossBoardMoveByCard('commenter', { $set: { boardId: 'victim' } }, source), true);
+});
+
+test('negative: an item whose own boardId names the victim board is still denied - the source is its card\'s board', async () => {
+  const forged = { cardId: 'attackerCard', boardId: 'victim' };
+  const source = await f.sourceBoardOfCardChild(forged);
+  assert.strictEqual(source, 'attacker', 'never the document\'s own boardId');
+  assert.strictEqual(await f.denyCrossBoardMoveByChecklistItem('commenter', { $set: { cardId: 'victimCard', checklistId: 'victimChecklist' } }, source), true);
+});
+
+test('negative: unknown destinations and a missing source still fail closed', async () => {
+  assert.strictEqual(await f.denyCrossBoardMoveByChecklistItem('commenter', { $set: { checklistId: 'nope' } }, 'own'), true);
+  assert.strictEqual(await f.denyCrossBoardMoveByCard('commenter', { $set: { cardId: 'nope' } }, 'own'), true);
+  assert.strictEqual(await f.sourceBoardOfCardChild({ cardId: 'gone' }), null);
+  assert.strictEqual(await f.denyCrossBoardMoveByCard('commenter', { $set: { cardId: 'victimCard' } }, null), true);
+  assert.strictEqual(await f.denyMoveToUnwritableBoard('commenter', 'victim', ''), true, 'an empty source skips nothing');
+});
+
+test('the three deny rules pass the source card\'s board, and nothing passes a document\'s own boardId', () => {
+  for (const [file, fn, key] of [
+    ['server/permissions/checklistItems.js', 'denyCrossBoardMoveByChecklistItem', 'checklist-item'],
+    ['server/permissions/checklists.js', 'denyCrossBoardMoveByCard', 'checklist'],
+    ['server/permissions/cardTextNotes.js', 'denyCrossBoardMoveByCard', 'cardTextNote'],
+  ]) {
+    const s = read(file);
+    assert.ok(s.includes(`if (!(await ${fn}(userId, modifier, await sourceBoardOfCardChild(doc)))) return false;\n    return tripCanaryDeny('${key}.cross-board-move', { userId });\n  },\n  fetch: ['cardId'],`), file);
+  }
+  // The shape, anywhere on the server: no cross-board deny fed a document's boardId.
+  const walk = dir => fs.readdirSync(path.join(root, dir), { withFileTypes: true }).flatMap(e => {
+    const rel = path.join(dir, e.name);
+    if (e.isDirectory()) return ['node_modules', 'tests'].includes(e.name) ? [] : walk(rel);
+    return /\.js$/.test(e.name) ? [rel] : [];
+  });
+  for (const file of [...walk('server'), ...walk('models')]) {
+    assert.ok(!/denyCrossBoardMoveBy\w+\([^)]*doc\s*&&\s*doc\.boardId|denyCrossBoardMoveBy\w+\([^)]*doc\.boardId/.test(read(file)), `${file}: a deny fed doc.boardId`);
+  }
+});
+
+test('on insert the board is always the card\'s, not the client\'s', () => {
+  for (const [file, coll] of [['server/models/checklistItems.js', 'ChecklistItems'], ['server/models/checklists.js', 'Checklists']]) {
+    const s = read(file);
+    const hook = s.slice(s.indexOf(`${coll}.before.insert(`), s.indexOf('});', s.indexOf(`${coll}.before.insert(`)));
+    assert.match(hook, /if \(doc\.cardId\) \{\n\s+const card = await ReactiveCache\.getCard\(doc\.cardId\);\n\s+if \(card\) \{\n\s+doc\.boardId = card\.boardId;/, file);
+    assert.ok(!/!doc\.boardId/.test(hook), `negative: ${file} no longer keeps a client's boardId`);
+  }
+  assert.match(read('models/cardTextNotes.js'), /if \(Meteor\.isServer\) \{\n\s+CardTextNotes\.before\.insert\(async \(userId, doc\) => \{\n\s+if \(!doc\.cardId\) return;\n\s+const card = await ReactiveCache\.getCard\(doc\.cardId\);\n\s+if \(card && card\.boardId\) doc\.boardId = card\.boardId;/);
+});
+
+(async () => {
+  console.log('checklistSameBoardCanary6813:');
+  for (const [name, fn] of pending) { await fn(); passed += 1; console.log('  ok -', name); }
+  console.log(`checklistSameBoardCanary6813: ${passed} passed`);
+})().catch(e => { console.error(e); process.exit(1); });

tests/playwright/specs/checklist-same-board-canary-6813.e2e.js

@@ -0,0 +1,37 @@
+'use strict';
+
+// #6813: a comment-only member moving a checklist item within its own card -
+// what a drag sends - is not a cross-board attack: no ChecklistBleed canary in
+// Admin Panel / Problems. A real move into a board they cannot write to still
+// trips it.
+const { test, expect } = require('../fixtures');
+const db = require('../helpers/db');
+
+const call = (page, method, ...args) => page.evaluate(async ({ method, args }) => {
+  try { return { value: await Meteor.callAsync(method, ...args) }; }
+  catch (error) { return { error: error.error || error.message }; }
+}, { method, args });
+const canaryRows = () => db.countDocuments('eventlog', { source: 'canary:checklist-item.cross-board-move' });
+
+test('#6813: a drag on one\'s own board is no cross-board move; a move into another board still is', async ({ boardPage: page, board, user }) => {
+  const card = db.findOne('cards', { boardId: board.boardId, title: 'Alpha Card' });
+  const checklistId = db.uid('cl');
+  const itemId = db.uid('ci');
+  db.insertOne('checklists', { _id: checklistId, cardId: card._id, boardId: board.boardId, title: 'Steps', sort: 0, createdAt: new Date() });
+  db.insertOne('checklistItems', { _id: itemId, checklistId, cardId: card._id, boardId: board.boardId, title: 'One', sort: 0, isFinished: false });
+  // The user is comment-only on their own board.
+  db.updateOne('boards', { _id: board.boardId, 'members.userId': user.id }, { $set: { 'members.$.isCommentOnly': true, 'members.$.isAdmin': false } });
+  db.deleteMany('eventlog', { source: 'canary:checklist-item.cross-board-move' });
+
+  // What a drag within the card sends: cardId and checklistId as they are.
+  await call(page, '/checklistItems/update', { _id: itemId }, { $set: { cardId: card._id, checklistId, sort: 1 } });
+  await page.waitForTimeout(1500);
+  expect(canaryRows()).toBe(0);
+
+  // Negative: into a card of a board the user is not a member of - still the canary.
+  const other = db.seedBoard({ ownerId: db.uid('owner'), cardTitlesPerList: [['Elsewhere']] });
+  const otherCard = db.findOne('cards', { boardId: other.boardId });
+  await call(page, '/checklistItems/update', { _id: itemId }, { $set: { cardId: otherCard._id } });
+  await expect.poll(canaryRows, { timeout: 15_000 }).toBeGreaterThan(0);
+  expect(db.findOne('checklistItems', { _id: itemId }).cardId).toBe(card._id);
+});