New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please provide a function for escaping special characters in selectors #1761

Closed
mgol opened this Issue Oct 21, 2014 · 18 comments

Comments

Projects
None yet
5 participants
@mgol
Member

mgol commented Oct 21, 2014

Originally reported by fejesjoco at: http://bugs.jquery.com/ticket/14692

I read this article:  http://learn.jquery.com/using-jquery-core/faq/how-do-i-select-an-element-by-an-id-that-has-characters-used-in-css-notation/ .

It says plain and simple that a colon and a period must be escaped. And then the example code also escapes square brackets, so there's a problem already. In the future, you may add a new special character for a cool new selector feature, and that will be a breaking change.

I can see already how many bugs have been filed about special characters in selectors and then closed as being documented. I understand it completely. This is not a bug report, this is a feature request.

Please provide a function for escaping special characters in selectors. Benefits:

  • it's a very easy utility function to implement and useful in many cases
  • all those people who never cared about such issues, will realize they should use this escaping mechanism
  • all those people who used to run into problems with special characters before, used to file all those unnecessary bugs, now they will have an out-of-the-box solution instead
  • if a spec changes or jQuery API changes, it won't be a breaking change because the updated escape function would handle the new cases too
  • the quoted documentation suggests to copy-paste some code into my own JS, but jQuery is about helping eliminate those kinds of code pieces, it would be great to have it embedded

I also realized there are much more characters which could need escaping if we're not just talking about the restricted ID's:  http://api.jquery.com/category/selectors/ . This proposed escape function could handle that as well. I hope it's not a problem if it unnecessarily escapes characters in a selector in cases when it's not needed.

Issue reported for jQuery 1.10.2

@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: scott.gonzalez

This issue came up on the WHATWG mailing list a few months ago:  http://lists.w3.org/Archives/Public/public-whatwg-archive/2013Oct/0075.html and was added to CSSOM:  http://dev.w3.org/csswg/cssom/#the-css.escape()-method

Member

mgol commented Oct 21, 2014

Comment author: scott.gonzalez

This issue came up on the WHATWG mailing list a few months ago:  http://lists.w3.org/Archives/Public/public-whatwg-archive/2013Oct/0075.html and was added to CSSOM:  http://dev.w3.org/csswg/cssom/#the-css.escape()-method

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: fejesjoco

I'm glad it's being worked on. Do you think it would be reasonable to provide that function in jQuery, and that would call CSS.escape() if it's available, or use a fixed regexp if it's not? I look at jQuery as a layer that, among others, hides those kinds of browser differences. Also, CSS.escape() is only a valid alternative as long as jQuery's syntax is in accordance with CSS, but jQuery can actually extend that syntax. jQuery has lots and lots of functions that depend on selectors, so I believe there would be a real place for this utility method.

Member

mgol commented Oct 21, 2014

Comment author: fejesjoco

I'm glad it's being worked on. Do you think it would be reasonable to provide that function in jQuery, and that would call CSS.escape() if it's available, or use a fixed regexp if it's not? I look at jQuery as a layer that, among others, hides those kinds of browser differences. Also, CSS.escape() is only a valid alternative as long as jQuery's syntax is in accordance with CSS, but jQuery can actually extend that syntax. jQuery has lots and lots of functions that depend on selectors, so I believe there would be a real place for this utility method.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: scott.gonzalez

jQuery allows for additional selectors, but not for additional syntax. I believe there should never be a case where a proper CSS escaping method would fail on a custom jQuery selector.

Member

mgol commented Oct 21, 2014

Comment author: scott.gonzalez

jQuery allows for additional selectors, but not for additional syntax. I believe there should never be a case where a proper CSS escaping method would fail on a custom jQuery selector.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: timmywil

There are certainly helpful use cases. The most common case I foresee is concatenating an attribute value in an attribute selector. Certain special characters have meaning in selectors unless they are escaped, so it is impossible to predict the user's intentions 100% of the time, which means inevitable non-bug reports as well.

I'm inclined to vote against the feature not because of a lack of utility externally, but because we have no need of it internally, which is usually the formula for a great plugin.

Member

mgol commented Oct 21, 2014

Comment author: timmywil

There are certainly helpful use cases. The most common case I foresee is concatenating an attribute value in an attribute selector. Certain special characters have meaning in selectors unless they are escaped, so it is impossible to predict the user's intentions 100% of the time, which means inevitable non-bug reports as well.

I'm inclined to vote against the feature not because of a lack of utility externally, but because we have no need of it internally, which is usually the formula for a great plugin.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

Since it's being addressed by standards, I don't know that we want to get out ahead of that. We could possibly offer a wrapper, but it seems like someone who wanted this functionality could just as easily create a polyfill. So I'm not a big fan either.

