From 2c3b78eb5a4f0b6fe2bf2ccd6b5aae50350d1c95 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Tue, 9 Jun 2026 18:08:42 +0000 Subject: [PATCH] fix(agent/agentcontext): canonicalize scan root in symlink boundary check On macOS, /var is a symlink to /private/var, so filepath.EvalSymlinks on a symlink target inside a scan root produces a /private/var path even when the caller's scanRoot is still /var/.... The boundary check in resolveReadTarget compared the canonical target against an un-canonical scan root and rejected legitimate in-root symlinks as StatusInvalid. This matches the lenient EvalSymlinks pattern already used in agentgit and agentmcp. Canonicalize scanRoot symmetrically with the target before the prefix check so symlinks inside the scan root are accepted while escaping targets are still rejected. Fixes the macOS test flakes in agent/agentcontext where t.TempDir() returns /var/folders/... paths that diverge from the canonical form the Manager and API surface. The affected tests now pin to the canonical temp dir via testutil.TempDirResolved, the same helper already used in agentmcp and agentgit. --- agent/agentcontext/api_test.go | 4 ++-- agent/agentcontext/manager_test.go | 10 +++++----- agent/agentcontext/paths_test.go | 3 ++- agent/agentcontext/resolve.go | 9 +++++++++ 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/agent/agentcontext/api_test.go b/agent/agentcontext/api_test.go index 1563345d9e3e8..2a5abd2362396 100644 --- a/agent/agentcontext/api_test.go +++ b/agent/agentcontext/api_test.go @@ -61,7 +61,7 @@ func TestAPI_ListSourcesEmpty(t *testing.T) { func TestAPI_AddAndListSource(t *testing.T) { t.Parallel() wd := t.TempDir() - src := t.TempDir() + src := testutil.TempDirResolved(t) srv, _ := newAPITestServer(t, agentcontext.ManagerOptions{ WorkingDir: func() string { return wd }, @@ -103,7 +103,7 @@ func TestAPI_AddSourceRejected(t *testing.T) { func TestAPI_GetAndDeleteSource(t *testing.T) { t.Parallel() wd := t.TempDir() - src := t.TempDir() + src := testutil.TempDirResolved(t) srv, m := newAPITestServer(t, agentcontext.ManagerOptions{ WorkingDir: func() string { return wd }, diff --git a/agent/agentcontext/manager_test.go b/agent/agentcontext/manager_test.go index 42a1edab03c8e..16ba6c4941353 100644 --- a/agent/agentcontext/manager_test.go +++ b/agent/agentcontext/manager_test.go @@ -67,8 +67,8 @@ func TestManager_InitialSnapshotIsPopulated(t *testing.T) { func TestManager_AddSourceTriggersResolve(t *testing.T) { t.Parallel() - wd := t.TempDir() - src := t.TempDir() + wd := testutil.TempDirResolved(t) + src := testutil.TempDirResolved(t) mustWriteFile(t, filepath.Join(src, "AGENTS.md"), "from source") m := newTestManager(t, agentcontext.ManagerOptions{ @@ -194,7 +194,7 @@ func TestManager_RemoveSource(t *testing.T) { func TestManager_HasSource(t *testing.T) { t.Parallel() wd := t.TempDir() - src := t.TempDir() + src := testutil.TempDirResolved(t) m := newTestManager(t, agentcontext.ManagerOptions{ WorkingDir: func() string { return wd }, @@ -285,7 +285,7 @@ func TestManager_ResyncCanceledKeepsLiveSnapshot(t *testing.T) { func TestManager_InitialSourcesSeeded(t *testing.T) { t.Parallel() wd := t.TempDir() - src := t.TempDir() + src := testutil.TempDirResolved(t) mustWriteFile(t, filepath.Join(src, "AGENTS.md"), "from initial") m := newTestManager(t, agentcontext.ManagerOptions{ @@ -312,7 +312,7 @@ func TestManager_InitialSourcesSeeded(t *testing.T) { func TestManager_SeedSourcesLateBindsAfterManifest(t *testing.T) { t.Parallel() wd := t.TempDir() - late := t.TempDir() + late := testutil.TempDirResolved(t) mustWriteFile(t, filepath.Join(late, "AGENTS.md"), "late binding") // AllowedRoots intentionally omits `late` so AddSource diff --git a/agent/agentcontext/paths_test.go b/agent/agentcontext/paths_test.go index f70af76788140..c737bb338dcc2 100644 --- a/agent/agentcontext/paths_test.go +++ b/agent/agentcontext/paths_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/agent/agentcontext" + "github.com/coder/coder/v2/testutil" ) // switchHomeEnv overrides the platform-specific environment @@ -99,7 +100,7 @@ func TestValidateSourcePath_RejectsParentSegments(t *testing.T) { func TestValidateSourcePath_AllowsInsideRoot(t *testing.T) { t.Parallel() - dir := t.TempDir() + dir := testutil.TempDirResolved(t) child := filepath.Join(dir, "child") require.NoError(t, os.MkdirAll(child, 0o755)) diff --git a/agent/agentcontext/resolve.go b/agent/agentcontext/resolve.go index 5f2dba9cf80eb..08a0fbe46f797 100644 --- a/agent/agentcontext/resolve.go +++ b/agent/agentcontext/resolve.go @@ -368,7 +368,16 @@ func resolveReadTarget(path string, info fs.FileInfo, scanRoot string) (readPath if err != nil { return "", nil, false, StatusUnreadable, fmt.Sprintf("symlink resolve: %v", err) } + // Canonicalize scanRoot symmetrically with the target so the + // boundary check survives platform-level symlinks in the scan + // root prefix. macOS, for example, exposes /var as a symlink + // to /private/var; EvalSymlinks on the target produces a + // /private/var path while the caller's scanRoot may still be + // /var, which would incorrectly trip the prefix check. rootClean := filepath.Clean(scanRoot) + if resolved, err := filepath.EvalSymlinks(rootClean); err == nil { + rootClean = resolved + } if !pathHasPrefix(target, rootClean) { return "", nil, false, StatusInvalid, fmt.Sprintf("symlink target %q escapes scan root %q", target, scanRoot) }