git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Sep 23, 2006, 22:36 UTC
Message-ID
<ef4csl$7vk$1@sea.gmane.org>
In-Reply-To
<20060923221841.18063.56589.stgit@rover>
Petr Baudis wrote:
Show 5 quoted lines
> Consider:
> 
>         http://repo.or.cz/?p=glibc-cvs.git;a=tree;h=2609cb0411389325f4ee2854cc7159756eb0671e;hb=2609cb0411389325f4ee2854cc7159756eb0671e
> 
> (click on the funny =__ify file)
Aaargh? Why this name?
Show 6 quoted lines
> 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 parameters.

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

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Previous: Petr BaudisNext: Jakub Narebski
Message 2 of 7 in “gitweb: Consolidate escaping/validation of query string”
  1. gitweb: Consolidate escaping/validation of query stringPetr Baudis, Sep 23, 2006
  2. Jakub NarebskiSep 23, 2006
  3. Jakub NarebskiSep 23, 2006
  4. Petr BaudisSep 24, 2006
  5. Jakub NarebskiSep 24, 2006
  6. gitweb: Consolidate escaping/validation of query stringPetr Baudis, Sep 24, 2006
  7. Jakub NarebskiSep 24, 2006

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.