gitweb: highlight: strip non-printable characters via col(1)

PATCH In review Christopher M. Fuhrman 3 participants 10 messages Jun 15, 2016

lore

Share & tools

lore

Christopher M. Fuhrman

author

2016-06-15 22:51 UTC #0 lore raw

PATCH 1/1 gitweb: highlight: strip non-printable characters via col(1)

From: "Christopher M. Fuhrman" <[email protected]>

The current code, as is, passes control characters, such as form-feed (^L) to highlight which then passes it through to the browser. This will cause the browser to display one of the following warnings:

Safari v5.1 (6534.50) & Google Chrome v13.0.782.112:

This page contains the following errors:

error on line 657 at column 38: PCDATA invalid Char value 12 Below is a rendering of the page up to the first error.

Mozilla Firefox 3.6.19 & Mozilla Firefox 5.0:

XML Parsing Error: not well-formed
Location:
http://path/to/git/repo/blah/blah

Both errors were generated by gitweb.perl v1.7.3.4 w/ highlight 2.7 using arch/ia64/kernel/unwind.c from the Linux kernel.

Strip non-printable control-characters by piping the output produced by git-cat-file(1) to col(1) as follows:

git cat-file blob deadbeef314159 | col -bx | highlight <args>

Note usage of the '-x' option which tells col(1) to output multiple spaces instead of tabs.

Tested under OpenSuSE 11.4 & NetBSD 5.1 using perl 5.12.3 and perl 5.12.2 respectively using Safari, Firefox, and Google Chrome.

Signed-off-by Christopher M. Fuhrman <[email protected]>

---
Howdy,

I haven't gotten any responses to my patch for a while, so I am now submitting this for general inclusion into git. Please note that this is based off the "maint" branch per Documentation/SubmittingPatches

For an example of this bug in action, see:

* http://git.fuhrbear.com/~cfuhrman/?p=linux/.git;a=blob;f=arch/alpha/kernel/core_titan.c;h=219bf271c0ba2e5f2d668af707df57fbbd00ccfd;hb=HEAD
* http://git.fuhrbear.com/~cfuhrman/?p=linux/.git;a=blob;f=arch/ia64/kernel/unwind.c;h=fed6afa2e8a9014e65229e51e64fa4b1c13cc284;hb=HEAD

WRT the col(1) command, I've verified that the binary is installed in /usr/bin on OpenSuSE, NetBSD, OpenBSD, Solaris 10, and AIX. This patch assumes that /usr/bin is in $PATH.

Cheers!

 gitweb/gitweb.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +1 -0

index 50a835a..4c68165 100755
@@ -3656,6 +3656,7 @@ sub run_highlighter {
 
 	close $fd;
 	open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
+	          "col -bx | ".
 	          quote_command($highlight_bin).
 	          " --replace-tabs=8 --fragment --syntax $syntax |"
 		or die_error(500, "Couldn't open file or run syntax highlighter");

Signature · 1 line

1.7.5.4

Junio C Hamano

2016-06-15 22:51 UTC #1 lore raw

Quoted 7 lines · Christopher M. Fuhrman

Strip non-printable control-characters by piping the output produced by git-cat-file(1) to col(1) as follows:
git cat-file blob deadbeef314159 | col -bx | highlight <args>
Note usage of the '-x' option which tells col(1) to output multiple spaces instead of tabs.

Are all implementations of col known to correctly handle bytes with their highest bit on, without mistaking them with unknown control sequences? Has the code updated by your patch been tested with non-ASCII payload, at least with UTF-8 outside US-ASCII?

In what locale does the code updated by your patch run under, and would the use of "col" affected by the choice of the locale in a negative way?

For example, here is what I get on my box:

    $ LANG=C LC_ALL=C col -bx <t/t3902-quoted.sh ; echo $?
    col: Invalid or incomplete multibyte or wide character
    1

that makes me ask you these questions.

Quoted 2 lines

I haven't gotten any responses to my patch for a while, so I am now submitting this for general inclusion into git.

Unfortunately, no news is not good news around here, and that is why I am asking you the above questions.

Thanks.

Jakub Narebski

2016-06-15 22:51 UTC #2 lore raw

In reply to Christopher M. Fuhrman

Quoted 20 lines · Christopher M. Fuhrman

The current code, as is, passes control characters, such as form-feed (^L) to highlight which then passes it through to the browser. This will cause the browser to display one of the following warnings:
Safari v5.1 (6534.50) & Google Chrome v13.0.782.112:
This page contains the following errors:
error on line 657 at column 38: PCDATA invalid Char value 12 Below is a rendering of the page up to the first error.
Mozilla Firefox 3.6.19 & Mozilla Firefox 5.0:
XML Parsing Error: not well-formed
Location:
http://path/to/git/repo/blah/blah
Both errors were generated by gitweb.perl v1.7.3.4 w/ highlight 2.7 using arch/ia64/kernel/unwind.c from the Linux kernel.

Quoted 7 lines

Strip non-printable control-characters by piping the output produced by git-cat-file(1) to col(1) as follows:
git cat-file blob deadbeef314159 | col -bx | highlight <args>
Note usage of the '-x' option which tells col(1) to output multiple spaces instead of tabs.

Why use external program (which ming be not installed, or might not strip control-characters), instead of making gitweb sanitize highlighter output itself. Something like the patch below (which additionally shows where there are control characters):

-- >8 --

gitweb/gitweb.perl +13 -1

index 7cf12af..192db2c 100755
@@ -1517,6 +1517,17 @@ sub esc_path {
 	return $str;
 }
 
+# Sanitize for use in XHTML + application/xml+xhtml
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;
+
+	$str = to_utf8($str);
+	$str =~ s|([[:cntrl:]])|quot_cec($1)|eg;
+	return $str;
+}
+
 # Make control characters "printable", using character escape codes (CEC)
 sub quot_cec {
 	my $cntrl = shift;

@@ -6546,7 +6557,8 @@ sub git_blob {
 			$nr++;
 			$line = untabify($line);
 			printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
-			       $nr, esc_attr(href(-replay => 1)), $nr, $nr, $syntax ? to_utf8($line) : esc_html($line, -nbsp=>1);
+			       $nr, esc_attr(href(-replay => 1)), $nr, $nr,
+			       $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
 		}
 	}
 	close $fd

-- 8< --

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:51 UTC #3 lore raw

Quoted 4 lines · Jakub Narebski

Why use external program (which ming be not installed, or might not strip control-characters), instead of making gitweb sanitize highlighter output itself. Something like the patch below (which additionally shows where there are control characters):

I agree that that would be a more sensible approach. What does your sample code below do to a HT by the way?

Quoted 35 lines

-- >8 --

gitweb/gitweb.perl +13 -2

index 7cf12af..192db2c 100755
@@ -1517,6 +1517,17 @@ sub esc_path {
 	return $str;
 }
 
+# Sanitize for use in XHTML + application/xml+xhtml
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;
+
+	$str = to_utf8($str);
+	$str =~ s|([[:cntrl:]])|quot_cec($1)|eg;
+	return $str;
+}
+
 # Make control characters "printable", using character escape codes (CEC)
 sub quot_cec {
 	my $cntrl = shift;

@@ -6546,7 +6557,8 @@ sub git_blob {
 			$nr++;
 			$line = untabify($line);
 			printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
-			       $nr, esc_attr(href(-replay => 1)), $nr, $nr, $syntax ? to_utf8($line) : esc_html($line, -nbsp=>1);
+			       $nr, esc_attr(href(-replay => 1)), $nr, $nr,
+			       $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
 		}
 	}
 	close $fd
 
-- 8< --

Jakub Narebski

2016-06-15 22:51 UTC #4 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 4 lines · Jakub Narebski
Why use external program (which ming be not installed, or might not strip control-characters), instead of making gitweb sanitize highlighter output itself. Something like the patch below (which additionally shows where there are control characters):
I agree that that would be a more sensible approach. What does your sample code below do to a HT by the way?

Actually the line earlier

$line = untabify($line);

replaces HT ("\t") with spaces.

Quoted 9 lines

-- >8 --

gitweb/gitweb.perl +0 -0

index 7cf12af..192db2c 100755
@@ -1517,6 +1517,17 @@ sub esc_path {
 	return $str;
 }

Quoted 10 lines

gitweb/gitweb.perl sub esc_path { · line 1529

Quoted 10 lines

gitweb/gitweb.perl +10 -0

+# Sanitize for use in XHTML + application/xml+xhtml
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;
+
+	$str = to_utf8($str);
+	$str =~ s|([[:cntrl:]])|quot_cec($1)|eg;
+	return $str;
+}

