Thanks for adding this! I've been holding off with this implementation until someone brought it up because it presents a huge exploitation risk, particularly that of remote code execution.

Let me dig up past conversations and take another look at the code before we move forward with this.

Previous conversation in #1292

I reworked the docs for scripts so that the risk is clear, and double checked the logic to ensure the risk is minimized. I forgot I had already refactored things so that event data wasn't sent as an argument anymore, but that doesn't stop someone from referencing $SLSKD_SCRIPT_DATA in a command. There's a very conspicuous warning (complements of chatgpt 🙃) that makes this very clear.

I made the decision to send chat events before the blacklist check, and include blacklisted in the data

I'm still mulling this over; I try to put a lot of thought in design so that things work as folks (who probably haven't read the docs) expect, and I could see getting events from blacklisted users being unexpected. I can also see value in raising events when blacklisted users send messages, so I'm wondering if a separate event for that would be the best of both worlds.

On the other hand, I chose to filter out replayed messages entirely

This is a bit of a tricky one; if messages are being replayed it means something in the call chain failed and the messages weren't acknowledged. I'll need to think about the best way to handle that. It's probably important to re-send the replayed messages with the replayed flag set.

I also haven't been able to test PublicChatMessageReceived—does slskd actually have public chat capability? If not, I can remove this event if it would just be bloat.

If it does, it's buried somewhere. I'm not sure any modern clients actually support it.

Finally, with regards to the code: I currently raise the events directly in Application.cs

I think there are better spots for room and private messages, let me think about that a bit. The existing code is confusing.

@Lemmmy apologies for the delay, lots of distractions. I'm going to merge this into a branch so I can tweak it a bit.

The replay flag on messages is an interesting one that I'll need to build around; the server retains all private messages until the client acknowledges them. Right now they are set to auto-acknowledge after they are inserted into the database, and that works for the internal messaging stuff, but if someone wants to manage messages externally it can potentially cause messages to be lost.

I'm still thinking about the best way to handle this by default, but to support external flows completely I'll need to add an option to turn auto-ack off and add an endpoint for external apps to explicitly ack messages. The the best approach would be to only ack messages after they have been stored in the local db and when any scripts or webhooks had been executed successfully, but I'm not sure how well that will work with the current event bus implementation.

If it starts to turn into a big deal I'll just get this merged with auto-ack enabled and deal with it later.