Files
github-spec-kit/tests/test_registrar_path_traversal.py
Manfred Riem 902f5431f9 Harden command registration path handling (#3088)
* 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>
2026-06-22 10:25:29 -05:00

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