diff --git a/bot_bottle/contrib/claude/agent_provider.py b/bot_bottle/contrib/claude/agent_provider.py index 1eee6ee..8999868 100644 --- a/bot_bottle/contrib/claude/agent_provider.py +++ b/bot_bottle/contrib/claude/agent_provider.py @@ -217,7 +217,7 @@ class ClaudeAgentProvider(AgentProvider): if not agent.skills: return skills_dir = _skills_dir(plan.guest_home) - bottle.exec(f"mkdir -p {skills_dir}", user="root") + bottle.exec(f"mkdir -p {shlex.quote(skills_dir)}", user="root") for name in agent.skills: src = host_skill_dir(name) if not os.path.isdir(src): @@ -227,9 +227,13 @@ class ClaudeAgentProvider(AgentProvider): ) dst = f"{skills_dir}/{name}" info(f"copying skill {name} into {bottle.name}:{dst}") - bottle.exec(f"rm -rf {dst} && mkdir -p {dst}", user="root") + # Defense in depth: skill names are validated kebab-case at + # manifest load, but quote the path so a future unvalidated + # field can't inject shell metacharacters here either. + dst_q = shlex.quote(dst) + bottle.exec(f"rm -rf {dst_q} && mkdir -p {dst_q}", user="root") bottle.cp_in(f"{src}/.", f"{dst}/") - bottle.exec(f"chown -R node:node {dst}", user="root") + bottle.exec(f"chown -R node:node {dst_q}", user="root") def provision_prompt(self, plan: "BottlePlan", bottle: "Bottle") -> str | None: """Copy the prompt file into the guest, fix ownership/mode. diff --git a/bot_bottle/contrib/codex/agent_provider.py b/bot_bottle/contrib/codex/agent_provider.py index ac13b46..812b13d 100644 --- a/bot_bottle/contrib/codex/agent_provider.py +++ b/bot_bottle/contrib/codex/agent_provider.py @@ -183,7 +183,7 @@ class CodexAgentProvider(AgentProvider): if not agent.skills: return skills_dir = _skills_dir(plan.guest_home) - bottle.exec(f"mkdir -p {skills_dir}", user="root") + bottle.exec(f"mkdir -p {shlex.quote(skills_dir)}", user="root") for name in agent.skills: src = host_skill_dir(name) if not os.path.isdir(src): @@ -193,9 +193,13 @@ class CodexAgentProvider(AgentProvider): ) dst = f"{skills_dir}/{name}" info(f"copying skill {name} into {bottle.name}:{dst}") - bottle.exec(f"rm -rf {dst} && mkdir -p {dst}", user="root") + # Defense in depth: skill names are validated kebab-case at + # manifest load, but quote the path so a future unvalidated + # field can't inject shell metacharacters here either. + dst_q = shlex.quote(dst) + bottle.exec(f"rm -rf {dst_q} && mkdir -p {dst_q}", user="root") bottle.cp_in(f"{src}/.", f"{dst}/") - bottle.exec(f"chown -R node:node {dst}", user="root") + bottle.exec(f"chown -R node:node {dst_q}", user="root") def provision_prompt(self, plan: "BottlePlan", bottle: "Bottle") -> str | None: """Copy the prompt file into the guest, fix ownership/mode. diff --git a/bot_bottle/contrib/pi/agent_provider.py b/bot_bottle/contrib/pi/agent_provider.py index 2c9bf38..9c03b32 100644 --- a/bot_bottle/contrib/pi/agent_provider.py +++ b/bot_bottle/contrib/pi/agent_provider.py @@ -238,7 +238,7 @@ class PiAgentProvider(AgentProvider): if not agent.skills: return skills_dir = _skills_dir(plan.guest_home) - bottle.exec(f"mkdir -p {skills_dir}", user="root") + bottle.exec(f"mkdir -p {shlex.quote(skills_dir)}", user="root") for name in agent.skills: src = host_skill_dir(name) if not os.path.isdir(src): @@ -248,9 +248,13 @@ class PiAgentProvider(AgentProvider): ) dst = f"{skills_dir}/{name}" info(f"copying skill {name} into {bottle.name}:{dst}") - bottle.exec(f"rm -rf {dst} && mkdir -p {dst}", user="root") + # Defense in depth: skill names are validated kebab-case at + # manifest load, but quote the path so a future unvalidated + # field can't inject shell metacharacters here either. + dst_q = shlex.quote(dst) + bottle.exec(f"rm -rf {dst_q} && mkdir -p {dst_q}", user="root") bottle.cp_in(f"{src}/.", f"{dst}/") - bottle.exec(f"chown -R node:node {dst}", user="root") + bottle.exec(f"chown -R node:node {dst_q}", user="root") def provision_prompt(self, plan: "BottlePlan", bottle: "Bottle") -> str | None: prompt_path = _prompt_path(plan.guest_home) diff --git a/bot_bottle/manifest_agent.py b/bot_bottle/manifest_agent.py index 0c7f9b2..c0fb1cf 100644 --- a/bot_bottle/manifest_agent.py +++ b/bot_bottle/manifest_agent.py @@ -8,7 +8,7 @@ from typing import cast from .agent_provider import PROVIDER_TEMPLATES from .manifest_util import ManifestError, as_json_object from .manifest_git import ManifestGitUser -from .manifest_schema import AGENT_MODEL_KEYS +from .manifest_schema import AGENT_MODEL_KEYS, is_valid_entity_name @dataclass(frozen=True) @@ -161,6 +161,16 @@ class ManifestAgent: f"agent '{name}' skills[{i}] must be a string " f"(was {type(skill).__name__})" ) + # Skill names become host/guest path segments and are + # interpolated into provisioning shell commands, so they + # must fit the same kebab-case convention as bottle/agent + # filenames — rejecting anything that could break out of a + # path segment or inject shell metacharacters. + if not is_valid_entity_name(skill): + raise ManifestError( + f"agent '{name}' skills[{i}] {skill!r} is not a valid " + f"skill name; must match [a-z][a-z0-9-]*" + ) collected.append(skill) skills = tuple(collected) diff --git a/bot_bottle/manifest_schema.py b/bot_bottle/manifest_schema.py index 13c86bf..3e292b2 100644 --- a/bot_bottle/manifest_schema.py +++ b/bot_bottle/manifest_schema.py @@ -33,13 +33,20 @@ AGENT_KEYS = ( AGENT_MODEL_KEYS = AGENT_KEYS | frozenset({"prompt"}) +def is_valid_entity_name(name: str) -> bool: + """True if `name` fits the kebab-case `[a-z][a-z0-9-]*` convention + shared by bottle/agent filenames and skill names. Names that satisfy + this are also safe to interpolate into a host/guest path segment.""" + return bool(_FILENAME_RX.match(name)) + + def entity_name_from_path(path: Path) -> str | None: """Return the entity name implied by the filename, or None if the filename does not fit the [a-z][a-z0-9-]* convention.""" if path.suffix != ".md": return None stem = path.stem - if not _FILENAME_RX.match(stem): + if not is_valid_entity_name(stem): return None return stem diff --git a/tests/unit/test_manifest_validation.py b/tests/unit/test_manifest_validation.py index dbc5a90..45e05cf 100644 --- a/tests/unit/test_manifest_validation.py +++ b/tests/unit/test_manifest_validation.py @@ -165,6 +165,22 @@ class TestAgentValidation(unittest.TestCase): with self.assertRaises(ManifestError): ManifestAgent.from_dict("a", {"skills": [5]}, set()) + def test_skill_name_rejects_shell_metacharacters(self) -> None: + # Skill names become host/guest path segments interpolated into + # provisioning shell commands; anything outside kebab-case is + # rejected at load so it can never reach a `bottle.exec` string. + for bad in ("foo; rm -rf /", "../escape", "foo bar", "Foo", "-leading"): + with self.assertRaises(ManifestError): + ManifestAgent.from_dict("a", {"skills": [bad]}, set()) + + def test_skill_name_accepts_kebab_case(self) -> None: + agent = ManifestAgent.from_dict( + "a", {"skills": ["init-entry", "quality-eval", "skill0"]}, set() + ) + self.assertEqual( + agent.skills, ("init-entry", "quality-eval", "skill0") + ) + def test_prompt_not_string(self) -> None: with self.assertRaises(ManifestError): ManifestAgent.from_dict("a", {"prompt": 5}, set())