mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-15 10:14:51 +00:00
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
This commit is contained in:
1 parent
2979f37050
commit
4583e5dd55
3 files changed
+166
-6
No files matched your search
@@ -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()
|
||||
|
||||
@@ -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/<team_id>/)
|
||||
// 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"
|
||||
|
||||
@@ -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).
|
||||
|
||||
Reference in new issue
Block a user