Fix: Now can delete other user' messages in group chats (require group admin) #251
Conversation
Summary of ChangesHello @PoroFever, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances group chat moderation by empowering group administrators to remove messages sent by other participants. This is achieved by modifying the message deletion API to accept a sender identifier, which is then used by the backend to correctly revoke the message. This change provides a crucial tool for maintaining appropriate content within group conversations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the ability for group admins to delete messages from other users in a group chat by including a SenderJID in the payload. The changes are well-structured, but there's a critical security vulnerability: the code doesn't verify if the user making the request is actually a group admin, which contradicts the feature's intent. I've added a comment with a suggested fix for this. I've also included a minor suggestion to improve error handling.
| // Determine the sender JID: if provided, convert; otherwise use EmptyJID | ||
| var sender types.JID | ||
| if t.SenderJID != "" { | ||
| var parseErr error | ||
| sender, parseErr = types.ParseJID(t.SenderJID) | ||
| if parseErr != nil { | ||
| s.Respond(w, r, http.StatusBadRequest, errors.New("invalid SenderJID")) | ||
| return | ||
| } | ||
| } else { | ||
| sender = types.EmptyJID | ||
| } |
There was a problem hiding this comment.
The current implementation introduces a critical security vulnerability. It allows any user to delete any message in a group chat by providing the SenderJID, without verifying if the user making the request is a group admin. The pull request description mentions this should be restricted to admins, but the check is missing in the code. You should add a check to verify that the user is a group admin before allowing the deletion of another user's message. Comparing user JIDs should be done carefully; using ToNonAD() is a robust way to compare user identities without device-specific information.
// If SenderJID is provided, verify user is a group admin before proceeding.
var sender types.JID
if t.SenderJID != "" {
// This is an attempt to delete someone else's message.
// We must verify the chat is a group and the current user is an admin.
if recipient.Server != types.GroupServer {
s.Respond(w, r, http.StatusForbidden, errors.New("can only delete other users' messages in a group chat"))
return
}
groupInfo, err := client.GetGroupInfo(context.Background(), recipient)
if err != nil {
s.Respond(w, r, http.StatusInternalServerError, errors.New(fmt.Sprintf("failed to get group info: %v", err)))
return
}
currentUserJIDString := r.Context().Value("userinfo").(Values).Get("Jid")
currentUserJID, err := types.ParseJID(currentUserJIDString)
if err != nil {
s.Respond(w, r, http.StatusInternalServerError, errors.New("could not parse current user JID"))
return
}
isAdmin := false
for _, p := range groupInfo.Participants {
if p.JID.ToNonAD() == currentUserJID.ToNonAD() {
if p.IsAdmin {
isAdmin = true
}
break
}
}
if !isAdmin {
s.Respond(w, r, http.StatusForbidden, errors.New("you must be a group admin to delete other users' messages"))
return
}
// User is admin, proceed to parse SenderJID
var parseErr error
sender, parseErr = types.ParseJID(t.SenderJID)
if parseErr != nil {
s.Respond(w, r, http.StatusBadRequest, errors.New("invalid SenderJID"))
return
}
} else {
// Deleting own message, no admin check needed.
sender = types.EmptyJID
}| client.BuildRevoke(recipient, sender, t.Id), | ||
| ) | ||
| if err != nil { | ||
| s.Respond(w, r, http.StatusInternalServerError, errors.New(fmt.Sprintf("error sending message: %v", err))) |
There was a problem hiding this comment.
For better error handling and to preserve the original error context, it's recommended to use fmt.Errorf with the %w verb to wrap errors instead of errors.New(fmt.Sprintf(...)). This allows for more robust error inspection up the call stack.
s.Respond(w, r, http.StatusInternalServerError, fmt.Errorf("error sending message: %w", err))1 participant
Used to only delete own messages.
No can delete other users' messages (required group admin)
add SenderJID in payload