Commands for convenient regex testing by decorator-factory · Pull Request #952 · python-discord/bot

decorator-factory · 2020-05-19T02:29:15Z

create RegularExpressions cog
create !regexp search command

TeamSpen210 · 2020-05-20T00:04:23Z

+
+    @group(name='regexp', aliases=('regex', 're'), invoke_without_command=True)
+    async def regexp_group(self, ctx: Context) -> None:
+        """Commands for exploring the misterious worldof regular expressions."""


Minor typos here.

Suggested change

"""Commands for exploring the misterious worldof regular expressions."""

"""Commands for exploring the mysterious world of regular expressions."""

Thanks, I'll fix that. I think format_* docstrings can be improved as well.

jb3 · 2020-05-20T00:09:40Z

I'll take a more in depth look at the code soon, but one question I have now is whether executing arbitrary RegExp code is safe?

We used to have an issue on the bot where the RegExp used for code block detection resulted in the bot locking up due something called catastrophic backtracing. How can we prevent unsafe regular expressions from locking up the bot?

jb3 · 2020-05-20T00:12:44Z

Found the conversation for when we discovered the issue in the bot, the solution was to adjust the regular expression to something safe but this still leaves the risk of abuse if people use dangerous regular expressions.

decorator-factory · 2020-05-20T00:15:29Z

@Jos-B Why not set a small time limit?

jb3 · 2020-05-20T00:17:07Z

Right, but then we need to isolate the process which would require migrating a regex executor to another platform (which with the new UNIX commands in snekbox and possibility for more things in future is feasible) which would make this implementation obsolete.

TeamSpen210 · 2020-05-20T00:17:24Z

Could they use snekbox somehow? Format the regex into a template snippet which executes it, then prints to stdout a JSON result. Then parse that in the bot, handling errors.

jb3 · 2020-05-20T00:17:39Z

Example of catastrophic backtracing from StackOverflow

jb3 · 2020-05-20T00:18:03Z

Snekbox could definitely be used, either by adding a new executor or by basically sending all the processing code as a regular eval to snekbox.

jb3 · 2020-05-20T00:18:53Z

However at that point I think it becomes something where we need to discuss things among the core developers before we can think about implementation detail.

decorator-factory · 2020-05-20T01:01:16Z

The third-party regex module seems to handle such regular expressions without any issues. All examples of catastrophic regexes I could find work instantly.
The stackoverflow example:

decorator-factory · 2020-05-20T01:13:02Z

A possible issue with regex is that it adds some extra regex features, so a regular expression might work with the command, but not in python with re, which might confuse people. A simple fix would be to parse the regex using re as well and see if it throws an error.

…ting

… backtracking)

Turns out that bumping the flake8 version up to 3.8 introduces a long list of new linting errors. Since this PR is the one that bumps the version, I suppose we will also fix all the linting errors in this branch. (cherry picked from commit e993566)

ks129

Found one bug. One other thing I want to say is when I tested it, all results (errors and matches) is so plain. I think some language formatting should be added to there. This will increase readability.
for errors I made this with diff formatting and added - before each line, then I got the red text. And maybe should success result inside embed that implements wait_for_deletion?

ks129 · 2020-05-21T05:19:47Z

+    @group(name='regexp', aliases=('regex', 're'), invoke_without_command=True)
+    async def regexp_group(self, ctx: Context) -> None:
+        """Commands for exploring the misterious world of regular expressions."""
+        await ctx.invoke(self.bot.get_command("help"), "regexp")


This will not work. We migrated to new help command lately, so this has to be changed:

Suggested change

await ctx.invoke(self.bot.get_command("help"), "regexp")

await ctx.send_help(ctx.command)

MarkKoz

I do like this feature and want to see it added. If the regex module handles catastrophic expressions well, but re doesn't, then re support needs to be dropped. I am not too concerned about people being mislead by the extended features of regex. However, if possible, using re to check for successful compilation is a good idea. On the other hand, if re can also handle catastrophic expression, then the regex module is redundant.

Regarding execution time, let's keep also keep in mind that users will only be able to search at most ~2000 characters of text.

MarkKoz · 2020-06-01T05:58:43Z

+        """Commands for exploring the misterious world of regular expressions."""
+        await ctx.invoke(self.bot.get_command("help"), "regexp")
+
+    @regexp_group.command(name='search', aliases=('find', 's', '🔍'))


I don't like the use of emojis to invoke commands. Emojis are general-purpose, so someone could unintentionally invoke this command.

On the other hand, if re can also handle catastrophic expression, then the regex module is redundant.
re easily freezes the interpreter if you run the example linked by @J03B
So I'm going to use re just for compatibility validation.

