8000 ListLockBleed: a list's write refusal holds for the checklist REST ro… · wekan/wekan@214b20e · GitHub
[go: up one dir, main page]

Skip to content

Commit 214b20e

Browse files
committed
ListLockBleed: a list's write refusal holds for the checklist REST routes too.
GHSA-4734-8vrg-8j3w. A board admin can refuse a role write access in one list (#4527, list.roleExceptions). The card and comment REST routes and the DDP checklist writes applied that refusal; the six checklist and checklist-item REST routes asked only the board, so a member the list refused still created, renamed, ticked and deleted its cards' checklists and items with an API token. They now call assertListNotRefused with the card's list, as the card routes do, and answer 403. The refusal is recorded in Admin Panel -> Problems as ListLockBleed (authz.list-locked, medium: recorded, never disables the account): no screen offers such a write, so it is the lock being stepped round. The card routes' refusals are recorded through the same gate. tests/listPermissions.test.cjs scans every REST write route of the tree that names a card and fails on one that does not ask the list - not only the per-list card routes it checked before - naming the three that follow their own rule (a comment's edit and deletion, dependency lines, a bot run's report), and checks the refusal is recorded. It fails on the old routes. tests/playwright/specs/list-lock-checklists.e2e.js refuses all six writes in a locked list with 403, leaves the checklist and item untouched, records the attempt, and still lets the same member write in an open list and the board admin in the locked one. Thanks to tsurutanmen and xet7 !
1 parent 0f89ad0 commit 214b20e

8 files changed

Lines changed: 163 additions & 3 deletions

File tree

‎models/lib/securityCategories.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,10 @@ const CATALOG = {
235235
// GHSA-6xqf-jh8x-4wxc: an assigned-only member archiving or converting a card
236236
// not assigned to them through archiveSelectedCards / createBoardFromCard.
237237
'authz.assigned-card-write': { category: 'authz', bleed: 'UnassignedArchiveBleed', severity: 'medium', cwe: 'CWE-863' },
238+
// GHSA-4734-8vrg-8j3w: a REST write in a list whose exception refuses the
239+
// caller's role (#4527). The web UI offers no such write, so it is the lock
240+
// being stepped round with a token. Medium: recorded, never disables.
241+
'authz.list-locked': { category: 'authz', bleed: 'ListLockBleed', severity: 'medium', cwe: 'CWE-863' },
238242
'authz.tenant': { category: 'authz', bleed: 'TenantBleed', severity: 'medium', cwe: 'CWE-862' },
239243
'authz.search-session': { category: 'authz', bleed: 'SessionBleed', severity: 'medium', cwe: 'CWE-639' },
240244
'authz.database': { category: 'authz', bleed: 'DatabaseBleed', severity: 'high', cwe: 'CWE-863' },

‎server/lib/listPermissionGate.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,19 @@ export async function canCommentInList(userId, board, listId) {
3333
* The REST API's half: a list that refuses this user's role refuses the API
3434
* too (403), so a restriction cannot be stepped round with a token. A list's
3535
* GRANT does not open the API to a role the board does not let write.
36+
*
37+
* ListLockBleed (GHSA-4734-8vrg-8j3w): the checklist and checklist-item routes
38+
* did not ask, so a member a list refused still wrote its cards' checklists
39+
* over REST. Every REST write that names a card asks now
40+
* (tests/listPermissions.test.cjs), and the refusal is recorded in Admin
41+
* Panel -> Problems: no screen offers such a write, so it is an attempt.
3642
*/
37-
export async function assertListNotRefused(userId, board, listId, capability = 'write') {
43+
export async function assertListNotRefused(userId, board, listId, capability = 'write', source = 'rest:list-locked') {
3844
if ((await listException(userId, board, listId, capability)) !== false) return;
45+
try {
46+
require('/server/lib/securityLog').record({ key: 'authz.list-locked', action: 'blocked', source, userId,
47+
detail: `A REST ${capability} in a list whose exception refuses the caller's role.` });
48+
} catch (e) { /* logging must never break the guard */ }
3949
const { Meteor } = require('meteor/meteor');
4050
const error = new Meteor.Error('Forbidden', 'Forbidden');
4151
error.statusCode = 403;

‎server/models/checklistItems.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { Meteor } from 'meteor/meteor';
33
import { WebApp } from 'meteor/webapp';
44
import { Authentication } from '/server/authentication';
55
import { sendJsonResult } from '/server/apiMiddleware';
6+
import { assertListNotRefused } from '/server/lib/listPermissionGate';
67
import { ReactiveCache } from '/imports/reactiveCache';
78
import ChecklistItems, {
89
itemCreation,
@@ -145,6 +146,8 @@ WebApp.handlers.post(
145146
if (checklist) {
146147
const card = await ReactiveCache.getCard(paramCardId);
147148
if (card && card.boardId === paramBoardId) {
149+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
150+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist-item');
148151
// Append, like the UI does. Every item created over REST used to be given
149152
// `sort: 0`, so a checklist filled through the API came out in an order
150153
// nobody chose - and every item after the first shared the first one's
@@ -214,6 +217,8 @@ WebApp.handlers.put(
214217
});
215218
return;
216219
}
220+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
221+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist-item');
217222

218223
const deadline = parseDueAt(req.body);
219224
if (deadline.error) {
@@ -277,6 +282,8 @@ WebApp.handlers.delete(
277282
});
278283
return;
279284
}
285+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
286+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist-item');
280287

281288
await ChecklistItems.direct.removeAsync({ _id: paramItemId });
282289
sendJsonResult(res, {

‎server/models/checklists.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { parseDueAt, checklistWithItems } from '/server/lib/checklistDeadlines';
22
import { Meteor } from 'meteor/meteor';
33
import { WebApp } from 'meteor/webapp';
4+
import { assertListNotRefused } from '/server/lib/listPermissionGate';
45
import { Authentication } from '/server/authentication';
56
import { sendJsonResult } from '/server/apiMiddleware';
67
import { ReactiveCache } from '/imports/reactiveCache';
@@ -266,6 +267,8 @@ WebApp.handlers.post(
266267
});
267268
return;
268269
}
270+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
271+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist');
269272

270273
const deadline = parseDueAt(req.body);
271274
if (deadline.error) {
@@ -335,6 +338,8 @@ WebApp.handlers.put(
335338
});
336339
return;
337340
}
341+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
342+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist');
338343

339344
const checklist = await ReactiveCache.getChecklist({
340345
_id: paramChecklistId,
@@ -390,6 +395,8 @@ WebApp.handlers.delete(
390395
});
391396
return;
392397
}
398+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the list's refusal holds over REST too (#4527).
399+
await assertListNotRefused(req.userId, await ReactiveCache.getBoard(paramBoardId), card.listId, 'write', 'rest:checklist');
393400

394401
const checklist = await ReactiveCache.getChecklist({
395402
_id: paramChecklistId,

‎tests/checklistDeadlines.test.cjs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@ assert.equal(parseDueAt({ dueAt: '2026-09-15T14:30:00+02:00' }).dueAt.toISOStrin
3030
vm.runInNewContext(src.slice(start, end), {
3131
WebApp: { handlers: { put: (route, fn) => { handler = fn; } } }, parseDueAt,
3232
Authentication: { checkBoardWriteAccess: async () => { if (denied) throw Error('denied'); } },
33-
ReactiveCache: { getCard: async () => wrongCard ? null : card, getChecklist: async () => ({ _id: 'check' }) },
33+
ReactiveCache: { getCard: async () => wrongCard ? null : card, getChecklist: async () => ({ _id: 'check' }), getBoard: async () => ({ _id: 'board' }) },
34+
// ListLockBleed's list check: tests/listPermissions.test.cjs pins it; here the list allows.
35+
assertListNotRefused: async () => {},
3436
Checklists: { direct: { updateAsync: async (...args) => updates.push(args) } },
3537
sendJsonResult: (res, data) => { response = data; },
3638
});
@@ -51,7 +53,8 @@ assert.equal(parseDueAt({ dueAt: '2026-09-15T14:30:00+02:00' }).dueAt.toISOStrin
5153
vm.runInNewContext(itemSource.slice(itemStart, itemEnd), {
5254
WebApp: { handlers: { put: (route, fn) => { itemHandler = fn; } } }, parseDueAt,
5355
Authentication: { checkBoardWriteAccess: async () => {} },
54-
ReactiveCache: { getChecklistItem: async () => ({ _id: 'item', cardId: 'card', checklistId: 'check' }), getCard: async () => ({ _id: 'card', boardId: 'board' }) },
56+
ReactiveCache: { getChecklistItem: async () => ({ _id: 'item', cardId: 'card', checklistId: 'check' }), getCard: async () => ({ _id: 'card', boardId: 'board' }), getBoard: async () => ({ _id: 'board' }) },
57+
assertListNotRefused: async () => {},
5558
ChecklistItems: { direct: { updateAsync: async (...args) => itemUpdates.push(args) } },
5659
sendJsonResult: (res, data) => { itemResponse = data; },
5760
});

‎tests/checklistRestBleed.test.cjs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,11 @@ function fixture() {
5757
getChecklistItems: async () => [],
5858
getCard: async id => cards[id] || null,
5959
getChecklist: async q => (checklists[q._id] && checklists[q._id].cardId === q.cardId ? checklists[q._id] : null),
60+
getBoard: async id => ({ _id: id }),
6061
},
62+
// ListLockBleed's list check (tests/listPermissions.test.cjs pins it); the
63+
// lists of this fixture refuse nobody.
64+
assertListNotRefused: async () => {},
6165
ChecklistItems: {
6266
insertAsync: async doc => { inserted.push(doc); return 'new'; },
6367
direct: {

‎tests/listPermissions.test.cjs‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,73 @@ test('the REST API honours a list\'s refusal on every per-list card route', () =
130130
assert.match(read('server/models/cardComments.js'), /canCommentInList\(req\.userId, board, card\.listId\)/, 'comments too');
131131
});
132132

133+
// ListLockBleed (GHSA-4734-8vrg-8j3w): the checklist and checklist-item REST
134+
// routes asked only the board, so a member a list refused still wrote its
135+
// cards' checklists with a token. Not one route at a time: EVERY REST write in
136+
// the tree that names a card must ask the list - or be named here, with why.
137+
test('negative: every REST write that names a card asks the list (ListLockBleed)', () => {
138+
const unrelated = {
139+
// A comment's edit and deletion follow the comment's own rule (its author,
140+
// or a board admin: assertCanMutateComment), on DDP as over REST; a list's
141+
// comment refusal stops NEW comments, which the POST route asks.
142+
'PUT /api/boards/:boardId/cards/:cardId/comments/:commentId': 'comment rule',
143+
'DELETE /api/boards/:boardId/cards/:cardId/comments/:commentId': 'comment rule',
144+
// Dependency lines have their own board-wide rule (canEditBoardDependencies),
145+
// the same on DDP (setBoardDependency) as over REST.
146+
'POST /api/boards/:boardId/cards/:cardId/dependencies': 'dependency rule',
147+
'PUT /api/boards/:boardId/cards/:cardId/dependencies/:targetId': 'dependency rule',
148+
'DELETE /api/boards/:boardId/cards/:cardId/dependencies/:targetId': 'dependency rule',
149+
// A bot runner's report of its own run, written to BotRuns, not the card.
150+
'POST /api/boards/:boardId/cards/:cardId/bot-runs/:runId': 'bot run report',
151+
};
152+
const found = [], missing = [];
153+
const walk = dir => {
154+
for (const entry of fs.readdirSync(path.join(ROOT, dir), { withFileTypes: true })) {
155+
const rel = path.join(dir, entry.name);
156+
if (entry.isDirectory()) { if (!['tests', 'node_modules'].includes(entry.name)) walk(rel); continue; }
157+
if (!/\.js$/.test(entry.name)) continue;
158+
const src = read(rel);
159+
for (const m of src.matchAll(/WebApp\.handlers\.(post|put|delete|patch)\(\s*['"`]([^'"`]*:cardId[^'"`]*)['"`]/g)) {
160+
const route = `${m[1].toUpperCase()} ${m[2]}`;
161+
found.push(route);
162+
const next = src.indexOf('WebApp.handlers.', m.index + 1);
163+
let body = src.slice(m.index, next < 0 ? undefined : next);
164+
const delegate = /await (\w+)\(req, res\b/.exec(body);
165+
if (delegate) {
166+
const start = src.indexOf(`async function ${delegate[1]}(`);
167+
if (start >= 0) body += src.slice(start, src.indexOf('\n}\n', start));
168+
}
169+
if (!/assertListNotRefused\(|canWriteInList\(|canCommentInList\(/.test(body) && !unrelated[route]) missing.push(`${route} (${rel})`);
170+
}
171+
}
172+
};
173+
walk('server');
174+
walk('models');
175+
assert.ok(found.length >= 20, `the REST card write routes are found (${found.length})`);
176+
for (const route of ['POST /api/boards/:boardId/cards/:cardId/checklists', 'PUT /api/boards/:boardId/cards/:cardId/checklists/:checklistId',
177+
'DELETE /api/boards/:boardId/cards/:cardId/checklists/:checklistId', 'POST /api/boards/:boardId/cards/:cardId/checklists/:checklistId/items',
178+
'PUT /api/boards/:boardId/cards/:cardId/checklists/:checklistId/items/:itemId',
179+
'DELETE /api/boards/:boardId/cards/:cardId/checklists/:checklistId/items/:itemId']) {
180+
assert.ok(found.includes(route), `${route} is scanned`);
181+
}
182+
assert.deepEqual(missing, []);
183+
// The exceptions are real routes: a renamed one must be looked at again.
184+
for (const route of Object.keys(unrelated)) assert.ok(found.includes(route), `${route} still exists`);
185+
});
186+
187+
test('the refusal is recorded in Admin Panel -> Problems, and never breaks the guard (ListLockBleed)', () => {
188+
const gate = read('server/lib/listPermissionGate.js');
189+
const fn = gate.slice(gate.indexOf('export async function assertListNotRefused('));
190+
assert.match(fn, /record\(\{ key: 'authz\.list-locked', action: 'blocked'/);
191+
assert.match(fn, /catch \(e\) \{ \/\* logging must never break the guard \*\/ \}/);
192+
// Recorded only on the refusal: after the early return of an allowed write.
193+
assert.ok(fn.indexOf("!== false) return;") < fn.indexOf('record('), 'an allowed write records nothing');
194+
const { categoryFor } = require('../models/lib/securityCategories');
195+
const cat = categoryFor('authz.list-locked');
196+
assert.equal(cat.bleed, 'ListLockBleed');
197+
assert.equal(cat.severity, 'medium', 'recorded, never disables the account');
198+
});
199+
133200
test('the client offers what the list allows', () => {
134201
const utils = read('client/lib/utils.js');
135202
assert.match(utils, /canModifyCard\([\s\S]*?return Utils\.currentUserCanInList\('write', card, board\);/);
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict';
2+
// ListLockBleed (GHSA-4734-8vrg-8j3w): a list whose exception refuses a role
3+
// write (#4527) refuses that role's checklist and checklist-item writes over
4+
// REST too, and the attempt is in Admin Panel -> Problems. A list without the
5+
// refusal still takes the same writes.
6+
const { test, expect } = require('../fixtures');
7+
const db = require('../helpers/db');
8+
9+
test('a list refusing a role write refuses its checklist REST writes (ListLockBleed)', async ({ request, board, user, user2 }) => {
10+
const original = db.getBoard(board.boardId).members;
11+
const [lockedListId, openListId] = board.listIds;
12+
const card = id => db.find('cards', { boardId: board.boardId, listId: id })[0];
13+
const locked = card(lockedListId), open = card(openListId);
14+
const api = (method, url, data) => request[method](url, {
15+
headers: { Authorization: `Bearer ${user2.token}`, 'Content-Type': 'application/json' }, ...(data ? { data } : {}) });
16+
const recorded = () => db.findOne('eventlog', { bleed: 'ListLockBleed' })?.count || 0;
17+
db.updateOne('boards', { _id: board.boardId }, { $set: { members: [...original.filter(m => m.userId !== user2.id),
18+
{ userId: user2.id, isActive: true, isAdmin: false }] } });
19+
db.updateOne('lists', { _id: lockedListId }, { $set: { roleExceptions: { normal: { write: false } } } });
20+
const checklist = { _id: db.uid('locked-checklist'), cardId: locked._id, title: 'Locked', sort: 0 };
21+
const item = { _id: db.uid('locked-item'), cardId: locked._id, checklistId: checklist._id, title: 'Locked item', sort: 0, isFinished: false };
22+
db.insertOne('checklists', checklist);
23+
db.insertOne('checklistItems', item);
24+
try {
25+
const before = recorded();
26+
const base = `/api/boards/${board.boardId}/cards/${locked._id}/checklists`;
27+
for (const [method, url, data] of [
28+
['post', base, { title: 'x' }],
29+
['put', `${base}/${checklist._id}`, { title: 'renamed' }],
30+
['post', `${base}/${checklist._id}/items`, { title: 'new item' }],
31+
['put', `${base}/${checklist._id}/items/${item._id}`, { isFinished: 'true', title: 'changed' }],
32+
['delete', `${base}/${checklist._id}/items/${item._id}`],
33+
['delete', `${base}/${checklist._id}`],
34+
]) {
35+
const res = await api(method, url, data);
36+
expect(res.status(), `${method.toUpperCase()} ${url}`).toBe(403);
37+
}
38+
expect(db.find('checklists', { cardId: locked._id })).toEqual([expect.objectContaining({ _id: checklist._id, title: 'Locked' })]);
39+
expect(db.find('checklistItems', { cardId: locked._id })).toEqual([expect.objectContaining({ _id: item._id, title: 'Locked item', isFinished: false })]);
40+
expect(recorded()).toBeGreaterThan(before);
41+
expect(db.findOne('users', { _id: user2.id }).loginDisabled || false).toBe(false);
42+
43+
// Negative: the same member writes the checklists of a list without the refusal.
44+
const made = await api('post', `/api/boards/${board.boardId}/cards/${open._id}/checklists`, { title: 'Open', items: ['one'] });
45+
expect(made.status()).toBe(200);
46+
const openId = (await made.json())._id;
47+
expect((await api('put', `/api/boards/${board.boardId}/cards/${open._id}/checklists/${openId}`, { title: 'Open renamed' })).status()).toBe(200);
48+
expect((await api('delete', `/api/boards/${board.boardId}/cards/${open._id}/checklists/${openId}`)).status()).toBe(200);
49+
// And the board admin, whom no list exception binds, writes the locked list's.
50+
const admin = await request.post(base, { headers: { Authorization: `Bearer ${user.token}`, 'Content-Type': 'application/json' }, data: { title: 'Admin' } });
51+
expect(admin.status()).toBe(200);
52+
} finally {
53+
db.updateOne('boards', { _id: board.boardId }, { $set: { members: original } });
54+
db.updateOne('lists', { _id: lockedListId }, { $unset: { roleExceptions: '' } });
55+
db.deleteMany('checklists', { cardId: { $in: [locked._id, open._id] } });
56+
db.deleteMany('checklistItems', { cardId: { $in: [locked._id, open._id] } });
57+
}
58+
});

0 commit comments

Comments
 (0)
0