Bug Tracker

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#13223 closed bug (fixed)

jQuery 1.9 + client-side template = “Syntax error, unrecognized expression”

Reported by: spizder@… Owned by:
Priority: high Milestone: 1.10
Component: core Version: 1.9.0
Keywords: Cc:
Blocked by: Blocking:

Description

I just updated jQuery from 1.8.3 to 1.9, and it started crashing all of a sudden.

This is my Mustache template:

<script type="text/template" id="modal_template">
    <div>hello</div>
</script>

This is how I read it:

modal_template_html = $("#modal_template").html();

This is how I transform it into jQuery object (I need to use jQuery methods on it):

template = $(modal_template_html);

... and jQuery crashes!

Error: Syntax error, unrecognized expression: <div>hello</div>

slice.call( docElem.childNodes, 0 )[0].nodeType;

jquery-1.9.0.js (line 3811)

However, if I declare template as a plain text variable, it starts working again:

modal_template_html = '<div>hello</div>';

http://stackoverflow.com/questions/14347611/jquery-1-9-client-side-template-syntax-error-unrecognized-expression#comment19945743_14347611

Change History (35)

comment:1 Changed 10 years ago by dmethvin

Resolution: notabug
Status: newclosed

You probably have whitespace or other junk in the string.

http://jquery.com/upgrade-guide/1.9/#jquery-htmlstring-versus-jquery-selectorstring

Last edited 10 years ago by dmethvin (previous) (diff)

comment:2 Changed 10 years ago by cbou

I edited after I read the comment of dmethvin, thanks.

Last edited 10 years ago by cbou (previous) (diff)

comment:3 Changed 10 years ago by mikesherov

Not that I want to retreat or anything, but surely whitespace at the beginning of an otherwise perfectly fine piece of HTML should be treated as HTML, right? dmethvin, perhaps we went a bit too far on "looks like HTML" strictness? I know we can have this debate for years, and the answer is "use $.parseHTML()", but just a fleeting thought.

comment:4 Changed 10 years ago by dmethvin

My thinking was that if there is whitespace then the text is probably being pulled in from some non-literal-string source such as it is here. That is exactly the case where we want to make people think about the fact that the string they are processing can contain scripts and therefore is a vector for XSS or other attacks. Although $.parseHTML() does not provide perfect protection it does allow control over whether script tags run.

Also, if we always pass the string through $.trim() or an equivalent regex for every use of $() it's more overhead. For a small string like " <br/>" it's nothing but again with a template that might be 100KB it would be better not to do that.

comment:5 Changed 10 years ago by gibson042

#13296 is a duplicate of this ticket.

comment:6 Changed 10 years ago by gibson042

#13368 is a duplicate of this ticket.

comment:7 Changed 10 years ago by andreas.korth@…

Seriously?

Instead of

$(' <div> ');

it's now

$($.parseHTML(' <div> '.trim()));

What was the reason behind crippling a great utility function? Is there anything gained from this?

comment:8 in reply to:  7 Changed 10 years ago by gibson042

Replying to andreas.korth@…:

What was the reason behind crippling a great utility function? Is there anything gained from this?

We gained the ability to reliably differentiate $( selector ) from $( html ), fixing #11290 and other "selector interpreted as HTML" bugs and getting a performance increase in the process. If you know you might have leading whitespace but don't care about it, just "uncripple" like $( $.trim(template) ). For anything more complex, yeah, you should be explicit about requesting HTML parsing (in which case the trim is probably unnecessary, if not undesirable).

comment:9 Changed 10 years ago by dmethvin

#13418 is a duplicate of this ticket.

comment:10 Changed 10 years ago by gibson042

Component: unfiledcore

comment:11 Changed 10 years ago by gibson042

#13429 is a duplicate of this ticket.

comment:12 Changed 10 years ago by dmethvin

#13476 is a duplicate of this ticket.

comment:13 Changed 10 years ago by dmethvin

#13512 is a duplicate of this ticket.

comment:14 Changed 10 years ago by dmethvin

#13582 is a duplicate of this ticket.

comment:15 Changed 10 years ago by Timmy Willison

I still say we should just trim it.

comment:16 Changed 10 years ago by Timmy Willison

#13591 is a duplicate of this ticket.

comment:17 Changed 10 years ago by anonymous

The amount of duplicates pretty much calls for a solution on the jQuery-side ...

comment:18 Changed 10 years ago by scottgonzalez

We're talking about a handful of reports over 2 months following a major release. The code is easy to change on the user's end, and the confusion will die down over time.

comment:19 Changed 10 years ago by anonymous

