Talk: Comments sometimes get approved even if they have banned words, and sometimes they go to a mistaken moderation queue

[HectorNM]

Hi everyone! Congratulations for this project!

I'm having some issues with banned words. We expected that, if a comment has a banned word, it goes automatically to "Rejected" queue on Admin. However, after running this system and receiving a lot of comments, we noticed a few behaviours regarding this:

• Comments that had banned words but were being approved;
• Comments that had banned words but went to "Pending" queue on Admin;
• Comments that had banned words but went to "Not moderaded" queue on Admin;

To reproduce this problem, it's really "random". I have to enter a comment with a banned word a lot of times (but with small variations on text so the comment is not repeated), and then I could see one time or another it going to the wrong queue (or being approved, without going to any queue).

For me, while debugging and putting some breakpoints inside the application, it looked like a race condition anywhere inside the application, because sometimes the method containsMatchingPhraseMemoized on line 26 of file src/core/server/services/comments/pipeline/phases/wordList.ts would return false for a comment with a banned word, although the expected behaviour would be it returning true.
However, I still couldn't understand the underlying conditions which this race condition happened - if it was realy a race condition.

I couldn't find a similar issue, so I'm opening this one.

Thanks in advance for any help!

Expected behavior: Comments with banned words being rejected automatically

Actual behavior: In a few cases, randomly, comments with banned words go approved or go the other moderation queues (non-moderated and pending)

Related Issues:

Versions:

• NodeJS: 10.16.0
• NPM: 6.12.1
• MongoDB: 3.6
• Redis: 3.2
• Browser: Google Chrome 79.0.3945.130 64 bits
• OS: macOS Catalina 10.15.2

[kgardnr]

Hey Hector, could you explain more what you're seeing step by step, so we can try to reproduce the issue?

[HectorNM]

Hi @kgardnr , sorry, I accidentally pressed "Enter" while still typing details for the issue. I've just edited with more information. Thank you for the help!

[kgardnr]

Update on this:

@HectorNM has tracked this down to be a bug in the regex for our wordlists, and I've confirmed the issue

[HectorNM]

I found a better way to reproduce and understand the problem.

Let's suppose we have a blacklist containing "banned" and "banned2". Then, we would write several comments like "banned 1", "banned 2", "banned 3", "banned 4" etc. When doing this, we saw that banned 2, banned 4, banned 6, etc, would be approved immediately, while others (banned 1, banned 3, etc) would be rejected.

We found the reason of this problem was on line 37 of the same file I mentioned before, when calling the function .test():

There's a logic that makes the regexp be memoized/cached after the first time it's executed (line 28, _export const generateRegExpMemoized = memoize(generateRegExp)_). So, for the first comment to be inserted, it'll call the method generateRegExp (line 16). It will call with an Array as input: _["banned1", "banned2"]_. And the result will be _/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu_ . So for the second comment onwards, it won't call this method anymore, it will always return the same value/internal object.

So, on line 37, generateRegExpMemoized is called passing phrases (the list of banned words). Supposing the list of banned words doesn't change, this method will always return the same value, as I said before. In this case, the value is _/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu_ . So with this returned value, it calls .test() with testString (which is the comment text).

So with the comments we made before, we would have these expressions running each time we made a comment:

/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 1')
/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 2')
/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 3')
/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 4')
... and so forth.

Because of memoization we did before, everytime we call generateRegExpMemoized, we are receiving the same object in memory. So we are calling .test('banned 1'), .test('banned 2'), etc, always on the same object.

If we read the docs for .test(), it says:

test() called multiple times on the same global regular expression instance will advance past the previous match.

That means that, if our regex has multiple matches, we will test it against for each match. Since we are always calling on the same global regular expression, it doesn't matter if the text input is different, we will test against the next match for the array of matches returned by .test().

So for example, if we run the first regular expression...

/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 1')

...it has two matches:

_["banned", ""]_

The blank string match is because of one of the groups defined in the regex, so it captures blank spaces as an extra match (only if it has a banned occurence, of course).

The first time we run .test(), it's using the input at index 0, so it'll return true. Since it returns true, the comment is rejected, which is the expected behaviour.

The second time we run .test(), since the regular expression is the same, .test() will consider the match at index 1 (as it is advancing the matches arrays, as said in the docs above). So, when running the regular expression for the second case:

/(^|[^\w])(banned|banned2)(?=[^\w]|$)/gimu.test('banned 2')

... it would have two matches:

["banned", ""]

... and then .test() would test if match at index 1 (which is a blank string) satisfies the regular expression, but we can clearly see it does not, and so it returns false - which means that the comment will be approved (which is not the expected behaviour).

An easy fix would be changing .test() to another function to test the regular expression matching. I've fixed this way and I'm opening a PR.

[wyattjoh]

Summing up the fix highlighted in that PR @HectorNM, removing the global flag makes the RegExp no longer a "global" expression, thereby fixing the bug you found. Thanks so much for all the work finding this bug!

[wyattjoh]

This will be addressed in a v5.5.1 release next week.

[HectorNM]

Great @wyattjoh ! It's good to know there was an even simpler solution :)

[wyattjoh]

Andddd it's done! Please try out our 5.5.1 release that includes this fix.