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

Search highlighting is prone to regular expression injection #1458

Closed
tobyzerner opened this issue Jun 15, 2018 · 8 comments · Fixed by #2273
Closed

Search highlighting is prone to regular expression injection #1458

tobyzerner opened this issue Jun 15, 2018 · 8 comments · Fixed by #2273

Comments

@tobyzerner
Copy link
Contributor

Whatever you enter in the search field is being put unescaped into a regular expression to locally highlight matches. For example:

https://discuss.flarum.org/?q=test%20%5Bblah%5D%2B%20test

You can also easily crash the search by typing an invalid pattern:

https://discuss.flarum.org/?q=test%20%2B%20test

@DanielTheGeek
Copy link

It's very inappropriate to post security bugs on the issue tracker page. You should have mailed the development team instead.

@luceos
Copy link
Member

luceos commented Jun 15, 2018

@DanielTheGeek thanks for your concern, I'd like to kindly point out that:

  • @tobscure is the project owner and belongs to the active core developer team.
  • this bug is not affecting other users, it only affects yourself.

@DanielTheGeek
Copy link

Oh, okay... thanks for pointing that out.

@sijad
Copy link
Contributor

sijad commented Jul 1, 2018

lodash has a escapeRegExp method, we can use that or something like (suggested at https://stackoverflow.com/a/6969486):

function escapeRegExp(str) {
  return str.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, "\\$&");
}

which one do you think is better?

@tobyzerner
Copy link
Contributor Author

@sijad lodash sounds good, just make sure Webpack tree shakes properly :)

@dsevillamartin
Copy link
Member

I cannot reproduce this in the latest dev-master.

image

@tobyzerner
Copy link
Contributor Author

@datitisev it's not enough to just visit a search URL; you need to actually type a phrase into the search box for it to be parsed as a regular expression.

@dsevillamartin
Copy link
Member

@tobscure Ah, the issue was that no discussions or posts matched the query. Now I can reproduce it.
Related to #1539, may want to escape regex in #1539 instead of the current solution.

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