Bug #40748 for CGI: Needs Test, Patch: when autoscape(undef) escapeHTML() no longer works

Fri Nov 07 11:41:57 2008 vindex [...] apartia.org - Ticket created

Subject:

when autoscape(undef) escapeHTML() no longer works (does nothing)

after calling autoscape(undef) escapeHTML() no longer works (does nothing)

Wed Jul 29 23:25:22 2009 MARKSTOS [...] cpan.org - Taken

Wed Jul 29 23:25:56 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Wed Jul 29 23:25:58 2009 The RT System itself - Status changed from 'new' to 'open'

Wed Jul 29 23:26:33 2009 MARKSTOS [...] cpan.org - Subject changed from 'when autoscape(undef) escapeHTML() no longer works (does nothing)' to 'Needs Test, Patch: when autoscape(undef) escapeHTML() no longer works'

Sun Aug 30 15:26:15 2009 BUBAFLUB [...] cpan.org - Correspondence added

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?

Sun Aug 30 22:17:32 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Mon Aug 31 21:45:54 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Tue Sep 01 12:22:31 2009 BUBAFLUB [...] cpan.org - Correspondence added

Subject:

Needs Test, Patch: when autoEscape(undef) escapeHTML() no longer works

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

Tue Sep 01 12:55:18 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Tue Sep 01 16:26:21 2009 BUBAFLUB [...] cpan.org - Correspondence added

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

Tue Sep 01 21:45:11 2009 MARKSTOS [...] cpan.org - Correspondence added

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

Wed Sep 02 00:48:55 2009 BUBAFLUB [...] cpan.org - Correspondence added

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 >

Wed Sep 02 09:45:30 2009 mark [...] summersault.com - Correspondence added

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>

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

Wed Sep 02 22:51:42 2009 MARKSTOS [...] cpan.org - Correspondence added

Thanks, this is now patched in my github repo now.

Thu Sep 03 08:28:14 2009 MARKSTOS [...] cpan.org - Status changed from 'open' to 'patched'

Wed Sep 09 22:07:18 2009 MARKSTOS [...] cpan.org - Correspondence added

Subject:

Thanks, released

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

Wed Sep 09 22:07:19 2009 The RT System itself - Status changed from 'patched' to 'open'

Wed Sep 09 22:07:19 2009 MARKSTOS [...] cpan.org - Status changed from 'open' to 'resolved'

Fri May 23 14:29:28 2014 The RT System itself - Queue changed from CGI.pm to CGI

Bug #40748 for CGI: Needs Test, Patch: when autoscape(undef) escapeHTML() no longer works

Preferred bug tracker