Skip to content

fix(agent/agentcontext): canonicalize scan root in symlink boundary check #26175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kylecarbs wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −8
Open

fix(agent/agentcontext): canonicalize scan root in symlink boundary check #26175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions agent/agentcontext/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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 },
Expand Down
10 changes: 5 additions & 5 deletions agent/agentcontext/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion agent/agentcontext/paths_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))

Expand Down
9 changes: 9 additions & 0 deletions agent/agentcontext/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Loading