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.

Sign up for GitHub

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

Jump to bottom

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
bug
Milestone
selected

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 :[email protected] 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 :[email protected] NOTICE netcat :Could not delete message

FiskFan1999 added a commit to FiskFan1999/ergo that referenced this issue Dec 25, 2022
@FiskFan1999
server.DeleteMessage catches channel or client==nil
bc40477 
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 FiskFan1999 linked a pull request Dec 25, 2022 that will close this issue
Open
@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
@FiskFan1999
Eph. memory catches invalid target (hi.s. delete)
f8cd846 
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
@slingamn slingamn modified the milestones: v2.14, selected Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
selected
Development

Successfully merging a pull request may close this issue.

server.DeleteMessage catches channel or client==nil FiskFan1999/ergo
2 participants
@slingamn @FiskFan1999