gitweb: Change to use explicitly function call cgi->escapHTML()

PATCH In review Li Yang 4 participants 21 messages Jun 15, 2016

Share & tools

Li Yang

author

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

PATCH 1/1 gitweb: Change to use explicitly function call cgi->escapHTML()

Change to use explicitly function call cgi->escapHTML(). This fix the problem on some systems that escapeHTML() is not functioning, as default CGI is not setting 'escape' parameter.

Signed-off-by Li Yang <[email protected]>

---

gitweb/gitweb.perl +0 -0

index 653ca3c..3a564d1 100755
@@ -591,7 +591,7 @@ sub esc_html ($;%) {

	my %opts = @_;

	$str = to_utf8($str);
-	$str = escapeHTML($str);
+	$str = $cgi->escapeHTML($str);
	if ($opts{'-nbsp'}) {
		$str =~ s/ /&nbsp;/g;
	}
@@ -605,7 +605,7 @@ sub esc_path {
	my %opts = @_;

	$str = to_utf8($str);
-	$str = escapeHTML($str);
+	$str = $cgi->escapeHTML($str);
	if ($opts{'-nbsp'}) {
		$str =~ s/ /&nbsp;/g;
	}

Junio C Hamano

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

Quoted 5 lines · Li Yang

Change to use explicitly function call cgi->escapHTML(). This fix the problem on some systems that escapeHTML() is not functioning, as default CGI is not setting 'escape' parameter.
Signed-off-by Li Yang <[email protected]>

Regardless of the recent xhtml+html vs html discussion, I think this is probably a sane change. Comments?

Jakub Narebski

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

Quoted 10 lines · Junio C Hamano

Quoted 5 lines · Li Yang
Change to use explicitly function call cgi->escapHTML(). This fix the problem on some systems that escapeHTML() is not functioning, as default CGI is not setting 'escape' parameter.
Signed-off-by Li Yang <[email protected]>
Regardless of the recent xhtml+html vs html discussion, I think this is probably a sane change. Comments?

Good (although a bit magic) solution. Ack, FWIW.

Signature · 1 line

Jakub Narebski

Jeff King

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

Quoted 3 lines · Jakub Narebski

Quoted 2 lines
Regardless of the recent xhtml+html vs html discussion, I think this is probably a sane change. Comments?
Good (although a bit magic) solution. Ack, FWIW.

I think this should do the same, and is perhaps less magic (or maybe more, depending on your perspective).

-Peff

-- >8 --

gitweb/gitweb.perl +1 -0

index 653ca3c..5d1d8cf 100755
@@ -17,6 +17,7 @@ use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
 binmode STDOUT, ':utf8';
+CGI::autoEscape(1);
 
 BEGIN {
        CGI->compile() if $ENV{MOD_PERL};

Junio C Hamano

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

Quoted 10 lines · Jeff King

Quoted 3 lines · Jakub Narebski
Quoted 2 lines
Regardless of the recent xhtml+html vs html discussion, I think this is probably a sane change. Comments?
Good (although a bit magic) solution. Ack, FWIW.
I think this should do the same, and is perhaps less magic (or maybe more, depending on your perspective).
-Peff

Thanks. I tend to agree, as it does not depend on the reader knowing what magic $cgi default behaviour is by being expliicit.

Quoted 14 lines

-- >8 --

gitweb/gitweb.perl +1 -0

index 653ca3c..5d1d8cf 100755
@@ -17,6 +17,7 @@ use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
 binmode STDOUT, ':utf8';
+CGI::autoEscape(1);
 
 BEGIN {
        CGI->compile() if $ENV{MOD_PERL};

Li Yang-r58472

author

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

In reply to Jeff King

Quoted 8 lines

-----Original Message-----
From: Jeff King [mailto:[email protected]]
Sent: Tuesday, March 06, 2007 5:39 PM
To: Jakub Narebski
Cc: Junio C Hamano; Li Yang-r58472; [email protected]
Subject: Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

Quoted 8 lines

Quoted 3 lines · Jakub Narebski
Quoted 2 lines
Regardless of the recent xhtml+html vs html discussion, I think this is probably a sane change. Comments?
Good (although a bit magic) solution. Ack, FWIW.
I think this should do the same, and is perhaps less magic (or maybe more, depending on your perspective).

Yes, it also fixed the problem. I'm not very familiar with perl. Will CGI::autoEscape(1) change CGI action for other users of CGI module on the system? If so, maybe it will break other CGIs.

- Leo

Jeff King

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

Quoted 3 lines · On Tue, Mar 06, 2007 at 06:31:23PM +0800, Li Yang-r58472 wrote:

Yes, it also fixed the problem. I'm not very familiar with perl. Will CGI::autoEscape(1) change CGI action for other users of CGI module on the system? If so, maybe it will break other CGIs.

I don't know enough about mod_perl to say, but if all scripts share the package globals from CGI, then yes, you're affecting all other scripts. Without mod_perl, obviously you have no impact.

If it is the case, then your original fix is probably better.

-Peff

Junio C Hamano

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

Quoted 11 lines · Jeff King

Quoted 3 lines · On Tue, Mar 06, 2007 at 06:31:23PM +0800, Li Yang-r58472 wrote:
Yes, it also fixed the problem. I'm not very familiar with perl. Will CGI::autoEscape(1) change CGI action for other users of CGI module on the system? If so, maybe it will break other CGIs.
I don't know enough about mod_perl to say, but if all scripts share the package globals from CGI, then yes, you're affecting all other scripts. Without mod_perl, obviously you have no impact.
If it is the case, then your original fix is probably better.

But then you are letting _other_ mod_perl users to affect your behaviour, aren't you? "sub autoEscape" does this:

       sub autoEscape {
           my($self,$escape) = self_or_default(@_);
           my $d = $self->{'escape'};
           $self->{'escape'} = $escape;
           $d;
       }

If we worry about mod_perl (provided if $CGI::Q is shared across mod_perl users), I suspect we would need to be a bit more paranoid, perhaps like this, woudln't we?

---

gitweb/gitweb.perl +1 -0

index 653ca3c..9c4e060 100755
@@ -26,6 +26,7 @@ our $cgi = new CGI;
 our $version = "++GIT_VERSION++";
 our $my_url = $cgi->url();
 our $my_uri = $cgi->url(-absolute => 1);
+$cgi->autoEscape(1);
 
 # core git executable to use
 # this can just be "git" if your webserver has a sensible PATH

Jeff King

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

Quoted 2 lines · Junio C Hamano

But then you are letting _other_ mod_perl users to affect your behaviour, aren't you? "sub autoEscape" does this:

Yes (but I don't know how mod_perl works, and I haven't been able to find a simple answer by skimming the docs).

Quoted 5 lines

If we worry about mod_perl (provided if $CGI::Q is shared across mod_perl users), I suspect we would need to be a bit more paranoid, perhaps like this, woudln't we?
[...]
+$cgi->autoEscape(1);

That rebreaks the original problem, though. Calling escapeHTML doesn't look at $cgi, it looks at $Q (the "default" CGI object). I believe escape is _already_ set to 1 for $cgi (which is why the $cgi->escapeHTML patch worked).

-Peff

Junio C Hamano

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

Quoted 8 lines · Jeff King

Quoted 2 lines · Junio C Hamano
But then you are letting _other_ mod_perl users to affect your behaviour, aren't you? "sub autoEscape" does this:
Yes (but I don't know how mod_perl works, and I haven't been able to find a simple answer by skimming the docs).

Quoted 7 lines

Quoted 5 lines
If we worry about mod_perl (provided if $CGI::Q is shared across mod_perl users), I suspect we would need to be a bit more paranoid, perhaps like this, woudln't we?
[...]
+$cgi->autoEscape(1);
That rebreaks the original problem, though.

Sorry, what I meant to say was on top of Li's patch.

Jeff King

2016-06-15 22:42 UTC #10 lore raw

Quoted 1 line · Junio C Hamano

Sorry, what I meant to say was on top of Li's patch.

Ah, I understand your point now. Yes, if other mod_perl CGIs can impact the value, then we should definitely set it explicitly, as per your patch (and we should use Li's patch for safety, then, not mine).

-Peff

Junio C Hamano

2016-06-15 22:42 UTC #11 lore raw

Quoted 7 lines · Jeff King

Quoted 1 line · Junio C Hamano
Sorry, what I meant to say was on top of Li's patch.
Ah, I understand your point now. Yes, if other mod_perl CGIs can impact the value, then we should definitely set it explicitly, as per your patch (and we should use Li's patch for safety, then, not mine).

Reading "sub autoEscape", "sub escapeHTML" and "sub self_or_default" again, I think other people cannot affect the value of our $cgi->{'escape'} by calling autoEscape, so what I said is probably bogus. Let's use Li's original patch.

Jeff King

2016-06-15 22:42 UTC #12 lore raw

Quoted 8 lines · Junio C Hamano

Quoted 3 lines
Ah, I understand your point now. Yes, if other mod_perl CGIs can impact the value, then we should definitely set it explicitly, as per your patch (and we should use Li's patch for safety, then, not mine).
Reading "sub autoEscape", "sub escapeHTML" and "sub self_or_default" again, I think other people cannot affect the value of our $cgi->{'escape'} by calling autoEscape, so what I said is probably bogus. Let's use Li's original patch.

Er, sorry, yes, I just accidentally agreed with your bogosity (while figuring out what you originally meant, I forgot which CGI we were talking about!). So I agree, Li's original is sufficient. Sorry for the noise. :)

-Peff

Li Yang-r58472

author

2016-06-15 22:42 UTC #13 lore raw

In reply to Jeff King

Quoted 8 lines

-----Original Message-----
From: Jeff King [mailto:[email protected]]
Sent: Tuesday, March 06, 2007 7:02 PM
To: Junio C Hamano
Cc: Li Yang-r58472; Jakub Narebski; [email protected]
Subject: Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

Quoted 5 lines

Quoted 1 line · Junio C Hamano
Sorry, what I meant to say was on top of Li's patch.
Ah, I understand your point now. Yes, if other mod_perl CGIs can

impact

Quoted 2 lines

the value, then we should definitely set it explicitly, as per your patch (and we should use Li's patch for safety, then, not mine).

I agree.

-Leo

Junio C Hamano

2016-06-15 22:42 UTC #14 lore raw

In reply to Li Yang-r58472

Quoted 9 lines · "Li Yang-r58472" <[email protected]> writes:

Quoted 5 lines
-----Original Message-----
From: Jeff King [mailto:[email protected]]
...
I think this should do the same, and is perhaps less magic (or maybe more, depending on your perspective).
Yes, it also fixed the problem. I'm not very familiar with perl. Will CGI::autoEscape(1) change CGI action for other users of CGI module on the system? If so, maybe it will break other CGIs.

By "other CGIs" if you mean other independent CGI scripts that do not have anything to do with gitweb, then I do think there is no need to worry.

What I'd be worried about more, however, is if all the callers of esc_html and esc_path are really expecting the full quoting done by CGI::autoEscape(1). I think we had some discussion on the path quoting when we introduced quot_cec and quot_upr, but do not recall the details. For example, many places esc_html() is used as the body of <a ...>$here</a> but some places it is used as

    $cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))

which would be the same as:

    print '<a title="' . esc_html($fullname) . '">' . esc_path($dir) . '</a>';

which may or may not be right (I do not know offhand).

Speaking of -title, I see "sub git_project_list_body" does this:

    $cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));

which seems inconsistent with the earlier quoted $fullname handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'} is not, which I find highly unlikely).

Jakub Narebski

2016-06-15 22:42 UTC #15 lore raw

Quoted 7 lines · Junio C Hamano

Speaking of -title, I see "sub git_project_list_body" does this:
$cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));
which seems inconsistent with the earlier quoted $fullname handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'} is not, which I find highly unlikely).

CGI::a() subroutine automatically quotes properly _attribute_ values, but it does not (and it should not) quote _contents_ of a tag.

So the above code is correct.

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:42 UTC #16 lore raw

Quoted 10 lines · Jakub Narebski

Quoted 7 lines · Junio C Hamano
Speaking of -title, I see "sub git_project_list_body" does this:
$cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));
which seems inconsistent with the earlier quoted $fullname handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'} is not, which I find highly unlikely).

