From 4669eab596c8d90de0cf9f9d359ad8dd8f48edb5 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 23 Oct 2008 16:29:50 -0400 Subject: [PATCH] more work on untrusted committers Wired up check_canedit and check_canremove, still need to deal with check_canattach, and test. --- IkiWiki.pm | 4 +- IkiWiki/Plugin/editpage.pm | 2 +- IkiWiki/Plugin/git.pm | 70 ++++++++++++------------- IkiWiki/Plugin/remove.pm | 2 +- IkiWiki/Receive.pm | 101 +++++++++++++++++++++++++++++++++++++ doc/plugins/write.mdwn | 24 ++++++--- doc/rcs/git.mdwn | 2 +- ikiwiki.in | 10 +++- 8 files changed, 168 insertions(+), 47 deletions(-) create mode 100644 IkiWiki/Receive.pm diff --git a/IkiWiki.pm b/IkiWiki.pm index 245eaafba..698244187 100644 --- a/IkiWiki.pm +++ b/IkiWiki.pm @@ -1582,8 +1582,8 @@ sub rcs_getctime ($) { #{{{ $hooks{rcs}{rcs_getctime}{call}->(@_); } #}}} -sub rcs_test_receive ($) { #{{{ - $hooks{rcs}{rcs_test_receive}{call}->(@_); +sub rcs_receive ($) { #{{{ + $hooks{rcs}{rcs_receive}{call}->(@_); } #}}} sub globlist_to_pagespec ($) { #{{{ diff --git a/IkiWiki/Plugin/editpage.pm b/IkiWiki/Plugin/editpage.pm index 30c93df20..fe2864bac 100644 --- a/IkiWiki/Plugin/editpage.pm +++ b/IkiWiki/Plugin/editpage.pm @@ -122,7 +122,7 @@ sub cgi_editpage ($$) { #{{{ my $absolute=($page =~ s#^/+##); if (! defined $page || ! length $page || file_pruned($page, $config{srcdir})) { - error("bad page name"); + error(gettext("bad page name")); } my $baseurl = urlto($page, undef, 1); diff --git a/IkiWiki/Plugin/git.pm b/IkiWiki/Plugin/git.pm index 1facb14c0..234e7af2e 100644 --- a/IkiWiki/Plugin/git.pm +++ b/IkiWiki/Plugin/git.pm @@ -23,7 +23,7 @@ sub import { #{{{ hook(type => "rcs", id => "rcs_recentchanges", call => \&rcs_recentchanges); hook(type => "rcs", id => "rcs_diff", call => \&rcs_diff); hook(type => "rcs", id => "rcs_getctime", call => \&rcs_getctime); - hook(type => "rcs", id => "rcs_test_receive", call => \&rcs_test_receive); + hook(type => "rcs", id => "rcs_receive", call => \&rcs_receive); } #}}} sub checkconfig () { #{{{ @@ -77,7 +77,7 @@ sub getsetup () { #{{{ safe => 0, # file rebuild => 0, }, - git_untrusted_committers => { + untrusted_committers => { type => "string", example => [], description => "unix users whose commits should be checked by the pre-receive hook", @@ -588,15 +588,7 @@ sub rcs_getctime ($) { #{{{ return $ctime; } #}}} -sub rcs_test_receive () { #{{{ - # quick success if the user is trusted - my $committer=(getpwuid($<))[0]; - if (! defined $committer) { - error("cannot determine username for $<"); - } - exit 0 if ! ref $config{git_untrusted_committers} || - ! grep { $_ eq $committer } @{$config{git_untrusted_committers}}; - +sub rcs_receive () { #{{{ # The wiki may not be the only thing in the git repo. # Determine if it is in a subdirectory by examining the srcdir, # and its parents, looking for the .git directory. @@ -610,54 +602,64 @@ sub rcs_test_receive () { #{{{ } } - my @errors; + my @rets; while (<>) { chomp; my ($oldrev, $newrev, $refname) = split(' ', $_, 3); # only allow changes to gitmaster_branch if ($refname !~ /^refs\/heads\/\Q$config{gitmaster_branch}\E$/) { - push @errors, sprintf(gettext("you are not allowed to change %s"), $refname); + error sprintf(gettext("you are not allowed to change %s"), $refname); } foreach my $ci (git_commit_info($oldrev."..".$newrev)) { foreach my $detail (@{ $ci->{'details'} }) { my $file = $detail->{'file'}; - # check that all changed files are in the subdir + # check that all changed files are in the + # subdir if (length $subdir && ! ($file =~ s/^\Q$subdir\E//)) { - push @errors, sprintf(gettext("you are not allowed to change %s"), $file); - next; + error sprintf(gettext("you are not allowed to change %s"), $file); } - if ($detail->{'mode_from'} ne $detail->{'mode_to'}) { - push @errors, gettext("you are not allowed to change file modes"); + my $action; + my $mode; + if ($detail->{'status'} =~ /^[M]+\d*$/) { + $action="change"; + $mode=$detail->{'mode_to'}; } - - if ($detail->{'status'} =~ /^D+\d*/) { - # TODO check_canremove + elsif ($detail->{'status'} =~ /^[AM]+\d*$/) { + $action="add"; + $mode=$detail->{'mode_to'}; } - elsif ($detail->{'status'} !~ /^[MA]+\d*$/) { - push @errors, "unknown status ".$detail->{'status'}; + elsif ($detail->{'status'} =~ /^[DAM]+\d*/) { + $action="remove"; + $mode=$detail->{'mode_from'}; } else { - # TODO check_canedit - # TODO check_canattach + error "unknown status ".$detail->{'status'}; } + + # test that the file mode is ok + if ($mode !~ /^100[64][64][64]$/) { + error sprintf(gettext("you cannot act on a file with mode %s"), $mode); + } + if ($action eq "change") { + if ($detail->{'mode_from'} ne $detail->{'mode_to'}) { + error gettext("you are not allowed to change file modes"); + } + } + + push @rets, { + file => $file, + action => $action, + }; } } } - if (@errors) { - # TODO clean up objects from failed push - - print STDERR "$_\n" foreach @errors; - exit 1; - } - else { - exit 0; - } + return @rets; } #}}} 1 diff --git a/IkiWiki/Plugin/remove.pm b/IkiWiki/Plugin/remove.pm index 68bf9d1ee..c512b3b97 100644 --- a/IkiWiki/Plugin/remove.pm +++ b/IkiWiki/Plugin/remove.pm @@ -41,7 +41,7 @@ sub check_canremove ($$$) { #{{{ error(sprintf(gettext("%s is not a file"), $file)); } - # Must be editiable. + # Must be editable. IkiWiki::check_canedit($page, $q, $session); # If a user can't upload an attachment, don't let them delete it. diff --git a/IkiWiki/Receive.pm b/IkiWiki/Receive.pm new file mode 100644 index 000000000..63944bb81 --- /dev/null +++ b/IkiWiki/Receive.pm @@ -0,0 +1,101 @@ +#!/usr/bin/perl + +package IkiWiki::Receive; + +use warnings; +use strict; +use IkiWiki; + +sub getuser () { #{{{ + my $user=(getpwuid($<))[0]; + if (! defined $user) { + error("cannot determine username for $<"); + } + return $user; +} #}}} + +sub trusted () { #{{{ + my $user=getuser(); + return ! ref $config{untrusted_committers} || + ! grep { $_ eq $user } @{$config{untrusted_committers}}; +} #}}} + +sub test () { #{{{ + exit 0 if trusted(); + IkiWiki::rcs_test_receive(); + + # Dummy up a cgi environment to use when calling check_canedit + # and friends. + eval q{use CGI}; + error($@) if $@; + my $cgi=CGI->new; + require IkiWiki::CGI; + my $session=IkiWiki::cgi_getsession($cgi); + my $user=getuser(); + $session->param("name", $user); + $ENV{REMOTE_ADDR}='unknown' unless exists $ENV{REMOTE_ADDR}; + + lockwiki(); + loadindex(); + + my %newfiles; + + foreach my $change (IkiWiki::rcs_receive()) { + # This untaint is safe because we check file_pruned and + # wiki_file_regexp. + my $file=$change->{file}=~/$config{wiki_file_regexp}/; + $file=possibly_foolish_untaint($file); + if (! defined $file || ! length $file || + IkiWiki::file_pruned($file, $config{srcdir})) { + error(gettext("bad file name")); + } + + my $type=pagetype($file); + my $page=pagename($file) if defined $type; + + if ($change->{action} eq 'add') { + $newfiles{$file}=1; + } + + if ($change->{action} eq 'change' || + $change->{action} eq 'add') { + if (defined $page) { + if (IkiWiki->can("check_canedit") && + IkiWiki::check_canedit($page, $cgi, $session)) { + next; + } + } + else { + # TODO + #if (IkiWiki::Plugin::attachment->can("check_canattach") && + # IkiWiki::Plugin::attachment::check_canattach($session, $file, $path)) { + # next; + #} + } + } + elsif ($change->{action} eq 'remove') { + # check_canremove tests to see if the file is present + # on disk. This will fail is a single commit adds a + # file and then removes it again. Avoid the problem + # by not testing the removal in such pairs of changes. + # (The add is still tested, just to make sure that + # no data is added to the repo that a web edit + # could add.) + next if $newfiles{$file}; + + if (IkiWiki::Plugin::remove->can("check_canremove") && + IkiWiki::Plugin::remove::check_canremove(defined $page ? $page : $file, $cgi, $session)) { + next; + } + } + else { + error "unknown action ".$change->{action}; + } + + error sprintf(gettext("you are not allowed to change %s"), $file); + } + + exit 0; +} #}}} + +1 diff --git a/doc/plugins/write.mdwn b/doc/plugins/write.mdwn index 5a5db6be0..9f096e4f7 100644 --- a/doc/plugins/write.mdwn +++ b/doc/plugins/write.mdwn @@ -820,14 +820,26 @@ it up in the history. It's ok if this is not implemented, and throws an error. -#### `rcs_test_receive()` +#### `rcs_receive()` -This is used to test if changes pushed into the RCS should be accepted. -Ikiwiki will be running as a pre-receive hook (or equivilant) and should -examine the incoming changes, decide if they are allowed, and communicate -that to the RCS. +This is called when ikiwiki is running as a pre-receive hook (or +equivilant), and is testing if changes pushed into the RCS from an +untrusted user should be accepted. This is optional, and doesn't make +sense to implement for all RCSs. -This is optional, and doesn't make sense for all RCSs. +It should examine the incoming changes, and do any sanity +checks that are appropriate for the RCS to limit changes to safe file adds, +removes, and renames. If something bad is found, it should exit +nonzero, to abort the push. Otherwise, it should return a list of +files that were changed, in the form: + + { + file => # name of file that was changed + action => # either "add", "change", or "remove" + } + +The list will then be checked to make sure that each change is one that +is allowed to be made via the web interface. ### PageSpec plugins diff --git a/doc/rcs/git.mdwn b/doc/rcs/git.mdwn index 2a6feecf5..6ba0da894 100644 --- a/doc/rcs/git.mdwn +++ b/doc/rcs/git.mdwn @@ -116,7 +116,7 @@ committers. Trusted committers, including the user that ikiwiki runs as, will not have their commits checked by the `pre-receive` hook. Untrusted committers will have their commits checked. The configuration settings to enable are `git_test_receive_wrapper`, which enables generation of a -`pre-receive` hook, and `git_untrusted_committers`, which is a list of +`pre-receive` hook, and `untrusted_committers`, which is a list of usernames of the untrusted committers. Note that when the `pre-receive` hook is checking incoming changes, it diff --git a/ikiwiki.in b/ikiwiki.in index 22addb463..60663bc89 100755 --- a/ikiwiki.in +++ b/ikiwiki.in @@ -119,10 +119,15 @@ sub getconfig () { #{{{ } delete $ENV{WRAPPED_OPTIONS}; - # optimisation for no-op post_commit if ($config{post_commit} && ! commit_hook_enabled()) { + # optimisation for no-op post_commit exit 0; } + elsif ($config{test_receive}) { + # quick success if the user is trusted + require IkiWiki::Receive; + exit 0 if IkiWiki::Receive::trusted(); + } loadplugins(); checkconfig(); @@ -190,7 +195,8 @@ sub main () { #{{{ # do nothing } elsif ($config{test_receive}) { - rcs_test_receive(); + require IkiWiki::Receive; + IkiWiki::Receive::test(); } else { if ($config{rebuild}) { -- 2.26.2