From 6fe3c6bb5f34dca6fc23ba910a9d9a583da9f4c9 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 2 Sep 2009 16:10:57 -0400 Subject: ruther review --- doc/plugins/contrib/cvs/discussion.mdwn | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'doc/plugins/contrib/cvs') diff --git a/doc/plugins/contrib/cvs/discussion.mdwn b/doc/plugins/contrib/cvs/discussion.mdwn index e142452d0..b5f03ac6f 100644 --- a/doc/plugins/contrib/cvs/discussion.mdwn +++ b/doc/plugins/contrib/cvs/discussion.mdwn @@ -93,3 +93,32 @@ the "cvs add " call and avoid doing anything in that case? >>> if a configured post-commit hook is missing, and it seems fine, >>> probably also thanks to IPC::Cmd. >>> --[[schmonz]] + +---- + + +Further review.. --[[Joey]] + +I don't understand what `cvs_shquote_commit` is +trying to do with the test message, but it seems +highly likely to be insecure; I never trust anything +that relies on safely quoting user input passed to the shell. + +(As an aside, `shell_quote` can die on certian inputs.) + +Seems to me that, if `IPC::Cmd` exposes input to the shell +(which I have not verified but its docs don't specify; a bad sign) +you chose the wrong tool and ended up doing down the wrong +route, dragging in shell quoting problems and fixes. Since you +chose to use `IPC::Cmd` just because you wanted to shut +up CVS stderr, my suggestion would be to use plain `system` +to run the command, with stderr temporarily sent to /dev/null: + + open(my $savederr, ">&STDERR"); + open(STDERR, ">", "/dev/null"); + my $ret=system("cvs", "-Q", @_); + open(STDERR, ">$savederr"); + +`cvs_runcvs` should not take an array reference. It's +usual for this type of function to take a list of parameters +to pass to the command. -- cgit v1.2.3 From 127ed4d6c7a7d2cc518102757b7b4be49d20fce8 Mon Sep 17 00:00:00 2001 From: Amitai Schlair Date: Fri, 4 Sep 2009 15:08:48 -0500 Subject: no more IPC::Cmd or String::ShellQuote, huzzah --- doc/plugins/contrib/cvs.mdwn | 4 ++-- doc/plugins/contrib/cvs/discussion.mdwn | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) (limited to 'doc/plugins/contrib/cvs') diff --git a/doc/plugins/contrib/cvs.mdwn b/doc/plugins/contrib/cvs.mdwn index 1c43fb272..fcf5b936f 100644 --- a/doc/plugins/contrib/cvs.mdwn +++ b/doc/plugins/contrib/cvs.mdwn @@ -5,8 +5,8 @@ This plugin allows ikiwiki to use [[!wikipedia desc="CVS" Concurrent Versions System]] as an [[rcs]]. ### Usage -7. Install [cvsps](http://www.cobite.com/cvsps/), [[!cpan IPC::Cmd]], -[[!cpan String::ShellQuote]], [[!cpan File::ReadBackwards]], and +7. Install [cvsps](http://www.cobite.com/cvsps/), [[!cpan +File::ReadBackwards]], and [cvsweb](http://www.freebsd.org/projects/cvsweb.html) or the like. 7. Adjust CVS-related parameters in your setup file. diff --git a/doc/plugins/contrib/cvs/discussion.mdwn b/doc/plugins/contrib/cvs/discussion.mdwn index b5f03ac6f..1f0ce0102 100644 --- a/doc/plugins/contrib/cvs/discussion.mdwn +++ b/doc/plugins/contrib/cvs/discussion.mdwn @@ -122,3 +122,6 @@ to run the command, with stderr temporarily sent to /dev/null: `cvs_runcvs` should not take an array reference. It's usual for this type of function to take a list of parameters to pass to the command. + +> Thanks for reading carefully. I've tested your suggestions and +> applied them on my branch. --[[schmonz]] -- cgit v1.2.3