gitweb not friendly to firefox revived

gitweb not friendly to firefox revived

[Uwe Kleine-König]

Hello,

gitweb (at least) doesn't quote author names enough.

Firefox barfs for me at looking at

	http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10

with an error:

	XML Parsing Error: not well-formed Location:
http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
Line Number 112, Column 81:
<td class="author"><a title="Search for commits authored by YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B" class="list" href="/?p=ukl/linux-2.6.git;a=search;h=v2.6.16.10;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a></td><td><a class="list subject" title="[PATCH] IPV6: XFRM: Fix decoding session with preceding extension header(s)." href="/?p=ukl/linux-2.6.git;a=commit;h=fa39df2ff7f6102f1f37d3cf1f68243534d56253">[PATCH] IPV6: XFRM: Fix decoding session with preceding... </a></td>
--------------------------------------------------------------------------------^

This is with git 1.7.1 and Iceweasel (aka. Firefox) 3.5.10.

Making

	title=>"Search for commits $performed by $author"

in line 1694 of Debian's /usr/lib/cgi-bin/gitweb.cgi from the git 1.7.1
package read

	title=>esc_html("Search for commits $performed by $author")

this problem goes away.  (Still my browser barfs when clicking at the name.)

I'm not sure if this is the right way to fix this and I'm too tired now
to do a complete patch, so I let this for someone else.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: gitweb not friendly to firefox revived

[Ævar Arnfjörð Bjarmason]

2010/8/1 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:

> This is with git 1.7.1 and Iceweasel (aka. Firefox) 3.5.10.
>
> Making
>
>        title=>"Search for commits $performed by $author"
>
> in line 1694 of Debian's /usr/lib/cgi-bin/gitweb.cgi from the git 1.7.1
> package read
>
>        title=>esc_html("Search for commits $performed by $author")
>
> this problem goes away.  (Still my browser barfs when clicking at the name.)

That fix seems correct, the author appears to have control characters
in his name. Probably as a result of some encoding corruption.

What's the URL of the Git repository itself? It'd be interesting to
see how that went wrong in the first place.

Re: gitweb not friendly to firefox revived

[Jakub Narebski]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

> Hello,
> 
> gitweb (at least) doesn't quote author names enough.
> 
> Firefox barfs for me at looking at
> 
> 	http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> 
> with an error:
> 
> 	XML Parsing Error: not well-formed Location:
> http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> Line Number 112, Column 81:
> <td class="author"><a title="Search for commits authored by YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B" class="list" href="/?p=ukl/linux-2.6.git;a=search;h=v2.6.16.10;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a></td><td><a class="list subject" title="[PATCH] IPV6: XFRM: Fix decoding session with preceding extension header(s)." href="/?p=ukl/linux-2.6.git;a=commit;h=fa39df2ff7f6102f1f37d3cf1f68243534d56253">[PATCH] IPV6: XFRM: Fix decoding session with preceding... </a></td>
> --------------------------------------------------------------------------------^
> 
> This is with git 1.7.1 and Iceweasel (aka. Firefox) 3.5.10.
> 
> Making
> 
> 	title=>"Search for commits $performed by $author"
> 
> in line 1694 of Debian's /usr/lib/cgi-bin/gitweb.cgi from the git 1.7.1
> package read
> 
> 	title=>esc_html("Search for commits $performed by $author")
> 
> this problem goes away.  (Still my browser barfs when clicking at the name.)
> 
> I'm not sure if this is the right way to fix this and I'm too tired now
> to do a complete patch, so I let this for someone else.

Actually gitweb leaves quoting of tag attributes to CGI module:

  return $cgi->a({-href => href(action=>"search", hash=>$hash,
                                searchtext=>$author, searchtype=>$searchtype),
                  -class => "list",
                  -title => "Search for commits $performed by $author"},
                 $displaytext);

I am worrying (perhaps unnecessary) that using esc_html would result
in double escaping.  But it looks like the problem is with Unicode,
so perhaps using

  	title => to_utf8("Search for commits $performed by $author")

in place of

  	title=>esc_html("Search for commits $performed by $author")

would be a better fix?  Does this fix work for you?


