From: Junio C Hamano <junkio@cox.net>
Date: Sat, 05 Aug 2006 00:15:31 GMT
Subject: Re: [PATCH 1/5] gitweb: Cleanup input validation and error messages
Message-ID: <7vu04sghr0.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200608050038.20534.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

>  our $action = $cgi->param('a');
>  if (defined $action) {
>  	if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
> -		undef $action;
> -		die_error(undef, "Invalid action parameter.");
> +		die_error(undef, "Invalid action parameter $action");
>  	}

Doesn't this make us parrot what the browser threw at us without
escaping back for HTML (iow, die_error does not seem to escape
$error)?

>  our $project = ($cgi->param('p') || $ENV{'PATH_INFO'});
> -if (defined $project) {
> -	$project =~ s|^/||; $project =~ s|/$||;
> -	$project = validate_input($project);
> -	if (!defined($project)) {
> -		die_error(undef, "Invalid project parameter.");
> +$project =~ s|^/||; $project =~ s|/$||;

Unrelated change but it is probably safe.

> +if (defined $project || $project) {
> +	if (!validate_input($project)) {
> +		die_error(undef, "Invalid project parameter $project");
>  	}

Because undef is not true, "|| $project" here does not make much
sense to me.  Even if you meant to say "&&" to exclude empty
string or "0", wouldn't validate_input() take care of them?
Same input parrotting comment applies here and everywhere.

> -	$rss_link = "<link rel=\"alternate\" title=\"" . esc_param($project) . " log\" href=\"" .
> -	            "$my_uri?" . esc_param("p=$project;a=rss") . "\" type=\"application/rss+xml\"/>";

The reason of removal is...?  Ah, you inlined it.  It was not
clear from the proposed commit log message.