app: MCP: Add permissions center - #6618
Conversation
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
app: add MCP permissions center— Description must start with a capital letter — e.g.frontend: HomeButton: Fix the buttonnotfrontend: HomeButton: fix the button.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
This PR adds a desktop-side MCP “permissions center” by persisting a per-server permission snapshot into MCP settings, enforcing that enabled servers with unapproved permission drift are skipped at runtime, and exposing permissions/tool-usage via a new Electron IPC endpoint.
Changes:
- Persist “approved effective permissions” into MCP settings on save, and skip enabled servers whose effective permissions no longer match the approved snapshot.
- Extend the MCP settings confirmation dialog to display effective permission details.
- Expose a new
mcp-get-permissionsIPC route via Electron preload, and add unit tests covering persistence/enforcement/permissions-center output.
Note: CI/check status and PR commit history (merge commits / commit coherence) are not available in the provided context—please confirm CI is green and the PR history is linear.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/electron/preload.ts | Exposes desktopApi.mcp.getPermissions() via ipcRenderer.invoke('mcp-get-permissions'). |
| app/electron/mcp/MCPSettings.ts | Adds permission snapshot types/helpers, approval enforcement when building mcpServers, and permissions summary output for the settings dialog/IPC. |
| app/electron/mcp/MCPSettings.test.ts | Updates tests for persisted permissions, adds coverage for unapproved-permission blocking and permissions-center output. |
| app/electron/mcp/MCPClient.ts | Adds mcp-get-permissions IPC handler that returns permissions-center data (plus tool usage). |
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
app: add MCP permissions center— Description must start with a capital letter — e.g.frontend: HomeButton: Fix the buttonnotfrontend: HomeButton: fix the button.app: align MCP permission env reporting— Description must start with a capital letter — e.g.frontend: HomeButton: Fix the buttonnotfrontend: HomeButton: fix the button.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/electron/mcp/MCPSettings.ts:133
- buildMcpServerEnv currently returns only the explicitly configured server.env values (or {}), which drops critical base environment like PATH/HOME/SystemRoot. That can prevent typical commands (e.g.
node,python,kubectl) from being found or can break runtimes that rely on HOME/TMP, even when permissions are otherwise approved. Consider inheriting a minimal, non-secret allowlist from process.env and including those keys in the permission snapshot/approval (since envKeys is derived from this function).
function buildMcpServerEnv(server: MCPServer): Record<string, string> {
return { ...(server.env || {}) };
}
|
@illume CI checks and review comments are green now. Please have a look whenever you have a chance to. |
b389ebc to
23738cb
Compare
|
@illume I reworded the commit messages to match the contribution guidelines. The force push retriggered CI, so I will wait for the new checks to finish. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
app/electron/mcp/MCPSettings.ts:133
- buildMcpServerEnv returns only the explicit server.env entries. Because this object is also used as the process environment for the spawned MCP server, it will typically omit required baseline variables like PATH (and on Windows, SystemRoot/PATHEXT). That can break command resolution for documented configs where
commandis a PATH-resolved executable (e.g.k8sgpt,flux-operator-mcp). Consider inheriting a minimal safe baseline env needed for process launching, then overlaying the explicitly approved server.env values so permissions reporting stays accurate.
function buildMcpServerEnv(server: MCPServer): Record<string, string> {
return { ...(server.env || {}) };
}
app/electron/mcp/MCPSettings.ts:424
- normalizeLastUsed calls
new Date(lastUsed).toISOString()without validating the input. If persisted tool state contains an invalid/unknown date string, this will throw a RangeError and break themcp-get-permissionsIPC handler. It’s safer to treat invalid dates asundefined.
function normalizeLastUsed(lastUsed?: Date | string): string | undefined {
if (!lastUsed) {
return undefined;
}
return new Date(lastUsed).toISOString();
}
|
@illume I checked the current branch commits and the commit titles on this PR are in the project format now. |
23738cb to
2712da2
Compare
2712da2 to
ad11916
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rootp1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ad11916 to
d491d01
Compare
|
@illume Pushed a fix for the last open comment: withApprovedMCPPermissions now keeps a server's existing approvedAt when its effective permissions are unchanged, instead of restamping it on every save. Squashed back into the single commit and all app tests, tsc, and lint are green. Would appreciate another look when you have time. |
39be7d8 to
dad5301
Compare
dad5301 to
c819baf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/electron/mcp/MCPSettings.ts:497
- The "request permission approval" change detection is currently too strict: it triggers whenever the saved
permissions.envKeysdiffers from the next effectiveenvKeys, even if the next permissions are already approved under the current snapshot (e.g. when the config narrows env access). This makes the dialog misleading and can also flag approval when the current server is unapproved but the next config returns to the approved baseline.
Instead of comparing snapshots with JSON.stringify and checking currentApproved, compute whether the next server would be approved under the current persisted snapshot using hasApprovedMCPServerPermissions({ ...nextServer, permissions: currentServer.permissions }). Only request approval when that returns false (or when no snapshot exists).
if (
!currentPermissions ||
!currentApproved ||
currentPermissions.command !== nextPermissions.command ||
JSON.stringify(currentPermissions.args || []) !== JSON.stringify(nextPermissions.args) ||
ebc9ad6 to
cb12963
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
app/electron/mcp/MCPSettings.ts:497
- This exact
envKeyscomparison still requests approval when access is narrowed. For example, removing one previously approved env key makes this dialog say “request permission approval,” whilehasApprovedMCPServerPermissions(nextServer)correctly reports the same effective permissions as approved because they are a subset. Use the same approval predicate here so the dialog does not contradict itself or prompt for non-broadened access.
currentPermissions.command !== nextPermissions.command ||
JSON.stringify(currentPermissions.args || []) !== JSON.stringify(nextPermissions.args) ||
JSON.stringify(currentPermissions.envKeys || []) !== JSON.stringify(nextPermissions.envKeys)
app/electron/preload.ts:106
- The linked feature asks for a user-facing permissions center that includes recent tool usage, but this API has no renderer consumer anywhere in the repository. The only visible surface added here is the save-confirmation summary, which calls
mcpPermissionsCenterwithout tool state and does not displayrecentToolUsage; therefore users still cannot inspect recent usage. Add a renderer UI that consumes this method, or narrow the PR/issue scope explicitly.
getPermissions: () => ipcRenderer.invoke('mcp-get-permissions'),
1767c21 to
783e0a4
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.
8a0e268 to
5021613
Compare
|
Addressed the remaining open thread: settings.json now persists a one-time permissionsMigrated marker, so a server added later by hand-editing the file (without a permissions field) is no longer silently auto-approved the way pre-existing legacy servers are — it now goes through the normal unapproved-permission flow. Also squashed onto a single commit without a Signed-off-by trailer, and fixed the PR title casing. CI was green on this branch before the push (the only failure was the Windows electron download EOF, unrelated to this change). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
app/electron/preload.ts:106
- The new API is not consumed anywhere in the renderer (the only repository match is this declaration), so users still cannot open a permissions center or inspect
approvedAtand recent tool usage. The native save-confirmation dialog is only shown during an update and does not expose those fields. Add a renderer surface that calls this API to satisfy issue #6617's user-inspection acceptance criterion.
getPermissions: () => ipcRenderer.invoke('mcp-get-permissions'),
app/electron/preload.ts:106
- The PR's sole commit has only the title
app: MCP: Add permissions center; it has no description explaining why permission persistence and enforcement are needed. Add a commit body that records the intent and rationale so the commit satisfies the repository's commit-message guidance.
getPermissions: () => ipcRenderer.invoke('mcp-get-permissions'),
app/electron/mcp/MCPSettings.ts:513
- This exact
envKeyscomparison contradicts the subset-aware approval check above. Removing an approved environment key is permitted byhasApprovedMCPServerPermissions, but this branch still says “request permission approval,” so the same dialog can report that the narrowed permissions are both approved and requesting approval. ReusehasApprovedMCPServerPermissionswith the current snapshot applied tonextServer.
!currentPermissions ||
!currentApproved ||
currentPermissions.command !== nextPermissions.command ||
JSON.stringify(currentPermissions.args || []) !== JSON.stringify(nextPermissions.args) ||
JSON.stringify(currentPermissions.envKeys || []) !== JSON.stringify(nextPermissions.envKeys)
Summary
This PR adds an MCP permissions center by persisting approved effective permissions for desktop MCP servers and exposing those permissions through Electron IPC.
Related Issue
Fixes #6617
Changes
mcpServersare constructed.mcp-get-permissionsIPC path exposed through preload so the desktop UI can inspect approved permissions and recent tool usage.Steps to Test
cd app && npx vitest run electron/mcp/MCPSettings.test.ts.npm run app:test:unit.npm run app:tsc.npm run app:lint.npm run lint.npm test.npm run app:build.Screenshots (if applicable)
There's no renderer-side UI, but the confirmation dialog itself is a real, user-facing surface (a native Electron
dialog.showMessageBox), so here's a screen recording of it showing the effective permission summary before applying MCP settings changes:Uploading demo.mp4…
Notes for the Reviewer
npm run app:test:e2ewas attempted but could not run becauseapp/e2e-testscurrently has notestscript.npm run lintattempt hit stale golangci-lint cache entries from another local checkout; rerunning with an isolatedGOLANGCI_LINT_CACHEpassed.Summary by CodeRabbit
New Features
Bug Fixes