abstract_backends: safe() all prepare_command args

[half-duplex]

Description

We previously had a silly pattern of calling safe() for most (but not all!) arguments passed to send_command. This is usually not an issue since the contents are either not user-generated or must be sent through IRC (so cannot contain \r\n), but sometimes that's not the case, especially for privmsg/notice. Some plugins may also call send_command directly. Let's just pre-empt the whole class of potential problems by safe()ing all args in prepare_command().

Supersedes #2356. Thanks to @semarie for finding this issue.

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 quality and make test)
• I have tested the functionality of the things this change touches

[dgw]

Indeed it was a silly pattern. You've done what I hadn't gotten around to yet (irl business / busy-ness).

(Haven't looked for missed calls to safe() because of the janky review setup I'm using tonight, as mentioned on IRC. For that reason I'll wait to merge until either someone else takes a look or I get to a proper PC setup to take another look-see myself.)

[half-duplex]

Thanks for the reminder to check other files for safe()s - the args= one is fine to remove, and I think the recipients one is okay but another pair of eyes would be good, I'm not confident I'm following what's going on there fully.

[dgw]

the args= one is fine to remove, and I think the recipients one is okay but another pair of eyes would be good, I'm not confident I'm following what's going on there fully.

I'm not convinced it's good to remove safe() from the recipient_stack stuff. Or rather, I think this patch has indirectly revealed a bug in the older implementation, where text (not safe()d) was being searched for in a list of safe()d values saved before. For true correctness in the loop detection feature, I would think it's wiser to safe(text) somewhere before the checking/caching logic because what's most important there is not the exact input but what is (or would) be sent to IRC.

@Exirel Any thoughts on this bit? I know you already approved (and so did I), but we hadn't noticed/thought of this detail before.

[Exirel]

I think we can keep the safe in that context, and go with .count(safe(text)) to ensure we always check against the safed version of the text.

[dgw]

I had a think about putting safe() back in irc/__init__.py for use with the recipient_stack business, and came to the decision that it's not worth it. Repetition detection is most likely to trigger by the output of a single callable, which will take the same code path through the bot every time to send its message, and that message will either already be safe()d or not, consistently.

In the future (post-8.0) we should consider making the IRC pipeline convert messages to a custom data type (possibly subclass of str or collections.UserString) that implements "IRC safety" constraints like stripping forbidden characters, and stop passing around basic str once text has been handed over to the bot for sending to IRC.

That's a whole separate discussion that I don't want to start just yet, though. For now I will close and reopen the PR, which will (IIRC) rerun the CI checks and satisfy the missing Python 3.11 test requirement, and then merge assuming all still passes.