From 2a4876eab6653f77f0dc90a8a1ae3921e6c84724 Mon Sep 17 00:00:00 2001 From: adilburaksen Date: Sun, 24 May 2026 00:38:25 +0300 Subject: [PATCH 1/2] fix(configurable): prevent path traversal in AgentTool config_path resolution Absolute config_path values were accepted unconditionally, and relative paths were joined without boundary validation, allowing traversal outside the agent directory via "../../../etc/passwd" style inputs. Fix: reject absolute paths; verify the resolved path stays within the parent agent's directory using strings.HasPrefix after filepath.Clean. --- internal/configurable/configurable_utils.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index 6f67e5bad..119b4c5cf 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -22,6 +22,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "sync" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -366,17 +367,24 @@ func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (age return nil, fmt.Errorf("agent reference path cannot be empty") } - targetPath := refPath - // Handle relative paths - if !filepath.IsAbs(refPath) { - targetPath = filepath.Join(filepath.Dir(parentPath), refPath) + if filepath.IsAbs(refPath) { + return nil, fmt.Errorf("absolute paths are not allowed in AgentTool config_path: %s", refPath) } + targetPath := filepath.Join(filepath.Dir(parentPath), refPath) + absPath, err := filepath.Abs(targetPath) if err != nil { return nil, fmt.Errorf("failed to resolve absolute path: %w", err) } + // Prevent path traversal outside the parent agent's directory. + parentDir := filepath.Clean(filepath.Dir(parentPath)) + if !strings.HasPrefix(absPath, parentDir+string(os.PathSeparator)) && absPath != parentDir { + return nil, fmt.Errorf( + "path traversal detected: config_path %q resolves outside agent directory", refPath) + } + registryMu.RLock() if a, ok := agentRegistry[absPath]; ok { registryMu.RUnlock() From 9941c4d3e7eee73a12879c06d4307f84d950a18f Mon Sep 17 00:00:00 2001 From: adilburaksen Date: Tue, 28 Jul 2026 13:52:21 +0300 Subject: [PATCH 2/2] fix(configurable): compare absolute, symlink-resolved paths in the config_path containment check The containment check for AgentTool config_path compared an absolute target path against a parent directory that was only cleaned, not made absolute, so a relative parentPath caused legitimate in-directory references to be rejected. The comparison was also purely lexical, so a symlink inside the agent directory could still resolve outside it and be loaded. Make both sides absolute before comparing and resolve symlinks where the paths exist, falling back to the lexical path when they do not so that a missing file still reports as not found rather than as a traversal. Add regression tests covering absolute paths, parent traversal, symlink escape, references inside the agent directory, and a relative parent path. --- internal/configurable/configurable_utils.go | 18 ++- .../configurable/configurable_utils_test.go | 144 ++++++++++++++++++ 2 files changed, 159 insertions(+), 3 deletions(-) create mode 100644 internal/configurable/configurable_utils_test.go diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index 119b4c5cf..ed1840fc0 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -378,9 +378,21 @@ func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (age return nil, fmt.Errorf("failed to resolve absolute path: %w", err) } - // Prevent path traversal outside the parent agent's directory. - parentDir := filepath.Clean(filepath.Dir(parentPath)) - if !strings.HasPrefix(absPath, parentDir+string(os.PathSeparator)) && absPath != parentDir { + // Prevent path traversal outside the parent agent's directory. Both sides are + // made absolute before comparing, and symlinks are resolved where the paths + // exist, so a symlink inside the agent directory cannot be used to escape it. + parentDir, err := filepath.Abs(filepath.Dir(parentPath)) + if err != nil { + return nil, fmt.Errorf("failed to resolve agent directory: %w", err) + } + if resolved, err := filepath.EvalSymlinks(parentDir); err == nil { + parentDir = resolved + } + checkPath := absPath + if resolved, err := filepath.EvalSymlinks(absPath); err == nil { + checkPath = resolved + } + if !strings.HasPrefix(checkPath, parentDir+string(os.PathSeparator)) && checkPath != parentDir { return nil, fmt.Errorf( "path traversal detected: config_path %q resolves outside agent directory", refPath) } diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go new file mode 100644 index 000000000..4733d9f34 --- /dev/null +++ b/internal/configurable/configurable_utils_test.go @@ -0,0 +1,144 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package configurable + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +const traversalError = "path traversal detected" + +// newAgentDir lays out an agent directory with a sibling config, a file outside +// the directory, and a symlink inside the directory pointing at that outside +// file. It returns the base directory and the parent agent config path. +func newAgentDir(t *testing.T) (base, parentPath string) { + t.Helper() + + base = t.TempDir() + // Resolve the temporary directory itself, since on some platforms it is + // reached through a symlink (for example /var -> /private/var on macOS). + if resolved, err := filepath.EvalSymlinks(base); err == nil { + base = resolved + } + + agentDir := filepath.Join(base, "agents", "root") + if err := os.MkdirAll(agentDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", agentDir, err) + } + + parentPath = filepath.Join(agentDir, "root_agent.yaml") + if err := os.WriteFile(parentPath, []byte("agent_class: LlmAgent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", parentPath, err) + } + if err := os.WriteFile(filepath.Join(agentDir, "sub_agent.yaml"), []byte("agent_class: LlmAgent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(sub_agent.yaml) failed: %v", err) + } + + outside := filepath.Join(base, "outside.yaml") + if err := os.WriteFile(outside, []byte("agent_class: LlmAgent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", outside, err) + } + if err := os.Symlink(outside, filepath.Join(agentDir, "link.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + return base, parentPath +} + +func TestResolveAgentReferenceRejectsEscapingConfigPath(t *testing.T) { + _, parentPath := newAgentDir(t) + + tests := []struct { + name string + refPath string + wantErr string + }{ + { + name: "absolute path", + refPath: filepath.Join(string(os.PathSeparator), "etc", "passwd"), + wantErr: "absolute paths are not allowed", + }, + { + name: "parent traversal", + refPath: filepath.Join("..", "..", "outside.yaml"), + wantErr: traversalError, + }, + { + name: "symlink escaping the agent directory", + refPath: "link.yaml", + wantErr: traversalError, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // The reference must be rejected by the containment check itself, not + // merely fail later while the config is being loaded. + _, err := ResolveAgentReference(context.Background(), parentPath, tc.refPath) + if err == nil { + t.Fatalf("ResolveAgentReference(_, %q, %q) succeeded, want error containing %q", parentPath, tc.refPath, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want error containing %q", parentPath, tc.refPath, err, tc.wantErr) + } + }) + } +} + +// TestResolveAgentReferenceAllowsPathsInsideAgentDir guards against the +// containment check rejecting legitimate references. The reference is not a +// loadable agent config, so an error is expected; it must not be the traversal +// error. +func TestResolveAgentReferenceAllowsPathsInsideAgentDir(t *testing.T) { + _, parentPath := newAgentDir(t) + + _, err := ResolveAgentReference(context.Background(), parentPath, "sub_agent.yaml") + if err != nil && strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", parentPath, "sub_agent.yaml", err) + } +} + +// TestResolveAgentReferenceRelativeParentPath covers a parent path that is not +// absolute, which the containment check must normalise before comparing. +func TestResolveAgentReferenceRelativeParentPath(t *testing.T) { + _, parentPath := newAgentDir(t) + agentDir := filepath.Dir(parentPath) + + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd failed: %v", err) + } + if err := os.Chdir(agentDir); err != nil { + t.Fatalf("Chdir(%q) failed: %v", agentDir, err) + } + t.Cleanup(func() { + if err := os.Chdir(cwd); err != nil { + t.Fatalf("restoring working directory failed: %v", err) + } + }) + + if _, err := ResolveAgentReference(context.Background(), "root_agent.yaml", "sub_agent.yaml"); err != nil && + strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference with a relative parent path = %v, want no traversal rejection", err) + } + + if _, err := ResolveAgentReference(context.Background(), "root_agent.yaml", filepath.Join("..", "..", "outside.yaml")); err == nil { + t.Error("ResolveAgentReference with a relative parent path and an escaping reference succeeded, want an error") + } +}