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

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

[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 <leoli@freescale.com>

---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 653ca3c..3a564d1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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;
 	}

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Li Yang <leoli@freescale.com> writes:

> 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 <leoli@freescale.com>

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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jakub Narebski]

On 3/6/07, Junio C Hamano <junkio@cox.net> wrote:
> Li Yang <leoli@freescale.com> writes:
>
> > 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 <leoli@freescale.com>
>
> 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.

-- 
Jakub Narebski

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jeff King]

On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:

> >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 --
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 653ca3c..5d1d8cf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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};

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:
>
>> >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.

>
> -- >8 --
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 653ca3c..5d1d8cf 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -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};

RE: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Li Yang-r58472]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Tuesday, March 06, 2007 5:39 PM
> To: Jakub Narebski
> Cc: Junio C Hamano; Li Yang-r58472; git@vger.kernel.org
> Subject: Re: [PATCH] gitweb: Change to use explicitly function call
> cgi->escapHTML()
> 
> On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:
> 
> > >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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jeff King]

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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

"Li Yang-r58472" <LeoLi@freescale.com> writes:

>> -----Original Message-----
>> From: Jeff King [mailto:peff@peff.net]
>> ...
>> 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).

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 653ca3c..9c4e060 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jeff King]

On Tue, Mar 06, 2007 at 02:53:07AM -0800, Junio C Hamano wrote:

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

> 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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 06, 2007 at 02:53:07AM -0800, Junio C Hamano wrote:
>
>> 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).
>
>> 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.

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jeff King]

On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:

> 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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:
>
>> 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.

RE: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Li Yang-r58472]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Tuesday, March 06, 2007 7:02 PM
> To: Junio C Hamano
> Cc: Li Yang-r58472; Jakub Narebski; git@vger.kernel.org
> Subject: Re: [PATCH] gitweb: Change to use explicitly function call
> cgi->escapHTML()
> 
> On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:
> 
> > 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).

I agree.

-Leo

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jeff King]

On Tue, Mar 06, 2007 at 03:05:55AM -0800, Junio C Hamano wrote:

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

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jakub Narebski]

Junio C Hamano wrote:

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

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>
>> 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.

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?

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Junio C Hamano wrote:
>>
>>> 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.
> 
> 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.
 
> 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

  <a href="URL">some <b>bold</b> text</a>

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.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

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

I trust a patch from you will follow shortly?

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

[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 <b>bold</b> text</a>
for example. So we have to esc_html (or esc_path) if needed.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> 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(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 653ca3c..ea58946 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -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);
-- 
1.5.0.2

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 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 <b>bold</b> text</a>
> for example. So we have to esc_html (or esc_path) if needed.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> 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.