diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index f0c9b97f8..92b5135d7 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -365,40 +365,77 @@ func ResolveCallbackReference(ctx context.Context, callbackName string) (any, er return nil, fmt.Errorf("callback '%s' not found", callbackName) } -// ResolveAgentReference builds an agent from a reference config. -func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (agent.Agent, error) { +// resolveContainedConfigPath resolves refPath against the directory holding +// parentPath and returns the canonical result, rejecting references that +// escape that directory. +// +// Every config reference naming another file on disk must go through this +// helper, before the file is read and before any cache is consulted. A config +// file is only as trustworthy as whoever supplied it, and the directory of the +// referencing config is the boundary the loader promises to stay inside. +// +// This mirrors adk-python's resolve_agent_reference: reject an absolute +// reference, canonicalise both the reference and the agent directory, and +// require the first to sit under the second. The canonical path is returned, so +// a config reached through a symlink resolves its own references relative to +// where it really lives, as it does in Python. +func resolveContainedConfigPath(parentPath, refPath string) (string, error) { if refPath == "" { - return nil, fmt.Errorf("agent reference path cannot be empty") + return "", fmt.Errorf("config reference path cannot be empty") } if filepath.IsAbs(refPath) { - return nil, fmt.Errorf("absolute paths are not allowed in AgentTool config_path: %s", refPath) + return "", fmt.Errorf("absolute paths are not allowed in config_path: %s", refPath) } - targetPath := filepath.Join(filepath.Dir(parentPath), refPath) - - absPath, err := filepath.Abs(targetPath) + agentDir, err := filepath.Abs(filepath.Dir(parentPath)) if err != nil { - return nil, fmt.Errorf("failed to resolve absolute path: %w", err) + return "", fmt.Errorf("failed to resolve agent directory: %w", err) + } + // Join cleans the result, so ".." segments collapse before anything looks + // at the filesystem. + resolvedPath := realPath(filepath.Join(agentDir, refPath)) + canonicalAgentDir := realPath(agentDir) + + // Equivalent to Python's os.path.commonpath([dir, path]) != dir: the + // reference must be the directory itself or sit beneath it, compared by + // whole path elements rather than by string prefix. + rel, err := filepath.Rel(canonicalAgentDir, resolvedPath) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", fmt.Errorf( + "path traversal detected: config_path %q resolves outside agent directory", refPath) } - // 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)) + return resolvedPath, nil +} + +// realPath is the Go equivalent of Python's os.path.realpath: it resolves +// symlinks as far as the path exists and keeps the remainder as written. +// +// [filepath.EvalSymlinks] alone is not equivalent, because it fails outright +// when any component is missing, and a reference to a config that does not +// exist yet is an ordinary typo rather than a containment failure. Resolving +// the longest existing prefix keeps a symlinked parent directory honest while +// letting the missing tail through, so the caller reports "config file not +// found" instead of a traversal error. +func realPath(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + + dir, last := filepath.Split(path) + dir = filepath.Clean(dir) + if last == "" || dir == path { + return path // reached the root without resolving anything + } + return filepath.Join(realPath(dir), last) +} + +// ResolveAgentReference builds an agent from a reference config. +func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (agent.Agent, error) { + absPath, err := resolveContainedConfigPath(parentPath, refPath) 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) + return nil, err } registryMu.RLock() diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 4733d9f34..8a19fae12 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -142,3 +142,151 @@ func TestResolveAgentReferenceRelativeParentPath(t *testing.T) { t.Error("ResolveAgentReference with a relative parent path and an escaping reference succeeded, want an error") } } + +// TestResolveAgentReferenceFollowsSymlinkInsideDir guards against the +// containment check being too strict: a symlink that stays inside the agent +// directory is a legitimate way to organise configs, and adk-python allows it. +func TestResolveAgentReferenceFollowsSymlinkInsideDir(t *testing.T) { + _, parentPath := newAgentDir(t) + agentDir := filepath.Dir(parentPath) + + if err := os.Symlink("sub_agent.yaml", filepath.Join(agentDir, "alias.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + _, err := ResolveAgentReference(context.Background(), parentPath, "alias.yaml") + if err != nil && strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", parentPath, "alias.yaml", err) + } +} + +// TestResolveAgentReferenceMissingFileIsNotTraversal covers a reference to a +// config that does not exist. Canonicalisation cannot resolve it, but a typo +// must report the missing file rather than a containment failure, which is what +// os.path.realpath gives adk-python for free. +func TestResolveAgentReferenceMissingFileIsNotTraversal(t *testing.T) { + _, parentPath := newAgentDir(t) + + _, err := ResolveAgentReference(context.Background(), parentPath, filepath.Join("nodes", "typo.yaml")) + if err == nil { + t.Fatal("ResolveAgentReference for a missing config succeeded, want an error") + } + if strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference for a missing config = %v, want a not-found error", err) + } + if !strings.Contains(err.Error(), "config file not found") { + t.Errorf("ResolveAgentReference for a missing config = %v, want a not-found error", err) + } +} + +// TestResolveAgentReferenceThroughSymlinkedAgentDir covers an agent directory +// reached through a symlink. Both sides are canonicalised, so a reference +// inside it must still resolve. +func TestResolveAgentReferenceThroughSymlinkedAgentDir(t *testing.T) { + base, parentPath := newAgentDir(t) + agentDir := filepath.Dir(parentPath) + + linkedDir := filepath.Join(base, "linked_root") + if err := os.Symlink(agentDir, linkedDir); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + viaLink := filepath.Join(linkedDir, "root_agent.yaml") + _, err := ResolveAgentReference(context.Background(), viaLink, "sub_agent.yaml") + if err != nil && strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", viaLink, "sub_agent.yaml", err) + } +} + +// TestResolveAgentReferenceRejectsSiblingPrefixDir covers a sibling directory +// whose name begins with the agent directory's name. Containment has to compare +// whole path elements, not raw string prefixes. +func TestResolveAgentReferenceRejectsSiblingPrefixDir(t *testing.T) { + base, parentPath := newAgentDir(t) + + siblingDir := filepath.Join(base, "agents", "root-evil") + if err := os.MkdirAll(siblingDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", siblingDir, err) + } + if err := os.WriteFile(filepath.Join(siblingDir, "evil.yaml"), []byte("agent_class: LlmAgent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(evil.yaml) failed: %v", err) + } + + refPath := filepath.Join("..", "root-evil", "evil.yaml") + _, err := ResolveAgentReference(context.Background(), parentPath, refPath) + if err == nil { + t.Fatalf("ResolveAgentReference(_, %q, %q) succeeded, want error containing %q", parentPath, refPath, traversalError) + } + if !strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want error containing %q", parentPath, refPath, err, traversalError) + } +} + +// TestResolveAgentReferenceRejectsThroughDanglingSymlinkedDir covers a +// reference through a symlinked directory whose final component does not +// exist. realPath cannot resolve the reference in one EvalSymlinks call in +// that case, and falls back to resolving the longest existing prefix and +// rejoining the missing tail; that fallback must still see through the +// directory symlink and reject the escape, rather than comparing against the +// reference's unresolved, lexical spelling. +func TestResolveAgentReferenceRejectsThroughDanglingSymlinkedDir(t *testing.T) { + base, parentPath := newAgentDir(t) + agentDir := filepath.Dir(parentPath) + + elsewhere := filepath.Join(base, "elsewhere") + if err := os.MkdirAll(elsewhere, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", elsewhere, err) + } + if err := os.Symlink(elsewhere, filepath.Join(agentDir, "dirlink")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + // "missing.yaml" does not exist under elsewhere, so the reference as a + // whole cannot be resolved by a single EvalSymlinks call. + refPath := filepath.Join("dirlink", "missing.yaml") + _, err := ResolveAgentReference(context.Background(), parentPath, refPath) + if err == nil { + t.Fatalf("ResolveAgentReference(_, %q, %q) succeeded, want error containing %q", parentPath, refPath, traversalError) + } + if !strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want error containing %q", parentPath, refPath, err, traversalError) + } +} + +// TestResolveAgentReferenceCleansParentSegmentBeforeSymlinkResolution covers a +// reference that walks into a symlinked directory and back out with ".." +// before the final component. filepath.Join cleans "dirlink/.." away +// textually before anything looks at the filesystem, so the reference +// resolves as a plain sibling of the symlink inside the agent directory; the +// symlink's target is never consulted. This matches adk-python's +// normpath-then-realpath order and is pinned here so a future refactor does +// not silently change which file a reference like this loads. +func TestResolveAgentReferenceCleansParentSegmentBeforeSymlinkResolution(t *testing.T) { + base, parentPath := newAgentDir(t) + agentDir := filepath.Dir(parentPath) + + elsewhere := filepath.Join(base, "elsewhere") + if err := os.MkdirAll(elsewhere, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", elsewhere, err) + } + if err := os.Symlink(elsewhere, filepath.Join(agentDir, "dirlink")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + refPath := filepath.Join("dirlink", "..", "sub_agent.yaml") + _, err := ResolveAgentReference(context.Background(), parentPath, refPath) + if err != nil && strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", parentPath, refPath, err) + } +} + +// TestResolveAgentReferenceRejectsEmptyPath covers the empty reference, which +// must be rejected outright rather than falling through to filepath.Dir's "." +// and silently resolving to the agent directory itself. +func TestResolveAgentReferenceRejectsEmptyPath(t *testing.T) { + _, parentPath := newAgentDir(t) + + if _, err := ResolveAgentReference(context.Background(), parentPath, ""); err == nil { + t.Error("ResolveAgentReference with an empty reference succeeded, want an error") + } +} diff --git a/internal/configurable/configurable_workflow.go b/internal/configurable/configurable_workflow.go index 84f5c1ebd..91f1f3d39 100644 --- a/internal/configurable/configurable_workflow.go +++ b/internal/configurable/configurable_workflow.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "os" - "path/filepath" "strings" "gopkg.in/yaml.v3" @@ -200,14 +199,16 @@ func resolveNodeLike(ctx context.Context, parentPath, ref string) (workflow.Node isYAML := strings.HasSuffix(ref, ".yaml") || strings.HasSuffix(ref, ".yml") if isYAML { - targetPath := ref - if !filepath.IsAbs(ref) { - targetPath = filepath.Join(filepath.Dir(parentPath), ref) - } + // Containment is checked here, before the file is read and before the + // node cache is consulted, so a reference escaping the parent config's + // directory never reaches os.ReadFile. The FunctionNode, JoinNode and + // ToolNode branches of resolveNodeFromYAML all return before its + // fall-through call to ResolveAgentReference, so this is their only + // containment check. var err error - absPath, err = filepath.Abs(targetPath) + absPath, err = resolveContainedConfigPath(parentPath, ref) if err != nil { - return nil, fmt.Errorf("failed to resolve absolute path: %w", err) + return nil, err } cacheKey = absPath } else { diff --git a/internal/configurable/configurable_workflow_test.go b/internal/configurable/configurable_workflow_test.go index 7853a129d..24663a0f7 100644 --- a/internal/configurable/configurable_workflow_test.go +++ b/internal/configurable/configurable_workflow_test.go @@ -20,6 +20,7 @@ import ( "iter" "os" "path/filepath" + "strings" "testing" "time" @@ -587,3 +588,212 @@ edges: t.Errorf("expected tool output result 'tool_output', got %v", toolOut["result"]) } } + +const ( + traversalErr = "path traversal detected" + absoluteErr = "absolute paths are not allowed" +) + +// newWorkflowDirs lays out an agent directory to hold workflow configs and a +// sibling directory outside it to hold node configs that must stay unreachable. +func newWorkflowDirs(t *testing.T) (agentDir, outsideDir 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) + } + outsideDir = filepath.Join(base, "outside") + if err := os.MkdirAll(outsideDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", outsideDir, err) + } + return agentDir, outsideDir +} + +func writeConfig(t *testing.T, path, content string) string { + t.Helper() + + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", path, err) + } + return path +} + +// writeWorkflow writes a workflow config whose single edge chain runs from +// START through ref. +func writeWorkflow(t *testing.T, path, name, ref string) string { + t.Helper() + + return writeConfig(t, path, fmt.Sprintf("name: %s\nagent_class: Workflow\nedges:\n - - START\n - %q\n", name, ref)) +} + +// TestWorkflowNodeReferenceRejectsEscapingPath covers every node class the +// workflow loader handles. FunctionNode, JoinNode and ToolNode are resolved +// before the fall-through to ResolveAgentReference, so each needs its own +// coverage rather than relying on the agent branch being checked. +func TestWorkflowNodeReferenceRejectsEscapingPath(t *testing.T) { + agentDir, outsideDir := newWorkflowDirs(t) + + nodes := []struct { + class string + file string + content string + }{ + {"FunctionNode", "fn.yaml", "name: outside_fn\nagent_class: FunctionNode\nfunc_code: alpha_fn\n"}, + {"JoinNode", "join.yaml", "name: outside_join\nagent_class: JoinNode\n"}, + {"ToolNode", "tool.yaml", "name: outside_tool\nagent_class: ToolNode\ntool_code: test_tool\n"}, + {"LlmAgent", "agent.yaml", "name: outside_agent\nagent_class: LlmAgent\n"}, + } + + for _, node := range nodes { + nodePath := writeConfig(t, filepath.Join(outsideDir, node.file), node.content) + + refs := []struct { + name string + ref string + wantErr string + }{ + {"relative", filepath.Join("..", "..", "outside", node.file), traversalErr}, + {"absolute", nodePath, absoluteErr}, + } + + for _, ref := range refs { + t.Run(node.class+"/"+ref.name, func(t *testing.T) { + wfName := node.class + "_" + ref.name + wfPath := writeWorkflow(t, filepath.Join(agentDir, wfName+".yaml"), wfName, ref.ref) + + _, err := FromConfig(t.Context(), wfPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with node reference %q succeeded, want error containing %q", wfPath, ref.ref, ref.wantErr) + } + if !strings.Contains(err.Error(), ref.wantErr) { + t.Errorf("FromConfig(_, %q) with node reference %q = %v, want error containing %q", wfPath, ref.ref, err, ref.wantErr) + } + }) + } + } +} + +// TestWorkflowNodeReferenceRejectsEscapingSymlink covers a symlink that sits +// inside the agent directory but points out of it. +func TestWorkflowNodeReferenceRejectsEscapingSymlink(t *testing.T) { + agentDir, outsideDir := newWorkflowDirs(t) + + target := writeConfig(t, filepath.Join(outsideDir, "linked_join.yaml"), "name: outside_join\nagent_class: JoinNode\n") + if err := os.Symlink(target, filepath.Join(agentDir, "link.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + wfPath := writeWorkflow(t, filepath.Join(agentDir, "symlink_wf.yaml"), "symlink_wf", "link.yaml") + + _, err := FromConfig(t.Context(), wfPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with a symlinked node reference succeeded, want error containing %q", wfPath, traversalErr) + } + if !strings.Contains(err.Error(), traversalErr) { + t.Errorf("FromConfig(_, %q) with a symlinked node reference = %v, want error containing %q", wfPath, err, traversalErr) + } +} + +// TestWorkflowNodeReferenceCheckedBeforeRead points at a file that does not +// exist. Rejecting with the containment error rather than a filesystem error +// shows the check runs before the reference is opened. +func TestWorkflowNodeReferenceCheckedBeforeRead(t *testing.T) { + agentDir, _ := newWorkflowDirs(t) + + ref := filepath.Join("..", "..", "outside", "absent.yaml") + wfPath := writeWorkflow(t, filepath.Join(agentDir, "absent_wf.yaml"), "absent_wf", ref) + + _, err := FromConfig(t.Context(), wfPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with node reference %q succeeded, want error containing %q", wfPath, ref, traversalErr) + } + if !strings.Contains(err.Error(), traversalErr) { + t.Errorf("FromConfig(_, %q) with node reference %q = %v, want the containment error %q, not a filesystem error", wfPath, ref, err, traversalErr) + } +} + +// TestWorkflowNodeReferenceCheckedBeforeCache loads a node legitimately so it +// is cached under its absolute path, then reaches for that same cached path +// from another directory. The containment check must run before the cache +// lookup, or the cache hands back a node the second config may not reference. +func TestWorkflowNodeReferenceCheckedBeforeCache(t *testing.T) { + agentDir, outsideDir := newWorkflowDirs(t) + + writeConfig(t, filepath.Join(outsideDir, "cached_join.yaml"), "name: cached_join\nagent_class: JoinNode\n") + + // Load it from its own directory, where the reference is contained. + ownerPath := writeConfig(t, filepath.Join(outsideDir, "owner_wf.yaml"), + "name: owner_wf\nagent_class: Workflow\nedges:\n - - START\n - alpha_fn\n - cached_join.yaml\n") + if _, err := FromConfig(t.Context(), ownerPath); err != nil { + t.Fatalf("FromConfig(_, %q) failed for the contained reference: %v", ownerPath, err) + } + + ref := filepath.Join("..", "..", "outside", "cached_join.yaml") + wfPath := writeWorkflow(t, filepath.Join(agentDir, "cache_wf.yaml"), "cache_wf", ref) + + _, err := FromConfig(t.Context(), wfPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with node reference %q succeeded from the node cache, want error containing %q", wfPath, ref, traversalErr) + } + if !strings.Contains(err.Error(), traversalErr) { + t.Errorf("FromConfig(_, %q) with node reference %q = %v, want error containing %q", wfPath, ref, err, traversalErr) + } +} + +// TestWorkflowNodeReferenceRejectsThroughDanglingSymlinkedDir covers a node +// reference through a symlinked directory whose final component does not +// exist. realPath cannot resolve such a reference in a single EvalSymlinks +// call and falls back to resolving the longest existing prefix and rejoining +// the missing tail; that fallback must still see through the directory +// symlink and reject the escape. +func TestWorkflowNodeReferenceRejectsThroughDanglingSymlinkedDir(t *testing.T) { + agentDir, outsideDir := newWorkflowDirs(t) + + if err := os.Symlink(outsideDir, filepath.Join(agentDir, "dirlink")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + // "missing_join.yaml" does not exist under outsideDir, so the reference as + // a whole cannot be resolved by a single EvalSymlinks call. + ref := filepath.Join("dirlink", "missing_join.yaml") + wfPath := writeWorkflow(t, filepath.Join(agentDir, "dangling_wf.yaml"), "dangling_wf", ref) + + _, err := FromConfig(t.Context(), wfPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with node reference %q succeeded, want error containing %q", wfPath, ref, traversalErr) + } + if !strings.Contains(err.Error(), traversalErr) { + t.Errorf("FromConfig(_, %q) with node reference %q = %v, want error containing %q", wfPath, ref, err, traversalErr) + } +} + +// TestWorkflowNodeReferenceAllowsSubdirectory guards against the containment +// check rejecting a legitimate reference below the agent directory. +func TestWorkflowNodeReferenceAllowsSubdirectory(t *testing.T) { + agentDir, _ := newWorkflowDirs(t) + + subDir := filepath.Join(agentDir, "nodes") + if err := os.MkdirAll(subDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", subDir, err) + } + writeConfig(t, filepath.Join(subDir, "inner_join.yaml"), "name: inner_join\nagent_class: JoinNode\n") + + ref := filepath.Join("nodes", "inner_join.yaml") + wfPath := writeWorkflow(t, filepath.Join(agentDir, "subdir_wf.yaml"), "subdir_wf", ref) + + ag, err := FromConfig(t.Context(), wfPath) + if err != nil { + t.Fatalf("FromConfig(_, %q) with node reference %q failed: %v", wfPath, ref, err) + } + if ag.Name() != "subdir_wf" { + t.Errorf("FromConfig(_, %q) built agent %q, want %q", wfPath, ag.Name(), "subdir_wf") + } +}