Skip to content

Commit d2e9aee

Browse files
author
epriestley
committed
Reject dangerous changes in Git repositories by default
Summary: Ref T4189. This adds a per-repository "dangerous changes" flag, which defaults to off. This flag must be enabled to do non-appending branch mutation (delete branches / rewrite history). Test Plan: With flag on and off, performed various safe and dangerous pushes. >>> orbital ~/repos/POEMS $ git push origin :blarp remote: +---------------------------------------------------------------+ remote: | * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * * | remote: +---------------------------------------------------------------+ remote: \ remote: \ ^ /^ remote: \ / \ // \ remote: \ |\___/| / \// .\ remote: \ /V V \__ / // | \ \ *----* remote: / / \/_/ // | \ \ \ | remote: @___@` \/_ // | \ \ \/\ \ remote: 0/0/| \/_ // | \ \ \ \ remote: 0/0/0/0/| \/// | \ \ | | remote: 0/0/0/0/0/_|_ / ( // | \ _\ | / remote: 0/0/0/0/0/0/`/,_ _ _/ ) ; -. | _ _\.-~ / / remote: ,-} _ *-.|.-~-. .~ ~ remote: \ \__/ `/\ / ~-. _ .-~ / remote: \____(Oo) *. } { / remote: ( (--) .----~-.\ \-` .~ remote: //__\\ \ DENIED! ///.----..< \ _ -~ remote: // \\ ///-._ _ _ _ _ _ _{^ - - - - ~ remote: remote: remote: DANGEROUS CHANGE: The change you're attempting to push deletes the branch 'blarp'. remote: Dangerous change protection is enabled for this repository. remote: Edit the repository configuration before making dangerous changes. remote: To ssh://dweller@localhost/diffusion/POEMS/ ! [remote rejected] blarp (pre-receive hook declined) error: failed to push some refs to 'ssh://dweller@localhost/diffusion/POEMS/' Reviewers: btrahan Reviewed By: btrahan CC: aran, chad, richardvanvelzen Maniphest Tasks: T4189 Differential Revision: https://secure.phabricator.com/D7689
1 parent 632e1ce commit d2e9aee

10 files changed

+252
-2
lines changed

‎scripts/repository/commit_hook.php

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,43 @@
8686

8787
$engine->setStdin($stdin);
8888

89-
$err = $engine->execute();
89+
try {
90+
$err = $engine->execute();
91+
} catch (DiffusionCommitHookRejectException $ex) {
92+
$console = PhutilConsole::getConsole();
93+
94+
if (PhabricatorEnv::getEnvConfig('phabricator.serious-business')) {
95+
$preamble = pht('*** PUSH REJECTED BY COMMIT HOOK ***');
96+
} else {
97+
$preamble = pht(<<<EOTXT
98+
+---------------------------------------------------------------+
99+
| * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * * |
100+
+---------------------------------------------------------------+
101+
\
102+
\ ^ /^
103+
\ / \ // \
104+
\ |\___/| / \// .\
105+
\ /V V \__ / // | \ \ *----*
106+
/ / \/_/ // | \ \ \ |
107+
@___@` \/_ // | \ \ \/\ \
108+
0/0/| \/_ // | \ \ \ \
109+
0/0/0/0/| \/// | \ \ | |
110+
0/0/0/0/0/_|_ / ( // | \ _\ | /
111+
0/0/0/0/0/0/`/,_ _ _/ ) ; -. | _ _\.-~ / /
112+
,-} _ *-.|.-~-. .~ ~
113+
\ \__/ `/\ / ~-. _ .-~ /
114+
\____(Oo) *. } { /
115+
( (--) .----~-.\ \-` .~
116+
//__\\\\ \ DENIED! ///.----..< \ _ -~
117+
// \\\\ ///-._ _ _ _ _ _ _{^ - - - - ~
118+
119+
EOTXT
120+
);
121+
}
122+
123+
$console->writeErr("%s\n\n", $preamble);
124+
$console->writeErr("%s\n\n", $ex->getMessage());
125+
$err = 1;
126+
}
90127

