Problem/Motivation
PrivatemsgThreadAccessControlHandler::checkAccess (src/Entity/Access/PrivatemsgThreadAccessControlHandler.php) grants the mark_read, mark_unread, remove_thread and change_tags operations based only on permissions (privatemsg use messages actions, privatemsg change thread tags), with no check that the acting account owns the thread:
case 'mark_unread':
case 'mark_read':
case 'remove_thread':
if ($account->hasPermission('privatemsg use messages actions')) {
return AccessResult::allowed();
}
break;
case 'change_tags':
if ($account->hasPermission('privatemsg use messages actions') && $account->hasPermission('privatemsg change thread tags')) {
return AccessResult::allowed();
}
break;
Every privatemsg_thread entity is a per-member copy owned by exactly one user (the thread's owner; the inbox view lists only the current user's threads via the uid_current filter). The VBO actions delegate their access checks to this handler (PrivatemsgReadThreadAction, PrivatemsgUnreadThreadAction, PrivatemsgRemoveThreadAction and PrivatemsgChangeTagsAction all call $object->access(...)), so any invocation path that reaches these operations with a foreign thread id succeeds. In particular PrivatemsgRemoveThreadAction::execute will mark any thread deleted and remove the acting user from all its group threads.
Steps to reproduce
1. User A and user B are in a conversation (each owns their own thread copy).
2. Log in as B (who has privatemsg use messages actions, e.g. a normal messaging role).
3. Directly invoke $threadA->access('remove_thread', $b), or drive the remove-thread VBO action against A's thread id — it is allowed even though B does not own that thread. Same for mark_read / mark_unread / change_tags on a thread B does not own.
Proposed resolution
Require that the acting account owns the thread before the permission check, in
PrivatemsgThreadAccessControlHandler::checkAccess:
case 'mark_unread':
case 'mark_read':
case 'remove_thread':
if ($entity->getOwnerId() === $account->id() && $account->hasPermission('privatemsg use messages actions')) {
return AccessResult::allowed();
}
break;
case 'change_tags':
if ($entity->getOwnerId() === $account->id() && $account->hasPermission('privatemsg use messages actions') && $account->hasPermission('privatemsg change thread tags')) {
return AccessResult::allowed();
}
break;
Ownership (rather than mere membership) is the correct gate here because each
member acts on their own thread copy, and the view operation already enforces
ownership the same way. Legitimate flows are unaffected: the inbox view only
ever lists the current user's own threads, so their copies pass the owner check.
Remaining tasks
Patch with tests is forthcoming.
Issue fork privatemsg-3617056
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- Issue-3617056
changes, plain diff MR !64
- 3617056-thread-operations-mark
compare
Comments
Comment #3
lisa.rae commentedComment #6
ivnishNice catch! Thanks!