Anyway, it could well be

+ $str =~ s|([[:cntrl:]])|(($1 ne "\t") ? quot_cec($1) : $1)|eg;
+ return $str;

like in esc_html rather than like in esc_path.

Quoted 3 lines

gitweb/gitweb.perl sub git_blob { · line 6558
Quoted 3 lines
gitweb/gitweb.perl +0 -0
@@ -6546,7 +6557,8 @@ sub git_blob {
 			$nr++;
 			$line = untabify($line);

                        ^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 7 lines

gitweb/gitweb.perl +2 -1

 			printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
-			       $nr, esc_attr(href(-replay => 1)), $nr, $nr, $syntax ? to_utf8($line) : esc_html($line, -nbsp=>1);
+			       $nr, esc_attr(href(-replay => 1)), $nr, $nr,
+			       $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
 		}
 	}
 	close $fd

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

2016-06-15 22:52 UTC #5 lore raw

PATCH 1/1 gitweb: Strip non-printable characters from syntax highlighter output

The current code, as is, passes control characters, such as form-feed (^L) to highlight which then passes it through to the browser. User agents (web browsers) that support 'application/xhtml+xml' usually require that web pages declared as XHTML and with this mimetype are well-formed XML. Unescaped control characters cannot appear within a contents of a valid XML document.

This will cause the browser to display one of the following warnings:

* Safari v5.1 (6534.50) & Google Chrome v13.0.782.112:

This page contains the following errors:

error on line 657 at column 38: PCDATA invalid Char value 12 Below is a rendering of the page up to the first error.

* Mozilla Firefox 3.6.19 & Mozilla Firefox 5.0:

XML Parsing Error: not well-formed
Location:
http://path/to/git/repo/blah/blah

Both errors were generated by gitweb.perl v1.7.3.4 w/ highlight 2.7 using arch/ia64/kernel/unwind.c from the Linux kernel.

When syntax highlighter is not used, control characters are replaced by esc_html(), but with syntax highlighter they were passed through to browser (to_utf8() doesn't remove control characters).

Introduce sanitize() subroutine which strips forbidden characters, but does not perform HTML escaping, and use it in git_blob() to sanitize syntax highlighter output for XHTML.

Note that excluding "\t" (U+0009), "\n" (U+000A) and "\r" (U+000D) is not strictly necessary, atleast for currently the only callsite: "\t" tabs are replaced by spaces by untabify(), "\n" is stripped from each line before processing it, and replacing "\r" could be considered improvement.

Originally-by Christopher M. Fuhrman <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
The commit message is from Christopher, but I have replaced his solution of stripping non-printable characters via col(1) program by having gitweb strip characters not allowed in XML.

Christopher, could you check that it fixes your issue?

 gitweb/gitweb.perl |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +13 -1

index 70a576a..c28b847 100755
@@ -1517,6 +1517,17 @@ sub esc_path {
 	return $str;
 }
 
+# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;
+
+	$str = to_utf8($str);
+	$str =~ s|([[:cntrl:]])|($1 =~ /[\t\n\r]/ ? $1 : quot_cec($1))|eg;
+	return $str;
+}
+
 # Make control characters "printable", using character escape codes (CEC)
 sub quot_cec {
 	my $cntrl = shift;

@@ -6484,7 +6495,8 @@ sub git_blob {
 			$nr++;
 			$line = untabify($line);
 			printf qq!<div class="pre"><a id="l%i" href="%s#l%i" class="linenr">%4i</a> %s</div>\n!,
-			       $nr, esc_attr(href(-replay => 1)), $nr, $nr, $syntax ? to_utf8($line) : esc_html($line, -nbsp=>1);
+			       $nr, esc_attr(href(-replay => 1)), $nr, $nr,
+			       $syntax ? sanitize($line) : esc_html($line, -nbsp=>1);
 		}
 	}
 	close $fd

Signature · 1 line

1.7.6

Junio C Hamano

2016-06-15 22:52 UTC #6 lore raw

Re: [PATCH] gitweb: Strip non-printable characters from syntax highlighter output

Quoted 5 lines · Jakub Narebski

