threads / patch / 5695

patch, 5 partsgitweb: A few code cleanup patches

Subject: [PATCH 0/5] gitweb: A few code cleanup patches

## tl;dr

10 messages between Sep 25, 2006 and Sep 26, 2006. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Jakub Narebski· Sep 25, 2006, 23:53 UTC · lore
This series of patches, based on 8391548e5e78677eb81f97334d998418802ea194
adds some code cleanups (including validating input and escaping output).
Most deals with changes introduced in
  [PATCH] gitweb: Consolidate escaping/validation of query string
  commit a2f3db2f5de2a3667b0e038aa65e3e097e642e7d (by Petr Baudis)
  Message-ID: <20060923221841.18063.56589.stgit@rover>
Shortlog:
 [PATCH 1/5] gitweb: Strip trailing slashes from $path in git_get_hash_by_path
 [PATCH 2/5] gitweb: Use "return" instead of "return undef" for some subs
 [PATCH 3/5] gitweb: Split validate_input into validate_pathname and validate_refname
 [PATCH 4/5] gitweb: Add git_url subroutine, and use it to quote full URLs
 [PATCH 5/5] gitweb: Quote filename in HTTP Content-Disposition: header
Diffstat:
 gitweb/gitweb.perl |  100 +++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 72 insertions(+), 28 deletions(-)
-- 
Jakub Narebski
ShadeHawk on #git
Warsaw, Poland
Jakub Narebski· Sep 25, 2006, 23:54 UTC · re: Jakub Narebski · lore

[PATCH 1/5] gitweb: Strip trailing slashes from $path in git_get_hash_by_path

