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

XSS vulnerability #134

Closed
unindented opened this Issue Dec 29, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@unindented
Contributor

unindented commented Dec 29, 2015

If server response contains HTML, jquery-typeahead blindly inserts it in the DOM, making it vulnerable to XSS. In my case, the endpoint returned this:

[
  {
    "id": "2053",
    "name": "Engineering <script>alert('test')</script>"
  }
]

Which caused jquery-typeahead to invoke alert.

The line responsible for this (although there may be others with the same mistake):

_aHtml = '<span class="' + scope.options.selector.display + '">' + _display.join(" ") + '</span>';

Instead of concatenating bits of HTML, it'd be better to use the appropriate jQuery functions like text() which handle escaping for you.

@running-coder

This comment has been minimized.

Owner

running-coder commented Dec 31, 2015

hey, thanks for flagging the issue, I unfortunatly have close to no time to fix it until maybe a week. Perhaps instead of a global escaping it would be best to have script and iframe escaped so possible react components and such can still be outputted properly, what do you think?

@running-coder running-coder added the Bug label Dec 31, 2015

@unindented

This comment has been minimized.

Contributor

unindented commented Dec 31, 2015

I think it could be a source of bugs and complexity... Handling escaping correctly is really tricky!

I'd do the simplest correct thing by default, which is to escape all displayed values, and leave it at that. If we hear from other people with more complex requirements we can reevaluate?

@running-coder

This comment has been minimized.

Owner

running-coder commented Dec 31, 2015

I added a sanitize string prototype for emptyTemplate and dynamic requests so I could as well apply it to the list items.

@unindented

This comment has been minimized.

Contributor

unindented commented Dec 31, 2015

Sounds good.

@running-coder running-coder added this to the 2.3.2 milestone Jan 6, 2016

running-coder pushed a commit that referenced this issue Jan 12, 2016

running-coder pushed a commit that referenced this issue Jan 12, 2016

running-coder pushed a commit that referenced this issue Jan 12, 2016

tom bertrand
Version 2.3.2
Feature
- #139 Added ``resultCountPerGroup`` parameter to ``onResult`` callback

Fixes
- #134 Fixed XSS vulnerability
- #136 Fixed UMD module definition

Note
- ``onSearch`` callback now gets triggered when a key is pressed (not only when query > options.minLength)

running-coder added a commit that referenced this issue Jan 14, 2016

Version 2.3.2
Feature
- #139 Added ``resultCountPerGroup`` parameter to ``onResult`` callback

Fixes
- #134 Fixed XSS vulnerability
- #136 Fixed UMD module definition

Note
- ``onSearch`` callback now gets triggered when a key is pressed (not only when query > options.minLength)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment