Bug Tracker

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14370 closed bug (patchwelcome)

.html(data) incorrectly modifies some regular expressions resulting in global eval script errors

Reported by: dgtanner@… Owned by: gibson042
Priority: low Milestone: 1.11/2.1
Component: manipulation Version: 1.8.3
Keywords: Cc:
Blocked by: Blocking: #14464

Description

Example of the problem (open up chrome console to view errors) Example of the problem:

http://www.cs.utah.edu/~dgtanner/cre/bugexample.html which uses ajax to call and load the page http://www.cs.utah.edu/~dgtanner/cre/badpage.html

Working example of a workaround. http://www.cs.utah.edu/~dgtanner/cre/workingexample.html which uses ajax to call and load the page http://www.cs.utah.edu/~dgtanner/cre/goodpage.html

the only difference is the regex change between badpage.html and goodpage.html

Change History (11)

comment:1 Changed 6 years ago by dmethvin

Owner: set to dgtanner@…
Status: newpending

In general it's dicey to attempt to inject an entire page (including <html>, <body>, etc) into a <div> element. This behavior, which is out of the control of jQuery, is mentioned in the docs.

Does the example break the same way when you inject a valid html fragment into the div? Does it fail the same way in all browsers? I didn't see a problem in Firefox.

comment:2 in reply to:  1 Changed 6 years ago by anonymous

Replying to dmethvin:

In general it's dicey to attempt to inject an entire page (including <html>, <body>, etc) into a <div> element. This behavior, which is out of the control of jQuery, is mentioned in the docs.

Does the example break the same way when you inject a valid html fragment into the div? Does it fail the same way in all browsers? I didn't see a problem in Firefox.

This fails in Chrome, Firefox and Internet Explorer. (The ajax replace does succeed, but a javascript exception is thrown when jquery tries to evaluate the script) This breaks other scripts run on the page after the call.

comment:3 Changed 6 years ago by dmethvin

Sorry, I'll be more specific. Please create a test case that eliminates the extraneous <body> etc.

comment:4 in reply to:  3 Changed 6 years ago by anonymous

Replying to dmethvin:

Sorry, I'll be more specific. Please create a test case that eliminates the extraneous <body> etc.

http://www.cs.utah.edu/~dgtanner/cre/bugexample2.html loads the following page page via ajax. http://www.cs.utah.edu/~dgtanner/cre/badpage2.html

<body><head> etc, have been removed as requested.

comment:5 Changed 6 years ago by dgtanner

http://www.cs.utah.edu/~dgtanner/cre/jqscreenshot.png

comment:6 Changed 6 years ago by dgtanner

If step through and look at the variables I notice that .replace(/<br\/>/g,"\n\t") is changed into something invalid like .replace(/<br/><t/>/g,"\n\t") before being globally evaluated (see stack above).

comment:7 Changed 6 years ago by gibson042

Component: unfiledmanipulation
Milestone: None1.11/2.1
Owner: changed from dgtanner@… to gibson042
Priority: undecidedlow
Status: pendingassigned

We use a very coarse regular expression to convert self-closing tags into open-close pairs before setting innerHTML for oldIE (#1549). Replacement happens regardless of context, be it inside a CDATA section, comment, textarea, or—as in your case—script. I'll look into a more sophisticated replacement scheme:

// no-innerHTML container ($1) or self-closing element that should close ($3-$7)
rxhtmlTag = /(<!\[CDATA\[[\w\W]*?\]\]>|<!--[\w\W]*?--|<(script|textarea)[\w\W]*?>[\w\W]*?<\/\2)|(<)(?!area|br|col|embed|hr|img|input|link|meta|param)(([\w:]+)[^>]*)(\/)(>)/gi;

// no-innerHTML container; "<"; element name and content; ">"; "<"; "/"; element name; ">"
rxhtmlTagReplacement = "$1$3$4$7$3$6$5$7";

...

html = html.replace( rxhtmlTag, rxhtmlTagReplacement );

Note, however, that the file size increase may be too great to justify the changes. It is highly recommended that you take advantage of escaping to dodge our regex (e.g., /[<]foo\/>/ or "<" + "bar").

comment:8 Changed 6 years ago by gibson042

Resolution: patchwelcome
Status: assignedclosed

As suspected, my implementation was just too big.

https://github.com/jquery/jquery/pull/1374

comment:9 Changed 6 years ago by gibson042

#13805 is a duplicate of this ticket.

comment:10 Changed 6 years ago by gibson042

Blocking: 14464 added

(In #14464) HTML5 throws in a monkey wrench, but in my opinion it's pointless to fix this without a more sophisticated regular expression, and I remain convinced that such an improvement does not offer enough value to justify its large size.

At most, I could see exposing pre-innerHTML replacement arguments as properties of jQuery for users to tweak if they want to accommodate such edge cases.

comment:4 Changed 6 years ago by gibson042

#14329 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.