I don't know how it complicates things but $( str ) could also be HTML or a script, not just a selector. Not sure whether a script injection attempt via a CSS fragment could be made to succeed.

Marking this open for discussion at the meeting next month.

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

Since it's being addressed by standards, I don't know that we want to get out ahead of that. We could possibly offer a wrapper, but it seems like someone who wanted this functionality could just as easily create a polyfill. So I'm not a big fan either.

I don't know how it complicates things but $( str ) could also be HTML or a script, not just a selector. Not sure whether a script injection attempt via a CSS fragment could be made to succeed.

Marking this open for discussion at the meeting next month.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: fejesjoco

$(str) is not a use case I was thinking about. The use case is when I want to add some variable string into a selector, without breaking the selector. For example, this is broken for parameters like "a.b":

function getElementById2(id) {
  return $("#" + id);
}

I should write this instead:

function getElementById2(id) {
  return $("#" + $.selectorescape(id));
}

There could be many such vulnerabilities out there like that already. For example, this was just fixed in ASP.NET MVC here:  https://aspnetwebstack.codeplex.com/SourceControl/changeset/e663ab603486b94162a233ea43d675e1e3452d06 . They added their own escape mechanism for 4 metacharacters .:[], I don't know if its enough or not, and who knows what some other people might think what should be escaped.

It makes sense to me that the question "hey, mr. interpreter, which characters should I escape for you?" can be best answered by the interpreter itself. A good example of that is the regex escaping in .NET:  http://msdn.microsoft.com/en-us/library/system.text.regularexpressions.regex.escape%28v=vs.110%29.aspx . It knows what the special characters are and how they should be escaped, so you can put user-supplied strings into a regexp without worrying about anything.

Member

mgol commented Oct 21, 2014

Comment author: fejesjoco

$(str) is not a use case I was thinking about. The use case is when I want to add some variable string into a selector, without breaking the selector. For example, this is broken for parameters like "a.b":

function getElementById2(id) {
  return $("#" + id);
}

I should write this instead:

function getElementById2(id) {
  return $("#" + $.selectorescape(id));
}

There could be many such vulnerabilities out there like that already. For example, this was just fixed in ASP.NET MVC here:  https://aspnetwebstack.codeplex.com/SourceControl/changeset/e663ab603486b94162a233ea43d675e1e3452d06 . They added their own escape mechanism for 4 metacharacters .:[], I don't know if its enough or not, and who knows what some other people might think what should be escaped.

It makes sense to me that the question "hey, mr. interpreter, which characters should I escape for you?" can be best answered by the interpreter itself. A good example of that is the regex escaping in .NET:  http://msdn.microsoft.com/en-us/library/system.text.regularexpressions.regex.escape%28v=vs.110%29.aspx . It knows what the special characters are and how they should be escaped, so you can put user-supplied strings into a regexp without worrying about anything.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

See also #11773. Parts of a built-up str may need to be selector-escaped or html-escaped before being passed to $(str) depending on its purpose and origin.

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

See also #11773. Parts of a built-up str may need to be selector-escaped or html-escaped before being passed to $(str) depending on its purpose and origin.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: fejesjoco

Yes, it's a similar problem. Every time when an engine supports certain magic characters, the user may accidentally submit a string which does something else than intended. Also the engine has no way to know what's intentional and what's not. The best solution is if the engine itself provides a proper escaping function. The engine knows what to escape, the user only has to know to call it for string parts where no special interpretation is intended. These functions are way too simple to put into plugins, and they should be the integral parts of the interpreting engines because the rules may change over time. I should be able to do $(whatever).html("<p>" + htmlescape(whatever) + "</p>") or $("#" + cssescape(whatever.id)), etc.

Member

mgol commented Oct 21, 2014

Comment author: fejesjoco

Yes, it's a similar problem. Every time when an engine supports certain magic characters, the user may accidentally submit a string which does something else than intended. Also the engine has no way to know what's intentional and what's not. The best solution is if the engine itself provides a proper escaping function. The engine knows what to escape, the user only has to know to call it for string parts where no special interpretation is intended. These functions are way too simple to put into plugins, and they should be the integral parts of the interpreting engines because the rules may change over time. I should be able to do $(whatever).html("<p>" + htmlescape(whatever) + "</p>") or $("#" + cssescape(whatever.id)), etc.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: fejesjoco

Found a similar thing: $.ui.autocomplete.escapeRegex. It could also be added to be available globally. It's not strictly jQuery related, but it's already there in jQuery UI (in a very wrong place) and it has a use-case. Since core JavaScript provides RegExp, of course it should also provide its own escaping function, but it doesn't.

Here's a very good example. I may want to find whole words that the user specifies like this: new RegExp('(^|[^\w])' + user_supplied_value + '($|[^\w])'). The user supplied value should be properly escaped so that it doesn't contain any characters which are handled specially in a regexp.

Member

