Bug #39122 for CGI: Needs Tests, Patch: unescapeHTML method falsely recognizes certain text as entities

Tue Sep 09 11:15:09 2008 GAMACHE [...] cpan.org - Ticket created

Subject:

unescapeHTML method falsely recognizes certain text as entities

CGI.pm's unescapeHTML method, intended to translate certain SGML entities into the characters they represent, will recognize anything, even including whitespace, between an ampersand and a semicolon as an entity. This results in the ampersand and semicolon being stripped. For instance, the string: Dewey, Cheatham & Howe are liars; they stole my life savings! would be improperly unescaped to read: Dewey, Cheatham Howe are liars they stole my life savings! The fix is provided by a small change to the regex, converting it from a minimal match between ampersand and semicolon, to a minimal match of non-whitespace between ampersand and semicolon. A patch for this fix is included. -pete gamache tack-weld my last name to google's mail server.

Subject:

cgi.patch

Common subdirectories: CGI.pm-3.42/CGI and CGI.pm-3.42-patched/CGI diff -c CGI.pm-3.42/CGI.pm CGI.pm-3.42-patched/CGI.pm *** CGI.pm-3.42/CGI.pm Mon Sep 8 10:13:23 2008 --- CGI.pm-3.42-patched/CGI.pm Tue Sep 9 11:10:51 2008 *************** *** 2235,2241 **** my $latin = defined $self->{'.charset'} ? $self->{'.charset'} =~ /^(ISO-8859-1|WINDOWS-1252)$/i : 1; # thanks to Randal Schwartz for the correct solution to this one ! $string=~ s[&(.*?);]{ local $_ = $1; /^amp$/i ? "&" : /^quot$/i ? '"' : --- 2235,2241 ---- my $latin = defined $self->{'.charset'} ? $self->{'.charset'} =~ /^(ISO-8859-1|WINDOWS-1252)$/i : 1; # thanks to Randal Schwartz for the correct solution to this one ! $string=~ s[&(\W*?);]{ local $_ = $1; /^amp$/i ? "&" : /^quot$/i ? '"' : Common subdirectories: CGI.pm-3.42/examples and CGI.pm-3.42-patched/examples Common subdirectories: CGI.pm-3.42/t and CGI.pm-3.42-patched/t

Sat Jul 25 17:09:04 2009 MARKSTOS [...] cpan.org - Taken

Sat Jul 25 17:09:23 2009 MARKSTOS [...] cpan.org - Subject changed from 'unescapeHTML method falsely recognizes certain text as entities' to 'PATCH: unescapeHTML method falsely recognizes certain text as entities'

Sat Jul 25 17:10:22 2009 MARKSTOS [...] cpan.org - Correspondence added

On Tue Sep 09 11:15:09 2008, GAMACHE wrote: Show quoted text

> CGI.pm's unescapeHTML method, intended to translate certain SGML > entities into the characters they represent, will recognize anything, > even including whitespace, between an ampersand and a semicolon as an > entity. This results in the ampersand and semicolon being stripped. > For instance, the string: > > Dewey, Cheatham & Howe are liars; they stole my life savings! > > would be improperly unescaped to read: > > Dewey, Cheatham Howe are liars they stole my life savings! > > The fix is provided by a small change to the regex, converting it from a > minimal match between ampersand and semicolon, to a minimal match of > non-whitespace between ampersand and semicolon. A patch for this fix is > included.

Pete, Thanks for the patch. I have peer-reviewed and believe it should be accepted. Could you also submit a simple automated test which illustrates the bug? Mark

Sat Jul 25 17:10:26 2009 The RT System itself - Status changed from 'new' to 'open'

Fri Jul 31 22:40:38 2009 MARKSTOS [...] cpan.org - Correspondence added

On Tue Sep 09 11:15:09 2008, GAMACHE wrote: Show quoted text

> CGI.pm's unescapeHTML method, intended to translate certain SGML > entities into the characters they represent, will recognize anything, > even including whitespace, between an ampersand and a semicolon as an > entity. This results in the ampersand and semicolon being stripped. > For instance, the string: > > Dewey, Cheatham & Howe are liars; they stole my life savings! > > would be improperly unescaped to read: > > Dewey, Cheatham Howe are liars they stole my life savings! > > The fix is provided by a small change to the regex, converting it from a > minimal match between ampersand and semicolon, to a minimal match of > non-whitespace between ampersand and semicolon. A patch for this fix is > included. > > -pete gamache > tack-weld my last name to google's mail server.

