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

From: Jakub Narebski <>
Date: 2006-09-24 08:36:13
Petr Baudis wrote:

> Consider:
> (click on the funny =__ify file)

Aaargh? Why this name?

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

First of all, before introduction href() subroutine we escaped (using 
esc_param) the _whole_ generated query string. Now we need to escape
only value part (as we can assume that parameter names are correct;
they are taken from limited pre-defined set).

I'd rather have new esc_param() or esc_param_value() quote like escape
subroutine from CGI::Util, with the esception of _not_ escaping '/'
(it makes funny bookmark, and lot less readable query string), and rename
current esc_param() to esc_query_string() or esc_params().

Perhaps we should have also esc_arg() for things like title attribute
of <a> (link) element (or other element) and filename="..." part of
Content-disposition: HTTP header.

By the way, the validate_input() should be split into separate subroutines:
validate_ref() for validating hash, hash_base, hash_parent, hash_parent_base,
and validate_path() for validating project, file_name and file_parent

We should _never_ use esc_html except during the output, or just before output.
It certainly shouldn't take place in parse_* subroutine (or in the fake parse
like in git_blobdiff)!

P.S. gitweb was tested I think with filenames wirh spaces. Perhaps we should
add some other test file to gitweb/test, with '=', '@', '$', '?', '*', ' ', '\n'
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at
Received on Sun Sep 24 08:37:23 2006

This archive was generated by hypermail 2.1.8 : 2006-09-24 08:38:11 EST