Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

histserv delete returns spurrious success message if incorrect parameters #2020

Open
FiskFan1999 opened this issue Dec 23, 2022 · 2 comments · May be fixed by #2024
Open

histserv delete returns spurrious success message if incorrect parameters #2020

FiskFan1999 opened this issue Dec 23, 2022 · 2 comments · May be fixed by #2024
Labels
Milestone

Comments

@FiskFan1999
Copy link
Contributor

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
Copy link
Member

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 added a commit to FiskFan1999/ergo that referenced this issue Dec 25, 2022
Fixes ergochat#2020

Calling /msg histserv delete #wrongchan <msgid> returns spurrious success
message. Calling /msg histserv delete <randomchars> <msgid> returns
similar message.

These would lead to the pointer hist == nil, and
server.historyDB.DeleteMsgid() would return nil.

Returns errNoSuchChannel or errInvalidTarget if channel or client == nil
@FiskFan1999
Copy link
Contributor Author

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.

ergo/irc/server.go

Lines 1057 to 1073 in f6f7315

if target != "" {
if target[0] == '#' {
channel := server.channels.Get(target)
if channel != nil {
if status, _, _ := channel.historyStatus(config); status == HistoryEphemeral {
hist = &channel.history
}
}
} else {
client := server.clients.Get(target)
if client != nil {
if status, _ := client.historyStatus(config); status == HistoryEphemeral {
hist = &client.history
}
}
}
}

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

@slingamn slingamn added this to the v2.12.0 milestone Dec 25, 2022
@slingamn slingamn added the bug label Dec 25, 2022
FiskFan1999 added a commit to FiskFan1999/ergo that referenced this issue Dec 25, 2022
If hist == nil and mysql database Delete msgid function returns
ErrDBIsNil, we know that the target does not match any channel or user.
Return invalid target error to operator (see ergochat#2020)
@slingamn slingamn modified the milestones: v2.12.0, v2.13 Oct 18, 2023
@slingamn slingamn modified the milestones: v2.13, 2.14 Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants