-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Pipe character breaks emojis #8168
Comments
Do you have an example of an emoji that contains a "|", or ir this only happening with custom emojis? |
This only happens with custom emoji. There aren't any default emoji/smileys that have the pipe. |
After taking a deep dive, my recommendation for this is to simply disabled the use of pipe character in custom emojis. We're using RegExp to parse the custom emojis, and as you can see on the line below, the pipe character is heavily used, which is causing the whole trouble: /* emojiCustom.js */
const regShortNames = new RegExp(`<object[^>]*>.*?<\/object>|<span[^>]*>.*?<\/span>|<(?:object|embed|svg|img|div|span|p|a)[^>]*>|(${ RocketChat.emoji.packages.emojiCustom.list.join('|') })`, 'gi'); Not only that, but the alias name is used to get the asset of the image, with is going to break because browsers don't support URL withs pipes. By the way, similar characters are already disabled, as you can see here: /* insertOrUpdateEmoji.js */
//allow all characters except colon, whitespace, comma, >, <, &, ", ', /, \, (, )
//more practical than allowing specific sets of characters; also allows foreign languages
const nameValidation = /[\s,:><&"'\/\\\(\)]/;
const aliasValidation = /[:><&"'\/\\\(\)]/; So, in my opinion we should close this, and open an issue to also disable the pipe character from custom emojis. What you think @Sarcaustic, @gdelavald ? |
Most of the recent browsers can encode the "|" character to %7C so that in itself isn't a problem as long as someone doesn't use an old version. |
@Sarcaustic exactly! And we're already banning characteres like: comma, >, <, &, /, , the pipe isn't that far away from them. I'm just waiting for a definition from @gdelavald so that I can close this and open another one. |
I agree with disabling the pipe character, unless @rodrigok has a different opinion, go ahead with the fix. No need to open another issue though, you can use this one to track it and just create a PR. |
@matheusml did you try to escape the regex? Like:
|
@rodrigok I tried escaping the pipe, not exactly like in your example, but I did. But I don't think that's point. If we're already banning characters like comma, > and <, that are 'tricky' to RegExp, why not keep the pattern and also ban the pipe character? It seems to me to be the safest decision. |
[FIX] Removing pipe and commas from custom emojis (#8168)
[FIX] Removing pipe and commas from custom emojis (#8168)
Description:
Vertical pipe character "|" breaks custom emoji loading. Emojis loaded before will work fine but all afterwards will not work.
Server Setup Information:
Steps to Reproduce:
Expected behavior:
Normal display and function of custom emoji
Actual behavior:
emojis do not load, there are listed in the search but no image is displayed and attempts to use it just sends the plain text.
Relevant logs:
server and browser both have no errors in logs.
The text was updated successfully, but these errors were encountered: