Allow Herald "diff" rules to reject content before it is written

Summary: Fixes T5915. Occasionally, users derp up and diff private key material. Adding a pre-write Herald phase enables configuration of a partial layer of protection that will reject these changes before they hit disk, provided they can be detected by, e.g., filename.

Test Plan:
  - Added a rule with checks on every field, verified they looked fine in the transcript.
  - Created some revisions to test those changes (I have a bunch of revision rules locally).
  - Verified rejects don't write transcripts to the database.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5915

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

src/__phutil_library_map__.php

@@ -247,6 +247,8 @@ phutil_register_library_map(array(
     'DifferentialDependsOnField' => 'applications/differential/customfield/DifferentialDependsOnField.php',
     'DifferentialDiff' => 'applications/differential/storage/DifferentialDiff.php',
     'DifferentialDiffCreateController' => 'applications/differential/controller/DifferentialDiffCreateController.php',
+    'DifferentialDiffCreationRejectException' => 'applications/differential/exception/DifferentialDiffCreationRejectException.php',
+    'DifferentialDiffEditor' => 'applications/differential/editor/DifferentialDiffEditor.php',
     'DifferentialDiffPHIDType' => 'applications/differential/phid/DifferentialDiffPHIDType.php',
     'DifferentialDiffProperty' => 'applications/differential/storage/DifferentialDiffProperty.php',
     'DifferentialDiffQuery' => 'applications/differential/query/DifferentialDiffQuery.php',
@@ -748,6 +750,8 @@ phutil_register_library_map(array(
     'HeraldController' => 'applications/herald/controller/HeraldController.php',
     'HeraldCustomAction' => 'applications/herald/extension/HeraldCustomAction.php',
     'HeraldDAO' => 'applications/herald/storage/HeraldDAO.php',
+    'HeraldDifferentialAdapter' => 'applications/herald/adapter/HeraldDifferentialAdapter.php',
+    'HeraldDifferentialDiffAdapter' => 'applications/herald/adapter/HeraldDifferentialDiffAdapter.php',
     'HeraldDifferentialRevisionAdapter' => 'applications/herald/adapter/HeraldDifferentialRevisionAdapter.php',
     'HeraldDisableController' => 'applications/herald/controller/HeraldDisableController.php',
     'HeraldEditLogQuery' => 'applications/herald/query/HeraldEditLogQuery.php',
@@ -2998,6 +3002,8 @@ phutil_register_library_map(array(
       'PhabricatorDestructibleInterface',
     ),
     'DifferentialDiffCreateController' => 'DifferentialController',
+    'DifferentialDiffCreationRejectException' => 'Exception',
+    'DifferentialDiffEditor' => 'PhabricatorEditor',
     'DifferentialDiffPHIDType' => 'PhabricatorPHIDType',
     'DifferentialDiffProperty' => 'DifferentialDAO',
     'DifferentialDiffQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
@@ -3529,7 +3535,9 @@ phutil_register_library_map(array(
     'HeraldCondition' => 'HeraldDAO',
     'HeraldController' => 'PhabricatorController',
     'HeraldDAO' => 'PhabricatorLiskDAO',
-    'HeraldDifferentialRevisionAdapter' => 'HeraldAdapter',
+    'HeraldDifferentialAdapter' => 'HeraldAdapter',
+    'HeraldDifferentialDiffAdapter' => 'HeraldDifferentialAdapter',
+    'HeraldDifferentialRevisionAdapter' => 'HeraldDifferentialAdapter',
     'HeraldDisableController' => 'HeraldController',
     'HeraldEditLogQuery' => 'PhabricatorOffsetPagedQuery',
     'HeraldInvalidActionException' => 'Exception',

src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php

@@ -162,7 +162,11 @@ final class DifferentialCreateDiffConduitAPIMethod
         break;
     }
 
-    $diff->save();
+    id(new DifferentialDiffEditor())
+      ->setActor($viewer)
+      ->setContentSource(
+        PhabricatorContentSource::newFromConduitRequest($request))
+      ->saveDiff($diff);
 
     // If we didn't get an explicit `repositoryPHID` (which means the client is
     // old, or couldn't figure out which repository the working copy belongs

src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php

@@ -59,7 +59,11 @@ final class DifferentialCreateRawDiffConduitAPIMethod
       $diff->setRepositoryPHID($repository->getPHID());
     }
 
-    $diff->save();
+    id(new DifferentialDiffEditor())
+      ->setActor($viewer)
+      ->setContentSource(
+        PhabricatorContentSource::newFromConduitRequest($request))
+      ->saveDiff($diff);
 
     return $this->buildDiffInfoDictionary($diff);
   }

src/applications/differential/editor/DifferentialDiffEditor.php

@@ -0,0 +1,80 @@
+<?php
+
+final class DifferentialDiffEditor extends PhabricatorEditor {
+
+  private $contentSource;
+
+  public function setContentSource($content_source) {
+    $this->contentSource = $content_source;
+    return $this;
+  }
+
+  public function getContentSource() {
+    return $this->contentSource;
+  }
+
+  public function saveDiff(DifferentialDiff $diff) {
+    $actor = $this->requireActor();
+
+    // Generate a PHID first, so the transcript will point at the object if
+    // we deicde to preserve it.
+    $phid = $diff->generatePHID();
+    $diff->setPHID($phid);
+
+    $adapter = id(new HeraldDifferentialDiffAdapter())
+      ->setDiff($diff);
+
+    $adapter->setContentSource($this->getContentSource());
+    $adapter->setIsNewObject(true);
+
+    $engine = new HeraldEngine();
+
+    $rules = $engine->loadRulesForAdapter($adapter);
+    $rules = mpull($rules, null, 'getID');
+
+    $effects = $engine->applyRules($rules, $adapter);
+
+    $blocking_effect = null;
+    foreach ($effects as $effect) {
+      if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
+        $blocking_effect = $effect;
+        break;
+      }
+    }
+
+    if ($blocking_effect) {
+      $rule = idx($rules, $effect->getRuleID());
+      if ($rule && strlen($rule->getName())) {
+        $rule_name = $rule->getName();
+      } else {
+        $rule_name = pht('Unnamed Herald Rule');
+      }
+
+      $message = $effect->getTarget();
+      if (!strlen($message)) {
+        $message = pht('(None.)');
+      }
+
+      throw new DifferentialDiffCreationRejectException(
+        pht(
+          "Creation of this diff was rejected by Herald rule %s.\n".
+          "  Rule: %s\n".
+          "Reason: %s",
+          'H'.$effect->getRuleID(),
+          $rule_name,
+          $message));
+    }
+
+    $diff->save();
+
+    // NOTE: We only save the transcript if we didn't block the diff.
+    // Otherwise, we might save some of the diff's content in the transcript
+    // table, which would defeat the purpose of allowing rules to block
+    // storage of key material.
+
+    $engine->applyEffects($effects, $adapter, $rules);
+    $xscript = $engine->getTranscript();
+
+  }
+
+}

src/applications/differential/exception/DifferentialDiffCreationRejectException.php

@@ -0,0 +1,5 @@
+<?php
+
+final class DifferentialDiffCreationRejectException extends Exception {
+
+}

src/applications/herald/adapter/HeraldDifferentialAdapter.php

@@ -0,0 +1,93 @@
+<?php
+
+abstract class HeraldDifferentialAdapter extends HeraldAdapter {
+
+  private $repository = false;
+  private $diff;
+
+  abstract protected function loadChangesets();
+  abstract protected function loadChangesetsWithHunks();
+
+  public function getDiff() {
+    return $this->diff;
+  }
+
+  public function setDiff(DifferentialDiff $diff) {
+    $this->diff = $diff;
+    return $this;
+  }
+
+  public function loadRepository() {
+    if ($this->repository === false) {
+      $repository_phid = $this->getObject()->getRepositoryPHID();
+      if ($repository_phid) {
+        $repository = id(new PhabricatorRepositoryQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withPHIDs(array($repository_phid))
+          ->needProjectPHIDs(true)
+          ->executeOne();
+      }
+      $this->repository = false;
+    }
+
+    return $this->repository;
+  }
+
+
+  protected function loadAffectedPaths() {
+    $changesets = $this->loadChangesets();
+
+    $paths = array();
+    foreach ($changesets as $changeset) {
+      $paths[] = $this->getAbsoluteRepositoryPathForChangeset($changeset);
+    }
+
+    return $paths;
+  }
+
+  protected function getAbsoluteRepositoryPathForChangeset(
+    DifferentialChangeset $changeset) {
+
+    $repository = $this->loadRepository();
+    if (!$repository) {
+      return '/'.ltrim($changeset->getFilename(), '/');
+    }
+
+    $diff = $this->getDiff();
+
+    return $changeset->getAbsoluteRepositoryPath($repository, $diff);
+  }
+
+  protected function loadContentDictionary() {
+    $add_lines = DifferentialHunk::FLAG_LINES_ADDED;
+    $rem_lines = DifferentialHunk::FLAG_LINES_REMOVED;
+    $mask = ($add_lines | $rem_lines);
+    return $this->loadContentWithMask($mask);
+  }
+
+  protected function loadAddedContentDictionary() {
+    return $this->loadContentWithMask(DifferentialHunk::FLAG_LINES_ADDED);
+  }
+
+  protected function loadRemovedContentDictionary() {
+    return $this->loadContentWithMask(DifferentialHunk::FLAG_LINES_REMOVED);
+  }
+
+  protected function loadContentWithMask($mask) {
+    $changesets = $this->loadChangesetsWithHunks();
+
+    $dict = array();
+    foreach ($changesets as $changeset) {
+      $content = array();
+      foreach ($changeset->getHunks() as $hunk) {
+        $content[] = $hunk->getContentWithMask($mask);
+      }
+
+      $path = $this->getAbsoluteRepositoryPathForChangeset($changeset);
+      $dict[$path] = implode("\n", $content);
+    }
+
+    return $dict;
+  }
+
+}

src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php

@@ -0,0 +1,164 @@
+<?php
+
+final class HeraldDifferentialDiffAdapter extends HeraldDifferentialAdapter {
+
+  public function getAdapterApplicationClass() {
+    return 'PhabricatorDifferentialApplication';
+  }
+
+  protected function loadChangesets() {
+    return $this->loadChangesetsWithHunks();
+  }
+
+  protected function loadChangesetsWithHunks() {
+    return $this->getDiff()->getChangesets();
+  }
+
+  public function getObject() {
+    return $this->getDiff();
+  }
+
+  public function getAdapterContentType() {
+    return 'differential.diff';
+  }
+
+  public function getAdapterContentName() {
+    return pht('Differential Diffs');
+  }
+
+  public function getAdapterContentDescription() {
+    return pht(
+      "React to new diffs being uploaded, before writes occur.\n".
+      "These rules can reject diffs before they are written to permanent ".
+      "storage, to prevent users from accidentally uploading private keys or ".
+      "other sensitive information.");
+  }
+
+  public function supportsRuleType($rule_type) {
+    switch ($rule_type) {
+      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+        return true;
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
+      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
+      default:
+        return false;
+    }
+  }
+
+  public function getFields() {
+    return array_merge(
+      array(
+        self::FIELD_AUTHOR,
+        self::FIELD_AUTHOR_PROJECTS,
+        self::FIELD_REPOSITORY,
+        self::FIELD_REPOSITORY_PROJECTS,
+        self::FIELD_DIFF_FILE,
+        self::FIELD_DIFF_CONTENT,
+        self::FIELD_DIFF_ADDED_CONTENT,
+        self::FIELD_DIFF_REMOVED_CONTENT,
+      ),
+      parent::getFields());
+  }
+
+  public function getRepetitionOptions() {
+    return array(
+      HeraldRepetitionPolicyConfig::FIRST,
+    );
+  }
+
+  public function getPHID() {
+    return $this->getObject()->getPHID();
+  }
+
+  public function getHeraldName() {
+    return pht('New Diff');
+  }
+
+  public function getActionNameMap($rule_type) {
+    return array(
+      self::ACTION_BLOCK => pht('Block diff with message'),
+    );
+  }
+
+  public function getHeraldField($field) {
+    switch ($field) {
+      case self::FIELD_AUTHOR:
+        return $this->getObject()->getAuthorPHID();
+        break;
+      case self::FIELD_AUTHOR_PROJECTS:
+        $author_phid = $this->getHeraldField(self::FIELD_AUTHOR);
+        if (!$author_phid) {
+          return array();
+        }
+
+        $projects = id(new PhabricatorProjectQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withMemberPHIDs(array($author_phid))
+          ->execute();
+
+        return mpull($projects, 'getPHID');
+      case self::FIELD_DIFF_FILE:
+        return $this->loadAffectedPaths();
+      case self::FIELD_REPOSITORY:
+        $repository = $this->loadRepository();
+        if (!$repository) {
+          return null;
+        }
+        return $repository->getPHID();
+      case self::FIELD_REPOSITORY_PROJECTS:
+        $repository = $this->loadRepository();
+        if (!$repository) {
+          return array();
+        }
+        return $repository->getProjectPHIDs();
+      case self::FIELD_DIFF_CONTENT:
+        return $this->loadContentDictionary();
+      case self::FIELD_DIFF_ADDED_CONTENT:
+        return $this->loadAddedContentDictionary();
+      case self::FIELD_DIFF_REMOVED_CONTENT:
+        return $this->loadRemovedContentDictionary();
+    }
+
+    return parent::getHeraldField($field);
+  }
+
+  public function getActions($rule_type) {
+    switch ($rule_type) {
+      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+        return array_merge(
+          array(
+            self::ACTION_BLOCK,
+            self::ACTION_NOTHING,
+          ),
+          parent::getActions($rule_type));
+    }
+  }
+
+  public function applyHeraldEffects(array $effects) {
+    assert_instances_of($effects, 'HeraldEffect');
+
+    $result = array();
+    foreach ($effects as $effect) {
+      $action = $effect->getAction();
+      switch ($action) {
+        case self::ACTION_NOTHING:
+          $result[] = new HeraldApplyTranscript(
+            $effect,
+            true,
+            pht('Did nothing.'));
+          break;
+        case self::ACTION_BLOCK:
+          $result[] = new HeraldApplyTranscript(
+            $effect,
+            true,
+            pht('Blocked diff.'));
+          break;
+        default:
+          throw new Exception(pht('No rules to handle action "%s"!', $action));
+      }
+    }
+
+    return $result;
+  }
+
+}

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -1,9 +1,9 @@
 <?php
 
-final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
+final class HeraldDifferentialRevisionAdapter
+  extends HeraldDifferentialAdapter {
 
   protected $revision;
-  protected $diff;
 
   protected $explicitCCs;
   protected $explicitReviewers;
@@ -17,7 +17,6 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
   protected $buildPlans = array();
   protected $requiredSignatureDocumentPHIDs = array();
 
-  protected $repository;
   protected $affectedPackages;
   protected $changesets;
   private $haveHunks;
@@ -160,25 +159,6 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $this->revision->getTitle();
   }
 
-  public function loadRepository() {
-    if ($this->repository === null) {
-      $this->repository = false;
-      $repository_phid = $this->getObject()->getRepositoryPHID();
-      if ($repository_phid) {
-        $repository = id(new PhabricatorRepositoryQuery())
-          ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->withPHIDs(array($repository_phid))
-          ->needProjectPHIDs(true)
-          ->executeOne();
-        if ($repository) {
-          $this->repository = $repository;
-        }
-      }
-    }
-
-    return $this->repository;
-  }
-
   protected function loadChangesets() {
     if ($this->changesets === null) {
       $this->changesets = $this->diff->loadChangesets();
@@ -186,7 +166,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $this->changesets;
   }
 
-  private function loadChangesetsWithHunks() {
+  protected function loadChangesetsWithHunks() {
     $changesets = $this->loadChangesets();
 
     if ($changesets && !$this->haveHunks) {
@@ -202,61 +182,6 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $changesets;
   }
 
-  protected function loadAffectedPaths() {
-    $changesets = $this->loadChangesets();
-
-    $paths = array();
-    foreach ($changesets as $changeset) {
-      $paths[] = $this->getAbsoluteRepositoryPathForChangeset($changeset);
-    }
-    return $paths;
-  }
-
-  protected function getAbsoluteRepositoryPathForChangeset(
-    DifferentialChangeset $changeset) {
-
-    $repository = $this->loadRepository();
-    if (!$repository) {
-      return '/'.ltrim($changeset->getFilename(), '/');
-    }
-
-    $diff = $this->diff;
-
-    return $changeset->getAbsoluteRepositoryPath($repository, $diff);
-  }
-
-  protected function loadContentDictionary() {
-    $add_lines = DifferentialHunk::FLAG_LINES_ADDED;
-    $rem_lines = DifferentialHunk::FLAG_LINES_REMOVED;
-    $mask = ($add_lines | $rem_lines);
-    return $this->loadContentWithMask($mask);
-  }
-
-  protected function loadAddedContentDictionary() {
-    return $this->loadContentWithMask(DifferentialHunk::FLAG_LINES_ADDED);
-  }
-
-  protected function loadRemovedContentDictionary() {
-    return $this->loadContentWithMask(DifferentialHunk::FLAG_LINES_REMOVED);
-  }
-
-  private function loadContentWithMask($mask) {
-    $changesets = $this->loadChangesetsWithHunks();
-
-    $dict = array();
-    foreach ($changesets as $changeset) {
-      $content = array();
-      foreach ($changeset->getHunks() as $hunk) {
-        $content[] = $hunk->getContentWithMask($mask);
-      }
-
-      $path = $this->getAbsoluteRepositoryPathForChangeset($changeset);
-      $dict[$path] = implode("\n", $content);
-    }
-
-    return $dict;
-  }
-
   public function loadAffectedPackages() {
     if ($this->affectedPackages === null) {
       $this->affectedPackages = array();