The commit message is from Christopher, but I have replaced his solution of stripping non-printable characters via col(1) program by having gitweb strip characters not allowed in XML.
Christopher, could you check that it fixes your issue?

Thanks.

Micronit:

gitweb/gitweb.perl sub esc_path { · line 1524

Quoted 5 lines

gitweb/gitweb.perl +5 -0

+# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;

Given that the _whole_ point of this subroutine is to make $str safe for printing, wouldn't you want to either (1) die, declaring that feeding an undef to this subroutine is a programming error, or (2) return an empty string?

Given that the input to this function is from the result of feeding $line to untabify, which relies on $line being defined, and that $line comes from "while (my $line = <$fd>)" (and then chomp $line), it may be Ok for this subroutine to make the same assumption as untabify makes.

Jakub Narebski

2016-06-15 22:52 UTC #7 lore raw

Re: [PATCH] gitweb: Strip non-printable characters from syntax highlighter output

Quoted 4 lines · Junio C Hamano

Jakub Narebski <[email protected]> writes:
Micronit:

Quoted 10 lines

gitweb/gitweb.perl sub esc_path { · line 1524
Quoted 5 lines
gitweb/gitweb.perl +5 -0
+# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
+sub sanitize {
+	my $str = shift;
+
+	return undef unless defined $str;
Given that the _whole_ point of this subroutine is to make $str safe for printing, wouldn't you want to either (1) die, declaring that feeding an undef to this subroutine is a programming error, or (2) return an empty string?

Well, that

	return undef unless defined $str;

line is copy'n'paste (as is most of sanitize() body) from esc_html(). This line was added in 1df4876 (gitweb: Protect escaping functions against calling on undef, 2010-02-07) with the following explanation

This is a bit of future-proofing esc_html and friends: when called with undefined value they would now would return undef... which would probably mean that error would still occur, but closer to the source of problem.

This means that we can safely use
esc_html(shift) || "Internal Server Error"
in die_error() instead of
esc_html(shift || "Internal Server Error")

So actually now I see that while this line is good to have in esc_html(), it is not really necessary in sanitize().

But anyway we don't want to replace undef with an empty string; undef is (usually) an error, and we want to catch it, not to hide it.

Quoted 4 lines

Given that the input to this function is from the result of feeding $line to untabify, which relies on $line being defined, and that $line comes from "while (my $line = <$fd>)" (and then chomp $line), it may be Ok for this subroutine to make the same assumption as untabify makes.

Right.

Passing undef to sanitize() is usually an error, and we don't want to hide it. We want for gitweb test to detect it.

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:52 UTC #8 lore raw

Re: [PATCH] gitweb: Strip non-printable characters from syntax highlighter output

Quoted 5 lines · Jakub Narebski

So actually now I see that while this line is good to have in esc_html(), it is not really necessary in sanitize().
But anyway we don't want to replace undef with an empty string; undef is (usually) an error, and we want to catch it, not to hide it.

Heh, get off your high horse---whoever wrote such a caller that calls the subroutine and uses its result without checking it against undef is not qualified to make such a statement. I do not think letting "perl -w" notice and complain about an attempt to concatenate undef with string counts as "catching" it.

Christopher M. Fuhrman

author

2016-06-15 22:52 UTC #9 lore raw

Re: [PATCH] gitweb: Strip non-printable characters from syntax highlighter output

In reply to Jakub Narebski

Howdy,

Quoted 5 lines · Jakub Narebski

The commit message is from Christopher, but I have replaced his solution of stripping non-printable characters via col(1) program by having gitweb strip characters not allowed in XML.
Christopher, could you check that it fixes your issue?

After applying the patch, I tested it successfully against the following files:

* linux.git : arch/ia64/kernel/unwind.c
* git.git : t/t3902-quoted.sh

Furthermore, I'm pleased to report that non en_US.UTF8 characters (e.g., Chinese hanzi) as found in t3902-quoted.sh are displayed properly when highlight is enabled.

Tested Web Browsers:

* Safari (5.1 (6534.50)
* Firefox 6.0.2 under Mac OS X Snow Leopard
* Google Chrome 13.0.782.220 under OpenSuSE 11.4

Quoted 4 lines

 gitweb/gitweb.perl |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

Cheers!

Signature · 2 lines

Chris Fuhrman
[email protected]