diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index df46f1e4..601ae570 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -212,9 +212,11 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ctx = tools.WithToolTeamID(ctx, req.TeamID) // Team root for dispatched tasks: resolve the UserChatLayer-stripped root // so the dispatched agent can still read peer-scoped files in the same team. + // l.dataDir is already tenant-scoped (see resolver.go: config.TenantDataDir), + // so TenantLayer must NOT be reapplied here — doing so double-joins the + // tenant segment (tenants//tenants//teams/). if teamUUID, err := uuid.Parse(req.TeamID); err == nil && l.dataDir != "" { teamRoot := tools.ResolveWorkspace(l.dataDir, - tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)), tools.TeamLayer(teamUUID), ) ctx = tools.WithToolTeamRoot(ctx, teamRoot) @@ -244,9 +246,10 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup wsChat = req.UserID } shared := tools.IsSharedWorkspace(team.Settings) - // Resolve team workspace via layered pipeline: tenant → team → user/chat. + // Resolve team workspace via layered pipeline: team → user/chat. + // l.dataDir is already tenant-scoped (see resolver.go: config.TenantDataDir) — + // do NOT reapply TenantLayer here, it would double-join the tenant segment. wsDir := tools.ResolveWorkspace(l.dataDir, - tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)), tools.TeamLayer(team.ID), tools.UserChatLayer(wsChat, shared), ) @@ -264,7 +267,6 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup // the same team. Writes still default to wsDir above; team root only // widens the allowed-prefix set for path boundary checks. teamRoot := tools.ResolveWorkspace(l.dataDir, - tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)), tools.TeamLayer(team.ID), ) ctx = tools.WithToolTeamRoot(ctx, teamRoot) @@ -294,12 +296,14 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup // Filesystem path segment must use agent_key, not UUID — matches // the v2 path in loop_pipeline_callbacks.go and the session_key // anchor. See docs/agent-identity-conventions.md. - AgentID: l.id, - AgentType: l.agentType, - UserID: req.UserID, - ChatID: req.ChatID, - TenantID: store.TenantIDFromContext(ctx).String(), - TenantSlug: store.TenantSlugFromContext(ctx), + AgentID: l.id, + AgentType: l.agentType, + UserID: req.UserID, + ChatID: req.ChatID, + // TenantID/TenantSlug intentionally left empty: l.dataDir (BaseDir below) + // is already tenant-scoped (see resolver.go: config.TenantDataDir). Setting + // TenantID here would make resolveTeam/resolvePersonal reapply tenantPath() + // and double-join the tenant segment (tenants//tenants//...). PeerKind: req.PeerKind, TeamID: teamIDPtr, TeamConfig: teamWSConfig, diff --git a/internal/config/tenant_paths.go b/internal/config/tenant_paths.go index 58652f55..c2515fd9 100644 --- a/internal/config/tenant_paths.go +++ b/internal/config/tenant_paths.go @@ -11,45 +11,46 @@ import ( // Duplicated here to avoid an import cycle (store imports config). var masterTenantID = uuid.MustParse("0193a5b0-7000-7000-8000-000000000001") +// TenantScopedDir is the single canonical implementation for joining a base +// directory with a tenant-scoped "tenants/{slug-or-id}" subdirectory. +// It is the sole source of truth for this join: TenantDataDir and +// TenantWorkspace (uuid.UUID-typed call sites) delegate to it, and +// internal/workspace's team/personal workspace resolution (string-typed +// tenant identifiers, not always parseable as uuid.UUID) delegates to it too. +// +// Master tenant (tenantIDStr == "" or the master sentinel) returns base +// unchanged (backward compat). +// Empty tenantSlug (transient DB lookup failure) would otherwise resolve to +// the tenants/ parent dir, granting cross-tenant access — falls back to an +// ID-based path instead. +// Includes defense-in-depth path traversal protection against a malicious slug. +func TenantScopedDir(base, tenantIDStr, tenantSlug string) string { + if tenantIDStr == "" || tenantIDStr == masterTenantID.String() { + return base + } + if tenantSlug == "" { + return filepath.Join(base, "tenants", tenantIDStr) + } + result := filepath.Join(base, "tenants", tenantSlug) + tenantsBase := filepath.Join(base, "tenants") + string(filepath.Separator) + if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) { + return filepath.Join(base, "tenants", tenantIDStr) + } + return result +} + // TenantDataDir returns the data directory root for a tenant. // Master tenant returns dataDir unchanged (backward compat). // Other tenants return dataDir/tenants/{slug}/. func TenantDataDir(dataDir string, tenantID uuid.UUID, tenantSlug string) string { - if tenantID == masterTenantID { - return dataDir - } - // Empty slug (transient DB lookup failure) would resolve to the tenants/ parent dir, - // granting cross-tenant access. Fall back to UUID-based path instead. - if tenantSlug == "" { - return filepath.Join(dataDir, "tenants", tenantID.String()) - } - result := filepath.Join(dataDir, "tenants", tenantSlug) - // Defense-in-depth: prevent path traversal via malicious slug. - tenantsBase := filepath.Join(dataDir, "tenants") + string(filepath.Separator) - if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) { - return filepath.Join(dataDir, "tenants", tenantID.String()) - } - return result + return TenantScopedDir(dataDir, tenantID.String(), tenantSlug) } // TenantWorkspace returns the workspace root for a tenant. // Master tenant returns workspace unchanged (backward compat). // Other tenants return workspace/tenants/{slug}/. func TenantWorkspace(workspace string, tenantID uuid.UUID, tenantSlug string) string { - if tenantID == masterTenantID { - return workspace - } - // Empty slug (transient DB lookup failure) would resolve to the tenants/ parent dir, - // granting cross-tenant access. Fall back to UUID-based path instead. - if tenantSlug == "" { - return filepath.Join(workspace, "tenants", tenantID.String()) - } - result := filepath.Join(workspace, "tenants", tenantSlug) - tenantsBase := filepath.Join(workspace, "tenants") + string(filepath.Separator) - if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) { - return filepath.Join(workspace, "tenants", tenantID.String()) - } - return result + return TenantScopedDir(workspace, tenantID.String(), tenantSlug) } // TenantTeamDir returns the team workspace directory for a tenant. diff --git a/internal/config/tenant_paths_test.go b/internal/config/tenant_paths_test.go new file mode 100644 index 00000000..d8dbb224 --- /dev/null +++ b/internal/config/tenant_paths_test.go @@ -0,0 +1,78 @@ +package config + +import ( + "path/filepath" + "testing" + + "github.com/google/uuid" +) + +func TestTenantScopedDir_MasterTenant(t *testing.T) { + got := TenantScopedDir("/data", masterTenantID.String(), "master") + if got != "/data" { + t.Errorf("master tenant should be no-op, got %s", got) + } +} + +func TestTenantScopedDir_EmptyTenantID(t *testing.T) { + got := TenantScopedDir("/data", "", "acme") + if got != "/data" { + t.Errorf("empty tenantID should be no-op, got %s", got) + } +} + +func TestTenantScopedDir_UsesSlug(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + got := TenantScopedDir("/data", tid.String(), "acme") + want := filepath.Join("/data", "tenants", "acme") + if got != want { + t.Errorf("got %s, want %s", got, want) + } +} + +func TestTenantScopedDir_EmptySlugFallsBackToID(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + got := TenantScopedDir("/data", tid.String(), "") + want := filepath.Join("/data", "tenants", tid.String()) + if got != want { + t.Errorf("got %s, want %s", got, want) + } +} + +func TestTenantScopedDir_TraversalDefense(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + got := TenantScopedDir("/data", tid.String(), "../../etc") + want := filepath.Join("/data", "tenants", tid.String()) + if got != want { + t.Errorf("malicious slug should fall back to ID-based path, got %s, want %s", got, want) + } +} + +// TestTenantDataDir_TenantWorkspace_DelegateToCanonical pins that both +// exported wrappers produce identical output shapes via the single +// canonical TenantScopedDir implementation — no independent join logic. +func TestTenantDataDir_TenantWorkspace_DelegateToCanonical(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + + dataDir := TenantDataDir("/data", tid, "acme") + wantDataDir := filepath.Join("/data", "tenants", "acme") + if dataDir != wantDataDir { + t.Errorf("TenantDataDir = %s, want %s", dataDir, wantDataDir) + } + + workspace := TenantWorkspace("/ws", tid, "acme") + wantWorkspace := filepath.Join("/ws", "tenants", "acme") + if workspace != wantWorkspace { + t.Errorf("TenantWorkspace = %s, want %s", workspace, wantWorkspace) + } +} + +func TestTenantTeamDir(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + got := TenantTeamDir("/data", tid, "acme", teamID) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String()) + if got != want { + t.Errorf("got %s, want %s", got, want) + } +} diff --git a/internal/tools/workspace_resolver_test.go b/internal/tools/workspace_resolver_test.go index 134bda67..0782ef14 100644 --- a/internal/tools/workspace_resolver_test.go +++ b/internal/tools/workspace_resolver_test.go @@ -2,6 +2,7 @@ package tools import ( "path/filepath" + "strings" "testing" "github.com/google/uuid" @@ -177,6 +178,31 @@ func TestResolveWorkspace_SharedTrue(t *testing.T) { } } +// TestResolveWorkspace_TenantScopedBaseDir_NoDoubleJoin pins a regression: +// when the base dir passed in is ALREADY tenant-scoped (as internal/agent's +// Loop.dataDir is — see resolver.go's config.TenantDataDir call), applying +// TeamLayer alone must produce the tenant segment exactly once, matching the +// path the web UI file-upload handler writes to (internal/http/files.go via +// config.TenantWorkspace). Regression: an earlier version of internal/agent's +// loop_context.go re-applied TenantLayer on top of this already-scoped base, +// producing "tenants/acme/tenants/acme/teams/". +func TestResolveWorkspace_TenantScopedBaseDir_NoDoubleJoin(t *testing.T) { + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + alreadyTenantScopedBase := filepath.Join("/data", "tenants", "acme") + + got := ResolveWorkspace(alreadyTenantScopedBase, + TeamLayer(teamID), + ) + + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String()) + if got != want { + t.Errorf("want %s, got %s", want, got) + } + if n := strings.Count(got, "tenants"+string(filepath.Separator)+"acme"); n != 1 { + t.Errorf("expected tenant segment to appear exactly once, got %d occurrences in %q", n, got) + } +} + func TestSanitizePathSegment(t *testing.T) { tests := []struct { input string diff --git a/internal/workspace/resolver_impl.go b/internal/workspace/resolver_impl.go index 5bcd1d73..0592daeb 100644 --- a/internal/workspace/resolver_impl.go +++ b/internal/workspace/resolver_impl.go @@ -7,6 +7,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/nextlevelbuilder/goclaw/internal/config" ) // masterTenantID is the sentinel UUID for the master/default tenant. @@ -123,22 +125,13 @@ func (r *defaultResolver) resolvePersonal(p ResolveParams) *WorkspaceContext { // tenantPath returns tenant-scoped directory. // Master tenant returns base dir directly (backward compat with v2). -// Uses slug when available (matches config.TenantWorkspace), falls back to UUID. +// Uses slug when available (matches config.TenantWorkspace), falls back to +// tenantID. Sanitizes both identifiers (workspace tenant IDs/slugs are not +// always valid UUIDs, unlike config's uuid.UUID-typed call sites) before +// delegating the actual join+traversal-defense logic to the single canonical +// implementation, config.TenantScopedDir. func tenantPath(base, tenantID, tenantSlug string) string { - if tenantID == "" || tenantID == masterTenantID { - return base - } - segment := tenantSlug - if segment == "" { - segment = tenantID - } - result := filepath.Join(base, "tenants", sanitizeSegment(segment)) - // Path traversal defense: ensure result stays under tenants/ base - tenantsBase := filepath.Join(base, "tenants") + string(filepath.Separator) - if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) { - return filepath.Join(base, "tenants", sanitizeSegment(tenantID)) - } - return result + return config.TenantScopedDir(base, sanitizeSegment(tenantID), sanitizeSegment(tenantSlug)) } // userChatSegment returns the isolation segment: chatID for group, userID for direct. diff --git a/internal/workspace/resolver_impl_test.go b/internal/workspace/resolver_impl_test.go index 9f042f09..ca851bfd 100644 --- a/internal/workspace/resolver_impl_test.go +++ b/internal/workspace/resolver_impl_test.go @@ -122,6 +122,46 @@ func TestResolve_TeamShared(t *testing.T) { } } +// TestResolve_TeamShared_TenantScopedBaseDir_NoDoubleJoin pins a regression: +// callers whose BaseDir is already tenant-scoped (e.g. internal/agent's +// Loop.dataDir, built via config.TenantDataDir) must pass an empty +// TenantID/TenantSlug so resolveTeam's tenantPath() call is a no-op. An +// earlier bug passed both an already-scoped BaseDir AND a non-empty +// TenantID/TenantSlug into this same call, producing +// ".../tenants/acme/tenants/acme/teams/" for the agent-facing path, +// while the web UI upload handler (config.TenantWorkspace, single join) +// correctly produced ".../tenants/acme/teams/". +func TestResolve_TeamShared_TenantScopedBaseDir_NoDoubleJoin(t *testing.T) { + rawBase := t.TempDir() + alreadyTenantScopedBase := filepath.Join(rawBase, "tenants", "acme") + r := NewResolver() + teamID := "team-abc" + + wc, err := r.Resolve(context.Background(), ResolveParams{ + AgentID: "agent-1", + AgentType: "open", + UserID: "user-1", + ChatID: "chat-1", + TenantID: "", // intentionally empty: BaseDir is already tenant-scoped + TenantSlug: "", + PeerKind: "direct", + TeamID: &teamID, + TeamConfig: &TeamWorkspaceConfig{WorkspaceScope: "shared"}, + BaseDir: alreadyTenantScopedBase, + }) + if err != nil { + t.Fatal(err) + } + + want := filepath.Join(rawBase, "tenants", "acme", "teams", "team-abc") + if wc.ActivePath != want { + t.Errorf("ActivePath = %q, want %q", wc.ActivePath, want) + } + if n := strings.Count(wc.ActivePath, filepath.Join("tenants", "acme")); n != 1 { + t.Errorf("expected tenant segment to appear exactly once, got %d occurrences in %q", n, wc.ActivePath) + } +} + func TestResolve_TeamIsolated(t *testing.T) { base := t.TempDir() r := NewResolver()