-
Notifications
You must be signed in to change notification settings - Fork 15
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 ElasticSearch special characters when sending search request to member api #121
Comments
@vikasrohit yes, can handle it this week. Is there by any chance any user on DEV which can be used for testing? I think we cannot register a new user with such special characters. |
@maxceem I am not aware of any such user in dev, but there should be some definitely. I will look into it, and update if I can find one. Meanwhile, you can test the issue and most probably fix it as well, by assuming few hypothetical usernames containing these reserved keywords. As of now, member api would return |
@vikasrohit The only issue is with this place https://github.com/topcoder-platform/tc-notifications/blob/dev/connect/service.js#L169 we include a list of handles without quotes:
In the other place https://github.com/topcoder-platform/tc-notifications/blob/dev/src/common/tcApiHelper.js#L86 we include handles in double quotes:
And it doesn't produce an error in member service even if include So the only one symbol which we have to encoded in the handles in I guess this issue is not connected with ES limitations, as when I tested with various characters, only characters |
@vikasrohit I've created a PR as per my findings https://github.com/topcoder-platform/tc-notifications/pull/124/files Note, that handles containing The only thing, I didn't test fix for this line https://github.com/topcoder-platform/tc-notifications/pull/124/files#diff-981bb6e6585b27c21e9b46950b61ca4cR86 The fix is very small but still, can we test it after deploying to dev? I'm not sure what should we do to call that place in the code. |
Thanks @maxceem I have tested on production with
Mention a user with such handle in any post to call that code. |
Also, could you please create hotfix for it? |
@vikasrohit here is a hotfix #127 |
Thanks @maxceem |
As of now, when whenever we making query for user via handle, we are not escaping any special character present in the handle which is causing troubles for fetching the user details for some of the handles e.g. any handle that has
[
or]
in it which in turn causes500
error from the member api.Purpose of this ticket is to update the https://github.com/topcoder-platform/tc-notifications/blob/dev/connect/service.js#L178 and https://github.com/topcoder-platform/tc-notifications/blob/dev/src/common/tcApiHelper.js#L86 to escape the special character identified for the ElasticSearch.
fyi @maxceem let me know if we can handle this this week.
The text was updated successfully, but these errors were encountered: