fix(backend): expand ~ home links and report missing file paths cleanly (#1468)
This commit is contained in:
@@ -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 (".../<workspace>/~/...:
|
||||
// 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)
|
||||
|
||||
@@ -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
|
||||
// (".../<workspace>/~/.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()
|
||||
|
||||
@@ -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 (".../<workspace>/~/...: 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
|
||||
// ".../<workspace>/~/..." 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",
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user