From 9bfe558587aa55d8f0cf037640e3cfee6d642593 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Nov 2018 05:11:45 -0800 Subject: [PATCH] Add a "touched paths" limit to repositories, limiting the maximum number of paths any commit may touch Summary: Depends on D19831. Ref T13216. See PHI908. Allegedly, a user copied a large repository into itself and then pushed it. Great backup strategy, but it can create headaches for administrators. Allow a "maximum paths you can touch with one commit" limit to be configured, to make it harder for users to make this push this kind of commit by accident. If you actually intended to do this, you can work around this by breaking your commit into pieces (or temporarily removing the limit). This isn't a security/policy sort of option, it's just a guard against silly mistakes. Test Plan: Set limit to 2, tried to push 3 files, got rejected. Raised limit, pushed changes successfully. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13216 Differential Revision: https://secure.phabricator.com/D19839 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 9 +++ .../engine/DiffusionCommitHookEngine.php | 55 +++++++++++++- ...ffusionRepositoryLimitsManagementPanel.php | 11 +++ .../storage/PhabricatorRepository.php | 8 ++ .../storage/PhabricatorRepositoryPushLog.php | 4 + ...ricatorRepositoryTouchLimitTransaction.php | 76 +++++++++++++++++++ .../user/userguide/diffusion_managing.diviner | 27 +++++++ 8 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 src/applications/repository/xaction/PhabricatorRepositoryTouchLimitTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2b6732b7d5..e4ffb74b0f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4203,6 +4203,7 @@ phutil_register_library_map(array( 'PhabricatorRepositorySyncEventPHIDType' => 'applications/repository/phid/PhabricatorRepositorySyncEventPHIDType.php', 'PhabricatorRepositorySyncEventQuery' => 'applications/repository/query/PhabricatorRepositorySyncEventQuery.php', 'PhabricatorRepositoryTestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php', + 'PhabricatorRepositoryTouchLimitTransaction' => 'applications/repository/xaction/PhabricatorRepositoryTouchLimitTransaction.php', 'PhabricatorRepositoryTrackOnlyTransaction' => 'applications/repository/xaction/PhabricatorRepositoryTrackOnlyTransaction.php', 'PhabricatorRepositoryTransaction' => 'applications/repository/storage/PhabricatorRepositoryTransaction.php', 'PhabricatorRepositoryTransactionQuery' => 'applications/repository/query/PhabricatorRepositoryTransactionQuery.php', @@ -10198,6 +10199,7 @@ phutil_register_library_map(array( 'PhabricatorRepositorySyncEventPHIDType' => 'PhabricatorPHIDType', 'PhabricatorRepositorySyncEventQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase', + 'PhabricatorRepositoryTouchLimitTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryTrackOnlyTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryTransaction' => 'PhabricatorModularTransaction', 'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 355a67480c..29dc588b45 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -491,6 +491,15 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Change the copy time limit.')) ->setConduitTypeDescription(pht('New repository copy time limit.')) ->setValue($object->getCopyTimeLimit()), + id(new PhabricatorTextEditField()) + ->setKey('touchLimit') + ->setLabel(pht('Touched Paths Limit')) + ->setTransactionType( + PhabricatorRepositoryTouchLimitTransaction::TRANSACTIONTYPE) + ->setDescription(pht('Maximum permitted paths touched per commit.')) + ->setConduitDescription(pht('Change the touch limit.')) + ->setConduitTypeDescription(pht('New repository touch limit.')) + ->setValue($object->getTouchLimit()), ); } diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 17f037471a..6856b8676d 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -39,6 +39,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $emailPHIDs = array(); private $changesets = array(); private $changesetsSize = 0; + private $filesizeCache = array(); /* -( Config )------------------------------------------------------------- */ @@ -174,6 +175,15 @@ final class DiffusionCommitHookEngine extends Phobject { throw $ex; } + try { + if (!$is_initial_import) { + $this->rejectCommitsAffectingTooManyPaths($content_updates); + } + } catch (DiffusionCommitHookRejectException $ex) { + $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_TOUCHES; + throw $ex; + } + try { if (!$is_initial_import) { $this->rejectEnormousChanges($content_updates); @@ -1276,7 +1286,8 @@ final class DiffusionCommitHookEngine extends Phobject { foreach ($content_updates as $update) { $identifier = $update->getRefNew(); - $sizes = $this->loadFileSizesForCommit($identifier); + $sizes = $this->getFileSizesForCommit($identifier); + foreach ($sizes as $path => $size) { if ($size <= $limit) { continue; @@ -1301,7 +1312,47 @@ final class DiffusionCommitHookEngine extends Phobject { } } - public function loadFileSizesForCommit($identifier) { + private function rejectCommitsAffectingTooManyPaths(array $content_updates) { + $repository = $this->getRepository(); + + $limit = $repository->getTouchLimit(); + if (!$limit) { + return; + } + + foreach ($content_updates as $update) { + $identifier = $update->getRefNew(); + + $sizes = $this->getFileSizesForCommit($identifier); + if (count($sizes) > $limit) { + $message = pht( + 'COMMIT AFFECTS TOO MANY PATHS'. + "\n". + 'This repository ("%s") is configured with a touched files limit '. + 'that caps the maximum number of paths any single commit may '. + 'affect. You are pushing a change ("%s") which exceeds this '. + 'limit: it affects %s paths, but the largest number of paths any '. + 'commit may affect is %s paths.', + $repository->getDisplayName(), + $identifier, + phutil_count($sizes), + new PhutilNumber($limit)); + + throw new DiffusionCommitHookRejectException($message); + } + } + } + + public function getFileSizesForCommit($identifier) { + if (!isset($this->filesizeCache[$identifier])) { + $file_sizes = $this->loadFileSizesForCommit($identifier); + $this->filesizeCache[$identifier] = $file_sizes; + } + + return $this->filesizeCache[$identifier]; + } + + private function loadFileSizesForCommit($identifier) { $repository = $this->getRepository(); return id(new DiffusionLowLevelFilesizeQuery()) diff --git a/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php index 861e1a22fb..b57ed5cf56 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php @@ -38,6 +38,7 @@ final class DiffusionRepositoryLimitsManagementPanel return array( 'filesizeLimit', 'copyTimeLimit', + 'touchLimit', ); } @@ -95,6 +96,16 @@ final class DiffusionRepositoryLimitsManagementPanel $view->addProperty(pht('Clone/Fetch Timeout'), $copy_display); + $touch_limit = $repository->getTouchLimit(); + if ($touch_limit) { + $touch_display = pht('%s Paths', new PhutilNumber($touch_limit)); + } else { + $touch_display = pht('Unlimited'); + $touch_display = phutil_tag('em', array(), $touch_display); + } + + $view->addProperty(pht('Touched Paths Limit'), $touch_display); + return $this->newBox(pht('Limits'), $view); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 83c5b411e4..3b7918cd59 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1926,6 +1926,14 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $this->setDetail('limit.filesize', $limit); } + public function getTouchLimit() { + return $this->getDetail('limit.touch'); + } + + public function setTouchLimit($limit) { + return $this->setDetail('limit.touch', $limit); + } + /** * Retrieve the service URI for the device hosting this repository. * diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index fa08ff0394..cfb5402c2a 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php @@ -25,6 +25,7 @@ final class PhabricatorRepositoryPushLog const CHANGEFLAG_DANGEROUS = 16; const CHANGEFLAG_ENORMOUS = 32; const CHANGEFLAG_OVERSIZED = 64; + const CHANGEFLAG_TOUCHES = 128; const REJECT_ACCEPT = 0; const REJECT_DANGEROUS = 1; @@ -33,6 +34,7 @@ final class PhabricatorRepositoryPushLog const REJECT_BROKEN = 4; const REJECT_ENORMOUS = 5; const REJECT_OVERSIZED = 6; + const REJECT_TOUCHES = 7; protected $repositoryPHID; protected $epoch; @@ -66,6 +68,7 @@ final class PhabricatorRepositoryPushLog self::CHANGEFLAG_DANGEROUS => pht('Dangerous'), self::CHANGEFLAG_ENORMOUS => pht('Enormous'), self::CHANGEFLAG_OVERSIZED => pht('Oversized'), + self::CHANGEFLAG_TOUCHES => pht('Touches Too Many Paths'), ); } @@ -78,6 +81,7 @@ final class PhabricatorRepositoryPushLog self::REJECT_BROKEN => pht('Rejected: Broken'), self::REJECT_ENORMOUS => pht('Rejected: Enormous'), self::REJECT_OVERSIZED => pht('Rejected: Oversized File'), + self::REJECT_TOUCHES => pht('Rejected: Touches Too Many Paths'), ); } diff --git a/src/applications/repository/xaction/PhabricatorRepositoryTouchLimitTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryTouchLimitTransaction.php new file mode 100644 index 0000000000..e3052d0894 --- /dev/null +++ b/src/applications/repository/xaction/PhabricatorRepositoryTouchLimitTransaction.php @@ -0,0 +1,76 @@ +getTouchLimit(); + } + + public function generateNewValue($object, $value) { + if (!strlen($value)) { + return null; + } + + $value = (int)$value; + if (!$value) { + return null; + } + + return $value; + } + + public function applyInternalEffects($object, $value) { + $object->setTouchLimit($value); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + if ($old && $new) { + return pht( + '%s changed the touch limit for this repository from %s paths to '. + '%s paths.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } else if ($new) { + return pht( + '%s set the touch limit for this repository to %s paths.', + $this->renderAuthor(), + $this->renderNewValue()); + } else { + return pht( + '%s removed the touch limit (%s paths) for this repository.', + $this->renderAuthor(), + $this->renderOldValue()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (!strlen($new)) { + continue; + } + + if (!preg_match('/^\d+\z/', $new)) { + $errors[] = $this->newInvalidError( + pht( + 'Unable to parse touch limit, specify a positive number of '. + 'paths.'), + $xaction); + continue; + } + } + + return $errors; + } + +} diff --git a/src/docs/user/userguide/diffusion_managing.diviner b/src/docs/user/userguide/diffusion_managing.diviner index 432b4de13d..a671b0f09e 100644 --- a/src/docs/user/userguide/diffusion_managing.diviner +++ b/src/docs/user/userguide/diffusion_managing.diviner @@ -246,11 +246,38 @@ repository. If a commit creates a larger file (or modifies an existing file so it becomes too large) it will be rejected. This option only applies to hosted repositories. +This limit is primarily intended to make it more difficult to accidentally push +very large files that shouldn't be version controlled (like logs, binaries, +machine learning data, or media assets). Pushing huge datafiles by mistake can +make the repository unwieldy by dramatically increasing how much data must be +transferred over the network to clone it, and simply reverting the changes +doesn't reduce the impact of this kind of mistake. + **Clone/Fetch Timeout**: Configure the internal timeout for creating copies of this repository during operations like intracluster synchronization and Drydock working copy construction. This timeout does not affect external users. +**Touch Limit**: Apply a limit to the maximum number of paths that any commit +may touch. If a commit affects more paths than this limit, it will be rejected. +This option only applies to hosted repositories. Users may work around this +limit by breaking the commit into several smaller commits which each affect +fewer paths. + +This limit is intended to offer a guard rail against users making silly +mistakes that create obviously mistaken changes, like copying an entire +repository into itself and pushing the result. This kind of change can take +some effort to clean up if it becomes part of repository history. + +Note that if you move a file, both the old and new locations count as touched +paths. You should generally configure this limit to be more than twice the +number of files you anticipate any user ever legitimately wanting to move in +a single commit. For example, a limit of `20000` will let users move up to +10,000 files in a single commit, but will reject users mistakenly trying to +push a copy of another repository or a directory with a million logfiles or +whatever other kind of creative nonsense they manage to dream up. + + Branches ========