#47 ✓resolved
same

Fixed String#escapeHTML and String#unescapeHTML (several fixes, memory leak)

Reported by same | April 24th, 2008 @ 09:52 PM | in 1.6.0.3

reads:

return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');

it should read:

return this.replace(/</g,'<').replace(/>/g,'>').replace(/&/g,'&');

Comments and changes to this ticket

  • same

    same April 24th, 2008 @ 09:54 PM

    that didn't show up

  • John-David Dalton

    John-David Dalton April 24th, 2008 @ 10:09 PM

    • Milestone set to 1.6.0.3
    • State changed from “new” to “bug”
    • Title changed from “version 1.6.0.2 line 533” to “Fixed String#escapeHTML and String#unescapeHTML (several fixes, memory leak)”
  • John-David Dalton

    John-David Dalton April 24th, 2008 @ 10:11 PM

    also the patch is committed in my fork

    http://github.com/jdalton/protot...

    I would like to discuss this because its a pretty drastic patch, maybe some things could be done better

  • same

    same April 24th, 2008 @ 10:40 PM

    This is simply a problem in the code. The ampersand should be replaced last in unescapeHTML...

    Otherwise, a string "&gt;" will turn into ">".

  • same

    same April 24th, 2008 @ 10:42 PM

    Sorry, i'm figuring out the formatting.

    As it stands:

    unescapeHTML on line 533 will turn "&amp;gt;" into ">"
    
  • John-David Dalton

    John-David Dalton April 24th, 2008 @ 10:47 PM

    Right. That simple problem is the tip of the iceberg of issues with these 2 methods.

    Check out the tickets I linked to for more info.

  • same

    same April 24th, 2008 @ 10:56 PM

    Just to confirm:

    you are saying that the difference between the following line 533: @@@

    return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');

    
    and this line: @@@
        return this.replace(/&lt;/g,'<').replace(/&gt;/g,'>').replace(/&amp;/g,'&');
    

    causes all kinds of problems?

    the former is broken... the latter is correct based on the escapeHTML definition on line 529

  • same

    same April 24th, 2008 @ 10:58 PM

    Still learning...

    you are saying that the difference between the following line 533: @@@

    return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');

    
    and this line: @@@
        return this.replace(/&lt;/g,'<').replace(/&gt;/g,'>').replace(/&amp;/g,'&');
    
  • John-David Dalton

    John-David Dalton April 25th, 2008 @ 06:26 AM

    I am not arguing with you, I am saying this as well as other issues have

    already been addressed, patched, and tested in a previous ticket. :)

    Before you post again please take the time to follow my links,

    read the tickets, read the tickets they link to and so on.

    I will give you some hints:

    IE/Safari only unescape about 3 characters where

    Firefox/Opera and others unescape hundreds more.

    There is a memoryleak in IE.

    There are issues in IE with escaping those 3 characters.

    There are issues between escaping whitespace in IE.

    Safari 3 has issues with the ">" char as well.

  • Andrew Dupont

    Andrew Dupont May 20th, 2008 @ 12:20 AM

    • State changed from “bug” to “resolved”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

People watching this ticket

Attachments

Tags

Pages