mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
* fix: validate command 'file' field against path traversal in registrar CommandRegistrar.register_commands() read each command body from source_dir / cmd_file without validating the manifest 'file' field, unlike the parallel skill and preset readers which already reject absolute paths and '..' traversal. A malicious extension/preset/bundle manifest with file: ../../../etc/passwd (or an absolute path) could read arbitrary host files verbatim into a generated agent command at a predictable path (GHSA-w5fv-7w9x-7fc5, CWE-22). Add the same containment guard at the command read site and reject a traversal/absolute 'file' at manifest-load time in ExtensionManifest._validate() for defense-in-depth, plus regression tests for both the read path and the manifest validator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test/fix: address review — robust absolute-path test and tolerant reads - register_commands(): use is_file() instead of exists() and skip the command if read_text() raises (directory or non-UTF8 file), aligning with the other command/skill readers. - Traversal tests: point the absolute-path payload at the real temp secret.txt (guaranteed to exist on all platforms) instead of /etc/passwd, so the absolute-path guard is genuinely exercised and the test fails if it regresses, rather than passing because the target happens not to exist (e.g. on Windows runners). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: rename traversal fixtures to avoid CodeQL secret-storage false positive The regression fixtures named an out-of-tree file secret.txt with TOP-SECRET-CREDENTIAL content. CodeQL's clear-text-storage heuristic treated that read content as sensitive and followed the static path into the pre-existing write_text sinks in _write_registered_output, raising false 'clear-text storage of sensitive information' alerts on PR 3088. Rename the fixtures to neutral outside.txt / OUTSIDE-FILE-MARKER and drop /etc/passwd payloads; the test semantics (a file outside source_dir must never be read into a generated command) are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject Windows drive-relative 'file' values in traversal guards is_absolute() is False for Windows drive-relative paths like C:outside.txt, which contain no '..' yet resolve against the process CWD on that drive — bypassing the containment guard on Windows. Evaluate the 'file' value under PureWindowsPath as well so both the registrar runtime guard and the manifest-load validator reject drive letters (and backslash '..' segments) cross-platform. Extend the regression tests with drive-relative cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use anchor under both path flavors so POSIX-absolute is rejected on Windows On a Windows runner WindowsPath('/abs/outside.md').is_absolute() is False (no drive), so the prior native-Path check let a leading-slash 'file' value through and the manifest validator did not raise. Evaluate the value under both PurePosixPath and PureWindowsPath and reject any non-empty anchor — covering POSIX-absolute, Windows drive-relative, Windows absolute, and rooted-without-drive — in both the registrar guard and the manifest validator. The registrar join now uses the raw 'file' string so native separators are handled by the resolve()/relative_to() containment check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: validate command 'file' field against path traversal in registrar CommandRegistrar.register_commands() read each command body from source_dir / cmd_file without validating the manifest 'file' field, unlike the parallel skill and preset readers which already reject absolute paths and '..' traversal. A malicious extension/preset/bundle manifest with file: ../../../etc/passwd (or an absolute path) could read arbitrary host files verbatim into a generated agent command at a predictable path (GHSA-w5fv-7w9x-7fc5, CWE-22). Add the same containment guard at the command read site and reject a traversal/absolute 'file' at manifest-load time in ExtensionManifest._validate() for defense-in-depth, plus regression tests for both the read path and the manifest validator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test/fix: address review — robust absolute-path test and tolerant reads - register_commands(): use is_file() instead of exists() and skip the command if read_text() raises (directory or non-UTF8 file), aligning with the other command/skill readers. - Traversal tests: point the absolute-path payload at the real temp secret.txt (guaranteed to exist on all platforms) instead of /etc/passwd, so the absolute-path guard is genuinely exercised and the test fails if it regresses, rather than passing because the target happens not to exist (e.g. on Windows runners). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: rename traversal fixtures to avoid CodeQL secret-storage false positive The regression fixtures named an out-of-tree file secret.txt with TOP-SECRET-CREDENTIAL content. CodeQL's clear-text-storage heuristic treated that read content as sensitive and followed the static path into the pre-existing write_text sinks in _write_registered_output, raising false 'clear-text storage of sensitive information' alerts on PR 3088. Rename the fixtures to neutral outside.txt / OUTSIDE-FILE-MARKER and drop /etc/passwd payloads; the test semantics (a file outside source_dir must never be read into a generated command) are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject Windows drive-relative 'file' values in traversal guards is_absolute() is False for Windows drive-relative paths like C:outside.txt, which contain no '..' yet resolve against the process CWD on that drive — bypassing the containment guard on Windows. Evaluate the 'file' value under PureWindowsPath as well so both the registrar runtime guard and the manifest-load validator reject drive letters (and backslash '..' segments) cross-platform. Extend the regression tests with drive-relative cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use anchor under both path flavors so POSIX-absolute is rejected on Windows On a Windows runner WindowsPath('/abs/outside.md').is_absolute() is False (no drive), so the prior native-Path check let a leading-slash 'file' value through and the manifest validator did not raise. Evaluate the value under both PurePosixPath and PureWindowsPath and reject any non-empty anchor — covering POSIX-absolute, Windows drive-relative, Windows absolute, and rooted-without-drive — in both the registrar guard and the manifest validator. The registrar join now uses the raw 'file' string so native separators are handled by the resolve()/relative_to() containment check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: harden register_commands inputs and tighten manifest 'file' validation Address review feedback on #3088: - register_commands(): skip non-string/empty 'file' values instead of raising TypeError, and hoist source_dir.resolve() out of the per-command loop. - ExtensionManifest._validate(): reject 'file' values with leading/trailing whitespace with a clear ValidationError instead of a confusing missing-file failure later. - tests: add non-string 'file' and whitespace cases; use yaml.safe_dump with explicit utf-8 encoding in the manifest validation test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: align runtime '..' policy, correct comment, dedupe test helper Address review feedback on #3088: - register_commands(): also reject '..' segments under both POSIX and Windows semantics, keeping runtime policy consistent with ExtensionManifest._validate() and the skill/preset readers (not just relying on the resolve()/relative_to() containment backstop). - Replace the version-dependent is_absolute() claim in the extensions.py comment with the actual portability rationale (native Path is OS- dependent; C:foo is anchored but not absolute). - Extract the duplicated leak-detection assertion into _assert_no_marker_leak() and add an in-bounds '..' payload that exercises the new runtime '..' rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Extract shared path-safety policy and warn on unreadable command files Introduce relative_extension_path_violation() in _utils.py as the single source of truth for the extension-relative `file` path-safety policy, and use it from both the runtime registrar guard (agents.py) and the manifest-load validator (extensions.py) so the two cannot drift. Warn (instead of silently skipping) when an in-bounds command file exists but cannot be read/decoded, surfacing misconfigured extensions. Add unit tests for the shared helper, a read-skip warning test, and make the in-bounds `..` test create its target file so the skip is attributable to the `..` rejection rather than file absence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Retrigger CI Empty commit to re-trigger code scanning / CodeQL analysis on the PR merge ref. Assisted-by: GitHub Copilot CLI (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
383 lines
13 KiB
Python
383 lines
13 KiB
Python
"""Tests for CommandRegistrar directory traversal guards around issue #2229."""
|
|
|
|
import errno
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
from specify_cli.agents import CommandRegistrar
|
|
from specify_cli._utils import relative_extension_path_violation
|
|
|
|
|
|
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("/", ""))
|
|
|
|
|
|
ABS_OUTSIDE = "__ABS_OUTSIDE__"
|
|
|
|
FILE_FIELD_PAYLOADS = [
|
|
"../outside.txt",
|
|
"../../outside.txt",
|
|
"commands/../../outside.txt",
|
|
"C:outside.txt",
|
|
ABS_OUTSIDE,
|
|
]
|
|
|
|
|
|
def _resolve_payload(bad_file: str, outside_file: Path) -> str:
|
|
"""Map the absolute-path sentinel to the real, existing outside file.
|
|
|
|
Using the temp file's own absolute path (instead of ``/etc/passwd``)
|
|
guarantees the file exists on every platform — so the test fails if the
|
|
absolute-path guard regresses, rather than passing because the target
|
|
happens not to exist (e.g. on Windows runners).
|
|
"""
|
|
return str(outside_file) if bad_file == ABS_OUTSIDE else bad_file
|
|
|
|
|
|
def _assert_no_marker_leak(project: Path, marker: str) -> None:
|
|
"""Fail if ``marker`` content was written into any file under ``project``."""
|
|
leaked = [
|
|
p for p in project.rglob("*")
|
|
if p.is_file() and marker in p.read_text(encoding="utf-8", errors="ignore")
|
|
]
|
|
assert leaked == [], f"Outside file leaked into generated command: {leaked}"
|
|
|
|
|
|
class TestCommandFileTraversal:
|
|
"""The manifest ``file`` field must not read files outside source_dir.
|
|
|
|
Regression for GHSA-w5fv-7w9x-7fc5: ``register_commands`` read
|
|
``source_dir / cmd_file`` with no containment check, so a manifest with
|
|
a traversal (``file: ../../../outside.txt``) or an absolute path read an
|
|
arbitrary host file verbatim into the generated agent command.
|
|
"""
|
|
|
|
@pytest.mark.parametrize("bad_file", FILE_FIELD_PAYLOADS)
|
|
def test_claude_skips_traversal_in_file_field(self, tmp_path, bad_file):
|
|
project, ext_dir = _project_and_source(tmp_path)
|
|
(project / ".claude" / "skills").mkdir(parents=True)
|
|
|
|
outside_file = tmp_path / "outside.txt"
|
|
outside_file.write_text("OUTSIDE-FILE-MARKER", encoding="utf-8")
|
|
|
|
registrar = CommandRegistrar()
|
|
registered = registrar.register_commands(
|
|
"claude",
|
|
[{"name": "speckit.myext.hello", "file": _resolve_payload(bad_file, outside_file), "aliases": []}],
|
|
"myext",
|
|
ext_dir,
|
|
project,
|
|
)
|
|
|
|
assert registered == []
|
|
_assert_no_marker_leak(project, "OUTSIDE-FILE-MARKER")
|
|
|
|
@pytest.mark.parametrize("bad_file", FILE_FIELD_PAYLOADS)
|
|
def test_gemini_skips_traversal_in_file_field(self, tmp_path, bad_file):
|
|
project, ext_dir = _project_and_source(tmp_path)
|
|
(project / ".gemini" / "commands").mkdir(parents=True)
|
|
|
|
outside_file = tmp_path / "outside.txt"
|
|
outside_file.write_text("OUTSIDE-FILE-MARKER", encoding="utf-8")
|
|
|
|
registrar = CommandRegistrar()
|
|
registered = registrar.register_commands(
|
|
"gemini",
|
|
[{"name": "speckit.myext.hello", "file": _resolve_payload(bad_file, outside_file), "aliases": []}],
|
|
"myext",
|
|
ext_dir,
|
|
project,
|
|
)
|
|
|
|
assert registered == []
|
|
_assert_no_marker_leak(project, "OUTSIDE-FILE-MARKER")
|
|
|
|
@pytest.mark.parametrize("bad_value", [None, 123, "", ["x"]])
|
|
def test_non_string_file_is_skipped(self, tmp_path, bad_value):
|
|
"""A non-string/empty ``file`` must be skipped, not raise TypeError."""
|
|
project, ext_dir = _project_and_source(tmp_path)
|
|
(project / ".gemini" / "commands").mkdir(parents=True)
|
|
|
|
registrar = CommandRegistrar()
|
|
registered = registrar.register_commands(
|
|
"gemini",
|
|
[{"name": "speckit.myext.hello", "file": bad_value, "aliases": []}],
|
|
"myext",
|
|
ext_dir,
|
|
project,
|
|
)
|
|
|
|
assert registered == []
|
|
|
|
def test_dotdot_rejected_even_when_target_is_in_bounds(self, tmp_path):
|
|
"""An in-bounds ``..`` payload is rejected by the ``..`` check itself.
|
|
|
|
``commands/../cmd.md`` resolves to ``ext_dir/cmd.md`` — inside
|
|
source_dir — so the resolve()/relative_to() containment backstop would
|
|
allow it. Creating that target file ensures the command is skipped
|
|
because of the ``..`` rejection, not merely because the file is absent.
|
|
"""
|
|
project, ext_dir = _project_and_source(tmp_path)
|
|
(project / ".gemini" / "commands").mkdir(parents=True)
|
|
(ext_dir / "cmd.md").write_text(
|
|
"---\ndescription: test\n---\n\nbody\n", encoding="utf-8"
|
|
)
|
|
|
|
registrar = CommandRegistrar()
|
|
registered = registrar.register_commands(
|
|
"gemini",
|
|
[{"name": "speckit.myext.hello", "file": "commands/../cmd.md", "aliases": []}],
|
|
"myext",
|
|
ext_dir,
|
|
project,
|
|
)
|
|
|
|
assert registered == []
|
|
|
|
|
|
class TestRelativeExtensionPathPolicy:
|
|
"""Unit tests for the shared ``relative_extension_path_violation`` policy."""
|
|
|
|
@pytest.mark.parametrize(
|
|
"value",
|
|
[
|
|
"commands/hello.md",
|
|
"hello.md",
|
|
"a/b/c/hello.md",
|
|
],
|
|
)
|
|
def test_safe_relative_paths_have_no_violation(self, value):
|
|
assert relative_extension_path_violation(value) is None
|
|
|
|
@pytest.mark.parametrize(
|
|
"value",
|
|
[
|
|
None,
|
|
123,
|
|
["x"],
|
|
"",
|
|
" ",
|
|
" hello.md",
|
|
"hello.md ",
|
|
"/abs/outside.md",
|
|
"/etc/passwd",
|
|
"C:foo.md",
|
|
"C:\\Windows\\system32",
|
|
"\\\\server\\share\\x.md",
|
|
"../escape.md",
|
|
"commands/../../escape.md",
|
|
],
|
|
)
|
|
def test_unsafe_values_report_violation(self, value):
|
|
assert relative_extension_path_violation(value) is not None
|
|
|
|
|
|
class TestReadSkipWarning:
|
|
"""Unregisterable but in-bounds files warn instead of failing silently."""
|
|
|
|
def test_unreadable_target_warns_and_skips(self, tmp_path):
|
|
project, ext_dir = _project_and_source(tmp_path)
|
|
(project / ".gemini" / "commands").mkdir(parents=True)
|
|
(ext_dir / "cmd.md").write_bytes(b"\xff\xfe\x00\x80bad")
|
|
|
|
registrar = CommandRegistrar()
|
|
with pytest.warns(UserWarning):
|
|
registered = registrar.register_commands(
|
|
"gemini",
|
|
[{"name": "speckit.myext.hello", "file": "cmd.md", "aliases": []}],
|
|
"myext",
|
|
ext_dir,
|
|
project,
|
|
)
|
|
|
|
assert registered == []
|
|
|
|
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()
|