Repository navigation
MCP tools: ownership checks and cleanup after the access gate #15962
Description
Follow-ups from the MCP access declarations (#16335, which replaced #15961). I audited all 72 T3 MCP tools; these gaps are outside what the gate checks, because they depend on who owns a resource rather than on the caller's modes.
Ownership checks missing
device_screenshotwith an explicitdeviceIddoesn't check that device belongs to the calling thread's sessions, so a thread can screenshot a device another thread opened (apps/server/src/mcp/toolkits/device/handlers.ts, thedeviceIdbranch).- Preview tab ownership isn't enforced on the server: the broker uses tab ownership only to pick a host and forwards
threadId/tabIdto the desktop (PreviewAutomationBroker.ts). Confirm the desktop refuses another thread's tab, or check it here. t3_attachment_discarddoesn't check that the pending upload belongs to the caller (toolkits/attachment/handlers.ts).
Annotations that don't match behavior
preview_snapshotis annotated read-only but writes a PNG to disk withsave: true.preview_opendeclaresDestructive: false, butbrowserTooloverwrites it totrue.
Cleanup left after #16335
- feat(server): every T3 MCP tool declares who may call it #16335 removed the per-handler copies of the caller checks (
readMutationCaller,readFullAccessCaller,readWritableThread, and the copies inThreadMetadataMcpServiceand the pull-request handlers).OrchestratorMcpServicestill repeats some internally:loadCaller,assertLiveCallerForOtherThread/ForOtherProject, and its own mode checks in send, interrupt and scheduled-task updates. These can shrink to the checks only it can make, such as a task belonging to its parent. OrchestratorMcpService.loadCallerdoesn't check whether the calling thread is deleted. The declarations cover this for every call, but the service-level helper is still inconsistent withthreadAccess.loadCaller.
Decided
t3_thread_launchwithscratch: truestays open to any caller within its own modes, including Supervised (Julius, 2026-10-05).
Activity
juliusmarminge commentedon Nov 12, 2026
I tried to reproduce this on the current build and could not.
On origin/main 080b10c I checked each item in the issue against the source. device_screenshot with a deviceId calls DeviceService.locateSession, which only matches sessions where session.threadId equals the caller's thread, so this is already fixed. t3_attachment_discard rejects callers other than the upload's owner ('Only the caller that prepared a pending upload can discard it.'), so this is fixed. preview_snapshot is now wrapped in safeBrowserTool and is no longer Readonly, so this is fixed. Still open: (1) preview_open. The tool is built with .annotate(Tool.Destructive, false), and then browserTool() wraps it and applies Destructive=true over that. I added a test to apps/server/src/mcp/toolkits/preview/tools.test.ts asserting Context.get(PreviewToolkit.tools.preview_open.annotations, Tool.Destructive) === false. It fails on main ('expected true to be false'), and the two existing tests pass. I did not drive a live /mcp tools/list on a dev server, because that endpoint needs a per-session bearer token or an OAuth client. (2) OrchestratorMcpService.loadCaller still loads the caller's records without a deletedAt check, while loadTargetThread does check it; the McpToolAccess gate covers deleted callers, so nobody can hit this. (3) assertLiveCallerForOtherThread and assertLiveCallerForOtherProject are still in OrchestratorMcpService (cleanup). (4) Preview tab ownership is enforced on the server-browser path (SessionControl.ts 'This tab belongs to another agent.', ServerBrowser.ts). I did not confirm the desktop-host path.
Closing as not planned. If it still happens for you, reply with the exact steps and the version you are on and it will be looked at again.