From: Paul Gortmaker Date: Mon, 20 Apr 2009 13:29:15 GMT Subject: Re: [PATCH] gitweb: filter escapes from longer commit titles that break firefox Message-ID: <49EC78AB.6020009@windriver.com> In-Reply-To: Jakub Narebski wrote: > Paul Gortmaker writes: > > >> If there is a commit that ends in ^X and is longer in length than >> what will fit in title_short, then it doesn't get fed through >> esc_html() and so the ^X will appear as-is in the page source. >> >> When Firefox comes across this, it will fail to display the page, >> and only display a couple lines of error messages that read like: >> >> XML Parsing Error: not well-formed >> Location: http://git .... >> >> Signed-off-by: Paul Gortmaker >> > > This is an issue for when project doesn't follow sanity (control > characters in commit message) nor commit message conventions of git > (limiting length of first line of commit message to 60-70 characters). > I agree - the situation should be that it doesn't happen, but it can happen (and it did happen) that a novice, or a simple mistake ends up with such a commit. > But I do not think that the solution presented here is good solution > for this problem. chop_and_escape_str is meant as _output_ filter, > because it generates (can generate) fragment of HTML. It is not a > good solution to use it for shortening in intermediate representation > of %co{'title'}. > > And I think that issue might be a bug elsewhere in gitweb if we have > text output which is not passed through esc_html... or bug in CGI.pm > if the error is in not escaping of -title _attribute_ (attribute > escaping has slightly different rules than escaping HTML, and should > be done automatically by CGI.pm). > > > So thanks for noticing the issue, but NAK on the solution. > Fair enough -- I wasn't familiar with the code in there, and there wasn't really any indication that it was for output only. I can easily believe that there is a better place for it -- I just didn't see where any global esc_html filtering was taking place... Paul. > >> --- >> gitweb/gitweb.perl | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl >> index 33ef190..e686e82 100755 >> --- a/gitweb/gitweb.perl >> +++ b/gitweb/gitweb.perl >> @@ -2470,7 +2470,7 @@ sub parse_commit_text { >> foreach my $title (@commit_lines) { >> $title =~ s/^ //; >> if ($title ne "") { >> - $co{'title'} = chop_str($title, 80, 5); >> + $co{'title'} = chop_and_escape_str($title, 80, 5); >> # remove leading stuff of merges to make the interesting part visible >> if (length($title) > 50) { >> $title =~ s/^Automatic //; >> -- >> 1.6.2.3 >> >> > >