It also removes unused local variable $tree 
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +1 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 66be619..2ad7eed 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -710,7 +710,7 @@ sub git_get_hash_by_path {
 	my $path = shift || return undef;
 	my $type = shift;
 
-	my $tree = $base;
+	$path =~ s,/+$,,;
 
 	open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
 		or die_error(undef, "Open git-ls-tree failed");
-- 
1.4.2.1
Jakub Narebski· Sep 25, 2006, 23:56 UTC · re: Jakub Narebski · lore

[PATCH 2/5] gitweb: Use "return" instead of "return undef" for some subs

Use "return" instead of "return undef" when subroutine can return, or always return, non-scalar (list) value.

Other places are left as is.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is inspired by "gitweb: Fix @git_base_url_list usage" patch
(74d6166751ddcf08029ffc90a14158a86f80cd40) by Petr Baudis.
 gitweb/gitweb.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to gitweb/gitweb.perl +2 −2
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 2ad7eed..6458d7b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -106,7 +106,7 @@ our %feature = (
 
 sub gitweb_check_feature {
 	my ($name) = @_;
-	return undef unless exists $feature{$name};
+	return unless exists $feature{$name};
 	my ($sub, $override, @defaults) = (
 		$feature{$name}{'sub'},
 		$feature{$name}{'override'},
@@ -781,7 +781,7 @@ sub git_get_projects_list {
 		# 'git%2Fgit.git Linus+Torvalds'
 		# 'libs%2Fklibc%2Fklibc.git H.+Peter+Anvin'
 		# 'linux%2Fhotplug%2Fudev.git Greg+Kroah-Hartman'
-		open my ($fd), $projects_list or return undef;
+		open my ($fd), $projects_list or return;
 		while (my $line = <$fd>) {
 			chomp $line;
 			my ($path, $owner) = split ' ', $line;
-- 
1.4.2.1
Jakub Narebski· Sep 25, 2006, 23:57 UTC · re: Jakub Narebski · lore

[PATCH 3/5] gitweb: Split validate_input into validate_pathname and validate_refname

Split validate_input subroutine into validate_pathname which is used for $project, $file_name and $file_parent parameters, and validate_refname which is used for $hash, $hash_base, $hash_parent and $hash_parent_base parameters. Reintroduce validation of $file_name and $file_parent parameters, removed in a2f3db2f5de2a3667b0e038aa65e3

validate_pathname in addition to what validate_input did checks also for doubled slashes and NUL character. It does not check if input is textual hash, and does not check if all characters are from the following set: [a-zA-Z0-9_\x80-\xff\ \t\.\/\-\+\#\~\%].

validate_refname first check if the input is textual hash, then checks if it is valid pathname, then checks for invalid characters (according to git-check-ref-format manpage). It does not check if all charactes are from the [a-zA-Z0-9_\x80-\xff\ \t\.\/\-\+\#\~\%] set.

We do not reintroduce validating pathnames we got from git.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
At last.
 gitweb/gitweb.perl |   66 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 50 insertions(+), 16 deletions(-)
Show changes to gitweb/gitweb.perl +50 −16
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6458d7b..7fce9a6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -200,9 +200,10 @@ if (defined $action) {
 	}
 }
 
+# parameters which are pathnames
 our $project = $cgi->param('p');
 if (defined $project) {
-	if (!validate_input($project) ||
+	if (!validate_pathname($project) ||
 	    !(-d "$projectroot/$project") ||
 	    !(-e "$projectroot/$project/HEAD") ||
 	    ($export_ok && !(-e "$projectroot/$project/$export_ok")) ||
@@ -212,38 +213,50 @@ if (defined $project) {
 	}
 }
 
-# We have to handle those containing any characters:
 our $file_name = $cgi->param('f');
+if (defined $file_name) {
+	if (!validate_pathname($file_name)) {
+		die_error(undef, "Invalid file parameter");
+	}
+}
+
 our $file_parent = $cgi->param('fp');
+if (defined $file_parent) {
+	if (!validate_pathname($file_parent)) {
+		die_error(undef, "Invalid file parent parameter");
+	}
+}
 
+# parameters which are refnames
 our $hash = $cgi->param('h');
 if (defined $hash) {
-	if (!validate_input($hash)) {
+	if (!validate_refname($hash)) {
 		die_error(undef, "Invalid hash parameter");
 	}
 }
 
 our $hash_parent = $cgi->param('hp');
 if (defined $hash_parent) {
-	if (!validate_input($hash_parent)) {
+	if (!validate_refname($hash_parent)) {
 		die_error(undef, "Invalid hash parent parameter");
 	}
 }
 
 our $hash_base = $cgi->param('hb');
 if (defined $hash_base) {
-	if (!validate_input($hash_base)) {
+	if (!validate_refname($hash_base)) {
 		die_error(undef, "Invalid hash base parameter");
 	}
 }
 
 our $hash_parent_base = $cgi->param('hpb');
 if (defined $hash_parent_base) {
-	if (!validate_input($hash_parent_base)) {
+	if (!validate_refname($hash_parent_base)) {
 		die_error(undef, "Invalid hash parent base parameter");
 	}
 }
 
+# other parameters
 our $page = $cgi->param('pg');
 if (defined $page) {
 	if ($page =~ m/[^0-9]/) {
@@ -273,7 +286,7 @@ sub evaluate_path_info {
 		$project =~ s,/*[^/]*$,,;
 	}
 	# validate project
-	$project = validate_input($project);
+	$project = validate_pathname($project);
 	if (!$project ||
 	    ($export_ok && !-e "$projectroot/$project/$export_ok") ||
 	    ($strict_export && !project_in_list($project))) {
@@ -294,12 +307,12 @@ sub evaluate_path_info {
 		} else {
 			$action  ||= "blob_plain";
 		}
-		$hash_base ||= validate_input($refname);
-		$file_name ||= $pathname;
+		$hash_base ||= validate_refname($refname);
+		$file_name ||= validate_pathname($pathname);
 	} elsif (defined $refname) {
 		# we got "project.git/branch"
 		$action ||= "shortlog";
-		$hash   ||= validate_input($refname);
+		$hash   ||= validate_refname($refname);
 	}
 }
 evaluate_path_info();
@@ -387,16 +400,37 @@ sub href(%) {
 ## ======================================================================
 ## validation, quoting/unquoting and escaping
 
-sub validate_input {
-	my $input = shift;
+sub validate_pathname {
+	my $input = shift || return undef;
 
-	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
-		return $input;
+	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
+	# at the beginning, at the end, and between slashes.
+	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
+		return undef;
 	}
-	if ($input =~ m/(^|\/)(|\.|\.\.)($|\/)/) {
+	# no doubled slashes
+	if ($input =~ m!//!) {
 		return undef;
 	}
-	if ($input =~ m/[^a-zA-Z0-9_\x80-\xff\ \t\.\/\-\+\#\~\%]/) {
+	# no null characters
+	if ($input =~ m!\0!) {
+		return undef;
+	}
+	return $input;
+}
+
+sub validate_refname {
+	my $input = shift || return undef;
+
+	# textual hashes are O.K.
+	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
+		return $input;
+	}
+	# it must be correct pathname
+	$input = validate_pathname($input)
+		or return undef;
+	# restrictions on ref name according to git-check-ref-format
+	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
 		return undef;
 	}
 	return $input;
-- 
1.4.2.1
Junio C Hamano· Sep 26, 2006, 04:11 UTC · re: Jakub Narebski · lore

Re: [PATCH 3/5] gitweb: Split validate_input into validate_pathname and validate_refname

Jakub Narebski <jnareb@gmail.com> writes:
Show 21 quoted lines
> @@ -387,16 +400,37 @@ sub href(%) {
>  ## ======================================================================
>  ## validation, quoting/unquoting and escaping
>  
> -sub validate_input {
> -	my $input = shift;
> +sub validate_pathname {
> +	my $input = shift || return undef;
>  
> -	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
> -		return $input;
> +	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
> +	# at the beginning, at the end, and between slashes.
> +	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
> +		return undef;
>  	}
> -	if ($input =~ m/(^|\/)(|\.|\.\.)($|\/)/) {
> +	# no doubled slashes
> +	if ($input =~ m!//!) {
>  		return undef;
>  	}

I do not think you need the second check for double-slash. The pattern you borrowed from the original:

	/(^|\/)(|\.|\.\.)($|\/)/)

cleverly matches an empty string with $2, so you already match double-slash with $1 = '/' $2 = '' $3 = '/', don't you?

Show 7 quoted lines
> +	# it must be correct pathname
> +	$input = validate_pathname($input)
> +		or return undef;
> +	# restrictions on ref name according to git-check-ref-format
> +	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
>  		return undef;
>  	}
Why would you need validate_pathname here?
Jakub Narebski· Sep 26, 2006, 07:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/5] gitweb: Split validate_input into validate_pathname and validate_refname

Dnia wtorek 26. września 2006 06:11, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > @@ -387,16 +400,37 @@ sub href(%) {
> >  ## 
======================================================================
Show 27 quoted lines
> >  ## validation, quoting/unquoting and escaping
> >  
> > -sub validate_input {
> > -	my $input = shift;
> > +sub validate_pathname {
> > +	my $input = shift || return undef;
> >  
> > -	if ($input =~ m/^[0-9a-fA-F]{40}$/) {
> > -		return $input;
> > +	# no '.' or '..' as elements of path, i.e. no '.' nor '..'
> > +	# at the beginning, at the end, and between slashes.
> > +	if ($input =~ m!(^|/)(|\.|\.\.)(/|$)!) {
> > +		return undef;
> >  	}
> > -	if ($input =~ m/(^|\/)(|\.|\.\.)($|\/)/) {
> > +	# no doubled slashes
> > +	if ($input =~ m!//!) {
> >  		return undef;
> >  	}
> 
> I do not think you need the second check for double-slash.  The
> pattern you borrowed from the original:
> 
> 	/(^|\/)(|\.|\.\.)($|\/)/)
> 
> cleverly matches an empty string with $2, so you already match
> double-slash with $1 = '/' $2 = '' $3 = '/', don't you?
Do I need to resend patch, then, to remove this unnecessary check?
Show 9 quoted lines
> > +	# it must be correct pathname
> > +	$input = validate_pathname($input)
> > +		or return undef;
> > +	# restrictions on ref name according to git-check-ref-format
> > +	if ($input =~ m!(/\.|\.\.|[\000-\040\177 ~^:?*\[]|/$)!) {
> >  		return undef;
> >  	}
> 
> Why would you need validate_pathname here?

refname _must_ be a valid pathname, no? It means for example that it cannot have double slashes, not NUL (the only thinkg not covered by git-check-ref-format restrictions). Well, we could add that to regexp instead...

-- 
Jakub Narebski
Poland
Jakub Narebski· Sep 25, 2006, 23:58 UTC · re: Jakub Narebski · lore

[PATCH 4/5] gitweb: Add git_url subroutine, and use it to quote full URLs

Add git_url subroutine, which does what git_param did before commit a2f3db2f5de2a3667b0e038aa65e3e097e642e7d, and is used to quote full URLs, currently only $home_link.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
It will also be used in patch inspired by
  "[RFC][PATCH] gitweb: Make the Git logo link target to point to the homepage"
  Message-ID: <7virjes7dq.fsf@assigned-by-dhcp.cox.net>
  http://permalink.gmane.org/gmane.comp.version-control.git/27619
 gitweb/gitweb.perl |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)
Show changes to gitweb/gitweb.perl +10 −1
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7fce9a6..b51e061 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -446,6 +446,15 @@ sub esc_param {
 	return $str;
 }
 
+# quote unsafe chars in whole URL, so some charactrs cannot be quoted
+sub esc_url {
+	my $str = shift;
+	$str =~ s/([^A-Za-z0-9\-_.~();\/;?:@&=])/sprintf("%%%02X", ord($1))/eg;
+	$str =~ s/\+/%2B/g;
+	$str =~ s/ /\+/g;
+	return $str;
+}
+
 # replace invalid utf8 character with SUBSTITUTION sequence
 sub esc_html {
 	my $str = shift;
@@ -1362,7 +1371,7 @@ EOF
 	      "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git documentation\">" .
 	      "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right; border-width:0px;\"/>" .
 	      "</a>\n";
-	print $cgi->a({-href => esc_param($home_link)}, $home_link_str) . " / ";
+	print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
 	if (defined $project) {
 		print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
 		if (defined $action) {
-- 
1.4.2.1
Jakub Narebski· Sep 25, 2006, 23:59 UTC · re: Jakub Narebski · lore

[PATCH 5/5] gitweb: Quote filename in HTTP Content-Disposition: header

Finish work started by a2f3db2f5de2a3667b0e038aa65e3e097e642e7d commit (although not documented in commit message) of quoting using quotemeta the filename in HTTP -content_disposition header. Uniquify output.

Just in case filename contains end of line character.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
Show changes to gitweb/gitweb.perl +9 −8
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b51e061..4dd7a5d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2326,7 +2326,7 @@ sub git_project_index {
 	print $cgi->header(
 		-type => 'text/plain',
 		-charset => 'utf-8',
-		-content_disposition => qq(inline; filename="index.aux"));
+		-content_disposition => 'inline; filename="index.aux"');
 
 	foreach my $pr (@projects) {
 		if (!exists $pr->{'owner'}) {
@@ -2672,7 +2672,7 @@ sub git_blob_plain {
 	print $cgi->header(
 		-type => "$type",
 		-expires=>$expires,
-		-content_disposition => "inline; filename=\"$save_as\"");
+		-content_disposition => 'inline; filename="' . quotemeta($save_as) . '"');
 	undef $/;
 	binmode STDOUT, ':raw';
 	print <$fd>;
@@ -2846,10 +2846,11 @@ sub git_snapshot {
 
 	my $filename = basename($project) . "-$hash.tar.$suffix";
 
-	print $cgi->header(-type => 'application/x-tar',
-	                   -content_encoding => $ctype,
-	                   -content_disposition => "inline; filename=\"$filename\"",
-	                   -status => '200 OK');
+	print $cgi->header(
+		-type => 'application/x-tar',
+		-content_encoding => $ctype,
+		-content_disposition => 'inline; filename="' . quotemeta($filename) . '"',
+		-status => '200 OK');
 
 	my $git_command = git_cmd_str();
 	open my $fd, "-|", "$git_command tar-tree $hash \'$project\' | $command" or
@@ -3159,7 +3160,7 @@ sub git_blobdiff {
 			-type => 'text/plain',
 			-charset => 'utf-8',
 			-expires => $expires,
-			-content_disposition => qq(inline; filename=") . quotemeta($file_name) . qq(.patch"));
+			-content_disposition => 'inline; filename="' . quotemeta($file_name) . '.patch"');
 
 		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
 
@@ -3262,7 +3263,7 @@ sub git_commitdiff {
 			-type => 'text/plain',
 			-charset => 'utf-8',
 			-expires => $expires,
-			-content_disposition => qq(inline; filename="$filename"));
+			-content_disposition => 'inline; filename="' . quotemeta($filename) . '"');
 		my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'});
 		print <<TEXT;
 From: $co{'author'}
-- 
1.4.2.1
Junio C Hamano· Sep 26, 2006, 04:11 UTC · re: Jakub Narebski · lore

Re: [PATCH 5/5] gitweb: Quote filename in HTTP Content-Disposition: header

Jakub Narebski <jnareb@gmail.com> writes:
Show 5 quoted lines
> Finish work started by a2f3db2f5de2a3667b0e038aa65e3e097e642e7d commit
> (although not documented in commit message) of quoting using quotemeta
> the filename in HTTP -content_disposition header.  Uniquify output.
>
> Just in case filename contains end of line character.
What do you mean by "uniquify output"?
Jakub Narebski· Sep 26, 2006, 07:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/5] gitweb: Quote filename in HTTP Content-Disposition: header

Junio C Hamano wrote:
Show 9 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Finish work started by a2f3db2f5de2a3667b0e038aa65e3e097e642e7d commit
>> (although not documented in commit message) of quoting using quotemeta
>> the filename in HTTP -content_disposition header.  Uniquify output.
>>
>> Just in case filename contains end of line character.
> 
> What do you mean by "uniquify output"?

It means alsways use 'inline; filename="' . (quotemeta <whatever>) . '"', and not sometimes qq(...), sometimes "...\"...\"", and sometimes '...'.

-- 
Jakub Narebski
Poland

← back to recent threads