Cc-ed Stephen Boyd, who is author of commit e133d65 (gitweb: linkify
author/committer names with search, 2009-10-15), introducing the code
you found this bug in.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: gitweb not friendly to firefox revived

[Uwe Kleine-König]

Hi,

> What's the URL of the Git repository itself? It'd be interesting to
> see how that went wrong in the first place.
it's just a copy of 2.6.16.y that is available at kernel.org:
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.16.y.git

The commit happend before 2.6.16.10.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: gitweb not friendly to firefox revived

[Uwe Kleine-König]

On Sun, Aug 01, 2010 at 01:26:16PM -0700, Jakub Narebski wrote:
> Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
> 
> > Hello,
> > 
> > gitweb (at least) doesn't quote author names enough.
> > 
> > Firefox barfs for me at looking at
> > 
> > 	http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> > 
> > with an error:
> > 
> > 	XML Parsing Error: not well-formed Location:
> > http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> > Line Number 112, Column 81:
> > <td class="author"><a title="Search for commits authored by YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B" class="list" href="/?p=ukl/linux-2.6.git;a=search;h=v2.6.16.10;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a></td><td><a class="list subject" title="[PATCH] IPV6: XFRM: Fix decoding session with preceding extension header(s)." href="/?p=ukl/linux-2.6.git;a=commit;h=fa39df2ff7f6102f1f37d3cf1f68243534d56253">[PATCH] IPV6: XFRM: Fix decoding session with preceding... </a></td>
> > --------------------------------------------------------------------------------^
> > 
> > This is with git 1.7.1 and Iceweasel (aka. Firefox) 3.5.10.
> > 
> > Making
> > 
> > 	title=>"Search for commits $performed by $author"
> > 
> > in line 1694 of Debian's /usr/lib/cgi-bin/gitweb.cgi from the git 1.7.1
> > package read
> > 
> > 	title=>esc_html("Search for commits $performed by $author")
> > 
> > this problem goes away.  (Still my browser barfs when clicking at the name.)
> > 
> > I'm not sure if this is the right way to fix this and I'm too tired now
> > to do a complete patch, so I let this for someone else.
> 
> Actually gitweb leaves quoting of tag attributes to CGI module:
> 
>   return $cgi->a({-href => href(action=>"search", hash=>$hash,
>                                 searchtext=>$author, searchtype=>$searchtype),
>                   -class => "list",
>                   -title => "Search for commits $performed by $author"},
>                  $displaytext);
> 
> I am worrying (perhaps unnecessary) that using esc_html would result
> in double escaping.  But it looks like the problem is with Unicode,
> so perhaps using
> 
>   	title => to_utf8("Search for commits $performed by $author")
> 
> in place of
> 
>   	title=>esc_html("Search for commits $performed by $author")
> 
> would be a better fix?  Does this fix work for you?
No, this doesn't help.  Firefox still barfs with to_utf8.

With esc_html the code generated is:

<a title="Search for commits authored by YOSHIFUJI Hideaki / <span class="cntrl">\e</span>$B5HF#1QL@<span class="cntrl">\e</span>(B" class="list" href="/?p=.git;a=search;h=f66ab685594d49e570b2176cfa20b03360e9a6e9;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: gitweb not friendly to firefox revived

[Jakub Narebski]

