From 18366382b436bdd690cc68190eb80aaacb430376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Tue, 23 Jun 2026 13:13:46 +0200 Subject: [PATCH] fix(backend): expand ~ home links and report missing file paths cleanly (#1468) --- .../server/process/workspace_files.go | 39 +++++++-- .../server/process/workspace_tracker_test.go | 52 ++++++++++++ .../task/chat-read-tilde-missing.spec.ts | 85 +++++++++++++++++++ 3 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 apps/web/e2e/tests/task/chat-read-tilde-missing.spec.ts diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index f0e505a7ad..281bc2d2ef 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -291,12 +291,21 @@ func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, return content, size, isBinary, resolved, nil } if alt := expandHomePath(stripReadSelector(reqPath)); alt != reqPath { - if c, s, b, r, altErr := wt.readResolvedPath(alt); altErr == nil { + c, s, b, r, altErr := wt.readResolvedPath(alt) + if altErr == nil { return c, s, b, r, nil } + // When the request used a "~" home shorthand, the literal attempt + // joins the tilde onto the workspace root ("...//~/...: + // no such file"), which reads as a mangled path rather than a missing + // file. Surface the expanded-path error, which names the real home + // location that was actually checked. + if isHomeShorthand(reqPath) { + return c, s, b, r, altErr + } } // Neither the literal path nor the stripped/expanded fallback opened; - // surface the original (literal-path) error, which is the most meaningful. + // surface the original (literal-path) error. return content, size, isBinary, resolved, err } @@ -315,11 +324,21 @@ func (wt *WorkspaceTracker) readResolvedPath(reqPath string) (string, int64, boo if err != nil { if errors.Is(err, errPathTraversal) { externalPath, ok := absoluteReadPath(reqPath) - if !ok { - return "", 0, false, "", err + if ok { + content, size, isBinary, readErr := readFileContent(externalPath) + return content, size, isBinary, "", readErr } - content, size, isBinary, readErr := readFileContent(externalPath) - return content, size, isBinary, "", readErr + // An absolute path that simply doesn't exist is not a traversal + // attempt — report a plain "file not found" naming the real path + // instead of the alarming "path traversal detected". Only remap + // genuine not-exist errors; permission/IO failures keep the + // original error so they aren't mislabeled as missing. + if cleaned := filepath.Clean(reqPath); filepath.IsAbs(cleaned) { + if _, statErr := os.Stat(cleaned); errors.Is(statErr, fs.ErrNotExist) { + return "", 0, false, "", fmt.Errorf("file not found: %w", statErr) + } + } + return "", 0, false, "", err } return "", 0, false, "", err } @@ -336,7 +355,7 @@ func (wt *WorkspaceTracker) readResolvedPath(reqPath string) (string, int64, boo // real filesystem path, so it must be expanded before stat/serve. Other paths // (absolute, workspace-relative) are returned unchanged. func expandHomePath(path string) string { - if path != "~" && !strings.HasPrefix(path, "~/") && !strings.HasPrefix(path, `~\`) { + if !isHomeShorthand(path) { return path } home, err := os.UserHomeDir() @@ -349,6 +368,12 @@ func expandHomePath(path string) string { return filepath.Join(home, path[2:]) } +// isHomeShorthand reports whether path uses the leading "~" / "~/" (or "~\" on +// Windows) home shorthand that expandHomePath rewrites. +func isHomeShorthand(path string) bool { + return path == "~" || strings.HasPrefix(path, "~/") || strings.HasPrefix(path, `~\`) +} + func readFileContent(safePath string) (string, int64, bool, error) { // Check if file exists and is a regular file info, err := os.Stat(safePath) diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go index 5d2e75e232..de9defc77b 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go @@ -669,6 +669,58 @@ func TestGetFileContent_PrefersLiteralTildePath(t *testing.T) { } } +// TestGetFileContent_MissingTildePathReportsHomeLocation reproduces the +// "Failed to open file" bug: clicking an agent read link like +// "~/.kandev/.../account-mapping.spec.ts" for a file that does not exist must +// not glue the tilde onto the workspace root +// ("...//~/.kandev/...: no such file"). The error must name the real +// home-expanded location and must not read as a path-traversal attempt. +func TestGetFileContent_MissingTildePathReportsHomeLocation(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + _, wt := setupTestDir(t) + + _, _, _, _, err := wt.GetFileContent("~/notes/missing/account-mapping.spec.ts:760-860") + if err == nil { + t.Fatal("expected error for missing tilde path, got nil") + } + // The tilde must never be path-joined onto the workspace root. + mangled := string(os.PathSeparator) + "~" + string(os.PathSeparator) + if strings.Contains(err.Error(), mangled) { + t.Fatalf("error joins tilde onto workspace (mangled path): %v", err) + } + // The error must name the real home-expanded location. + want := filepath.Join(home, "notes", "missing", "account-mapping.spec.ts") + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not name expanded home path %q", err.Error(), want) + } + // A missing file is not a traversal attempt. + if strings.Contains(err.Error(), "path traversal") { + t.Fatalf("missing tilde file reported as traversal: %v", err) + } +} + +// TestGetFileContent_MissingAbsolutePathNotTraversal verifies that an absolute +// path to a non-existent file (read-only external access per ADR 0016) reports +// a plain "file not found" naming the path, not the alarming "path traversal +// detected". +func TestGetFileContent_MissingAbsolutePathNotTraversal(t *testing.T) { + _, wt := setupTestDir(t) + missing := filepath.Join(t.TempDir(), "nope", "file.txt") + + _, _, _, _, err := wt.GetFileContent(missing) + if err == nil { + t.Fatal("expected error for missing absolute path, got nil") + } + if strings.Contains(err.Error(), "path traversal") { + t.Fatalf("missing absolute file reported as traversal: %v", err) + } + if !strings.Contains(err.Error(), missing) { + t.Fatalf("error %q does not name the requested path %q", err.Error(), missing) + } +} + // setupTestDir creates a temp directory with a WorkspaceTracker (no git required). func setupTestDir(t *testing.T) (string, *WorkspaceTracker) { t.Helper() diff --git a/apps/web/e2e/tests/task/chat-read-tilde-missing.spec.ts b/apps/web/e2e/tests/task/chat-read-tilde-missing.spec.ts new file mode 100644 index 0000000000..0b6922cf45 --- /dev/null +++ b/apps/web/e2e/tests/task/chat-read-tilde-missing.spec.ts @@ -0,0 +1,85 @@ +// Regression guard for "~/"-home agent read links. OMP emits read paths like +// "~/.kandev/x/foo.spec.ts:760-860"; the chat link strips the selector and opens +// the bare "~/..." path. When that file does not exist, the backend must report +// the failure against the expanded $HOME location, NOT by gluing the tilde onto +// the workspace root ("...//~/...: no such file"), which reads as a +// mangled path. Companion to chat-read-multi-file.spec.ts (selector splitting) +// and the Go unit coverage in workspace_tracker_test.go. +import { test, expect } from "../../fixtures/test-base"; +import { createStandardProfile } from "../../helpers/git-helper"; +import { SessionPage } from "../../pages/session-page"; + +test.describe("Chat tilde read links", () => { + test("missing ~/ read link fails with a clean, non-mangled error", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + const suffix = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + // A "~/"-home path whose target exists nowhere (neither under the workspace + // nor under $HOME). The bug glued the tilde onto the workspace root; the fix + // surfaces the expanded-home location instead. + const linkPath = `~/kandev-e2e-missing-${suffix}/account-mapping.spec.ts`; + + const profile = await createStandardProfile(apiClient, `tilde-read-${Date.now()}`); + // `e2e:message(...)` emits a single non-activity text message, so the seeded + // read card is the only activity message and renders as a standalone card. + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Chat Tilde Read", + profile.id, + { + description: 'e2e:message("ready")', + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + + // Seed a read card whose file_path is a "~/"-home shorthand with a line + // selector, exactly as the backend stores an omp read of a home-path file. + await apiClient.seedSessionMessage(task.session_id, { + type: "tool_read", + content: "Read file", + metadata: { + status: "complete", + tool_call_id: "tc-tilde-missing-read", + normalized: { + read_file: { + file_path: `${linkPath}:760-860`, + output: { content: "", line_count: 0 }, + }, + }, + }, + }); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle({ timeout: 45_000 }); + + // The link carries the bare home path (selector stripped by splitReadFiles). + const chat = session.activeChat(); + const link = chat.locator(`button[title="${linkPath}"]`); + await expect(link).toBeVisible({ timeout: 15_000 }); + + await link.click(); + + // The open fails (the file is missing). The error toast must report a clean + // "file not found" naming the expanded $HOME path — never the mangled + // "...//~/..." join, and never an alarming "path traversal". + const toast = testPage.getByTestId("toast-message").filter({ hasText: "Failed to open file" }); + await expect(toast).toBeVisible({ timeout: 10_000 }); + const text = (await toast.textContent()) ?? ""; + expect(text, `toast must not glue ~ onto the workspace root: ${text}`).not.toContain("/~/"); + expect(text.toLowerCase(), `toast should report a missing file: ${text}`).toContain( + "not found", + ); + expect(text.toLowerCase(), `missing file must not read as traversal: ${text}`).not.toContain( + "path traversal", + ); + }); +});