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

Sanitize mentions in cleanup (prior to role logic) #92

Merged
merged 3 commits into from
Feb 8, 2019

Conversation

shikhir-arora
Copy link
Contributor

Per DM, opening PR. Sanitizes mentions in the raw article via. the cleanup function with a zero-width space. This makes all mentions (such as @everyone - that could be contained in the RSS body) not get triggered on Discord but does not affect the output, nor has any impact on subscribed roles, as that logic is done after this cleanup.

Per DM, opening PR. Sanitizes mentions in the raw article via. the `cleanup` function with a zero-width space. This makes all mentions (such as `@everyone` - that could be contained in the RSS body) not get triggered on Discord but does not affect the output, nor has any impact on subscribed roles, as that logic is done after this cleanup.
@synzen
Copy link
Owner

synzen commented Feb 5, 2019

Good catch, but what is the purpose of this?
text.replace(/`/g, '\`' + String.fromCharCode(8203))

@shikhir-arora
Copy link
Contributor Author

Good catch, but what is the purpose of this?
text.replace(/`/g, '\`' + String.fromCharCode(8203))

An additional safety check that sanitizes markdown-enclosed stuff, so `@hello` would be caught by adding a zero-width after `, and then adding the zero-width after @ which will make the mention not trigger.

@synzen
Copy link
Owner

synzen commented Feb 6, 2019

Ah so you're escaping backticks as well - in that case, did you mean to do

text.replace(/`/g, '\\`' + String.fromCharCode(8203))

then? A single backslash unnecessarily escapes the backtick, which is just equivalent to a backtick rather than a backslash and a backtick

@shikhir-arora
Copy link
Contributor Author

shikhir-arora commented Feb 7, 2019

Ah so you're escaping backticks as well - in that case, did you mean to do

text.replace(/`/g, '\\`' + String.fromCharCode(8203))

then? A single backslash unnecessarily escapes the backtick, which is just equivalent to a backtick rather than a backslash and a backtick

I may be misexplaining it. Pictures are worth a thousand words they say, so here's a few:

@synzen
Copy link
Owner

synzen commented Feb 8, 2019

It seems like you're trying to escape backticks - I have tried this:

function clean1 (text) {
  return text.replace(/`/g, '\`' + String.fromCharCode(8203))  // Sanitize mentions with zero-width character "\u200b"
    .replace(/@/g, '@' + String.fromCharCode(8203))
}

function clean2 (text) {
  return text.replace(/`/g, String.fromCharCode(8203))  // Sanitize mentions with zero-width character "\u200b"
    .replace(/@/g, '@' + String.fromCharCode(8203))
}


client.on('message', msg => {
  if (!msg.author.bot) {
    msg.channel.send(clean1(msg.content))
    msg.channel.send(clean2(msg.content))
  }
})

clean1 is your function, but the second message's clean1 result does not equate to yours
image

The clean2 function seems to do what you want however.

But overall, does the backtick replacement relate to escaping mentions? It seems like it would work fine without the first replace.

@shikhir-arora
Copy link
Contributor Author

But overall, does the backtick replacement relate to escaping mentions? It seems like it would work fine without the first replace.

It doesn't matter much here, from the standpoint that this is an edge case. I'll remove that now.

@synzen synzen merged commit 1c07238 into synzen:dev-web Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants