[took Tommi out of cc; he must be getting enough mail as is...]
On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@xxxxxxxxx> wrote:
> A few comments about the code, taking gl-auth-command as example.
> Wouldn't it be better to use "use warnings" instead of 'perl -w'?
I'm not sure what is the minimum perl required for git
itself. Has it needed perl > 5.6.0 for more than a year at
least? The only real difference between these two is scope,
which is a non-issue here, so I played safe.
> It would be, I think, better if you have used POD for such
> documentation. One would be able to generate manpage using pod2man,
> and it is no less readable in source code. See e.g. perl/Git.pm or
> contrib/hooks/update-paranoid.
Hmm... I've been spoiled by Markdown's sane bullet list
handling. Visually, POD forces everything other than code
to be flush left -- any sort of list is definitely less
readable in source code as a result. IMHO of course.
>> our $GL_ADMINDIR;
>> our $GL_CONF;
>> our $GL_KEYDIR;
>> our $GL_CONF_COMPILED;
>> our $REPO_BASE;
>> our %repos;
>
> Why is the reason behind using 'our' instead of 'my' here?
They are assigned values in some file being "do"-ed, so they
can't be lexical scoped. However, I found a few others that
were holdovers from an earlier version. Fixed; thanks for
catching that.
>> # first, fix the biggest gripe I have with gitosis, a 1-line change
>> my $user=$ENV{GL_USER}=shift; # there; now that's available everywhere!
>
> Eh? This is standalone script, isn't it? Shouldn't it be
>
> my $user = $ENV{GL_USER} = $ARGV[0]; # there; now that's available everywhere!
Hmm... I didn't know there was a difference, other than
depleting @ARGV, if you're outside a subroutine. I'll take
a relook at it.
>> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;
>
> Either split it into two lines, or use ?: confitional operator:
>
> my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');
much nicer... Fixed, thanks.
>> open(LOG, ">>", "$GL_ADMINDIR/log");
>> print LOG "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
>> close(LOG);
>
> It is better practice to use lexical variables instead of barewords
> for filehandles:
Good catch; thanks! I guess I'm showing my age :) Fixed
all of them!
> Don't forget to check for error.
Hmm.. well I'm still debating if a log file write error
should block git access / push, but there were two more
important closes (again in gl-compile-conf) that were
unguarded. Fixed, thanks.
>> $repo = "'$REPO_BASE/$repo.git'";
>> exec("git", "shell", "-c", "$verb $repo");
>
> That's not enough. You have to shell-quote $repo, like in gitweb or
> using String::ShellQuote module, or somehow use list form to pass
> arguments to git-shell. You protect here againts spaces in filename,
> but not againts "'" (single quote) and for show shells "!"
> (exclamation mark).
It'll never get here. It'll die much earlier if the
reponame does not match a much stricter pattern. Maybe too
strict, actually: ^[0-9a-zA-Z][0-9a-zA-Z._/-]*$
However, I then realised I should tighten up the R_COMMANDS
and W_COMMANDS patterns a wee bit; as it stands, if someone
could create a file called, say "git-upload-pack.pwned", and
put it in the PATH, he could get the "git" user to execute
it! Sure if he managed to put something in the PATH that's
already game over in some sense but we have to stop what we
can :-) Thanks for catching this.
And once again, I really appreciate the extra eyeballs on
the code!
--
Sitaram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html