On Tue, Aug 03, 2010, Uwe Kleine-König wrote:
> On Sun, Aug 01, 2010 at 01:26:16PM -0700, Jakub Narebski wrote:
> > Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
> > 
> > > Hello,
> > > 
> > > gitweb (at least) doesn't quote author names enough.
> > > 
> > > Firefox barfs for me at looking at
> > > 
> > > 	http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> > > 
> > > with an error:
> > > 
> > > 	XML Parsing Error: not well-formed Location:
> > > http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=v2.6.16.10
> > > Line Number 112, Column 81:
> > > <td class="author"><a title="Search for commits authored by YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B" class="list" href="/?p=ukl/linux-2.6.git;a=search;h=v2.6.16.10;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a></td><td><a class="list subject" title="[PATCH] IPV6: XFRM: Fix decoding session with preceding extension header(s)." href="/?p=ukl/linux-2.6.git;a=commit;h=fa39df2ff7f6102f1f37d3cf1f68243534d56253">[PATCH] IPV6: XFRM: Fix decoding session with preceding... </a></td>
> > > --------------------------------------------------------------------------------^
> > > 
> > > This is with git 1.7.1 and Iceweasel (aka. Firefox) 3.5.10.
> > > 
> > > Making
> > > 
> > > 	title=>"Search for commits $performed by $author"
> > > 
> > > in line 1694 of Debian's /usr/lib/cgi-bin/gitweb.cgi from the git 1.7.1
> > > package read
> > > 
> > > 	title=>esc_html("Search for commits $performed by $author")
> > > 
> > > this problem goes away.  (Still my browser barfs when clicking at the name.)
> > > 
> > > I'm not sure if this is the right way to fix this and I'm too tired now
> > > to do a complete patch, so I let this for someone else.
> > 
> > Actually gitweb leaves quoting of tag attributes to CGI module:
> > 
> >   return $cgi->a({-href => href(action=>"search", hash=>$hash,
> >                                 searchtext=>$author, searchtype=>$searchtype),
> >                   -class => "list",
> >                   -title => "Search for commits $performed by $author"},
> >                  $displaytext);
> > 
> > I am worrying (perhaps unnecessary) that using esc_html would result
> > in double escaping.  But it looks like the problem is with Unicode,
> > so perhaps using
> > 
> >   	title => to_utf8("Search for commits $performed by $author")
> > 
> > in place of
> > 
> >   	title=>esc_html("Search for commits $performed by $author")
> > 
> > would be a better fix?  Does this fix work for you?
>
> No, this doesn't help.  Firefox still barfs with to_utf8.
> 
> With esc_html the code generated is:
> 
> <a title="Search for commits authored by YOSHIFUJI Hideaki / <span class="cntrl">\e</span>$B5HF#1QL@<span class="cntrl">\e</span>(B" class="list" href="/?p=.git;a=search;h=f66ab685594d49e570b2176cfa20b03360e9a6e9;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author"><span title="YOSHIFUJI Hideaki / ?$B5HF#1QL@?(B">YOSHIFUJI Hideaki...  </span></a>

As you can see the HTML code generated with esc_html solution is way wrong
because of embedded '<span class="cntrl">\e</span>' as you see _without_
'"' being escaped, so HTML is wrong.

