Refine MCP tool access controls
This commit is contained in:
1 parent
a2e64cd6fb
commit
8c2dae2a94
8 files changed
+129
-58
No files matched your search
@@ -36,11 +36,12 @@ nonisolated enum AgentMCPTools {
|
||||
// make this *stricter*. Nothing a server asserts can grant
|
||||
// parallelism, so the rule that remote annotations are risk hints
|
||||
// rather than proof of safety still holds — and a tool the server
|
||||
// marks destructive, or wants asked about every time, takes its
|
||||
// marks destructive, or is configured to ask every time, takes its
|
||||
// whole turn serial.
|
||||
concurrency: tool.annotations.destructiveHint == true || tool.requiresApproval
|
||||
concurrency: tool.annotations.destructiveHint == true
|
||||
|| tool.accessPolicy == .alwaysAsk
|
||||
? .sequential : .parallel,
|
||||
alwaysAskUser: tool.requiresApproval,
|
||||
alwaysAskUser: tool.accessPolicy == .alwaysAsk,
|
||||
presentation: .init(
|
||||
symbol: "puzzlepiece.extension",
|
||||
activity: .label(MCPToolNaming.qualifiedName(
|
||||
|
||||
@@ -458,11 +458,9 @@ final class AIConfigurationModel {
|
||||
|
||||
/// What a successful `Connect` writes back.
|
||||
///
|
||||
/// A tool the user had already switched off, or marked as not needing
|
||||
/// approval, keeps those settings across a rediscovery — otherwise every
|
||||
/// reconnect would silently undo the decisions made about this server. A
|
||||
/// tool that has genuinely appeared for the first time arrives at the
|
||||
/// defaults, which means approval required.
|
||||
/// A tool keeps its access policy across rediscovery — otherwise every
|
||||
/// reconnect would silently undo the decision made about this server. A
|
||||
/// genuinely new tool arrives at the default, which means always asking.
|
||||
func setTools(_ discovered: [MCPTool], version: String, for id: MCPServer.ID) {
|
||||
guard let index = mcpServers.firstIndex(where: { $0.id == id }) else { return }
|
||||
let previous = Dictionary(uniqueKeysWithValues: mcpServers[index].tools.map { ($0.id, $0) })
|
||||
@@ -470,20 +468,17 @@ final class AIConfigurationModel {
|
||||
mcpServers[index].tools = discovered.map { tool in
|
||||
guard let old = previous[tool.id] else { return tool }
|
||||
var merged = tool
|
||||
merged.isEnabled = old.isEnabled
|
||||
merged.requiresApproval = old.requiresApproval
|
||||
merged.accessPolicy = old.accessPolicy
|
||||
return merged
|
||||
}
|
||||
mcpServers[index].negotiatedVersion = version
|
||||
mcpServers[index].lastConnectedAt = Date()
|
||||
}
|
||||
|
||||
func setToolEnabled(_ enabled: Bool, tool toolID: String, on serverID: MCPServer.ID) {
|
||||
mutate(toolID, on: serverID) { $0.isEnabled = enabled }
|
||||
}
|
||||
|
||||
func setToolRequiresApproval(_ required: Bool, tool toolID: String, on serverID: MCPServer.ID) {
|
||||
mutate(toolID, on: serverID) { $0.requiresApproval = required }
|
||||
func setToolAccessPolicy(
|
||||
_ policy: MCPTool.AccessPolicy, tool toolID: String, on serverID: MCPServer.ID
|
||||
) {
|
||||
mutate(toolID, on: serverID) { $0.accessPolicy = policy }
|
||||
}
|
||||
|
||||
private func mutate(_ toolID: String, on serverID: MCPServer.ID, _ change: (inout MCPTool) -> Void) {
|
||||
|
||||
@@ -88,7 +88,7 @@ nonisolated struct MCPServer: Identifiable, Hashable, Sendable {
|
||||
}
|
||||
|
||||
var enabledTools: [MCPTool] {
|
||||
tools.filter(\.isEnabled)
|
||||
tools.filter { $0.accessPolicy != .disabled }
|
||||
}
|
||||
|
||||
var isReachable: Bool {
|
||||
@@ -182,14 +182,19 @@ nonisolated struct MCPHeader: Identifiable, Hashable, Sendable {
|
||||
|
||||
/// One tool a server offers.
|
||||
///
|
||||
/// `requiresApproval` defaults to `true`, and that default is the point rather
|
||||
/// than caution. Termind's rule is that nothing runs on the user's say-so alone —
|
||||
/// an AI-proposed command is always a click away from the PTY, never closer. An
|
||||
/// MCP tool is a command by another name, and it arrives from a list fetched
|
||||
/// over the network from a server the model can talk to. If the default went
|
||||
/// the other way, the one rule the whole feature rests on would have a hole in
|
||||
/// it that nobody opened deliberately.
|
||||
/// `accessPolicy` defaults to `alwaysAsk`, and that default is the point rather
|
||||
/// than caution. An MCP tool is a command fetched over the network from a server
|
||||
/// the model can talk to, so a newly discovered tool must not silently inherit a
|
||||
/// more permissive conversation setting.
|
||||
nonisolated struct MCPTool: Identifiable, Hashable, Sendable {
|
||||
nonisolated enum AccessPolicy: String, CaseIterable, Identifiable, Hashable, Sendable {
|
||||
case disabled
|
||||
case alwaysAsk
|
||||
case followPermissions
|
||||
|
||||
nonisolated var id: String { rawValue }
|
||||
}
|
||||
|
||||
/// The tool name as the server reports it. Its identity, since MCP names
|
||||
/// are unique per server.
|
||||
var id: String
|
||||
@@ -199,8 +204,7 @@ nonisolated struct MCPTool: Identifiable, Hashable, Sendable {
|
||||
"type": .string("object"), "properties": .object([:])
|
||||
])
|
||||
var annotations: MCPToolAnnotations = MCPToolAnnotations()
|
||||
var isEnabled: Bool = true
|
||||
var requiresApproval: Bool = true
|
||||
var accessPolicy: AccessPolicy = .alwaysAsk
|
||||
|
||||
func matches(_ query: String) -> Bool {
|
||||
guard !query.isEmpty else { return true }
|
||||
|
||||
@@ -243,7 +243,7 @@ struct MCPServerEditorPage: View {
|
||||
} footer: {
|
||||
if !server.tools.isEmpty {
|
||||
Text(
|
||||
"With Ask First on, you approve each call. Turn it off only for trusted, read-only tools."
|
||||
"Always Ask requires approval for every call. Follow Permissions uses the conversation's permission mode and safety checks."
|
||||
)
|
||||
.font(.caption)
|
||||
.foregroundStyle(.secondary)
|
||||
@@ -353,7 +353,7 @@ private struct ToolRow: View {
|
||||
}
|
||||
|
||||
var body: some View {
|
||||
HStack(alignment: .top, spacing: Metrics.Space.m) {
|
||||
HStack(alignment: .center, spacing: Metrics.Space.m) {
|
||||
VStack(alignment: .leading, spacing: 1) {
|
||||
Text(qualifiedToolName)
|
||||
.font(.callout.monospaced())
|
||||
@@ -364,41 +364,41 @@ private struct ToolRow: View {
|
||||
.font(.caption)
|
||||
.foregroundStyle(.secondary)
|
||||
.lineLimit(2)
|
||||
.fixedSize(horizontal: false, vertical: true)
|
||||
}
|
||||
}
|
||||
.frame(minWidth: 0, maxWidth: .infinity, alignment: .leading)
|
||||
|
||||
Spacer(minLength: Metrics.Space.m)
|
||||
|
||||
Toggle("Offer", isOn: enabled)
|
||||
.toggleStyle(.switch)
|
||||
.controlSize(.mini)
|
||||
.fixedSize()
|
||||
|
||||
Toggle("Ask First", isOn: asksFirst)
|
||||
.toggleStyle(.switch)
|
||||
.controlSize(.mini)
|
||||
.fixedSize()
|
||||
// Greyed rather than hidden when the tool is off: the setting
|
||||
// still exists and comes back with it, and a control that
|
||||
// vanishes reads as one that was reset.
|
||||
.disabled(!tool.isEnabled)
|
||||
Picker("Permission", selection: permission) {
|
||||
ForEach(MCPTool.AccessPolicy.allCases) { option in
|
||||
Text(option.displayName).tag(option)
|
||||
}
|
||||
}
|
||||
.labelsHidden()
|
||||
.pickerStyle(.menu)
|
||||
.fixedSize()
|
||||
.accessibilityLabel(Text("Permission"))
|
||||
}
|
||||
.frame(maxWidth: .infinity)
|
||||
.font(.caption)
|
||||
.help(Text(tool.summary.isEmpty ? qualifiedToolName : tool.summary))
|
||||
}
|
||||
|
||||
private var enabled: Binding<Bool> {
|
||||
private var permission: Binding<MCPTool.AccessPolicy> {
|
||||
Binding(
|
||||
get: { tool.isEnabled },
|
||||
set: { app.aiConfiguration.setToolEnabled($0, tool: tool.id, on: serverID) }
|
||||
)
|
||||
}
|
||||
|
||||
private var asksFirst: Binding<Bool> {
|
||||
Binding(
|
||||
get: { tool.requiresApproval },
|
||||
set: { app.aiConfiguration.setToolRequiresApproval($0, tool: tool.id, on: serverID) }
|
||||
get: { tool.accessPolicy },
|
||||
set: { policy in
|
||||
app.aiConfiguration.setToolAccessPolicy(policy, tool: tool.id, on: serverID)
|
||||
}
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private extension MCPTool.AccessPolicy {
|
||||
var displayName: LocalizedStringKey {
|
||||
switch self {
|
||||
case .disabled: "Disabled"
|
||||
case .alwaysAsk: "Always Ask"
|
||||
case .followPermissions: "Follow Permissions"
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -17,7 +17,7 @@ nonisolated struct MCPCallResult: Hashable, Sendable {
|
||||
/// Discovery only. This runs the handshake and `tools/list`, and stops there —
|
||||
/// actually *calling* a tool belongs with the chat loop in P3, and it will go
|
||||
/// through the same approval gate an AI-proposed command does. That gate is why
|
||||
/// `MCPTool.requiresApproval` arrives from here already set.
|
||||
/// a discovered `MCPTool` arrives with its access policy set to always ask.
|
||||
///
|
||||
/// Two transports, both HTTP. There is no stdio here because Termind is sandboxed
|
||||
/// and cannot launch a local process — see `MCPServer`.
|
||||
@@ -477,7 +477,7 @@ nonisolated private struct ToolsListResult: Decodable {
|
||||
var inputSchema: AgentJSONValue?
|
||||
var annotations: MCPToolAnnotations?
|
||||
|
||||
/// Arrives enabled and requiring approval. See `MCPTool`.
|
||||
/// Arrives with the always-ask policy. See `MCPTool`.
|
||||
func tool() -> MCPTool {
|
||||
MCPTool(
|
||||
id: name, title: title ?? "", summary: description ?? title ?? "",
|
||||
|
||||
@@ -1371,6 +1371,16 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"Always Ask" : {
|
||||
"localizations" : {
|
||||
"zh-Hans" : {
|
||||
"stringUnit" : {
|
||||
"state" : "translated",
|
||||
"value" : "始终询问"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"Always included" : {
|
||||
"localizations" : {
|
||||
"zh-Hans" : {
|
||||
@@ -4633,6 +4643,16 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"Follow Permissions" : {
|
||||
"localizations" : {
|
||||
"zh-Hans" : {
|
||||
"stringUnit" : {
|
||||
"state" : "translated",
|
||||
"value" : "跟随权限设置"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"Follow System" : {
|
||||
"localizations" : {
|
||||
"zh-Hans" : {
|
||||
@@ -12786,12 +12806,12 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"With Ask First on, you approve each call. Turn it off only for trusted, read-only tools." : {
|
||||
"Always Ask requires approval for every call. Follow Permissions uses the conversation's permission mode and safety checks." : {
|
||||
"localizations" : {
|
||||
"zh-Hans" : {
|
||||
"stringUnit" : {
|
||||
"state" : "translated",
|
||||
"value" : "开启“先询问”后,每次调用都需你批准。仅对可信的只读工具关闭此选项。"
|
||||
"value" : "“始终询问”要求每次调用都由你批准;“跟随权限设置”则使用当前会话的权限模式和安全检查。"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -775,6 +775,33 @@ struct AgentRuntimeTests {
|
||||
})
|
||||
}
|
||||
|
||||
@Test
|
||||
func mcpToolAccessPoliciesControlExposureAndApproval() throws {
|
||||
var server = MCPServer(name: "Gateway")
|
||||
server.tools = [
|
||||
MCPTool(id: "disabled", accessPolicy: .disabled),
|
||||
MCPTool(id: "always-ask", accessPolicy: .alwaysAsk),
|
||||
MCPTool(id: "follow-permissions", accessPolicy: .followPermissions),
|
||||
]
|
||||
|
||||
let descriptors = AgentMCPTools.make(servers: [(server: server, bearerToken: nil)])
|
||||
.map(\.descriptor)
|
||||
#expect(descriptors.count == 2)
|
||||
#expect(!descriptors.contains { $0.name == "disabled" })
|
||||
|
||||
let alwaysAsk = try #require(descriptors.first { $0.name == "always-ask" })
|
||||
#expect(alwaysAsk.safety == .requiresAuthorization)
|
||||
#expect(alwaysAsk.alwaysAskUser)
|
||||
#expect(alwaysAsk.concurrency == .sequential)
|
||||
|
||||
let followPermissions = try #require(
|
||||
descriptors.first { $0.name == "follow-permissions" }
|
||||
)
|
||||
#expect(followPermissions.safety == .requiresAuthorization)
|
||||
#expect(!followPermissions.alwaysAskUser)
|
||||
#expect(followPermissions.concurrency == .parallel)
|
||||
}
|
||||
|
||||
@Test
|
||||
func secretHandlesAreSingleUseAndBindingScoped() async throws {
|
||||
let broker = SecretBroker()
|
||||
|
||||
@@ -83,4 +83,28 @@ struct SettingsNavigationTests {
|
||||
#expect(!store.lockMCPNamespace(for: duplicate.id))
|
||||
#expect(store.mcpServer(duplicate.id)?.namespaceID == "")
|
||||
}
|
||||
|
||||
@Test
|
||||
func mcpToolAccessPolicyIsAtomicAndSurvivesRediscovery() throws {
|
||||
var existingTool = MCPTool(id: "search")
|
||||
existingTool.accessPolicy = .disabled
|
||||
var server = MCPServer(name: "Gateway")
|
||||
server.tools = [existingTool]
|
||||
let store = AIConfigurationModel(mcpServers: [server], persistsSecrets: false)
|
||||
|
||||
store.setTools(
|
||||
[MCPTool(id: "search", title: "Updated"), MCPTool(id: "new-tool")],
|
||||
version: "2025-06-18", for: server.id
|
||||
)
|
||||
|
||||
let rediscovered = try #require(store.mcpServer(server.id))
|
||||
#expect(rediscovered.tools.first { $0.id == "search" }?.accessPolicy == .disabled)
|
||||
#expect(rediscovered.tools.first { $0.id == "new-tool" }?.accessPolicy == .alwaysAsk)
|
||||
|
||||
store.setToolAccessPolicy(.followPermissions, tool: "search", on: server.id)
|
||||
#expect(
|
||||
store.mcpServer(server.id)?.tools.first { $0.id == "search" }?.accessPolicy
|
||||
== .followPermissions
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user