Quoted 4 lines

CGI::a() subroutine automatically quotes properly _attribute_ values, but it does not (and it should not) quote _contents_ of a tag.
So the above code is correct.

Sorry, you lost me... I am wondering what you mean by "automatically". Do you mean 'always'?

And if that is the case, shouldn't we drop esc_html() around $fullname here?

... For example, many places esc_html()
is used as the body of <a ...>$here</a> but some places it is used as

$cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))

as we do not have it around $pr->{'descr_long'} in the above?

Jakub Narebski

2016-06-15 22:42 UTC #17 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 10 lines · Jakub Narebski
Quoted 7 lines · Junio C Hamano
Speaking of -title, I see "sub git_project_list_body" does this:
$cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));
which seems inconsistent with the earlier quoted $fullname handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'} is not, which I find highly unlikely).

Quoted 7 lines

Quoted 4 lines
CGI::a() subroutine automatically quotes properly _attribute_ values, but it does not (and it should not) quote _contents_ of a tag.
So the above code is correct.
Sorry, you lost me... I am wondering what you mean by "automatically". Do you mean 'always'?

Yes, I mean that CGI::a() does quoting _of attributes_, always.

Quoted 10 lines

And if that is the case, shouldn't we drop esc_html() around $fullname here?
... For example, many places esc_html()
is used as the body of <a ...>$here</a> but some places it is used as
$cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))
as we do not have it around $pr->{'descr_long'} in the above?

