recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix an issue where handles could load with the incorrect viewer when building mail about changes to related objects

Summary:
See <https://phabricator.wikimedia.org/T179591>. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.

Later, we introduced `HandlePool` and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.

When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.

Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of `setViewer()` and `clone` games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.

Test Plan:
- Created task A (public) with subtask B (private).
- Closed subtask B as a user with access to it.
- Viewed mail sent to subscribers of task A who can not see subtask B.
- Before change: mail discloses title of subtask B.
- After change: mail properly labels subtask B as "Restricted Task".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20525

epriestley b8f6248e 06778ea5

+20 -3
+20 -3
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 3048 3048 // Set this explicitly before we start swapping out the effective actor. 3049 3049 $this->setActingAsPHID($this->getActingAsPHID()); 3050 3050 3051 + $xaction_phids = mpull($xactions, 'getPHID'); 3052 + 3051 3053 $messages = array(); 3052 3054 foreach ($targets as $target) { 3053 3055 $original_actor = $this->getActor(); ··· 3059 3061 $caught = null; 3060 3062 $mail = null; 3061 3063 try { 3062 - // Reload handles for the new viewer. 3063 - $this->loadHandles($xactions); 3064 + // Reload the transactions for the current viewer. 3065 + if ($xaction_phids) { 3066 + $query = PhabricatorApplicationTransactionQuery::newQueryForObject( 3067 + $object); 3068 + 3069 + $mail_xactions = $query 3070 + ->setViewer($viewer) 3071 + ->withObjectPHIDs(array($object->getPHID())) 3072 + ->withPHIDs($xaction_phids) 3073 + ->execute(); 3074 + } else { 3075 + $mail_xactions = array(); 3076 + } 3077 + 3078 + // Reload handles for the current viewer. This covers older code which 3079 + // emits a list of handle PHIDs upfront. 3080 + $this->loadHandles($mail_xactions); 3064 3081 3065 - $mail = $this->buildMailForTarget($object, $xactions, $target); 3082 + $mail = $this->buildMailForTarget($object, $mail_xactions, $target); 3066 3083 3067 3084 if ($mail) { 3068 3085 if ($this->mustEncrypt) {