Bug: Overly permissive url regex

[randomsamples]

The line:

regex_url = re.compile("http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\\(\\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+")

should probably be:

regex_url = re.compile("http[s]?://(?:[a-zA-Z]|[0-9]|[$\\-_@.&+]|[!*\\(\\),]|(?:%[0-9a-fA-F][0-9a-fA-F]))+")

use unescaped '-' creates a very permissive character range there.

[sarahyurick]

Hi @randomsamples perhaps good solution here could be to make regex_url a customizable parameter for the relevant filters under https://github.com/NVIDIA-NeMo/Curator/blob/main/nemo_curator/stages/text/filters/heuristic/string.py ? There are a decent amount of options like r"https?://[^\s]+" or r"https?://[^\s\"'<>]+" too.

[randomsamples]

You're right, there isn't really a "correct" regex here. Making it overridable while providing a useful default seems like a good idea. I was under the impression that the original regex there was just mistakenly written since it appeared to intend to include "-" in that character class rather than create a character range in the expression, if that is true then I would also suggest updating that regex to match the intent, assuming that doesnt cause any serious regressions.

[sarahyurick]

Thanks @randomsamples, I have no strong preference on what the default value should be here. It looks like the current value being used has been hardcoded since Curator was open sourced, haha. That being said, I personally am not opposed to updating it to something else.

I am going to mark this as a "good first issue" for contributors :) if you would like to work on it yourself, then I can assign it to you! If not, then I would encourage one of our community members to pick it up.

[nightcityblade]

Hi, I'd like to work on this. I'll submit a PR shortly.

[nightcityblade]

I’m picking this up now and will send a focused PR shortly.