The above is wrong, thrice. First, it should be esc_path($fullname). Second, rules for escaping attribute values are different from escaping HTML. Third, CGI::a() does escaping of attribute values.

Explanation:

$cgi->a({ ... -attribute => atribute_value }, tag_contents)

is translated to

<a ... attribute="attribute_value">tag_contents</a>

The rules for escaping attribute values (which are string contents) are different. For example you have to take care about escaping embedded '"' and "'" characters; CGI::a() does that for us automatically.

CGI::a() cannot HTML escape tag contents automatically; we might want to write

for example. Soe we have to esc_html (or esc_path) if needed.

In short: escape tag contents if needed, do not escape attrbure values.

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:42 UTC #18 lore raw

Quoted 1 line · Jakub Narebski

In short: escape tag contents if needed, do not escape attrbure values.

I trust a patch from you will follow shortly?

Jakub Narebski

2016-06-15 22:42 UTC #19 lore raw

PATCH 1/1 gitweb: Don't escape attributes in CGI.pm HTML methods

There is no need to escape HTML tag's attributes in CGI.pm HTML methods (like CGI::a()), because CGI.pm does attribute escaping automatically.

Explanation:
$cgi->a({ ... -attribute => atribute_value }, tag_contents)
is translated to
<a ... attribute="attribute_value">tag_contents</a>
The rules for escaping attribute values (which are string contents) are different. For example you have to take care about escaping embedded '"' and "'" characters; CGI::a() does that for us automatically.

