Harden command registration path handling

[mnriem]

What this fixes

CommandRegistrar.register_commands() read each command body from source_dir / cmd_file using the manifest's file field without validating it. Unlike the parallel skill and preset readers — which already reject absolute paths and .. traversal and require the resolved path to stay within the extension directory — the shared command path applied no such check.

As a result, a manifest with file: ../../../etc/passwd (or a bare absolute path) could cause an arbitrary host file to be read verbatim into a generated agent command file at a predictable path under the project (e.g. .claude/commands/..., mirrored for each detected agent).

Changes

• agents.py — register_commands(): add the same containment guard used by the skill/preset readers before reading the source file — reject absolute paths and ensure the resolved path stays within source_dir; skip the command otherwise.
• extensions.py — ExtensionManifest._validate(): reject an absolute or ..-containing file value at manifest-load time with a clear ValidationError (defense-in-depth, surfaces a real error instead of a silent skip).
• Tests:
  • test_registrar_path_traversal.py: new TestCommandFileTraversal covering the file read path (the existing tests only covered command name/alias output traversal). Asserts traversal/absolute file values register nothing and never leak file contents into generated commands.
  • test_extensions.py: parametrized manifest-load validation test for the file field.

Verification

New and targeted suites pass. Full suite: 3975 passed, with 2 pre-existing failures in test_init_dir.py (a SPECIFY_INIT_DIR core-scripts environment issue) confirmed unrelated to these changes by stashing.

[Copilot]

Pull request overview

This PR hardens extension command registration against path traversal and absolute-path reads via the manifest provides.commands[].file field, preventing arbitrary host files from being copied into generated agent command outputs. It adds both runtime containment checks and manifest-load validation, plus regression tests for the file read path.

Changes:

• Add containment checks in CommandRegistrar.register_commands() to ensure the resolved file path stays within source_dir.
• Validate provides.commands[].file during ExtensionManifest load to reject absolute paths and .. segments with a clear ValidationError.
• Add/extend tests to ensure traversal and absolute file values do not register commands or leak external file contents.

Show a summary per file

File	Description
src/specify_cli/agents.py	Adds source-file path containment guard before reading command bodies.
src/specify_cli/extensions.py	Adds defense-in-depth manifest validation for traversal/absolute file values.
tests/test_registrar_path_traversal.py	Adds regression tests ensuring file traversal/absolute paths don’t leak host content into generated commands.
tests/test_extensions.py	Adds manifest-load validation tests for invalid file values.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/specify_cli/agents.py:568

• register_commands() checks source_file.exists() then calls read_text(), which will raise (e.g., IsADirectoryError, UnicodeDecodeError) if a manifest points file at a directory or a non-UTF8 file. This can crash command registration; other readers in the codebase use is_file() and/or catch read errors. Consider aligning here by requiring source_file.is_file() and handling read errors by skipping the command.

            if not source_file.exists():
                continue

            content = source_file.read_text(encoding="utf-8")

• Files reviewed: 4/4 changed files
• Comments generated: 2

[mnriem]

Addressed the review feedback in b2e71f5:

• Absolute-path test reliability (test_registrar_path_traversal.py): the absolute-path payload now points at the real temp secret.txt (guaranteed to exist on every platform) instead of /etc/passwd, so the absolute-path guard is genuinely exercised — the test fails if the guard regresses, rather than passing because the target happens not to exist on a Windows runner.
• Tolerant reads (agents.py, the suppressed low-confidence note): register_commands() now uses is_file() instead of exists() and skips the command if read_text() raises (directory or non-UTF8 file), aligning with the other command/skill readers so a malformed manifest can't crash registration.

Full affected suites pass (318 passed).

Posted on behalf of @mnriem by GitHub Copilot CLI (model: Claude Opus 4.8).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 3

[mnriem]

Addressed the Windows drive-relative edge case in 3d5f039.

Path('C:outside.txt').is_absolute() is False and the value contains no .., so a drive-relative file previously passed both guards yet would resolve against the process CWD on that drive under Windows. Both the registrar runtime guard (agents.py) and the manifest-load validator (extensions.py) now also evaluate the value under PureWindowsPath, rejecting drive letters (and backslash .. segments) cross-platform. Extended the regression tests with drive-relative cases (C:escape.md, C:\Windows\x.md, ..\..\escape.md).

323 tests pass (up from 318).

Posted on behalf of @mnriem by GitHub Copilot CLI (model: Claude Opus 4.8).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 4

[mnriem]

Addressed this round in c4855b4:

• agents.py register_commands(): skip non-string/empty file values up front (no more potential TypeError from a malformed entry), and hoisted source_dir.resolve() out of the per-command loop.
• extensions.py _validate(): reject file values with leading/trailing whitespace with a clear ValidationError instead of a confusing missing-file failure later.
• tests: added non-string file and whitespace cases; switched the manifest validation test to yaml.safe_dump with explicit utf-8 encoding.
• .claude/skills dir: replied inline — that's the correct output path for Claude (skills-based agent), consistent with the existing positive test; left as-is.

330 affected tests pass (full suite also green on the prior rebase).

Posted on behalf of @mnriem by GitHub Copilot CLI (model: Claude Opus 4.8).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 3

[mnriem]

Addressed this round in 7b9a113:

• agents.py runtime guard: now also rejects .. segments under both POSIX and Windows semantics, so runtime policy matches ExtensionManifest._validate() and the skill/preset readers rather than relying solely on the resolve()/relative_to() backstop. Added an in-bounds commands/../cmd.md payload that exercises this (resolves inside source_dir, so only the .. check rejects it).
• extensions.py comment: replaced the version-dependent is_absolute() claim with the actual portability rationale — a native Path is OS-dependent (a PurePosixPath on POSIX won't interpret Windows drive/UNC forms), and C:foo is anchored but not absolute.
• tests: extracted the duplicated leak-detection block into _assert_no_marker_leak(project, marker).

332 affected tests pass.

Posted on behalf of @mnriem by GitHub Copilot CLI (model: Claude Opus 4.8).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 4

[mnriem]

Addressed review #4544960266 in 85246cc.

• Shared path-safety policy (comments 1 & 2): Extracted relative_extension_path_violation() into _utils.py as the single source of truth for the extension-relative file policy (non-empty string, no whitespace, no anchor under POSIX/Windows semantics, no ..). Both the runtime registrar guard (agents.py) and the manifest-load validator (extensions.py) now delegate to it, so the two can't drift. The manifest validator's error messages (Invalid command 'file', whitespace wording) are unchanged.
• Warn on unreadable file (comment 3): When an in-bounds command file exists but can't be read/decoded, the registrar now emits a warnings.warn (with command name + file) instead of skipping silently.
• In-bounds .. test (comment 4): test_dotdot_rejected_even_when_target_is_in_bounds now creates ext_dir/cmd.md so the skip is attributable to the .. rejection, not file absence. Added unit tests for the shared helper and a read-skip warning test.

Affected suites green locally (349 passed).

Posted on behalf of @mnriem by GitHub Copilot CLI (model: Claude Opus 4.8).

[Copilot]

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 0 new