People are not normally used to the idea that adding new line or indentation to their template for clarity purposes will lead to some nasty error message.

Frankly, current behavior is not logical (who would have thought THAT?), very frustrating and extremely hard to debug. Hours and hours will be wasted before a reasonable person will figure out that a newline character is to blame.

So if it looks like a bug, feels like a bug, and behaves like a bug - it is probably a bug.

comment:20 Changed 10 years ago by dmethvin

For 1.10 we're planning to disable running scripts by default, which will solve the problem from the other direction and coincidentally allow more spaciness. Anyone who took our advice and used $($.parseHTML(html, document, true)) to run html+scripts will be fine with the new setup as well.

comment:21 Changed 10 years ago by anonymous

@dmethvin What do you mean by "disable running scripts by default"?

In regards to wasting hours: I (and am absolutely "unproud" of saying that), invested 6,5 hours debugging the issue - for the first half investigating my code.

Remember that it's unlikely someone is encountering the issue while typing strings explicitly. It's more likely that the markup is the result of (a lot of) server-side logic.

At the end it's surprising, that the simplest part (a declaration) actually is at fault.

Lesson learned: Always expect strange bugs in even the most established resources.

I'll try the recommended logic. For now I solved it by calling three more methods server-side. - Which is definitely far from causing a "performance gain".

comment:22 Changed 10 years ago by dmethvin

What do you mean by "disable running scripts by default"?

The current plan for 1.10 is this:

// This can come from an evil person
var html = "<p>hello</p><script src='bad.js'></script>"

// Won't load/run the script in 1.10
$(html).appendTo("body");

// Loads and runs script
$($.parseHTML(html, document, true)).appendTo("body");

If your template does not have script it will work as it did in 1.8.3. If it has script you'll need to opt into running it.

Lesson learned: Always expect strange bugs in even the most established resources.

I'd have a bit more sympathy if people experiencing this problem had reported this during the beta. If you want jQuery to be bug-free and for design decisions to break your way, you'll need to participate in the beta tests and see how *your* code fares.

comment:23 Changed 10 years ago by dmethvin

#13790 is a duplicate of this ticket.

comment:24 Changed 10 years ago by anonymous

Having

$(html)

perform a trim on the html string before further processing would not affect the 1.9 improvements to that function's security, and would fix plugins that this change has broken (for example, waypoints-infinite).

comment:25 Changed 10 years ago by Rick Waldron

#13836 is a duplicate of this ticket.

comment:26 Changed 10 years ago by Timmy Willison

Resolution: notabug
Status: closedreopened

comment:27 Changed 10 years ago by Timmy Willison

Milestone: None2.0.1
Priority: undecidedhigh
Status: reopenedopen

The team has decided to allow leading whitespace here (and possibly trailing).

comment:28 Changed 10 years ago by paul

I would throw my vote towards allowing both leading and trailing whitespace, as I often leave a trailing newline at the end of my HTML templates.

comment:29 Changed 10 years ago by dmethvin

So is this going into 1.10/2.0.1, or is it going into 1.11/2.1? We'll be making the rules tighter for the latter (no scripts), but should we loosen them in the meantime?

I ask since 1.10b1 is imminent...

comment:30 Changed 10 years ago by Timmy Willison

@dmethvin: I think both. We can simply match leading whitespace like we did before and still not allow scripts.

comment:31 Changed 10 years ago by dmethvin

Milestone: 2.0.11.10

comment:32 Changed 10 years ago by Dave Methvin

Resolution: fixed
Status: openclosed

Fix #13223. Re-allow leading space in HTML. Close gh-1264. (cherry picked from commit 00eafdf028f7730665ce1c05ab44e3f0bc80fbc2)

Changeset: 9fdbc8bf33418f7b3a3a88df9a04ffee53c8dc66

comment:33 Changed 10 years ago by anonymous

So, what is the reason behind fixing this in 1.10 and leaving it as is in 1.9? Wasn't the defect opened in 1.9 and should be fixed in the same version?

Not all can just upgrade, see JQM:

jQuery Mobile 1.3 supports jQuery 1.7.0 to 1.9.1

So, my question is - when will this be fixed in 1.9?

As a side note, I didn't expect such a huge blunder from jQuery team - this probably breaks code on thousands of sites that use client-side templates.

Thanks.

comment:34 Changed 10 years ago by dmethvin

#14097 is a duplicate of this ticket.

comment:35 Changed 9 years ago by vineetb

Can this fix be applied to 1.9.x branch, since 1.10 is still in beta?

Note: See TracTickets for help on using tickets.