mgol commented Oct 21, 2014

Comment author: fejesjoco

Found a similar thing: $.ui.autocomplete.escapeRegex. It could also be added to be available globally. It's not strictly jQuery related, but it's already there in jQuery UI (in a very wrong place) and it has a use-case. Since core JavaScript provides RegExp, of course it should also provide its own escaping function, but it doesn't.

Here's a very good example. I may want to find whole words that the user specifies like this: new RegExp('(^|[^\w])' + user_supplied_value + '($|[^\w])'). The user supplied value should be properly escaped so that it doesn't contain any characters which are handled specially in a regexp.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: gibson042

Reference:  https://github.com/mathiasbynens/CSS.escape

Member

mgol commented Oct 21, 2014

Comment author: gibson042

Reference:  https://github.com/mathiasbynens/CSS.escape

@mgol mgol added the Feature label Oct 21, 2014

@timmywil timmywil added the Selector label Feb 1, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 14, 2015

Member

There hasn't been a lot of traction here recently and it's a new feature so I'm bumping the milestone to 3.1.0.

@gibson042 if you disagree, please revert my change.

Member

mgol commented Sep 14, 2015

There hasn't been a lot of traction here recently and it's a new feature so I'm bumping the milestone to 3.1.0.

@gibson042 if you disagree, please revert my change.

@mgol mgol modified the milestones: 3.1.0, 3.0.0 Sep 14, 2015

@timmywil timmywil modified the milestones: 3.0.0, 3.1.0 Sep 21, 2015

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 6, 2015

Member

By our general criteria for adding new features it seems like this doesn't pass muster. We don't need it internally and it can easily be done as a plugin and the CSS.escape polyfill already exists. I'll close this but if someone has good reason to reopen we can always do that.

Member

dmethvin commented Nov 6, 2015

By our general criteria for adding new features it seems like this doesn't pass muster. We don't need it internally and it can easily be done as a plugin and the CSS.escape polyfill already exists. I'll close this but if someone has good reason to reopen we can always do that.

@dmethvin dmethvin closed this Nov 6, 2015

@dmethvin dmethvin removed this from the 3.0.0 milestone Nov 6, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 6, 2015

Member

@dmethvin Please apply the wontfix label if you're closing an issue as such.

Member

mgol commented Nov 6, 2015

@dmethvin Please apply the wontfix label if you're closing an issue as such.

@mgol mgol added the wontfix label Nov 6, 2015

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Nov 7, 2015

Member

I reread the discussion, and am convinced by the arguments about escape functionality going hand-in-hand with the engine interpreting special characters, to maintain stability over time. This will be going in Sizzle, so might as well be documented/exposed in jQuery as well.

EDIT: I'd also claim adherence to the "Does it support best practice?" bullet point.

Member

gibson042 commented Nov 7, 2015

I reread the discussion, and am convinced by the arguments about escape functionality going hand-in-hand with the engine interpreting special characters, to maintain stability over time. This will be going in Sizzle, so might as well be documented/exposed in jQuery as well.

EDIT: I'd also claim adherence to the "Does it support best practice?" bullet point.

@gibson042 gibson042 reopened this Nov 7, 2015

@gibson042 gibson042 removed the wontfix label Nov 7, 2015

gibson042 added a commit to gibson042/sizzle that referenced this issue Nov 9, 2015

@markelog markelog added this to the 3.0.0 milestone Nov 23, 2015

gibson042 added a commit to jquery/sizzle that referenced this issue Dec 19, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 21, 2015

Member

I suggest jQuery.escapeCSS

Member

timmywil commented Dec 21, 2015

I suggest jQuery.escapeCSS

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 21, 2015

Member

It's just escaping the selector though, not a CSS rule or set of rules. Something like escapeSelector would be more descriptive although I admit it's pretty long. Just escape in the context of jQuery seems to ambiguous, although it's fine for Sizzle.

Member

dmethvin commented Dec 21, 2015

It's just escaping the selector though, not a CSS rule or set of rules. Something like escapeSelector would be more descriptive although I admit it's pretty long. Just escape in the context of jQuery seems to ambiguous, although it's fine for Sizzle.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 21, 2015

Member

It's escaping a CSS selector...

Member

timmywil commented Dec 21, 2015

It's escaping a CSS selector...

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 21, 2015

Member

Although I see your point. I was thinking escapeCSS to align more with CSS.escape.

Member

timmywil commented Dec 21, 2015

Although I see your point. I was thinking escapeCSS to align more with CSS.escape.

timmywil added a commit to timmywil/jquery that referenced this issue Jan 27, 2016

timmywil added a commit to timmywil/jquery that referenced this issue Jan 27, 2016

@timmywil timmywil closed this in 25068bf Jan 28, 2016

@mgol mgol removed the Has Pull Request label Mar 6, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.