Nevertheless it shows what's the problem.  Somehow (perhaps wrong
encoding, perhaps screw up with quoted-printable and git-am, perhaps
copy'n' paste included ANSII color codes from terminal, perhaps something
different altogether) you got control characters (\e = ESC) in $author.
In strict XHTML mode (with 'application/xml

Please try the following patch

-- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Harden format_search_author()

Protect format_search_author against control characters in $author.
While at it simplify it a bit, and use spaces for align.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8b02767..ea9c09c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1856,23 +1856,22 @@ sub format_search_author {
 	my ($author, $searchtype, $displaytext) = @_;
 	my $have_search = gitweb_check_feature('search');
 
-	if ($have_search) {
-		my $performed = "";
-		if ($searchtype eq 'author') {
-			$performed = "authored";
-		} elsif ($searchtype eq 'committer') {
-			$performed = "committed";
-		}
-
-		return $cgi->a({-href => href(action=>"search", hash=>$hash,
-				searchtext=>$author,
-				searchtype=>$searchtype), class=>"list",
-				title=>"Search for commits $performed by $author"},
-				$displaytext);
+	return $displaytext unless ($have_search);
 
-	} else {
-		return $displaytext;
+	my $performed = "";
+	if ($searchtype eq 'author') {
+		$performed = "authored";
+	} elsif ($searchtype eq 'committer') {
+		$performed = "committed";
 	}
+
+	my $title = to_utf8("Search for commits $performed by $author");
+	$title =~ s/[[:cntrl:]]/?/g;
+
+	return $cgi->a({-href => href(action=>"search", hash=>$hash,
+	                              searchtext=>$author, searchtype=>$searchtype),
+	                -class=>"list", -title=>$title},
+	               $displaytext);
 }
 
 # format the author name of the given commit with the given tag
-- 
1.7.2.1

Re: gitweb not friendly to firefox revived

[Uwe Kleine-König]

Hello,

> Nevertheless it shows what's the problem.  Somehow (perhaps wrong
> encoding, perhaps screw up with quoted-printable and git-am, perhaps
> copy'n' paste included ANSII color codes from terminal, perhaps something
> different altogether) you got control characters (\e = ESC) in $author.
> In strict XHTML mode (with 'application/xml
> 
> Please try the following patch
> 
> -- >8 --
> From: Jakub Narebski <jnareb@gmail.com>
> Subject: [PATCH] gitweb: Harden format_search_author()
> 
> Protect format_search_author against control characters in $author.
> While at it simplify it a bit, and use spaces for align.
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
>  gitweb/gitweb.perl |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8b02767..ea9c09c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1856,23 +1856,22 @@ sub format_search_author {
>  	my ($author, $searchtype, $displaytext) = @_;
>  	my $have_search = gitweb_check_feature('search');
>  
> -	if ($have_search) {
> -		my $performed = "";
> -		if ($searchtype eq 'author') {
> -			$performed = "authored";
> -		} elsif ($searchtype eq 'committer') {
> -			$performed = "committed";
> -		}
> -
> -		return $cgi->a({-href => href(action=>"search", hash=>$hash,
> -				searchtext=>$author,
> -				searchtype=>$searchtype), class=>"list",
> -				title=>"Search for commits $performed by $author"},
> -				$displaytext);
> +	return $displaytext unless ($have_search);
>  
> -	} else {
> -		return $displaytext;
> +	my $performed = "";
> +	if ($searchtype eq 'author') {
> +		$performed = "authored";
> +	} elsif ($searchtype eq 'committer') {
> +		$performed = "committed";
>  	}
> +
> +	my $title = to_utf8("Search for commits $performed by $author");
> +	$title =~ s/[[:cntrl:]]/?/g;
> +
> +	return $cgi->a({-href => href(action=>"search", hash=>$hash,
> +	                              searchtext=>$author, searchtype=>$searchtype),
> +	                -class=>"list", -title=>$title},
> +	               $displaytext);
>  }
Seems to do the right thing.  With Firefox I get a correct listing now
instead of the xml parse error.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: gitweb not friendly to firefox revived

[Stephen Boyd]

  On 08/03/2010 02:50 PM, Jakub Narebski wrote:
> +
> +	my $title = to_utf8("Search for commits $performed by $author");
> +	$title =~ s/[[:cntrl:]]/?/g;
> +
>

Isn't it possible that other data coming from git could have escape characters in them such as the commit subject line? In which case this same bug would occur?

Therefore isn't it better to strip out control characters (that's what this patch is doing right?) in esc_html?

Re: gitweb not friendly to firefox revived

[Ævar Arnfjörð Bjarmason]

2010/8/14 Stephen Boyd <bebarino@gmail.com>:
>  On 08/03/2010 02:50 PM, Jakub Narebski wrote:
>>
>> +
>> +       my $title = to_utf8("Search for commits $performed by $author");
>> +       $title =~ s/[[:cntrl:]]/?/g;
>> +
>>
>
> Isn't it possible that other data coming from git could have escape
> characters in them such as the commit subject line? In which case this same
> bug would occur?
>
> Therefore isn't it better to strip out control characters (that's what this
> patch is doing right?) in esc_html?

I don't think stripping them out is the right thing either, hiding
from you that something is Really Wrong (binary garbage in patches)
isn't good.

Something like this would be better:

    s/([[:cntrl:]])/sprintf("\\%03x", ord $1)/ge

E.g.:

    $ perl -E 'my $s = join "", map { chr } 1 .. 40; $s =~
s/([[:cntrl:]])/sprintf("\\%03x", ord $1)/ge; say $s'
    \001\002\003\004\005\006\007\008\009\00a\00b\00c\00d\00e\00f\010\011\012\013\014\015\016\017\018\019\01a\01b\01c\01d\01e\01f
!"#$%&'(

Re: gitweb not friendly to firefox revived

[Jakub Narebski]

On Sat, 14 Aug 2010, Stephen Boyd wrote:
>   On 08/03/2010 02:50 PM, Jakub Narebski wrote:
> > +
> > +	my $title = to_utf8("Search for commits $performed by $author");
> > +	$title =~ s/[[:cntrl:]]/?/g;
> > +
> >
> 
> Isn't it possible that other data coming from git could have escape
> characters in them such as the commit subject line? In which case this
> same bug would occur?  
> 
> Therefore isn't it better to strip out control characters (that's what
> this patch is doing right?) in esc_html? 

