From c3340efbdfca2fb3809cf8cc75f143677643fd4d Mon Sep 17 00:00:00 2001 From: Anton Dzyatkovsky Date: Mon, 10 Aug 2026 12:17:39 -0700 Subject: [PATCH 1/6] fix(configurable): apply config_path containment to workflow node refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #878 added a containment check to ResolveAgentReference: an AgentTool config_path may not be absolute, and a relative one must resolve inside the referencing agent's directory. Workflow edge references were left on the pre-#878 code — absolute refs accepted unconditionally, relative ones joined with no boundary check — and resolveNodeFromYAML reads them straight off disk with os.ReadFile before dispatching on agent_class. A workflow config could therefore load and instantiate a FunctionNode, JoinNode or ToolNode from anywhere on the filesystem. Extract the containment logic into resolveConfigReference and route both ResolveAgentReference and resolveNodeLike through it, so the rule has one implementation rather than one per reference kind. Two fixes to the check itself, both found while covering the shared helper: - The parent directory is now resolved before the reference is joined onto it, so both sides of the comparison are rooted in the same real path. Resolving only the parent side meant that a parent directory reached through a symlink plus a not-yet-existing target — which has no symlinks to resolve and so keeps its unresolved spelling — was reported as a traversal instead of as not found, the case #878 intended to allow. - A reference carrying a volume name is rejected alongside absolute ones. On Windows a drive-relative reference such as `C:node.yaml` is not absolute yet still escapes, by resolving against the current directory of that drive; filepath.VolumeName also covers UNC paths and is empty on Unix. BREAKING: an absolute config_path is no longer accepted in workflow edge references, matching the #878 change to agent references. Assisted-by: Claude Opus 5 --- internal/configurable/configurable_utils.go | 70 +++++--- .../configurable/configurable_utils_test.go | 57 +++++++ .../configurable/configurable_workflow.go | 12 +- .../configurable_workflow_test.go | 152 ++++++++++++++++++ 4 files changed, 265 insertions(+), 26 deletions(-) diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index f0c9b97f8..272d174ef 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -365,40 +365,72 @@ 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) { +// resolveConfigReference turns a config-supplied reference into an absolute path +// that is guaranteed to sit inside the referencing config's own directory. +// +// Absolute references are rejected outright, and a relative one must resolve +// inside the parent 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. Where a path does not exist the lexical +// path is used, so a missing file still reports as not found rather than as a +// traversal. +// +// Every place that loads a nested YAML config from a reference must route +// through here: the check is the trust boundary between the config being loaded +// and the rest of the filesystem, and a second copy of it is a second place to +// forget. +func resolveConfigReference(parentPath, refPath string) (string, error) { if refPath == "" { - return nil, fmt.Errorf("agent 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("config reference path cannot be empty") } - 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) + // IsAbs alone is not enough on Windows: a drive-relative reference such as + // `C:node.yaml` is not absolute, yet it escapes the parent directory by + // resolving against the current directory of that drive. VolumeName covers + // that and UNC paths, and is always empty on Unix. + if filepath.IsAbs(refPath) || filepath.VolumeName(refPath) != "" { + return "", fmt.Errorf("absolute paths are not allowed in config references: %s", 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)) if err != nil { - return nil, fmt.Errorf("failed to resolve agent directory: %w", err) - } + return "", fmt.Errorf("failed to resolve agent directory: %w", err) + } + // Resolve the parent directory BEFORE joining, so that both sides of the + // containment check are rooted in the same real path. Resolving only one + // side would reject a legitimate reference whenever the parent directory is + // reached through a symlink and the target does not exist yet — the target + // has no symlinks to resolve, so it would keep the unresolved spelling and + // fail the prefix test. if resolved, err := filepath.EvalSymlinks(parentDir); err == nil { parentDir = resolved } + + // parentDir is absolute and Join cleans the result, so this is the absolute, + // lexically-normalized target path. + absPath := filepath.Join(parentDir, refPath) + + // Resolve the target too, so a symlink inside the agent directory cannot be + // used to escape it. Where the target does not exist there is nothing to + // resolve and the lexical path stands, so a missing file reports as not found + // rather than as a traversal. 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 "", fmt.Errorf( + "path traversal detected: config reference %q resolves outside agent directory", refPath) + } + + return absPath, nil +} + +// ResolveAgentReference builds an agent from a reference config. +func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (agent.Agent, error) { + absPath, err := resolveConfigReference(parentPath, refPath) + if err != nil { + return nil, err } registryMu.RLock() diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 4733d9f34..50457207c 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -142,3 +142,60 @@ func TestResolveAgentReferenceRelativeParentPath(t *testing.T) { t.Error("ResolveAgentReference with a relative parent path and an escaping reference succeeded, want an error") } } + +// TestResolveAgentReferenceSymlinkedParentDir covers a parent directory that is +// itself reached through a symlink, together with a reference that does not +// exist on disk. Resolving only the parent side of the containment check would +// leave the two sides rooted differently — the target has no symlinks to +// resolve, so it keeps its unresolved spelling — and a missing file would be +// reported as a traversal instead of as not found. +func TestResolveAgentReferenceSymlinkedParentDir(t *testing.T) { + base := t.TempDir() + if resolved, err := filepath.EvalSymlinks(base); err == nil { + base = resolved + } + + realDir := filepath.Join(base, "real") + if err := os.MkdirAll(realDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", realDir, err) + } + if err := os.WriteFile(filepath.Join(realDir, "root_agent.yaml"), []byte("agent_class: LlmAgent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(root_agent.yaml) failed: %v", err) + } + + alias := filepath.Join(base, "alias") + if err := os.Symlink(realDir, alias); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + parentPath := filepath.Join(alias, "root_agent.yaml") + if _, err := ResolveAgentReference(context.Background(), parentPath, "missing.yaml"); err != nil && + strings.Contains(err.Error(), traversalError) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", parentPath, "missing.yaml", err) + } + + // The symlinked parent must not weaken the check itself. + if _, err := ResolveAgentReference(context.Background(), parentPath, filepath.Join("..", "outside.yaml")); err == nil { + t.Error("ResolveAgentReference through a symlinked parent with an escaping reference succeeded, want an error") + } +} + +// TestResolveConfigReferenceRejectsVolumeQualifiedRefs covers references that +// carry a volume name. On Windows a drive-relative reference such as +// `C:node.yaml` is not absolute yet still escapes the parent directory, so +// IsAbs alone is not a sufficient guard. filepath.VolumeName is empty on Unix, +// where these are ordinary (if odd) relative file names. +func TestResolveConfigReferenceRejectsVolumeQualifiedRefs(t *testing.T) { + _, parentPath := newAgentDir(t) + + for _, refPath := range []string{`C:node.yaml`, `D:\escaped.yaml`, `\\host\share\escaped.yaml`} { + _, err := resolveConfigReference(parentPath, refPath) + if filepath.VolumeName(refPath) == "" { + // Not volume-qualified on this platform; nothing to assert. + continue + } + if err == nil { + t.Errorf("resolveConfigReference(%q, %q) succeeded, want rejection", parentPath, refPath) + } + } +} diff --git a/internal/configurable/configurable_workflow.go b/internal/configurable/configurable_workflow.go index 84f5c1ebd..f0c249ed9 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,13 @@ 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) - } + // Node refs are loaded straight off disk by resolveNodeFromYAML, so they + // need the same containment check as agent refs: without it a workflow + // config can read and instantiate a node from anywhere on the filesystem. var err error - absPath, err = filepath.Abs(targetPath) + absPath, err = resolveConfigReference(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..ee728b14f 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,154 @@ edges: t.Errorf("expected tool output result 'tool_output', got %v", toolOut["result"]) } } + +// newWorkflowDir lays out a workflow directory with an in-directory node config, +// a node config outside the directory, and a symlink inside the directory that +// points at the outside one. It returns the base directory and the workflow +// directory. +func newWorkflowDir(t *testing.T) (base, workflowDir 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 + } + + workflowDir = filepath.Join(base, "agents", "root") + if err := os.MkdirAll(filepath.Join(workflowDir, "nodes"), 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", workflowDir, err) + } + + const joinNode = "name: join_node\nagent_class: JoinNode\n" + if err := os.WriteFile(filepath.Join(workflowDir, "nodes", "join.yaml"), []byte(joinNode), 0o644); err != nil { + t.Fatalf("WriteFile(nodes/join.yaml) failed: %v", err) + } + + outside := filepath.Join(base, "outside.yaml") + if err := os.WriteFile(outside, []byte(joinNode), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", outside, err) + } + if err := os.Symlink(outside, filepath.Join(workflowDir, "link.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + return base, workflowDir +} + +// writeWorkflow writes a workflow config in dir whose single edge targets ref. +func writeWorkflow(t *testing.T, dir, name, ref string) string { + t.Helper() + + yaml := fmt.Sprintf("name: %s\nagent_class: Workflow\nedges:\n - - START\n - %s\n", name, ref) + path := filepath.Join(dir, name+".yaml") + if err := os.WriteFile(path, []byte(yaml), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", path, err) + } + return path +} + +// TestWorkflowNodeReferenceRejectsEscapingPath covers workflow edge refs with the +// same containment rules ResolveAgentReference applies to agent refs. Node refs +// are read straight off disk by resolveNodeFromYAML, so without the check a +// workflow config can instantiate a node from anywhere on the filesystem. +func TestWorkflowNodeReferenceRejectsEscapingPath(t *testing.T) { + base, workflowDir := newWorkflowDir(t) + + tests := []struct { + name string + ref string + wantErr string + }{ + { + name: "absolute path", + ref: filepath.Join(base, "outside.yaml"), + wantErr: "absolute paths are not allowed", + }, + { + name: "parent traversal", + ref: filepath.Join("..", "..", "outside.yaml"), + wantErr: traversalError, + }, + { + name: "symlink escaping the workflow directory", + ref: "link.yaml", + wantErr: traversalError, + }, + } + + for i, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Each case gets its own workflow file so that the resolved-node cache, + // which is keyed by absolute path, cannot mask a later case. + workflowPath := writeWorkflow(t, workflowDir, fmt.Sprintf("wf_%d", i), tc.ref) + + // The reference must be rejected by the containment check itself, not + // merely fail later while the node config is being loaded. + _, err := FromConfig(context.Background(), workflowPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with node ref %q succeeded, want error containing %q", workflowPath, tc.ref, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("FromConfig(_, %q) with node ref %q = %v, want error containing %q", workflowPath, tc.ref, err, tc.wantErr) + } + }) + } +} + +// TestWorkflowNodeReferenceAllowsPathsInsideWorkflowDir guards against the +// containment check rejecting legitimate references, including ones in a +// subdirectory of the workflow's own directory. +func TestWorkflowNodeReferenceAllowsPathsInsideWorkflowDir(t *testing.T) { + _, workflowDir := newWorkflowDir(t) + + for i, ref := range []string{ + filepath.Join("nodes", "join.yaml"), + filepath.Join("nodes", "..", "nodes", "join.yaml"), + } { + t.Run(ref, func(t *testing.T) { + workflowPath := writeWorkflow(t, workflowDir, fmt.Sprintf("wf_ok_%d", i), ref) + + ag, err := FromConfig(context.Background(), workflowPath) + if err != nil { + t.Fatalf("FromConfig(_, %q) with node ref %q failed: %v", workflowPath, ref, err) + } + if ag == nil { + t.Fatal("FromConfig returned a nil agent") + } + }) + } +} + +// TestWorkflowNodeReferenceCacheDoesNotBypassCheck pins the order of the +// containment check against the resolved-node cache. The cache is a package +// global keyed by absolute path, so if it were consulted before the check, a +// node legitimately loaded by one workflow could be served to another workflow +// that must not reach it. +func TestWorkflowNodeReferenceCacheDoesNotBypassCheck(t *testing.T) { + base, workflowDir := newWorkflowDir(t) + + // First, load the node legitimately so that it is in the cache. + okPath := writeWorkflow(t, workflowDir, "wf_cache_ok", filepath.Join("nodes", "join.yaml")) + if _, err := FromConfig(context.Background(), okPath); err != nil { + t.Fatalf("FromConfig(_, %q) failed: %v", okPath, err) + } + + // Now reference that same, already-cached node from a workflow that sits + // outside its directory. It must still be rejected. + otherDir := filepath.Join(base, "other") + if err := os.MkdirAll(otherDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", otherDir, err) + } + escaping := filepath.Join("..", "agents", "root", "nodes", "join.yaml") + escapingPath := writeWorkflow(t, otherDir, "wf_cache_escape", escaping) + + _, err := FromConfig(context.Background(), escapingPath) + if err == nil { + t.Fatalf("FromConfig(_, %q) with cached node ref %q succeeded, want rejection", escapingPath, escaping) + } + if !strings.Contains(err.Error(), traversalError) { + t.Errorf("FromConfig(_, %q) = %v, want error containing %q", escapingPath, err, traversalError) + } +} From 3db8d48561bc8d584e458c47b41d58f08d069794 Mon Sep 17 00:00:00 2001 From: Sasha Shynkaruk Date: Mon, 24 Aug 2026 14:14:32 +0000 Subject: [PATCH 2/6] fix(configurable): refuse symlinked components rather than resolving them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveConfigReference resolved the reference's symlinks and compared the result against the parent directory. EvalSymlinks fails when the target does not exist and the fallback keeps the lexical spelling, so a symlink pointing out of the directory at a target that is not there yet passes as contained. Only the missing target stops the read, and it stops being missing the moment anything creates it. Refuse links instead of following them. A reference must satisfy filepath.IsLocal, which settles the lexical half on every platform, and no component below the resolved parent directory may be a symlink. Refusing does not depend on the link's target existing at the moment of the check, which is what the previous approach relied on. IsLocal also replaces the hand-rolled IsAbs and VolumeName pair. It rejects the same drive-relative and UNC spellings, and additionally the Windows reserved device names — NUL, com1, LPT1, CONIN$ and the trailing-space and trailing-dot variants — none of which VolumeName catches. The component walk covers Windows directory junctions too, since os.Lstat reports IO_REPARSE_TAG_MOUNT_POINT with ModeSymlink set. Rejections are sentinel errors, so the tests identify them with errors.Is rather than by matching the message text. --- internal/configurable/configurable_utils.go | 95 ++++++++++++------- .../configurable/configurable_utils_test.go | 46 ++++----- .../configurable_workflow_test.go | 25 ++--- 3 files changed, 95 insertions(+), 71 deletions(-) diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index 272d174ef..030507679 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -18,7 +18,9 @@ package configurable import ( "context" "encoding/json" + "errors" "fmt" + "io/fs" "os" "os/exec" "path/filepath" @@ -365,43 +367,53 @@ func ResolveCallbackReference(ctx context.Context, callbackName string) (any, er return nil, fmt.Errorf("callback '%s' not found", callbackName) } +// Reasons a config reference can be rejected. They are sentinels so that callers +// and tests can identify the rejection without matching on the message text. +var ( + // errConfigReferenceNotLocal reports a reference that names a file outside + // the referencing config's directory by its spelling alone. + errConfigReferenceNotLocal = errors.New("config reference must be a relative path inside the agent directory") + // errConfigReferenceSymlink reports a reference that reaches its target + // through a symbolic link below that directory. + errConfigReferenceSymlink = errors.New("config reference traverses a symbolic link") +) + // resolveConfigReference turns a config-supplied reference into an absolute path // that is guaranteed to sit inside the referencing config's own directory. // -// Absolute references are rejected outright, and a relative one must resolve -// inside the parent 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. Where a path does not exist the lexical -// path is used, so a missing file still reports as not found rather than as a -// traversal. +// A reference must be local to the parent directory in filepath.IsLocal's sense, +// which settles the lexical half of the question on every platform. The other +// half is symlinks, and they are refused rather than resolved: a link is not +// followed to see where it points, it simply disqualifies the reference. That is +// the stricter of the two rules, and unlike resolving it does not depend on the +// link's target existing at the moment of the check. +// +// The parent directory itself is resolved first, so an agent directory reached +// through a symlink is not penalised for it. Only components below the parent +// are refused. // // Every place that loads a nested YAML config from a reference must route // through here: the check is the trust boundary between the config being loaded // and the rest of the filesystem, and a second copy of it is a second place to // forget. func resolveConfigReference(parentPath, refPath string) (string, error) { - if refPath == "" { - return "", fmt.Errorf("config reference path cannot be empty") - } - - // IsAbs alone is not enough on Windows: a drive-relative reference such as - // `C:node.yaml` is not absolute, yet it escapes the parent directory by - // resolving against the current directory of that drive. VolumeName covers - // that and UNC paths, and is always empty on Unix. - if filepath.IsAbs(refPath) || filepath.VolumeName(refPath) != "" { - return "", fmt.Errorf("absolute paths are not allowed in config references: %s", refPath) + // IsLocal is purely lexical and rejects, in one call, everything that could + // name a file outside the directory the reference is evaluated in: the empty + // string, absolute paths, any ".." that escapes, and on Windows drive-relative + // refs such as `C:node.yaml`, UNC paths and reserved names such as NUL. + if !filepath.IsLocal(refPath) { + return "", fmt.Errorf("%w: %s", errConfigReferenceNotLocal, refPath) } parentDir, err := filepath.Abs(filepath.Dir(parentPath)) if err != nil { return "", fmt.Errorf("failed to resolve agent directory: %w", err) } - // Resolve the parent directory BEFORE joining, so that both sides of the - // containment check are rooted in the same real path. Resolving only one - // side would reject a legitimate reference whenever the parent directory is - // reached through a symlink and the target does not exist yet — the target - // has no symlinks to resolve, so it would keep the unresolved spelling and - // fail the prefix test. + // Resolve the parent directory BEFORE joining, so that the check and the path + // it approves are rooted in the same real path. Resolving only one side would + // reject a legitimate reference whenever the parent directory is reached + // through a symlink: the refusal below would start from a directory the + // reference never names, and disqualify everything inside it. if resolved, err := filepath.EvalSymlinks(parentDir); err == nil { parentDir = resolved } @@ -410,22 +422,39 @@ func resolveConfigReference(parentPath, refPath string) (string, error) { // lexically-normalized target path. absPath := filepath.Join(parentDir, refPath) - // Resolve the target too, so a symlink inside the agent directory cannot be - // used to escape it. Where the target does not exist there is nothing to - // resolve and the lexical path stands, so a missing file reports as not found - // rather than as a traversal. - checkPath := absPath - if resolved, err := filepath.EvalSymlinks(absPath); err == nil { - checkPath = resolved - } - if !strings.HasPrefix(checkPath, parentDir+string(os.PathSeparator)) && checkPath != parentDir { - return "", fmt.Errorf( - "path traversal detected: config reference %q resolves outside agent directory", refPath) + // IsLocal already guarantees the join lands inside parentDir lexically, so the + // only remaining way out is a symlink on the way down. Refusing links is + // stronger than resolving them: a link whose target does not exist yet + // resolves to nothing, and resolving would wave exactly that through. + if err := refuseSymlinkComponents(parentDir, refPath); err != nil { + return "", err } return absPath, nil } +// refuseSymlinkComponents fails if any component of refPath below dir is a +// symbolic link. A component that does not exist cannot be a link, so a missing +// file still reports as not found rather than as a traversal. +func refuseSymlinkComponents(dir, refPath string) error { + cur := dir + // IsLocal guarantees Clean leaves no ".." components to walk through. + for _, part := range strings.Split(filepath.Clean(refPath), string(os.PathSeparator)) { + cur = filepath.Join(cur, part) + fi, err := os.Lstat(cur) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("failed to inspect config reference %q: %w", refPath, err) + } + if fi.Mode()&fs.ModeSymlink != 0 { + return fmt.Errorf("%w: %s", errConfigReferenceSymlink, refPath) + } + } + return nil +} + // ResolveAgentReference builds an agent from a reference config. func ResolveAgentReference(ctx context.Context, parentPath, refPath string) (agent.Agent, error) { absPath, err := resolveConfigReference(parentPath, refPath) diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 50457207c..e93baa871 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -16,13 +16,18 @@ package configurable import ( "context" + "errors" "os" "path/filepath" - "strings" "testing" ) -const traversalError = "path traversal detected" +// isContainmentRejection reports whether err is the containment check refusing a +// reference, as opposed to a later failure to load the config that it names. +// Asserting on the sentinels keeps these tests independent of the message text. +func isContainmentRejection(err error) bool { + return errors.Is(err, errConfigReferenceNotLocal) || errors.Is(err, errConfigReferenceSymlink) +} // 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 @@ -67,22 +72,22 @@ func TestResolveAgentReferenceRejectsEscapingConfigPath(t *testing.T) { tests := []struct { name string refPath string - wantErr string + wantErr error }{ { name: "absolute path", refPath: filepath.Join(string(os.PathSeparator), "etc", "passwd"), - wantErr: "absolute paths are not allowed", + wantErr: errConfigReferenceNotLocal, }, { name: "parent traversal", refPath: filepath.Join("..", "..", "outside.yaml"), - wantErr: traversalError, + wantErr: errConfigReferenceNotLocal, }, { name: "symlink escaping the agent directory", refPath: "link.yaml", - wantErr: traversalError, + wantErr: errConfigReferenceSymlink, }, } @@ -91,11 +96,8 @@ func TestResolveAgentReferenceRejectsEscapingConfigPath(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) + if !errors.Is(err, tc.wantErr) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want %v", parentPath, tc.refPath, err, tc.wantErr) } }) } @@ -109,8 +111,8 @@ 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) + if isContainmentRejection(err) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no containment rejection", parentPath, "sub_agent.yaml", err) } } @@ -133,9 +135,8 @@ func TestResolveAgentReferenceRelativeParentPath(t *testing.T) { } }) - 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", "sub_agent.yaml"); isContainmentRejection(err) { + t.Errorf("ResolveAgentReference with a relative parent path = %v, want no containment rejection", err) } if _, err := ResolveAgentReference(context.Background(), "root_agent.yaml", filepath.Join("..", "..", "outside.yaml")); err == nil { @@ -169,9 +170,8 @@ func TestResolveAgentReferenceSymlinkedParentDir(t *testing.T) { } parentPath := filepath.Join(alias, "root_agent.yaml") - if _, err := ResolveAgentReference(context.Background(), parentPath, "missing.yaml"); err != nil && - strings.Contains(err.Error(), traversalError) { - t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no traversal rejection", parentPath, "missing.yaml", err) + if _, err := ResolveAgentReference(context.Background(), parentPath, "missing.yaml"); isContainmentRejection(err) { + t.Errorf("ResolveAgentReference(_, %q, %q) = %v, want no containment rejection", parentPath, "missing.yaml", err) } // The symlinked parent must not weaken the check itself. @@ -182,9 +182,11 @@ func TestResolveAgentReferenceSymlinkedParentDir(t *testing.T) { // TestResolveConfigReferenceRejectsVolumeQualifiedRefs covers references that // carry a volume name. On Windows a drive-relative reference such as -// `C:node.yaml` is not absolute yet still escapes the parent directory, so -// IsAbs alone is not a sufficient guard. filepath.VolumeName is empty on Unix, -// where these are ordinary (if odd) relative file names. +// `C:node.yaml` is not absolute yet still escapes the parent directory, by +// resolving against the current directory of that drive; filepath.IsLocal is +// what rules it out. VolumeName is empty on Unix, where these three are +// ordinary (if odd) relative file names and are correctly accepted, so the +// assertion only applies where the platform gives them a volume. func TestResolveConfigReferenceRejectsVolumeQualifiedRefs(t *testing.T) { _, parentPath := newAgentDir(t) diff --git a/internal/configurable/configurable_workflow_test.go b/internal/configurable/configurable_workflow_test.go index ee728b14f..50b3ff328 100644 --- a/internal/configurable/configurable_workflow_test.go +++ b/internal/configurable/configurable_workflow_test.go @@ -16,11 +16,11 @@ package configurable import ( "context" + "errors" "fmt" "iter" "os" "path/filepath" - "strings" "testing" "time" @@ -646,22 +646,22 @@ func TestWorkflowNodeReferenceRejectsEscapingPath(t *testing.T) { tests := []struct { name string ref string - wantErr string + wantErr error }{ { name: "absolute path", ref: filepath.Join(base, "outside.yaml"), - wantErr: "absolute paths are not allowed", + wantErr: errConfigReferenceNotLocal, }, { name: "parent traversal", ref: filepath.Join("..", "..", "outside.yaml"), - wantErr: traversalError, + wantErr: errConfigReferenceNotLocal, }, { name: "symlink escaping the workflow directory", ref: "link.yaml", - wantErr: traversalError, + wantErr: errConfigReferenceSymlink, }, } @@ -674,11 +674,8 @@ func TestWorkflowNodeReferenceRejectsEscapingPath(t *testing.T) { // The reference must be rejected by the containment check itself, not // merely fail later while the node config is being loaded. _, err := FromConfig(context.Background(), workflowPath) - if err == nil { - t.Fatalf("FromConfig(_, %q) with node ref %q succeeded, want error containing %q", workflowPath, tc.ref, tc.wantErr) - } - if !strings.Contains(err.Error(), tc.wantErr) { - t.Errorf("FromConfig(_, %q) with node ref %q = %v, want error containing %q", workflowPath, tc.ref, err, tc.wantErr) + if !errors.Is(err, tc.wantErr) { + t.Errorf("FromConfig(_, %q) with node ref %q = %v, want %v", workflowPath, tc.ref, err, tc.wantErr) } }) } @@ -731,11 +728,7 @@ func TestWorkflowNodeReferenceCacheDoesNotBypassCheck(t *testing.T) { escaping := filepath.Join("..", "agents", "root", "nodes", "join.yaml") escapingPath := writeWorkflow(t, otherDir, "wf_cache_escape", escaping) - _, err := FromConfig(context.Background(), escapingPath) - if err == nil { - t.Fatalf("FromConfig(_, %q) with cached node ref %q succeeded, want rejection", escapingPath, escaping) - } - if !strings.Contains(err.Error(), traversalError) { - t.Errorf("FromConfig(_, %q) = %v, want error containing %q", escapingPath, err, traversalError) + if _, err := FromConfig(context.Background(), escapingPath); !errors.Is(err, errConfigReferenceNotLocal) { + t.Errorf("FromConfig(_, %q) with cached node ref %q = %v, want %v", escapingPath, escaping, err, errConfigReferenceNotLocal) } } From 66486839457d639e28be9abd81aec7f45231adb4 Mon Sep 17 00:00:00 2001 From: Anton Dziatkovskii <194927794+tonydzi@users.noreply.github.com> Date: Tue, 25 Aug 2026 10:20:53 +0100 Subject: [PATCH 3/6] fix(configurable): restore the strings import the merge dropped 34896ab merged main into this branch and took the import block from this side, which no longer imports strings, while keeping the test main added on the other side. That test still calls strings.Contains, so the package does not compile and no CI job on this PR can report anything but a build failure. Verified by building the test binary for linux/amd64, which is what every job in go.yml and nightly.yml runs on. Assisted-by: Claude (Anthropic) / claude-opus-5 Machine: A-2022BAYAREA Account: a Operator: robot:git-s7-fast --- internal/configurable/configurable_utils_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 555920310..f4bb19198 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -19,6 +19,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" ) From 66adbff20190c071b04227005c049a5af5137a20 Mon Sep 17 00:00:00 2001 From: Anton Dziatkovskii <194927794+tonydzi@users.noreply.github.com> Date: Tue, 25 Aug 2026 10:21:09 +0100 Subject: [PATCH 4/6] fix(configurable): refuse junctions, not just symlinks, in the component walk The walk tests os.Lstat's ModeSymlink, and the commit that introduced it says that covers Windows directory junctions because Lstat reports IO_REPARSE_TAG_MOUNT_POINT with ModeSymlink set. That was true until Go 1.23. Since then, and by default under godebug winsymlink=1, only IO_REPARSE_TAG_SYMLINK sets ModeSymlink and every other reparse tag falls through to ModeIrregular: os/types_windows.go mode(), with the old behaviour kept beside it in modePreGo1_23. go.mod declares go 1.26, so this module gets the newer mapping. Measured on windows/amd64, go1.26.7: Lstat of a junction returns ?rw-rw-rw-, ModeSymlink false and ModeIrregular true, and a junction placed inside the agent directory pointing out of it is accepted as contained -- with the target present and, which is the shape this check exists for, with the target still missing. A junction needs no elevation to create, unlike a symlink, so it is the cheaper of the two to arrange. Refusing ModeIrregular as well keeps the rule the same for every reparse tag rather than for one of them. The predicate is split out so it can be asserted on every platform, since the junction test needs Windows to build one and this project has no Windows CI to build it on. Assisted-by: Claude (Anthropic) / claude-opus-5 Machine: A-2022BAYAREA Account: a Operator: robot:git-s7-fast --- internal/configurable/configurable_utils.go | 33 ++++++-- .../configurable/configurable_utils_test.go | 25 ++++++ .../configurable_utils_windows_test.go | 83 +++++++++++++++++++ 3 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 internal/configurable/configurable_utils_windows_test.go diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index 90016d7d1..34372ed05 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -382,8 +382,9 @@ var ( // the referencing config's directory by its spelling alone. errConfigReferenceNotLocal = errors.New("config reference must be a relative path inside the agent directory") // errConfigReferenceSymlink reports a reference that reaches its target - // through a symbolic link below that directory. - errConfigReferenceSymlink = errors.New("config reference traverses a symbolic link") + // through a symbolic link, or through any other reparse point, below that + // directory. + errConfigReferenceSymlink = errors.New("config reference traverses a link") ) // resolveConfigReference turns a config-supplied reference into an absolute path @@ -441,9 +442,29 @@ func resolveConfigReference(parentPath, refPath string) (string, error) { return absPath, nil } -// refuseSymlinkComponents fails if any component of refPath below dir is a -// symbolic link. A component that does not exist cannot be a link, so a missing -// file still reports as not found rather than as a traversal. +// isLinkLike reports whether a component may redirect the read somewhere other +// than where its own name sits in the tree. +// +// ModeSymlink alone is not enough on Windows. A directory junction is a reparse +// point with tag IO_REPARSE_TAG_MOUNT_POINT, and since Go 1.23 (godebug +// winsymlink=1) os.Lstat reports it as ModeIrregular, not ModeSymlink: see +// os/types_windows.go, where only IO_REPARSE_TAG_SYMLINK sets ModeSymlink and +// every other tag falls through to ModeIrregular. Before Go 1.23 mount points +// did carry ModeSymlink, which is why testing for it alone looks sufficient. +// This module declares go 1.26, so it gets the newer mapping and a junction +// would walk straight past a ModeSymlink-only test. +// +// Refusing ModeIrregular as well keeps the rule "do not follow a redirection, +// refuse it" true for every reparse tag rather than for one of them. +func isLinkLike(mode fs.FileMode) bool { + return mode&(fs.ModeSymlink|fs.ModeIrregular) != 0 +} + +// refuseSymlinkComponents fails if any component of refPath below dir can +// redirect the read out of dir: a symbolic link on any platform, or a junction +// or other reparse point on Windows. A component that does not exist cannot be +// a link, so a missing file still reports as not found rather than as a +// traversal. func refuseSymlinkComponents(dir, refPath string) error { cur := dir // IsLocal guarantees Clean leaves no ".." components to walk through. @@ -456,7 +477,7 @@ func refuseSymlinkComponents(dir, refPath string) error { if err != nil { return fmt.Errorf("failed to inspect config reference %q: %w", refPath, err) } - if fi.Mode()&fs.ModeSymlink != 0 { + if isLinkLike(fi.Mode()) { return fmt.Errorf("%w: %s", errConfigReferenceSymlink, refPath) } } diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index f4bb19198..4df4f6f7f 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -17,6 +17,7 @@ package configurable import ( "context" "errors" + "io/fs" "os" "path/filepath" "strings" @@ -30,6 +31,30 @@ func isContainmentRejection(err error) bool { return errors.Is(err, errConfigReferenceNotLocal) || errors.Is(err, errConfigReferenceSymlink) } +// TestIsLinkLike pins the mode predicate the component walk uses. It runs +// everywhere, unlike the junction test, which needs Windows to build one. +func TestIsLinkLike(t *testing.T) { + for _, tc := range []struct { + name string + mode fs.FileMode + want bool + }{ + {name: "regular file", mode: 0o644, want: false}, + {name: "directory", mode: fs.ModeDir | 0o755, want: false}, + {name: "symlink", mode: fs.ModeSymlink | 0o777, want: true}, + // A Windows directory junction: os.Lstat reports a reparse point whose + // tag is not IO_REPARSE_TAG_SYMLINK as irregular, so a ModeSymlink-only + // test would walk past it. + {name: "junction or other reparse point", mode: fs.ModeIrregular | 0o666, want: true}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := isLinkLike(tc.mode); got != tc.want { + t.Errorf("isLinkLike(%v) = %v, want %v", tc.mode, got, tc.want) + } + }) + } +} + // 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. diff --git a/internal/configurable/configurable_utils_windows_test.go b/internal/configurable/configurable_utils_windows_test.go new file mode 100644 index 000000000..827606284 --- /dev/null +++ b/internal/configurable/configurable_utils_windows_test.go @@ -0,0 +1,83 @@ +// 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. + +//go:build windows + +package configurable + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// TestResolveConfigReferenceRefusesJunction covers the Windows-only shape of the +// containment check: a directory junction is a reparse point that redirects the +// read like a symlink does, but since Go 1.23 os.Lstat reports it as +// ModeIrregular rather than ModeSymlink, so a ModeSymlink-only test walks past +// it. A junction needs no elevation to create, unlike a symlink, so this is the +// cheaper escape of the two for a caller to arrange. +// +// The target is left missing in one case on purpose: that is the shape the +// containment check exists for, since a link whose target does not exist yet +// resolves to nothing and stops being missing the moment anything creates it. +func TestResolveConfigReferenceRefusesJunction(t *testing.T) { + for _, tc := range []struct { + name string + createTarget bool + refPath string + }{ + {name: "target exists", createTarget: true, refPath: `escape\outside.yaml`}, + {name: "target does not exist yet", createTarget: false, refPath: `escape\outside.yaml`}, + } { + t.Run(tc.name, func(t *testing.T) { + base := t.TempDir() + if resolved, err := filepath.EvalSymlinks(base); err == nil { + base = resolved + } + agentDir := filepath.Join(base, "agent") + outsideDir := filepath.Join(base, "outside") + for _, dir := range []string{agentDir, outsideDir} { + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatalf("Mkdir(%s) = %v, want no error", dir, err) + } + } + parentPath := filepath.Join(agentDir, "agent.yaml") + if err := os.WriteFile(parentPath, []byte("name: parent\n"), 0o644); err != nil { + t.Fatalf("WriteFile(parent) = %v, want no error", err) + } + if tc.createTarget { + outside := filepath.Join(outsideDir, "outside.yaml") + if err := os.WriteFile(outside, []byte("name: outside\n"), 0o644); err != nil { + t.Fatalf("WriteFile(outside) = %v, want no error", err) + } + } + + junction := filepath.Join(agentDir, "escape") + if out, err := exec.Command("cmd", "/c", "mklink", "/J", junction, outsideDir).CombinedOutput(); err != nil { + t.Skipf("cannot create a directory junction here: %v: %s", err, out) + } + + got, err := resolveConfigReference(parentPath, tc.refPath) + if err == nil { + t.Fatalf("resolveConfigReference(%q) = %q, want a rejection: the junction leads outside %s", + tc.refPath, got, agentDir) + } + if !isContainmentRejection(err) { + t.Errorf("resolveConfigReference(%q) = %v, want a containment rejection sentinel", tc.refPath, err) + } + }) + } +} From 99f777e54a30171e2bbcd9f8527dcf84f996f34c Mon Sep 17 00:00:00 2001 From: Sasha Shynkaruk Date: Tue, 1 Sep 2026 12:47:31 +0000 Subject: [PATCH 5/6] fix(configurable): pin the containment semantics and correct its diagnostics Review found that the one behaviour this change deliberately chose was the one behaviour nothing asserted. Both os.Symlink calls in the package tests build links pointing outside the config directory, so an implementation that went back to resolving links and refusing only escaping ones passed the whole suite unchanged -- carrying with it the dangling-target hole that motivated refusing links in the first place. Pin it from both sides, and correct three places where the code says more or less than it does. - TestResolveConfigReferenceRefusesLinksThatStayInside asserts that a link wholly inside the config directory is refused for being a link, in its minimal form and in the Kubernetes projected-volume layout the kubelet materialises for a ConfigMap. That layout is the cost of the choice and is now visible in the suite rather than only in a review comment. - TestResolveConfigReferenceCanonicalizesRegistryKey pins the EvalSymlinks call on the parent directory. Deleting that call changes no verdict -- os.Lstat follows every component but the last, so the two sides of the walk cannot end up rooted differently -- and the whole suite passed without it. What it does is canonicalise the path callers use as the agentRegistry and nodeRegistry key, so a directory and a symlink to it share one entry instead of each building its own copy of the same agent. Its comment described the hazard of the earlier prefix-compare design. - A reference below a component that is not a directory reported as an inspection failure rather than as either rejection sentinel, giving callers a third class of error for a reference that simply cannot exist. ENOTDIR is now treated as ErrNotExist is, and left for the caller's own read to report. Note that ENOTDIR does not satisfy fs.ErrNotExist, unlike ENOENT, so it needs naming explicitly. - An empty reference lost its diagnostic when IsLocal replaced the hand-rolled checks: it rendered as the generic message with a dangling ": " and no path. An empty reference is almost always an unfilled template, so it gets its own message again, under the same sentinel. - TestResolveConfigReferenceVolumeQualifiedRefs asserted nothing on Unix, where VolumeName is empty for all three inputs and every iteration hit the continue before the assertion. With no Windows job in CI it therefore asserted nothing anywhere. The expectation is now platform-dependent rather than skipped: refused where the spelling carries a volume, accepted where it is an ordinary if odd file name. Two comments overclaimed. refuseSymlinkComponents said it refuses components that can redirect the read out of the directory; it refuses all links regardless of target, and catches neither hard links nor FIFOs, so it now says which boundary it draws and names what falls outside it. isLinkLike said every reparse tag other than IO_REPARSE_TAG_SYMLINK falls through to ModeIrregular; two do not, as IO_REPARSE_TAG_AF_UNIX maps to ModeSocket and IO_REPARSE_TAG_DEDUP is reported as an ordinary file. Neither redirects a read out of the directory, so the junction conclusion stands. Each new test was killed by a mutant rather than only observed passing. Removing the EvalSymlinks call fails the registry-key test and nothing else; reverting the walk to resolve-and-refuse-only-escaping fails the stays-inside test and nothing else; removing the empty-reference branch, removing the ENOTDIR clause, and over-rejecting a colon in a reference each fail their own test and no other. Hard links are left alone, as reviewers agreed: link counts are not portable on FileInfo, the realistic vector is archive extraction, and the behaviour is unchanged from the merge-base. Assisted-by: Claude Opus 5 Agent-session: ses_e5fa3107d7affe0SsJobIWTTb6 --- internal/configurable/configurable_utils.go | 86 +++++--- .../configurable/configurable_utils_test.go | 203 ++++++++++++++++-- 2 files changed, 251 insertions(+), 38 deletions(-) diff --git a/internal/configurable/configurable_utils.go b/internal/configurable/configurable_utils.go index 34372ed05..4a5e7b1d2 100644 --- a/internal/configurable/configurable_utils.go +++ b/internal/configurable/configurable_utils.go @@ -26,6 +26,7 @@ import ( "path/filepath" "strings" "sync" + "syscall" "github.com/modelcontextprotocol/go-sdk/mcp" "gopkg.in/yaml.v3" @@ -397,19 +398,34 @@ var ( // the stricter of the two rules, and unlike resolving it does not depend on the // link's target existing at the moment of the check. // -// The parent directory itself is resolved first, so an agent directory reached -// through a symlink is not penalised for it. Only components below the parent -// are refused. +// Refusing rather than resolving means a link that stays inside the directory +// is refused too. That is a deliberate trade with a real cost: a config +// directory materialised entirely out of links — a Kubernetes ConfigMap or +// projected volume, a Bazel runfiles forest, a Nix store path — cannot use +// nested references at all, and has to be staged into a directory of real files +// first. The alternative, resolving each link and re-testing containment, cannot +// be made safe: a link pointing outside at a target that does not exist yet +// resolves to nothing and passes the test, and it stops being missing the moment +// anything creates the target. +// +// Only components below the parent directory are refused. The parent itself may +// be reached through any number of links. // // Every place that loads a nested YAML config from a reference must route // through here: the check is the trust boundary between the config being loaded // and the rest of the filesystem, and a second copy of it is a second place to // forget. func resolveConfigReference(parentPath, refPath string) (string, error) { + // IsLocal rejects the empty string along with the escaping spellings, but an + // empty reference is almost always an unfilled template rather than an + // attempt to escape, and the generic message renders it as a dangling ": ". + if refPath == "" { + return "", fmt.Errorf("%w: reference is empty", errConfigReferenceNotLocal) + } // IsLocal is purely lexical and rejects, in one call, everything that could - // name a file outside the directory the reference is evaluated in: the empty - // string, absolute paths, any ".." that escapes, and on Windows drive-relative - // refs such as `C:node.yaml`, UNC paths and reserved names such as NUL. + // name a file outside the directory the reference is evaluated in: absolute + // paths, any ".." that escapes, and on Windows drive-relative refs such as + // `C:node.yaml`, UNC paths and reserved names such as NUL. if !filepath.IsLocal(refPath) { return "", fmt.Errorf("%w: %s", errConfigReferenceNotLocal, refPath) } @@ -418,11 +434,15 @@ func resolveConfigReference(parentPath, refPath string) (string, error) { if err != nil { return "", fmt.Errorf("failed to resolve agent directory: %w", err) } - // Resolve the parent directory BEFORE joining, so that the check and the path - // it approves are rooted in the same real path. Resolving only one side would - // reject a legitimate reference whenever the parent directory is reached - // through a symlink: the refusal below would start from a directory the - // reference never names, and disqualify everything inside it. + // Canonicalise the parent directory before joining, so that the path returned + // from here is the real one. Callers use it as the key of agentRegistry and + // nodeRegistry, and without this two spellings of one directory — the real + // path and a symlink to it — would each get their own cache entry and each + // build their own copy of the same agent. + // + // It is not what makes the containment check correct. The component walk + // below relies on os.Lstat, which follows every component except the last, so + // the two sides cannot end up rooted differently whether or not this runs. if resolved, err := filepath.EvalSymlinks(parentDir); err == nil { parentDir = resolved } @@ -448,30 +468,48 @@ func resolveConfigReference(parentPath, refPath string) (string, error) { // ModeSymlink alone is not enough on Windows. A directory junction is a reparse // point with tag IO_REPARSE_TAG_MOUNT_POINT, and since Go 1.23 (godebug // winsymlink=1) os.Lstat reports it as ModeIrregular, not ModeSymlink: see -// os/types_windows.go, where only IO_REPARSE_TAG_SYMLINK sets ModeSymlink and -// every other tag falls through to ModeIrregular. Before Go 1.23 mount points -// did carry ModeSymlink, which is why testing for it alone looks sufficient. -// This module declares go 1.26, so it gets the newer mapping and a junction -// would walk straight past a ModeSymlink-only test. +// os/types_windows.go, where IO_REPARSE_TAG_SYMLINK sets ModeSymlink and mount +// points fall through to ModeIrregular. Before Go 1.23 mount points did carry +// ModeSymlink, which is why testing for it alone looks sufficient. This module +// declares go 1.26, so it gets the newer mapping and a junction would walk +// straight past a ModeSymlink-only test. // -// Refusing ModeIrregular as well keeps the rule "do not follow a redirection, -// refuse it" true for every reparse tag rather than for one of them. +// ModeIrregular is a broad term: it covers reparse tags that do not redirect +// anywhere, such as cloud-provider placeholder files, and those are refused +// along with the rest. It is not universal either — IO_REPARSE_TAG_AF_UNIX maps +// to ModeSocket and IO_REPARSE_TAG_DEDUP is reported as an ordinary file — but +// neither of those redirects a read out of the directory, so neither matters +// here. func isLinkLike(mode fs.FileMode) bool { return mode&(fs.ModeSymlink|fs.ModeIrregular) != 0 } -// refuseSymlinkComponents fails if any component of refPath below dir can -// redirect the read out of dir: a symbolic link on any platform, or a junction -// or other reparse point on Windows. A component that does not exist cannot be -// a link, so a missing file still reports as not found rather than as a -// traversal. +// refuseSymlinkComponents fails if any component of refPath below dir is a +// link: a symbolic link on any platform, or a junction or other reparse point +// on Windows. Where the link points is not consulted, so one that stays inside +// dir is refused too — see resolveConfigReference for why the check is drawn +// that way. +// +// The boundary is narrower than "cannot reach outside dir" in two directions. A +// hard link inside dir to a file outside it is indistinguishable from a regular +// file here, and defending against it belongs wherever the directory is +// populated, typically archive extraction. A FIFO or device node is likewise +// left alone, since it redirects nothing. +// +// A component that cannot exist cannot be a link, so a reference naming a +// missing file, or one below a component that is not a directory, is left for +// the caller's own read to report as not found. func refuseSymlinkComponents(dir, refPath string) error { cur := dir // IsLocal guarantees Clean leaves no ".." components to walk through. for _, part := range strings.Split(filepath.Clean(refPath), string(os.PathSeparator)) { cur = filepath.Join(cur, part) fi, err := os.Lstat(cur) - if errors.Is(err, fs.ErrNotExist) { + // ENOTDIR is the same situation as ErrNotExist for this check: nothing can + // exist below a component that is not a directory, so there is no link to + // find. Reporting it as an inspection failure would give callers a third + // class of error to handle for a reference that is simply not there. + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) { return nil } if err != nil { diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 4df4f6f7f..7534980c6 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -17,6 +17,7 @@ package configurable import ( "context" "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -269,24 +270,198 @@ func TestResolveAgentReferenceSymlinkedParentDir(t *testing.T) { } } -// TestResolveConfigReferenceRejectsVolumeQualifiedRefs covers references that -// carry a volume name. On Windows a drive-relative reference such as -// `C:node.yaml` is not absolute yet still escapes the parent directory, by -// resolving against the current directory of that drive; filepath.IsLocal is -// what rules it out. VolumeName is empty on Unix, where these three are -// ordinary (if odd) relative file names and are correctly accepted, so the -// assertion only applies where the platform gives them a volume. -func TestResolveConfigReferenceRejectsVolumeQualifiedRefs(t *testing.T) { +// TestResolveConfigReferenceVolumeQualifiedRefs covers references that carry a +// volume name. On Windows a drive-relative reference such as `C:node.yaml` is +// not absolute yet still escapes the parent directory, by resolving against the +// current directory of that drive, and filepath.IsLocal is what rules it out. +// +// The expectation is platform-dependent rather than skipped, so that the test +// asserts something everywhere: on Unix these are ordinary, if odd, relative +// file names, and refusing them would be over-rejection. +func TestResolveConfigReferenceVolumeQualifiedRefs(t *testing.T) { _, parentPath := newAgentDir(t) for _, refPath := range []string{`C:node.yaml`, `D:\escaped.yaml`, `\\host\share\escaped.yaml`} { - _, err := resolveConfigReference(parentPath, refPath) - if filepath.VolumeName(refPath) == "" { - // Not volume-qualified on this platform; nothing to assert. - continue + t.Run(refPath, func(t *testing.T) { + // Volume-qualified on this platform means the reference escapes and must + // be refused; otherwise it is a legal file name and must be accepted. + wantRejected := filepath.VolumeName(refPath) != "" + + _, err := resolveConfigReference(parentPath, refPath) + if gotRejected := err != nil; gotRejected != wantRejected { + t.Errorf("resolveConfigReference(%q, %q) rejected = %v (%v), want rejected = %v", + parentPath, refPath, gotRejected, err, wantRejected) + } + }) + } +} + +// TestResolveConfigReferenceRefusesLinksThatStayInside pins the one behaviour +// this check deliberately chose: a link is refused for being a link, not for +// where it points, so a link wholly inside the config directory is refused too. +// +// Without this, an implementation that went back to resolving links and +// refusing only those that leave the directory would pass the rest of the suite +// unchanged — and it would carry the hole that motivated refusing them, since a +// link pointing outside at a target that does not exist yet resolves to nothing +// and passes containment. +// +// The projected-volume case is the cost of that choice, and it is the reason +// this is a trade rather than a free win: the kubelet materialises a Kubernetes +// ConfigMap as a `..data` symlink to a timestamped directory plus one symlink +// per key, so every reference in such a mount is refused. Bazel runfiles +// forests and Nix store paths have the same shape. +func TestResolveConfigReferenceRefusesLinksThatStayInside(t *testing.T) { + const nodeYAML = "name: join_node\nagent_class: JoinNode\n" + + for _, tc := range []struct { + name string + // layout populates dir and returns the reference to resolve against + // dir/root_agent.yaml. + layout func(t *testing.T, dir string) string + }{ + { + name: "alias to a sibling in the same directory", + layout: func(t *testing.T, dir string) string { + target := filepath.Join(dir, "nodes", "join.yaml") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", filepath.Dir(target), err) + } + if err := os.WriteFile(target, []byte(nodeYAML), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", target, err) + } + if err := os.Symlink(target, filepath.Join(dir, "alias.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + return "alias.yaml" + }, + }, + { + name: "kubernetes projected volume layout", + layout: func(t *testing.T, dir string) string { + data := filepath.Join(dir, "..2026_09_01_10_00_00.123456") + if err := os.MkdirAll(data, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", data, err) + } + if err := os.WriteFile(filepath.Join(data, "join.yaml"), []byte(nodeYAML), 0o644); err != nil { + t.Fatalf("WriteFile(join.yaml) failed: %v", err) + } + if err := os.Symlink(data, filepath.Join(dir, "..data")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + if err := os.Symlink(filepath.Join("..data", "join.yaml"), filepath.Join(dir, "join.yaml")); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + return "join.yaml" + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if resolved, err := filepath.EvalSymlinks(dir); err == nil { + dir = resolved + } + refPath := tc.layout(t, dir) + + parentPath := filepath.Join(dir, "root_agent.yaml") + if _, err := resolveConfigReference(parentPath, refPath); !errors.Is(err, errConfigReferenceSymlink) { + t.Errorf("resolveConfigReference(%q, %q) = %v, want %v: a link inside the directory is refused for being a link", + parentPath, refPath, err, errConfigReferenceSymlink) + } + }) + } +} + +// TestResolveConfigReferenceCanonicalizesRegistryKey pins the EvalSymlinks call +// on the parent directory. That call is not what makes the containment check +// correct — the component walk uses os.Lstat, which follows every component but +// the last, so removing it changes no verdict. What it does is canonicalise the +// path returned from here, which callers use as the agentRegistry and +// nodeRegistry key. Without it, a directory and a symlink to that directory get +// separate cache entries and each builds its own copy of the same agent. +func TestResolveConfigReferenceCanonicalizesRegistryKey(t *testing.T) { + base := t.TempDir() + if resolved, err := filepath.EvalSymlinks(base); err == nil { + base = resolved + } + + realDir := filepath.Join(base, "real") + if err := os.MkdirAll(realDir, 0o755); err != nil { + t.Fatalf("MkdirAll(%q) failed: %v", realDir, err) + } + for _, name := range []string{"root_agent.yaml", "sub_agent.yaml"} { + cfg := fmt.Sprintf("name: %s\nagent_class: LlmAgent\nmodel: gemini-2.0-flash\n", strings.TrimSuffix(name, ".yaml")) + if err := os.WriteFile(filepath.Join(realDir, name), []byte(cfg), 0o644); err != nil { + t.Fatalf("WriteFile(%q) failed: %v", name, err) } - if err == nil { - t.Errorf("resolveConfigReference(%q, %q) succeeded, want rejection", parentPath, refPath) + } + + aliasDir := filepath.Join(base, "alias") + if err := os.Symlink(realDir, aliasDir); err != nil { + t.Skipf("symlinks are not supported in this environment: %v", err) + } + + realKey, err := resolveConfigReference(filepath.Join(realDir, "root_agent.yaml"), "sub_agent.yaml") + if err != nil { + t.Fatalf("resolveConfigReference through the real directory failed: %v", err) + } + aliasKey, err := resolveConfigReference(filepath.Join(aliasDir, "root_agent.yaml"), "sub_agent.yaml") + if err != nil { + t.Fatalf("resolveConfigReference through the symlinked directory failed: %v", err) + } + if realKey != aliasKey { + t.Errorf("resolveConfigReference returned %q through the real directory and %q through a symlink to it, want one key", + realKey, aliasKey) + } + + // The consequence the key exists for: resolving through both spellings must + // leave one registry entry, not one per spelling. + for _, parentDir := range []string{realDir, aliasDir} { + if _, err := ResolveAgentReference(context.Background(), filepath.Join(parentDir, "root_agent.yaml"), "sub_agent.yaml"); err != nil { + t.Fatalf("ResolveAgentReference through %q failed: %v", parentDir, err) } } + registryMu.RLock() + defer registryMu.RUnlock() + if _, ok := agentRegistry[filepath.Join(aliasDir, "sub_agent.yaml")]; ok { + t.Errorf("agentRegistry holds a separate entry keyed by the symlinked spelling %q, want only %q", + filepath.Join(aliasDir, "sub_agent.yaml"), realKey) + } + if _, ok := agentRegistry[realKey]; !ok { + t.Errorf("agentRegistry has no entry keyed by the canonical path %q", realKey) + } +} + +// TestResolveConfigReferenceBelowNonDirectory covers a reference whose +// intermediate component is a regular file. Nothing can exist below it, so +// there is no link to find and no reason to report an inspection failure: the +// reference is simply not there, and the caller's own read says so. Reporting +// it separately would give callers a third class of error to distinguish from +// "refused" and "loaded". +func TestResolveConfigReferenceBelowNonDirectory(t *testing.T) { + _, parentPath := newAgentDir(t) + + // sub_agent.yaml is a regular file, so sub_agent.yaml/nested.yaml cannot exist. + refPath := filepath.Join("sub_agent.yaml", "nested.yaml") + if _, err := resolveConfigReference(parentPath, refPath); err != nil { + t.Errorf("resolveConfigReference(%q, %q) = %v, want no error: the reference cannot exist, which is the caller's read to report", + parentPath, refPath, err) + } +} + +// TestResolveConfigReferenceEmptyRef covers the empty reference, which is +// usually an unfilled template rather than an attempt to escape. IsLocal +// rejects it along with the escaping spellings, so it needs its own message to +// avoid rendering as a bare "config reference must be ...: " with nothing after +// the colon. +func TestResolveConfigReferenceEmptyRef(t *testing.T) { + _, parentPath := newAgentDir(t) + + _, err := resolveConfigReference(parentPath, "") + if !errors.Is(err, errConfigReferenceNotLocal) { + t.Fatalf("resolveConfigReference(%q, \"\") = %v, want %v", parentPath, err, errConfigReferenceNotLocal) + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("resolveConfigReference(%q, \"\") = %q, want the message to name the reference as empty", parentPath, err) + } } From 0b63fc1dd7212002db78ebbdb7f36ebac604dd60 Mon Sep 17 00:00:00 2001 From: Sasha Shynkaruk Date: Wed, 2 Sep 2026 10:37:19 +0000 Subject: [PATCH 6/6] test(configurable): drop the API-key dependency from the registry-key test TestResolveConfigReferenceCanonicalizesRegistryKey wrote its fixture configs as LlmAgent with a model, so ResolveAgentReference built a genai client and failed with "api key is required for Google AI backend" wherever no key was set. Reproduced on linux, darwin and windows. CI defines no API key secret, so the Go workflow would have gone red on it as soon as the fork run was approved. Worse than the red line: execution stopped at the Fatalf before the two registry assertions, so the half of the test that pins the consequence the canonical key exists for never ran in a keyless environment. SequentialAgent is registered alongside LlmAgent and needs no model. With it, removing the EvalSymlinks call now fails the test twice, on the key equality and on the registry entry, instead of once. Reported by @tonydzi from a run on windows/amd64, go1.26.7. --- internal/configurable/configurable_utils_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/configurable/configurable_utils_test.go b/internal/configurable/configurable_utils_test.go index 7534980c6..c912766b8 100644 --- a/internal/configurable/configurable_utils_test.go +++ b/internal/configurable/configurable_utils_test.go @@ -389,8 +389,12 @@ func TestResolveConfigReferenceCanonicalizesRegistryKey(t *testing.T) { if err := os.MkdirAll(realDir, 0o755); err != nil { t.Fatalf("MkdirAll(%q) failed: %v", realDir, err) } + // SequentialAgent, not LlmAgent: an LlmAgent needs a model, and building one + // constructs a genai client, so the ResolveAgentReference half of this test + // stops at the Fatalf below in any environment without an API key. CI + // defines no such secret, so that half would never run there. for _, name := range []string{"root_agent.yaml", "sub_agent.yaml"} { - cfg := fmt.Sprintf("name: %s\nagent_class: LlmAgent\nmodel: gemini-2.0-flash\n", strings.TrimSuffix(name, ".yaml")) + cfg := fmt.Sprintf("name: %s\nagent_class: SequentialAgent\n", strings.TrimSuffix(name, ".yaml")) if err := os.WriteFile(filepath.Join(realDir, name), []byte(cfg), 0o644); err != nil { t.Fatalf("WriteFile(%q) failed: %v", name, err) }