From 9db2b4730875ada168c2614581d349a39748765e Mon Sep 17 00:00:00 2001 From: George Weale Date: Mon, 17 Aug 2026 23:04:00 +0000 Subject: [PATCH 1/2] fix(skills): block path traversal when extracting skill resources (v1) The wrapper script generated by _SkillScriptCodeExecutor joined each skill resource path onto the extraction directory as-is, so a resource named "../../pwned" resolved outside that directory and its content was written there before the skill script ran. The generated code now normalizes each relative path, raises PermissionError when the result starts with ".." or is absolute, and joins the remainder onto the absolute extraction directory. Nested paths such as "subdir/notes.md" still materialize. A resource whose filename itself begins with two dots, for example "..config", is refused as well; this matches the upstream fix. Ports the extraction hunk of upstream PR #5927. The unrelated argument-validation refactors in that commit are not included. --- src/google/adk/tools/skill_toolset.py | 8 ++- tests/unittests/tools/test_skill_toolset.py | 65 ++++++++++++++++++++- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/google/adk/tools/skill_toolset.py b/src/google/adk/tools/skill_toolset.py index ef579d8256c..45974fcb9cb 100644 --- a/src/google/adk/tools/skill_toolset.py +++ b/src/google/adk/tools/skill_toolset.py @@ -626,7 +626,13 @@ def _build_wrapper_code( " _orig_cwd = os.getcwd()", " with tempfile.TemporaryDirectory() as td:", " for rel_path, content in _files.items():", - " full_path = os.path.join(td, rel_path)", + " norm_rel = os.path.normpath(rel_path)", + " if norm_rel.startswith('..') or os.path.isabs(norm_rel):", + ( + " raise PermissionError('Path traversal blocked in skill" + " file: ' + rel_path)" + ), + " full_path = os.path.join(os.path.abspath(td), norm_rel)", " os.makedirs(os.path.dirname(full_path), exist_ok=True)", " mode = 'wb' if isinstance(content, bytes) else 'w'", " with open(full_path, mode) as f:", diff --git a/tests/unittests/tools/test_skill_toolset.py b/tests/unittests/tools/test_skill_toolset.py index f6373775135..bc78970808b 100644 --- a/tests/unittests/tools/test_skill_toolset.py +++ b/tests/unittests/tools/test_skill_toolset.py @@ -1031,8 +1031,9 @@ def get_script_extended(name): # ── Integration tests using real UnsafeLocalCodeExecutor ── -def _make_skill_with_script(skill_name, script_name, script): +def _make_skill_with_script(skill_name, script_name, script, references=None): """Creates a minimal mock Skill with a single script.""" + references = references or {} skill = mock.create_autospec(models.Skill, instance=True) skill.name = skill_name skill.description = f"Test skill {skill_name}" @@ -1058,9 +1059,9 @@ def get_script(name): return None skill.resources.get_script.side_effect = get_script - skill.resources.get_reference.return_value = None + skill.resources.get_reference.side_effect = references.get skill.resources.get_asset.return_value = None - skill.resources.list_references.return_value = [] + skill.resources.list_references.return_value = list(references) skill.resources.list_assets.return_value = [] skill.resources.list_scripts.return_value = [script_name] return skill @@ -1326,6 +1327,64 @@ async def test_integration_shell_nonzero_exit(): assert "42" in result["stderr"] +# ── Integration: skill resource paths stay inside the extraction dir ── + + +@pytest.mark.asyncio +async def test_integration_traversing_resource_name_is_refused( + tmp_path, monkeypatch +): + """Real executor: a resource name that escapes the temp dir is refused.""" + monkeypatch.setenv("TMPDIR", str(tmp_path)) + script = models.Script(src="print('ran')") + skill = _make_skill_with_script( + "test_skill", + "hello.py", + script, + references={"../../pwned": "owned"}, + ) + toolset = _make_real_executor_toolset([skill]) + tool = skill_toolset.RunSkillScriptTool(toolset) + ctx = _make_tool_context_with_agent() + result = await tool.run_async( + args={ + "skill_name": "test_skill", + "file_path": "hello.py", + }, + tool_context=ctx, + ) + assert "status" in result, f"Result missing status: {result}" + assert result["status"] == "error" + assert "PermissionError" in result["stderr"] + assert result["stdout"] == "" + assert not (tmp_path / "pwned").exists() + + +@pytest.mark.asyncio +async def test_integration_nested_resource_still_materializes(): + """Real executor: a nested resource path is still extracted.""" + script = models.Script(src="print(open('references/subdir/notes.md').read())") + skill = _make_skill_with_script( + "test_skill", + "hello.py", + script, + references={"subdir/notes.md": "nested content"}, + ) + toolset = _make_real_executor_toolset([skill]) + tool = skill_toolset.RunSkillScriptTool(toolset) + ctx = _make_tool_context_with_agent() + result = await tool.run_async( + args={ + "skill_name": "test_skill", + "file_path": "hello.py", + }, + tool_context=ctx, + ) + assert "status" in result, f"Result missing status: {result}" + assert result["status"] == "success" + assert "nested content" in result["stdout"] + + # ── Finding 1: system instruction references correct tool name ── From 5000d1c00388b37886fbe71e9fbb840a09ebeaa4 Mon Sep 17 00:00:00 2001 From: George Weale Date: Mon, 17 Aug 2026 23:06:45 +0000 Subject: [PATCH 2/2] fix(skills): validate skill name before building the registry resource path (v1) GCPSkillRegistry.get_skill() interpolated the caller-supplied name straight into "projects/.../locations/.../skills/{name}" and handed that to the Vertex SDK, so a name containing a slash or "../" addressed a different resource than the one asked for. The name comes from a model-issued tool call, so nothing upstream of this point constrains it. It now checks the name against the same snake-or-kebab pattern skill names are already held to, and raises ValueError before any request is made. Behaviour change: a name outside that character set, such as one with an uppercase letter, a dot, or a slash, now raises ValueError locally instead of reaching the registry. The registry would have rejected it anyway, so what changes is the error type and the fact that no request goes out. Ports the validation half of the upstream change. The percent-encoding half does not apply here, because this branch builds an SDK resource name rather than a URL and every character the pattern accepts is already safe. --- .../skill_registry/gcp_skill_registry.py | 15 +++++++++ .../skill_registry/test_gcp_skill_registry.py | 33 +++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py index 277913c1b44..d06b32d15ef 100644 --- a/src/google/adk/integrations/skill_registry/gcp_skill_registry.py +++ b/src/google/adk/integrations/skill_registry/gcp_skill_registry.py @@ -56,7 +56,22 @@ async def get_skill(self, *, name: str) -> models.Skill: Returns: A Skill object. + + Raises: + ValueError: If the name is not a valid skill name. """ + # The name reaches here straight from a model-issued tool call, so it must + # be a single path segment before it is interpolated into the resource + # path. Accept the same character set skill names are already held to; the + # snake-or-kebab pattern is the superset of the two accepted spellings. + # pylint: disable-next=protected-access + if not models._SNAKE_OR_KEBAB_NAME_PATTERN.match(name): + raise ValueError( + f"Invalid skill name {name!r}: name must be lowercase kebab-case" + " (a-z, 0-9, hyphens) or snake_case (a-z, 0-9, underscores), with" + " no leading, trailing, or consecutive delimiters." + ) + full_name = ( f"projects/{self.project_id}/locations/{self.location}/skills/{name}" ) diff --git a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py index bf410456e99..7834c320130 100644 --- a/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py +++ b/tests/unittests/integrations/skill_registry/test_gcp_skill_registry.py @@ -56,8 +56,9 @@ def _create_fake_zip_bytes(): return zip_buffer.getvalue() +@pytest.mark.parametrize("valid_name", ["my-skill", "my_skill", "skill2"]) @pytest.mark.asyncio -async def test_get_skill_success(mock_vertex_client): +async def test_get_skill_success(mock_vertex_client, valid_name): """Verifies that get_skill successfully fetches and loads a skill in memory.""" registry = GCPSkillRegistry() @@ -71,13 +72,13 @@ async def test_get_skill_success(mock_vertex_client): return_value=mock_skill_resource ) - skill = await registry.get_skill(name="my-skill") + skill = await registry.get_skill(name=valid_name) assert skill.frontmatter.name == "my-skill" assert skill.frontmatter.description == "test" assert skill.instructions == "# My Skill" mock_vertex_client.aio.skills.get.assert_called_once_with( - name="projects/test-project/locations/us-central1/skills/my-skill" + name=f"projects/test-project/locations/us-central1/skills/{valid_name}" ) @@ -182,3 +183,29 @@ async def test_get_skill_raises_on_invalid_skill_name(mock_vertex_client): with pytest.raises(ValueError, match="Invalid skill name in SKILL.md"): await registry.get_skill(name="my-skill") + + +@pytest.mark.parametrize( + "unsafe_name", + [ + "../../../projects/victim/locations/us-central1/skills/secret", + "my-skill/../other-skill", + "..%2f..%2fsecret", + "my-skill?alt=media", + "my-skill#fragment", + "my-skill/revisions/rev-123", + "My-Skill", + "", + ], +) +@pytest.mark.asyncio +async def test_get_skill_rejects_unsafe_name_before_any_request( + mock_vertex_client, unsafe_name +): + """Verifies that a name that is not a single safe path segment is rejected.""" + registry = GCPSkillRegistry() + + with pytest.raises(ValueError, match="Invalid skill name"): + await registry.get_skill(name=unsafe_name) + + mock_vertex_client.aio.skills.get.assert_not_called()