First, esc_html and esc_path *do* escape control characters using
either control escape characters (e.g. "\n" for LF), or escaped octal 
representation (e.g. "\001").

Second, it does not help with contents of *attributes* of HTML tag 
elements (like e.g. 'title' attribute) when those elements are 
generated using CGI (e.g. $cgi->a({..., -title => ...},esc_html(...))).
Unfortunately (older?) CGI.pm does not escape control characters, and
we cannot do escape ourselves because it would lead to double escaping.


The problem with Firefox is that in strict XHTML conformance model 
(XHTML DTD and application/xhtml+xml mimetype) it *enforces* that XML
is well formed, which includes lack of control characters, instead of
silently allowing it like in more loose HTML mode.

-- 
Jakub Narebski
Poland

Re: gitweb not friendly to firefox revived

[Jakub Narebski]

On Sat, 14 Aug 2010, Ævar Arnfjörð Bjarmason wrote:
> 2010/8/14 Stephen Boyd <bebarino@gmail.com>:
>>  On 08/03/2010 02:50 PM, Jakub Narebski wrote:
>>>
>>> +
>>> +       my $title = to_utf8("Search for commits $performed by $author");
>>> +       $title =~ s/[[:cntrl:]]/?/g;
>>> +
>>>
>>
>> Isn't it possible that other data coming from git could have escape
>> characters in them such as the commit subject line? In which case this same
>> bug would occur?
>>
>> Therefore isn't it better to strip out control characters (that's what this
>> patch is doing right?) in esc_html?
> 
> I don't think stripping them out is the right thing either, hiding
> from you that something is Really Wrong (binary garbage in patches)
> isn't good.
> 
> Something like this would be better:
> 
>     s/([[:cntrl:]])/sprintf("\\%03x", ord $1)/ge

Or 
      s|([[:cntrl:]])|quot_cec($1)|eg;

But is it worth it?  This is about _title_ attribute, shown only on 
mouseover (mouse hover).


But perhaps it would be worth it to add 'prep_attr' and 'esc_attr'
functions, though esc_html can be used in those places where esc_attr
would be needed...

-- 
Jakub Narebski
Poland

Re: gitweb not friendly to firefox revived

[Uwe Kleine-König]

Hello,

just to stop this topic dying ...

On Sat, Aug 14, 2010 at 02:33:33PM +0200, Jakub Narebski wrote:
> On Sat, 14 Aug 2010, Ævar Arnfjörð Bjarmason wrote:
> > 2010/8/14 Stephen Boyd <bebarino@gmail.com>:
> >>  On 08/03/2010 02:50 PM, Jakub Narebski wrote:
> >>>
> >>> +
> >>> +       my $title = to_utf8("Search for commits $performed by $author");
> >>> +       $title =~ s/[[:cntrl:]]/?/g;
> >>> +
> >>>
> >>
> >> Isn't it possible that other data coming from git could have escape
> >> characters in them such as the commit subject line? In which case this same
> >> bug would occur?
> >>
> >> Therefore isn't it better to strip out control characters (that's what this
> >> patch is doing right?) in esc_html?
> > 
> > I don't think stripping them out is the right thing either, hiding
> > from you that something is Really Wrong (binary garbage in patches)
> > isn't good.
> > 
> > Something like this would be better:
> > 
> >     s/([[:cntrl:]])/sprintf("\\%03x", ord $1)/ge
> 
> Or 
>       s|([[:cntrl:]])|quot_cec($1)|eg;
> 
> But is it worth it?  This is about _title_ attribute, shown only on 
> mouseover (mouse hover).
> 
> 
> But perhaps it would be worth it to add 'prep_attr' and 'esc_attr'
> functions, though esc_html can be used in those places where esc_attr
> would be needed...
Is there something I can do to bring this forward?  (I assume this isn't
fixed yet, at least it isn't in Debian's 1.7.2.3 package.)

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |