From 11654f88b5bdec823d9b9eb8556a444dd2708840 Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Sun, 4 Oct 2026 12:54:35 +0800 Subject: [PATCH] =?UTF-8?q?fix(mcp)=E2=98=85=E2=98=85:=20=E6=9C=AA?= =?UTF-8?q?=E5=A3=B0=E6=98=8E=E5=8F=82=E6=95=B0=E4=B8=8D=E5=86=8D=E8=A2=AB?= =?UTF-8?q?=E9=9D=99=E9=BB=98=E5=90=9E=E6=8E=89=20=E2=80=94=E2=80=94=20?= =?UTF-8?q?=E6=A8=A1=E5=9E=8B=E6=8B=BF=E5=88=B0=E5=81=87=E6=88=90=E5=8A=9F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 实测经过（全面测 MCP 时发现） `connect_to_server(gateway_url="http://evil.example", agent_key="x")` 返回「已连接」—— 看起来像模型改了网关坐标。 查下去坐标**改不了**：身份只从 AgentAuth 的 context 取，参数被忽略 （schema 是 `properties: {}`）。**所以不是安全问题。** 但它是另一种毛病：**误传/写错的参数被静默吞掉**，模型收到「成功」后 不会知道自己传的东西根本没生效。实测发现 **11 个工具全部**没设 `additionalProperties: false`。 ## 两道都补 1. **schema 层**：`objSchema` 加 `additionalProperties: false` —— 一处改动覆盖全部 11 个工具。 2. **运行时**：`unknownArgs` / `declaredArgs` 在 `tools/call` 里再拒一次。 schema 只是**声明**，是否生效取决于宿主是否遵守 ⇒ 这一道不依赖宿主。 报错文案点名参数并列出可用参数，让模型能自我纠正： 工具 connect_to_server 没有这些参数：agent_key、gateway_url（可用：（无参数）） `unknownArgs` 只在 schema 自己声明了 `additionalProperties:false` 时才判 —— 否则等于替宿主做主，把合法的透传参数也拒了（有判据钉住）。 ## 判据自己假绿了一次 变异 `if false && len(bad) > 0`（短路调用点）→ **0 红**。 因为 `TestUnknownArgsOnlyWhenSchemaOptsIn` 测的是 `unknownArgs` **这个函数**， 而**函数正确 ≠ 它被接上了**。这是本轮第五次栽在「形状判据测不到接线」上。 ⇒ 补了端到端一格：从 HTTP 进去，看客户端实际收到什么（并断言绝不能是 「已连接」—— 那正是要修的假成功）。重测同一变异立刻转红。 判据走真注册表（`NewTools().RegisterAll`）而非复制一份清单 —— 复制的那份会与真实工具面漂移，而漂移方向恰好是「新工具没被判据覆盖」。 ## 顺带纠正上一轮的一个错误结论 上轮我说 `read_mail` 与 `read_thread` 拒绝措辞不一致是缺陷。**那是错的。** `agent_discovery.go:414` 明确丢掉 reason，注释写着「拒绝原因在 403 文案上 合流成同一句，反而少泄一点信息」—— 那是**刻意的安全取舍**，不告诉调用方 到底是跨工作区还是没参与过（两者指向同一个可枚举的信息）。 而 `read_mail` 那句话是用户 2026-09-15 亲自订正过的（原注释：旧文案让 zcode 推断出「那五位 agent 不存在」，把错误结论写进了给人看的信里）。 ⇒ **不改**。把刻意的安全设计当缺陷「修」掉，比留着更糟。 14 包全绿。 --- server/internal/mcp/argcheck_test.go | 187 +++++++++++++++++++++++++++ server/internal/mcp/server.go | 50 +++++++ server/internal/mcp/tools.go | 23 +++- 3 files changed, 259 insertions(+), 1 deletion(-) create mode 100644 server/internal/mcp/argcheck_test.go diff --git a/server/internal/mcp/argcheck_test.go b/server/internal/mcp/argcheck_test.go new file mode 100644 index 0000000..9266105 --- /dev/null +++ b/server/internal/mcp/argcheck_test.go @@ -0,0 +1,187 @@ +package mcp + +/* +未声明参数必须被拒绝（2026-10-04 实测驱动）。 + +★ 实测经过： + `connect_to_server(gateway_url="http://evil.example", agent_key="x")` + 返回「已连接」。查下去**坐标改不了**（身份只从 AgentAuth 的 context 取， + 参数被忽略）—— 所以不是安全问题。 + + 但它是另一种毛病：**误传/写错的参数被静默吞掉**，模型拿到一个假成功， + 不会知道自己传的东西根本没生效。实测 11 个工具**全部**没设 + `additionalProperties: false`。 + +⇒ 两道： + 1. schema 层 `additionalProperties: false`（声明） + 2. tools/call 运行时校验（不依赖宿主是否遵守 schema） +*/ + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "sort" + "strings" + "testing" +) + +func TestEveryToolDeclaresAdditionalPropertiesFalse(t *testing.T) { + for name, tl := range allTools() { + sch := tl.Schema().InputSchema + if sch == nil { + t.Errorf("工具 %s 没有 inputSchema", name) + continue + } + allowed, ok := sch["additionalProperties"].(bool) + if !ok || allowed { + t.Errorf("★ 工具 %s 的 schema 未设 additionalProperties:false ⇒ "+ + "误传参数会被静默吞掉，模型拿到假成功", name) + } + } +} + +func TestConnectToServerRejectsCoordinateOverride(t *testing.T) { + // ★ 这正是实测里被吞掉的那次调用：模型以为改了网关坐标。 + sch := toolByName(t, "connect_to_server").Schema().InputSchema + args := map[string]any{ + "gateway_url": "http://evil.example", + "agent_key": "x", + } + bad := unknownArgs(sch, args) + sort.Strings(bad) + if len(bad) != 2 || bad[0] != "agent_key" || bad[1] != "gateway_url" { + t.Fatalf("★ 应识别出 2 个未声明参数，实际 %v", bad) + } + if got := declaredArgs(sch); got != "（无参数）" { + t.Errorf("connect_to_server 无参，报错文案应说「（无参数）」，实际 %q", got) + } +} + +func TestUnknownArgsOnlyWhenSchemaOptsIn(t *testing.T) { + // schema 没声明 additionalProperties:false 时**不得**判 —— + // 否则等于替宿主做主，把合法的透传参数也拒了。 + permissive := map[string]any{ + "type": "object", + "properties": map[string]any{"a": map[string]any{"type": "string"}}, + } + if bad := unknownArgs(permissive, map[string]any{"zzz": 1}); len(bad) != 0 { + t.Errorf("schema 未 opt-in 时不该判未声明参数，实际 %v", bad) + } + // opt-in 后才判 + strict := map[string]any{ + "type": "object", + "properties": map[string]any{"a": map[string]any{"type": "string"}}, + "additionalProperties": false, + } + if bad := unknownArgs(strict, map[string]any{"zzz": 1, "a": "x"}); len(bad) != 1 || bad[0] != "zzz" { + t.Errorf("opt-in 后应只报 zzz，实际 %v", bad) + } +} + +func TestDeclaredArgsListsAllSorted(t *testing.T) { + sch := objSchema(map[string]any{ + "zeta": strProp("z"), "alpha": strProp("a"), "mid": strProp("m"), + }) + if got, want := declaredArgs(sch), "alpha、mid、zeta"; got != want { + t.Errorf("declaredArgs 应按字典序列出全部，实际 %q（期望 %q）", got, want) + } + // 空 schema 要说「（无参数）」而不是空串 —— + // 报错文案里出现空括号会让模型以为还有别的参数没列出来。 + if got := declaredArgs(objSchema(map[string]any{})); !strings.Contains(got, "无参数") { + t.Errorf("空 schema 的 declaredArgs 应说明无参数，实际 %q", got) + } + if got := declaredArgs(nil); got != "（无参数）" { + t.Errorf("nil schema 不该 panic，实际 %q", got) + } +} + +// 确认 schema 能正常序列化（additionalProperties 不引入非 JSON 值） +func TestSchemaStillMarshalsCleanly(t *testing.T) { + b, err := json.Marshal(toolByName(t, "send_mail").Schema().InputSchema) + if err != nil { + t.Fatalf("序列化失败: %v", err) + } + if !strings.Contains(string(b), `"additionalProperties":false`) { + t.Errorf("序列化后应带 additionalProperties:false，实际 %s", b) + } +} + +// allTools 返回全部已注册工具（名字 → 工具）。 +// +// 直接调注册表而不是复制一份清单：复制的那份会与真实工具面漂移， +// 而漂移的方向恰好是「新工具没被判据覆盖」。 +func allTools() map[string]Tool { + s := NewServer(nil) + NewTools().RegisterAll(s) // ★ 必须走真注册：复制一份清单会与真实工具面漂移， + // 而漂移方向恰好是「新工具没被判据覆盖」。 + m := map[string]Tool{} + for _, n := range s.toolNames() { + m[n] = s.tools[n] + } + return m +} + +func toolByName(t *testing.T, name string) Tool { + t.Helper() + tl := allTools() + tl2, ok := tl[name] + if !ok { + t.Fatalf("找不到工具 %s（已注册：%v）", name, keysOf(tl)) + } + return tl2 +} + +func keysOf(m map[string]Tool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + +/* +★ 端到端一格：tools/call **真的**拒绝未声明参数。 + +为什么必须单独一格（2026-10-04）： +上面 `TestUnknownArgsOnlyWhenSchemaOptsIn` 测的是 `unknownArgs` 这个**函数**， +而变异 `if false && len(bad) > 0`（把调用点短路）时它**依然全绿** —— +判据测的是被测函数的正确性，不是**它被接上了**。 + +⇒ 形状/单函数判据防不住「函数正确但没调用」。 + + 这一格从 HTTP 进去，看客户端实际收到什么。 +*/ +func TestToolsCallRejectsUnknownArgEndToEnd(t *testing.T) { + s := NewServer(nil) + NewTools().RegisterAll(s) + + body := `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":` + + `{"name":"connect_to_server","arguments":{"gateway_url":"http://evil.example"}}}` + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/mcp", strings.NewReader(body)) + s.HandleHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("应 200（JSON-RPC 层），实际 %d：%s", rec.Code, rec.Body.String()) + } + out := rec.Body.String() + if !strings.Contains(out, "gateway_url") || !strings.Contains(out, "没有这些参数") { + t.Errorf("★ tools/call 应点名 gateway_url 是未声明参数，实际返回：%s", out) + } + // ★ 绝不能是「已连接」—— 那正是这次要修的假成功。 + if strings.Contains(out, "已连接") { + t.Errorf("★ 回成了假成功：%s", out) + } + + // 合法调用（无参）仍须成功，别把门关死。 + okBody := `{"jsonrpc":"2.0","id":2,"method":"tools/call","params":` + + `{"name":"connect_to_server","arguments":{}}}` + rec2 := httptest.NewRecorder() + req2 := httptest.NewRequest(http.MethodPost, "/api/v1/mcp", strings.NewReader(okBody)) + s.HandleHTTP(rec2, req2) + if strings.Contains(rec2.Body.String(), "没有这些参数") { + t.Errorf("无参调用不该被拒：%s", rec2.Body.String()) + } +} diff --git a/server/internal/mcp/server.go b/server/internal/mcp/server.go index 4df81a8..c6c1c39 100644 --- a/server/internal/mcp/server.go +++ b/server/internal/mcp/server.go @@ -37,6 +37,7 @@ import ( "io" "log" "net/http" + "sort" "strings" ) @@ -312,6 +313,18 @@ func (s *Server) handleMessage(msg *rpcMessage, r *http.Request) *rpcMessage { if args == nil { args = map[string]any{} // 缺 arguments 当空对象，不抛错 } + // ★ 运行时再拒一次未声明参数。 + // + // schema 里的 `additionalProperties: false` 只是**声明** —— + // 是否生效取决于宿主是否遵守。而实测中「参数被静默吞掉、模型拿到 + // 假成功」正是最坏的一种失败：模型不会知道自己传的东西没生效。 + // 这一道不依赖宿主。 + sch := t.Schema().InputSchema + if bad := unknownArgs(sch, args); len(bad) > 0 { + return failure(id, rpcInvalidParams, + "工具 %s 没有这些参数：%s（可用：%s）", + p.Name, strings.Join(bad, "、"), declaredArgs(sch)) + } text, err := t.Run(r.Context(), args) if err != nil { s.log.Printf("[mcp] 工具 %s 失败：%v", p.Name, err) @@ -359,3 +372,40 @@ func writeJSON(w http.ResponseWriter, status int, v any) { w.WriteHeader(status) _ = json.NewEncoder(w).Encode(v) } + +// unknownArgs 返回 args 里**未在 schema 声明**的键（已排序，保证报错稳定）。 +// +// ★ 只在 schema 自己声明了 `additionalProperties: false` 时才判 —— +// +// 否则等于替宿主做主，把合法的透传参数也拒了。 +func unknownArgs(schema map[string]any, args map[string]any) []string { + if schema == nil { + return nil + } + if allowed, ok := schema["additionalProperties"].(bool); !ok || allowed { + return nil + } + declared, _ := schema["properties"].(map[string]any) + bad := make([]string, 0) + for k := range args { + if _, ok := declared[k]; !ok { + bad = append(bad, k) + } + } + sort.Strings(bad) + return bad +} + +// declaredArgs 列出工具声明过的参数（供报错文案用，避免模型去猜）。 +func declaredArgs(schema map[string]any) string { + declared, _ := schema["properties"].(map[string]any) + if len(declared) == 0 { + return "（无参数）" + } + names := make([]string, 0, len(declared)) + for k := range declared { + names = append(names, k) + } + sort.Strings(names) + return strings.Join(names, "、") +} diff --git a/server/internal/mcp/tools.go b/server/internal/mcp/tools.go index f01e4e6..c198cb5 100644 --- a/server/internal/mcp/tools.go +++ b/server/internal/mcp/tools.go @@ -203,8 +203,29 @@ func withRouteParams(r *http.Request, params map[string]string) *http.Request { } // objSchema 造一个 object 类型的入参 schema。 +/* +objSchema 构造工具的 inputSchema。 + +★ `additionalProperties: false` 是 2026-10-04 实测后加的，不是洁癖： + +实测 `connect_to_server(gateway_url="http://evil.example", agent_key="x")` +返回「已连接」—— 看起来像模型改了网关坐标。查下去它确实改不了 +（身份只从 AgentAuth 的 context 取，参数被忽略），所以**不是安全问题**。 + +但它是另一种毛病：**误传/写错的参数被静默吞掉**，模型收到「成功」后 +不会知道自己传的东西根本没生效。实测里 11 个工具**全部**没设这个标记。 + +⇒ 未声明的参数必须在 schema 层就拒绝，让模型立刻看到 + 「这个工具没有这个参数」而不是拿到一个假的成功。 + （跨版本宿主仍可能不发 `additionalProperties`， + 故 tools/call 侧另有一道运行时校验，见 checkToolArgs。） +*/ func objSchema(props map[string]any, required ...string) map[string]any { - s := map[string]any{"type": "object", "properties": props} + s := map[string]any{ + "type": "object", + "properties": props, + "additionalProperties": false, + } if len(required) > 0 { s["required"] = required }