[MM-69253] Restrict call leave to the view that created it and the widget#3884
Open
devinbinnie wants to merge 1 commit into
Open
[MM-69253] Restrict call leave to the view that created it and the widget#3884devinbinnie wants to merge 1 commit into
devinbinnie wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThe ChangesCalls leave sender validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant IpcMain
participant CallsWidgetWindow
Renderer->>IpcMain: emit calls-leave-call event
IpcMain->>CallsWidgetWindow: handleCallsLeave(event)
alt sender is widget or mainView
CallsWidgetWindow->>CallsWidgetWindow: close()
else sender mismatch
CallsWidgetWindow-->>IpcMain: log and return
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/specs/calls/calls_functionality.test.ts (1)
28-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLoose substring match risks false positives.
text.toLowerCase().includes('call')will match any unrelated post containing "call" as a substring (e.g. "recall", "called"), which could causepollCallStartOutcometo falsely report a'post'outcome and mask a real failure in the security fix this test suite validates.🔧 Suggested tighter match
const text = last.querySelector('.post-message__text')?.textContent ?? ''; - return text.toLowerCase().includes('call'); + return /\bcall\b/i.test(text);🤖 Prompt for 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. In `@e2e/specs/calls/calls_functionality.test.ts` around lines 28 - 36, The post detection in the calls functionality test is too broad because the serverWin.evaluate check uses a loose substring match on the post text, which can falsely treat unrelated posts as call-related. Tighten the matcher in the newPostMentionsCall logic by checking for a more specific call-related phrase or token boundary instead of any occurrence of "call", and keep the existing post selection/idBefore guard so pollCallStartOutcome only returns 'post' for an actual relevant call post.
🧹 Nitpick comments (1)
src/app/callsWidgetWindow.test.js (1)
366-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for the popout sender.
isCallsWidgetalso matchesthis.popOut?.webContents.id, but no test exerciseshandleCallsLeavewith a popout sender id (only the main widgetwinid is covered). Consider adding a case that setscallsWidgetWindow.popOut = {webContents: {id: 'popoutID'}}and assertscloseis called.✅ Suggested additional test
it('should close when the sender is the main view of the call', () => { callsWidgetWindow.handleCallsLeave({sender: {id: 'mainViewID'}}); expect(callsWidgetWindow.close).toHaveBeenCalled(); }); + + it('should close when the sender is the calls widget popout', () => { + callsWidgetWindow.popOut = {webContents: {id: 'popoutID'}}; + callsWidgetWindow.handleCallsLeave({sender: {id: 'popoutID'}}); + expect(callsWidgetWindow.close).toHaveBeenCalled(); + }); });🤖 Prompt for 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. In `@src/app/callsWidgetWindow.test.js` around lines 366 - 397, `handleCallsLeave` is missing coverage for the popout sender path in `CallsWidgetWindow`. Add a test in the `describe('handleCallsLeave')` block that sets `callsWidgetWindow.popOut` with a `webContents.id` value, then calls `handleCallsLeave` with a matching sender id and asserts `close` is invoked, alongside the existing `win` and `mainView` cases.
🤖 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.
Outside diff comments:
In `@e2e/specs/calls/calls_functionality.test.ts`:
- Around line 28-36: The post detection in the calls functionality test is too
broad because the serverWin.evaluate check uses a loose substring match on the
post text, which can falsely treat unrelated posts as call-related. Tighten the
matcher in the newPostMentionsCall logic by checking for a more specific
call-related phrase or token boundary instead of any occurrence of "call", and
keep the existing post selection/idBefore guard so pollCallStartOutcome only
returns 'post' for an actual relevant call post.
---
Nitpick comments:
In `@src/app/callsWidgetWindow.test.js`:
- Around line 366-397: `handleCallsLeave` is missing coverage for the popout
sender path in `CallsWidgetWindow`. Add a test in the
`describe('handleCallsLeave')` block that sets `callsWidgetWindow.popOut` with a
`webContents.id` value, then calls `handleCallsLeave` with a matching sender id
and asserts `close` is invoked, alongside the existing `win` and `mainView`
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc23e19-2bde-4cde-94a9-3fa059430755
📒 Files selected for processing (3)
e2e/specs/calls/calls_functionality.test.tssrc/app/callsWidgetWindow.test.jssrc/app/callsWidgetWindow.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
leaveCalldesktop API did not validate the sender, so a server view could affect a call owned by a different server. This PR adds sender validation so the request is only honored when it comes from the server that owns the call.Ticket Link
https://mattermost.atlassian.net/browse/MM-69253
Release Note
Change Impact: 🟠 Medium
Regression Risk: This changes sender validation in the call-leave IPC flow, which is a user-facing call-control path and introduces authorization-like checks. The affected code is localized to the Calls widget window, and the new tests cover the main allowed/denied cases, but there is still risk around edge cases where sender identity or window state differs.
QA Recommendation: Recommend targeted manual QA for leaving calls from the widget and from the owning view, plus a quick negative check that other views cannot end the call.
Generated by CodeRabbitAI