diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php index 4f6997c52a..44c772225c 100755 --- a/scripts/repository/commit_hook.php +++ b/scripts/repository/commit_hook.php @@ -54,10 +54,62 @@ if (!$repository->isHosted()) { $engine->setRepository($repository); +$args = new PhutilArgumentParser($argv); +$args->parsePartial( + array( + array( + 'name' => 'hook-mode', + 'param' => 'mode', + 'help' => pht('Hook execution mode.'), + ), + )); + +$argv = array_merge( + array($argv[0]), + $args->getUnconsumedArgumentVector()); // Figure out which user is writing the commit. +$hook_mode = $args->getArg('hook-mode'); +if ($hook_mode !== null) { + $known_modes = array( + 'svn-revprop' => true, + ); -if ($repository->isGit() || $repository->isHg()) { + if (empty($known_modes[$hook_mode])) { + throw new Exception( + pht( + 'Invalid Hook Mode: This hook was invoked in "%s" mode, but this '. + 'is not a recognized hook mode. Valid modes are: %s.', + $hook_mode, + implode(', ', array_keys($known_modes)))); + } +} + +$is_svnrevprop = ($hook_mode == 'svn-revprop'); + +if ($is_svnrevprop) { + // For now, we let these through if the repository allows dangerous changes + // and prevent them if it doesn't. See T11208 for discussion. + + $revprop_key = $argv[5]; + + if ($repository->shouldAllowDangerousChanges()) { + $err = 0; + } else { + $err = 1; + + $console = PhutilConsole::getConsole(); + $console->writeErr( + pht( + "DANGEROUS CHANGE: Dangerous change protection is enabled for this ". + "repository, so you can not change revision properties (you are ". + "attempting to edit \"%s\").\n". + "Edit the repository configuration before making dangerous changes.", + $revprop_key)); + } + + exit($err); +} else if ($repository->isGit() || $repository->isHg()) { $username = getenv(DiffusionCommitHookEngine::ENV_USER); if (!strlen($username)) { throw new Exception( diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php index d0dd2cc2e9..1088733cfc 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php @@ -18,25 +18,14 @@ final class DiffusionRepositoryEditDangerousController ->getPanelURI(); if (!$repository->canAllowDangerousChanges()) { - if ($repository->isSVN()) { - return $this->newDialog() - ->setTitle(pht('Not in Danger')) - ->appendParagraph( - pht( - 'It is not possible for users to push any dangerous changes '. - 'to a Subversion repository. Pushes to a Subversion repository '. - 'can always be reverted and never destroy data.')) - ->addCancelButton($panel_uri); - } else { - return $this->newDialog() - ->setTitle(pht('Unprotectable Repository')) - ->appendParagraph( - pht( - 'This repository can not be protected from dangerous changes '. - 'because Phabricator does not control what users are allowed '. - 'to push to it.')) - ->addCancelButton($panel_uri); - } + return $this->newDialog() + ->setTitle(pht('Unprotectable Repository')) + ->appendParagraph( + pht( + 'This repository can not be protected from dangerous changes '. + 'because Phabricator does not control what users are allowed '. + 'to push to it.')) + ->addCancelButton($panel_uri); } if ($request->isFormPost()) { @@ -57,18 +46,34 @@ final class DiffusionRepositoryEditDangerousController if ($repository->shouldAllowDangerousChanges()) { $title = pht('Prevent Dangerous Changes'); - $body = pht( - 'It will no longer be possible to delete branches from this '. - 'repository, or %s push to this repository.', - $force); + + if ($repository->isSVN()) { + $body = pht( + 'It will no longer be possible to edit revprops in this '. + 'repository.'); + } else { + $body = pht( + 'It will no longer be possible to delete branches from this '. + 'repository, or %s push to this repository.', + $force); + } + $submit = pht('Prevent Dangerous Changes'); } else { $title = pht('Allow Dangerous Changes'); - $body = pht( - 'If you allow dangerous changes, it will be possible to delete '. - 'branches and %s push this repository. These operations can '. - 'alter a repository in a way that is difficult to recover from.', - $force); + if ($repository->isSVN()) { + $body = pht( + 'If you allow dangerous changes, it will be possible to edit '. + 'reprops in this repository, including arbitrarily rewriting '. + 'commit messages. These operations can alter a repository in a '. + 'way that is difficult to recover from.'); + } else { + $body = pht( + 'If you allow dangerous changes, it will be possible to delete '. + 'branches and %s push this repository. These operations can '. + 'alter a repository in a way that is difficult to recover from.', + $force); + } $submit = pht('Allow Dangerous Changes'); } diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 7e54bf4620..08fe8ea224 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -191,7 +191,7 @@ final class PhabricatorRepositoryPullEngine )); } - private function installHook($path) { + private function installHook($path, array $hook_argv = array()) { $this->log('%s', pht('Installing commit hook to "%s"...', $path)); $repository = $this->getRepository(); @@ -202,10 +202,11 @@ final class PhabricatorRepositoryPullEngine $full_php_path = Filesystem::resolveBinary('php'); $cmd = csprintf( - 'exec %s -f %s -- %s "$@"', + 'exec %s -f %s -- %s %Ls "$@"', $full_php_path, $bin, - $identifier); + $identifier, + $hook_argv); $hook = "#!/bin/sh\nexport TERM=dumb\n{$cmd}\n"; @@ -585,8 +586,16 @@ final class PhabricatorRepositoryPullEngine $root = $repository->getLocalPath(); $path = '/hooks/pre-commit'; - $this->installHook($root.$path); + + $revprop_path = '/hooks/pre-revprop-change'; + + $revprop_argv = array( + '--hook-mode', + 'svn-revprop', + ); + + $this->installHook($root.$revprop_path, $revprop_argv); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index d0f3f9f4d8..4a7e3d4681 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1623,11 +1623,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return false; } - if ($this->isGit() || $this->isHg()) { - return true; - } + // In Git and Mercurial, ref deletions and rewrites are dangerous. + // In Subversion, editing revprops is dangerous. - return false; + return true; } public function shouldAllowDangerousChanges() {