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

Command icon 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:

Comments

lisa.rae created an issue. See original summary.

lisa.rae’s picture

Status: Active » Needs review

ivnish changed the visibility of the branch 3617056-thread-operations-mark to hidden.

  • ivnish committed 1fbebe99 on 2.0.x authored by lisa.rae
    fix: #3617056 Thread operations (mark read/unread, remove thread, change...
ivnish’s picture

Status: Needs review » Fixed

Nice catch! Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.