fix: Port agent-config code-reference and config_path validation to v1 - #6790
Open
GWeale wants to merge 4 commits into
Open
fix: Port agent-config code-reference and config_path validation to v1#6790GWeale wants to merge 4 commits into
GWeale wants to merge 4 commits into
Conversation
The module-level flag guarding the `args` YAML-key check in `config_agent_utils` was called `_ENFORCE_DENYLIST`, with `_set_enforce_denylist()` as its setter. It now reads `_ENFORCE_YAML_KEY_DENYLIST` / `_set_enforce_yaml_key_denylist()`, matching the name used on the main branch. The name is being freed for a second, unrelated control that the next commit adds: a denylist of modules an agent config may import from. Leaving both checks on one flag would make enabling the key check silently enable the module check as well. Pure rename with no behaviour change. The only caller in product code is `get_fast_api_app`, which enables the check when the web UI is on, and it is updated here.
A YAML agent config names Python code by dotted path, and the loader
imported whatever it was given. `before_agent_callbacks: [{name: os.system}]`
resolved and became a callback; `tools: [{name: cProfile.run}]` resolved and
became a tool that runs a string it is handed. Now every name a config
supplies is checked before the import, and a name whose top-level module is
part of the standard library is rejected with a ValueError.
The check is `_validate_module_reference()`, called at the four places that
turn a config-supplied name into an object: `resolve_fully_qualified_name`,
`_resolve_agent_code_reference` and `resolve_code_reference` in
config_agent_utils, and the user-defined branch of `LlmAgent._resolve_tools`.
The blocked set is `sys.stdlib_module_names` and `sys.builtin_module_names`,
plus an explicit list for names that stay importable without being reported
as standard library any more: `distutils`, `telnetlib`, `pipes`, `crypt`,
CPython's own `test` and `_testcapi` packages, and the `posix`, `nt`,
`_posixsubprocess` and `_socket` aliases.
Blocking the whole standard library rather than a list of dangerous modules
is deliberate. A short list has to be right about which modules can run code,
and `cProfile.run`, `timeit.timeit` and `trace.Trace.run` all execute a string
you pass them, with more arriving in each Python release.
Behaviour change: an existing config that names a standard-library callable
stops loading. Nothing in this repository does. The case to watch is an agent
package whose own name matches a standard-library module, such as `test`,
`secrets` or `calendar`; `_set_enforce_denylist(False)` turns the check off.
Third-party packages stay resolvable by name, so integrations keep working
and this narrows the surface rather than closing it.
Ports the final state of three changes that supersede one another upstream,
so the intermediate module lists are not reproduced here.
Co-authored-by: Ashutosh Kumar Singh <161562995+Ashutosh0x@users.noreply.github.com>
Co-authored-by: HenD.YA <yusmer96@gmail.com>
Co-authored-by: Kathy Wu <wukathy@google.com>
…on (v1) `resolve_agent_reference` took the `config_path` of a sub-agent or AgentTool reference straight from the YAML. An absolute path was loaded as given, and a relative one was joined to the referencing config's directory with no check on where it landed, so `../../../../etc/passwd` read that file on the server and the FileNotFoundError told the caller whether a path existed. It now rejects an absolute config_path outright, and resolves a relative one through os.path.realpath and requires the result to stay inside the directory holding the config that named it. Behaviour change, and the one in this area that can break a working setup. An absolute config_path is legal on 1.x today and is the obvious way to point at a shared agent library outside the app tree; it now raises. A relative path that climbs above the referencing config's own directory also raises, and the boundary is that directory rather than the agents root, so `../shared/x.yaml` is rejected even though it stays under agents_dir. No config in this repository does either. Co-authored-by: Adil Burak Şen <56400880+adilburaksen@users.noreply.github.com>
The Agent Builder upload check tested one key, `args`, and let everything else
through. So `tools: [{name: os.system}]` was accepted, written under the
agents directory, and imported and called the next time that agent loaded. It
now also validates every field whose value names Python code the loader will
import, and requires each name to live under the app being edited or to be an
ADK built-in.
The fields checked are the twelve in `_CODE_REFERENCE_KEYS`: agent_class, the
six callback lists, code, input_schema, output_schema, model_code and tools. A
name with no dots is left alone, because the loader resolves it against
`google.adk.agents` or `google.adk.tools` rather than anything the upload
controls. A qualified ADK name is allowed one segment past those namespaces,
which is what an undotted name would have reached anyway; a deeper path walks
into a submodule and is refused.
An app whose own name matches an importable module, `os` for instance, gets a
distinct error rather than an allow, since a reference starting `os.` cannot
be told apart from one leaving the app.
Behaviour change, confined to the builder UI. A builder user whose YAML
references a helper in a sibling app, or a third-party package by dotted name,
now gets a 400 on save.
This is upload-time and stricter than the load-time module denylist added
earlier in this branch. Neither replaces the other: the loader must keep
accepting third-party packages, and the builder need not.
Upstream this lives in `dev_server.py`, which does not exist on this branch;
the equivalent code here is in `fast_api.py`, gated on `web` in the same way.
GWeale
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR ports three agent-config fixes to the
v1branch, plus a preparatory rename.Module denylist for code references (
6a5be34b,faa17442,a16f6da3)sys.stdlib_module_names | sys.builtin_module_names; anything else raisesValueError. Enforced at every import site, on by default.config_agent_utils._set_enforce_denylist(False), which an agent package whose own name matches a standard-library module (secrets,types) needs.config_pathresolution (171ae9e7)config_pathin anAgentRefConfigmust be relative and resolve under the directory of the config that names it; an absolute path raises.Builder upload validation (
89950051)POST /builder/savereturns 400 unless every code reference in the uploaded YAML names code under the app being edited or an ADK built-in. Builder routes are mounted only whenweb=True.v1rather than ported.Preparatory rename:
_ENFORCE_DENYLISTand_set_enforce_denylistbecome_ENFORCE_YAML_KEY_DENYLISTand_set_enforce_yaml_key_denylist, matchingmain._set_enforce_denylistcontrols the module denylist;_set_enforce_yaml_key_denylistcontrols theargsYAML-key check.