CGI::a() cannot HTML escape tag contents automatically; we might want to write
<a href="URL">some bold text</a>
for example. So we have to esc_html (or esc_path) if needed.

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

---

Quoted 5 lines · Junio C Hamano

Quoted 1 line · Jakub Narebski
In short: escape tag contents if needed, do not escape attrbure values.
I trust a patch from you will follow shortly?

Here it is. I hope I found everything.

Commit message is bit long, so you can cut it to first sentence only (or even only to title/subject).

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

gitweb/gitweb.perl +3 -3

index 653ca3c..ea58946 100755
@@ -1974,17 +1974,17 @@ sub git_print_page_path {
 			$fullname .= ($fullname ? '/' : '') . $dir;
 			print $cgi->a({-href => href(action=>"tree", file_name=>$fullname,
 			                             hash_base=>$hb),
-			              -title => esc_html($fullname)}, esc_path($dir));
+			              -title => $fullname}, esc_path($dir));
 			print " / ";
 		}
 		if (defined $type && $type eq 'blob') {
 			print $cgi->a({-href => href(action=>"blob_plain", file_name=>$file_name,
 			                             hash_base=>$hb),
-			              -title => esc_html($name)}, esc_path($basename));
+			              -title => $name}, esc_path($basename));
 		} elsif (defined $type && $type eq 'tree') {
 			print $cgi->a({-href => href(action=>"tree", file_name=>$file_name,
 			                             hash_base=>$hb),
-			              -title => esc_html($name)}, esc_path($basename));
+			              -title => $name}, esc_path($basename));
 			print " / ";
 		} else {
 			print esc_path($basename);

Signature · 1 line

1.5.0.2

Junio C Hamano

2016-06-15 22:42 UTC #20 lore raw

Re: [PATCH] gitweb: Don't escape attributes in CGI.pm HTML methods

Quoted 22 lines · Jakub Narebski

There is no need to escape HTML tag's attributes in CGI.pm HTML methods (like CGI::a()), because CGI.pm does attribute escaping automatically.
Explanation:
$cgi->a({ ... -attribute => atribute_value }, tag_contents)
is translated to
<a ... attribute="attribute_value">tag_contents</a>
The rules for escaping attribute values (which are string contents) are different. For example you have to take care about escaping embedded '"' and "'" characters; CGI::a() does that for us automatically.
CGI::a() cannot HTML escape tag contents automatically; we might want to write
<a href="URL">some bold text</a>
for example. So we have to esc_html (or esc_path) if needed.
Signed-off-by Jakub Narebski <[email protected]>
---
Quoted 2 lines · Junio C Hamano
Jakub Narebski <[email protected]> writes:

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
In short: escape tag contents if needed, do not escape attrbure values.
I trust a patch from you will follow shortly?
Here it is. I hope I found everything.
Commit message is bit long, so you can cut it to first sentence only (or even only to title/subject).

Thanks. I think your explanation in the log message has the right amount of details and keeping it there would help people who would want to later touch the code.