MarkKoz · 2020-06-01T06:00:51Z

+    @regexp_group.command(name='search+', aliases=('find+', 's+', '🔍+'))
+    async def match_plus_command(self, ctx: Context, pattern: RegexRegex, test: str) -> None:
+        """
+        Like `!re search`, but with an extended regex format.


It's a bad user experience to make them have to invoke the help command again to look at the docstring for !re search. Just copy the docstring over to this one.

MarkKoz · 2020-06-01T06:01:18Z

+
+    @group(name='regexp', aliases=('regex', 're'), invoke_without_command=True)
+    async def regexp_group(self, ctx: Context) -> None:
+        """Commands for exploring the misterious world of regular expressions."""


Suggested change

"""Commands for exploring the misterious world of regular expressions."""

"""Commands for exploring the mysterious world of regular expressions."""

MarkKoz · 2020-06-01T06:07:20Z

+ReRegex = ConvertRegex(supports_extended_features=False)
+RegexRegex = ConvertRegex(supports_extended_features=True)


I think better names would be Regex and ExtendedRegex.

MarkKoz · 2020-06-01T06:11:05Z

+    r"""
+    Format a match result to display in a response.
+
+    >>> format_match(re.search("(\d\d)+", "hello123456world"))
+    ['    hello123456world', ' 0:      ^^^^^^', ' 1:          ^^']
+
+    Which will look as:
+    |    hello123456world
+    | 0:      ^^^^^^
+    | 1:          ^^
+    """


What do you think about showing the actual text matched instead of the carets? That may end up being easier to read, especially if there are a lot of groups (eyes won't have to travel far to match the carets to the original string).

I think this would be better:

| hello123456world | 0: 123456 | 1: 56

just showing the text might be ambiguous if a substring appears multiple times in a string.

However, we still have to figure out how to display zero-width matches 🤔

MarkKoz · 2020-06-01T06:19:24Z

+        await ctx.send(match_and_format(pattern, test))
+
+    @regexp_group.command(name='search+', aliases=('find+', 's+', '🔍+'))
+    async def match_plus_command(self, ctx: Context, pattern: RegexRegex, test: str) -> None:


test is not a clear name for the parameter. Perhaps simply named it string?

This parameter should also be a kw-only arg so it "consumes all" if the string has spaces. This means the pattern needs to be in quotes if it has spaces, but I think this is the best option we have.

I thought about that. Is there any way to put the pattern or the string inside a code block?

/re search `\d+` 123456helloworld

I am not sure. I think discord.py does not consider backticks to be the same as quotes, which means you will need to parse all arguments yourself from a single string to accomplish this. But don't take my word for it, double check this.

jb3 · 2020-06-03T11:18:18Z

I've been investigating regex a bit more and have found it is still very much susceptible to DoS attacks and should not be used with arbitrary user input.

I'll attach an example:

>>> import regex
>>> regex.match("^(([a-z])+.)+[A-Z]([a-z])+$", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!")
... hang ...

decorator-factory · 2020-06-03T11:19:46Z

I've been investigating regex a bit more and have found it is still very much susceptible to DoS attacks and should not be used with arbitrary user input.

I'll attach an example:
>>> import regex
>>> regex.match("^(([a-z])+.)+[A-Z]([a-z])+$", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!")
... hang ...

>>> regex.match("^(([a-z])+.)+[A-Z]([a-z])+$", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!",
... timeout=0.05)
>>> TimeoutError: regex timed out

jb3 · 2020-06-03T11:20:07Z

Huh!

decorator-factory · 2020-06-03T11:20:31Z

Good point, though, I'll add that as well.

nnsee · 2020-06-03T11:40:11Z

Alternatively, we can use snekbox for execution, leaving DoS and other similar attacks out of the equation. Would require some effort to move the code there, though.

EDIT: I see this has been discussed already, my bad.

…/decorator-factory/bot into decorator-factory-add-re-command

decorator-factory · 2020-06-03T12:43:12Z

Now it looks like this:

ks129 · 2020-06-03T12:48:56Z

+        await ctx.invoke(self.bot.get_command("help"), "regexp")
+
+    @regexp_group.command(name='search', aliases=('find', 's'))
+    async def match_command(self, ctx: Context, pattern: Regex, test: str) -> None:


Suggested change

async def match_command(self, ctx: Context, pattern: Regex, test: str) -> None:

async def match_command(self, ctx: Context, pattern: Regex, *, test: str) -> None:

ks129 · 2020-06-03T12:49:18Z

+        await ctx.send(match_and_format(pattern, test))
+
+    @regexp_group.command(name='search+', aliases=('find+', 's+'))
+    async def match_plus_command(self, ctx: Context, pattern: ExtendedRegex, test: str) -> None:


Suggested change

async def match_plus_command(self, ctx: Context, pattern: ExtendedRegex, test: str) -> None:

async def match_plus_command(self, ctx: Context, pattern: ExtendedRegex, *, test: str) -> None:

Thanks, added now

decorator-factory · 2020-06-03T12:54:14Z

TODO:

⚠️ move the matching/searching to snekbox
somehow show zero-width groups (right now they just don't show up)
support named groups (right now they show up as anonymous groups)

create RegularExpressions cog and the regexp search command

74bad61

decorator-factory requested a review from a team as a code owner May 19, 2020 02:29

decorator-factory requested review from Den4200 and MrHemlock and removed request for a team May 19, 2020 02:29

fix typo in RegularExpressions cog

e339143

decorator-factory mentioned this pull request May 19, 2020

Create !regexp command #953

Closed

decorator-factory linked an issue May 19, 2020 that may be closed by this pull request

Create !regexp command #953

Closed

decorator-factory closed this May 19, 2020

decorator-factory reopened this May 20, 2020

TeamSpen210 reviewed May 20, 2020

View reviewed changes

decorator-factory and others added 6 commits May 20, 2020 04:31

improve docstrings in RegularExpressions cog

f39f069

Make error message formatting consistent with the match result format…

92ef769

…ting

Switch from re to third-party regex module (to avoid catastrophic…

0b8f2d8

… backtracking)

Merge branch 'master' into decorator-factory-add-re-command

e612b28

make Pipfile.lock up to date in regular_expressions cog

bb0c698

ks129 suggested changes May 21, 2020

View reviewed changes

MarkKoz added a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 3 - low Low Priority labels May 22, 2020

MarkKoz added status: needs review t: feature New feature or request labels May 22, 2020

Merge remote-tracking branch 'origin/master' into HEAD

63e665b

MarkKoz requested changes Jun 1, 2020

View reviewed changes

decorator-factory added 2 commits June 3, 2020 14:22

improve naming and fix doscstrings

38a4914

limit searching time (hanging protection)

0a0064b

decorator-factory added 2 commits June 3, 2020 14:59

improve error and search result formatting

4ced119

Merge branch 'decorator-factory-add-re-command' of https://github.com…

ab53c7f

…/decorator-factory/bot into decorator-factory-add-re-command

ks129 suggested changes Jun 3, 2020

View reviewed changes

make re commands consume everything after the regex

c46e728

jb3 removed the status: needs review label Jun 13, 2020

ghost added the needs 2 approvals label Jun 16, 2020

decorator-factory closed this Jul 26, 2020

	"""Commands for exploring the misterious worldof regular expressions."""
	"""Commands for exploring the mysterious world of regular expressions."""

	await ctx.invoke(self.bot.get_command("help"), "regexp")
	await ctx.send_help(ctx.command)

		ReRegex = ConvertRegex(supports_extended_features=False)
		RegexRegex = ConvertRegex(supports_extended_features=True)

	async def match_command(self, ctx: Context, pattern: Regex, test: str) -> None:
	async def match_command(self, ctx: Context, pattern: Regex, *, test: str) -> None:

	async def match_plus_command(self, ctx: Context, pattern: ExtendedRegex, test: str) -> None:
	async def match_plus_command(self, ctx: Context, pattern: ExtendedRegex, *, test: str) -> None:

Uh oh!

Conversation

decorator-factory commented May 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jb3 commented May 20, 2020

Uh oh!

jb3 commented May 20, 2020

Uh oh!

decorator-factory commented May 20, 2020

Uh oh!

jb3 commented May 20, 2020

Uh oh!

TeamSpen210 commented May 20, 2020

Uh oh!

jb3 commented May 20, 2020

Uh oh!

jb3 commented May 20, 2020

Uh oh!

jb3 commented May 20, 2020

Uh oh!

decorator-factory commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decorator-factory commented May 20, 2020

Uh oh!

ks129 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkKoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

decorator-factory Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jb3 commented Jun 3, 2020

Uh oh!

decorator-factory commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jb3 commented Jun 3, 2020

Uh oh!

decorator-factory commented Jun 3, 2020

Uh oh!

nnsee commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decorator-factory commented Jun 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

decorator-factory commented May 20, 2020 •

edited

Loading

decorator-factory Jun 1, 2020 •

edited

Loading

decorator-factory commented Jun 3, 2020 •

edited

Loading

nnsee commented Jun 3, 2020 •

edited

Loading

decorator-factory commented Jun 3, 2020 •

edited

Loading