aboutsummaryrefslogtreecommitdiff
path: root/IkiWiki
diff options
context:
space:
mode:
authorSimon McVittie <smcv@debian.org>2016-12-19 13:48:56 +0000
committerSimon McVittie <smcv@debian.org>2016-12-28 21:32:12 +0000
commita8a7462382ff235086743f06a92a9ab9100083b4 (patch)
tree0d69d59b5c84950aa17f8ca08df4bc5ba4f1118c /IkiWiki
parent469c842fd56ce811d431058714d9c2700a5314f8 (diff)
downloadikiwiki-a8a7462382ff235086743f06a92a9ab9100083b4.tar
ikiwiki-a8a7462382ff235086743f06a92a9ab9100083b4.tar.gz
Try revert operations (on a branch) before approving them
Otherwise, we have a time-of-check/time-of-use vulnerability: rcs_preprevert previously looked at what changed in the commit we are reverting, not at what would result from reverting it now. In particular, if some files were renamed since the commit we are reverting, a revert of changes that were within the designated subdirectory and allowed by check_canchange() might now affect files that are outside the designated subdirectory or disallowed by check_canchange(). It is not sufficient to disable rename detection, since git older than 2.8.0rc0 (in particular the version in Debian stable) silently accepts and ignores the relevant options. OVE-20161226-0002
Diffstat (limited to 'IkiWiki')
-rw-r--r--IkiWiki/Plugin/git.pm85
1 files changed, 80 insertions, 5 deletions
diff --git a/IkiWiki/Plugin/git.pm b/IkiWiki/Plugin/git.pm
index 64a47c8e8..56d649372 100644
--- a/IkiWiki/Plugin/git.pm
+++ b/IkiWiki/Plugin/git.pm
@@ -425,6 +425,16 @@ sub parse_diff_tree ($) {
}
shift @{ $dt_ref } if $dt_ref->[0] =~ /^$/;
+ $ci{details} = [parse_changed_files($dt_ref)];
+
+ return \%ci;
+}
+
+sub parse_changed_files {
+ my $dt_ref = shift;
+
+ my @files;
+
# Modified files.
while (my $line = shift @{ $dt_ref }) {
if ($line =~ m{^
@@ -442,7 +452,7 @@ sub parse_diff_tree ($) {
my $status = shift(@tmp);
if (length $file) {
- push @{ $ci{'details'} }, {
+ push @files, {
'file' => decode_git_file($file),
'sha1_from' => $sha1_from[0],
'sha1_to' => $sha1_to,
@@ -456,7 +466,7 @@ sub parse_diff_tree ($) {
last;
}
- return \%ci;
+ return @files;
}
sub git_commit_info ($;$) {
@@ -955,10 +965,14 @@ sub rcs_preprevert ($) {
my $rev=shift;
my ($sha1) = $rev =~ /^($sha1_pattern)$/; # untaint
+ my @undo; # undo stack for cleanup in case of an error
+
+ ensure_committer();
+
# Examine changes from root of git repo, not from any subdir,
# in order to see all changes.
my ($subdir, $rootdir) = git_find_root();
- in_git_dir($rootdir, sub {
+ return in_git_dir($rootdir, sub {
my @commits=git_commit_info($sha1, 1);
if (! @commits) {
@@ -971,7 +985,68 @@ sub rcs_preprevert ($) {
error gettext("you are not allowed to revert a merge");
}
+ # Due to the presence of rename-detection, we cannot actually
+ # see what will happen in a revert without trying it.
+ # But we can guess, which is enough to rule out most changes
+ # that we won't allow reverting.
git_parse_changes(1, @commits);
+
+ my $failure;
+ my @ret;
+ # If it looks OK, do it for real, on a branch.
+ eval {
+ IkiWiki::disable_commit_hook();
+ push @undo, sub {
+ IkiWiki::enable_commit_hook();
+ };
+ my $branch = "ikiwiki_revert_${sha1}"; # supposed to be unique
+
+ push @undo, sub {
+ run_or_cry('git', 'branch', '-D', $branch) if $failure;
+ };
+ if (run_or_non('git', 'rev-parse', '--quiet', '--verify', $branch)) {
+ run_or_non('git', 'branch', '-D', $branch);
+ }
+ run_or_die('git', 'branch', $branch, $config{gitmaster_branch});
+
+ push @undo, sub {
+ if (!run_or_cry('git', 'checkout', '--quiet', $config{gitmaster_branch})) {
+ run_or_cry('git', 'checkout','-f', '--quiet', $config{gitmaster_branch});
+ }
+ };
+ run_or_die('git', 'checkout', '--quiet', $branch);
+
+ run_or_die('git', 'revert', '--no-commit', $sha1);
+ run_or_non('git', 'commit', '-m', "revert $sha1", '-a');
+
+ # Re-switch to master.
+ run_or_die('git', 'checkout', '--quiet', $config{gitmaster_branch});
+
+ my @raw_lines;
+ @raw_lines = run_or_die('git', 'diff', '--pretty=raw',
+ '--raw', '--abbrev=40', '--always', '--no-renames',
+ "ikiwiki_revert_${sha1}..");
+
+ my $ci = {
+ details => [parse_changed_files(\@raw_lines)],
+ };
+
+ @ret = git_parse_changes(0, $ci);
+ };
+ $failure = $@;
+
+ # Process undo stack (in reverse order). By policy cleanup
+ # actions should normally print a warning on failure.
+ while (my $handle = pop @undo) {
+ $handle->();
+ }
+
+ if ($failure) {
+ my $message = sprintf(gettext("Failed to revert commit %s"), $sha1);
+ error("$message\n$failure\n");
+ }
+
+ return @ret;
});
}
@@ -982,11 +1057,11 @@ sub rcs_revert ($) {
ensure_committer();
- if (run_or_non('git', 'revert', '--no-commit', $sha1)) {
+ if (run_or_non('git', 'merge', '--ff-only', "ikiwiki_revert_$sha1")) {
return undef;
}
else {
- run_or_die('git', 'reset', '--hard');
+ run_or_non('git', 'branch', '-D', "ikiwiki_revert_$sha1");
return sprintf(gettext("Failed to revert commit %s"), $sha1);
}
}