Re: [PATCH 1/5] gitweb: Cleanup input validation and error messages

From: Junio C Hamano <junkio@cox.net>
Date: Aug 5, 2006, 00:15 UTC
Message-ID: <7vu04sghr0.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200608050038.20534.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 7 quoted lines

>  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)?

Show 7 quoted lines

>  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.