Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
17 changes: 16 additions & 1 deletion src/specify_cli/agents.py
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,22 @@ def register_commands(
aliases = cmd_info.get("aliases", [])
cmd_file = cmd_info["file"]

source_file = source_dir / cmd_file
# Guard against path traversal: reject absolute paths and ensure
# the resolved file stays within the source directory. Mirrors the
# containment checks already applied on the skill, preset, and
# restore paths (see extensions.py and presets/__init__.py) so a
# malicious manifest ``file`` field (e.g. ``../../../etc/passwd``)
# cannot read arbitrary host files into a generated command.
cmd_path = Path(cmd_file)
if cmd_path.is_absolute():
continue
Comment thread
mnriem marked this conversation as resolved.
Outdated
try:
source_root = source_dir.resolve()
source_file = (source_root / cmd_path).resolve()
source_file.relative_to(source_root) # raises ValueError if outside
except (OSError, ValueError):
continue

if not source_file.exists():
continue

Expand Down
16 changes: 16 additions & 0 deletions src/specify_cli/extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,22 @@ def _validate(self):
if "name" not in cmd or "file" not in cmd:
raise ValidationError("Command missing 'name' or 'file'")

# Validate the 'file' field for path traversal at manifest-load
# time. This is defense-in-depth: the command/skill/preset readers
# also contain the resolved path, but rejecting a traversal here
# surfaces a clear error instead of silently skipping the command.
cmd_file = cmd["file"]
if not isinstance(cmd_file, str) or not cmd_file:
raise ValidationError(
f"Command 'file' for '{cmd.get('name')}' must be a non-empty string"
)
Comment thread
mnriem marked this conversation as resolved.
Outdated
cmd_file_path = Path(cmd_file)
if cmd_file_path.is_absolute() or ".." in cmd_file_path.parts:
raise ValidationError(
f"Invalid command 'file' '{cmd_file}': must be a relative path "
"within the extension directory (no absolute paths or '..' segments)"
)
Comment thread
mnriem marked this conversation as resolved.
Outdated

# Validate command name format
if not EXTENSION_COMMAND_NAME_PATTERN.match(cmd["name"]):
corrected = self._try_correct_command_name(cmd["name"], ext["id"])
Expand Down
20 changes: 20 additions & 0 deletions tests/test_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,26 @@ def test_invalid_command_name(self, temp_dir, valid_manifest_data):
with pytest.raises(ValidationError, match="Invalid command name"):
ExtensionManifest(manifest_path)

@pytest.mark.parametrize(
"bad_file",
["../../../etc/passwd", "../escape.md", "a/../../escape.md", "/etc/passwd"],
)
Comment thread
mnriem marked this conversation as resolved.
def test_command_file_traversal_rejected(self, temp_dir, valid_manifest_data, bad_file):
"""Manifest 'file' field with traversal/absolute path raises ValidationError.

Defense-in-depth for GHSA-w5fv-7w9x-7fc5.
"""
import yaml

valid_manifest_data["provides"]["commands"][0]["file"] = bad_file

manifest_path = temp_dir / "extension.yml"
with open(manifest_path, 'w') as f:
yaml.dump(valid_manifest_data, f)
Comment thread
mnriem marked this conversation as resolved.
Outdated

with pytest.raises(ValidationError, match="Invalid command 'file'"):
ExtensionManifest(manifest_path)

def test_command_name_autocorrect_speckit_prefix(self, temp_dir, valid_manifest_data):
"""Test that 'speckit.command' is auto-corrected to 'speckit.{ext_id}.command'."""
import yaml
Expand Down
66 changes: 66 additions & 0 deletions tests/test_registrar_path_traversal.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,72 @@ def test_rejects_traversal_names(self, tmp_path, bad_name):
_assert_no_stray_files(tmp_path, Path(bad_name).name.replace("/", ""))


FILE_FIELD_PAYLOADS = [
"../secret.txt",
"../../secret.txt",
"commands/../../secret.txt",
"/etc/passwd",
]
Comment thread
mnriem marked this conversation as resolved.


class TestCommandFileTraversal:
"""The manifest ``file`` field must not read host 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
``file: ../../../etc/passwd`` (or an absolute path) leaked arbitrary host
files 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)
Comment thread
mnriem marked this conversation as resolved.

secret = tmp_path / "secret.txt"
secret.write_text("TOP-SECRET-CREDENTIAL", encoding="utf-8")

registrar = CommandRegistrar()
registered = registrar.register_commands(
Comment thread
mnriem marked this conversation as resolved.
Outdated
"claude",
[{"name": "speckit.myext.hello", "file": bad_file, "aliases": []}],
"myext",
ext_dir,
project,
)

assert registered == []
leaked = [
p for p in (project).rglob("*")
if p.is_file() and "TOP-SECRET-CREDENTIAL" in p.read_text(encoding="utf-8", errors="ignore")
]
assert leaked == [], f"Secret leaked into generated command: {leaked}"

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

secret = tmp_path / "secret.txt"
secret.write_text("TOP-SECRET-CREDENTIAL", encoding="utf-8")

registrar = CommandRegistrar()
registered = registrar.register_commands(
Comment thread
mnriem marked this conversation as resolved.
Outdated
"gemini",
[{"name": "speckit.myext.hello", "file": bad_file, "aliases": []}],
"myext",
ext_dir,
project,
)

assert registered == []
leaked = [
p for p in (project).rglob("*")
if p.is_file() and "TOP-SECRET-CREDENTIAL" in p.read_text(encoding="utf-8", errors="ignore")
]
assert leaked == [], f"Secret leaked into generated command: {leaked}"


class TestSafeRegistration:
"""Positive regression — well-formed names continue to register."""

Expand Down
Loading