{"thread":{"id":"7101","subject":"[PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","startedAt":"2007-03-06T03:58:56Z","lastAt":"2007-03-07T01:40:28Z","messageCount":21,"participants":["Li Yang","Junio C Hamano","Jakub Narebski","Jeff King","Li Yang-r58472"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"36414","messageId":"45ECE700.8090205@freescale.com","threadId":"7101","inReplyTo":null,"subject":"[PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Li Yang","fromEmail":"leoli@freescale.com","sentAt":"2007-03-06T03:58:56Z","receivedAt":"2007-03-06T03:58:56Z","isPatch":true,"sender":{"key":"leoli@freescale.com","avatar":null},"body":"Change to use explicitly function call cgi->escapHTML().\nThis fix the problem on some systems that escapeHTML() is not\nfunctioning, as default CGI is not setting 'escape' parameter.\n\nSigned-off-by: Li Yang <leoli@freescale.com>\n\n---\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 653ca3c..3a564d1 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -591,7 +591,7 @@ sub esc_html ($;%) {\n \tmy %opts = @_;\n \n \t$str = to_utf8($str);\n-\t$str = escapeHTML($str);\n+\t$str = $cgi->escapeHTML($str);\n \tif ($opts{'-nbsp'}) {\n \t\t$str =~ s/ /&nbsp;/g;\n \t}\n@@ -605,7 +605,7 @@ sub esc_path {\n \tmy %opts = @_;\n \n \t$str = to_utf8($str);\n-\t$str = escapeHTML($str);\n+\t$str = $cgi->escapeHTML($str);\n \tif ($opts{'-nbsp'}) {\n \t\t$str =~ s/ /&nbsp;/g;\n \t}\n"},{"id":"36423","messageId":"7v649euai8.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"45ECE700.8090205@freescale.com","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T06:55:11Z","receivedAt":"2007-03-06T06:55:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Li Yang <leoli@freescale.com> writes:\n\n> Change to use explicitly function call cgi->escapHTML().\n> This fix the problem on some systems that escapeHTML() is not\n> functioning, as default CGI is not setting 'escape' parameter.\n>\n> Signed-off-by: Li Yang <leoli@freescale.com>\n\nRegardless of the recent xhtml+html vs html discussion, I think\nthis is probably a sane change.  Comments?\n"},{"id":"36443","messageId":"8fe92b430703060134l14fffcc4rbece3c2071c56422@mail.gmail.com","threadId":"7101","inReplyTo":"7v649euai8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-03-06T09:34:32Z","receivedAt":"2007-03-06T09:34:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 3/6/07, Junio C Hamano <junkio@cox.net> wrote:\n> Li Yang <leoli@freescale.com> writes:\n>\n> > Change to use explicitly function call cgi->escapHTML().\n> > This fix the problem on some systems that escapeHTML() is not\n> > functioning, as default CGI is not setting 'escape' parameter.\n> >\n> > Signed-off-by: Li Yang <leoli@freescale.com>\n>\n> Regardless of the recent xhtml+html vs html discussion, I think\n> this is probably a sane change.  Comments?\n\nGood (although a bit magic) solution. Ack, FWIW.\n\n-- \nJakub Narebski\n"},{"id":"36444","messageId":"20070306093917.GA1761@coredump.intra.peff.net","threadId":"7101","inReplyTo":"8fe92b430703060134l14fffcc4rbece3c2071c56422@mail.gmail.com","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-03-06T09:39:17Z","receivedAt":"2007-03-06T09:39:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:\n\n> >Regardless of the recent xhtml+html vs html discussion, I think\n> >this is probably a sane change.  Comments?\n> Good (although a bit magic) solution. Ack, FWIW.\n\nI think this should do the same, and is perhaps less magic (or maybe\nmore, depending on your perspective).\n\n-Peff\n\n-- >8 --\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 653ca3c..5d1d8cf 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -17,6 +17,7 @@ use Fcntl ':mode';\n use File::Find qw();\n use File::Basename qw(basename);\n binmode STDOUT, ':utf8';\n+CGI::autoEscape(1);\n \n BEGIN {\n        CGI->compile() if $ENV{MOD_PERL};\n"},{"id":"36445","messageId":"7vwt1ur9fs.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"20070306093917.GA1761@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T09:46:31Z","receivedAt":"2007-03-06T09:46:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:\n>\n>> >Regardless of the recent xhtml+html vs html discussion, I think\n>> >this is probably a sane change.  Comments?\n>> Good (although a bit magic) solution. Ack, FWIW.\n>\n> I think this should do the same, and is perhaps less magic (or maybe\n> more, depending on your perspective).\n>\n> -Peff\n\nThanks.  I tend to agree, as it does not depend on the reader\nknowing what magic $cgi default behaviour is by being\nexpliicit.\n\n>\n> -- >8 --\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 653ca3c..5d1d8cf 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -17,6 +17,7 @@ use Fcntl ':mode';\n>  use File::Find qw();\n>  use File::Basename qw(basename);\n>  binmode STDOUT, ':utf8';\n> +CGI::autoEscape(1);\n>  \n>  BEGIN {\n>         CGI->compile() if $ENV{MOD_PERL};\n"},{"id":"36446","messageId":"989B956029373F45A0B8AF02970818902DAA12@zch01exm26.fsl.freescale.net","threadId":"7101","inReplyTo":"20070306093917.GA1761@coredump.intra.peff.net","subject":"RE: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Li Yang-r58472","fromEmail":"leoli@freescale.com","sentAt":"2007-03-06T10:31:23Z","receivedAt":"2007-03-06T10:31:23Z","isPatch":true,"sender":{"key":"leoli@freescale.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Tuesday, March 06, 2007 5:39 PM\n> To: Jakub Narebski\n> Cc: Junio C Hamano; Li Yang-r58472; git@vger.kernel.org\n> Subject: Re: [PATCH] gitweb: Change to use explicitly function call\n> cgi->escapHTML()\n> \n> On Tue, Mar 06, 2007 at 10:34:32AM +0100, Jakub Narebski wrote:\n> \n> > >Regardless of the recent xhtml+html vs html discussion, I think\n> > >this is probably a sane change.  Comments?\n> > Good (although a bit magic) solution. Ack, FWIW.\n> \n> I think this should do the same, and is perhaps less magic (or maybe\n> more, depending on your perspective).\n\nYes, it also fixed the problem.  I'm not very familiar with perl.  Will\nCGI::autoEscape(1) change CGI action for other users of CGI module on\nthe system?  If so, maybe it will break other CGIs.\n\n- Leo\n"},{"id":"36447","messageId":"20070306104127.GA13096@coredump.intra.peff.net","threadId":"7101","inReplyTo":"989B956029373F45A0B8AF02970818902DAA12@zch01exm26.fsl.freescale.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-03-06T10:41:27Z","receivedAt":"2007-03-06T10:41:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2007 at 06:31:23PM +0800, Li Yang-r58472 wrote:\n\n> Yes, it also fixed the problem.  I'm not very familiar with perl.  Will\n> CGI::autoEscape(1) change CGI action for other users of CGI module on\n> the system?  If so, maybe it will break other CGIs.\n\nI don't know enough about mod_perl to say, but if all scripts share the\npackage globals from CGI, then yes, you're affecting all other scripts.\nWithout mod_perl, obviously you have no impact.\n\nIf it is the case, then your original fix is probably better.\n\n-Peff\n"},{"id":"36448","messageId":"7vzm6qps51.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"989B956029373F45A0B8AF02970818902DAA12@zch01exm26.fsl.freescale.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T10:45:30Z","receivedAt":"2007-03-06T10:45:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Li Yang-r58472\" <LeoLi@freescale.com> writes:\n\n>> -----Original Message-----\n>> From: Jeff King [mailto:peff@peff.net]\n>> ...\n>> I think this should do the same, and is perhaps less magic (or maybe\n>> more, depending on your perspective).\n>\n> Yes, it also fixed the problem.  I'm not very familiar with perl.  Will\n> CGI::autoEscape(1) change CGI action for other users of CGI module on\n> the system?  If so, maybe it will break other CGIs.\n\nBy \"other CGIs\" if you mean other independent CGI scripts that\ndo not have anything to do with gitweb, then I do think there is\nno need to worry.\n\nWhat I'd be worried about more, however, is if all the callers\nof esc_html and esc_path are really expecting the full quoting\ndone by CGI::autoEscape(1).  I think we had some discussion on\nthe path quoting when we introduced quot_cec and quot_upr, but\ndo not recall the details.  For example, many places esc_html()\nis used as the body of <a ...>$here</a> but some places it is\nused as\n\n    $cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))\n\nwhich would be the same as:\n\n    print '<a title=\"' . esc_html($fullname) . '\">' . esc_path($dir) . '</a>';\n\nwhich may or may not be right (I do not know offhand).\n\nSpeaking of -title, I see \"sub git_project_list_body\" does this:\n\n    $cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));\n\t\nwhich seems inconsistent with the earlier quoted $fullname\nhandling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'}\nis not, which I find highly unlikely).\n"},{"id":"36449","messageId":"7vveheprsc.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"20070306104127.GA13096@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T10:53:07Z","receivedAt":"2007-03-06T10:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 06, 2007 at 06:31:23PM +0800, Li Yang-r58472 wrote:\n>\n>> Yes, it also fixed the problem.  I'm not very familiar with perl.  Will\n>> CGI::autoEscape(1) change CGI action for other users of CGI module on\n>> the system?  If so, maybe it will break other CGIs.\n>\n> I don't know enough about mod_perl to say, but if all scripts share the\n> package globals from CGI, then yes, you're affecting all other scripts.\n> Without mod_perl, obviously you have no impact.\n>\n> If it is the case, then your original fix is probably better.\n\nBut then you are letting _other_ mod_perl users to affect your\nbehaviour, aren't you?  \"sub autoEscape\" does this:\n\n       sub autoEscape {\n           my($self,$escape) = self_or_default(@_);\n           my $d = $self->{'escape'};\n           $self->{'escape'} = $escape;\n           $d;\n       }\n\nIf we worry about mod_perl (provided if $CGI::Q is shared across\nmod_perl users), I suspect we would need to be a bit more\nparanoid, perhaps like this, woudln't we?\n\n---\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 653ca3c..9c4e060 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -26,6 +26,7 @@ our $cgi = new CGI;\n our $version = \"++GIT_VERSION++\";\n our $my_url = $cgi->url();\n our $my_uri = $cgi->url(-absolute => 1);\n+$cgi->autoEscape(1);\n \n # core git executable to use\n # this can just be \"git\" if your webserver has a sensible PATH\n"},{"id":"36451","messageId":"20070306105629.GA13285@coredump.intra.peff.net","threadId":"7101","inReplyTo":"7vveheprsc.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-03-06T10:56:29Z","receivedAt":"2007-03-06T10:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2007 at 02:53:07AM -0800, Junio C Hamano wrote:\n\n> But then you are letting _other_ mod_perl users to affect your\n> behaviour, aren't you?  \"sub autoEscape\" does this:\n\nYes (but I don't know how mod_perl works, and I haven't been able to\nfind a simple answer by skimming the docs).\n\n> If we worry about mod_perl (provided if $CGI::Q is shared across\n> mod_perl users), I suspect we would need to be a bit more\n> paranoid, perhaps like this, woudln't we?\n> [...]\n> +$cgi->autoEscape(1);\n\nThat rebreaks the original problem, though. Calling escapeHTML doesn't\nlook at $cgi, it looks at $Q (the \"default\" CGI object). I believe\nescape is _already_ set to 1 for $cgi (which is why the $cgi->escapeHTML\npatch worked).\n\n-Peff\n"},{"id":"36452","messageId":"7vps7mprj6.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"20070306105629.GA13285@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T10:58:37Z","receivedAt":"2007-03-06T10:58:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 06, 2007 at 02:53:07AM -0800, Junio C Hamano wrote:\n>\n>> But then you are letting _other_ mod_perl users to affect your\n>> behaviour, aren't you?  \"sub autoEscape\" does this:\n>\n> Yes (but I don't know how mod_perl works, and I haven't been able to\n> find a simple answer by skimming the docs).\n>\n>> If we worry about mod_perl (provided if $CGI::Q is shared across\n>> mod_perl users), I suspect we would need to be a bit more\n>> paranoid, perhaps like this, woudln't we?\n>> [...]\n>> +$cgi->autoEscape(1);\n>\n> That rebreaks the original problem, though.\n\nSorry, what I meant to say was on top of Li's patch.\n"},{"id":"36453","messageId":"20070306110156.GA13380@coredump.intra.peff.net","threadId":"7101","inReplyTo":"7vps7mprj6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-03-06T11:01:56Z","receivedAt":"2007-03-06T11:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:\n\n> Sorry, what I meant to say was on top of Li's patch.\n\nAh, I understand your point now. Yes, if other mod_perl CGIs can impact\nthe value, then we should definitely set it explicitly, as per your\npatch (and we should use Li's patch for safety, then, not mine).\n\n-Peff\n"},{"id":"36455","messageId":"7vlkiapr70.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"20070306110156.GA13380@coredump.intra.peff.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T11:05:55Z","receivedAt":"2007-03-06T11:05:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:\n>\n>> Sorry, what I meant to say was on top of Li's patch.\n>\n> Ah, I understand your point now. Yes, if other mod_perl CGIs can impact\n> the value, then we should definitely set it explicitly, as per your\n> patch (and we should use Li's patch for safety, then, not mine).\n\nReading \"sub autoEscape\", \"sub escapeHTML\" and \"sub\nself_or_default\" again, I think other people cannot affect the\nvalue of our $cgi->{'escape'} by calling autoEscape, so what I\nsaid is probably bogus.  Let's use Li's original patch.\n"},{"id":"36456","messageId":"989B956029373F45A0B8AF02970818902DAA14@zch01exm26.fsl.freescale.net","threadId":"7101","inReplyTo":"20070306110156.GA13380@coredump.intra.peff.net","subject":"RE: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Li Yang-r58472","fromEmail":"leoli@freescale.com","sentAt":"2007-03-06T11:07:52Z","receivedAt":"2007-03-06T11:07:52Z","isPatch":true,"sender":{"key":"leoli@freescale.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Tuesday, March 06, 2007 7:02 PM\n> To: Junio C Hamano\n> Cc: Li Yang-r58472; Jakub Narebski; git@vger.kernel.org\n> Subject: Re: [PATCH] gitweb: Change to use explicitly function call\n> cgi->escapHTML()\n> \n> On Tue, Mar 06, 2007 at 02:58:37AM -0800, Junio C Hamano wrote:\n> \n> > Sorry, what I meant to say was on top of Li's patch.\n> \n> Ah, I understand your point now. Yes, if other mod_perl CGIs can\nimpact\n> the value, then we should definitely set it explicitly, as per your\n> patch (and we should use Li's patch for safety, then, not mine).\n\nI agree.\n\n-Leo\n"},{"id":"36457","messageId":"20070306110754.GA13470@coredump.intra.peff.net","threadId":"7101","inReplyTo":"7vlkiapr70.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-03-06T11:07:54Z","receivedAt":"2007-03-06T11:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2007 at 03:05:55AM -0800, Junio C Hamano wrote:\n\n> > Ah, I understand your point now. Yes, if other mod_perl CGIs can impact\n> > the value, then we should definitely set it explicitly, as per your\n> > patch (and we should use Li's patch for safety, then, not mine).\n> \n> Reading \"sub autoEscape\", \"sub escapeHTML\" and \"sub\n> self_or_default\" again, I think other people cannot affect the\n> value of our $cgi->{'escape'} by calling autoEscape, so what I\n> said is probably bogus.  Let's use Li's original patch.\n\nEr, sorry, yes, I just accidentally agreed with your bogosity (while\nfiguring out what you originally meant, I forgot which CGI we were\ntalking about!). So I agree, Li's original is sufficient. Sorry for the\nnoise. :)\n\n-Peff\n"},{"id":"36461","messageId":"200703061423.18417.jnareb@gmail.com","threadId":"7101","inReplyTo":"7vzm6qps51.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-03-06T13:23:17Z","receivedAt":"2007-03-06T13:23:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n\n> Speaking of -title, I see \"sub git_project_list_body\" does this:\n> \n>     $cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));\n>         \n> which seems inconsistent with the earlier quoted $fullname\n> handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'}\n> is not, which I find highly unlikely).\n\nCGI::a() subroutine automatically quotes properly _attribute_ values,\nbut it does not (and it should not) quote _contents_ of a tag.\n\nSo the above code is correct.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"36495","messageId":"7vzm6qm07l.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"200703061423.18417.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-06T23:17:02Z","receivedAt":"2007-03-06T23:17:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>\n>> Speaking of -title, I see \"sub git_project_list_body\" does this:\n>> \n>>     $cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));\n>>         \n>> which seems inconsistent with the earlier quoted $fullname\n>> handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'}\n>> is not, which I find highly unlikely).\n>\n> CGI::a() subroutine automatically quotes properly _attribute_ values,\n> but it does not (and it should not) quote _contents_ of a tag.\n>\n> So the above code is correct.\n\nSorry, you lost me...  I am wondering what you mean by\n\"automatically\".  Do you mean 'always'?\n\nAnd if that is the case, shouldn't we drop esc_html() around\n$fullname here?\n\n    ...  For example, many places esc_html()\n    is used as the body of <a ...>$here</a> but some places it is\n    used as\n\n        $cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))\n\nas we do not have it around $pr->{'descr_long'} in the above?\n"},{"id":"36506","messageId":"200703070137.07477.jnareb@gmail.com","threadId":"7101","inReplyTo":"7vzm6qm07l.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-03-07T00:37:06Z","receivedAt":"2007-03-07T00:37:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>> Junio C Hamano wrote:\n>>\n>>> Speaking of -title, I see \"sub git_project_list_body\" does this:\n>>> \n>>>     $cgi->a({ ... -title => $pr->{'descr_long'}}, esc_html($pr->{'descr'}));\n>>>         \n>>> which seems inconsistent with the earlier quoted $fullname\n>>> handling (unless $pr->{'descr_long'} is already quoted and $pr->{'descr'}\n>>> is not, which I find highly unlikely).\n>>\n>> CGI::a() subroutine automatically quotes properly _attribute_ values,\n>> but it does not (and it should not) quote _contents_ of a tag.\n>>\n>> So the above code is correct.\n> \n> Sorry, you lost me...  I am wondering what you mean by\n> \"automatically\".  Do you mean 'always'?\n\nYes, I mean that CGI::a() does quoting _of attributes_, always.\n \n> And if that is the case, shouldn't we drop esc_html() around\n> $fullname here?\n> \n>     ...  For example, many places esc_html()\n>     is used as the body of <a ...>$here</a> but some places it is\n>     used as\n> \n>         $cgi->a({ ... -title =>esc_html($fullname) }, esc_path($dir))\n> \n> as we do not have it around $pr->{'descr_long'} in the above?\n\nThe above is wrong, thrice. First, it should be esc_path($fullname).\nSecond, rules for escaping attribute values are different from escaping\nHTML. Third, CGI::a() does escaping of attribute values.\n\nExplanation:\n\n  $cgi->a({ ... -attribute => atribute_value }, tag_contents)\n\nis translated to\n\n  <a ... attribute=\"attribute_value\">tag_contents</a>\n\nThe rules for escaping attribute values (which are string contents) are\ndifferent. For example you have to take care about escaping embedded '\"'\nand \"'\" characters; CGI::a() does that for us automatically.\n\nCGI::a() cannot HTML escape tag contents automatically; we might want to\nwrite\n\n  <a href=\"URL\">some <b>bold</b> text</a>\n\nfor example. Soe we have to esc_html (or esc_path) if needed.\n\n\nIn short: escape tag contents if needed, do not escape attrbure values.\n-- \nJakub Narebski\nPoland\n"},{"id":"36508","messageId":"7vvehdnaib.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"200703070137.07477.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Change to use explicitly function call cgi->escapHTML()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-07T00:49:16Z","receivedAt":"2007-03-07T00:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> In short: escape tag contents if needed, do not escape attrbure values.\n\nI trust a patch from you will follow shortly?\n"},{"id":"36512","messageId":"200703070221.25519.jnareb@gmail.com","threadId":"7101","inReplyTo":"7vvehdnaib.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] gitweb: Don't escape attributes in CGI.pm HTML methods","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-03-07T01:21:25Z","receivedAt":"2007-03-07T01:21:25Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"There is no need to escape HTML tag's attributes in CGI.pm\nHTML methods (like CGI::a()), because CGI.pm does attribute\nescaping automatically.\n\nExplanation:\n  $cgi->a({ ... -attribute => atribute_value }, tag_contents)\nis translated to\n  <a ... attribute=\"attribute_value\">tag_contents</a>\nThe rules for escaping attribute values (which are string contents) are\ndifferent. For example you have to take care about escaping embedded '\"'\nand \"'\" characters; CGI::a() does that for us automatically.\n\nCGI::a() cannot HTML escape tag contents automatically; we might want to\nwrite\n  <a href=\"URL\">some <b>bold</b> text</a>\nfor example. So we have to esc_html (or esc_path) if needed.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nJunio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n>> In short: escape tag contents if needed, do not escape attrbure values.\n> \n> I trust a patch from you will follow shortly?\n\nHere it is. I hope I found everything.\n\nCommit message is bit long, so you can cut it to first sentence only\n(or even only to title/subject).\n\n\n gitweb/gitweb.perl |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 653ca3c..ea58946 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1974,17 +1974,17 @@ sub git_print_page_path {\n \t\t\t$fullname .= ($fullname ? '/' : '') . $dir;\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$fullname,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => esc_html($fullname)}, esc_path($dir));\n+\t\t\t              -title => $fullname}, esc_path($dir));\n \t\t\tprint \" / \";\n \t\t}\n \t\tif (defined $type && $type eq 'blob') {\n \t\t\tprint $cgi->a({-href => href(action=>\"blob_plain\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => esc_html($name)}, esc_path($basename));\n+\t\t\t              -title => $name}, esc_path($basename));\n \t\t} elsif (defined $type && $type eq 'tree') {\n \t\t\tprint $cgi->a({-href => href(action=>\"tree\", file_name=>$file_name,\n \t\t\t                             hash_base=>$hb),\n-\t\t\t              -title => esc_html($name)}, esc_path($basename));\n+\t\t\t              -title => $name}, esc_path($basename));\n \t\t\tprint \" / \";\n \t\t} else {\n \t\t\tprint esc_path($basename);\n-- \n1.5.0.2\n"},{"id":"36513","messageId":"7vk5xtn84z.fsf@assigned-by-dhcp.cox.net","threadId":"7101","inReplyTo":"200703070221.25519.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Don't escape attributes in CGI.pm HTML methods","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-07T01:40:28Z","receivedAt":"2007-03-07T01:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> There is no need to escape HTML tag's attributes in CGI.pm\n> HTML methods (like CGI::a()), because CGI.pm does attribute\n> escaping automatically.\n>\n> Explanation:\n>   $cgi->a({ ... -attribute => atribute_value }, tag_contents)\n> is translated to\n>   <a ... attribute=\"attribute_value\">tag_contents</a>\n> The rules for escaping attribute values (which are string contents) are\n> different. For example you have to take care about escaping embedded '\"'\n> and \"'\" characters; CGI::a() does that for us automatically.\n>\n> CGI::a() cannot HTML escape tag contents automatically; we might want to\n> write\n>   <a href=\"URL\">some <b>bold</b> text</a>\n> for example. So we have to esc_html (or esc_path) if needed.\n>\n> Signed-off-by: Jakub Narebski <jnareb@gmail.com>\n> ---\n> Junio C Hamano wrote:\n>> Jakub Narebski <jnareb@gmail.com> writes:\n>> \n>>> In short: escape tag contents if needed, do not escape attrbure values.\n>> \n>> I trust a patch from you will follow shortly?\n>\n> Here it is. I hope I found everything.\n>\n> Commit message is bit long, so you can cut it to first sentence only\n> (or even only to title/subject).\n\nThanks.  I think your explanation in the log message has the\nright amount of details and keeping it there would help people\nwho would want to later touch the code.\n"}]}