From 5ac36e8f77ee017964b9971b4e8c481d14b0d2da Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Aug 2014 14:26:29 -0700 Subject: [PATCH] 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 | 10 +- ...DifferentialCreateDiffConduitAPIMethod.php | 6 +- ...ferentialCreateRawDiffConduitAPIMethod.php | 6 +- .../editor/DifferentialDiffEditor.php | 80 +++++++++ ...ifferentialDiffCreationRejectException.php | 5 + .../adapter/HeraldDifferentialAdapter.php | 93 ++++++++++ .../adapter/HeraldDifferentialDiffAdapter.php | 164 ++++++++++++++++++ .../HeraldDifferentialRevisionAdapter.php | 81 +-------- 8 files changed, 364 insertions(+), 81 deletions(-) create mode 100644 src/applications/differential/editor/DifferentialDiffEditor.php create mode 100644 src/applications/differential/exception/DifferentialDiffCreationRejectException.php create mode 100644 src/applications/herald/adapter/HeraldDifferentialAdapter.php create mode 100644 src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 99d372b7d0..9987ef205a 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php index 0c26035ecc..8da95dc19a 100644 --- a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php +++ b/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 diff --git a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php index fcd7e15ac1..f87dcb1a73 100644 --- a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php +++ b/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); } diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php new file mode 100644 index 0000000000..738dd7afeb --- /dev/null +++ b/src/applications/differential/editor/DifferentialDiffEditor.php @@ -0,0 +1,80 @@ +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(); + + } + +} diff --git a/src/applications/differential/exception/DifferentialDiffCreationRejectException.php b/src/applications/differential/exception/DifferentialDiffCreationRejectException.php new file mode 100644 index 0000000000..15f8383d91 --- /dev/null +++ b/src/applications/differential/exception/DifferentialDiffCreationRejectException.php @@ -0,0 +1,5 @@ +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; + } + +} diff --git a/src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php b/src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php new file mode 100644 index 0000000000..766be528f5 --- /dev/null +++ b/src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php @@ -0,0 +1,164 @@ +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; + } + +} diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 6e5cab8354..ef436894b7 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -1,9 +1,9 @@ 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();