Page MenuHomePhabricator
Files F13197513

D20468.diff
No OneTemporary
Actions

D20468.diff
View Options

diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php
--- a/src/applications/diffusion/herald/HeraldCommitAdapter.php
+++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php
@@ -175,31 +175,34 @@
}
public function loadDifferentialRevision() {
- $viewer = $this->getViewer();
-
if ($this->affectedRevision === null) {
- $this->affectedRevision = false;
-
- $commit = $this->getObject();
- $data = $commit->getCommitData();
-
- $revision_id = $data->getCommitDetail('differential.revisionID');
- if ($revision_id) {
- // NOTE: The Herald rule owner might not actually have access to
- // the revision, and can control which revision a commit is
- // associated with by putting text in the commit message. However,
- // the rules they can write against revisions don't actually expose
- // anything interesting, so it seems reasonable to load unconditionally
- // here.
-
- $revision = id(new DifferentialRevisionQuery())
- ->withIDs(array($revision_id))
- ->setViewer($viewer)
- ->needReviewers(true)
- ->executeOne();
- if ($revision) {
- $this->affectedRevision = $revision;
- }
+ $viewer = $this->getViewer();
+
+ // NOTE: The viewer here is omnipotent, which means that Herald discloses
+ // some information users do not normally have access to when rules load
+ // the revision related to a commit. See D20468.
+
+ // A user who wants to learn about "Dxyz" can write a Herald rule which
+ // uses all the "Related revision..." fields, then push a commit which
+ // contains "Differential Revision: Dxyz" in the message to make Herald
+ // evaluate the commit with "Dxyz" as the related revision.
+
+ // At time of writing, this commit will link to the revision and the
+ // transcript for the commit will disclose some information about the
+ // revision (like reviewers, subscribers, and build status) which the
+ // commit author could not otherwise see.
+
+ // For now, we just accept this. The disclosures are relatively
+ // uninteresting and you have to jump through a lot of hoops (and leave
+ // a lot of evidence) to get this information.
+
+ $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit(
+ $viewer,
+ $this->getObject());
+ if ($revision) {
+ $this->affectedRevision = $revision;
+ } else {
+ $this->affectedRevision = false;
}
}
diff --git a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php
--- a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php
@@ -46,4 +46,23 @@
return $map;
}
+ public static function loadRevisionForCommit(
+ PhabricatorUser $viewer,
+ PhabricatorRepositoryCommit $commit) {
+
+ $data = $commit->getCommitData();
+
+ $revision_id = $data->getCommitDetail('differential.revisionID');
+ if (!$revision_id) {
+ return null;
+ }
+
+ return id(new DifferentialRevisionQuery())
+ ->setViewer($viewer)
+ ->withIDs(array($revision_id))
+ ->needReviewers(true)
+ ->executeOne();
+ }
+
+
}
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php
@@ -135,16 +135,10 @@
$data = $commit->getCommitData();
$author_phid = $data->getCommitDetail('authorPHID');
- $revision_id = $data->getCommitDetail('differential.revisionID');
- if ($revision_id) {
- $revision = id(new DifferentialRevisionQuery())
- ->setViewer($viewer)
- ->withIDs(array($revision_id))
- ->needReviewers(true)
- ->executeOne();
- } else {
- $revision = null;
- }
+
+ $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit(
+ $viewer,
+ $commit);
$requests = $commit->getAudits();
$requests = mpull($requests, null, 'getAuditorPHID');

File Metadata

Mime Type
text/plain
Expires
Tue, May 14, 12:14 AM (3 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6277477
Default Alt Text
D20468.diff (4 KB)

Event Timeline

Phabricator · Built by Phacility