ergochat / ergo

A modern IRC server (daemon/ircd) written in Go.
https://ergo.chat/
MIT License
2.27k stars 180 forks source link

histserv delete returns spurrious success message if incorrect parameters #2020

Open FiskFan1999 opened 1 year ago

FiskFan1999 commented 1 year ago

Just checked that this works on v2.11.0-rc1

msg histserv delete returns the success message no matter what the two parameters are (it does correctly throw an error if not enough or two many parameters are given).

On latest ergo, the following should all return the same message

msg histserv delete #<channel> <wrong msgid> msg histserv delete #<nonexistent channel> <some msgid> msg histserv delete first second

slingamn commented 1 year ago

I can't reproduce this. With the MySQL backend, I get:

@time=2022-12-25T07:14:34.980Z :HistServ!HistServ@example.network NOTICE netcat :Error deleting message: sql: no rows in result set

With the in-memory backend, I get:

@time=2022-12-25T07:15:11.898Z :HistServ!HistServ@example.network NOTICE netcat :Could not delete message
FiskFan1999 commented 1 year ago

thanks for your reply. Tested some more and I found that using a correct channel name and incorrect msgid returned the noop reply. I don't know how I thought it was something else in the past, my apologies.

I realized that using an incorrect channel name or other name for the target (which gets interpreted as a username) causes the issue however.

https://github.com/ergochat/ergo/blob/f6f7315458b8e70fb6211354663be2564798e08d/irc/server.go#L1057-L1073

If client == nil and channel == nil, that would mean that the channel or username was not found. This is not caught by this function, and hist is left as a nil pointer, which leads to server.historyDB.DeleteMsgid being called, which returns nil if mysql.db is nil. Since err == nil as a result of this, this caused the success message to be displayed on my ephemeral storage network.

Submitted #2024 which should fix this, by returning an error if the target is not a valid channel or username. We can discuss more over there, if you would consider this the intended behavior