Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/google/adk/integrations/skill_registry/gcp_skill_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
)
Expand Down
8 changes: 7 additions & 1 deletion src/google/adk/tools/skill_toolset.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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}"
)


Expand Down Expand Up @@ -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()
65 changes: 62 additions & 3 deletions tests/unittests/tools/test_skill_toolset.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand All @@ -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
Expand Down Expand Up @@ -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 ──


Expand Down
Loading