tell: fix edge case with `@` as the tellee #2582

bronzeguy · 2023-12-11T01:30:55Z

Description

There's an annoying bug where an oversight in code that checks if a message is prepended with an @ allows for a user to make the bot spit out error messages forever until the bot is restarted. I have fixed this oversight here

Checklist

I have read CONTRIBUTING.md
I can and do license this contribution under the EFLv2
No issues are reported by make qa (runs make lint and make test)
I have tested the functionality of the things this change touches

…spam error messages until the bot is restarted

bronzeguy · 2023-12-11T01:31:33Z

I don't know how to add labels on github but easyfix would be appropriate here

Thanks

SnoopJ · 2023-12-11T01:43:29Z

@bronzeguy I understand what this change does, but we need to be able to reproduce the bug as part of our review process.

Some information we need:

What version of Sopel and Python is this?
What steps are necessary to reproduce the bug?
What errors do you see?

I am unable to reproduce the bug as you've described it using either the current tip of development or the last stable version (v7.1.9).

bronzeguy · 2023-12-11T02:03:04Z

My apologies I forgot to include this information earlier. This occurs on the latest version of Master branch [6a25d17] on the latest version of Python shipped in Debian stable which is 3.11.2. In order to reproduce the bug simply send a message using .tell where the recipient is a single @. For example .tell @ a It will send a message in the channel you are on where it says there was an out of index range error. Please let me know if there are any other issues

…

On Sun, Dec 10, 2023, 7:43 PM James ***@***.***> wrote: @bronzeguy <https://github.com/bronzeguy> I understand what this change does, but we need to be able to reproduce the bug as part of our review process. Some information we need: - What version of Sopel and Python is this? - What steps are necessary to reproduce the bug? - What errors do you see? I am unable to reproduce the bug as you've described it using either the current tip of development or the last stable version (v7.1.9). — Reply to this email directly, view it on GitHub <#2582 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BEDIA2GINTZEY2LZUUVI4RTYIZQMXAVCNFSM6AAAAABAO7276SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBZGE4TMOBVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bronzeguy · 2023-12-11T02:07:33Z

Adding on to the error message it will continue to send this error message for every message recieved

…

On Sun, Dec 10, 2023, 8:02 PM HikingSnow ***@***.***> wrote: My apologies I forgot to include this information earlier. This occurs on the latest version of Master branch [6a25d17] on the latest version of Python shipped in Debian stable which is 3.11.2. In order to reproduce the bug simply send a message using .tell where the recipient is a single @. For example .tell @ a It will send a message in the channel you are on where it says there was an out of index range error. Please let me know if there are any other issues On Sun, Dec 10, 2023, 7:43 PM James ***@***.***> wrote: > @bronzeguy <https://github.com/bronzeguy> I understand what this change > does, but we need to be able to reproduce the bug as part of our review > process. > > Some information we need: > > - What version of Sopel and Python is this? > - What steps are necessary to reproduce the bug? > - What errors do you see? > > I am unable to reproduce the bug as you've described it using either the > current tip of development or the last stable version (v7.1.9). > > — > Reply to this email directly, view it on GitHub > <#2582 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/BEDIA2GINTZEY2LZUUVI4RTYIZQMXAVCNFSM6AAAAABAO7276SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBZGE4TMOBVHE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

SnoopJ · 2023-12-11T02:08:06Z

It will send a message in the channel you are on where it says there was an out of index range error.

Please share the complete error report. We still cannot reproduce this issue with Python 3.11 on the current master (6a25d17).

dgw · 2023-12-11T02:09:15Z

@bronzeguy Logs would help a lot. As it stands, we're unable to reproduce this on the same Python version and git commit.

bronzeguy · 2023-12-11T03:11:11Z

Hello, here are some logs from a channel and the bot itself. I am still able to reproduce this issue.

SOPEL: https://pastebin.com/mQSDB1wE

