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

bronzeguy commented 6 months ago

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

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

bronzeguy commented 6 months ago

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

Thanks

SnoopJ commented 6 months ago

@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 commented 6 months ago

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 https://github.com/sopel-irc/sopel/pull/2582#issuecomment-1849196859, or unsubscribe https://github.com/notifications/unsubscribe-auth/BEDIA2GINTZEY2LZUUVI4RTYIZQMXAVCNFSM6AAAAABAO7276SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBZGE4TMOBVHE . You are receiving this because you were mentioned.Message ID: @.***>

bronzeguy commented 6 months ago

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 https://github.com/sopel-irc/sopel/pull/2582#issuecomment-1849196859, or unsubscribe https://github.com/notifications/unsubscribe-auth/BEDIA2GINTZEY2LZUUVI4RTYIZQMXAVCNFSM6AAAAABAO7276SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBZGE4TMOBVHE . You are receiving this because you were mentioned.Message ID: @.***>

SnoopJ commented 6 months ago

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 commented 6 months ago

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

bronzeguy commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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.

https://github.com/sopel-irc/sopel/blob/6a25d173444ea50025c4e7cae98f727d49c00b4e/sopel/builtins/tell.py#L184

SnoopJ commented 6 months ago

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

dgw commented 6 months ago

@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.

sopel-irc / sopel

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

Description

Checklist