Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the CGI CPAN distribution.

Report information
The Basics
Id: 40748
Status: resolved
Priority: 0/
Queue: CGI

People
Owner: MARKSTOS [...] cpan.org
Requestors: vindex [...] apartia.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: (no value)
Fixed in: (no value)



Subject: when autoscape(undef) escapeHTML() no longer works (does nothing)
after calling autoscape(undef) escapeHTML() no longer works (does nothing)
Download (untitled) / with headers
text/plain 263b
On Fri Nov 07 11:41:57 2008, vindex wrote: Show quoted text
> after calling autoscape(undef) escapeHTML() no longer works (does nothing)
I agree this sounds like a bug. To proceed, next we'll need an automated test which illustrates the bug, and then a patch to fix it. Mark
Download (untitled) / with headers
text/plain 503b
Show quoted text
> > I agree this sounds like a bug. To proceed, next we'll need an automated > test which illustrates the bug, and then a patch to fix it. > > Mark
I have a series of commits today that make an automated test for this issue (http://github.com/bubaflub/CGI.pm/commit/ce4e3b9e0c6e04251a2dc77539967a8a81b1e0f2 is the latest). However, I'm not sure what the expected functionality is: both autoEscape(undef) and autoEscape(0) disable all future direct invocations of escapeHTML(). Is that expected?
Download (untitled) / with headers
text/plain 961b
On Sun Aug 30 15:26:15 2009, BUBAFLUB wrote: Show quoted text
> > > > I agree this sounds like a bug. To proceed, next we'll need an
> automated
> > test which illustrates the bug, and then a patch to fix it. > > > > Mark
> > I have a series of commits today that make an automated test for this > issue >
(http://github.com/bubaflub/CGI.pm/commit/ce4e3b9e0c6e04251a2dc77539967a8a81b1e0f2 Show quoted text
> is the latest). However, I'm not sure what the expected functionality > is: both autoEscape(undef) and autoEscape(0) disable all future direct > invocations of escapeHTML(). Is that expected?
The docs only say: "To turn autoescaping off completely, use autoEscape(0)" But the logical behavior seems to be that this should only set the default, and that escapeHTML() could always escapeHTML() regardless of this setting. So, that's my opinion of how this should be patched. The docs should be clarified as well. We run this change by Lincoln to confirm his intent. Mark
Download (untitled) / with headers
text/plain 1.1k
bubaflub, Does this give you what you need to proceed with the final changes? On Sun Aug 30 22:17:32 2009, MARKSTOS wrote: Show quoted text
> On Sun Aug 30 15:26:15 2009, BUBAFLUB wrote:
> > > > > > I agree this sounds like a bug. To proceed, next we'll need an
> > automated
> > > test which illustrates the bug, and then a patch to fix it. > > > > > > Mark
> > > > I have a series of commits today that make an automated test for
> this
> > issue > >
>
(http://github.com/bubaflub/CGI.pm/commit/ce4e3b9e0c6e04251a2dc77539967a8a81b1e0f2 Show quoted text
> > is the latest). However, I'm not sure what the expected
> functionality
> > is: both autoEscape(undef) and autoEscape(0) disable all future
> direct
> > invocations of escapeHTML(). Is that expected?
> > The docs only say: > "To turn autoescaping off completely, use autoEscape(0)" > > But the logical behavior seems to be that this should only set the > default, and that escapeHTML() could always escapeHTML() regardless of > this setting. So, that's my opinion of how this should be patched. The > docs should be clarified as well. > > We run this change by Lincoln to confirm his intent. > > Mark > >
Subject: Needs Test, Patch: when autoEscape(undef) escapeHTML() no longer works
Download (untitled) / with headers
text/plain 480b
Just to confirm, the autoEscape(undef) should turn off the auto-escaping but a direct call to escapeHTML() should still work, correct? If so, all I have to do is every time escapeHTML is called internally (as part of the auto-escaping) check if the flag has been set rather than (what is done currently) checking inside the escapeHTML function. On Mon Aug 31 21:45:54 2009, MARKSTOS wrote: Show quoted text
> bubaflub, > > Does this give you what you need to proceed with the final changes? >
Download (untitled) / with headers
text/plain 452b
On Tue Sep 01 12:22:31 2009, BUBAFLUB wrote: Show quoted text
> Just to confirm, the autoEscape(undef) should turn off the auto-escaping > but a direct call to escapeHTML() should still work, correct?
Correct. Show quoted text
> If so, all I have to do is every time escapeHTML is called internally > (as part of the auto-escaping) check if the flag has been set rather > than (what is done currently) checking inside the escapeHTML function.
Thanks for preparing this patch. Mark
Download (untitled) / with headers
text/plain 662b
I don't suppose there is any "easier" or more elegant way? Is there anyway to distinguish when escapeHTML is being called internally in CGI.pm and when it's being called on it's own? There are around 30 instances of escapeHTML being called in internal functions. If not I'll just tag every line (that has escapeHTML() in it) with an if statement checking if $self->{'escape'} is set. Show quoted text
> > If so, all I have to do is every time escapeHTML is called internally > > (as part of the auto-escaping) check if the flag has been set rather > > than (what is done currently) checking inside the escapeHTML function.
> > Thanks for preparing this patch. > > Mark
On Tue Sep 01 16:26:21 2009, BUBAFLUB wrote: Show quoted text
> I don't suppose there is any "easier" or more elegant way? Is there > anyway to distinguish when escapeHTML is being called internally in > CGI.pm and when it's being called on it's own? There are around 30 > instances of escapeHTML being called in internal functions. If not I'll > just tag every line (that has escapeHTML() in it) with an if statement > checking if $self->{'escape'} is set.
So, to clarify, autoescape() should control whether or not CGI.pm functions escape internally. Any direct call to escapeHTML should always escapeHTML, as you would expect. If we are on the same page, then perhaps this is another way to approach it. Internal functions could call _maybe_escapeHTML(). This logic from escapeHTML would be moved there: return $toencode if ref($self) && !$self->{'escape'}; Then, _maybe_escapeHTML() can a have a little internal code comment explaining what's going on. To me that seems a little cleaner than adding "if" blocks everywhere we call escapeHTML() internally. Mark
Download (untitled) / with headers
text/plain 963b
My last two commits fix this as well as add two more tests to make sure this happens. I realized I had also clobbered your formatting of that function (because I didn't do this work on a branch... whoops). http://github.com/bubaflub/CGI.pm/commit/d3015f3626d71267657c4582df5ef7cff0c5c127 Show quoted text
> > So, to clarify, autoescape() should control whether or not CGI.pm > functions escape internally. Any direct call to escapeHTML should always > escapeHTML, as you would expect. > > If we are on the same page, then perhaps this is another way to approach > it. > > Internal functions could call _maybe_escapeHTML(). > > This logic from escapeHTML would be moved there: > > return $toencode if ref($self) && !$self->{'escape'}; > > Then, _maybe_escapeHTML() can a have a little internal code comment > explaining what's going on. > > To me that seems a little cleaner than adding "if" blocks everywhere we > call escapeHTML() internally. > > Mark >
Subject: Re: [rt.cpan.org #40748] Needs Test, Patch: when autoscape(undef) escapeHTML() no longer works
Date: Wed, 2 Sep 2009 09:45:08 -0400
To: bug-CGI.pm [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Download (untitled) / with headers
text/plain 605b
On Wed, 02 Sep 2009 00:48:56 -0400 "Robert Kuo via RT" <bug-CGI.pm@rt.cpan.org> wrote: Show quoted text
> Queue: CGI.pm > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=40748 > > > My last two commits fix this as well as add two more tests to make sure > this happens. I realized I had also clobbered your formatting of that > function (because I didn't do this work on a branch... whoops). > > http://github.com/bubaflub/CGI.pm/commit/d3015f3626d71267657c4582df5ef7cff0c5c127
Thanks, and don't worry about the formatting. I'll try to get this reviewed and integrated in the next 48 hours. Mark
Thanks, this is now patched in my github repo now.
Subject: Thanks, released
Download (untitled) / with headers
text/plain 160b
The patch for this ticket has now been released in CGI.pm 3.47, and this ticket is considered resolved. Thanks again for you help to improve CGI.pm! Mark


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.