Bug Tracker

Modify

Ticket #11290 (closed bug: fixed)

Opened 2 years ago

Last modified 17 months ago

selector interpreted as HTML

Reported by: gibson042 Owned by: timmywil
Priority: blocker Milestone: 1.9
Component: core Version: 1.7.1
Keywords: Cc:
Blocking: Blocked by:

Description

#9521 and #6429 and probably others identify specific instances of a general problem: jQuery( strInput ) cannot reliably differentiate selectors from HTML.

 http://jsfiddle.net/C8dgG/

Looking for "<" past the first character creates vulnerabilities and confusing behavior on complex input.

quickExpr should be abandoned in favor of a simpler "parse as HTML if and only if there is a leading less-than" rule, with intentional parsing handled by the jQuery( "<div/>" ).html( strHtml ).contents() pattern.

Change History

comment:1 Changed 2 years ago by gibson042

Also (for reference),  CSS lexical tokenization:

string	{string1}|{string2}
string1	\"([^\n\r\f\\"]|\\{nl}|{escape})*\"
string2	\'([^\n\r\f\\']|\\{nl}|{escape})*\'
escape	{unicode}|\\[^\n\r\f0-9a-f]
unicode	\\[0-9a-f]{1,6}(\r\n|[ \n\r\t\f])?

comment:2 Changed 2 years ago by dmethvin

  • Priority changed from undecided to blocker
  • Status changed from new to open
  • Component changed from unfiled to core
  • Milestone changed from None to 1.8

comment:3 Changed 23 months ago by timmywil

  • Owner set to timmywil
  • Status changed from open to assigned

comment:4 Changed 23 months ago by dmethvin

As http://bugs.jquery.com/ticket/9521#comment:24 points out, a simple "starts with <" rule will break some unit tests and is likely to cause issues in outside code. However, I think we should be willing to do that here for at least some cases.

The only issue I'm on the fence about is whether to trim leading spaces. I can imagine situations where people may have templates that end up with leading spaces, but then again it seems expensive to always pass the string through a regexp to remove them for the handful of sloppy cases that could be resolved by using $.trim() externally.

If people *want* to parse arbitrary complex HTML and they know it's HTML, we should encourage them to use the new $.parseHTML method per #11617. It would be best if $(html) didn't execute scripts for example.

comment:5 Changed 23 months ago by gibson042

Given that we'll be providing jQuery.parseHTML for explicit HTML parsing, I'd like to be as strict as possible in jQuery.

comment:6 Changed 23 months ago by timmywil

It is possible to adjust the regex to avoid matching html when it is within brackets, parens, or quotes, as well as ignoring any escaped html characters. This would fix the case presented in this ticket as well as jQuery mobile's, @jdalton's and @mathias' issue in #9521. The other issue in #9521 is that there is a xss vulnerability when unexpected input is passed to jQuery(). I think it is safe to say that is much lower priority, but that would also be fixed by specifying that if some selector contains characters normally recognized as html, they should be escaped. This would resolve all of the issues and be backwards-compatible, unless I've missed a case.

comment:7 Changed 22 months ago by timmywil

I've implemented what I meant and adjusted the regex so we maintain backwards-compatibility as well as restrict the unexpected behaviors from occurring. This will fix most issues. Patch incoming.

comment:8 Changed 22 months ago by timmywil

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

When detecting html in init, ignore html characters within quotes, brackets, and parens as well as escaped characters which are valid in selectors. Fixes #11290.

Changeset: 7692ae419d4c19bd06a0ba01fc2af8d21035873c

comment:9 Changed 22 months ago by gibson042

  • Status changed from closed to reopened
  • Resolution fixed deleted

Now it's gone too far the other way...  http://jsfiddle.net/HCa89/1/

comment:10 Changed 22 months ago by timmywil

Having discussed it with gibson and Dave, we're adopting a different strategy (the starts-with rule). While ignoring html within quotes, brackets, and parens could have worked, I agree it would have come at too great a cost. Commit incoming.

comment:11 Changed 22 months ago by timmywil

  • Status changed from reopened to assigned

comment:12 Changed 22 months ago by timmywil

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

Adjust jQuery('html') detection to only match when html starts with '<' (counting space characters). Fixes #11290

Changeset: 239fc86b01d52fd9df6d1963027ed37b4f6251fc

comment:13 Changed 22 months ago by timmywil

Revert "Adjust jQuery('html') detection to only match when html starts with '<' (counting space characters). Fixes #11290"

This reverts commit 239fc86b01d52fd9df6d1963027ed37b4f6251fc.

The consensus is that this would change behavior too abruptly. We will warn in 1.8 and do this in 1.9.

Changeset: c20e031058c6210a1ed753f75af80588f076d60d

comment:14 Changed 18 months ago by dmethvin

  • Milestone changed from 1.8 to 1.9

comment:15 Changed 18 months ago by dmethvin

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:16 Changed 18 months ago by dmethvin

  • Status changed from reopened to open

comment:17 Changed 18 months ago by dmethvin

#12531 is a duplicate of this ticket.

comment:18 Changed 17 months ago by dmethvin

Going into 1.9; the compat plugin will restore the old behavior.

comment:19 Changed 17 months ago by timmywil

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

Adjust jQuery('html') detection to only match when html starts with '<' (not counting space characters). Fixes #11290.

Changeset: 05531fc4080ae24070930d15ae0cea7ae056457d

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.