diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 1a0e5a8317..ef4e879c03 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -6,6 +6,7 @@ command files into agent-specific directories in the correct format. """ +import os from pathlib import Path from typing import Dict, List, Any @@ -399,6 +400,28 @@ def _compute_output_name( return f"speckit-{short_name}" + @staticmethod + def _ensure_inside(candidate: Path, base: Path) -> None: + """Validate that a write target stays within the expected base directory. + + Uses lexical normalization so traversal via ``..`` or absolute paths is + rejected while intentionally symlinked sub-directories remain + supported. + + Args: + candidate: Path that will be written. + base: Directory the write must remain within. + + Raises: + ValueError: If the normalized candidate path escapes ``base``. + """ + normalized = Path(os.path.normpath(candidate)) + base_normalized = Path(os.path.normpath(base)) + if not normalized.is_relative_to(base_normalized): + raise ValueError( + f"Output path {candidate!r} escapes directory {base!r}" + ) + def register_commands( self, agent_name: str, @@ -485,6 +508,7 @@ def register_commands( raise ValueError(f"Unsupported format: {agent_config['format']}") dest_file = commands_dir / f"{output_name}{agent_config['extension']}" + self._ensure_inside(dest_file, commands_dir) dest_file.parent.mkdir(parents=True, exist_ok=True) dest_file.write_text(output, encoding="utf-8") @@ -550,12 +574,7 @@ def register_commands( alias_file = ( commands_dir / f"{alias_output_name}{agent_config['extension']}" ) - try: - alias_file.resolve().relative_to(commands_dir.resolve()) - except ValueError: - raise ValueError( - f"Alias output path escapes commands directory: {alias_file!r}" - ) + self._ensure_inside(alias_file, commands_dir) alias_file.parent.mkdir(parents=True, exist_ok=True) alias_file.write_text(alias_output, encoding="utf-8") if agent_name == "copilot": @@ -575,6 +594,7 @@ def write_copilot_prompt(project_root: Path, cmd_name: str) -> None: prompts_dir = project_root / ".github" / "prompts" prompts_dir.mkdir(parents=True, exist_ok=True) prompt_file = prompts_dir / f"{cmd_name}.prompt.md" + CommandRegistrar._ensure_inside(prompt_file, prompts_dir) prompt_file.write_text(f"---\nagent: {cmd_name}\n---\n", encoding="utf-8") def register_commands_for_all_agents( diff --git a/tests/test_registrar_path_traversal.py b/tests/test_registrar_path_traversal.py new file mode 100644 index 0000000000..fc423b4056 --- /dev/null +++ b/tests/test_registrar_path_traversal.py @@ -0,0 +1,204 @@ +"""Tests for CommandRegistrar directory traversal guards around issue #2229.""" + +import errno +from pathlib import Path + +import pytest + +from specify_cli.agents import CommandRegistrar + + +TRAVERSAL_PAYLOADS = [ + "../pwned", + "../../etc/passwd", + "subdir/../../escape", + "/absolute/evil", +] + + +def _write_source(ext_dir: Path) -> Path: + ext_dir.mkdir(parents=True, exist_ok=True) + (ext_dir / "commands").mkdir(exist_ok=True) + (ext_dir / "commands" / "cmd.md").write_text( + "---\ndescription: test\n---\n\nbody\n", encoding="utf-8" + ) + return ext_dir + + +def _cmd(name: str, aliases: list[str] | None = None) -> dict[str, object]: + return { + "name": name, + "file": "commands/cmd.md", + "aliases": list(aliases or []), + } + + +def _project_and_source(tmp_path): + project = tmp_path / "project" + project.mkdir() + ext_dir = _write_source(tmp_path / "ext-src") + return project, ext_dir + + +def _assert_no_stray_files(tmp_root: Path, marker: str) -> None: + """Fail if a file matching ``marker`` exists outside the project tree.""" + stray = [ + p for p in tmp_root.rglob("*") + if p.is_file() and marker in p.name and "project" not in p.parts + ] + assert stray == [], ( + f"Traversal payload leaked files outside the project tree: {stray}" + ) + + +class TestPrimaryCommandTraversal: + """Primary command names must not escape the agent's commands directory.""" + + @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS) + def test_gemini_rejects_traversal_in_primary_name(self, tmp_path, bad_name): + project, ext_dir = _project_and_source(tmp_path) + (project / ".gemini" / "commands").mkdir(parents=True) + + registrar = CommandRegistrar() + with pytest.raises(ValueError, match="escapes|outside|Invalid"): + registrar.register_commands( + "gemini", [_cmd(bad_name)], "myext", ext_dir, project + ) + + _assert_no_stray_files(tmp_path, Path(bad_name).name.replace("/", "")) + + @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS) + def test_copilot_rejects_traversal_in_primary_name(self, tmp_path, bad_name): + project, ext_dir = _project_and_source(tmp_path) + (project / ".github" / "agents").mkdir(parents=True) + (project / ".github" / "prompts").mkdir(parents=True) + + registrar = CommandRegistrar() + with pytest.raises(ValueError, match="escapes|outside|Invalid"): + registrar.register_commands( + "copilot", [_cmd(bad_name)], "myext", ext_dir, project + ) + + _assert_no_stray_files(tmp_path, Path(bad_name).name.replace("/", "")) + + +class TestAliasTraversal: + """Free-form aliases must not escape commands_dir (regression for b67b285).""" + + @pytest.mark.parametrize("bad_alias", TRAVERSAL_PAYLOADS) + def test_gemini_rejects_traversal_in_alias(self, tmp_path, bad_alias): + project, ext_dir = _project_and_source(tmp_path) + (project / ".gemini" / "commands").mkdir(parents=True) + + registrar = CommandRegistrar() + with pytest.raises(ValueError, match="escapes|outside|Invalid"): + registrar.register_commands( + "gemini", + [_cmd("speckit.myext.ok", [bad_alias])], + "myext", + ext_dir, + project, + ) + + _assert_no_stray_files(tmp_path, Path(bad_alias).name.replace("/", "")) + + @pytest.mark.parametrize("bad_alias", TRAVERSAL_PAYLOADS) + def test_copilot_rejects_traversal_in_alias(self, tmp_path, bad_alias): + project, ext_dir = _project_and_source(tmp_path) + (project / ".github" / "agents").mkdir(parents=True) + (project / ".github" / "prompts").mkdir(parents=True) + + registrar = CommandRegistrar() + with pytest.raises(ValueError, match="escapes|outside|Invalid"): + registrar.register_commands( + "copilot", + [_cmd("speckit.myext.ok", [bad_alias])], + "myext", + ext_dir, + project, + ) + + _assert_no_stray_files(tmp_path, Path(bad_alias).name.replace("/", "")) + + +class TestCopilotPromptTraversal: + """`write_copilot_prompt` is a public static method — guard it directly.""" + + @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS) + def test_rejects_traversal_names(self, tmp_path, bad_name): + project = tmp_path / "project" + (project / ".github" / "prompts").mkdir(parents=True) + + with pytest.raises(ValueError, match="escapes|outside|Invalid"): + CommandRegistrar.write_copilot_prompt(project, bad_name) + + _assert_no_stray_files(tmp_path, Path(bad_name).name.replace("/", "")) + + +class TestSafeRegistration: + """Positive regression — well-formed names continue to register.""" + + def test_symlinked_subdir_under_commands_dir_is_preserved(self, tmp_path): + """Lexical check must not block legitimately symlinked sub-directories. + + Teams sometimes symlink shared skills into their agent commands dir + (e.g. ``.gemini/commands/shared -> /team/shared-commands``). The + guard is purely lexical, so such a setup continues to work even though + the resolved target lives outside commands_dir on disk. + """ + project, ext_dir = _project_and_source(tmp_path) + commands_dir = project / ".gemini" / "commands" + commands_dir.mkdir(parents=True) + + external_shared = tmp_path / "external-shared" + external_shared.mkdir() + try: + (commands_dir / "shared").symlink_to( + external_shared, target_is_directory=True + ) + except OSError as exc: + if exc.errno in {errno.EPERM, errno.EACCES}: + pytest.skip("symlink creation is not permitted in this environment") + raise + + registrar = CommandRegistrar() + registered = registrar.register_commands( + "gemini", + [_cmd("shared/hello")], + "myext", + ext_dir, + project, + ) + + assert registered == ["shared/hello"] + assert (external_shared / "hello.toml").exists() + + def test_safe_command_and_alias_still_register(self, tmp_path): + project, ext_dir = _project_and_source(tmp_path) + (project / ".claude" / "skills").mkdir(parents=True) + + registrar = CommandRegistrar() + registered = registrar.register_commands( + "claude", + [_cmd("speckit.myext.hello", ["speckit.myext.hi"])], + "myext", + ext_dir, + project, + ) + + assert "speckit.myext.hello" in registered + assert "speckit.myext.hi" in registered + assert ( + project + / ".claude" + / "skills" + / "speckit-myext-hello" + / "SKILL.md" + ).exists() + assert ( + project + / ".claude" + / "skills" + / "speckit-myext-hi" + / "SKILL.md" + ).exists()