Re: [PATCH] gitweb: Consolidate escaping/validation of query string

[Jakub Narebski <jnareb@gmail.com>]

Petr Baudis wrote:

> Consider:
> 
>       http://repo.or.cz/?p=glibc-cvs.git;a=tree;h=2609cb0411389325f4ee2854cc7159756eb0671e;hb=2609cb0411389325f4ee2854cc7159756eb0671e
> 
> (click on the funny =__ify file)
> 
> We ought to handle anything in filenames and I actually see no reason why
> we don't, modulo very little missing escaping that this patch hopefully
> also fixes.
> 
> I have also made esc_param() escape [?=&;]. Not escaping [&;] was downright
> buggy and [?=] just feels better escaped. ;-) YMMV.
> 
> Signed-off-by: Petr Baudis <pasky@suse.cz>

This patch contains a few unrelated changes:
 * changing semantics of esc_param subroutine
 * adding some esc_html where it could be needed
 * removing $file_name and $file_parent validation

About change to esc_param: we need current version of esc_param, perhaps
to be named esc_url to be able to say esc_url($home_link) and soon
esc_url($githelp_url).

About adding esc_html where it could be needed: good change, see
my comments below.

About removing $file_name and $file_parent validation: those parameters
have exactly the same textual restrictions as $project parameter - they
are pathnames.

> @@ -2439,7 +2429,7 @@ sub git_blame2 {
>       if ($ftype !~ "blob") {
>               die_error("400 Bad Request", "Object is not a blob");
>       }
> -     open ($fd, "-|", git_cmd(), "blame", '-l', $file_name, $hash_base)
> +     open ($fd, "-|", git_cmd(), "blame", '-l', '--', $file_name, $hash_base)
>               or die_error(undef, "Open git-blame failed");
>       git_header_html();
>       my $formats_nav =

Slightly unrelated change. Shouldn't it be

        open $fd, "-|", git_cmd(), "blame", '-l', $hash_base, "--", $file_name

by the way?

> @@ -3135,7 +3125,7 @@ sub git_blobdiff {
>                       -type => 'text/plain',
>                       -charset => 'utf-8',
>                       -expires => $expires,
> -                     -content_disposition => qq(inline; filename="${file_name}.patch"));
> +                     -content_disposition => qq(inline; filename=") . quotemeta($file_name) . qq(.patch"));
>  
>               print "X-Git-Url: " . $cgi->self_url() . "\n\n";
>  

I'd check other places where we output Content-Disposition: header.
At least one place needs similar quotemeta somewhere.

> @@ -3585,7 +3575,7 @@ XML
>                       if (!($line =~ m/^:([0-7]{6}) ([0-7]{6}) ([0-9a-fA-F]{40}) ([0-9a-fA-F]{40}) (.)([0-9]{0,3})\t(.*)$/)) {
>                               next;
>                       }
> -                     my $file = validate_input(unquote($7));
> +                     my $file = esc_html(unquote($7));
>                       $file = decode("utf8", $file, Encode::FB_DEFAULT);
>                       print "$file<br/>\n";
>               }

I'd say perhaps

        my $file = unquote($7);
        $file = decode("utf8", $file, Encode::FB_DEFAULT);
        print esc_html($file) . "<br/>\n";

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git