feat: allow read-only absolute file paths (#1371)
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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.
|
||||
@@ -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 |
|
||||
|
||||
Reference in New Issue
Block a user