Re: [PATCH] gitweb: filter escapes from longer commit titles that break firefox

[Paul Gortmaker <paul.gortmaker@windriver.com>]

Jakub Narebski wrote:
> 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. 

> 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
>>
>>     
>
>