diff --git a/apps/backend/internal/agentctl/server/process/manager_rescan_path_test.go b/apps/backend/internal/agentctl/server/process/manager_rescan_path_test.go index 78432b9979..ec9ae1c8df 100644 --- a/apps/backend/internal/agentctl/server/process/manager_rescan_path_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_rescan_path_test.go @@ -18,7 +18,7 @@ import ( // Windows (`C:\...`) and POSIX (`/...`) without per-OS branches in // every case. func TestResolveRescanPath(t *testing.T) { - root := absRoot() // "/" on POSIX, "C:\" on Windows + root := filepath.Join(t.TempDir(), "root") repo := join(root, "task", "repo") task := join(root, "task") other := join(root, "task", "other") @@ -43,8 +43,8 @@ func TestResolveRescanPath(t *testing.T) { {"child of current (rejected)", sub, repo, "", false}, {"sibling repo (rejected)", other, repo, "", false}, {"unrelated absolute (rejected)", etc, repo, "", false}, - {"current is filesystem root", root, root, root, true}, - {"current is root, promotion impossible", join(root, "foo"), root, "", false}, + {"current is anchor root", root, root, root, true}, + {"current is anchor root, child rejected", join(root, "foo"), root, "", false}, {"dotted segment is legal as exact match", dotted, dotted, dotted, true}, {"dotted segment is legal as promotion", dotted, dottedRepo, dotted, true}, } @@ -72,13 +72,6 @@ func TestResolveRescanPath(t *testing.T) { } } -// absRoot returns the platform's absolute filesystem root prefix — -// "/" on POSIX, "C:\" (the runner's drive) on Windows. -func absRoot() string { - root, _ := filepath.Abs(string(filepath.Separator)) - return root -} - // join wraps filepath.Join so callers can express paths with separate // segments without thinking about platform separators. func join(root string, segs ...string) string { diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index 3437ed50c8..d02e41bc97 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -32,6 +32,8 @@ const ( const maxFileSize = 10 * 1024 * 1024 // 10MB +var errPathTraversal = errors.New("path traversal detected") + // updateFiles updates the file listing func (wt *WorkspaceTracker) updateFiles(ctx context.Context) { files, err := wt.getFileList(ctx) @@ -169,6 +171,25 @@ func (wt *WorkspaceTracker) resolvedWorkDir() string { return resolved } +func absoluteReadPath(reqPath string) (string, bool) { + cleanReqPath := filepath.Clean(reqPath) + if !filepath.IsAbs(cleanReqPath) { + return "", false + } + + realPath, err := filepath.EvalSymlinks(cleanReqPath) + if err != nil { + return "", false + } + + // codeql[go/path-injection] Intentional read-only absolute file access; see ADR 0016. + info, err := os.Stat(realPath) + if err != nil || !info.Mode().IsRegular() { + return "", false + } + return realPath, true +} + // resolveSafePath resolves reqPath to an absolute path within workDir, // rejecting any path traversal attempts. The returned path is always // constructed as filepath.Join(resolvedWorkDir, validatedRelPath) so that @@ -216,7 +237,7 @@ func (wt *WorkspaceTracker) resolveSafePath(reqPath string) (string, error) { // Ensure the relative path doesn't escape the workspace if strings.HasPrefix(relPath, ".."+string(os.PathSeparator)) || relPath == ".." { - return "", fmt.Errorf("path traversal detected: %s", reqPath) + return "", fmt.Errorf("%w: %s", errPathTraversal, reqPath) } // Reconstruct the absolute path from the trusted workspace root and the @@ -260,31 +281,44 @@ func resolveNonExistentPath(path string) (string, error) { func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, string, error) { safePath, err := wt.resolveSafePath(reqPath) if err != nil { + if errors.Is(err, errPathTraversal) { + externalPath, ok := absoluteReadPath(reqPath) + if !ok { + return "", 0, false, "", err + } + content, size, isBinary, readErr := readFileContent(externalPath) + return content, size, isBinary, "", readErr + } return "", 0, false, "", err } // Check if the original path is a symlink and compute the resolved relative path. resolvedPath := wt.resolveSymlinkRelPath(reqPath) + content, size, isBinary, err := readFileContent(safePath) + return content, size, isBinary, resolvedPath, err +} + +func readFileContent(safePath string) (string, int64, bool, error) { // Check if file exists and is a regular file info, err := os.Stat(safePath) if err != nil { - return "", 0, false, "", fmt.Errorf("file not found: %w", err) + return "", 0, false, fmt.Errorf("file not found: %w", err) } - if info.IsDir() { - return "", 0, false, "", fmt.Errorf("path is a directory, not a file") + if !info.Mode().IsRegular() { + return "", 0, false, fmt.Errorf("path is not a regular file") } // Check file size if info.Size() > maxFileSize { - return "", info.Size(), false, "", fmt.Errorf("file too large (max 10MB)") + return "", info.Size(), false, fmt.Errorf("file too large (max 10MB)") } // Read file content file, err := os.Open(safePath) if err != nil { - return "", 0, false, "", fmt.Errorf("failed to open file: %w", err) + return "", 0, false, fmt.Errorf("failed to open file: %w", err) } defer func() { _ = file.Close() @@ -292,16 +326,16 @@ func (wt *WorkspaceTracker) GetFileContent(reqPath string) (string, int64, bool, content, err := io.ReadAll(file) if err != nil { - return "", 0, false, "", fmt.Errorf("failed to read file: %w", err) + return "", 0, false, fmt.Errorf("failed to read file: %w", err) } // Detect binary: if content is not valid UTF-8, base64-encode it if !utf8.Valid(content) { encoded := base64.StdEncoding.EncodeToString(content) - return encoded, info.Size(), true, resolvedPath, nil + return encoded, info.Size(), true, nil } - return string(content), info.Size(), false, resolvedPath, nil + return string(content), info.Size(), false, nil } // resolveSymlinkRelPath checks if reqPath is a symlink and returns the resolved 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 de49703409..81ff052c35 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files_test.go @@ -115,6 +115,9 @@ func TestResolveNonExistentPath(t *testing.T) { } t.Cleanup(func() { _ = os.Chmod(restrictedDir, 0o755) }) + if _, probeErr := filepath.EvalSymlinks(innerDir); probeErr == nil { + t.Skip("chmod 0o000 did not block path resolution in this environment") + } path := filepath.Join(innerDir, "file.txt") _, err := resolveNonExistentPath(path) if err == nil { 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 63acf560cf..22bbf9bcbb 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker_test.go @@ -514,6 +514,48 @@ func TestGetFileContent_BinaryDetection(t *testing.T) { } } +func TestGetFileContent_ExternalAbsolutePath(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + defer cleanup() + + externalDir := t.TempDir() + externalPath := filepath.Join(externalDir, "sprite-doc.txt") + externalContent := "outside workspace\n" + if err := os.WriteFile(externalPath, []byte(externalContent), 0o644); err != nil { + t.Fatalf("failed to write external file: %v", err) + } + + log := newTestLogger(t) + wt := NewWorkspaceTracker(repoDir, log) + + content, size, isBinary, resolvedPath, err := wt.GetFileContent(externalPath) + if err != nil { + t.Fatalf("GetFileContent(external absolute path) error: %v", err) + } + if content != externalContent { + t.Fatalf("content = %q, want %q", content, externalContent) + } + if size != int64(len(externalContent)) { + t.Fatalf("size = %d, want %d", size, len(externalContent)) + } + if isBinary { + t.Fatal("expected text file to not be binary") + } + if resolvedPath != "" { + t.Fatalf("resolvedPath = %q, want empty for external file", resolvedPath) + } + + _, _, _, _, err = wt.GetFileContent(externalDir) + if err == nil { + t.Fatal("expected error for external absolute directory") + } + + _, _, _, _, err = wt.GetFileContent(filepath.Join(externalDir, "missing.txt")) + if err == nil { + t.Fatal("expected error for missing external absolute file") + } +} + // setupTestDir creates a temp directory with a WorkspaceTracker (no git required). func setupTestDir(t *testing.T) (string, *WorkspaceTracker) { t.Helper() diff --git a/apps/backend/internal/persistence/upgrade_test.go b/apps/backend/internal/persistence/upgrade_test.go index 832ff08c1d..1be8aa3d98 100644 --- a/apps/backend/internal/persistence/upgrade_test.go +++ b/apps/backend/internal/persistence/upgrade_test.go @@ -162,6 +162,11 @@ func TestProvide_BackupFailureReturnsError(t *testing.T) { if os.Getuid() == 0 { t.Skip("running as root; read-only dir check not reliable") } + probePath := filepath.Join(backupDir, ".write-probe") + if err := os.WriteFile(probePath, []byte("probe"), 0o644); err == nil { + _ = os.Remove(probePath) + t.Skip("read-only backup dir is writable in this environment") + } cfg := &config.Config{HomeDir: dir} cfg.Database.Path = dbPath diff --git a/apps/backend/internal/system/disk/walker_test.go b/apps/backend/internal/system/disk/walker_test.go index 0df6ee9042..2f2506c1fc 100644 --- a/apps/backend/internal/system/disk/walker_test.go +++ b/apps/backend/internal/system/disk/walker_test.go @@ -72,6 +72,9 @@ func TestWalkSubdir_RecordsPermissionWarningAndContinues(t *testing.T) { // Restore permissions so the temp dir can be cleaned up. _ = os.Chmod(blocked, 0o755) }) + if _, err := os.ReadDir(blocked); err == nil { + t.Skip("chmod 0o000 did not block directory reads in this environment") + } res := walkSubdir(root) diff --git a/docs/decisions/0016-observed-external-file-reads.md b/docs/decisions/0016-observed-external-file-reads.md new file mode 100644 index 0000000000..e1554b4e49 --- /dev/null +++ b/docs/decisions/0016-observed-external-file-reads.md @@ -0,0 +1,25 @@ +# 0016: Read-Only Absolute File Paths + +**Status:** accepted +**Date:** 2026-06-14 +**Area:** backend + +## Context + +Agent CLIs can read files outside the configured task workspace, and those absolute paths can appear in normalized ACP tool-call messages. Kandev's workspace file API previously rejected every outside-workspace path as path traversal, which prevented the UI from reopening files the agent could already inspect. The product decision is to let the UI read absolute file paths directly for parity with agent CLI behavior. + +## Decision + +`internal/agentctl/server/process.WorkspaceTracker.GetFileContent` remains workspace-scoped for relative paths and relative traversal attempts, but when the request path is absolute it resolves symlinks and reads the canonical file directly. The read path reuses the existing file-size cap and binary handling. + +Write operations, directory tree reads, search, git-ref reads, directories, missing files, and relative traversal remain rejected by their existing workspace-scoped path validation. Only the read-only current-file content endpoint accepts absolute paths. + +## Consequences + +The UI can display absolute files, including Sprite documentation files outside `/workspace`, without needing an ACP observation first. This intentionally makes the session file-content API a read-only host-file reader for any file the agentctl process can read. Deployments that expose Kandev beyond trusted users must account for that risk. + +## Alternatives Considered + +- Allow only observed absolute paths from agent read events: rejected because it prevents direct inspection of absolute paths even though the agent CLI can already read them. +- Allow specific host prefixes such as `/.sprite`: rejected because the problem is not Sprite-specific and prefix allowlists are easy to broaden incorrectly. +- Keep rejecting all external paths: rejected because it breaks inspection parity with the agent CLI. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index d13a1a043c..dfb7fd887b 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -21,3 +21,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 0013 | [Multi-branch task support — N (repo, branch) pairs per task](0013-multi-branch-tasks.md) | accepted | backend, frontend | 2026-06-01 | | 0014 | [Per-CLI MCP server injection for passthrough mode](0014-passthrough-mcp-injection-strategies.md) | accepted | backend | 2026-05-29 | | 0015 | [Explicit completion signal for auto-advance](0015-explicit-completion-signal-for-auto-advance.md) | proposed | backend, frontend | 2026-06-04 | +| 0016 | [Read-only absolute file paths](0016-observed-external-file-reads.md) | accepted | backend | 2026-06-14 |