From 2c06cf5b6f8a09504874dd6a2d8bbefa2c3b5389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 22 Sep 2026 08:15:41 +0100 Subject: [PATCH] fix: keep file tree paths workspace-relative (#3855) Co-authored-by: Kandev Agent --- .../agent/handlers/workspace_file_handlers.go | 4 + .../handlers/workspace_file_handlers_test.go | 42 ++++ .../server/process/workspace_files.go | 4 + .../server/process/workspace_files_test.go | 40 ++++ .../internal/common/workspacepath/tree.go | 61 ++++++ .../common/workspacepath/tree_test.go | 35 ++++ .../chat/messages/tool-edit-message.test.tsx | 18 ++ .../task/chat/messages/tool-edit-message.tsx | 3 +- .../task/file-browser-apply-changes.test.ts | 54 ++++- .../web/components/task/file-browser-hooks.ts | 9 +- .../file-browser-restore-expanded.test.tsx | 16 ++ .../components/task/file-browser-restore.ts | 3 +- .../components/task/file-tree-reveal.test.ts | 20 ++ apps/web/components/task/file-tree-reveal.ts | 16 +- apps/web/e2e/helpers/session.ts | 18 ++ .../chat-read-absolute-workspace-path.spec.ts | 101 +++++++++ .../e2e/tests/task/mobile-file-viewer.spec.ts | 29 +-- apps/web/hooks/use-file-editors.test.tsx | 26 +++ apps/web/hooks/use-file-editors.ts | 10 +- apps/web/hooks/use-lsp-file-opener.ts | 6 +- apps/web/lib/workspace-file-path.test.ts | 90 ++++++++ apps/web/lib/workspace-file-path.ts | 73 +++++++ docs/plans/file-tree-path-scope/plan.md | 184 +++++++++++++++++ .../task-01-normalize-tool-file-paths.md | 145 +++++++++++++ .../task-02-enforce-tree-request-scope.md | 172 ++++++++++++++++ .../ui/requirements/file-tree-path-scope.md | 96 +++++++++ .../ui/system-design/file-tree-path-scope.md | 192 ++++++++++++++++++ 27 files changed, 1432 insertions(+), 35 deletions(-) create mode 100644 apps/backend/internal/common/workspacepath/tree.go create mode 100644 apps/backend/internal/common/workspacepath/tree_test.go create mode 100644 apps/web/e2e/tests/task/chat-read-absolute-workspace-path.spec.ts create mode 100644 apps/web/lib/workspace-file-path.test.ts create mode 100644 apps/web/lib/workspace-file-path.ts create mode 100644 docs/plans/file-tree-path-scope/plan.md create mode 100644 docs/plans/file-tree-path-scope/task-01-normalize-tool-file-paths.md create mode 100644 docs/plans/file-tree-path-scope/task-02-enforce-tree-request-scope.md create mode 100644 docs/specs/ui/requirements/file-tree-path-scope.md create mode 100644 docs/specs/ui/system-design/file-tree-path-scope.md diff --git a/apps/backend/internal/agent/handlers/workspace_file_handlers.go b/apps/backend/internal/agent/handlers/workspace_file_handlers.go index 3b616125b2..dfbaf6fa2d 100644 --- a/apps/backend/internal/agent/handlers/workspace_file_handlers.go +++ b/apps/backend/internal/agent/handlers/workspace_file_handlers.go @@ -10,6 +10,7 @@ import ( agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/workspacepath" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) @@ -62,6 +63,9 @@ func (h *WorkspaceFileHandlers) wsGetFileTree(ctx context.Context, msg *ws.Messa if req.SessionID == "" { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "session_id is required", nil) } + if err := workspacepath.ValidateTreePath(req.Path); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) + } // Get agent execution for this session execution, err := h.lifecycle.GetOrEnsureExecution(ctx, req.SessionID) if err != nil { diff --git a/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go b/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go index b066ea4a7e..dcda89a5bf 100644 --- a/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go +++ b/apps/backend/internal/agent/handlers/workspace_file_handlers_test.go @@ -19,6 +19,19 @@ import ( "go.uber.org/zap/zaptest/observer" ) +type countingWorkspaceExecutionLookup struct { + calls int +} + +func (l *countingWorkspaceExecutionLookup) GetExecutionBySessionID(string) (*lifecycle.AgentExecution, bool) { + return nil, false +} + +func (l *countingWorkspaceExecutionLookup) GetOrEnsureExecution(context.Context, string) (*lifecycle.AgentExecution, error) { + l.calls++ + return nil, context.Canceled +} + func TestWorkspaceFileHandlersValidateRequestsBeforeLookup(t *testing.T) { tests := []struct { name string @@ -50,6 +63,35 @@ func TestWorkspaceFileHandlersValidateRequestsBeforeLookup(t *testing.T) { } } +func TestWorkspaceFileHandlersRejectAbsoluteTreePathBeforeLookup(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + log, err := commonlogger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatal(err) + } + lookup := &countingWorkspaceExecutionLookup{} + handler := NewWorkspaceFileHandlers(lookup, log) + msg, err := ws.NewRequest("id", ws.ActionWorkspaceFileTreeGet, map[string]any{ + "session_id": "s", + "path": "/home/jcfs/project/public/assets", + }) + if err != nil { + t.Fatal(err) + } + + response, err := handler.wsGetFileTree(context.Background(), msg) + if err != nil { + t.Fatalf("wsGetFileTree returned error: %v", err) + } + assertWorkspaceContentSearchErrorCode(t, response, ws.ErrorCodeValidation) + if lookup.calls != 0 { + t.Fatalf("execution lookup calls = %d, want 0", lookup.calls) + } + if logs.FilterLevelExact(zap.ErrorLevel).Len() != 0 { + t.Fatalf("error logs = %v, want none", logs.FilterLevelExact(zap.ErrorLevel).All()) + } +} + func TestWorkspaceFileHandlersSuccessContracts(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index b5b8f1d77f..5514e562b1 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -22,6 +22,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types" "github.com/kandev/kandev/internal/common/readselector" "github.com/kandev/kandev/internal/common/subproc" + "github.com/kandev/kandev/internal/common/workspacepath" storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" "go.uber.org/zap" ) @@ -126,6 +127,9 @@ func (wt *WorkspaceTracker) getFileListClass(ctx context.Context, class subproc. // GetFileTree returns the file tree for a given path and depth func (wt *WorkspaceTracker) GetFileTree(reqPath string, depth int) (*types.FileTreeNode, error) { + if err := workspacepath.ValidateTreePath(reqPath); err != nil { + return nil, err + } // Resolve the full path with path traversal protection safePath := filepath.Join(wt.workDir, filepath.Clean(reqPath)) cleanWorkDir := filepath.Clean(wt.workDir) diff --git a/apps/backend/internal/agentctl/server/process/workspace_files_test.go b/apps/backend/internal/agentctl/server/process/workspace_files_test.go index 9bcce1a039..b2f07cf725 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files_test.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/common/workspacepath" storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" "go.uber.org/zap" ) @@ -509,6 +510,45 @@ func TestGetFileTree_HidesOnlyRootOwnershipMarker(t *testing.T) { } } +func TestGetFileTree_RejectsAbsolutePathBeforeFilesystemAccess(t *testing.T) { + workspace := t.TempDir() + external := t.TempDir() + log, err := logger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatal(err) + } + + _, err = (&WorkspaceTracker{workDir: workspace, logger: log}).GetFileTree(external, 1) + if !errors.Is(err, workspacepath.ErrTreePathNotRelative) { + t.Fatalf("GetFileTree error = %v, want %v", err, workspacepath.ErrTreePathNotRelative) + } +} + +func TestGetFileTree_AllowsLiteralColonInRelativePath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows does not permit literal colons in path components") + } + workspace := t.TempDir() + directory := filepath.Join(workspace, "config:dev") + if err := os.Mkdir(directory, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(directory, "settings.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + tree, err := (&WorkspaceTracker{workDir: workspace}).GetFileTree("config:dev", 1) + if err != nil { + t.Fatalf("GetFileTree failed: %v", err) + } + if tree.Path != "config:dev" { + t.Fatalf("GetFileTree root path = %q, want %q", tree.Path, "config:dev") + } + if findChild(tree, "settings.json") == nil { + t.Fatal("file beneath colon-containing directory should be visible") + } +} + func TestGetFileList_HidesOnlyRootOwnershipMarker(t *testing.T) { taskRoot := createOwnershipMarkerFixture(t) initGitRepoAt(t, taskRoot) diff --git a/apps/backend/internal/common/workspacepath/tree.go b/apps/backend/internal/common/workspacepath/tree.go new file mode 100644 index 0000000000..4b78659d03 --- /dev/null +++ b/apps/backend/internal/common/workspacepath/tree.go @@ -0,0 +1,61 @@ +// Package workspacepath validates path identities used by workspace services. +package workspacepath + +import ( + "errors" + pathpkg "path" + "path/filepath" + "strings" +) + +// ErrTreePathNotRelative identifies a path that cannot address the workspace tree. +var ErrTreePathNotRelative = errors.New("file tree path must be workspace-relative") + +// ValidateTreePath accepts the empty workspace root and canonical slash-separated relative paths. +func ValidateTreePath(requested string) error { + if requested == "" { + return nil + } + if strings.IndexByte(requested, 0) >= 0 || + strings.Contains(requested, `\`) || + filepath.IsAbs(requested) || + pathpkg.IsAbs(filepath.ToSlash(requested)) || + filepath.VolumeName(requested) != "" || + hasWindowsDrivePrefix(requested) || + hasAbsoluteURIForm(requested) { + return ErrTreePathNotRelative + } + cleaned := pathpkg.Clean(requested) + if cleaned != requested || cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return ErrTreePathNotRelative + } + return nil +} + +func hasAbsoluteURIForm(path string) bool { + colon := strings.IndexByte(path, ':') + if colon <= 0 || !isASCIILetter(path[0]) { + return false + } + for index := 1; index < colon; index++ { + char := path[index] + if !isASCIILetter(char) && (char < '0' || char > '9') && char != '+' && char != '-' && char != '.' { + return false + } + } + remainder := path[colon+1:] + return strings.HasPrefix(remainder, "//") || + strings.EqualFold(path[:colon], "file") && strings.HasPrefix(remainder, "/") +} + +func hasWindowsDrivePrefix(path string) bool { + if len(path) < 2 || path[1] != ':' { + return false + } + first := path[0] + return isASCIILetter(first) +} + +func isASCIILetter(char byte) bool { + return char >= 'A' && char <= 'Z' || char >= 'a' && char <= 'z' +} diff --git a/apps/backend/internal/common/workspacepath/tree_test.go b/apps/backend/internal/common/workspacepath/tree_test.go new file mode 100644 index 0000000000..03f6cb52ce --- /dev/null +++ b/apps/backend/internal/common/workspacepath/tree_test.go @@ -0,0 +1,35 @@ +package workspacepath + +import "testing" + +func TestValidateTreePath(t *testing.T) { + for _, path := range []string{"", "src", "src/components", ".codex/agents", "config:dev", "dir/.env:"} { + t.Run("valid_"+path, func(t *testing.T) { + if err := ValidateTreePath(path); err != nil { + t.Fatalf("ValidateTreePath(%q) = %v, want nil", path, err) + } + }) + } + + for _, path := range []string{ + "/home/jcfs/project/src", + `C:\workspace\src`, + "C:/workspace/src", + "file:///workspace/src", + "file:/workspace/src", + ".", + "..", + "../src", + "src/../outside", + "src//components", + "src/components/", + "src\\components", + "src\x00components", + } { + t.Run("invalid_"+path, func(t *testing.T) { + if err := ValidateTreePath(path); err != ErrTreePathNotRelative { + t.Fatalf("ValidateTreePath(%q) = %v, want %v", path, err, ErrTreePathNotRelative) + } + }) + } +} diff --git a/apps/web/components/task/chat/messages/tool-edit-message.test.tsx b/apps/web/components/task/chat/messages/tool-edit-message.test.tsx index 3799eb8909..7475d0274c 100644 --- a/apps/web/components/task/chat/messages/tool-edit-message.test.tsx +++ b/apps/web/components/task/chat/messages/tool-edit-message.test.tsx @@ -57,3 +57,21 @@ describe("ToolEditMessage write summary", () => { expect(html).not.toContain("line)"); }); }); + +describe("ToolEditMessage file action", () => { + // @covers AC-UI-FILE-TREE-PATH-SCOPE-001.1 + it("does not treat a workspace prefix collision as an in-workspace file", () => { + const comment = writeComment("Write utils.ts", "only line"); + const metadata = comment.metadata as { + normalized: { modify_file: { file_path: string } }; + }; + metadata.normalized.modify_file.file_path = "/workspace-old/apps/web/lib/utils.ts"; + + const html = renderToStaticMarkup( + undefined} />, + ); + + expect(html).toContain('title="Copy path"'); + expect(html).not.toContain('title="Open file"'); + }); +}); diff --git a/apps/web/components/task/chat/messages/tool-edit-message.tsx b/apps/web/components/task/chat/messages/tool-edit-message.tsx index 2effbb671a..aca3682ff2 100644 --- a/apps/web/components/task/chat/messages/tool-edit-message.tsx +++ b/apps/web/components/task/chat/messages/tool-edit-message.tsx @@ -18,6 +18,7 @@ import { ExpandableRow } from "./expandable-row"; import { transformFileMutation, type FileMutation } from "@/lib/diff"; import { useExpandState } from "./use-expand-state"; import { useOpenFileAtLine } from "@/hooks/use-file-editors"; +import { workspaceRelativeFilePath } from "@/lib/workspace-file-path"; import { useTranslation } from "react-i18next"; import type { TFunction } from "i18next"; @@ -79,7 +80,7 @@ function FileActionButton({ onCopyPath, }: FileActionButtonProps) { const { t } = useTranslation(); - const isFileInWorktree = worktreePath && filePath.startsWith(worktreePath); + const isFileInWorktree = workspaceRelativeFilePath(filePath, worktreePath) !== null; if (onOpenFile && isFileInWorktree) { return (