From 4e32dd31457ecf8595a2ce2df91b884f62d3c8e1 Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Fri, 11 Sep 2026 23:22:53 +0800 Subject: [PATCH] =?UTF-8?q?fix(permission):=20Agent=20=E4=B8=8D=E8=83=BD?= =?UTF-8?q?=E6=8A=8A=E5=AE=A1=E6=89=B9=E6=8C=87=E6=B4=BE=E7=BB=99=E4=B8=8E?= =?UTF-8?q?=E4=BB=BB=E5=8A=A1=E6=97=A0=E5=85=B3=E7=9A=84=E4=BA=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 漏洞 `POST /permission/request` 的 `to` 字段由 Agent 自由填写，服务端只检查 「这个名字是不是一个合法的人类用户」： decider := req.To if isHuman, _ := repo.IsHumanUser(ctx, decider); !isHuman { …回落… } 于是任何 Agent 都能把「是否允许执行 bash」这类危险操作的审批丢给**任意一个 与这条任务无关的人**（例如管理员）。被点名的人看到一封没有上下文的待办， 只能凭猜点头或拒绝。 这跟同一份代码里的另一段注释直接冲突。那段在论证为什么不把权限转给管理员： 管理员对这条 Agent 链的上下文一无所知，既不知道这个 bash 命令在做什么， 也不知道拒绝后 Agent 该怎么绕过去。 这个理由同样适用于「Agent 自己点名一个无关的人」—— 而且更弱：至少管理员还能 查日志，一个随机被点名的用户连从哪查都不知道。两处都指向同一条规则： **权限应当追溯到最初分配任务的人**，也就是这条线索上的人。 这是静态审计发现的四项之一。当时三桥实测都不传 `to`，所以是潜在面而非活跃 漏洞 —— 但 `to` 是公开的 Agent API 字段，第三方插件照着文档填就会踩上。 # 修法 新增 `repo.IsHumanOnSessionThread(ctx, sessionID, name)`：人类身份 **且** （会话 owner 或在这条会话的某封邮件里出现过）。 两个来源缺一不可，各有实测场景： - **只要参与方**会漏掉「会话由 Agent 建立、owner 由平台指派」的会话 —— 那种 owner 可能一封邮件都没收发过，只看邮件会把合法 owner 判成外人， 于是每次审批都回落到线索上随便一个人类。 - **只要 owner** 会漏掉「人在别人的会话里被抄送进来说了话」这种正常协作。 采信与否的处置是**丢弃提示而不是报错**：`to` 只是一个偏好，丢弃后常规解析仍会 给出一个合法人类（owner 或线索上最近的人），实在没有就是既有的 409 —— 无论哪条 分支，都不会把审批送到错的人手上。硬失败则会让 Agent 一次乐观的提示断掉整个 任务，而它并没有做错什么。因为丢弃是静默的，所以**必须留下日志**： [permission] 忽略不属于本线索的决策人 "jianf"（会话 …, 由 pi 指定）—— 改走常规解析 # 测试 补了这条路径此前**完全缺失**的两层覆盖（审计发现：决策路径 RequestPermission/DecidePermission/ListPendingPermissions 都没有测试）： - `repo/threadhuman_test.go`：白名单的六种输入（线索上发信/收信的人类、没发过 邮件的 owner、线索外的存在用户、不存在的名字、空串、抄送方），每条都写清 为什么期望这个结果。 - `handler/permission_request_test.go`：**真实 HTTP 层**跑 `RequestPermission`， 断言响应里的 decider 与库里那封权限邮件的 to_name。repo 层 helper 正确但 handler 漏调一次，漏洞就会回来，所以必须有端到端这一层。含纯 Agent 链的 fail-closed 断言（不得退回管理员）。 # 验证 - `go test ./... -count=1` 全绿；`go vet` 干净；`gofmt` 差异行数与改动前完全 相同（8 行，既有的一处空行）—— 即本次改动零新增格式问题 - 真机（workspace 档会话，owner=gui-lab，线索参与者 gui-lab+pi）： - `to=jianf`（线索外人类）→ decider=gui-lab，库中 to_name=gui-lab， 日志有忽略记录 - `to=gui-lab`（线索内）→ 采纳 - `to=pi`（Agent）→ 忽略，回落 owner - 已部署（redeploy-gateway.sh 自动项全绿） --- server/internal/handler/permission.go | 21 ++- .../handler/permission_request_test.go | 176 ++++++++++++++++++ server/internal/repo/participants.go | 42 +++++ server/internal/repo/threadhuman_test.go | 106 +++++++++++ 4 files changed, 344 insertions(+), 1 deletion(-) create mode 100644 server/internal/handler/permission_request_test.go create mode 100644 server/internal/repo/threadhuman_test.go diff --git a/server/internal/handler/permission.go b/server/internal/handler/permission.go index eae5986..75b84cb 100644 --- a/server/internal/handler/permission.go +++ b/server/internal/handler/permission.go @@ -3,6 +3,7 @@ package handler import ( "errors" "fmt" + "log" "net/http" "strings" "time" @@ -166,7 +167,25 @@ func RequestPermission(w http.ResponseWriter, r *http.Request) { // 没有任何人类参与过。而且那段 409 自己的注释就在论证兜底是错的： // 「管理员对这条 Agent 链的上下文一无所知」。两条策略互相矛盾， // 先执行的那条把后写的那条变成了死代码。 - decider := req.To + decider := strings.TrimSpace(req.To) + + // 显式指定的决策人只在**确属这条线索**时才采信。 + // + // `to` 由 Agent 自由填写，原来只要它是合法的人类用户名就直接采用，于是 + // 一个 Agent 可以把「是否允许执行 bash」丢给任意一个与这条任务无关的人。 + // 这与上面 409 分支的理由直接冲突：既然「管理员对这条 Agent 链一无所知」 + // 是拒绝转给管理员的依据，那 Agent 自己点名一个无关的人同样不成立。 + // 权限应当追溯到最初分配任务的人，也就是这条线索上的人。 + // + // 这里**丢弃提示而不是报错**：它只是一个偏好，下面常规解析仍会给出一个 + // 合法人类（owner 或线索上最近的人），否则明确 409 —— 无论哪条，都不会 + // 把审批送到错的人手上。硬失败则会让 Agent 一次乐观的提示断掉整个任务， + // 而它并没有做错什么。丢掉是静默的，所以这里必须留下日志。 + if decider != "" && decider != "human" && !repo.IsHumanOnSessionThread(r.Context(), sessionID, decider) { + log.Printf("[permission] 忽略不属于本线索的决策人 %q（会话 %s，由 %s 指定）—— 改走常规解析", + decider, sessionID, agentName) + decider = "" + } if decider == "" || decider == "human" { owner, err := repo.SessionOwnerUsername(r.Context(), sessionID) if err == nil && owner != "" { diff --git a/server/internal/handler/permission_request_test.go b/server/internal/handler/permission_request_test.go new file mode 100644 index 0000000..3801d3f --- /dev/null +++ b/server/internal/handler/permission_request_test.go @@ -0,0 +1,176 @@ +package handler + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/agentmail/gateway/internal/db" + "github.com/agentmail/gateway/internal/middleware" + "github.com/agentmail/gateway/internal/repo" + "github.com/google/uuid" +) + +// RequestPermission 的决策人解析必须在 HTTP handler 层有覆盖。 +// +// 过去 `to` 是 Agent 可自由填写的用户名，只要对应人类账号存在就直接采用。 +// 这允许 Agent 把危险操作的审批丢给任意一个与任务无关的人；repo 层的参与方 +// helper 即使正确，handler 漏调一次仍会把漏洞带回来。 +func TestRequestPermissionRestrictsExplicitDeciderToThread(t *testing.T) { + setupPermissionHandlerDB(t) + ctx := context.Background() + + mustPermissionUser(t, "owner") + mustPermissionUser(t, "reviewer") + mustPermissionUser(t, "outsider") + if err := repo.CreateOrUpdateAgent(ctx, "pi", "secret", "test", nil); err != nil { + t.Fatalf("注册 pi: %v", err) + } + + ownerID := permissionUserID(t, "owner") + sid, err := repo.CreateSession(ctx, nil, "pi", "授权路由", "/tmp") + if err != nil { + t.Fatalf("建会话: %v", err) + } + if err := repo.SetSessionOwner(ctx, sid, ownerID); err != nil { + t.Fatalf("设置 owner: %v", err) + } + if _, err := repo.CreateMail(ctx, sid, nil, "reviewer", "", "pi", "/tmp", + "任务", "请执行", nil); err != nil { + t.Fatalf("建线索邮件: %v", err) + } + + t.Run("无关人类被忽略并回落 owner", func(t *testing.T) { + resp := requestPermission(t, sid, "outsider", "是否允许执行 bash？") + if resp.Code != http.StatusOK { + t.Fatalf("HTTP %d: %s", resp.Code, resp.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(resp.Body.Bytes(), &body); err != nil { + t.Fatalf("解析响应: %v", err) + } + if got := body["decider"]; got != "owner" { + t.Fatalf("无关人类不应收到审批，decider=%v，期望 owner", got) + } + assertPermissionRecipient(t, body["mail_id"].(string), "owner") + }) + + t.Run("线索上的人类可被显式指定", func(t *testing.T) { + resp := requestPermission(t, sid, "reviewer", "是否允许写文件？") + if resp.Code != http.StatusOK { + t.Fatalf("HTTP %d: %s", resp.Code, resp.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(resp.Body.Bytes(), &body); err != nil { + t.Fatalf("解析响应: %v", err) + } + if got := body["decider"]; got != "reviewer" { + t.Fatalf("线索上的人类应保持显式优先级，decider=%v", got) + } + assertPermissionRecipient(t, body["mail_id"].(string), "reviewer") + }) + + t.Run("线索上的 Agent 仍不能成为决策人", func(t *testing.T) { + resp := requestPermission(t, sid, "pi", "是否允许编辑？") + if resp.Code != http.StatusOK { + t.Fatalf("HTTP %d: %s", resp.Code, resp.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(resp.Body.Bytes(), &body); err != nil { + t.Fatalf("解析响应: %v", err) + } + if got := body["decider"]; got != "owner" { + t.Fatalf("Agent 不能在 WebUI 决策，应回落 owner，实际 %v", got) + } + }) +} + +func TestRequestPermissionRejectsAllAgentThread(t *testing.T) { + setupPermissionHandlerDB(t) + ctx := context.Background() + for _, name := range []string{"pi", "dsh"} { + if err := repo.CreateOrUpdateAgent(ctx, name, "secret", "test", nil); err != nil { + t.Fatalf("注册 %s: %v", name, err) + } + } + sid, err := repo.CreateSession(ctx, nil, "pi", "纯 Agent 链", "/tmp") + if err != nil { + t.Fatalf("建会话: %v", err) + } + if _, err := repo.CreateMail(ctx, sid, nil, "dsh", "", "pi", "/tmp", + "任务", "请执行", nil); err != nil { + t.Fatalf("建线索邮件: %v", err) + } + + resp := requestPermission(t, sid, "human", "是否允许执行 bash？") + if resp.Code != http.StatusConflict { + t.Fatalf("纯 Agent 链必须 fail closed，HTTP %d: %s", resp.Code, resp.Body.String()) + } + if bytes.Contains(resp.Body.Bytes(), []byte(`"decider":"admin"`)) { + t.Fatalf("不得退回无关管理员: %s", resp.Body.String()) + } +} + +func setupPermissionHandlerDB(t *testing.T) { + t.Helper() + db.Close() + path := filepath.Join(t.TempDir(), "permission-handler.db") + if err := db.Connect(context.Background(), "sqlite://"+path); err != nil { + t.Fatalf("连接测试库: %v", err) + } + if err := db.Migrate(context.Background()); err != nil { + t.Fatalf("迁移测试库: %v", err) + } + t.Cleanup(db.Close) +} + +func mustPermissionUser(t *testing.T, username string) { + t.Helper() + if _, err := db.DB.ExecContext(context.Background(), + `INSERT INTO users (user_id, username, display_name, password_hash, role) + VALUES ($1, $2, $3, 'x', 'user')`, uuid.New(), username, username); err != nil { + t.Fatalf("建用户 %s: %v", username, err) + } +} + +func permissionUserID(t *testing.T, username string) uuid.UUID { + t.Helper() + var id uuid.UUID + if err := db.DB.QueryRowContext(context.Background(), + `SELECT user_id FROM users WHERE username = $1`, username).Scan(&id); err != nil { + t.Fatalf("查用户 %s: %v", username, err) + } + return id +} + +func requestPermission(t *testing.T, sid uuid.UUID, to, question string) *httptest.ResponseRecorder { + t.Helper() + payload, _ := json.Marshal(map[string]any{ + "session_id": sid.String(), + "to": to, + "question": question, + "options": []string{"同意", "拒绝"}, + }) + req := httptest.NewRequest(http.MethodPost, "/api/v1/permission/request", bytes.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + ctx := context.WithValue(req.Context(), middleware.AgentNameKey, "pi") + resp := httptest.NewRecorder() + RequestPermission(resp, req.WithContext(ctx)) + return resp +} + +func assertPermissionRecipient(t *testing.T, mailID, want string) { + t.Helper() + var got string + if err := db.DB.QueryRowContext(context.Background(), + `SELECT to_name FROM mails WHERE mail_id = $1`, mailID).Scan(&got); err != nil { + t.Fatalf("查权限邮件 %s: %v", mailID, err) + } + if got != want { + t.Fatalf("权限邮件收件人=%q，期望 %q", got, want) + } +} diff --git a/server/internal/repo/participants.go b/server/internal/repo/participants.go index c065cd3..e9a7c99 100644 --- a/server/internal/repo/participants.go +++ b/server/internal/repo/participants.go @@ -133,3 +133,45 @@ func sortedKeys(m map[string]bool) []string { sort.Strings(out) return out } + +// IsHumanOnSessionThread 判断某个用户名是否确属这条会话的线索 —— +// 会话 owner，或在会话某封邮件里出现过（发件人 / 收件人 / 抄送）。 +// +// 用途：权限询问的决策人白名单。 +// +// # 为什么需要它 +// +// `/permission/request` 的 `to` 字段由 Agent 自由填写，原来只要它是合法的人类 +// 用户名就直接采用。于是一个 Agent 可以把「是否允许执行 bash」这类审批丢给任意 +// 一个与这条任务无关的人（例如管理员）。而上游刚用「管理员对这条 Agent 链的 +// 上下文一无所知 —— 既不知道这个 bash 命令在做什么，也不知道拒绝后 Agent 该 +// 怎么绕过去」的理由删掉了「退回第一个管理员」的兜底；同一个理由也适用于 +// 「Agent 自己点名一个无关的人」。 +// +// 语义上，权限应当追溯到**最初分配任务的人**，也就是这条线索上的人。 +// +// 注意 owner 与 participations 是两处来源、缺一不可：owner 可能从未在这条会话里 +// 收发过任何邮件（会话由 Agent 建立、owner 由平台指派），那种情况下只看邮件会 +// 把合法 owner 判成外人。 +func IsHumanOnSessionThread(ctx context.Context, sessionID uuid.UUID, name string) bool { + if name == "" { + return false + } + isHuman, err := IsHumanUser(ctx, name) + if err != nil || !isHuman { + return false + } + if owner, err := SessionOwnerUsername(ctx, sessionID); err == nil && owner == name { + return true + } + parts, err := SessionParticipants(ctx, sessionID) + if err != nil { + return false + } + for _, p := range parts { + if p.Name == name { + return true + } + } + return false +} diff --git a/server/internal/repo/threadhuman_test.go b/server/internal/repo/threadhuman_test.go new file mode 100644 index 0000000..aabb5fa --- /dev/null +++ b/server/internal/repo/threadhuman_test.go @@ -0,0 +1,106 @@ +package repo + +import ( + "context" + "testing" + + "github.com/agentmail/gateway/internal/db" + "github.com/agentmail/gateway/internal/models" + "github.com/google/uuid" +) + +// 权限询问的决策人白名单。 +// +// `/permission/request` 的 `to` 字段由 Agent 自由填写，原来只要它是合法的人类 +// 用户名就直接采用 —— 于是一个 Agent 可以把「是否允许执行 bash」丢给任意一个 +// 与这条任务无关的人。这与 handler 里那段 409 的理由直接冲突：既然「管理员对 +// 这条 Agent 链的上下文一无所知」是拒绝转给管理员的依据，那 Agent 自己点名一个 +// 无关的人同样不成立。权限应当追溯到最初分配任务的人，也就是这条线索上的人。 + +func mustHuman(t *testing.T, username string) uuid.UUID { + t.Helper() + id := uuid.New() + if _, err := db.DB.ExecContext(context.Background(), + `INSERT INTO users (user_id, username, display_name, password_hash, role) + VALUES ($1, $2, $3, 'x', 'user')`, + id, username, username); err != nil { + t.Fatalf("建用户 %s: %v", username, err) + } + return id +} + +func TestIsHumanOnSessionThread(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + + sid, _ := CreateSession(ctx, nil, "pi", "权限决策人", "/home/program/agentmail") + ownerID := mustHuman(t, "laoban") + mustHuman(t, "dispatcher") + mustHuman(t, "reviewer") + mustHuman(t, "passerby") // 存在，但与这条线索无关 + + // 会话里有人类 dispatcher / reviewer 往来；owner 由平台指派、从未收发过邮件。 + mustMail(t, sid, "dispatcher", "", "reviewer", "/home/program/agentmail", nil) + if err := SetSessionOwner(ctx, sid, ownerID); err != nil { + t.Fatalf("指派 owner: %v", err) + } + + cases := []struct { + name string + who string + want bool + why string + }{ + {"线索上发过信的人类", "dispatcher", true, + "发件人就是最初分配任务的人，权限第一该问他"}, + {"线索上收过信的人类", "reviewer", true, + "人类收件人也是参与者"}, + {"线索上的 Agent 不是决策人", "pi", false, + "出现在邮件里只是必要条件；只有人类用户才能在 WebUI 决策"}, + {"会话 owner（没发过邮件）", "laoban", true, + "owner 与参与方是两处来源，缺一不可：会话由 Agent 建立、owner 由平台指派时，只看邮件会把合法 owner 判成外人"}, + {"存在但与线索无关的人", "passerby", false, + "这就是要堵的那个洞：Agent 不能把审批丢给与任务无关的人"}, + {"根本不存在的人", "nobody", false, + "白名单从正面判定，不存在的名字自然不通过"}, + {"空串", "", false, + "空串的语义是「没指定」，走常规解析，不该被当成线索上的人"}, + } + + for _, c := range cases { + if got := IsHumanOnSessionThread(ctx, sid, c.who); got != c.want { + t.Errorf("%s：IsHumanOnSessionThread(%q) = %v，应为 %v —— %s", + c.name, c.who, got, c.want, c.why) + } + } +} + +// 抄送方也算线索上的人：一次抄送就把人带进了这条线索。 +func TestIsHumanOnSessionThreadCountsCC(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + + sid, _ := CreateSession(ctx, nil, "pi", "抄送也算", "/home/program/agentmail") + mustHuman(t, "dispatcher") + mustHuman(t, "observer") + mustMail(t, sid, "dispatcher", "", "pi", "/home/program/agentmail", nil) + + // 抄送方由第二封信带入 —— 一次抄送就把人带进了这条线索。 + cc := []models.Address{{Name: "observer", Path: "/home", Session: "new", Raw: "observer@/home.new"}} + if _, err := CreateMail(ctx, sid, nil, "dispatcher", "/home/program/agentmail", + "pi", "/home/program/agentmail", "带抄送", "正文", cc); err != nil { + t.Fatalf("建带抄送的邮件: %v", err) + } + + if !IsHumanOnSessionThread(ctx, sid, "observer") { + t.Error("人类抄送方应算作线索上的人：一次抄送就把他带进了这条线索") + } +} + +// 会话不存在时不得 panic，且必须返回 false（fail closed）。 +func TestIsHumanOnSessionThreadUnknownSession(t *testing.T) { + setupTestDB(t) + if IsHumanOnSessionThread(context.Background(), uuid.New(), "admin") { + t.Error("会话不存在时应返回 false（fail closed），不能因为查不到就往放行方向倒") + } +}