# gitweb not friendly to firefox revived
12 messages from 2010-08-01 to 2010-09-07. Participants: Uwe Kleine-König, Ævar Arnfjörð Bjarmason, Jakub Narebski, Stephen Boyd.
Thread: https://gitlist.dev/t/24601
## Uwe Kleine-König, 2010-08-01 19:51
Subject: gitweb not friendly to firefox revived
Message-ID: <20100801195138.GA1980@pengutronix.de>
URL: https://gitlist.dev/e/20100801195138.GA1980%40pengutronix.de
```
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:
| YOSHIFUJI Hideaki... | [PATCH] IPV6: XFRM: Fix decoding session with preceding... |
--------------------------------------------------------------------------------^
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/ |
```
## Ævar Arnfjörð Bjarmason, 2010-08-01 20:15
Subject: Re: gitweb not friendly to firefox revived
Message-ID:
URL: https://gitlist.dev/e/AANLkTimZ_DxO_TGnDBwRnurcdFuct-t509F0%2BDRhHo%2BB%40mail.gmail.com
In-Reply-To: <20100801195138.GA1980@pengutronix.de>
```
2010/8/1 Uwe Kleine-König :
> 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.
```
## Jakub Narebski, 2010-08-01 20:26
Subject: Re: gitweb not friendly to firefox revived
Message-ID:
URL: https://gitlist.dev/e/m3lj8qozan.fsf%40localhost.localdomain
In-Reply-To: <20100801195138.GA1980@pengutronix.de>
```
Uwe Kleine-König 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:
> YOSHIFUJI Hideaki... | [PATCH] IPV6: XFRM: Fix decoding session with preceding... |
> --------------------------------------------------------------------------------^
>
> 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
```
## Uwe Kleine-König, 2010-08-02 05:31
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <20100802053115.GB8538@pengutronix.de>
URL: https://gitlist.dev/e/20100802053115.GB8538%40pengutronix.de
In-Reply-To:
```
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/ |
```
## Uwe Kleine-König, 2010-08-03 21:07
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <20100803210730.GA1254@pengutronix.de>
URL: https://gitlist.dev/e/20100803210730.GA1254%40pengutronix.de
In-Reply-To:
```
On Sun, Aug 01, 2010 at 01:26:16PM -0700, Jakub Narebski wrote:
> Uwe Kleine-König 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:
> > YOSHIFUJI Hideaki... | [PATCH] IPV6: XFRM: Fix decoding session with preceding... |
> > --------------------------------------------------------------------------------^
> >
> > 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:
\e$B5HF#1QL@\e(B" class="list" href="/?p=.git;a=search;h=f66ab685594d49e570b2176cfa20b03360e9a6e9;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author">YOSHIFUJI Hideaki...
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
```
## Jakub Narebski, 2010-08-03 21:50
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <201008032350.40117.jnareb@gmail.com>
URL: https://gitlist.dev/e/201008032350.40117.jnareb%40gmail.com
In-Reply-To: <20100803210730.GA1254@pengutronix.de>
```
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 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:
> > > YOSHIFUJI Hideaki... | [PATCH] IPV6: XFRM: Fix decoding session with preceding... |
> > > --------------------------------------------------------------------------------^
> > >
> > > 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:
>
> \e$B5HF#1QL@\e(B" class="list" href="/?p=.git;a=search;h=f66ab685594d49e570b2176cfa20b03360e9a6e9;s=YOSHIFUJI+Hideaki+/+%1B%24B5HF%231QL@%1B(B;st=author">YOSHIFUJI Hideaki...
As you can see the HTML code generated with esc_html solution is way wrong
because of embedded '\e' 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
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
---
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
```
## Uwe Kleine-König, 2010-08-12 09:23
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <20100812092356.GC9763@pengutronix.de>
URL: https://gitlist.dev/e/20100812092356.GC9763%40pengutronix.de
In-Reply-To: <201008032350.40117.jnareb@gmail.com>
```
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
> 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
> ---
> 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/ |
```
## Stephen Boyd, 2010-08-14 10:33
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <4C6670EE.3060806@gmail.com>
URL: https://gitlist.dev/e/4C6670EE.3060806%40gmail.com
In-Reply-To: <201008032350.40117.jnareb@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?
```
## Ævar Arnfjörð Bjarmason, 2010-08-14 10:48
Subject: Re: gitweb not friendly to firefox revived
Message-ID:
URL: https://gitlist.dev/e/AANLkTimR9Wkz%2BYOchyjvO8fgAwk5YsGtcpMOpyZY4Fjv%40mail.gmail.com
In-Reply-To: <4C6670EE.3060806@gmail.com>
```
2010/8/14 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?
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
!"#$%&'(
```
## Jakub Narebski, 2010-08-14 12:29
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <201008141429.36829.jnareb@gmail.com>
URL: https://gitlist.dev/e/201008141429.36829.jnareb%40gmail.com
In-Reply-To: <4C6670EE.3060806@gmail.com>
```
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
```
## Jakub Narebski, 2010-08-14 12:33
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <201008141433.35451.jnareb@gmail.com>
URL: https://gitlist.dev/e/201008141433.35451.jnareb%40gmail.com
In-Reply-To:
```
On Sat, 14 Aug 2010, Ævar Arnfjörð Bjarmason wrote:
> 2010/8/14 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?
>
> 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
```
## Uwe Kleine-König, 2010-09-07 08:22
Subject: Re: gitweb not friendly to firefox revived
Message-ID: <20100907082253.GA16198@pengutronix.de>
URL: https://gitlist.dev/e/20100907082253.GA16198%40pengutronix.de
In-Reply-To: <201008141433.35451.jnareb@gmail.com>
```
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 :
> >> 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/ |
```