91128
exit($err);

‎src/__phutil_library_map__.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,7 @@
475475
'DiffusionCommitController' => 'applications/diffusion/controller/DiffusionCommitController.php',
476476
'DiffusionCommitEditController' => 'applications/diffusion/controller/DiffusionCommitEditController.php',
477477
'DiffusionCommitHookEngine' => 'applications/diffusion/engine/DiffusionCommitHookEngine.php',
478+
'DiffusionCommitHookRejectException' => 'applications/diffusion/exception/DiffusionCommitHookRejectException.php',
478479
'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionCommitParentsQuery.php',
479480
'DiffusionCommitQuery' => 'applications/diffusion/query/DiffusionCommitQuery.php',
480481
'DiffusionCommitTagsController' => 'applications/diffusion/controller/DiffusionCommitTagsController.php',
@@ -532,6 +533,7 @@
532533
'DiffusionRepositoryEditBasicController' => 'applications/diffusion/controller/DiffusionRepositoryEditBasicController.php',
533534
'DiffusionRepositoryEditBranchesController' => 'applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php',
534535
'DiffusionRepositoryEditController' => 'applications/diffusion/controller/DiffusionRepositoryEditController.php',
536+
'DiffusionRepositoryEditDangerousController' => 'applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php',
535537
'DiffusionRepositoryEditDeleteController' => 'applications/diffusion/controller/DiffusionRepositoryEditDeleteController.php',
536538
'DiffusionRepositoryEditEncodingController' => 'applications/diffusion/controller/DiffusionRepositoryEditEncodingController.php',
537539
'DiffusionRepositoryEditHostingController' => 'applications/diffusion/controller/DiffusionRepositoryEditHostingController.php',
@@ -2806,6 +2808,7 @@
28062808
'DiffusionCommitController' => 'DiffusionController',
28072809
'DiffusionCommitEditController' => 'DiffusionController',
28082810
'DiffusionCommitHookEngine' => 'Phobject',
2811+
'DiffusionCommitHookRejectException' => 'Exception',
28092812
'DiffusionCommitParentsQuery' => 'DiffusionQuery',
28102813
'DiffusionCommitQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
28112814
'DiffusionCommitTagsController' => 'DiffusionController',
@@ -2854,6 +2857,7 @@
28542857
'DiffusionRepositoryEditBasicController' => 'DiffusionRepositoryEditController',
28552858
'DiffusionRepositoryEditBranchesController' => 'DiffusionRepositoryEditController',
28562859
'DiffusionRepositoryEditController' => 'DiffusionController',
2860+
'DiffusionRepositoryEditDangerousController' => 'DiffusionRepositoryEditController',
28572861
'DiffusionRepositoryEditDeleteController' => 'DiffusionRepositoryEditController',
28582862
'DiffusionRepositoryEditEncodingController' => 'DiffusionRepositoryEditController',
28592863
'DiffusionRepositoryEditHostingController' => 'DiffusionRepositoryEditController',

‎src/applications/diffusion/application/PhabricatorApplicationDiffusion.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ public function getRoutes() {
7070
'basic/' => 'DiffusionRepositoryEditBasicController',
7171
'encoding/' => 'DiffusionRepositoryEditEncodingController',
7272
'activate/' => 'DiffusionRepositoryEditActivateController',
73+
'dangerous/' => 'DiffusionRepositoryEditDangerousController',
7374
'policy/' => 'DiffusionRepositoryEditPolicyController',
7475
'branches/' => 'DiffusionRepositoryEditBranchesController',
7576
'subversion/' => 'DiffusionRepositoryEditSubversionController',

‎src/applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
<?php
2+
3+
final class DiffusionRepositoryEditDangerousController
4+
extends DiffusionRepositoryEditController {
5+
6+
public function processRequest() {
7+
$request = $this->getRequest();
8+
$viewer = $request->getUser();
9+
$drequest = $this->diffusionRequest;
10+
$repository = $drequest->getRepository();
11+
12+
$repository = id(new PhabricatorRepositoryQuery())
13+
->setViewer($viewer)
14+
->requireCapabilities(
15+
array(
16+
PhabricatorPolicyCapability::CAN_VIEW,
17+
PhabricatorPolicyCapability::CAN_EDIT,
18+
))
19+
->withIDs(array($repository->getID()))
20+
->executeOne();
21+
22+
if (!$repository) {
23+
return new Aphront404Response();
24+
}
25+
26+
if (!$repository->canAllowDangerousChanges()) {
27+
return new Aphront400Response();
28+
}
29+
30+
$edit_uri = $this->getRepositoryControllerURI($repository, 'edit/');
31+
32+
if ($request->isFormPost()) {
33+
$xaction = id(new PhabricatorRepositoryTransaction())
34+
->setTransactionType(PhabricatorRepositoryTransaction::TYPE_DANGEROUS)
35+
->setNewValue(!$repository->shouldAllowDangerousChanges());
36+
37+
$editor = id(new PhabricatorRepositoryEditor())
38+
->setContinueOnNoEffect(true)
39+
->setContentSourceFromRequest($request)
40+
->setActor($viewer)
41+
->applyTransactions($repository, array($xaction));
42+
43+
return id(new AphrontReloadResponse())->setURI($edit_uri);
44+
}
45+
46+
$dialog = id(new AphrontDialogView())
47+
->setUser($viewer);
48+
49+
$force = phutil_tag('tt', array(), '--force');
50+
51+
if ($repository->shouldAllowDangerousChanges()) {
52+
$dialog
53+
->setTitle(pht('Prevent Dangerous changes?'))
54+
->appendChild(
55+
pht(
56+
'It will no longer be possible to delete branches from this '.
57+
'repository, or %s push to this repository.',
58+
$force))
59+
->addSubmitButton(pht('Prevent Dangerous Changes'))
60+
->addCancelButton($edit_uri);
61+
} else {
62+
$dialog
63+
->setTitle(pht('Allow Dangerous Changes?'))
64+
->appendChild(
65+
pht(
66+
'If you allow dangerous changes, it will be possible to delete '.
67+
'branches and %s push this repository. These operations can '.
68+
'alter a repository in a way that is difficult to recover from.',
69+
$force))
70+
->addSubmitButton(pht('Allow Dangerous Changes'))
71+
->addCancelButton($edit_uri);
72+
}
73+
74+
return id(new AphrontDialogResponse())
75+
->setDialog($dialog);
76+
}
77+
78+
}

‎src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,25 @@ private function buildHostingActions(PhabricatorRepository $repository) {
577577
$this->getRepositoryControllerURI($repository, 'edit/hosting/'));
578578
$view->addAction($edit);
579579

580+
if ($repository->canAllowDangerousChanges()) {
581+
if ($repository->shouldAllowDangerousChanges()) {
582+
$changes = id(new PhabricatorActionView())
583+
->setIcon('blame')
584+
->setName(pht('Prevent Dangerous Changes'))
585+
->setHref(
586+
$this->getRepositoryControllerURI($repository, 'edit/dangerous/'))
587+
->setWorkflow(true);
588+
} else {
589+
$changes = id(new PhabricatorActionView())
590+
->setIcon('warning')
591+
->setName(pht('Allow Dangerous Changes'))
592+
->setHref(
593+
$this->getRepositoryControllerURI($repository, 'edit/dangerous/'))
594+
->setWorkflow(true);
595+
}
596+
$view->addAction($changes);
597+
}
598+
580599
return $view;
581600
}
582601

@@ -611,6 +630,18 @@ private function buildHostingProperties(
611630
PhabricatorRepository::getProtocolAvailabilityName(
612631
$repository->getServeOverSSH())));
613632

