fix(permission): Agent 不能把审批指派给与任务无关的人
# 漏洞
`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 自动项全绿）
This commit is contained in:
@ -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 != "" {
|
||||
|
||||
176
server/internal/handler/permission_request_test.go
Normal file
176
server/internal/handler/permission_request_test.go
Normal file
@ -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)
|
||||
}
|
||||
}
|
||||
@ -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
|
||||
}
|
||||
|
||||
106
server/internal/repo/threadhuman_test.go
Normal file
106
server/internal/repo/threadhuman_test.go
Normal file
@ -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），不能因为查不到就往放行方向倒")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user