Re: [PATCH] Do not chop HTML tags in commit search result

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2008, 19:43 UTC
Message-ID: <7vbq6kprql.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <ae63f8b50802130937mddf9df9re2a95bee44661ee3@mail.gmail.com>

"Jean-Baptiste Quenot" <jbq@caraldi.com> writes:

> ... I encountered an annoying bug
> with gitweb 1.5.4.1, when searching for commits, if the search string
> is too long, the generated HTML is munged leading to an ill-formed
> XHTML document.

Show 13 quoted lines

> Here is the patch, hope it helps:
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ae2d057..2c0b990 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3780,7 +3780,10 @@ sub git_search_grep_body {
>                                 my $trail = esc_html($3) || "";
>                                 $trail = chop_str($trail, 30, 10);
> ...
>                                 my $text = "$lead<span
> class=\"match\">$match</span>$trail";
> -                               print chop_str($text, 80, 5) . "<br/>\n";

I think esc_html() and chop_str() are backwards here. If $3 is overlong it is cut in the middle of some markup. Even though chop_str() claims to be "HTML aware", I do not think it is. It seems to know about "&entities;" but not mark-ups.

There are quite a many instances of esc_html() first then chop_str() in that function, and I think they all deserve to be fixed.

	my $comment = $co{'comment'};
	foreach my $line (@$comment) {
		if ($line =~ m/^(.*)($search_regexp)(.*)$/i) {
			my $lead = esc_html($1) || "";
			$lead = chop_str($lead, 30, 10);
			my $match = esc_html($2) || "";
			my $trail = esc_html($3) || "";
			$trail = chop_str($trail, 30, 10);
			my $text = "$lead<span class=\"match\">$match</span>$trail";
			print chop_str($text, 80, 5) . "<br/>\n";
		}
	}

I think this is trying to fit the result on a line, showing the match sandwitched by not-too-long part taken from leading and trailing context ($lead and $trail can be chomped aggressively than $match). But $lead and $trail are escaped then chomped which is already wrong.

I think the body of that if() would be better written like this:

	my ($lead, $match, $trail) = ($1, $2, $3);
	$match = chop_str($match, 70, $slop); # in case it is very long...
	$contextlen = (80 - len($match)) / 2; # and the remainder...
        if ($contextlen > 30) { $contextlen = 30 }; # but not too much
        $trail = chop_str($trail, $contextlen, $slop);
        $lead = chop_str($lead, $contextlen, $slop);

	$lead = esc_html($lead);
	$match = esc_html($match);
	$trail = esc_html($trail);

        print "$lead<span ...>$match</span>$trail";