633+
if ($repository->canAllowDangerousChanges()) {
634+
if ($repository->shouldAllowDangerousChanges()) {
635+
$description = pht('Allowed');
636+
} else {
637+
$description = pht('Not Allowed');
638+
}
639+
640+
$view->addProperty(
641+
pht('Dangerous Changes'),
642+
$description);
643+
}
644+
614645
return $view;
615646
}
616647

‎src/applications/diffusion/engine/DiffusionCommitHookEngine.php

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ public function execute() {
6767
private function executeGitHook() {
6868
$updates = $this->parseGitUpdates($this->getStdin());
6969

70+
$this->rejectGitDangerousChanges($updates);
71+
7072
// TODO: Do cheap checks: non-ff commits, mutating refs without access,
7173
// creating or deleting things you can't touch. We can do all non-content
7274
// checks here.
@@ -101,8 +103,10 @@ private function parseGitUpdates($stdin) {
101103

102104
if (preg_match('(^refs/heads/)', $update['ref'])) {
103105
$update['type'] = 'branch';
106+
$update['ref.short'] = substr($update['ref'], strlen('refs/heads/'));
104107
} else if (preg_match('(^refs/tags/)', $update['ref'])) {
105108
$update['type'] = 'tag';
109+
$update['ref.short'] = substr($update['ref'], strlen('refs/tags/'));
106110
} else {
107111
$update['type'] = 'unknown';
108112
}
@@ -159,7 +163,7 @@ private function findGitMergeBases(array $updates) {
159163
private function findGitNewCommits(array $updates) {
160164
$futures = array();
161165
foreach ($updates as $key => $update) {
162-
if ($update['type'] == 'delete') {
166+
if ($update['operation'] == 'delete') {
163167
// Deleting a branch or tag can never create any new commits.
164168
continue;
165169
}
@@ -183,6 +187,61 @@ private function findGitNewCommits(array $updates) {
183187
return $updates;
184188
}
185189

190+
private function rejectGitDangerousChanges(array $updates) {
191+
$repository = $this->getRepository();
192+
if ($repository->shouldAllowDangerousChanges()) {
193+
return;
194+
}
195+
196+
foreach ($updates as $update) {
197+
if ($update['type'] != 'branch') {
198+
// For now, we don't consider deleting or moving tags to be a
199+
// "dangerous" update. It's way harder to get wrong and should be easy
200+
// to recover from once we have better logging.
201+
continue;
202+
}
203+
204+
if ($update['operation'] == 'create') {
205+
// Creating a branch is never dangerous.
206+
continue;
207+
}
208+
209+
if ($update['operation'] == 'change') {
210+
if ($update['old'] == $update['merge-base']) {
211+
// This is a fast-forward update to an existing branch.
212+
// These are safe.
213+
continue;
214+
}
215+
}
216+
217+
// We either have a branch deletion or a non fast-forward branch update.
218+
// Format a message and reject the push.
219+
220+
if ($update['operation'] == 'delete') {
221+
$message = pht(
222+
"DANGEROUS CHANGE: The change you're attempting to push deletes ".
223+
"the branch '%s'.",
224+
$update['ref.short']);
225+
} else {
226+
$message = pht(
227+
"DANGEROUS CHANGE: The change you're attempting to push updates ".
228+
"the branch '%s' from '%s' to '%s', but this is not a fast-forward. ".
229+
"Pushes which rewrite published branch history are dangerous.",
230+
$update['ref.short'],
231+
$update['old.short'],
232+
$update['new.short']);
233+
}
234+
235+
$boilerplate = pht(
236+
"Dangerous change protection is enabled for this repository.\n".
237+
"Edit the repository configuration before making dangerous changes.");
238+
239+
$message = $message."\n".$boilerplate;
240+
241+
throw new DiffusionCommitHookRejectException($message);
242+
}
243+
}
244+
186245
private function executeSubversionHook() {
187246

188247
// TODO: Do useful things here, too.

‎src/applications/diffusion/exception/DiffusionCommitHookRejectException.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
3+
final class DiffusionCommitHookRejectException extends Exception {
4+
5+
}

‎src/applications/repository/editor/PhabricatorRepositoryEditor.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ public function getTransactionTypes() {
3030
$types[] = PhabricatorRepositoryTransaction::TYPE_PROTOCOL_SSH;
3131
$types[] = PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY;
3232
$types[] = PhabricatorRepositoryTransaction::TYPE_CREDENTIAL;
33+
$types[] = PhabricatorRepositoryTransaction::TYPE_DANGEROUS;
3334

3435
$types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
3536
$types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
@@ -80,6 +81,8 @@ protected function getCustomTransactionOldValue(
8081
return $object->getPushPolicy();
8182
case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
8283
return $object->getCredentialPHID();
84+
case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
85+
return $object->shouldAllowDangerousChanges();
8386
}
8487
}
8588

@@ -110,6 +113,7 @@ protected function getCustomTransactionNewValue(
110113
case PhabricatorRepositoryTransaction::TYPE_PROTOCOL_SSH:
111114
case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY:
112115
case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
116+
case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
113117
return $xaction->getNewValue();
114118
case PhabricatorRepositoryTransaction::TYPE_NOTIFY:
115119
case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE:
@@ -175,6 +179,9 @@ protected function applyCustomInternalTransaction(
175179
return $object->setPushPolicy($xaction->getNewValue());
176180
case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
177181
return $object->setCredentialPHID($xaction->getNewValue());
182+
case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
183+
$object->setDetail('allow-dangerous-changes', $xaction->getNewValue());
184+
return;
178185
case PhabricatorRepositoryTransaction::TYPE_ENCODING:
179186
// Make sure the encoding is valid by converting to UTF-8. This tests
180187
// that the user has mbstring installed, and also that they didn't type
@@ -285,6 +292,7 @@ protected function requireCapabilities(
285292
case PhabricatorRepositoryTransaction::TYPE_PROTOCOL_SSH:
286293
case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY:
287294
case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL:
295+
case PhabricatorRepositoryTransaction::TYPE_DANGEROUS:
288296
PhabricatorPolicyFilter::requireCapability(
289297
$this->requireActor(),
290298
$object,

‎src/applications/repository/storage/PhabricatorRepository.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,22 @@ public function canMirror() {
911911
return false;
912912
}
913913

914+
public function canAllowDangerousChanges() {
915+
if (!$this->isHosted()) {
916+
return false;
917+
}
918+
919+
if ($this->isGit()) {
920+
return true;
921+
}
922+
923+
return false;
924+
}
925+
926+
public function shouldAllowDangerousChanges() {
927+
return (bool)$this->getDetail('allow-dangerous-changes');
928+
}
929+
914930
public function writeStatusMessage(
915931
$status_type,
916932
$status_code,

‎src/applications/repository/storage/PhabricatorRepositoryTransaction.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ final class PhabricatorRepositoryTransaction
2222
const TYPE_PROTOCOL_SSH = 'repo:serve-ssh';
2323
const TYPE_PUSH_POLICY = 'repo:push-policy';
2424
const TYPE_CREDENTIAL = 'repo:credential';
25+
const TYPE_DANGEROUS = 'repo:dangerous';
2526

2627
// TODO: Clean up these legacy transaction types.
2728
const TYPE_SSH_LOGIN = 'repo:ssh-login';
@@ -338,6 +339,16 @@ public function getTitle() {
338339
$this->renderHandleLink($author_phid),
339340
$this->renderPolicyName($old),
340341
$this->renderPolicyName($new));
342+
case self::TYPE_DANGEROUS:
343+
if ($new) {
344+
return pht(
345+
'%s disabled protection against dangerous changes.',
346+
$this->renderHandleLink($author_phid));
347+
} else {
348+
return pht(
349+
'%s enabled protection against dangerous changes.',
350+
$this->renderHandleLink($author_phid));
351+
}
341352
}
342353

343354
return parent::getTitle();

0 commit comments

Comments
 (0)