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

From: Petr Baudis <pasky@suse.cz>
Date: Sep 24, 2006, 11:36 UTC
Message-ID: <20060924113613.GM20017@pasky.or.cz>
In-Reply-To: <ef4csl$7vk$1@sea.gmane.org>

Dear diary, on Sun, Sep 24, 2006 at 12:36:13AM CEST, I got a letter where Jakub Narebski <jnareb@gmail.com> said that...

> Petr Baudis wrote:
> > (click on the funny =__ify file)
> 
> Aaargh? Why this name?

You tell me... ;-)

> > I have also made esc_param() escape [?=&;]. Not escaping [&;] was downright
> > buggy and [?=] just feels better escaped. ;-) YMMV.

..snip..

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

Huh, well, what's the point with the rename and why not keep it as it is with just removing the four characters above? Escaped stuff looks ugly in a URL. ;-)

BTW, looking at CGI::Util innards, what sick mind serves CGIs from an EBCDIC machine?

> Perhaps we should have also esc_arg() for things like title attribute
> of <a> (link) element (or other element)

Yes. I wanted to implement your few months old wish to have full string of abbreviated column contents in title attributes but delayed it for now because we have no such function yet.

> and filename="..." part of Content-disposition: HTTP header.

This is not HTML-ish so you need quotemeta() here, using entities makes no sense in this case.

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

Yes, that would be nice.

> file_name and file_parent parameters.

What's the point in validating those?

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

Yes, I agree. Will send a fixed patch.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)