technically you should use \ because then it's a continuation and gets parsed as one string rather than parsed as two and have to be concatenated as an unnecessary operation
@ArtOfCode They aren't always failing. These failures are not just the "normal" failures. The CI record idicates there are 3 separate failures. Only the one in the middle is "normal". Both of the errors which are "ActiveRecord::StatementInvalid: Mysql2::Error: Cannot delete or update a parent row" are new and not "normal".
The error which includes "Load data local infile forbidden" is "normal" and intermittent (and the reported error isn't isn't the actual problem). (cc @NobodyNada-ReinstateMonica)
@ArtOfCode Cool. Are they going to be errors which are to be always expected? In other words, do we need to either adjust the tests or remove them to return to having the frustratingly intermittent error as the only error in the tests?
@Makyen I have no access to my password database now. Is it possible for you to directly edit the source code? (I think that I enabled "allow people with write access to edit")
@user12986714 I suspect that it's going to be a race condition in getting the raw version of the message vs. validating the number of arguments against the non-raw version. I assume, but I haven't checked the code. The assumption is based on the fat that getting the raw version of the message is a separate HTTPS access. So, the message was probably deleted prior to the raw version being fetched.
@user12986714 Room Owners have access to deleted messages in any room where they are an owner. Moderators have access to deleted messages everywhere.
@Makyen Looks that way to me too. The "Missing an argument" check is done against message.content while the "Invalid pattern" check is done against message.content_source.
It's been forever since I've touched CE, but IIRC content is what comes over the websocket, while content_source is the one that does a seperate request
@NobodyNada-ReinstateMonica Yeah, but SD is logged into chat and is a room owner here. It should return valid message content. (I checked in a test room).
@Makyen yes, that is what I thought too. However on line 515, in chatcommunicate.py the function dispatch_command takes an argument msg, then parses it and validates the args, and then calls the command with msg. Therefore I got really confused where the bug is.
Great point. So why it is implemented like that? Maybe it should validate against content_source. Also is chatexchange an external library, as otherwise we can implement locking/validation within chatexchange.
but for a deleted message, it just says "deleted this message", and the .message-source element is empty
and content_source is an empty string
@user12986714 If I recall correctly (and remember that I last touched this code 2 or 3 years ago), we want to avoid calling content_source whenever possible for performance, since it makes an extra HTTP request
There's quite a bit of information available on the history page, when you ask CE for anything that can be obtained by scraping history, it grabs and caches all of it
@NobodyNada-ReinstateMonica I suppose as we say msg.content rather than msg.content(), it means content field is already in msg the time it is passed to dispatch_command? Same as content_source.
@NobodyNada-ReinstateMonica Looks like this means a trade off. We have to either sacrifice performance or accept that content_source may not be the same as content. Also, why don't we directly fetch content_sources, and process content out of content_source? (auto completion is annoying)
@user12986714 because we get content for free over the websocket, while the only way to get content_source is by making an HTTP request
We generally accept that content_source and content may be different. content is almost always good enough, except for things like blacklist commands that might include special characters that get eaten by SE
content is what's displayed in chat and includes having been processed from Markdown into HTML. content_source is the Markdown. That matters for things like regular expressions, where the content can have characters which would be recognized as Markdown and changed.
Even if we fix the scraping of the history page, it would still break in other rooms like SOCVR where Smokey isn't a room owner and the history page 404's for deleted messages
True. Just showing an error message is fine. Even the one we had originally would be OK. I just didn't see it as something that should be logged in errorlogs as an uninformative IndexError.
What about this: let's just accept that users may edit/delete command before content_source is fetched. Then check in dispatch_command if the content_source fetched is empty, throw a msg to chat and refuse to execute the command. Otherwise just pass the content_source to the target command function. Also document this and tell users that it is a bad idea to edit commands shortly after being posted.
At least after inspection I don't think users can exploit this issue for privilege escalation.
@user12986714 I'm kinda inclined to say this is enough of a rare case that it's not worth the trouble of fixing, though if you really want to take a stab at it feel free.
I'd just adjust the error message we have now. Add a comment in the code that it's likely the user deleted the chat message and have a URL to this conversation in the code comment.
IMO a proper fix would be to recognize deleted messages in browser.get_history, and either properly populate content_source with the last undeleted version, or throw an exception or something like that
@Makyen this issue is global to all commands requiring original message. Hence it is better fixed in dispatch_command rather than writing every time in each command.
@user12986714 Yes and no. dispatch_command currently knows nothing of if the command is going to use content_source, and we don't want to require the extra HTTPS access for every command we process. So, it either needs to be dealt with in the commands that want to use content_source, or we have to have a parameter in the setup of commands that says the extra data is required, which dispatch_command can then use to get the data.
IMO, the issue is that the blacklisting command was just making an assumption that the content_source would be valid, without any attempt to validate it or handle issues where it wasn't available.
@Makyen yeah, also all the other regex related commands I think? I just discovered that full_msg != will use content_source so my proposal is not valid :-/
@Makyen Socket Science is not a concern though as we are sure Smokey won't delete its own socket science msg, and ROs of the room set in rooms.yml are trusted and (probably) informed not to delete those msg.
This looks like mostly a "meh" issue from our POV. We'll already error-out and tell the user there was an error. So, other than documenting it, which can just be some code comments, we don't really need to handle this case. I just made the change because I didn't like seeing the error in errorlogs. Given that we now know what it really is and it's a "meh" for us, it really shouldn't be in errorlogs. So, I guess we should verify that we try/except where it will bite us.
@user12986714 Moderators can delete or edit any message at any time. The owner of a chat message can delete or edit it for 2 minutes. Nobody else can delete or edit. ROs and moderators can move the message into another room. ROs used to be able to delete for 2 minutes, but that was removed 2 or 3 years ago, or so.
If we're going to do anything, we probably should also validate that content_source contains the same command that content does. I've seen cases where the chat message has been edited from one command to another and the wrong argument has been use for a changed command. Hmmm... Actually, if we do that validation, then it will cover the deleted case too.
Really, all we have to do is check that the content_source starts with the same command which content does, which is a very quick check, and only needs to be done in the commands where we use content_source.
@Makyen The problem is, after finding out that the command is edited to another one, what should we do? Raise a CmdException? Then that is of no difference from the one we currently implemented (after some minor changes in exception msg)
@user12986714 Yeah, it's a minor difference, but we can just have a single function def minimally_validate_content_source(msg): if msg.content.split(" ")[0] != msg.content_source.split(" ")[0]: raise CmdException('There was a problem with this command. Was the chat message edited or deleted?") Then call minimally_validate_content_source(msg) at the beginning of any command where we are going to use content_source.
Oooops let's give up. Just use the current implementation and tell users that it is a terrible idea to edit commands in a short time and the outcome is undefined.
@user12986714 I expect it's happened, but we'll already error out, and only quite rarely. Someone deleting the command isn't all that uncommon. They really should just leave the command in the transcript, but clicking "delete" is an easy thing to think to do.
@user12986714 But....but... it's such a nice rat hole... it looks comfy... maybe we can co down it a bit further...
Frankly, the most important thing is adding some code comments which just say that the split can fail if the user edited or deleted the chat message. Then if someone else sees the errors in the log they don't spend a huge time re-figuring out what's going on.
@Makyen That pattern looks like it's already caught by Potentially bad keyword in body and Potentially bad keyword in answer; append -force if you really want to do that.
[ SmokeDetector | MS ] Bad ip for hostname in body, bad keyword in body, bad keyword in title, blacklisted website in body, pattern-matching website in body, +2 more (492): Keto Body Tone France by roat on askubuntu.com
@Makyen That pattern looks like it's already caught by Potentially bad keyword in answer and Potentially bad keyword in body; append -force if you really want to do that.
[ SmokeDetector | MS ] Bad ip for hostname in body, bad keyword in body, bad keyword in title, link at end of body, pattern-matching website in body, +2 more (492): Where To Buy Trim Fast Keto? by fastau24 on workplace.SE
