Bug Tracker

Modify

Ticket #13223 (closed bug: fixed)

Opened 15 months ago

Last modified 10 months ago

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:
Blocking: Blocked by:

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

comment:1 Changed 15 months ago by dmethvin

  • Status changed from new to closed
  • Resolution set to notabug

You probably have whitespace or other junk in the string.

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

Last edited 15 months ago by dmethvin (previous) (diff)

comment:2 Changed 15 months ago by cbou

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

Last edited 15 months ago by cbou (previous) (diff)

comment:3 Changed 15 months 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 15 months 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 15 months ago by gibson042

#13296 is a duplicate of this ticket.

comment:6 Changed 15 months ago by gibson042

#13368 is a duplicate of this ticket.

comment:7 follow-up: ↓ 8 Changed 15 months 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 15 months 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 14 months ago by dmethvin

#13418 is a duplicate of this ticket.

comment:10 Changed 14 months ago by gibson042

  • Component changed from unfiled to core

comment:11 Changed 14 months ago by gibson042

#13429 is a duplicate of this ticket.

comment:12 Changed 14 months ago by dmethvin

#13476 is a duplicate of this ticket.

comment:13 Changed 14 months ago by dmethvin

#13512 is a duplicate of this ticket.

comment:14 Changed 14 months ago by dmethvin

#13582 is a duplicate of this ticket.

comment:15 Changed 13 months ago by timmywil

I still say we should just trim it.

comment:16 Changed 13 months ago by timmywil

#13591 is a duplicate of this ticket.

comment:17 Changed 13 months ago by anonymous

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

comment:18 Changed 13 months ago by scott.gonzalez

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 13 months 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 13 months 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 13 months 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 13 months 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 12 months ago by dmethvin

#13790 is a duplicate of this ticket.

comment:24 Changed 12 months 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 12 months ago by rwaldron

#13836 is a duplicate of this ticket.

comment:26 Changed 12 months ago by timmywil

  • Status changed from closed to reopened
  • Resolution notabug deleted

comment:27 Changed 12 months ago by timmywil

  • Priority changed from undecided to high
  • Status changed from reopened to open
  • Milestone changed from None to 2.0.1

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

comment:28 Changed 12 months 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 12 months 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 12 months ago by timmywil

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

comment:31 Changed 12 months ago by dmethvin

  • Milestone changed from 2.0.1 to 1.10

comment:32 Changed 11 months ago by Dave Methvin

  • Status changed from open to closed
  • Resolution set to fixed

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

Changeset: 9fdbc8bf33418f7b3a3a88df9a04ffee53c8dc66

comment:33 Changed 10 months 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 months ago by dmethvin

#14097 is a duplicate of this ticket.

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.