From 425e11c4442ec1377a95add038c311e98d10e7a5 Mon Sep 17 00:00:00 2001 From: Yoilun Date: Mon, 25 May 2026 17:58:22 +0800 Subject: [PATCH] fix: validate agent toml boundaries --- internal/agents/store.go | 34 +++++++++++++++ internal/agents/store_test.go | 79 +++++++++++++++++++++++++++++++++++ progress.md | 10 +++++ task_plan.md | 1 + 4 files changed, 124 insertions(+) diff --git a/internal/agents/store.go b/internal/agents/store.go index 7dcc966..79e330e 100644 --- a/internal/agents/store.go +++ b/internal/agents/store.go @@ -61,6 +61,15 @@ func (s Store) readOne(fileName string) AgentDefinition { def.ParseError = err.Error() return def } + if info, err := os.Lstat(safePath); err != nil { + def.ParseStatus = "invalid" + def.ParseError = err.Error() + return def + } else if info.Mode()&os.ModeSymlink != 0 { + def.ParseStatus = "invalid" + def.ParseError = codexhome.ErrForbiddenPath.Error() + return def + } info, statErr := os.Stat(safePath) if statErr == nil { def.ModifiedAt = info.ModTime() @@ -109,6 +118,12 @@ func parseSimpleTOML(input string) (map[string]string, error) { if key == "" { return values, fmt.Errorf("第 %d 行缺少字段名", lineNumber) } + if !isValidBareKey(key) { + return values, fmt.Errorf("第 %d 行包含无效字段名", lineNumber) + } + if _, exists := values[key]; exists { + return values, fmt.Errorf("第 %d 行重复字段名 %q", lineNumber, key) + } value, err := parseTOMLString(raw, scanner) if err != nil { @@ -122,6 +137,25 @@ func parseSimpleTOML(input string) (map[string]string, error) { return values, nil } +func isValidBareKey(key string) bool { + for _, char := range key { + if char >= 'a' && char <= 'z' { + continue + } + if char >= 'A' && char <= 'Z' { + continue + } + if char >= '0' && char <= '9' { + continue + } + if char == '_' || char == '-' { + continue + } + return false + } + return true +} + func parseTOMLString(raw string, scanner *bufio.Scanner) (string, error) { if strings.HasPrefix(raw, `"""`) { block := strings.TrimPrefix(raw, `"""`) diff --git a/internal/agents/store_test.go b/internal/agents/store_test.go index dfc19a0..ae3a101 100644 --- a/internal/agents/store_test.go +++ b/internal/agents/store_test.go @@ -72,6 +72,54 @@ func TestListAgentsReportsParseError(t *testing.T) { } } +func TestListAgentsReportsDuplicateKeyAsParseError(t *testing.T) { + root := t.TempDir() + agentsDir := filepath.Join(root, "agents") + if err := os.MkdirAll(agentsDir, 0o755); err != nil { + t.Fatal(err) + } + content := "name = \"a\"\nname = \"b\"\n" + if err := os.WriteFile(filepath.Join(agentsDir, "duplicate.toml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + store := Store{CodexHome: root} + got, err := store.List() + if err != nil { + t.Fatalf("List should return parse status, not fatal error: %v", err) + } + if len(got) != 1 { + t.Fatalf("agent count = %d, want 1", len(got)) + } + if got[0].ParseStatus != "invalid" || got[0].ParseError == "" { + t.Fatalf("expected duplicate key to be invalid, got %#v", got[0]) + } +} + +func TestListAgentsReportsInvalidKeyAsParseError(t *testing.T) { + root := t.TempDir() + agentsDir := filepath.Join(root, "agents") + if err := os.MkdirAll(agentsDir, 0o755); err != nil { + t.Fatal(err) + } + content := "bad key = \"value\"\n" + if err := os.WriteFile(filepath.Join(agentsDir, "bad-key.toml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + store := Store{CodexHome: root} + got, err := store.List() + if err != nil { + t.Fatalf("List should return parse status, not fatal error: %v", err) + } + if len(got) != 1 { + t.Fatalf("agent count = %d, want 1", len(got)) + } + if got[0].ParseStatus != "invalid" || got[0].ParseError == "" { + t.Fatalf("expected invalid key to be invalid, got %#v", got[0]) + } +} + func TestListAgentsRejectsSensitiveSymlinkTargets(t *testing.T) { root := t.TempDir() agentsDir := filepath.Join(root, "agents") @@ -100,3 +148,34 @@ func TestListAgentsRejectsSensitiveSymlinkTargets(t *testing.T) { t.Fatalf("sensitive file content leaked: %#v", got[0]) } } + +func TestListAgentsRejectsSymlinkToNonAgentToml(t *testing.T) { + root := t.TempDir() + agentsDir := filepath.Join(root, "agents") + if err := os.MkdirAll(agentsDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "config.toml"), []byte(`name = "project-secret"`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../config.toml", filepath.Join(agentsDir, "leak.toml")); err != nil { + t.Fatal(err) + } + + store := Store{CodexHome: root} + got, err := store.List() + if err != nil { + t.Fatalf("List should report unsafe files per item, not fatal error: %v", err) + } + if len(got) != 1 { + t.Fatalf("agent count = %d, want 1", len(got)) + } + if got[0].ParseStatus != "invalid" { + t.Fatalf("expected non-agent symlink target to be invalid, got %#v", got[0]) + } + if strings.Contains(got[0].Name, "project-secret") || + strings.Contains(got[0].Description, "project-secret") || + strings.Contains(got[0].ParseError, "project-secret") { + t.Fatalf("non-agent TOML content leaked: %#v", got[0]) + } +} diff --git a/progress.md b/progress.md index 86c0f67..dbe58d6 100644 --- a/progress.md +++ b/progress.md @@ -11,6 +11,8 @@ | 2026-05-25 | 1 | review loop | 规格复审发现 `ResolveAgentTOML` 可经 `agents/demo.toml -> ../auth.json` symlink 绕过 forbidden 检查 | 已按 TDD 修复，并通过最终门禁 | | 2026-05-25 | planning | main agent | 修正 task_plan.md 阶段命名，与实施计划 Task 2-7 对齐 | 下一阶段明确为 Agent TOML 只读读取 | | 2026-05-25 | 2 | coding agent | TDD 实现 Agent TOML 只读读取和 `/api/agents` | 完成；提交 `feat: read codex agent definitions` | +| 2026-05-25 | 2 | spec review | 规格审查未通过：重复键 TOML 可 valid；agent symlink 可读取 root `config.toml` | coding agent 按 blocking 范围修复 | +| 2026-05-25 | 2 | coding agent | TDD 修复 agent TOML parser 和 symlink 边界 | 完成；提交 `fix: validate agent toml boundaries` | ## Test Results @@ -44,6 +46,12 @@ | 2026-05-25 | `go test ./...` | PASS | Required verification | | 2026-05-25 | `git diff --check` | PASS | Required verification | | 2026-05-25 | `git status --short` | PASS | Required verification；Phase 2 文件待提交 | +| 2026-05-25 | `go test ./internal/agents` | FAIL | TDD 红灯：duplicate key、invalid key、`agents/leak.toml -> ../config.toml` 均被错误报告为 valid/泄漏内容 | +| 2026-05-25 | `go test ./internal/agents` | PASS | duplicate key 和 invalid key 返回 invalid；agent TOML symlink 被拒绝且不读取非 agent TOML | +| 2026-05-25 | `go test ./internal/codexhome` | PASS | Required verification | +| 2026-05-25 | `go test ./...` | PASS | Required verification | +| 2026-05-25 | `git diff --check` | PASS | Required verification | +| 2026-05-25 | `git status --short` | PASS | Required verification；Phase 2 review fix 文件待提交 | ## Bug Loop @@ -54,3 +62,5 @@ | 1 | 缺少操作域 resolver，通用 `ResolveInside` 容易误用 | 新增 `ResolveAgentTOML`，只允许 `agents/` 直属 `.toml` 文件名 | `go test ./internal/codexhome` PASS | | 1 | `docs/project.md` 记录 `CODEX_HOME` 但默认配置未读取 | `DefaultConfig` 增加 `CODEX_HOME` 非空 override | `go test ./internal/app` PASS | | 1 | `ResolveAgentTOML` 可通过 `agents/*.toml` symlink 指向 root `auth.json` 绕过 forbidden 检查 | 在 symlink 解析后对 evaluated final target 再执行 forbidden 检查 | `go test ./internal/codexhome` PASS | +| 2 | Agent TOML parser 对重复键使用 map 覆盖，且未校验 bare key | 增加 duplicate key 和 invalid key 检测，遇到 malformed TOML 返回单条 invalid | `go test ./internal/agents` PASS | +| 2 | Agent symlink 只校验最终路径在 Codex home 内，可读取 root `config.toml` | 在 agent store 层拒绝 `.toml` symlink，避免读取非 agent TOML 内容 | `go test ./internal/agents` PASS | diff --git a/task_plan.md b/task_plan.md index f0cceb4..f607e83 100644 --- a/task_plan.md +++ b/task_plan.md @@ -32,3 +32,4 @@ | --- | --- | --- | --- | --- | | 2026-05-25 | 1 | 代码质量审查发现 symlink 边界绕过、敏感文件大小写匹配、缺少操作域 resolver、`CODEX_HOME` 未生效 | TDD 补充失败测试后修复 `codexhome` 和 `app` | 已通过最终验证 | | 2026-05-25 | 1 | 规格复审发现 `ResolveAgentTOML` 可通过 `agents/*.toml` symlink 指向 root `auth.json` 绕过 forbidden 检查 | TDD 补充 symlink-to-auth 测试后检查 resolved final target | 已通过最终验证 | +| 2026-05-25 | 2 | 规格审查发现重复键 TOML 被报告为 valid，且 agent symlink 可读取 Codex home 内非 agent TOML | TDD 补充 duplicate key、invalid key、symlink-to-config 测试后修复 parser 和 agent store 边界 | 已通过最终验证 |