Pete, Automating testing reveals that the proposed patch breaks the cases which are supposed to work: is( unescapeHTML( '&'), '&', 'unescapeHTML: &'); is( unescapeHTML( '"'), '"', 'unescapeHTML: "'); is( unescapeHTML( 'Bob & Tom went to the store; Where did you go?'), 'Bob & Tom went to the store; Where did you go?', 'unescapeHTML: a case where &...; should not be escaped.'); Applying the patch makes the third test start pass, but the first two start to fail. Please re-submit a patch which makes all tests pass. More tests to other possible HTML escaping cases would be ideal as well.

Fri Jul 31 22:41:05 2009 MARKSTOS [...] cpan.org - Subject changed from 'PATCH: unescapeHTML method falsely recognizes certain text as entities' to 'Needs Tests, Patch: unescapeHTML method falsely recognizes certain text as entities'

Sat Aug 15 20:07:29 2009 BUBAFLUB [...] cpan.org - Correspondence added

Attached is a patch to the unescapeHTML method that will not match whitespace characters and that moves the third out of the TODO block. I also have this on my CGI.pm fork at http://github.com/bubaflub/CGI.pm/tree/master which you can pull from if that's easier. Show quoted text

> > > Applying the patch makes the third test start pass, but the first two > start to fail. Please re-submit a patch which makes all tests pass. More > tests to other possible HTML escaping cases would be ideal as well. > > > > >

From ccec3724afe9daa864bd6f285478fc8e19296eda Mon Sep 17 00:00:00 2001 From: Bob Kuo <bob@celect.org> Date: Sat, 15 Aug 2009 19:04:13 -0500 Subject: [PATCH] fixes RT #39122 - unescapeHTML recognizing some text as entites --- lib/CGI.pm | 2 +- t/unescapeHTML.t | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/CGI.pm b/lib/CGI.pm index 775871c..ec6e8ad 100644 --- a/lib/CGI.pm +++ b/lib/CGI.pm @@ -2295,7 +2295,7 @@ sub unescapeHTML { my $latin = defined $self->{'.charset'} ? $self->{'.charset'} =~ /^(ISO-8859-1|WINDOWS-1252)$/i : 1; # thanks to Randal Schwartz for the correct solution to this one - $string=~ s[&(.*?);]{ + $string=~ s[&(\S*?);]{ local $_ = $1; /^amp$/i ? "&" : /^quot$/i ? '"' : diff --git a/t/unescapeHTML.t b/t/unescapeHTML.t index fc0f750..4cc5386 100644 --- a/t/unescapeHTML.t +++ b/t/unescapeHTML.t @@ -4,8 +4,5 @@ use CGI 'unescapeHTML'; is( unescapeHTML( '&'), '&', 'unescapeHTML: &'); is( unescapeHTML( '"'), '"', 'unescapeHTML: "'); -TODO: { - local $TODO = 'waiting on patch. Reference: https://rt.cpan.org/Ticket/Display.html?id=39122'; - is( unescapeHTML( 'Bob & Tom went to the store; Where did you go?'), - 'Bob & Tom went to the store; Where did you go?', 'unescapeHTML: a case where &...; should not be escaped.'); -} +is( unescapeHTML( 'Bob & Tom went to the store; Where did you go?'), + 'Bob & Tom went to the store; Where did you go?', 'unescapeHTML: a case where &...; should not be escaped.'); -- 1.6.3.1

Sat Aug 15 22:13:50 2009 MARKSTOS [...] cpan.org - Correspondence added

Thanks. This is now patched in my github repo now, with credit to you.

Sat Aug 15 22:13:51 2009 MARKSTOS [...] cpan.org - Status changed from 'open' to 'patched'

Wed Sep 09 22:07:08 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:09 2009 The RT System itself - Status changed from 'patched' to 'open'

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

Wed Sep 08 15:13:31 2010 MARKSTOS [...] cpan.org - Reference by ticket #61120 added

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

Bug #39122 for CGI: Needs Tests, Patch: unescapeHTML method falsely recognizes certain text as entities

Preferred bug tracker