[2023-12-10 21:09:19,458] sopel.bot            ERROR    - Unexpected IndexError (string index out of range) from kirby. Message was: .tell @ a
Traceback (most recent call last):
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/bot.py", line 695, in call_rule
    rule.execute(sopel, trigger)
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/plugins/rules.py", line 1274, in execute
    exit_code = self._handler(bot, trigger)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 291, in message
    tellees = list(reversed(sorted(
                            ^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 294, in <genexpr>
    if nick_match_tellee(nick, tellee)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 273, in nick_match_tellee
    if tellee[-1] in ['*', ':']:  # these are wildcard token
       ~~~~~~^^^^
IndexError: string index out of range
[2023-12-10 21:09:20,846] sopel.bot            ERROR    - Unexpected IndexError (string index out of range) from kirby. Message was: a
Traceback (most recent call last):
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/bot.py", line 695, in call_rule
    rule.execute(sopel, trigger)
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/plugins/rules.py", line 1274, in execute
    exit_code = self._handler(bot, trigger)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 291, in message
    tellees = list(reversed(sorted(
                            ^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 294, in <genexpr>
    if nick_match_tellee(nick, tellee)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 273, in nick_match_tellee
    if tellee[-1] in ['*', ':']:  # these are wildcard token
       ~~~~~~^^^^
IndexError: string index out of range
[2023-12-10 21:09:21,980] sopel.bot            ERROR    - Unexpected IndexError (string index out of range) from kirby. Message was: a
Traceback (most recent call last):
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/bot.py", line 695, in call_rule
    rule.execute(sopel, trigger)
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/plugins/rules.py", line 1274, in execute
    exit_code = self._handler(bot, trigger)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 291, in message
    tellees = list(reversed(sorted(
                            ^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 294, in <genexpr>
    if nick_match_tellee(nick, tellee)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/chris/git/.venv/lib/python3.11/site-packages/sopel/builtins/tell.py", line 273, in nick_match_tellee
    if tellee[-1] in ['*', ':']:  # these are wildcard token
       ~~~~~~^^^^
IndexError: string index out of range

IRC: https://pastebin.com/HrdXpKvJ

<kirby> .tell @ a
<bot> kirby: I'll pass that on when  is around.
<bot> Unexpected IndexError (string index out of range) from kirby. Message was: .tell @ a
<kirby> a
<bot> Unexpected IndexError (string index out of range) from kirby. Message was: a
<kirby> a
<bot> Unexpected IndexError (string index out of range) from kirby. Message was: a
<kirby> a
<bot> Unexpected IndexError (string index out of range) from kirby. Message was: a

Please let me know if there are any other issues

bronzeguy · 2023-12-11T03:13:14Z

Apologies for the last message with giant code snippet I do not know how to put it in properly on github, I have put snippets in a pastebins

SnoopJ · 2023-12-11T03:40:33Z

Thanks, we were able to reproduce. Turns out there was an issue on our end.

@dgw the patch here seems to fix the specific issue in question, but I think the more general solution might be to use Identifier to confirm that the tellee is appropriate, and bail out with an error if it isn't? What do you think?

Edit: I'm thinking here about the case where the tellee is @@ (and so on), but there are other forms of invalid tellees and it seems hard to deal with all of them this way. Unfortunately, Identifier("@") is valid (perhaps that's a bug?) so my original idea doesn't quite work.

dgw · 2023-12-11T03:55:51Z

Ah, we've discovered the dangers of allowing custom code to override builtins: @SnoopJ and I were at first testing a bot that had a custom tell.py file without this bug.

@SnoopJ The tellee already is converted to an Identifier, but it becomes a normal str due to the slice here if it starts with @. Could use a bit of refactoring. Plus as you discovered prior to your comment edit, '@' is a valid Identifier anyway (which also is_nick(); I checked).

The best way to fix this would be if Identifier was aware of what characters are allowed in a nickname or channel name, and refused to work if constructed from an invalid string. That capability won't make it into Sopel 8.0, though, certainly. We should fix what we can now and open a follow-up issue for the rest.

Your idea about using lstrip('@') is good, modulo the fact that that too returns a str instead of an Identifier.

Apologies for the last message with giant code snippet I do not know how to put it in properly on github, I have put snippets in a pastebins

I extracted the relevant parts and included them here. Use triple-backticks (```) around multi-line code snippets.

@bronzeguy Please look at the contributing guidelines in preparation for amending the patch. In particular, we'll want the commit message to match the style convention (see edited PR title for example) before it can be merged.

dgw · 2023-12-11T04:01:24Z

@bronzeguy Before you just amend for commit message style only, here's an idea: Add .lstrip('@') to the existing cleanup line, and remove the problematic if block entirely.

sopel/sopel/builtins/tell.py

Line 184 in 6a25d17

tellee = trigger.group(3).rstrip('.,:;')

Fix an edge case which allowed a user to put in an invalid tellee name which in turn would case the bot to spam index out of range error messages until Sopel was restarted.

SnoopJ · 2023-12-11T04:25:16Z

After discussion with @dgw on IRC, I've filed an alternative patch as #2584 (and added @bronzeguy as a co-author there). Thanks!

dgw · 2023-12-11T04:35:19Z

@bronzeguy We'll treat this PR as more or less an "issue" that will be fixed by #2584. We appreciate the bug report! Plus as @SnoopJ said, you'll get co-author credit on the new patch that builds on what you started here.

@SnoopJ Thanks for distilling this discussion into a PR that fixes the bug & simplifies the existing code, plus the follow-up issue.

Fix bug with builtin tell plugin that allowed a user to make the bot …

46226d4

…spam error messages until the bot is restarted

dgw added Needs Info Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Dec 11, 2023

dgw changed the title ~~Fix bug with builtin tell plugin that allowed a user to make the bot …~~ tell: fix error spam in edge case with @ Dec 11, 2023

dgw changed the title ~~tell: fix error spam in edge case with @~~ tell: fix edge case with @ as the tellee Dec 11, 2023

dgw removed the Needs Info label Dec 11, 2023

dgw added this to the 8.0.0 milestone Dec 11, 2023

tell: fix edge case with @ as the tellee

98a9322

Fix an edge case which allowed a user to put in an invalid tellee name which in turn would case the bot to spam index out of range error messages until Sopel was restarted.

This was referenced Dec 11, 2023

tell: check if a tellee is a valid identifier #2583

Open

tell: avoid errors produced by empty tellee #2584

Merged

dgw added the Replaced Superseded by a newer PR label Dec 11, 2023

dgw removed this from the 8.0.0 milestone Dec 11, 2023

dgw closed this Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tell: fix edge case with `@` as the tellee #2582

tell: fix edge case with `@` as the tellee #2582

bronzeguy commented Dec 11, 2023 •

edited by dgw

Loading

bronzeguy commented Dec 11, 2023

SnoopJ commented Dec 11, 2023

bronzeguy commented Dec 11, 2023 via email

bronzeguy commented Dec 11, 2023 via email

SnoopJ commented Dec 11, 2023

dgw commented Dec 11, 2023

bronzeguy commented Dec 11, 2023 •

edited by dgw

Loading

bronzeguy commented Dec 11, 2023

SnoopJ commented Dec 11, 2023 •

edited

Loading

dgw commented Dec 11, 2023

dgw commented Dec 11, 2023

SnoopJ commented Dec 11, 2023

dgw commented Dec 11, 2023

tell: fix edge case with @ as the tellee #2582

tell: fix edge case with @ as the tellee #2582

Conversation

bronzeguy commented Dec 11, 2023 • edited by dgw Loading

Description

Checklist

bronzeguy commented Dec 11, 2023

SnoopJ commented Dec 11, 2023

bronzeguy commented Dec 11, 2023 via email

bronzeguy commented Dec 11, 2023 via email

SnoopJ commented Dec 11, 2023

dgw commented Dec 11, 2023

bronzeguy commented Dec 11, 2023 • edited by dgw Loading

bronzeguy commented Dec 11, 2023

SnoopJ commented Dec 11, 2023 • edited Loading

dgw commented Dec 11, 2023

dgw commented Dec 11, 2023

SnoopJ commented Dec 11, 2023

dgw commented Dec 11, 2023

tell: fix edge case with `@` as the tellee #2582

tell: fix edge case with `@` as the tellee #2582

bronzeguy commented Dec 11, 2023 •

edited by dgw

Loading

bronzeguy commented Dec 11, 2023 •

edited by dgw

Loading

SnoopJ commented Dec 11, 2023 •

edited

Loading