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/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/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() 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 ──