Skip to content
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

Escape "-", don't escape "}" #9

Closed
twhb opened this issue Apr 17, 2016 · 11 comments
Closed

Escape "-", don't escape "}" #9

twhb opened this issue Apr 17, 2016 · 11 comments

Comments

@twhb
Copy link

twhb commented Apr 17, 2016

"-": this is a control within character sets. Escaping content for use within character sets is already fully supported except for this character, including escaping "]", which doesn't need escaping except within character sets.

"}": this isn't a control anywhere except within quantifiers. Within a quantifier, escaping this results in an invalid regex. escape-string-regexp doesn't, and can't possibly, support escaping strings meant for a quantifier context.

If you approve of the change, I'll make a PR.

@kevva kevva mentioned this issue Sep 7, 2016
@twhb
Copy link
Author

twhb commented Jan 1, 2017

It's been eight months with no response, please give this issue some attention.

At the very least the README should be updated to state that character sets aren't supported.

@dptole
Copy link

dptole commented Jan 7, 2017

@twhb Considering what you said this should be the correct solution

var matchOperatorsRe = /[|\\{()[^$+*?.-]/g;

E.g.:

var matchOperatorsRe = /[|\\{()[^$+*?.-]/g;
'the characters {, (, ), [, ^, ?, +, *, . and - are to be escaped'
  .replace(matchOperatorsRe, '\\$&');
// 'the characters \\{, \\(, \\), \\[, \\^, \\?, \\+, \\*, \\. and \\- are to be escaped'

@jbruni
Copy link

jbruni commented Oct 9, 2017

Lodash version does not escape "-":
https://github.com/lodash/lodash/blob/master/escapeRegExp.js

In the other hand, bobince's version does:
https://stackoverflow.com/a/3561711

@dptole
Copy link

dptole commented Oct 10, 2017

@jbruni I take back what I said. The character "-" need to be escaped only if inside a [ and ] block. These are escaped already so there is no need to escape "-".

var matchOperatorsRe = /[|\\{()[^$+*?.]/g;
var text = 'a-z [a-z]'
var escaped_text = text.replace(matchOperatorsRe, '\\$&')
var regexp = new RegExp(escaped_text)
regexp.test('a-z a') // false
regexp.test('a-z [a-z]') // true

@twhb twhb closed this as completed May 8, 2018
@twhb twhb reopened this Oct 25, 2018
@ujikit
Copy link

ujikit commented Feb 9, 2019

i have same problem.
try this sql pattern:
SELECT * FROM members WHERE username = 'admin'--' AND password = 'password' - --
string: = and - does not escaped.

@sindresorhus
Copy link
Owner

I have decided to add - so this module can be used to escape a string that will be inserted in the middle of a regex. I don't see the point of removing }. Better to leave it for the same reason as previously mentioned.

@sindresorhus
Copy link
Owner

@ujikit = doesn't need to be escaped.

@twhb
Copy link
Author

twhb commented Apr 17, 2019

Thanks for the update. What reason are you referring to when you say “as previously mentioned”, “so this module can be used to escape a string that will be inserted in the middle of a regex”? If that’s the case, then escaping “}” does not forward that goal: an escaped “}” is not valid anywhere that an unescaped “}” would not be interpreted literally.

@sindresorhus
Copy link
Owner

@twhb You might be right, but there's a reason it was added in the first place and why all other similar libraries escape it. I don't really have the time to prove you wrong. I'm happy to remove it if you manage to convince https://github.com/lodash/lodash/blob/master/escapeRegExp.js to remove it.

@steadicat
Copy link

FYI this breaks Unicode regexes:
image

@shreyasminocha
Copy link

@steadicat Could you please create an issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants