fix(configurable): apply config_path containment to workflow node references - #1305
fix(configurable): apply config_path containment to workflow node references#1305jjsasha63 wants to merge 4 commits into
Conversation
jjsasha63
changed the title
…erences #878 added a containment check to ResolveAgentReference, but the workflow loader resolves node references on a separate path that never reaches it. resolveNodeLike accepted an absolute reference and joined a relative one without validating the result, so a workflow config could name any .yaml or .yml file on disk. The FunctionNode, JoinNode and ToolNode branches of resolveNodeFromYAML all return before its fall-through call to ResolveAgentReference, so for those node classes an out-of-tree file was read, parsed and instantiated with no containment check at all. Only the agent branch was covered, and by the time it ran the read had already happened. Extract the check into resolveContainedConfigPath and call it from both resolvers, before the file is read and before the node cache is consulted. One shared copy is the point: two independent resolvers is how the gap appeared in the first place. BREAKING: an absolute workflow node reference is no longer accepted, matching the config_path behaviour #878 established.
Resolve the agent directory once instead of deriving it twice, compare with filepath.Rel rather than a separator-terminated string prefix, and move the symlink resolution into a helper that documents why a failure falls back to the cleaned path. No behaviour change. Adds a case for a sibling directory whose name begins with the agent directory's name, which is what the prefix comparison was guarding against.
690e9a2 to
b9f70bf
Compare
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
The containment gap here is real and worth closing. On main, a workflow config
naming ../../outside/fn.yaml doesn't just error — it loads and instantiates the
node, and that holds for FunctionNode, JoinNode and ToolNode alike. Sharing
one resolveContainedConfigPath between both resolvers is the right shape for
the fix.
Four things I'd like to see resolved.
A dangling symlink pointing outside is still accepted as contained.
realPath
returns early only when EvalSymlinks succeeds. When it fails, the fallback
splits the path and keeps the final component exactly as written, so a symlink
whose target does not exist yet is never Readlinked and the function hands back
the link's own in-tree spelling. filepath.Rel then finds it contained:
dangling link -> path=".../agents/root/link.yaml" err=<nil>
same link, target exists -> err=path traversal detected
Reading the approved path afterwards follows the link out of the tree. This is
inherited from the old ResolveAgentReference, which fell back to the same
lexical path when EvalSymlinks failed, so it isn't a regression. It matters
here because the doc comment on realPath says the fallback "keeps a symlinked
parent directory honest", and this is the one case where it doesn't.
The two tests named for that case don't exercise it.
TestResolveAgentReferenceRejectsThroughDanglingSymlinkedDir
and its workflow twin at
configurable_workflow_test.go:757
both MkdirAll the symlink target before creating the link, so EvalSymlinks
resolves the dirlink component and only the final missing.yaml is absent. A
genuinely dangling link is never covered, which is why the suite stays green.
The adk-python equivalence doesn't hold. The comment at
L377-L381
says this mirrors resolve_agent_reference. Python's os.path.join doesn't
normalise and realpath resolves components in order, so it walks
dirlink/../sub_agent.yaml through the symlink and rejects it. filepath.Join
cleans .. lexically first, so Go resolves it as a sibling of the link and loads
a different file. Go's direction is the stricter one and nothing escapes through
it, but the doc comment and the test comment citing "adk-python's
normpath-then-realpath order" both describe an ordering that isn't there.
Two behaviour changes beyond the declared one. The BREAKING note covers
absolute node references. Also changing:
- Relative references that leave the directory.
../common/join.yamlloaded
before and is rejected now. Unavoidable given the fix, worth naming in the note
so anyone using a shared sibling directory for nodes sees it coming. - The node and agent cache keys, now the symlink-canonicalised path rather than
filepath.Abs
(configurable_workflow.go:209-213).
Two spellings of one file that used to get separate entries now collide.
Smaller:
newAgentDir
calls t.Skipf when os.Symlink fails, and every test built on it inherits
that, including the absolute-path and .. rejections. On a filesystem without
symlink support the whole containment suite passes by disappearing.
Line references are against ce74a295a34f9151dea938af207ce257ccf64013.
3 participants
#878 added a containment check to ResolveAgentReference, but the workflow loader resolves node references on a separate path that never reaches it. resolveNodeLike accepted an absolute reference and joined a relative one without validating the result, so a workflow config could name any .yaml or .yml file on disk.
The FunctionNode, JoinNode and ToolNode branches of resolveNodeFromYAML all return before its fall-through call to ResolveAgentReference, so for those node classes an out-of-tree file was read, parsed and instantiated with no containment check at all. Only the agent branch was covered, and by the time it ran the read had already happened.
Extract the check into resolveContainedConfigPath and call it from both resolvers, before the file is read and before the node cache is consulted. One shared copy is the point: two independent resolvers is how the gap appeared in the first place.
BREAKING: an absolute workflow node reference is no longer accepted, matching the config_path behaviour #878 established.