Re: [PATCH] gitweb: Improve repository verification

[Junio C Hamano <jch@google.com>]

Jakub Narebski <jnareb@gmail.com> writes:

> Bring repository verification in check_export_ok() to standards of
> is_git_directory function from setup.c (core git), and validate_headref()
> to standards of the same function in path.c,... and a bit more.
>
> validate_headref() replaces check_head_link(); note that the former
> requires path to HEAD file, while the late latter path to repository.
>
> Issues of note:
> * is_git_directory() in gitweb is a bit stricter: it checks that
>   "/objects" and "/refs" are directories, and not only 'executable'
>   permission,
> * validate_headref() in gitweb is a bit stricter: it checks that
>   reference symlink or symref points to starts with "refs/heads/",
>   and not only with "refs/",
> * calls to check_head_link(), all of which were meant to check if
>   given directory can be a git repository, were replaced by newly
>   introduced is_git_directory().
>
> This change is preparation for removing "Last change" column from list
> of projects, which is currently used also for validating repository.
>
> Suggested-by: Kacper Kornet <draenog@pld-linux.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Here is how such first step could look like...

Do you mean by "could look like" that this is still an RFC, or is this
something we want to apply and see how well it makes people's lives in
the field?

By the way, I wonder (1) if it is worth adding support for the textual
".git" file that contains "gitdir: $path", and (2) if so how big a
change would we need to do so.

>  gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 098e527..767d7a5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -621,19 +621,51 @@ sub feature_avatar {
>  	return @val ? @val : @_;
>  }
>  
> -# checking HEAD file with -e is fragile if the repository was
> -# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
> -# and then pruned.
> -sub check_head_link {
> -	my ($dir) = @_;
> -	my $headfile = "$dir/HEAD";
> -	return ((-e $headfile) ||
> -		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
> +# Test if it looks like we're at a git directory.
> +# We want to see:
> +#
> +#  - an objects/ directory,
> +#  - a refs/ directory,
> +#  - either a HEAD symlink or a HEAD file that is formatted as
> +#    a proper "ref:", or a regular file HEAD that has a properly
> +#    formatted sha1 object name.
> +#
> +# See is_git_directory() in setup.c
> +sub is_git_directory {
> +	my $dir = shift;
> +	return
> +		-x "$dir/objects" && -d _ &&
> +		-x "$dir/refs"    && -d _ &&
> +		validate_headref("$dir/HEAD");
> +}
> +
> +# Check HEAD file, that it is either
> +#
> +#  - a "refs/heads/.." symlink, or
> +#  - a symbolic ref to "refs/heads/..", or
> +#  - a detached HEAD.
> +#
> +# See validate_headref() in path.c
> +sub validate_headref {
> +	my $headfile = shift;
> +	if (-l $headfile) {
> +		return readlink($headfile) =~ m!^refs/heads/!;
> +
> +	} elsif (-e _) {
> +		open my $fh, '<', $headfile or return;
> +		my $line = <$fh>;
> +		close $fh or return;
> +
> +		return
> +			$line =~ m!^ref:\s*refs/heads/! ||  # symref
> +			$line =~ m!^[0-9a-z]{40}$!i;        # detached HEAD
> +	}
> +	return;
>  }
>  
>  sub check_export_ok {
>  	my ($dir) = @_;
> -	return (check_head_link($dir) &&
> +	return (is_git_directory($dir) &&
>  		(!$export_ok || -e "$dir/$export_ok") &&
>  		(!$export_auth_hook || $export_auth_hook->($dir)));
>  }
> @@ -842,7 +874,7 @@ sub evaluate_path_info {
>  	# find which part of PATH_INFO is project
>  	my $project = $path_info;
>  	$project =~ s,/+$,,;
> -	while ($project && !check_head_link("$projectroot/$project")) {
> +	while ($project && !is_git_directory("$projectroot/$project")) {
>  		$project =~ s,/*[^/]*$,,;
>  	}
>  	return unless $project;