Re: [PATCH] gitweb: filter escapes from longer commit titles that break firefox
- From
- Paul Gortmaker <paul.gortmaker@windriver.com>
- Date
- Apr 20, 2009, 13:29 UTC
- Message-ID
- <49EC78AB.6020009@windriver.com>
- In-Reply-To
- <m3r5znpt5g.fsf@localhost.localdomain>
Jakub Narebski wrote:
Show 20 quoted lines
> Paul Gortmaker <paul.gortmaker@windriver.com> 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 <paul.gortmaker@windriver.com> >> > > 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.
Show 15 quoted lines
> 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.
Show 24 quoted lines
>
>> ---
>> 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
>>
>>
>
>