From d3bc73ba29103a18c3504e0918373edf913518b6 Mon Sep 17 00:00:00 2001 From: Yoilun Date: Mon, 25 May 2026 21:42:02 +0800 Subject: [PATCH] fix: bind writeback operations to agents directory --- go.mod | 2 +- internal/agents/writeback.go | 319 ++++++++++++++++++------------ internal/agents/writeback_test.go | 40 ++++ progress.md | 9 + task_plan.md | 1 + 5 files changed, 248 insertions(+), 123 deletions(-) diff --git a/go.mod b/go.mod index fe07535..39c7b7c 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/ncruces/go-strftime v0.1.9 // indirect github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0 // indirect - golang.org/x/sys v0.28.0 // indirect + golang.org/x/sys v0.28.0 modernc.org/libc v1.61.13 // indirect modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.8.2 // indirect diff --git a/internal/agents/writeback.go b/internal/agents/writeback.go index 1718ba3..15a0db2 100644 --- a/internal/agents/writeback.go +++ b/internal/agents/writeback.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "errors" "fmt" + "io" "os" "path/filepath" "regexp" @@ -15,6 +16,7 @@ import ( "time" "codex-agent-manager/internal/codexhome" + "golang.org/x/sys/unix" ) var ErrWriteConflict = errors.New("目标文件已在校验后发生变化") @@ -23,22 +25,29 @@ var safeAgentID = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_-]*$`) var writebackMu sync.Mutex var writebackTestHookBeforeBackup func() +var writebackTestHookAfterVerifyBeforeBackup func() var writebackTestHookAfterBackup func() type fileIdentity struct { - dev uint64 - ino uint64 - mode os.FileMode + dev uint64 + ino uint64 } type writeTarget struct { path string + base string content []byte mode os.FileMode agentsIdentity fileIdentity targetIdentity fileIdentity } +type agentsDirHandle struct { + fd int + path string + identity fileIdentity +} + func (s Store) ValidateDraft(id string, content string) (DraftValidation, error) { target, err := s.readWriteTarget(id) if err != nil { @@ -83,10 +92,11 @@ func (s Store) WriteDraft(id string, content string, expectedHash string) (Write return WriteResult{}, ErrWriteConflict } - target, err := s.readWriteTarget(id) + target, dir, err := s.openWriteTarget(id) if err != nil { return WriteResult{}, err } + defer dir.close() if hashBytes(target.content) != expectedHash { return WriteResult{}, ErrWriteConflict } @@ -94,18 +104,24 @@ func (s Store) WriteDraft(id string, content string, expectedHash string) (Write if writebackTestHookBeforeBackup != nil { writebackTestHookBeforeBackup() } - if _, err := s.verifyWriteTarget(id, target, expectedHash); err != nil { + if _, err := s.verifyWriteTarget(dir, id, target, expectedHash); err != nil { return WriteResult{}, err } - backupPath, err := s.createBackup(target.path, target.content, target.mode) + if writebackTestHookAfterVerifyBeforeBackup != nil { + writebackTestHookAfterVerifyBeforeBackup() + } + if _, err := s.verifyWriteTarget(dir, id, target, expectedHash); err != nil { + return WriteResult{}, err + } + backupPath, err := s.createBackup(dir, target) if err != nil { return WriteResult{}, err } if writebackTestHookAfterBackup != nil { writebackTestHookAfterBackup() } - if err := atomicWrite(target, []byte(content), func() error { - _, err := s.verifyWriteTarget(id, target, expectedHash) + if err := atomicWrite(dir, target, []byte(content), func() error { + _, err := s.verifyWriteTarget(dir, id, target, expectedHash) return err }); err != nil { return WriteResult{}, err @@ -120,56 +136,136 @@ func (s Store) WriteDraft(id string, content string, expectedHash string) (Write } func (s Store) readWriteTarget(id string) (writeTarget, error) { - if !safeAgentID.MatchString(id) { - return writeTarget{}, codexhome.ErrForbiddenPath - } - agentsPath := filepath.Join(s.CodexHome, "agents") - agentsInfo, err := os.Lstat(agentsPath) - if err != nil { - return writeTarget{}, err - } else if agentsInfo.Mode()&os.ModeSymlink != 0 || !agentsInfo.IsDir() { - return writeTarget{}, codexhome.ErrForbiddenPath - } - agentsIdentity, err := identityOf(agentsInfo) + target, dir, err := s.openWriteTarget(id) if err != nil { return writeTarget{}, err } - - fileName := id + ".toml" - targetPath, err := codexhome.ResolveAgentTOML(s.CodexHome, fileName) - if err != nil { - return writeTarget{}, err - } - targetInfo, err := os.Lstat(targetPath) - if err != nil { - return writeTarget{}, err - } - if targetInfo.Mode()&os.ModeSymlink != 0 || !targetInfo.Mode().IsRegular() { - return writeTarget{}, codexhome.ErrForbiddenPath - } - targetIdentity, err := identityOf(targetInfo) - if err != nil { - return writeTarget{}, err - } - data, err := os.ReadFile(targetPath) - if err != nil { - return writeTarget{}, err - } - target := writeTarget{ - path: targetPath, - content: data, - mode: targetInfo.Mode().Perm(), - agentsIdentity: agentsIdentity, - targetIdentity: targetIdentity, - } - if _, err := s.verifyWriteTarget(id, target, hashBytes(data)); err != nil { - return writeTarget{}, err - } + defer dir.close() return target, nil } -func (s Store) verifyWriteTarget(id string, expected writeTarget, expectedHash string) (writeTarget, error) { - current, err := s.readWriteTargetUnchecked(id) +func (s Store) openWriteTarget(id string) (writeTarget, agentsDirHandle, error) { + if !safeAgentID.MatchString(id) { + return writeTarget{}, agentsDirHandle{}, codexhome.ErrForbiddenPath + } + dir, err := openAgentsDir(s.CodexHome) + if err != nil { + return writeTarget{}, agentsDirHandle{}, err + } + target, err := s.readTargetFromDir(dir, id) + if err != nil { + dir.close() + return writeTarget{}, agentsDirHandle{}, err + } + if err := ensureAgentsPathStillMatches(dir); err != nil { + dir.close() + return writeTarget{}, agentsDirHandle{}, err + } + return target, dir, nil +} + +func openAgentsDir(home string) (agentsDirHandle, error) { + path := filepath.Join(home, "agents") + fd, err := unix.Open(path, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if err != nil { + if errors.Is(err, unix.ELOOP) || errors.Is(err, unix.ENOTDIR) { + return agentsDirHandle{}, codexhome.ErrForbiddenPath + } + return agentsDirHandle{}, err + } + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + _ = unix.Close(fd) + return agentsDirHandle{}, err + } + dir := agentsDirHandle{fd: fd, path: path, identity: identityOfUnix(stat)} + if err := ensureAgentsPathStillMatches(dir); err != nil { + _ = unix.Close(fd) + return agentsDirHandle{}, err + } + return dir, nil +} + +func (d agentsDirHandle) close() { + if d.fd >= 0 { + _ = unix.Close(d.fd) + } +} + +func ensureAgentsPathStillMatches(dir agentsDirHandle) error { + info, err := os.Lstat(dir.path) + if err != nil { + if os.IsNotExist(err) { + return ErrWriteConflict + } + return err + } + if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return ErrWriteConflict + } + identity, err := identityOf(info) + if err != nil { + return err + } + if identity != dir.identity { + return ErrWriteConflict + } + return nil +} + +func (s Store) readTargetFromDir(dir agentsDirHandle, id string) (writeTarget, error) { + if !safeAgentID.MatchString(id) { + return writeTarget{}, codexhome.ErrForbiddenPath + } + base := id + ".toml" + if _, err := codexhome.ResolveAgentTOML(s.CodexHome, base); err != nil { + return writeTarget{}, err + } + fd, err := unix.Openat(dir.fd, base, unix.O_RDONLY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if err != nil { + if errors.Is(err, unix.ELOOP) { + return writeTarget{}, codexhome.ErrForbiddenPath + } + return writeTarget{}, err + } + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + _ = unix.Close(fd) + return writeTarget{}, err + } + if stat.Mode&unix.S_IFMT != unix.S_IFREG { + _ = unix.Close(fd) + return writeTarget{}, codexhome.ErrForbiddenPath + } + data, err := readAllFromFD(fd, base) + if err != nil { + return writeTarget{}, err + } + return writeTarget{ + path: filepath.Join(dir.path, base), + base: base, + content: data, + mode: os.FileMode(stat.Mode & 0o777), + agentsIdentity: dir.identity, + targetIdentity: identityOfUnix(stat), + }, nil +} + +func readAllFromFD(fd int, name string) ([]byte, error) { + file := os.NewFile(uintptr(fd), name) + if file == nil { + _ = unix.Close(fd) + return nil, errors.New("无法打开目标文件") + } + defer file.Close() + return io.ReadAll(file) +} + +func (s Store) verifyWriteTarget(dir agentsDirHandle, id string, expected writeTarget, expectedHash string) (writeTarget, error) { + if err := ensureAgentsPathStillMatches(dir); err != nil { + return writeTarget{}, err + } + current, err := s.readTargetFromDir(dir, id) if err != nil { if os.IsNotExist(err) { return writeTarget{}, ErrWriteConflict @@ -177,6 +273,7 @@ func (s Store) verifyWriteTarget(id string, expected writeTarget, expectedHash s return writeTarget{}, err } if current.path != expected.path || + current.base != expected.base || current.agentsIdentity != expected.agentsIdentity || current.targetIdentity != expected.targetIdentity { return writeTarget{}, ErrWriteConflict @@ -187,101 +284,79 @@ func (s Store) verifyWriteTarget(id string, expected writeTarget, expectedHash s return current, nil } -func (s Store) readWriteTargetUnchecked(id string) (writeTarget, error) { - if !safeAgentID.MatchString(id) { - return writeTarget{}, codexhome.ErrForbiddenPath - } - agentsPath := filepath.Join(s.CodexHome, "agents") - agentsInfo, err := os.Lstat(agentsPath) - if err != nil { - return writeTarget{}, err - } - if agentsInfo.Mode()&os.ModeSymlink != 0 || !agentsInfo.IsDir() { - return writeTarget{}, codexhome.ErrForbiddenPath - } - agentsIdentity, err := identityOf(agentsInfo) - if err != nil { - return writeTarget{}, err - } - targetPath, err := codexhome.ResolveAgentTOML(s.CodexHome, id+".toml") - if err != nil { - return writeTarget{}, err - } - targetInfo, err := os.Lstat(targetPath) - if err != nil { - return writeTarget{}, err - } - if targetInfo.Mode()&os.ModeSymlink != 0 || !targetInfo.Mode().IsRegular() { - return writeTarget{}, codexhome.ErrForbiddenPath - } - targetIdentity, err := identityOf(targetInfo) - if err != nil { - return writeTarget{}, err - } - data, err := os.ReadFile(targetPath) - if err != nil { - return writeTarget{}, err - } - return writeTarget{ - path: targetPath, - content: data, - mode: targetInfo.Mode().Perm(), - agentsIdentity: agentsIdentity, - targetIdentity: targetIdentity, - }, nil -} - func identityOf(info os.FileInfo) (fileIdentity, error) { - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { + switch stat := info.Sys().(type) { + case *unix.Stat_t: + return identityOfUnix(*stat), nil + case *syscall.Stat_t: + return fileIdentity{ + dev: uint64(stat.Dev), + ino: uint64(stat.Ino), + }, nil + default: return fileIdentity{}, errors.New("无法确认文件身份") } - return fileIdentity{ - dev: uint64(stat.Dev), - ino: uint64(stat.Ino), - mode: info.Mode(), - }, nil } -func (s Store) createBackup(targetPath string, content []byte, mode os.FileMode) (string, error) { - backupPath := fmt.Sprintf("%s.bak-%s", targetPath, time.Now().UTC().Format("20060102T150405.000000000Z")) - file, err := os.OpenFile(backupPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) +func identityOfUnix(stat unix.Stat_t) fileIdentity { + return fileIdentity{ + dev: uint64(stat.Dev), + ino: uint64(stat.Ino), + } +} + +func (s Store) createBackup(dir agentsDirHandle, target writeTarget) (string, error) { + if err := ensureAgentsPathStillMatches(dir); err != nil { + return "", err + } + backupName := fmt.Sprintf("%s.bak-%s", target.base, time.Now().UTC().Format("20060102T150405.000000000Z")) + fd, err := unix.Openat(dir.fd, backupName, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_CLOEXEC|unix.O_NOFOLLOW, uint32(target.mode)) if err != nil { return "", err } - if _, err := file.Write(content); err != nil { + + file := os.NewFile(uintptr(fd), backupName) + if file == nil { + _ = unix.Close(fd) + _ = unix.Unlinkat(dir.fd, backupName, 0) + return "", errors.New("无法创建备份") + } + if _, err := file.Write(target.content); err != nil { _ = file.Close() - _ = os.Remove(backupPath) + _ = unix.Unlinkat(dir.fd, backupName, 0) return "", err } if err := file.Close(); err != nil { - _ = os.Remove(backupPath) + _ = unix.Unlinkat(dir.fd, backupName, 0) return "", err } - return backupPath, nil + return filepath.Join(dir.path, backupName), nil } -func atomicWrite(target writeTarget, content []byte, beforeRename func() error) error { - dir := filepath.Dir(target.path) - base := filepath.Base(target.path) - tmp, err := os.CreateTemp(dir, "."+base+".tmp-*") +func atomicWrite(dir agentsDirHandle, target writeTarget, content []byte, beforeRename func() error) error { + tmpName := fmt.Sprintf(".%s.tmp-%d-%d", target.base, os.Getpid(), time.Now().UnixNano()) + fd, err := unix.Openat(dir.fd, tmpName, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_CLOEXEC|unix.O_NOFOLLOW, uint32(target.mode)) if err != nil { return err } - tmpPath := tmp.Name() defer func() { - _ = os.Remove(tmpPath) + _ = unix.Unlinkat(dir.fd, tmpName, 0) }() - if err := tmp.Chmod(target.mode); err != nil { - _ = tmp.Close() + if err := unix.Fchmod(fd, uint32(target.mode)); err != nil { + _ = unix.Close(fd) return err } - if _, err := tmp.Write(content); err != nil { - _ = tmp.Close() + file := os.NewFile(uintptr(fd), tmpName) + if file == nil { + _ = unix.Close(fd) + return errors.New("无法创建临时文件") + } + if _, err := file.Write(content); err != nil { + _ = file.Close() return err } - if err := tmp.Close(); err != nil { + if err := file.Close(); err != nil { return err } if beforeRename != nil { @@ -289,7 +364,7 @@ func atomicWrite(target writeTarget, content []byte, beforeRename func() error) return err } } - return os.Rename(tmpPath, target.path) + return unix.Renameat(dir.fd, tmpName, dir.fd, target.base) } func hashBytes(data []byte) string { diff --git a/internal/agents/writeback_test.go b/internal/agents/writeback_test.go index 41dcc89..560306b 100644 --- a/internal/agents/writeback_test.go +++ b/internal/agents/writeback_test.go @@ -115,6 +115,46 @@ func TestWriteRejectsAgentsDirectoryReplacementBeforeBackup(t *testing.T) { assertFileContent(t, filepath.Join(externalDir, "backend.toml"), `name = "外部"`+"\n") } +func TestWriteBindsBackupToAgentsDirectoryAfterFinalVerify(t *testing.T) { + store, target := writebackFixture(t, `name = "旧名称"`+"\n") + validation, err := store.ValidateDraft("backend", `name = "新名称"`+"\n") + if err != nil { + t.Fatal(err) + } + root := filepath.Dir(filepath.Dir(target)) + agentsDir := filepath.Join(root, "agents") + realAgentsDir := filepath.Join(root, "agents-real") + externalDir := filepath.Join(root, "external") + if err := os.MkdirAll(externalDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(externalDir, "backend.toml"), []byte(`name = "外部"`+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + writebackTestHookAfterVerifyBeforeBackup = func() { + if err := os.Rename(agentsDir, realAgentsDir); err != nil { + t.Fatal(err) + } + if err := os.Symlink(externalDir, agentsDir); err != nil { + t.Fatal(err) + } + } + defer func() { writebackTestHookAfterVerifyBeforeBackup = nil }() + + _, err = store.WriteDraft("backend", `name = "新名称"`+"\n", validation.CurrentHash) + if !errors.Is(err, ErrWriteConflict) && !errors.Is(err, codexhome.ErrForbiddenPath) { + t.Fatalf("expected post-verify directory replacement to be rejected, got %v", err) + } + assertFileContent(t, filepath.Join(realAgentsDir, "backend.toml"), `name = "旧名称"`+"\n") + assertFileContent(t, filepath.Join(externalDir, "backend.toml"), `name = "外部"`+"\n") + if backups, err := filepath.Glob(filepath.Join(externalDir, "*.bak-*")); err != nil { + t.Fatal(err) + } else if len(backups) != 0 { + t.Fatalf("backup was created through replaced symlink directory: %#v", backups) + } +} + func TestWriteRejectsTargetChangeAfterBackup(t *testing.T) { store, target := writebackFixture(t, `name = "旧名称"`+"\n") validation, err := store.ValidateDraft("backend", `name = "新名称"`+"\n") diff --git a/progress.md b/progress.md index 05a269d..3a0ca97 100644 --- a/progress.md +++ b/progress.md @@ -30,6 +30,7 @@ | 2026-05-25 | 6 | coding agent | TDD 实现智能体草稿校验、diff、hash 冲突检测、备份和原子写回 | 完成；待最终全量验证 | | 2026-05-25 | 6 | spec review | 规格审查未通过：TOML 字符串解析错误泄漏英文 `invalid syntax` | coding agent 按 blocking 范围修复 | | 2026-05-25 | 6 | security review | 安全审查未通过：写回存在 TOCTOU、备份后 CAS 缺失、POST body 无限制、错误响应泄漏路径/英文 | coding agent 按 blocking 范围修复 | +| 2026-05-25 | 6 | security review | 安全复审未通过：复核后到 createBackup/rename 前仍可能重新解析被替换的 `agents` 路径 | coding agent 按 blocking 范围修复 | ## Test Results @@ -169,6 +170,13 @@ | 2026-05-25 | `cd web && pnpm test` | PASS | Phase 6 安全修复后前端单测验证通过；共 13 个单测 | | 2026-05-25 | `cd web && pnpm build` | PASS | Phase 6 安全修复后前端生产构建通过 | | 2026-05-25 | `git diff --check` | PASS | Phase 6 安全修复 whitespace 检查通过 | +| 2026-05-25 | `go test ./internal/agents ./internal/server` | FAIL | TDD 红灯：新增复核后备份前 hook 后缺少 `writebackTestHookAfterVerifyBeforeBackup`，暴露未覆盖窗口 | +| 2026-05-25 | `go test ./internal/agents ./internal/server` | PASS | Phase 6 dirfd 绑定写回目标包测试通过 | +| 2026-05-25 | `go test ./internal/agents ./internal/server` | PASS | Phase 6 dirfd 绑定修复后指定后端目标包验证通过 | +| 2026-05-25 | `go test ./...` | PASS | Phase 6 dirfd 绑定修复后全量 Go 验证通过 | +| 2026-05-25 | `cd web && pnpm test` | PASS | Phase 6 dirfd 绑定修复后前端单测验证通过；共 13 个单测 | +| 2026-05-25 | `cd web && pnpm build` | PASS | Phase 6 dirfd 绑定修复后前端生产构建通过 | +| 2026-05-25 | `git diff --check` | PASS | Phase 6 dirfd 绑定修复 whitespace 检查通过 | ## Bug Loop @@ -200,3 +208,4 @@ | 6 | TOML 未闭合字符串错误会把 `strconv.Unquote` 的英文 `invalid syntax` 返回给 UI/API | 在 parser 层将字符串字段语法错误包装为中文并带行号；List/Validate/Write 增加中文错误断言 | `go test ./internal/agents ./internal/server` PASS | | 6 | 写回备份/rename 前路径身份可能变化，且备份后并发修改可能被覆盖 | 写回加进程内临界区，记录 agents 目录和目标文件 inode identity；备份前和 rename 前复核 identity 与 expectedHash | `go test ./internal/agents ./internal/server` PASS | | 6 | validate/write POST 可接收超大 body、trailing JSON，且错误响应透传路径和英文系统错误 | validate/write 使用 1MiB `MaxBytesReader`、拒绝 trailing JSON，并将错误映射为安全中文响应 | `go test ./internal/agents ./internal/server` PASS | +| 6 | 复核后到备份/rename 前仍有父目录路径替换窗口 | 使用 `Openat`/`Renameat` 将目标读取、备份、临时文件和 rename 绑定到已打开的 `agents` 目录 fd，并继续复核目录路径身份和目标 hash | `go test ./internal/agents ./internal/server` PASS | diff --git a/task_plan.md b/task_plan.md index a69923f..86c4547 100644 --- a/task_plan.md +++ b/task_plan.md @@ -41,3 +41,4 @@ | 2026-05-25 | 5 | 质量复审发现未知 workflow phase status 会导致真实阶段被过滤消失 | TDD 补 `in_progress` phase 测试后只过滤表头/伪行，未知状态显示“未知” | 待复审 | | 2026-05-25 | 6 | 规格审查发现 malformed TOML 会通过 `strconv.Unquote` 泄漏英文 `invalid syntax` | TDD 补 List/Validate/Write 中文错误断言后包装字符串解析错误 | 待最终验证 | | 2026-05-25 | 6 | 安全审查发现写回 TOCTOU、备份后 CAS 缺失、POST body 无限制、错误响应泄漏路径/英文 | TDD 补目录替换、备份后修改、请求体限制和错误脱敏测试后加身份复核/CAS/MaxBytesReader/中文错误映射 | 待最终验证 | +| 2026-05-25 | 6 | 安全复审发现复核后到 createBackup/rename 前仍通过路径重新解析父目录 | TDD 补复核后替换 `agents` 为 symlink 测试后，将备份、临时文件和 rename 绑定到已打开的 agents dirfd | 待最终验证 |