From fcacaf9a6a9885b980dd245c895f0612c40f1f17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adil=20Burak=20=C5=9Een?= <56400880+adilburaksen@users.noreply.github.com> Date: Fri, 7 Aug 2026 11:02:33 +0300 Subject: [PATCH] fix(configurable): prevent path traversal in AgentTool config_path resolution (#878) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(configurable): prevent path traversal in AgentTool config_path resolution Reject absolute config_path values and verify that a relative one resolves inside the referencing agent's directory, resolving symlinks where the paths exist. Matches the hard rejection adk-python landed in 171ae9e; adk-java (#1218) chose a warn-only deprecation instead. BREAKING: an absolute config_path is no longer accepted. --------- * 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. * 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. --------- Co-authored-by: João Westerberg (cherry picked from commit 604dd63e647b12a2f0025f68606d692d7ae24d7a) --- internal/configurable/configurable_utils.go | 28 +++- .../configurable/configurable_utils_test.go | 144 ++++++++++++++++++ 2 files changed, 168 insertions(+), 4 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 6f67e5bad..ed1840fc0 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,36 @@ 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. 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) + } + registryMu.RLock() if a, ok := agentRegistry[absPath]; ok { registryMu.RUnlock() 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") + } +}