From 4583e5dd553ff71a964ef265621abc326c66e8ce Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 23 Apr 2026 19:16:02 +0700 Subject: [PATCH] feat(tools): add team-root cross-chat read access with separate write-allowed paths - Add WithToolTeamRoot context key to allow agents in same team to read peer-chat files - Split allowedWithTeamWorkspace into read (includes team root) and write (excludes it) - Write operations now use allowedWriteWithTeamWorkspace to prevent cross-chat mutations - Add comprehensive tests for team root merging, deduping, and cross-chat access control - Refactor path merging into buildAllowedPrefixes helper to reduce duplication --- internal/tools/boundary_test.go | 105 ++++++++++++++++++++++++++++++++ internal/tools/context_keys.go | 21 +++++++ internal/tools/filesystem.go | 46 ++++++++++++-- 3 files changed, 166 insertions(+), 6 deletions(-) diff --git a/internal/tools/boundary_test.go b/internal/tools/boundary_test.go index 53cb815f..fb1a7345 100644 --- a/internal/tools/boundary_test.go +++ b/internal/tools/boundary_test.go @@ -510,6 +510,111 @@ func TestAllowedWithTeamWorkspace_TenantPathsOnly(t *testing.T) { } } +func TestAllowedWithTeamWorkspace_TeamRootMerged(t *testing.T) { + // Team root should be appended after team workspace so leader/member agents + // can read peer-scoped files in the same team without enabling shared mode. + ctx := context.Background() + base := []string{"/global/skills"} + teamWs := "/data/teams/abc/chatA" + teamRoot := "/data/teams/abc" + + ctx = WithToolTeamWorkspace(ctx, teamWs) + ctx = WithToolTeamRoot(ctx, teamRoot) + + result := allowedWithTeamWorkspace(ctx, base) + + expected := []string{"/global/skills", teamWs, teamRoot} + if len(result) != len(expected) { + t.Fatalf("expected %d paths, got %d: %v", len(expected), len(result), result) + } + for i, exp := range expected { + if result[i] != exp { + t.Errorf("path[%d]: expected %q, got %q", i, exp, result[i]) + } + } +} + +func TestAllowedWriteWithTeamWorkspace_ExcludesTeamRoot(t *testing.T) { + // Write variant must NOT include team root — cross-chat writes are blocked + // even when reads across the same team are allowed. Shared-mode parity: + // when teamWs == teamRoot (shared workspace), writing to teamWs is still + // permitted because teamWs is the leaf scope. + ctx := context.Background() + base := []string{"/global/skills"} + teamWs := "/data/teams/abc/chatA" + teamRoot := "/data/teams/abc" + + ctx = WithToolTeamWorkspace(ctx, teamWs) + ctx = WithToolTeamRoot(ctx, teamRoot) + + writeAllowed := allowedWriteWithTeamWorkspace(ctx, base) + readAllowed := allowedWithTeamWorkspace(ctx, base) + + // Read allowed should include team root. + if len(readAllowed) != 3 { + t.Fatalf("read: expected 3 prefixes, got %d: %v", len(readAllowed), readAllowed) + } + // Write allowed must NOT include team root. + if len(writeAllowed) != 2 { + t.Fatalf("write: expected 2 prefixes (base + teamWs), got %d: %v", len(writeAllowed), writeAllowed) + } + for _, p := range writeAllowed { + if p == teamRoot { + t.Errorf("write allowed must not include team root %q", teamRoot) + } + } +} + +func TestAllowedWithTeamWorkspace_TeamRootDeduped(t *testing.T) { + // When team root == team workspace (shared-workspace mode), avoid duplicate entry. + ctx := context.Background() + base := []string{"/global/skills"} + same := "/data/teams/abc" + + ctx = WithToolTeamWorkspace(ctx, same) + ctx = WithToolTeamRoot(ctx, same) + + result := allowedWithTeamWorkspace(ctx, base) + + if len(result) != 2 { + t.Fatalf("expected 2 paths (base + one team path), got %d: %v", len(result), result) + } + if result[0] != "/global/skills" || result[1] != same { + t.Errorf("unexpected result: %v", result) + } +} + +func TestResolvePathWithAllowed_TeamRootCrossChatAccess(t *testing.T) { + // Reproduces trace 019db4df-c2e2: leader agent in chat scope "chatA" tries to + // read a file generated by a teammate under chat scope "chatB" within the + // same team. Team root as allowed prefix must permit this cross-chat read. + teamRoot := t.TempDir() + chatA := filepath.Join(teamRoot, "chatA") + chatB := filepath.Join(teamRoot, "chatB", "generated") + if err := os.MkdirAll(chatA, 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(chatB, 0755); err != nil { + t.Fatal(err) + } + peerFile := filepath.Join(chatB, "v3-02-prompt-mode.png") + if err := os.WriteFile(peerFile, []byte("img"), 0644); err != nil { + t.Fatal(err) + } + + // Without team root: leader scoped to chatA cannot reach chatB. + _, err := resolvePathWithAllowed(peerFile, chatA, true, nil) + if err == nil { + t.Fatal("expected error without team root in allowed prefixes, got nil") + } + + // With team root as allowed prefix: access granted. + _, err = resolvePathWithAllowed(peerFile, chatA, true, []string{teamRoot}) + if err != nil { + t.Fatalf("expected success with team root in allowed prefixes, got: %v", err) + } +} + func TestAllowedWithTeamWorkspace_EmptyContext(t *testing.T) { // Test with no tenant paths or team workspace in context ctx := context.Background() diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index be554e6f..842178b6 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -401,6 +401,27 @@ func ToolTeamWorkspaceFromCtx(ctx context.Context) string { return "" } +// --- Team root (team-wide shared root, above UserChatLayer) --- + +const ctxTeamRoot toolContextKey = "tool_team_root" + +// WithToolTeamRoot stores the team-wide root directory (e.g. /app/workspace/teams//) +// without the UserChatLayer suffix. Any agent belonging to the team (leader or member) sees +// this path as an allowed prefix so file tools can read across chat/user scopes within the +// same team. Per-chat write isolation is preserved by still resolving writes against the +// agent's own workspace first; this key only widens the allowed-prefix set for path checks. +func WithToolTeamRoot(ctx context.Context, dir string) context.Context { + return context.WithValue(ctx, ctxTeamRoot, dir) +} + +// ToolTeamRootFromCtx returns the team-wide root directory, or empty if not set. +func ToolTeamRootFromCtx(ctx context.Context) string { + if v, _ := ctx.Value(ctxTeamRoot).(string); v != "" { + return v + } + return "" +} + // --- Team task ID propagation (delegation origin → workspace tools) --- const ctxTeamTaskID toolContextKey = "tool_team_task_id" diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index 2a081b63..ea7b0435 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -304,22 +304,53 @@ func (t *ReadFileTool) paginateOutput(content string, args map[string]any) *Resu return SilentResult(output) } -// allowedWithTeamWorkspace returns the allowed prefixes with team workspace and -// tenant-specific paths appended if present in context. Thread-safe: creates a -// new slice per request. Merge order: base (global) → tenant paths → team workspace. +// allowedWithTeamWorkspace returns the READ-allowed prefixes with team workspace, +// team root, and tenant-specific paths appended if present in context. +// Thread-safe: creates a new slice per request. +// Merge order: base (global) → tenant paths → team workspace (leaf scope) → team root. +// Team root is the team-wide directory without UserChatLayer suffix; it lets +// any agent in the team read files generated by peers under different chat scopes. +// +// Use this variant for read operations (read_file, read_image, list_files, send_file). +// For write operations, use allowedWriteWithTeamWorkspace instead — team root is +// intentionally excluded from write to prevent cross-chat write leakage. func allowedWithTeamWorkspace(ctx context.Context, base []string) []string { + return buildAllowedPrefixes(ctx, base, true) +} + +// allowedWriteWithTeamWorkspace returns the WRITE-allowed prefixes. Same as the +// read variant but WITHOUT team root — writes must stay within the agent's leaf +// scope (team workspace = own chat dir for isolated mode, team root itself for +// shared mode). This prevents an agent in chat A from writing into chat B's +// workspace through a cross-chat absolute path. +// +// Use this variant for write/mutation operations (write_file, edit, shell). +func allowedWriteWithTeamWorkspace(ctx context.Context, base []string) []string { + return buildAllowedPrefixes(ctx, base, false) +} + +// buildAllowedPrefixes merges base + tenant paths + team workspace, optionally +// including team root. Extracted to share the slice-building logic between read +// and write variants without duplication. +func buildAllowedPrefixes(ctx context.Context, base []string, includeTeamRoot bool) []string { tenantPaths := TenantAllowedPathsFromCtx(ctx) teamWs := ToolTeamWorkspaceFromCtx(ctx) + var teamRoot string + if includeTeamRoot { + teamRoot = ToolTeamRootFromCtx(ctx) + } - if len(tenantPaths) == 0 && teamWs == "" { + if len(tenantPaths) == 0 && teamWs == "" && teamRoot == "" { return base } - // Pre-allocate capacity for all sources capacity := len(base) + len(tenantPaths) if teamWs != "" { capacity++ } + if teamRoot != "" && teamRoot != teamWs { + capacity++ + } out := make([]string, 0, capacity) out = append(out, base...) @@ -327,6 +358,9 @@ func allowedWithTeamWorkspace(ctx context.Context, base []string) []string { if teamWs != "" { out = append(out, teamWs) } + if teamRoot != "" && teamRoot != teamWs { + out = append(out, teamRoot) + } return out } @@ -482,7 +516,7 @@ func resolvePath(path, workspace string, restrict bool) (string, error) { // Validate canonical path stays within canonical workspace. if !isPathInside(real, wsReal) { slog.Warn("security.path_escape", "path", path, "resolved", real, "workspace", wsReal) - return "", fmt.Errorf("access denied: path outside workspace — if this file was discovered via vault_search, use vault_read(doc_id) instead") + return "", fmt.Errorf("access denied: path outside workspace") } // Reject paths with mutable symlink components (TOCTOU symlink rebind risk).