MM-68829 Improving UX for DMs sent on PR comments#1010
Conversation
- Preventing edit or delete PR message events from sending a "new comment" DM notification - Improving logic that cleans body to prevent issues with html comments in the message from interfering with visibility of message
avasconcelos114
added
2: Dev Review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds mention and author notification flows for PR review comments. Introduces cleanBody template function, invokes mention/author handlers on created review comments, parses mentioned users, enforces permission and mute checks, sends direct posts or DMs, and updates tests and mocks. ChangesReview Comment Mention and Author Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/plugin/webhook.go`:
- Around line 1106-1111: The comparisons that suppress self/author mentions are
using case-sensitive equality against event.GetSender().GetLogin() and
event.GetPullRequest().GetUser().GetLogin(), so usernames with different casing
can bypass suppression; update those checks to perform case-insensitive
comparisons (e.g., use strings.EqualFold or compare lowercased strings) for the
expressions that reference username, event.GetSender().GetLogin(), and
event.GetPullRequest().GetUser().GetLogin(); make the same change at the other
occurrence around lines 1143-1145 so both sender and PR author checks are
case-insensitive.
- Around line 1091-1097: The code reads the comment body into body and only
strips the email footer before calling parseGitHubUsernamesFromText, but HTML
comment blocks (<!-- ... -->) can still contain mentions and trigger DMs; before
calling parseGitHubUsernamesFromText, remove or sanitize any HTML comments from
the comment text (the variable body produced from event.GetComment().GetBody())
so mentions inside <!-- --> are ignored, then pass the cleaned body to
parseGitHubUsernamesFromText (update the body variable or use a new cleanedBody)
to ensure hidden-comment mentions are not parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03b5dc71-be81-4372-b8f0-96ade155671a
📒 Files selected for processing (4)
server/plugin/template.goserver/plugin/test_utils.goserver/plugin/webhook.goserver/plugin/webhook_test.go
avasconcelos114
changed the title
| } | ||
| } | ||
|
|
||
| func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullRequestReviewCommentEvent) { |
There was a problem hiding this comment.
handleCommentMentionNotification filters out mentioned users who are also assignees. Maybe worth mirroring the skip logic for consistency and to avoid double DM?
There was a problem hiding this comment.
For the PullRequestReviewCommentEvent events there isn't an assignee handler like we do for IssueCommentEvent, only one handler for the author which this function is skipping inhandleReviewCommentMentionNotification:1112 to avoid duplication, so I think in here we're safe from the risk of double DMing here
|
|
||
| func (p *Plugin) handleCommentAssigneeNotification(event *github.IssueCommentEvent) { | ||
| action := event.GetAction() | ||
| if action == actionEdited || action == actionDeleted { |
There was a problem hiding this comment.
We can check if handleCommentAssigneeNotification caller already filters out edit and delete actions. If yes this guard is dead code.
There was a problem hiding this comment.
The caller (handleWebhook) doesn't filter out by action, this has been done for each individual handler previously but this handler in particular was missing it
I'm wondering if we want to move the guard up to handleWebhook but not entirely certain as this would deviate from how every other webhook event is processed (specific action-related guards happening at the handler function-level), what do you think works best here?
| } | ||
| cleaned = mdCommentRegex.ReplaceAllString(cleaned, "") | ||
| cleaned = strings.TrimSpace(cleaned) | ||
| if cleaned == "" { |
There was a problem hiding this comment.
nit: this if cleaned == "" { return "" } block returns "" either way. Just return cleaned
|
This pull request may allow notification and refresh-event spam in
🟡 Notification and Refresh Event Spam in
|
| Vulnerability | Notification and Refresh Event Spam |
|---|---|
| Description | The handleReviewCommentMentionNotification function processes mentions in GitHub pull request review comments and, for each mentioned user, sends a direct message via Mattermost and triggers a client refresh event. While the parseGitHubUsernamesFromText function iterates through the comment text, the function does not implement a maximum limit on the number of mentions processed per comment. An attacker could potentially mention a large number of users in a single comment to cause significant load on the Mattermost server and client. |
mattermost-plugin-github/server/plugin/webhook.go
Lines 1107 to 1110 in ea29a91
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
2 participants
Summary
This PR mainly does two things to improve the general experience of receiving DM notifications from the GitHub bot
Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-68829
QA Notes
The main webhook event affected by this is the PR commenting, which mainly consists of:
We'll want to make sure that editing a PR comment does not send a new notification to the users as a DM
We might also want to run some regression tests on the following DM notifications to confirm they still append a quote with the body of the comment as well:
Change Impact: 🟡 Medium
Reasoning: The PR adds new DM notification handlers for PR review comments and updates message-body cleaning logic used across multiple DM templates; these are user-facing changes spanning several files (webhook handling, templates, and tests) but remain focused on notification behavior.
Regression Risk: Moderate. Early-return guards reduce unwanted DMs, but new code paths (user mapping, permission checks, muted-user filtering, and DM posting) introduce failure points that could silently suppress or misroute notifications. The new cleanBody helper changes message rendering for many templates; while templates were updated, dedicated tests for all template output permutations appear limited.
** QA Recommendation:** Perform targeted manual QA on PR comment workflows: verify that editing or deleting PR comments no longer sends "new comment" DMs, and confirm quoted comment bodies are correctly cleaned/omitted for the affected notification types (commentMentionNotification, commentAuthorIssueNotification, commentAssigneePullRequestNotification, commentAssigneeIssueNotification, commentAssigneeSelfMentionPullRequestNotification, commentAssigneeSelfMentionIssueNotification). Automated tests cover many scenarios but manual verification is recommended for edge cases (HTML comments, GitHub footer removal, and permission/mapping failures).
Generated by CodeRabbitAI