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

Fix search-on-click #4691

Merged
merged 4 commits into from Feb 20, 2017

Conversation

Projects
None yet
3 participants
@numrut
Copy link
Contributor

commented Feb 15, 2017

// Update the .sel link.
var searchUrl = settings['$form'].attr('action') + '?q={0}';
settings['$results'].find('.sel').attr('href', format(searchUrl,
settings.searchTerm));

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 15, 2017

Member

Since this is being used in a URL it should be url encoded.

@muffinresearch

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@numrut I think for this to work the value of the un-escaped search value needs to be run through encodeURIComponent and then HTML escaped afterwards. Otherwise the urlencoding is going to escape the & in the HTML escaped string, which then results in double escaping following a request.

See http://jsbin.com/cokacoh/edit?js,console,output which highlights the differences.

See also the difference between submitting a search containing HTML on production versus running this current version of your patch.

@muffinresearch

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@numrut let me know if you need any pointers re: my last comment.

@numrut

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

@muffinresearch, okay, thanks! I'll try to figure it out myself at first.

@numrut

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

Found.

settings.searchTerm value is escaped by the escape_ function from init.js. So, we need to get an unescaped value for the encodeURIComponent function.

There are many possible solutions:

  • settings['$form'].find('#search-q').val() instead of settings.searchTerm;
  • _.unescape(settings.searchTerm);
  • add, for example, rawSearchTerm parameter to the settings object in inputHandler;
  • ...

Which one should we choose?

@muffinresearch

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

@numrut Thinking the nicest thing would be to add a urlSearchTerm to the settings object and do the url encoding and HTML escaping there, that way the settings object doesn't need to contain a raw value.

@@ -130,12 +130,14 @@ $.fn.searchSuggestions = function($results, processCallback, searchType) {
$results.filter('.visible').removeClass('visible');
return;
}
var urlVal = encodeURIComponent($self.val());

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 17, 2017

Member

This will also need HTML escaping too.

This comment has been minimized.

Copy link
@numrut

numrut Feb 17, 2017

Author Contributor

What is "HTML escaping"?)

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 17, 2017

Member

If you don't escape the HTML in the form value this would result in an XSS vulnerability [1].

In this case you need to encode for a URL and also escape any HTML in the string so that HTML can't be potentially be injected.

To do that you just need to run escape_() on the URI-encode string. See also the previous comments and jsbin for an example.

[1] https://en.wikipedia.org/wiki/Cross-site_scripting

This comment has been minimized.

Copy link
@numrut

numrut Feb 17, 2017

Author Contributor

That function escapes &, >, <, ' and ". But almost all of those symbols are already processed by encodeURIComponent. The only exception is '.

Query string must be percent-encoded [1]. And encodeURIComponent does it. escape_ makes another replace and breaks strings with ' in them. It adds ampersand, which is reserved symbol in query string.

[1] https://en.wikipedia.org/wiki/Percent-encoding

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 17, 2017

Member

The ' doesn't cause a problem in the url and ends-up as %27 in the resulting href when inserted. Whilst it seems somewhat overkill I think escaping for the HTML context is still worth doing.

http://jsbin.com/huxadul/edit?js,console,output

This comment has been minimized.

Copy link
@numrut

numrut Feb 17, 2017

Author Contributor

The ' doesn't cause a problem in the url and ends-up as %27 in the resulting href when inserted

Seems, it happens on JS Bin only. Try escape_(encodeURIComponent("'foo'")) in browser's console.

If we really need to escape everything, str.replace(/[^]/g, escape) does the trick. But I think encodeURIComponent(str) or encodeURIComponent(str).replace(/'/g, '%27') is quite enough.

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 20, 2017

Member

Seems, it happens on JS Bin only. Try escape_(encodeURIComponent("'foo'")) in browser's console.

It's not just the escaping and encoding you need to append the HTML into the DOM.

This comment has been minimized.

Copy link
@muffinresearch

muffinresearch Feb 20, 2017

Member

Looking at this a bit more, it's using attr for the href so escaping is implicit. I'm not sure why I was thinking this was using html() but that's only used here: https://github.com/mozilla/addons-server/blob/master/static/js/impala/site_suggestions.js#L13 so this patch is fine as it is. Sorry for the noise!

@muffinresearch muffinresearch merged commit 8d8cbee into mozilla:master Feb 20, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@muffinresearch

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

@numrut thanks for your contribution!

@numrut

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

@muffinresearch, thank you too! It was a very good experience!

@numrut numrut deleted the numrut:search-on-click branch Feb 20, 2017

@caitmuenster

This comment has been minimized.

Copy link

commented Feb 21, 2017

Thanks so much, @numrut! Your contribution has been added to our recognition wiki.

I'd also love to help you set up your profile on mozillians.org and vouch for your awesome work. :) If you could send me your email address (either here or to cneiman [at] mozilla [dot] com, I'll send you an invite.

@numrut

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

@caitmuenster

This comment has been minimized.

Copy link

commented Feb 21, 2017

Awesome! Invite sent. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.