diff --git a/src/specify_cli/_download_security.py b/src/specify_cli/_download_security.py index b4f6d28add..131e68087a 100644 --- a/src/specify_cli/_download_security.py +++ b/src/specify_cli/_download_security.py @@ -1,10 +1,19 @@ -"""Helpers for bounded HTTP downloads.""" +"""Helpers for bounded downloads and archive extraction.""" from __future__ import annotations import io +import re import socket +import stat +import struct +import unicodedata +import zipfile +from collections.abc import Iterator +from contextlib import ExitStack, contextmanager from ipaddress import IPv4Address, IPv6Address, ip_address +from itertools import pairwise +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import NoReturn, TypeVar from urllib.parse import ParseResult, urlparse @@ -12,17 +21,52 @@ ErrorT = TypeVar("ErrorT", bound=Exception) MAX_DOWNLOAD_BYTES = 50 * 1024 * 1024 +MAX_ZIP_ENTRIES = 512 +MAX_ZIP_MEMBER_BYTES = 10 * 1024 * 1024 +MAX_ZIP_TOTAL_BYTES = 50 * 1024 * 1024 +MAX_ZIP_PATH_BYTES = 4096 +MAX_ZIP_COMPONENT_BYTES = 255 +# ``ZipFile`` reads this whole structure into memory. Four MiB leaves roughly +# 8 KiB of filename/extra/comment metadata for each of the 512 allowed entries. +MAX_ZIP_CENTRAL_DIRECTORY_BYTES = 4 * 1024 * 1024 READ_CHUNK_SIZE = 64 * 1024 -# Tighter ceiling for responses that are read fully into memory and parsed as +# Tighter ceilings for responses that are read fully into memory and parsed as # JSON. The 50 MiB MAX_DOWNLOAD_BYTES default is sized for archive/payload -# downloads; JSON metadata responses are far smaller, so capping them close to -# their real size shrinks the memory-DoS surface and keeps the "too large" -# error reachable (rather than only triggering on tens of MiB). Pass it +# downloads; JSON responses are far smaller, so capping them close to their real +# size shrinks the memory-DoS surface and keeps the "too large" error reachable +# (rather than only triggering on tens of MiB). Pass the matching constant # explicitly at each JSON call site so the intended bound is pinned there. -# METADATA covers fixed-shape single-object responses (an OAuth token, one -# release's metadata): a few KiB in practice, 1 MiB is already generous. +# * METADATA - fixed-shape single-object responses (an OAuth token, one +# release's metadata): a few KiB in practice, 1 MiB is already generous. +# * CATALOG - listings that grow with the number of published items. The +# largest bundled catalog is ~130 KiB today, so 8 MiB leaves ~60x headroom +# for growth while staying well under the download ceiling. MAX_JSON_METADATA_BYTES = 1 * 1024 * 1024 +MAX_JSON_CATALOG_BYTES = 8 * 1024 * 1024 + +_WINDOWS_INVALID_FILENAME_CHARS = frozenset('<>:"|?*') +_WINDOWS_RESERVED_FILENAME = re.compile( + r"^(?:con|prn|aux|nul|conin\$|conout\$|" + r"com[1-9\u00b9\u00b2\u00b3]|lpt[1-9\u00b9\u00b2\u00b3])$", + re.IGNORECASE, +) +_ZIP_EOCD = struct.Struct("<4s4H2LH") +_ZIP_EOCD_SIGNATURE = b"PK\x05\x06" +_ZIP64_LOCATOR_SIGNATURE = b"PK\x06\x07" +_ZIP_CENTRAL_HEADER_SIZE = 46 +_ZIP_CENTRAL_SIGNATURE = b"PK\x01\x02" +_ZIP_LOCAL_HEADER_SIZE = 30 +_ZIP_LOCAL_SIGNATURE = b"PK\x03\x04" +_ZIP_EXTRA_HEADER = struct.Struct(" NoReturn: raise error_type(message) +def _raise_from(error_type: type[ErrorT], message: str, exc: Exception) -> NoReturn: + raise error_type(message) from exc + + +class _ReadLimitExceeded(Exception): + """Internal signal used to keep domain-specific errors at call sites.""" + + +def _validate_non_negative_int(value: int, name: str) -> None: + if isinstance(value, bool) or not isinstance(value, int): + raise TypeError(f"{name} must be an integer") + if value < 0: + raise ValueError(f"{name} must be non-negative") + + +def _validate_max_bytes(max_bytes: int) -> None: + _validate_non_negative_int(max_bytes, "max_bytes") + + +def _read_limited(response, max_bytes: int) -> bytes: + """Read a stream with bounded requests and without retaining fragments.""" + output = io.BytesIO() + total = 0 + limit = max_bytes + 1 + while total < limit: + chunk = response.read(min(READ_CHUNK_SIZE, limit - total)) + if not chunk: + break + total += len(chunk) + if total > max_bytes: + raise _ReadLimitExceeded + output.write(chunk) + return output.getvalue() + + def read_response_limited( response, *, @@ -199,20 +278,619 @@ def read_response_limited( explicit value so the intended bound is pinned at the call site rather than tracking changes to the shared default. """ - if isinstance(max_bytes, bool) or not isinstance(max_bytes, int): - raise TypeError("max_bytes must be an integer") - if max_bytes < 0: - raise ValueError("max_bytes must be non-negative") + _validate_max_bytes(max_bytes) + try: + return _read_limited(response, max_bytes) + except _ReadLimitExceeded: + _raise(error_type, f"{label!r} exceeds maximum size of {max_bytes} bytes") - output = io.BytesIO() - total = 0 - limit = max_bytes + 1 - while total < limit: - chunk = response.read(min(READ_CHUNK_SIZE, limit - total)) - if not chunk: - break - total += len(chunk) - if total > max_bytes: - _raise(error_type, f"{label} exceeds maximum size of {max_bytes} bytes") - output.write(chunk) - return output.getvalue() + +def build_safe_download_path( + target_dir: Path, + identifier: object, + version: object, + *, + error_type: type[ErrorT] = ValueError, + label: str = "archive", +) -> Path: + """Build a portable single-component archive path inside *target_dir*.""" + if not isinstance(identifier, str) or not isinstance(version, str): + _raise( + error_type, + f"Unsafe {label} download filename derived from " + f"{identifier!r} and {version!r}", + ) + + filename = f"{identifier}-{version}.zip" + try: + filename_too_long = ( + len(filename.encode("utf-8")) > MAX_ZIP_COMPONENT_BYTES + ) + except UnicodeEncodeError: + filename_too_long = True + posix_path = PurePosixPath(filename) + windows_path = PureWindowsPath(filename) + if ( + filename_too_long + or posix_path.name != filename + or windows_path.name != filename + or any(unicodedata.category(character) == "Cc" for character in filename) + or any( + character in _WINDOWS_INVALID_FILENAME_CHARS + for character in filename + ) + or filename.endswith((" ", ".")) + ): + _raise( + error_type, + f"Unsafe {label} download filename derived from " + f"{identifier!r} and {version!r}", + ) + return Path(target_dir) / filename + + +def read_zip_member_limited( + zf: zipfile.ZipFile, + name: str, + *, + max_bytes: int = MAX_ZIP_MEMBER_BYTES, + error_type: type[ErrorT] = ValueError, + label: str | None = None, +) -> bytes: + """Read a single ZIP member into memory under a hard size cap. + + Reading a member with ``zf.open(name).read()`` is unbounded: a crafted + archive can declare a tiny ``file_size`` yet decompress to many gigabytes (a + "zip bomb"), exhausting memory before the caller ever inspects the data. + This rejects members whose *declared* size already exceeds *max_bytes* and, + to defend against headers that lie, also reads in bounded chunks and stops + one byte past the limit. + + Use this for any inline manifest/metadata read that happens *before* + :func:`safe_extract_zip` (which already enforces the same per-member bound + during extraction); a raw ``zf.open(...).read()`` bypasses that protection. + """ + _validate_max_bytes(max_bytes) + member_label = label or name + try: + info = zf.getinfo(name) + except KeyError as exc: + _raise_from(error_type, f"ZIP member not found: {name!r}", exc) + if info.file_size > max_bytes: + _raise( + error_type, + f"ZIP member {member_label!r} exceeds maximum size of {max_bytes} bytes", + ) + + try: + with zf.open(name, "r") as source: + return _read_limited(source, max_bytes) + except _ReadLimitExceeded: + _raise( + error_type, + f"ZIP member {member_label!r} exceeds maximum size of {max_bytes} bytes", + ) + except Exception as exc: + _raise_from( + error_type, + f"Failed to read ZIP member {member_label!r}: {exc!r}", + exc, + ) + + +def normalize_zip_member_name( + name: str, + *, + error_type: type[ErrorT] = ValueError, +) -> str: + """Return a normalized, portable ZIP member name or raise if unsafe.""" + if "\x00" in name: + _raise(error_type, f"Unsafe path in ZIP archive: {name!r}") + + normalized = name.replace("\\", "/") + try: + encoded_name = normalized.encode("utf-8") + except UnicodeEncodeError: + _raise(error_type, f"Unsafe path in ZIP archive: {name!r}") + if len(encoded_name) > MAX_ZIP_PATH_BYTES: + _raise( + error_type, + f"Unsafe path in ZIP archive: {name!r} " + "(not portable across supported filesystems)", + ) + path = PurePosixPath(normalized) + raw_parts = normalized.split("/") + # Strip a single trailing empty segment, i.e. the one-slash directory + # marker that legitimate ZIPs use ("mydir/", "mydir/subdir/"). Anything + # else that produces an empty segment - consecutive slashes ("a//b") or a + # second trailing slash - is left in place and rejected below as malformed. + if raw_parts and raw_parts[-1] == "": + raw_parts = raw_parts[:-1] + has_windows_drive = re.match(r"^[A-Za-z]:", normalized) is not None + if ( + not raw_parts + or path.is_absolute() + or has_windows_drive + or any(part in {"", ".", ".."} for part in raw_parts) + ): + _raise( + error_type, + f"Unsafe path in ZIP archive: {name!r} (potential path traversal)", + ) + for part in raw_parts: + reserved_stem = part.partition(".")[0].partition(":")[0].rstrip(" ") + if ( + len(part.encode("utf-8")) > MAX_ZIP_COMPONENT_BYTES + or any( + unicodedata.category(character) == "Cc" + for character in part + ) + or any(character in _WINDOWS_INVALID_FILENAME_CHARS for character in part) + or part.startswith(" ") + or part.endswith((" ", ".")) + or _WINDOWS_RESERVED_FILENAME.fullmatch(reserved_stem) + ): + _raise( + error_type, + f"Unsafe path in ZIP archive: {name!r} " + "(not portable across supported filesystems)", + ) + return normalized + + +def portable_zip_path_key(name: str) -> tuple[str, ...]: + """Return a comparison key for filesystems with case/Unicode folding.""" + normalized_name = name.replace("\\", "/") + return tuple( + unicodedata.normalize("NFC", part.casefold()) + for part in normalized_name.removesuffix("/").split("/") + ) + + +def _raise_zip64(error_type: type[ErrorT]) -> NoReturn: + _raise( + error_type, + "ZIP64 archives are not supported by the bounded extractor", + ) + + +def _preflight_zip_entry_features( + extract_version: int, + compression_method: int, + *, + error_type: type[ErrorT], +) -> None: + """Enforce the formats whose output can be bounded by ``ZipExtFile``. + + Python's BZIP2 and LZMA ``ZipExtFile`` paths do not pass the requested + output length to the decompressor; only STORED and DEFLATED preserve this + module's hard memory bound. APPNOTE assigns extract version 4.5 to ZIP64 + size extensions. Because this field declares the minimum extractor feature + level, reject 4.5 and every newer level for the supported methods, + independently of the usual size sentinels and extra field. + """ + if compression_method not in _BOUNDED_ZIP_COMPRESSION_METHODS: + _raise( + error_type, + f"Unsupported ZIP compression method {compression_method}; " + "the bounded extractor supports only STORED and DEFLATED", + ) + if extract_version >= _ZIP64_MIN_EXTRACT_VERSION: + _raise( + error_type, + "ZIP64 or newer ZIP features requiring extractor version 4.5 or " + "newer are not supported by the bounded extractor", + ) + + +def _reject_zip64_extra_fields( + extra: bytes, + zip_path: Path, + *, + error_type: type[ErrorT], +) -> None: + """Reject ZIP64 extra fields and malformed complete extra records.""" + offset = 0 + while offset + _ZIP_EXTRA_HEADER.size <= len(extra): + field_id, field_size = _ZIP_EXTRA_HEADER.unpack_from(extra, offset) + field_end = offset + _ZIP_EXTRA_HEADER.size + field_size + if field_id == _ZIP64_EXTRA_FIELD_ID: + _raise_zip64(error_type) + if field_end > len(extra): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + offset = field_end + + +def _preflight_zip_local_header( + archive_file, + zip_path: Path, + *, + error_type: type[ErrorT], + archive_prefix_size: int, + central_directory_start: int, + local_header_offset: int, +) -> None: + """Reject local-entry ZIP64 indicators before ``ZipFile`` is constructed.""" + physical_offset = archive_prefix_size + local_header_offset + if ( + physical_offset < archive_prefix_size + or physical_offset + _ZIP_LOCAL_HEADER_SIZE > central_directory_start + ): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + archive_file.seek(physical_offset) + header = archive_file.read(_ZIP_LOCAL_HEADER_SIZE) + if ( + len(header) != _ZIP_LOCAL_HEADER_SIZE + or header[:4] != _ZIP_LOCAL_SIGNATURE + ): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + extract_version = struct.unpack_from(" central_directory_start: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + archive_file.seek(extra_offset) + extra = archive_file.read(extra_size) + if len(extra) != extra_size: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + _reject_zip64_extra_fields(extra, zip_path, error_type=error_type) + + +def _preflight_zip_central_directory( + archive_file, + zip_path: Path, + *, + error_type: type[ErrorT], + max_entries: int, +) -> None: + """Bound and count the central directory before ``ZipFile`` materializes it.""" + archive_file.seek(0, 2) + file_size = archive_file.tell() + tail_size = min(file_size, _ZIP_EOCD.size + _ZIP_MAX_COMMENT_BYTES) + archive_file.seek(file_size - tail_size) + tail = archive_file.read(tail_size) + + # ZipFile selects the last EOCD signature in the search window. Inspect + # exactly that record too: falling back to an earlier signature would let + # the preflight validate one central directory while ZipFile materializes + # another. + eocd_index = tail.rfind(_ZIP_EOCD_SIGNATURE) + if eocd_index < 0 or eocd_index + _ZIP_EOCD.size > len(tail): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + eocd = _ZIP_EOCD.unpack_from(tail, eocd_index) + comment_size = eocd[-1] + if eocd_index + _ZIP_EOCD.size + comment_size != len(tail): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + eocd_offset = file_size - len(tail) + eocd_index + if eocd_offset >= 20: + archive_file.seek(eocd_offset - 20) + if archive_file.read(4) == _ZIP64_LOCATOR_SIGNATURE: + _raise_zip64(error_type) + + ( + _signature, + disk_number, + central_directory_disk, + entries_on_disk, + declared_entries, + central_directory_size, + central_directory_offset, + _comment_size, + ) = eocd + if ( + disk_number != 0 + or central_directory_disk != 0 + or entries_on_disk != declared_entries + ): + _raise(error_type, "Multi-disk ZIP archives are not supported") + if ( + declared_entries == _ZIP_UINT16_MAX + or central_directory_size == _ZIP_UINT32_MAX + or central_directory_offset == _ZIP_UINT32_MAX + ): + _raise_zip64(error_type) + if declared_entries > max_entries: + _raise( + error_type, + f"ZIP archive contains too many entries " + f"({declared_entries} > {max_entries})", + ) + if central_directory_size > MAX_ZIP_CENTRAL_DIRECTORY_BYTES: + _raise( + error_type, + f"ZIP central directory exceeds maximum size of " + f"{MAX_ZIP_CENTRAL_DIRECTORY_BYTES} bytes", + ) + + central_directory_start = eocd_offset - central_directory_size + if ( + central_directory_start < 0 + or central_directory_offset > central_directory_start + ): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + archive_prefix_size = central_directory_start - central_directory_offset + + consumed = 0 + actual_entries = 0 + local_header_offsets: list[int] = [] + while consumed < central_directory_size: + archive_file.seek(central_directory_start + consumed) + remaining = central_directory_size - consumed + if remaining < _ZIP_CENTRAL_HEADER_SIZE: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + header = archive_file.read(_ZIP_CENTRAL_HEADER_SIZE) + if ( + len(header) != _ZIP_CENTRAL_HEADER_SIZE + or header[:4] != _ZIP_CENTRAL_SIGNATURE + ): + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + extract_version = struct.unpack_from(" remaining: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + variable_data = archive_file.read(variable_size) + if len(variable_data) != variable_size: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + extra = variable_data[filename_size : filename_size + extra_size] + _reject_zip64_extra_fields(extra, zip_path, error_type=error_type) + local_header_offsets.append(local_header_offset) + + consumed += record_size + actual_entries += 1 + if actual_entries > max_entries: + _raise( + error_type, + f"ZIP archive contains too many entries " + f"({actual_entries} > {max_entries})", + ) + + if actual_entries != declared_entries: + _raise(error_type, f"Invalid ZIP archive: {zip_path}") + + for local_header_offset in local_header_offsets: + _preflight_zip_local_header( + archive_file, + zip_path, + error_type=error_type, + archive_prefix_size=archive_prefix_size, + central_directory_start=central_directory_start, + local_header_offset=local_header_offset, + ) + + +@contextmanager +def open_zip_bounded( + zip_path: Path, + *, + error_type: type[ErrorT] = ValueError, + max_entries: int = MAX_ZIP_ENTRIES, +) -> Iterator[zipfile.ZipFile]: + """Open an untrusted ZIP after a bounded-memory header preflight.""" + _validate_non_negative_int(max_entries, "max_entries") + zip_path = Path(zip_path) + with ExitStack() as stack: + try: + archive_file = stack.enter_context(zip_path.open("rb")) + except OSError as exc: + _raise_from(error_type, f"Invalid ZIP archive: {zip_path}", exc) + try: + _preflight_zip_central_directory( + archive_file, + zip_path, + error_type=error_type, + max_entries=max_entries, + ) + except OSError as exc: + _raise_from(error_type, f"Invalid ZIP archive: {zip_path}", exc) + try: + archive_file.seek(0) + zf = stack.enter_context(zipfile.ZipFile(archive_file, "r")) + except Exception as exc: + _raise_from(error_type, f"Invalid ZIP archive: {zip_path}", exc) + yield zf + + +def safe_extract_zip( + zip_path: Path, + target_dir: Path, + *, + error_type: type[ErrorT] = ValueError, + max_entries: int = MAX_ZIP_ENTRIES, + max_member_bytes: int = MAX_ZIP_MEMBER_BYTES, + max_total_bytes: int = MAX_ZIP_TOTAL_BYTES, +) -> None: + """Extract a ZIP archive after path, symlink, and size validation.""" + _validate_non_negative_int(max_member_bytes, "max_member_bytes") + _validate_non_negative_int(max_total_bytes, "max_total_bytes") + try: + target_root = target_dir.resolve() + except OSError as exc: + _raise_from(error_type, f"Invalid ZIP extraction target: {target_dir}", exc) + + with open_zip_bounded( + zip_path, + error_type=error_type, + max_entries=max_entries, + ) as zf: + try: + members = zf.infolist() + except zipfile.BadZipFile as exc: + _raise_from(error_type, f"Invalid ZIP archive: {zip_path}", exc) + if len(members) > max_entries: + _raise( + error_type, + f"ZIP archive contains too many entries ({len(members)} > {max_entries})", + ) + + normalized_members: list[tuple[zipfile.ZipInfo, str, bool]] = [] + validated_paths: dict[tuple[str, ...], tuple[str, bool]] = {} + total_size = 0 + for member in members: + normalized_name = normalize_zip_member_name( + member.filename, + error_type=error_type, + ) + is_dir = member.is_dir() or normalized_name.endswith("/") + path_key = portable_zip_path_key(normalized_name) + + existing = validated_paths.get(path_key) + if existing is not None: + _raise( + error_type, + f"Conflicting path in ZIP archive: {member.filename} conflicts " + f"with {existing[0]}", + ) + validated_paths[path_key] = (member.filename, is_dir) + + mode = member.external_attr >> 16 + if stat.S_ISLNK(mode): + _raise(error_type, f"Unsafe symlink in ZIP archive: {member.filename}") + + member_path = (target_dir / normalized_name).resolve() + try: + member_path.relative_to(target_root) + except ValueError: + _raise( + error_type, + f"Unsafe path in ZIP archive: {member.filename} " + "(potential path traversal)", + ) + + if not is_dir: + if member.file_size > max_member_bytes: + _raise( + error_type, + f"ZIP member {member.filename} exceeds maximum size " + f"of {max_member_bytes} bytes", + ) + total_size += member.file_size + if total_size > max_total_bytes: + _raise( + error_type, + f"ZIP archive exceeds maximum uncompressed size " + f"of {max_total_bytes} bytes", + ) + + normalized_members.append((member, normalized_name, is_dir)) + + # Tuple sorting places every path immediately before its descendants. + # One adjacent comparison per entry detects file/directory conflicts + # without repeatedly rebuilding every path prefix. + for ( + (path_key, (original, is_dir)), + (next_key, (next_original, _next_is_dir)), + ) in pairwise(sorted(validated_paths.items())): + if ( + not is_dir + and len(next_key) > len(path_key) + and next_key[: len(path_key)] == path_key + ): + _raise( + error_type, + f"Conflicting path in ZIP archive: {original} conflicts " + f"with {next_original}", + ) + + # The loop above bounds the *declared* total via member.file_size, but a + # crafted archive can understate those headers. Mirror the per-member + # guard below with a cumulative count of the bytes actually written so + # the total-size bound holds even when the headers lie. + total_written = 0 + for member, normalized_name, is_dir in normalized_members: + member_path = target_dir / normalized_name + if is_dir: + try: + member_path.mkdir(parents=True, exist_ok=True) + except OSError as exc: + _raise_from( + error_type, + f"Failed to create ZIP directory {member.filename}: {exc}", + exc, + ) + continue + + try: + member_path.parent.mkdir(parents=True, exist_ok=True) + except OSError as exc: + _raise_from( + error_type, + f"Failed to create parent directory for ZIP member {member.filename}: {exc}", + exc, + ) + written = 0 + # Raised outside the try below: if error_type subclasses OSError or + # RuntimeError, raising inside would re-wrap the limit error as + # "Failed to extract" and lose the size-bound message. + limit_error: str | None = None + try: + with zf.open(member, "r") as source, member_path.open("wb") as dest: + while True: + chunk = source.read(READ_CHUNK_SIZE) + if not chunk: + break + written += len(chunk) + if written > max_member_bytes: + limit_error = ( + f"ZIP member {member.filename} exceeds maximum size " + f"of {max_member_bytes} bytes" + ) + break + total_written += len(chunk) + if total_written > max_total_bytes: + limit_error = ( + f"ZIP archive exceeds maximum uncompressed size " + f"of {max_total_bytes} bytes" + ) + break + dest.write(chunk) + except Exception as exc: + _raise_from( + error_type, + f"Failed to extract ZIP member {member.filename}: {exc}", + exc, + ) + if limit_error is not None: + _raise(error_type, limit_error) diff --git a/src/specify_cli/_utils.py b/src/specify_cli/_utils.py index de7836698e..85b659d67b 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -12,6 +12,7 @@ from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any from ._console import console +from ._download_security import normalize_zip_member_name CLAUDE_LOCAL_PATH = Path.home() / ".claude" / "local" / "claude" CLAUDE_NPM_LOCAL_PATH = Path.home() / ".claude" / "local" / "node_modules" / ".bin" / "claude" @@ -27,19 +28,22 @@ def relative_extension_path_violation(value: Any) -> str | None: ``None`` when it is an acceptable relative path within the extension directory. - Policy: the value must be a non-empty string with no leading/trailing - whitespace, no absolute/anchored form, and no ``..`` traversal. The value is + Policy: the value must be a non-empty, portable file path with no + leading/trailing whitespace, absolute/anchored form, ``..`` traversal, + platform-reserved component, or directory-only suffix. The value is evaluated under both POSIX and Windows path semantics because a native ``Path`` is OS-dependent (a ``PurePosixPath`` on POSIX does not interpret - Windows drive/UNC forms, and ``C:foo`` is anchored but not ``is_absolute()`` - yet resolves against the CWD on its drive). Rejecting any non-empty anchor - covers POSIX-absolute (``/abs``), Windows drive-relative (``C:foo``), Windows - absolute (``C:\\foo``), and UNC/rooted forms. + Windows drive/UNC forms, and ``C:foo`` is anchored but not + ``is_absolute()`` yet resolves against the CWD on its drive). Rejecting any + non-empty anchor covers POSIX-absolute (``/abs``), Windows drive-relative + (``C:foo``), Windows absolute (``C:\\foo``), and UNC/rooted forms. """ if not isinstance(value, str) or not value: return "must be a non-empty string" if value.strip() != value: return "must not have leading or trailing whitespace" + if "\\" in value: + return "must use forward slashes as path separators" posix_path = PurePosixPath(value) win_path = PureWindowsPath(value) if ( @@ -52,6 +56,15 @@ def relative_extension_path_violation(value: Any) -> str | None: "must be a relative path within the extension directory " "(no absolute paths, drive letters, or '..' segments)" ) + if value.endswith(("/", "\\")): + return "must name a file or command, not a directory" + try: + normalize_zip_member_name(value) + except ValueError: + return ( + "must use portable path components " + "(no reserved names or platform-invalid characters)" + ) return None diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 6f71aa3682..41a97cca7a 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -675,6 +675,23 @@ def register_commands( cmd_name = cmd_info["name"] aliases = cmd_info.get("aliases", []) cmd_file = cmd_info["file"] + name_reason = relative_extension_path_violation(cmd_name) + if name_reason: + raise ValueError( + f"Invalid command name {cmd_name!r}: {name_reason}" + ) + if aliases is None: + aliases = [] + if not isinstance(aliases, list): + raise ValueError( + f"Aliases for command {cmd_name!r} must be a list" + ) + for alias in aliases: + alias_reason = relative_extension_path_violation(alias) + if alias_reason: + raise ValueError( + f"Invalid command alias {alias!r}: {alias_reason}" + ) # Guard against path traversal using the single shared policy in # relative_extension_path_violation(), so the runtime guard stays @@ -957,10 +974,16 @@ def write_copilot_prompt(project_root: Path, cmd_name: str) -> None: project_root: Path to project root cmd_name: Command name (e.g. 'speckit.my-ext.example') """ + name_reason = relative_extension_path_violation(cmd_name) + if name_reason: + raise ValueError( + f"Invalid Copilot prompt name {cmd_name!r}: {name_reason}" + ) 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.parent.mkdir(parents=True, exist_ok=True) prompt_file.write_text(f"---\nagent: {cmd_name}\n---\n", encoding="utf-8") @staticmethod diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index 42485fd1a8..f763a21c65 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -152,6 +152,8 @@ def add_source( # keeps that ValueError inside the guard instead of leaking a raw # traceback past the CLI's `except BundlerError`. Reuse the value below. hostname = parsed.hostname + # Accessing ``port`` performs urllib's syntax/range validation. + _ = parsed.port except ValueError as exc: raise BundlerError(f"Invalid catalog url: '{url}'.") from exc if not (parsed.scheme or parsed.path): diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index 88a08818f9..53e83a52e7 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -144,6 +144,7 @@ class CatalogEntry: license: str download_url: str requires_speckit_version: str + sha256: str | None = None provides: dict[str, int] = field(default_factory=dict) repository: str | None = None tags: tuple[str, ...] = () @@ -186,6 +187,11 @@ def from_dict(cls, data: Any) -> "CatalogEntry": license=str(data.get("license", "")).strip(), download_url=str(data.get("download_url", "")).strip(), requires_speckit_version=str(requires.get("speckit_version", "")).strip(), + sha256=( + None + if data.get("sha256") is None + else str(data["sha256"]).strip() + ), provides=dict(provides_raw), repository=(str(data["repository"]) if data.get("repository") else None), tags=_parse_tags(data.get("tags"), entry_id), @@ -198,6 +204,7 @@ def with_provenance(self, source: CatalogSource) -> "CatalogEntry": description=self.description, author=self.author, license=self.license, download_url=self.download_url, requires_speckit_version=self.requires_speckit_version, + sha256=self.sha256, provides=self.provides, repository=self.repository, tags=self.tags, verified=self.verified, source_id=source.id, source_policy=source.install_policy, diff --git a/src/specify_cli/bundler/services/adapters.py b/src/specify_cli/bundler/services/adapters.py index 610627b908..403232a7f0 100644 --- a/src/specify_cli/bundler/services/adapters.py +++ b/src/specify_cli/bundler/services/adapters.py @@ -16,6 +16,7 @@ from urllib.request import url2pathname from ..._assets import _locate_core_pack, _repo_root +from ..._download_security import MAX_JSON_CATALOG_BYTES, read_response_limited from .. import BundlerError from ..lib.yamlio import loads_json from ..models.catalog import CatalogSource @@ -76,6 +77,8 @@ def _validate_remote_url(source_id: str, url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname + # Accessing ``port`` performs urllib's syntax/range validation. + _ = parsed.port except ValueError: raise BundlerError( f"Catalog '{source_id}' URL is malformed: {url}" @@ -117,7 +120,15 @@ def make_catalog_fetcher(*, allow_network: bool = True): def fetch(source: CatalogSource) -> dict: url = source.url - parsed = urlparse(url) + try: + parsed = urlparse(url) + # Keep malformed authorities and ports inside the BundlerError + # contract even when a config file was edited by hand. + _ = parsed.port + except ValueError: + raise BundlerError( + f"Catalog {source.id!r} URL is malformed: {url!r}" + ) from None scheme = parsed.scheme.lower() if scheme == "builtin": @@ -180,7 +191,12 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: ) as response: final_url = response.geturl() _validate_remote_url(source_id, final_url) - raw = response.read().decode("utf-8") + raw = read_response_limited( + response, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=BundlerError, + label=f"bundle catalog '{source_id}'", + ).decode("utf-8") except BundlerError: raise except Exception as exc: # noqa: BLE001 diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index d8fb22e511..04ac0b0289 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -14,6 +14,7 @@ import typer from ..._console import console, err_console +from ..._download_security import MAX_DOWNLOAD_BYTES, read_response_limited from ...bundler import BundlerError from ...bundler.lib.project import ( active_integration, @@ -337,6 +338,10 @@ def bundle_install( local_manifest = _local_manifest_source(bundle_id) if local_manifest is not None: manifest = local_manifest + _validate_manifest_structure( + manifest, + source=f"Local bundle source {bundle_id!r}", + ) else: stack = _build_stack(project_root or Path.cwd(), offline=offline) resolved = stack.resolve(bundle_id) @@ -350,6 +355,16 @@ def bundle_install( if project_root is None: init_integration = _resolve_init_integration(integration, manifest) + # Resolve all hard compatibility gates before ``specify init``. + # Otherwise an incompatible but structurally valid bundle would + # initialize a project and only then fail its version/integration + # checks, leaving state behind after a failed install. + resolve_install_plan( + manifest, + speckit_version=_speckit_version(), + active_integration=init_integration, + integration_explicit=True, + ) console.print( f"[cyan]No Spec Kit project here; initializing with integration " f"'{init_integration}'…[/cyan]" @@ -711,17 +726,24 @@ def _local_manifest_source(arg: str): if candidate.suffix == ".zip": import io - import zipfile import yaml as _yaml - with zipfile.ZipFile(candidate) as archive: + from ..._download_security import open_zip_bounded, read_zip_member_limited + + with open_zip_bounded(candidate, error_type=BundlerError) as archive: try: - raw = archive.read("bundle.yml") + archive.getinfo("bundle.yml") except KeyError as exc: raise BundlerError( f"Artifact '{candidate}' does not contain a bundle.yml." ) from exc + raw = read_zip_member_limited( + archive, + "bundle.yml", + error_type=BundlerError, + label="bundle manifest", + ) data = _yaml.safe_load(io.BytesIO(raw)) return BundleManifest.from_dict(data) @@ -805,7 +827,13 @@ def _download_manifest(resolved, *, offline: bool): f"Network access disabled; cannot download bundle '{resolved.entry.id}' " f"from {url}." ) - return _download_remote_manifest(resolved.entry.id, url) + manifest = _download_remote_manifest( + resolved.entry.id, + url, + expected_sha256=getattr(resolved.entry, "sha256", None), + ) + _validate_catalog_manifest(resolved.entry, manifest) + return manifest def _require_https(label: str, url: str) -> None: @@ -817,6 +845,8 @@ def _require_https(label: str, url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname + # Accessing ``port`` performs urllib's syntax/range validation. + _ = parsed.port except ValueError: raise BundlerError( f"Refusing to download {label}: URL is malformed: {url}" @@ -830,7 +860,12 @@ def _require_https(label: str, url: str) -> None: raise BundlerError(f"Refusing to download {label} from URL with no host: {url}") -def _download_remote_manifest(entry_id: str, url: str): +def _download_remote_manifest( + entry_id: str, + url: str, + *, + expected_sha256: str | None = None, +): """Fetch a remote bundle artifact over HTTPS and extract its manifest.""" import io import tempfile @@ -842,6 +877,7 @@ def _download_remote_manifest(entry_id: str, url: str): from ...authentication.http import github_provider_hosts, open_url from ..._github_http import resolve_github_release_asset_api_url from ...bundler.models.manifest import BundleManifest + from ...shared_infra import verify_archive_sha256 def _validate_redirect(old_url: str, new_url: str) -> None: _require_https(f"bundle '{entry_id}'", new_url) @@ -879,7 +915,18 @@ def _validate_redirect(old_url: str, new_url: str) -> None: extra_headers=extra_headers, ) as resp: _require_https(f"bundle '{entry_id}'", resp.geturl()) - raw = resp.read() + raw = read_response_limited( + resp, + max_bytes=MAX_DOWNLOAD_BYTES, + error_type=BundlerError, + label=f"bundle '{entry_id}' download", + ) + verify_archive_sha256( + raw, + expected_sha256, + entry_id, + BundlerError, + ) except BundlerError: raise except Exception as exc: # noqa: BLE001 @@ -940,6 +987,38 @@ def _validate_redirect(old_url: str, new_url: str) -> None: ) from exc +def _validate_manifest_structure(manifest, *, source: str) -> None: + """Reject a malformed manifest before any project mutation can occur.""" + from ...bundler.services.validator import validate_manifest + + report = validate_manifest(manifest) + if report.ok: + return + raise BundlerError( + f"{source} contains an invalid bundle manifest:\n - " + + "\n - ".join(report.errors) + ) + + +def _validate_catalog_manifest(entry, manifest) -> None: + """Bind a downloaded manifest to the catalog identity that selected it.""" + if manifest.bundle.id != entry.id: + raise BundlerError( + f"Downloaded bundle id mismatch: catalog entry {entry.id!r} points to " + f"a manifest for {manifest.bundle.id!r}." + ) + if manifest.bundle.version != entry.version: + raise BundlerError( + f"Downloaded bundle version mismatch for {entry.id!r}: catalog declares " + f"{entry.version!r}, but the manifest declares " + f"{manifest.bundle.version!r}." + ) + _validate_manifest_structure( + manifest, + source=f"Downloaded bundle {entry.id!r}", + ) + + def register(app: typer.Typer) -> None: """Attach the bundle command group to the root Typer app.""" app.add_typer(bundle_app, name="bundle") diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2874012fb7..6cd48582b0 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -17,7 +17,6 @@ import shutil import stat import tempfile -import zipfile from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path @@ -29,6 +28,13 @@ from packaging.specifiers import InvalidSpecifier, SpecifierSet from .._assets import _locate_core_pack, _repo_root +from .._download_security import ( + MAX_JSON_CATALOG_BYTES, + build_safe_download_path, + is_https_or_localhost_http, + read_response_limited, + safe_extract_zip, +) from .._init_options import is_ai_skills_enabled from .._invocation_style import is_dollar_skills_agent, is_slash_skills_agent from .._utils import dump_frontmatter, relative_extension_path_violation, version_satisfies @@ -398,6 +404,12 @@ def _validate(self): raise ValidationError( f"Aliases for command '{cmd['name']}' must be strings" ) + alias_reason = relative_extension_path_violation(alias) + if alias_reason: + raise ValidationError( + f"Invalid alias {alias!r} for command " + f"'{cmd['name']}': {alias_reason}" + ) # Rewrite any hook command references that pointed at a renamed command or # an alias-form ref (ext.cmd → speckit.ext.cmd). Always emit a warning when @@ -804,7 +816,7 @@ def _collect_manifest_command_names(manifest: ExtensionManifest) -> Dict[str, st - primary commands must use this extension's namespace - command namespaces must not shadow core commands - duplicate command/alias names inside one manifest are rejected - - aliases are validated for type and uniqueness only (no pattern enforcement) + - aliases are free-form but must remain safe relative output paths Args: manifest: Parsed extension manifest @@ -841,6 +853,12 @@ def _collect_manifest_command_names(manifest: ExtensionManifest) -> Dict[str, st f"{kind.capitalize()} for command '{primary_name}' must be a string" ) + path_reason = relative_extension_path_violation(name) + if path_reason: + raise ValidationError( + f"Invalid {kind} {name!r}: {path_reason}" + ) + # Enforce canonical pattern only for primary command names; # aliases are free-form to preserve community extension compat. if kind == "command": @@ -1006,18 +1024,21 @@ def _ignore(directory: str, entries: List[str]) -> Set[str]: return _ignore - def _get_skills_dir(self) -> Optional[Path]: + def _get_skills_dir(self, *, create: bool = True) -> Optional[Path]: """Return the active skills directory for extension skill registration. Delegates to :func:`resolve_active_skills_dir` which reads init-options, applies the Kimi native-skills fallback, and - safely creates the directory when ``ai_skills`` is enabled. + safely creates the directory when ``ai_skills`` is enabled and + ``create`` is true. Read-only callers can pass ``create=False`` to + resolve the configured target without changing the filesystem. Returns ``None`` (instead of raising) when the directory cannot be created due to symlink, containment, or permission issues so that callers can fall back gracefully. """ from .. import ( + _get_skills_dir as resolve_configured_skills_dir, _print_cli_warning, load_init_options, resolve_active_skills_dir, @@ -1039,6 +1060,41 @@ def _ensure_usable(skills_dir: Path) -> Optional[Path]: return None return skills_dir + opts = load_init_options(self.project_root) + if not isinstance(opts, dict): + return None + selected_ai = opts.get("ai") + if not isinstance(selected_ai, str) or not selected_ai: + return None + + from ..agents import CommandRegistrar + + registrar = CommandRegistrar() + agent_config = registrar.AGENT_CONFIGS.get(selected_ai) + ai_skills_enabled = is_ai_skills_enabled(opts) + if not create: + if not ai_skills_enabled and selected_ai != "kimi": + return None + configured_skills_dir = resolve_configured_skills_dir( + self.project_root, selected_ai + ) + from ..shared_infra import _validate_safe_shared_directory + + try: + _validate_safe_shared_directory( + self.project_root, configured_skills_dir + ) + except (OSError, ValueError): + return None + skills_dir = configured_skills_dir + if agent_config and agent_config.get("extension") == "/SKILL.md": + skills_dir = registrar._resolve_agent_dir( + selected_ai, agent_config, self.project_root + ) + if ai_skills_enabled: + return skills_dir + return skills_dir if skills_dir.is_dir() else None + try: skills_dir = resolve_active_skills_dir(self.project_root) except (ValueError, OSError) as exc: @@ -1053,24 +1109,96 @@ def _ensure_usable(skills_dir: Path) -> Optional[Path]: if skills_dir is None: return None - opts = load_init_options(self.project_root) - if not isinstance(opts, dict): - return _ensure_usable(skills_dir) - selected_ai = opts.get("ai") - if not isinstance(selected_ai, str) or not selected_ai: - return _ensure_usable(skills_dir) - - from ..agents import CommandRegistrar - - registrar = CommandRegistrar() - agent_config = registrar.AGENT_CONFIGS.get(selected_ai) if agent_config and agent_config.get("extension") == "/SKILL.md": - agent_skills_dir = registrar._resolve_agent_dir( + skills_dir = registrar._resolve_agent_dir( selected_ai, agent_config, self.project_root ) - return _ensure_usable(agent_skills_dir) return _ensure_usable(skills_dir) + @staticmethod + def _skill_name_for_command(command_name: str) -> str: + """Return the generated skill directory name for an extension command.""" + short_name = command_name + if short_name.startswith("speckit."): + short_name = short_name[len("speckit.") :] + return f"speckit-{short_name.replace('.', '-')}" + + def _active_command_registration_scope(self) -> Optional[set[str]]: + """Return the agents a new extension install may render commands for. + + ``None`` means legacy detection-based registration when init-options + is absent. An empty set means registration must fail closed. + """ + from .. import load_init_options + from .._init_options import ( + MISSING_INIT_OPTIONS_FILE, + resolve_active_agent_for_registration, + ) + + active_agent = resolve_active_agent_for_registration(self.project_root) + if active_agent is MISSING_INIT_OPTIONS_FILE: + return None + if active_agent is None: + return set() + + from ..agents import CommandRegistrar as AgentRegistrar + + agent_config = AgentRegistrar().AGENT_CONFIGS.get(active_agent) + if ( + agent_config + and is_ai_skills_enabled(load_init_options(self.project_root)) + and agent_config.get("extension") != "/SKILL.md" + ): + # Command-backed integrations render extension artifacts through + # _register_extension_skills while their skills mode is active. + return set() + return {active_agent} + + def _command_registration_targets(self) -> Dict[str, Path]: + """Return current or recoverable command roots for a new install.""" + from ..agents import CommandRegistrar as AgentRegistrar + + registrar = AgentRegistrar() + agent_scope = self._active_command_registration_scope() + active_skills_agent = registrar._active_skills_agent(self.project_root) + recoverable_active_skills_dir = ( + self._get_skills_dir(create=False) + if active_skills_agent is not None + else None + ) + targets: Dict[str, Path] = {} + + for agent_name, agent_config in registrar.AGENT_CONFIGS.items(): + if agent_scope is not None and agent_name not in agent_scope: + continue + + active_skills_output = ( + agent_name == active_skills_agent + and agent_config.get("extension") == "/SKILL.md" + ) + commands_dir = registrar._resolve_agent_dir( + agent_name, agent_config, self.project_root + ) + active_output_is_recoverable = ( + active_skills_output + and recoverable_active_skills_dir is not None + and registrar._same_lexical_path( + commands_dir, recoverable_active_skills_dir + ) + ) + detect_dir = agent_config.get("detect_dir") + if ( + detect_dir + and not (self.project_root / detect_dir).is_dir() + and not active_output_is_recoverable + ): + continue + + if commands_dir.is_dir() or active_output_is_recoverable: + targets[agent_name] = commands_dir + + return targets + def _register_commands_for_active_agent( self, manifest: ExtensionManifest, @@ -1103,16 +1231,10 @@ def _register_commands_for_active_agent( Mapping of agent name to registered command names, matching the ``registered_commands`` registry shape. """ - from .. import load_init_options - from .._init_options import ( - MISSING_INIT_OPTIONS_FILE, - resolve_active_agent_for_registration, - ) - registrar = CommandRegistrar() - active_agent = resolve_active_agent_for_registration(self.project_root) + agent_scope = self._active_command_registration_scope() - if active_agent is MISSING_INIT_OPTIONS_FILE: + if agent_scope is None: return registrar.register_commands_for_all_agents( manifest, extension_dir, @@ -1121,30 +1243,13 @@ def _register_commands_for_active_agent( create_missing_active_skills_dir=True, ) - if active_agent is None: + if not agent_scope: # init-options.json exists but could not provide a valid active - # agent (corrupted/unreadable/non-object JSON, or a malformed - # "ai" value). Fail closed instead of falling back to all agents - # or passing a non-string key into AGENT_CONFIGS.get() below, - # which would raise TypeError for unhashable values like a list. + # agent, or the active command-backed integration is in skills + # mode. Fail closed instead of falling back to all agents. return {} - init_options = load_init_options(self.project_root) - - # A recorded active key with no registrar config (e.g. "generic", - # deliberately excluded from AGENT_CONFIGS) has nothing to register - # through this path, but it is still an active integration. Passing - # it as only_agent below naturally yields no matches instead of - # falling back to registering every detected agent. - agent_config = registrar.AGENT_CONFIGS.get(active_agent) - if ( - agent_config - and is_ai_skills_enabled(init_options) - and agent_config.get("extension") != "/SKILL.md" - ): - # Active agent runs skills mode: extension artifacts render as - # skills via _register_extension_skills, not as command files. - return {} + active_agent = next(iter(agent_scope)) # Route through the all-agents pass restricted to the active agent so # detection and missing-skills-dir recovery safeguards still apply. @@ -1244,10 +1349,7 @@ def _replacement(match: re.Match[str]) -> str: # Derive skill name from command name using the same hyphenated # convention as hook rendering and preset skill registration. - short_name_raw = cmd_name - if short_name_raw.startswith("speckit."): - short_name_raw = short_name_raw[len("speckit.") :] - skill_name = f"speckit-{short_name_raw.replace('.', '-')}" + skill_name = self._skill_name_for_command(cmd_name) # Check if skill already exists before creating the directory skill_subdir = skills_dir / skill_name @@ -1255,6 +1357,9 @@ def _replacement(match: re.Match[str]) -> str: cache_root = extension_dir / ".specify-dev" / "extension-skills" cache_file = cache_root / skill_name / "SKILL.md" use_dev_symlink = link_outputs and not agent_config.get("dev_no_symlink") + skill_dir_preexists = ( + skill_subdir.exists() or skill_subdir.is_symlink() + ) CommandRegistrar._ensure_inside(cache_file, cache_root) if skill_file.exists() or skill_file.is_symlink(): is_expected_dev_symlink = self._is_expected_dev_symlink( @@ -1265,6 +1370,11 @@ def _replacement(match: re.Match[str]) -> str: # to be refreshed on a subsequent dev install. if not is_expected_dev_symlink: continue + elif skill_dir_preexists: + # Never add files to a pre-existing user directory. Without a + # verifiable SKILL.md ownership marker, rollback/removal cannot + # distinguish our output from unrelated user artifacts. + continue # Create skill directory; track whether we created it so we can clean # up safely if reading the source file subsequently fails. @@ -1357,6 +1467,97 @@ def _is_expected_dev_symlink(skill_file: Path, cache_file: Path) -> bool: except OSError: return False + def _find_extension_skill_dirs( + self, + skill_names: List[str], + extension_id: str, + skills_dir: Optional[Path] = None, + *, + create_skills_dir: bool = True, + ) -> List[Path]: + """Return owned skill directories that removal is allowed to delete. + + This is the single discovery path used by both update backups and + unregistration. Keeping the ownership and containment checks shared + prevents rollback from backing up a different set of artifacts than + ``remove()`` later deletes. + """ + if not skill_names: + return [] + + requested_skills_dir = skills_dir + + project_root = Path(os.path.abspath(self.project_root)) + fallback_candidates = { + candidate: trusted_root + for candidate, trusted_root in self._extension_skill_candidate_dirs().items() + if trusted_root == project_root + } + if requested_skills_dir is None: + candidate_dirs = dict(fallback_candidates) + elif skills_dir: + candidate = Path(os.path.abspath(skills_dir)) + trusted_root = self._extension_skill_trusted_root(candidate) + candidate_dirs = ( + {candidate: trusted_root} if trusted_root is not None else {} + ) + else: + candidate_dirs = {} + + from ..shared_infra import _validate_safe_shared_directory + + owned_dirs: List[Path] = [] + seen_dirs: set[Path] = set() + for skills_candidate, trusted_root in candidate_dirs.items(): + try: + # Validate roots lexically before resolving them. Otherwise a + # symlinked root resolves to its target and makes descendants + # appear contained within itself. Explicit configured roots can + # legitimately be global (for example Hermes), while fallback + # roots remain restricted to this project. + _validate_safe_shared_directory(trusted_root, skills_candidate) + except (OSError, ValueError): + continue + if not skills_candidate.is_dir(): + continue + for skill_name in skill_names: + # Guard against path traversal from a corrupted registry entry. + sn_path = Path(skill_name) + if sn_path.is_absolute() or len(sn_path.parts) != 1: + continue + skill_subdir = skills_candidate / skill_name + try: + _validate_safe_shared_directory(trusted_root, skill_subdir) + resolved_skill_dir = skill_subdir.resolve() + except (OSError, ValueError): + continue + if resolved_skill_dir in seen_dirs or not skill_subdir.is_dir(): + continue + + skill_md = skill_subdir / "SKILL.md" + if not skill_md.is_file(): + continue + try: + from ..agents import CommandRegistrar as _Registrar + + raw = skill_md.read_text(encoding="utf-8") + fm, _ = _Registrar.parse_frontmatter(raw) + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) + if source != f"extension:{extension_id}": + continue + except Exception: + # If ownership cannot be verified, preserve the directory. + continue + + seen_dirs.add(resolved_skill_dir) + owned_dirs.append(resolved_skill_dir) + + return owned_dirs + def _extension_skill_trusted_root(self, candidate: Path) -> Optional[Path]: """Return the project or home root allowed to contain *candidate*.""" candidate = Path(os.path.abspath(candidate)) @@ -1428,157 +1629,10 @@ def _unregister_extension_skills( every configured agent's skills directory is scanned instead of resolving just the currently active one. """ - if not skill_names: - return - - from ..shared_infra import _validate_safe_shared_directory - - if skills_dir: - # Reject the candidate directory itself (any path component, - # including the final one) if it's a symlink escaping the - # trusted project/home root, before probing or deleting anything - # inside it. - # A caller-supplied skills_dir (e.g. a specific agent's - # directory resolved without side effects) could have been - # replaced with a symlink between registration and removal; - # resolving it and only checking children relative to the - # already-resolved candidate (the previous approach) would - # silently follow the symlink instead of rejecting it. - trusted_root = self._extension_skill_trusted_root(skills_dir) - if trusted_root is None: - return - try: - _validate_safe_shared_directory(trusted_root, skills_dir) - except (ValueError, OSError): - return - - # Fast path: we know the exact skills directory - for skill_name in skill_names: - # Guard against path traversal from a corrupted registry entry: - # reject names that are absolute, contain path separators, or - # resolve to a path outside the skills directory. - sn_path = Path(skill_name) - if sn_path.is_absolute() or len(sn_path.parts) != 1: - continue - skill_subdir = skills_dir / skill_name - # Validate every path component down to the skill's own - # subdirectory, not just the already-validated parent - # skills_dir: a per-skill child can itself be a symlink to - # another directory whose *resolved* target still lands - # inside this same (safe) skills root, which the previous - # resolve()+relative_to() containment check alone would - # not catch. Reject the symlink outright rather than - # following it, even when the target is otherwise - # in-bounds (#2948). - try: - _validate_safe_shared_directory(trusted_root, skill_subdir) - except (ValueError, OSError): - continue - if not skill_subdir.is_dir(): - continue - # Safety check: only delete if SKILL.md exists and its - # metadata.source matches exactly this extension — mirroring - # the fallback branch — so a corrupted registry entry cannot - # delete an unrelated user skill. - skill_md = skill_subdir / "SKILL.md" - if not skill_md.is_file(): - continue - try: - from ..agents import CommandRegistrar as _Registrar - - raw = skill_md.read_text(encoding="utf-8") - # Parse on the ``---`` delimiter *line*, not any ``---`` - # substring: a description containing ``---`` would trip a - # raw ``split("---", 2)`` and hide metadata.source, so this - # extension's own skill would look unrelated and be left - # orphaned. Mirrors the #3590 parse_frontmatter fix. - fm, _ = _Registrar.parse_frontmatter(raw) - source = ( - fm.get("metadata", {}).get("source", "") - if isinstance(fm, dict) - else "" - ) - if source != f"extension:{extension_id}": - continue - except (OSError, UnicodeDecodeError, Exception): - continue - shutil.rmtree(skill_subdir) - else: - # Fallback: scan all possible agent skills directories - for ( - skills_candidate, - trusted_root, - ) in self._extension_skill_candidate_dirs().items(): - # Only project-local skills directories are eligible: the - # flat (non-agent-scoped) registered_skills provenance - # cannot prove a home-directory skill belongs to this - # project, so deleting there could remove another - # project's files. Revisit if registry entries ever record - # the owning project/agent. - if trusted_root != Path(os.path.abspath(self.project_root)): - continue - if not skills_candidate.is_dir(): - continue - # Reject the candidate directory itself (any path - # component) if it's a symlink escaping the project - # root, before probing or deleting anything inside it — - # same guard as the fast path above. - try: - _validate_safe_shared_directory( - trusted_root, skills_candidate - ) - except (ValueError, OSError): - continue - for skill_name in skill_names: - # Same path-traversal guard as the fast path above - sn_path = Path(skill_name) - if sn_path.is_absolute() or len(sn_path.parts) != 1: - continue - skill_subdir = skills_candidate / skill_name - # Validate every path component down to the skill's - # own subdirectory, not just the already-validated - # candidate parent: a per-skill child can itself be a - # symlink to another directory whose resolved target - # still lands inside this same candidate, which the - # previous resolve()+relative_to() containment check - # alone would not catch (#2948). - try: - _validate_safe_shared_directory( - trusted_root, skill_subdir - ) - except (ValueError, OSError): - continue - if not skill_subdir.is_dir(): - continue - # Safety check: only delete if SKILL.md exists and its - # metadata.source matches exactly this extension. If the - # file is missing or unreadable we skip to avoid deleting - # unrelated user-created directories. - skill_md = skill_subdir / "SKILL.md" - if not skill_md.is_file(): - continue - try: - from ..agents import CommandRegistrar as _Registrar - - raw = skill_md.read_text(encoding="utf-8") - # Parse on the ``---`` delimiter *line*, not any ``---`` - # substring: a description containing ``---`` would trip - # a raw ``split("---", 2)`` and hide metadata.source, so - # this extension's own skill would look unrelated and be - # left orphaned. Mirrors the #3590 parse_frontmatter fix. - fm, _ = _Registrar.parse_frontmatter(raw) - source = ( - fm.get("metadata", {}).get("source", "") - if isinstance(fm, dict) - else "" - ) - # Only remove skills explicitly created by this extension - if source != f"extension:{extension_id}": - continue - except (OSError, UnicodeDecodeError, Exception): - # If we can't verify, skip to avoid accidental deletion - continue - shutil.rmtree(skill_subdir) + for skill_subdir in self._find_extension_skill_dirs( + skill_names, extension_id, skills_dir=skills_dir + ): + shutil.rmtree(skill_subdir) def _extension_owned_skill_names( self, skill_names: List[str], extension_id: str @@ -2329,21 +2383,7 @@ def install_from_zip( with tempfile.TemporaryDirectory() as tmpdir: temp_path = Path(tmpdir) - # Extract ZIP safely (prevent Zip Slip attack) - with zipfile.ZipFile(zip_path, "r") as zf: - # Validate all paths first before extracting anything - temp_path_resolved = temp_path.resolve() - for member in zf.namelist(): - member_path = (temp_path / member).resolve() - # Use is_relative_to for safe path containment check - try: - member_path.relative_to(temp_path_resolved) - except ValueError: - raise ValidationError( - f"Unsafe path in ZIP archive: {member} (potential path traversal)" - ) - # Only extract after all paths are validated - zf.extractall(temp_path) + safe_extract_zip(zip_path, temp_path, error_type=ValidationError) # Find extension directory (may be nested) extension_dir = temp_path @@ -3351,7 +3391,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: final_url = response.geturl() if final_url != entry.url: self._validate_catalog_url(final_url) - catalog_data = json.loads(response.read()) + catalog_data = json.loads( + read_response_limited( + response, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=ExtensionError, + label=f"extension catalog {entry.url}", + ) + ) self._validate_catalog_payload(catalog_data, entry.url) @@ -3539,7 +3586,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: final_url = response.geturl() if final_url != catalog_url: self._validate_catalog_url(final_url) - catalog_data = json.loads(response.read()) + catalog_data = json.loads( + read_response_limited( + response, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=ExtensionError, + label=f"extension catalog {catalog_url}", + ) + ) # Validate catalog structure. Reuses the same helper as # ``_fetch_single_catalog`` so all three branches (root type, @@ -3697,6 +3751,10 @@ def download_extension( download_url = ext_info.get("download_url") if not download_url: raise ExtensionError(f"Extension '{extension_id}' has no download URL") + if not isinstance(download_url, str): + raise ExtensionError( + f"Extension download URL is malformed: {download_url}" + ) # Validate download URL requires HTTPS (prevent man-in-the-middle attacks) from urllib.parse import urlparse @@ -3710,12 +3768,16 @@ def download_extension( try: parsed = urlparse(download_url) hostname = parsed.hostname + parsed.port except ValueError: raise ExtensionError( f"Extension download URL is malformed: {download_url}" ) from None - is_localhost = hostname in ("localhost", "127.0.0.1", "::1") - if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost): + if not hostname: + raise ExtensionError( + f"Extension download URL is malformed: {download_url}" + ) + if not is_https_or_localhost_http(download_url): raise ExtensionError( f"Extension download URL must use HTTPS: {download_url}" ) @@ -3723,11 +3785,16 @@ def download_extension( # Determine target path if target_dir is None: target_dir = self.cache_dir / "downloads" - target_dir.mkdir(parents=True, exist_ok=True) - + target_dir = Path(target_dir) version = ext_info.get("version", "unknown") - zip_filename = f"{extension_id}-{version}.zip" - zip_path = target_dir / zip_filename + zip_path = build_safe_download_path( + target_dir, + extension_id, + version, + error_type=ExtensionError, + label="extension", + ) + target_dir.mkdir(parents=True, exist_ok=True) extra_headers = None resolved_download_url = self._resolve_github_release_asset_api_url(download_url) @@ -3740,7 +3807,11 @@ def download_extension( with self._open_url( download_url, timeout=60, extra_headers=extra_headers ) as response: - zip_data = response.read() + zip_data = read_response_limited( + response, + error_type=ExtensionError, + label=f"extension '{extension_id}' download", + ) verify_archive_sha256( zip_data, ext_info.get("sha256"), extension_id, ExtensionError diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index c096de44a6..80b7d2cafc 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -8,12 +8,14 @@ """ from __future__ import annotations +import hashlib import os import shutil import tempfile import zipfile from pathlib import Path from typing import Optional +from uuid import uuid4 import typer import yaml @@ -23,6 +25,15 @@ from .._console import console from .._assets import get_speckit_version +from .._download_security import ( + is_https_or_localhost_http, + normalize_zip_member_name, + open_zip_bounded, + portable_zip_path_key, + read_response_limited, + read_zip_member_limited, +) +from .._init_options import is_ai_skills_enabled extension_app = typer.Typer( name="extension", @@ -443,14 +454,17 @@ def extension_add( # "Invalid URL" message instead of leaking a raw traceback past the # CLI. Reuse the value below. hostname = parsed.hostname + parsed.port except ValueError: console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") raise typer.Exit(1) - is_localhost = hostname in ("localhost", "127.0.0.1", "::1") + if not hostname: + console.print(f"[red]Error:[/red] Invalid URL: {_escape_markup(from_url)}") + raise typer.Exit(1) - if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost): + if not is_https_or_localhost_http(from_url): console.print("[red]Error:[/red] URL must use HTTPS for security.") - console.print("HTTP is only allowed for localhost URLs.") + console.print("HTTP is only allowed for loopback URLs.") raise typer.Exit(1) safe_url = _escape_markup(from_url) @@ -533,7 +547,11 @@ def extension_add( with dl_catalog._open_url( download_url, timeout=60, extra_headers=extra_headers ) as response: - zip_data = response.read() + zip_data = read_response_limited( + response, + error_type=ExtensionError, + label=f"extension {from_url}", + ) if not zipfile.is_zipfile(io.BytesIO(zip_data)): console.print( @@ -1079,6 +1097,7 @@ def extension_update( from . import ( ExtensionManager, ExtensionCatalog, + ExtensionManifest, ExtensionError, ValidationError, CommandRegistrar, @@ -1193,9 +1212,17 @@ def extension_update( console.print(f"📦 Updating {safe_ext_name}...") # Backup paths - backup_base = manager.extensions_dir / ".backup" / f"{extension_id}-update" + backup_root = manager.extensions_dir / ".backup" + backup_key = hashlib.sha256( + extension_id.encode("utf-8") + ).hexdigest()[:16] + backup_base = ( + backup_root + / f"update-{backup_key}-{uuid4().hex}" + ) backup_ext_dir = backup_base / "extension" backup_commands_dir = backup_base / "commands" + backup_skills_dir = backup_base / "skills" backup_config_dir = backup_base / "config" # Store backup state @@ -1203,14 +1230,125 @@ def extension_update( backup_installed = UNSET # Original installed list from extensions.yml backup_hooks = None # None means backup step 4 not yet reached; {} or {...} means backup was captured backed_up_command_files = {} + backed_up_command_symlinks = {} + backed_up_skill_dirs = {} + new_command_dirs_absent_before_update = [] + new_command_paths_absent_before_update = [] + new_skill_names = [] + new_skill_paths_absent_before_update = [] + # Validation failures must not rewrite an untouched installation. + installation_modified = False + zip_cleanup_error = None + backup_created_by_attempt = False + + def backup_command_artifact(original_file, backup_file): + """Back up one command artifact once, preserving its full path.""" + nonlocal backup_created_by_attempt + original_key = str(original_file) + if original_key in backed_up_command_files: + return + if original_file.is_symlink(): + backed_up_command_symlinks[original_key] = os.readlink( + original_file + ) + else: + if original_file.stat().st_nlink > 1: + raise RuntimeError( + "Cannot safely update hard-linked generated " + f"artifact '{original_file}'" + ) + backup_created_by_attempt = True + backup_file.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(original_file, backup_file) + backed_up_command_files[original_key] = str(backup_file) + + def restore_command_artifact(original_path, backup_path): + """Restore one regular file or symlink without following it.""" + original_key = str(original_path) + original_file = Path(original_path) + backup_file = Path(backup_path) + symlink_state = backed_up_command_symlinks.get( + original_key + ) + + if symlink_state is not None: + if original_file.is_symlink() or original_file.is_file(): + original_file.unlink() + elif original_file.exists(): + raise RuntimeError( + "Command rollback found an unexpected directory " + f"at '{original_file}'" + ) + original_file.parent.mkdir(parents=True, exist_ok=True) + os.symlink(symlink_state, original_file) + return + + if not backup_file.is_file() or backup_file.is_symlink(): + raise RuntimeError( + "Command rollback backup is missing for " + f"'{original_file}'" + ) + if original_file.is_symlink() or original_file.is_file(): + original_file.unlink() + elif original_file.exists(): + raise RuntimeError( + "Command rollback found an unexpected directory " + f"at '{original_file}'" + ) + original_file.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(backup_file, original_file) + + def remember_absent_parent_dirs(artifact_path, root_dir): + """Remember absent parents a failed renderer may create.""" + boundary = root_dir.parent + if root_dir.is_relative_to(project_root): + boundary = project_root + parent = artifact_path.parent + while parent != boundary: + if parent.exists() or parent.is_symlink(): + break + new_command_dirs_absent_before_update.append(parent) + parent = parent.parent + + def backup_extension_skills(skill_names, *, skills_dir=None): + """Back up every owned skill directory that remove() may delete.""" + nonlocal backup_created_by_attempt + for skill_dir in manager._find_extension_skill_dirs( + skill_names, + extension_id, + skills_dir=skills_dir, + create_skills_dir=False, + ): + original_key = str(skill_dir) + if original_key in backed_up_skill_dirs: + continue + backup_created_by_attempt = True + backup_skills_dir.mkdir(parents=True, exist_ok=True) + backup_skill_dir = backup_skills_dir / str( + len(backed_up_skill_dirs) + ) + shutil.copytree(skill_dir, backup_skill_dir, symlinks=True) + backed_up_skill_dirs[original_key] = str(backup_skill_dir) try: + if backup_root.is_symlink(): + raise RuntimeError( + "Cannot safely create update backup under symlinked " + f"directory '{backup_root}'" + ) + if backup_base.exists() or backup_base.is_symlink(): + raise RuntimeError( + "Cannot safely reuse an existing update backup " + f"directory '{backup_base}'" + ) + # 1. Backup registry entry (always, even if extension dir doesn't exist) backup_registry_entry = manager.registry.get(extension_id) # 2. Backup extension directory extension_dir = manager.extensions_dir / extension_id if extension_dir.exists(): + backup_created_by_attempt = True backup_base.mkdir(parents=True, exist_ok=True) if backup_ext_dir.exists(): shutil.rmtree(backup_ext_dir) @@ -1234,30 +1372,91 @@ def extension_update( commands_dir = _AgentReg._resolve_agent_dir( agent_name, agent_config, project_root ) + dirs_to_backup = [commands_dir] + legacy = agent_config.get("legacy_dir") + if legacy: + legacy_dir = project_root / legacy + if ( + legacy_dir.exists() + and legacy_dir != commands_dir + ): + dirs_to_backup.append(legacy_dir) for cmd_name in cmd_names: - output_name = _AgentReg._compute_output_name(agent_name, cmd_name, agent_config) - cmd_file = commands_dir / f"{output_name}{agent_config['extension']}" - if cmd_file.exists(): - # Mirror the real on-disk layout under the backup dir. - # Skills agents (extension == "/SKILL.md") name every - # command file "SKILL.md", living in a per-command - # subdir (e.g. speckit-plan/SKILL.md). Using cmd_file.name - # alone would collide all of them onto one backup path and - # break rollback; keep the relative path to stay unique. - backup_cmd_path = backup_commands_dir / agent_name / cmd_file.relative_to(commands_dir) - backup_cmd_path.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(cmd_file, backup_cmd_path) - backed_up_command_files[str(cmd_file)] = str(backup_cmd_path) + output_name = _AgentReg._compute_output_name( + agent_name, cmd_name, agent_config + ) + names_to_backup = [output_name] + if ( + output_name != cmd_name + and _AgentReg._is_safe_command_name(cmd_name) + ): + names_to_backup.append(cmd_name) + + for dir_index, target_dir in enumerate( + dirs_to_backup + ): + for name in names_to_backup: + cmd_file = ( + target_dir + / f"{name}{agent_config['extension']}" + ) + try: + _AgentReg._ensure_inside( + cmd_file, target_dir + ) + except ValueError: + continue + if ( + cmd_file.exists() + or cmd_file.is_symlink() + ): + # Keep both the directory location and + # relative path unique. unregister_commands() + # removes legacy and canonical copies, and + # skills agents place every SKILL.md in its + # own command subdirectory. + backup_cmd_path = ( + backup_commands_dir + / agent_name + / f"location-{dir_index}" + / cmd_file.relative_to(target_dir) + ) + backup_command_artifact( + cmd_file, backup_cmd_path + ) # Also backup copilot prompt files if agent_name == "copilot": - prompt_file = project_root / ".github" / "prompts" / f"{cmd_name}.prompt.md" - if prompt_file.exists(): - backup_prompt_path = backup_commands_dir / "copilot-prompts" / prompt_file.name - backup_prompt_path.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(prompt_file, backup_prompt_path) - backed_up_command_files[str(prompt_file)] = str(backup_prompt_path) + prompts_dir = ( + project_root / ".github" / "prompts" + ) + prompt_file = ( + prompts_dir / f"{cmd_name}.prompt.md" + ) + try: + _AgentReg._ensure_inside( + prompt_file, prompts_dir + ) + except ValueError: + continue + if prompt_file.exists() or prompt_file.is_symlink(): + backup_prompt_path = ( + backup_commands_dir + / "copilot-prompts" + / prompt_file.relative_to(prompts_dir) + ) + backup_command_artifact( + prompt_file, backup_prompt_path + ) + + raw_registered_skills = ( + backup_registry_entry.get("registered_skills", []) + if isinstance(backup_registry_entry, dict) + else [] + ) + registered_skills = manager._valid_name_list(raw_registered_skills) + backup_extension_skills(registered_skills) # 4. Backup hooks and installed list from extensions.yml # get_project_config() always normalizes installed->[] and hooks->{}, @@ -1281,24 +1480,107 @@ def extension_update( try: # 6. Validate extension ID from ZIP BEFORE modifying installation # Handle both root-level and nested extension.yml (GitHub auto-generated ZIPs) - with zipfile.ZipFile(zip_path, "r") as zf: + with open_zip_bounded(zip_path) as zf: import yaml manifest_data = None + manifest_bytes = None namelist = zf.namelist() - # First try root-level extension.yml - if "extension.yml" in namelist: - with zf.open("extension.yml") as f: - parsed_manifest = yaml.safe_load(f) - manifest_data = parsed_manifest if parsed_manifest is not None else {} - else: - # Look for extension.yml in a single top-level subdirectory - # (e.g., "repo-name-branch/extension.yml") - manifest_paths = [n for n in namelist if n.endswith("/extension.yml") and n.count("/") == 1] - if len(manifest_paths) == 1: - with zf.open(manifest_paths[0]) as f: - parsed_manifest = yaml.safe_load(f) - manifest_data = parsed_manifest if parsed_manifest is not None else {} + # Read the manifest under a hard size cap: this happens + # before install_from_zip()'s safe_extract_zip(), so a + # raw zf.open().read() here would bypass that bound and + # let a zip-bomb extension.yml exhaust memory. + # Normalize separators before choosing the manifest so + # this pre-scan cannot approve one entry while extraction + # later overwrites it with a backslash alias. + manifest_candidates = [] + archive_entries = [] + for name in namelist: + normalized_name = normalize_zip_member_name(name) + parts = normalized_name.removesuffix("/").split( + "/" + ) + path_key = portable_zip_path_key(normalized_name) + archive_entries.append( + (normalized_name, parts) + ) + if ( + len(parts) in {1, 2} + and path_key[-1] == "extension.yml" + ): + manifest_candidates.append( + (name, normalized_name, path_key) + ) + + seen_manifest_keys = {} + for name, _normalized_name, path_key in manifest_candidates: + previous = seen_manifest_keys.get(path_key) + if previous is not None: + raise ValueError( + "Downloaded extension archive contains multiple " + "extension.yml manifests" + ) + seen_manifest_keys[path_key] = name + + for _name, normalized_name, _path_key in manifest_candidates: + if normalized_name.split("/")[-1] != "extension.yml": + raise ValueError( + "Downloaded extension archive manifest " + "filenames must use canonical " + "'extension.yml' casing" + ) + + root_manifest = next( + ( + name + for name, _normalized_name, path_key + in manifest_candidates + if path_key == ("extension.yml",) + ), + None, + ) + nested_manifests = [ + (name, normalized_name) + for name, normalized_name, path_key + in manifest_candidates + if len(path_key) == 2 + and path_key[-1] == "extension.yml" + ] + manifest_path = root_manifest + if manifest_path is None and len(nested_manifests) == 1: + manifest_path, normalized_manifest_path = ( + nested_manifests[0] + ) + manifest_root = normalized_manifest_path.split( + "/", 1 + )[0] + top_level_dirs = { + parts[0] + for normalized_name, parts in archive_entries + if ( + len(parts) > 1 + or normalized_name.endswith("/") + ) + } + if top_level_dirs != {manifest_root}: + raise ValueError( + "Downloaded extension archive with a " + "nested extension.yml must contain exactly " + "one top-level directory" + ) + + if manifest_path is not None: + manifest_bytes = read_zip_member_limited( + zf, manifest_path + ) + parsed_manifest = yaml.safe_load( + manifest_bytes + ) + manifest_data = ( + parsed_manifest + if parsed_manifest is not None + else {} + ) if manifest_data is None: raise ValueError("Downloaded extension archive is missing 'extension.yml'") @@ -1312,13 +1594,205 @@ def extension_update( "Invalid extension manifest in downloaded archive: expected 'extension' mapping" ) - zip_extension_id = extension_data.get("id") + # Run the same manifest and compatibility validation as a + # normal install while the existing extension is still + # untouched. Reuse the exact bounded bytes selected above. + if manifest_bytes is None: + raise ValueError( + "Downloaded extension archive is missing 'extension.yml'" + ) + with tempfile.TemporaryDirectory( + prefix="speckit-update-manifest-" + ) as manifest_tmpdir: + manifest_file = Path(manifest_tmpdir) / "extension.yml" + manifest_file.write_bytes(manifest_bytes) + preflight_manifest = ExtensionManifest(manifest_file) + manager.check_compatibility( + preflight_manifest, speckit_version + ) + + zip_extension_id = preflight_manifest.id if zip_extension_id != extension_id: raise ValueError( f"Extension ID mismatch: expected '{extension_id}', got '{zip_extension_id}'" ) + expected_version = pkg_version.Version(update["available"]) + archive_version = pkg_version.Version( + preflight_manifest.version + ) + if archive_version != expected_version: + raise ValueError( + "Extension version mismatch: " + f"expected '{update['available']}', " + f"got '{preflight_manifest.version}'" + ) + + # Match the remaining deterministic install validation + # before crossing the destructive boundary. The helper + # excludes this extension's current registry entry while + # still detecting namespace, core, duplicate, and + # cross-extension command conflicts. + manager._validate_install_conflicts(preflight_manifest) + + new_command_names = list( + manager._collect_manifest_command_names( + preflight_manifest + ) + ) + new_skill_names = list( + dict.fromkeys( + manager._skill_name_for_command(command_name) + for command_name in new_command_names + ) + ) + + # Command rendering happens before hook registration and + # registry.add(). Preserve every candidate output that + # already exists, and remember paths that are absent now so + # rollback can remove files created before registry state is + # available. Include aliases and Copilot companion prompts. + for ( + agent_name, + commands_dir, + ) in manager._command_registration_targets().items(): + agent_config = registrar.AGENT_CONFIGS[agent_name] + for command_name in new_command_names: + output_name = _AgentReg._compute_output_name( + agent_name, command_name, agent_config + ) + command_file = ( + commands_dir + / f"{output_name}{agent_config['extension']}" + ) + _AgentReg._ensure_inside(command_file, commands_dir) + backup_command_path = ( + backup_commands_dir + / agent_name + / command_file.relative_to(commands_dir) + ) + if command_file.exists() or command_file.is_symlink(): + backup_command_artifact( + command_file, backup_command_path + ) + else: + new_command_paths_absent_before_update.append( + command_file + ) + remember_absent_parent_dirs( + command_file, commands_dir + ) + + if agent_name == "copilot": + prompts_dir = ( + project_root / ".github" / "prompts" + ) + prompt_file = ( + prompts_dir / f"{command_name}.prompt.md" + ) + _AgentReg._ensure_inside( + prompt_file, prompts_dir + ) + if prompt_file.is_symlink(): + raise RuntimeError( + "Cannot safely update symlinked Copilot " + f"prompt artifact '{prompt_file}'" + ) + backup_prompt_path = ( + backup_commands_dir + / "copilot-prompts" + / prompt_file.relative_to(prompts_dir) + ) + if ( + prompt_file.exists() + or prompt_file.is_symlink() + ): + backup_command_artifact( + prompt_file, backup_prompt_path + ) + else: + new_command_paths_absent_before_update.append( + prompt_file + ) + remember_absent_parent_dirs( + prompt_file, prompts_dir + ) + + new_command_paths_absent_before_update = list( + dict.fromkeys( + new_command_paths_absent_before_update + ) + ) + new_command_dirs_absent_before_update = list( + dict.fromkeys( + new_command_dirs_absent_before_update + ) + ) + + # A newly introduced command may reuse an existing + # extension-owned skill directory that was not present in + # the old registry. Back it up before cleanup can touch it. + backup_extension_skills(new_skill_names) + new_skills_dir = manager._get_skills_dir(create=False) + if new_skills_dir is not None: + # Unscoped removal deliberately ignores home-scoped + # outputs because the flat registry cannot establish + # project ownership. The active install can still + # replace a marker-owned skill in its explicit root, + # so back up that exact project/home target separately. + backup_extension_skills( + list( + dict.fromkeys( + registered_skills + new_skill_names + ) + ), + skills_dir=new_skills_dir, + ) + init_options = load_init_options(project_root) + if ( + isinstance(init_options, dict) + and is_ai_skills_enabled(init_options) + and isinstance(init_options.get("ai"), str) + and init_options["ai"] + ): + # resolve_active_skills_dir() first creates the + # configured project-local skills marker. Some + # agents (notably Hermes) then redirect rendered + # skills to a different global root, so snapshot + # both locations for exact rollback. + from .. import _get_skills_dir + + configured_skills_dir = _get_skills_dir( + project_root, init_options["ai"] + ) + remember_absent_parent_dirs( + configured_skills_dir / ".update-marker", + configured_skills_dir, + ) + new_skills_root = new_skills_dir.resolve() + for skill_name in new_skill_names: + skill_path = new_skills_dir / skill_name + resolved_skill_path = skill_path.resolve(strict=False) + resolved_skill_path.relative_to(new_skills_root) + if not ( + skill_path.exists() or skill_path.is_symlink() + ): + new_skill_paths_absent_before_update.append( + skill_path + ) + remember_absent_parent_dirs( + skill_path / "SKILL.md", + new_skills_dir, + ) + + new_command_dirs_absent_before_update = list( + dict.fromkeys( + new_command_dirs_absent_before_update + ) + ) + # 7. Remove old extension (handles command file cleanup and registry removal) + installation_modified = True manager.remove(extension_id, keep_config=True) # 8. Install new version @@ -1368,15 +1842,42 @@ def extension_update( hook["enabled"] = False hook_executor.save_project_config(config) finally: - # Clean up downloaded ZIP + # ZIP cleanup is housekeeping: never replace an install + # error or roll back an already committed update because a + # scanner temporarily locks the download on Windows. if zip_path.exists(): - zip_path.unlink() - - # 10. Clean up backup on success - if backup_base.exists(): - shutil.rmtree(backup_base) + try: + zip_path.unlink() + except OSError as error: + zip_cleanup_error = error + + # 10. Clean up backup on success. The update has committed at + # this point, so a locked backup file must not trigger rollback + # of an otherwise successful installation. + cleanup_error = None + if backup_created_by_attempt and backup_base.exists(): + try: + shutil.rmtree(backup_base) + except OSError as error: + cleanup_error = error console.print(f" [green]✓[/green] Updated to v{update['available']}") + if cleanup_error is not None: + console.print( + " [yellow]Warning:[/yellow] Could not fully remove " + "update backup: " + f"{_escape_markup(str(cleanup_error))}" + ) + console.print( + " [dim]Backup may remain at: " + f"{_escape_markup(str(backup_base))}[/dim]" + ) + if zip_cleanup_error is not None: + console.print( + " [yellow]Warning:[/yellow] Could not remove " + "downloaded update archive: " + f"{_escape_markup(str(zip_cleanup_error))}" + ) updated_extensions.append(ext_name) except KeyboardInterrupt: @@ -1384,6 +1885,24 @@ def extension_update( except Exception as e: console.print(f" [red]✗[/red] Failed: {_escape_markup(str(e))}") failed_updates.append((ext_name, str(e))) + if zip_cleanup_error is not None: + console.print( + " [yellow]Warning:[/yellow] Could not remove " + "downloaded update archive: " + f"{_escape_markup(str(zip_cleanup_error))}" + ) + + if not installation_modified: + if backup_created_by_attempt and backup_base.exists(): + try: + shutil.rmtree(backup_base) + except OSError as cleanup_error: + console.print( + " [yellow]Warning:[/yellow] Could not remove " + "untouched-update backup: " + f"{_escape_markup(str(cleanup_error))}" + ) + continue # Rollback on failure console.print(f" [yellow]↩[/yellow] Rolling back {safe_ext_name}...") @@ -1400,13 +1919,28 @@ def extension_update( shutil.copytree(backup_ext_dir, extension_dir) # Remove any NEW command files created by failed install - # (files that weren't in the original backup) + # (files that weren't in the original backup). Registration + # writes before registry.add(), so start with the paths that + # were absent at the destructive boundary instead of relying + # only on a possibly missing new registry entry. + for command_path in new_command_paths_absent_before_update: + if command_path.is_symlink() or command_path.is_file(): + command_path.unlink() + elif command_path.exists(): + raise RuntimeError( + "Command rollback found an unexpected directory " + f"at '{command_path}'" + ) + new_registered_skills = [] try: new_registry_entry = manager.registry.get(extension_id) if new_registry_entry is None or not isinstance(new_registry_entry, dict): new_registered_commands = {} else: new_registered_commands = new_registry_entry.get("registered_commands", {}) + new_registered_skills = manager._valid_name_list( + new_registry_entry.get("registered_skills", []) + ) for agent_name, cmd_names in new_registered_commands.items(): if agent_name not in registrar.AGENT_CONFIGS: continue @@ -1430,13 +1964,78 @@ def extension_update( except KeyError: pass # No new registry entry exists, nothing to clean up - # Restore backed up command files + # Restore command artifacts that existed before the update + # before extension-skill cleanup inspects ownership. A + # failed skills registrar may have overwritten a user's + # pre-existing SKILL.md with extension metadata; restoring + # it first prevents the conservative skill unregistrar from + # misclassifying and deleting the user's whole directory. for original_path, backup_path in backed_up_command_files.items(): - backup_file = Path(backup_path) - if backup_file.exists(): - original_file = Path(original_path) - original_file.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(backup_file, original_file) + restore_command_artifact( + original_path, backup_path + ) + + # Skill generation happens before hooks and registry.add(), + # so a failed install may have created skills that are not + # recorded in any registry entry yet. Derive names from the + # preflighted manifest as well as any partial new entry. + skills_to_remove = list( + dict.fromkeys(new_skill_names + new_registered_skills) + ) + # A write failure can leave a partial skill without valid + # ownership metadata, which the normal conservative + # unregistrar intentionally refuses to delete. Paths that + # were absent at the destructive boundary are safe to + # remove directly during rollback. + for skill_path in new_skill_paths_absent_before_update: + if skill_path.is_symlink() or skill_path.is_file(): + skill_path.unlink() + elif skill_path.exists(): + shutil.rmtree(skill_path) + manager._unregister_extension_skills( + skills_to_remove, extension_id + ) + + # Restore all original registered skill artifacts after + # removing skills created by the failed installation. + for original_path, backup_path in backed_up_skill_dirs.items(): + backup_skill_dir = Path(backup_path) + if not backup_skill_dir.is_dir(): + raise RuntimeError( + "Skill rollback backup is missing for " + f"'{original_path}'" + ) + original_skill_dir = Path(original_path) + if ( + original_skill_dir.is_symlink() + or original_skill_dir.is_file() + ): + original_skill_dir.unlink() + elif original_skill_dir.exists(): + shutil.rmtree(original_skill_dir) + original_skill_dir.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree( + backup_skill_dir, + original_skill_dir, + symlinks=True, + ) + + # Remove empty artifact directories that did not exist at + # the destructive boundary. Do this after skill cleanup and + # restoration so newly created skills roots and their + # project-local parents can also be removed exactly. + for command_dir in sorted( + new_command_dirs_absent_before_update, + key=lambda path: len(path.parts), + reverse=True, + ): + if command_dir.is_dir() and not command_dir.is_symlink(): + try: + command_dir.rmdir() + except OSError: + # Preserve any non-empty directory: other + # content may belong to the user. + pass # Restore metadata in extensions.yml (hooks and installed list). # Only run if backup step 4 was reached (backup_hooks is not None); @@ -1491,10 +2090,26 @@ def extension_update( if backup_registry_entry: manager.registry.restore(extension_id, backup_registry_entry) + # Backup cleanup is post-rollback housekeeping. A locked + # file (notably on Windows) must not turn successfully + # restored state into a contradictory "Rollback failed". + cleanup_error = None + if backup_created_by_attempt and backup_base.exists(): + try: + shutil.rmtree(backup_base) + except OSError as error: + cleanup_error = error console.print(" [green]✓[/green] Rollback successful") - # Clean up backup directory only on successful rollback - if backup_base.exists(): - shutil.rmtree(backup_base) + if cleanup_error is not None: + console.print( + " [yellow]Warning:[/yellow] Could not fully " + "remove rollback backup: " + f"{_escape_markup(str(cleanup_error))}" + ) + console.print( + " [dim]Backup may remain at: " + f"{_escape_markup(str(backup_base))}[/dim]" + ) except Exception as rollback_error: console.print(f" [red]✗[/red] Rollback failed: {_escape_markup(str(rollback_error))}") console.print(f" [dim]Backup preserved at: {_escape_markup(str(backup_base))}[/dim]") diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 93ad8fb80d..b9ca6e6d68 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -12,7 +12,6 @@ import hashlib import os import tempfile -import zipfile import shutil from dataclasses import dataclass from pathlib import Path @@ -27,6 +26,13 @@ from packaging import version as pkg_version from packaging.specifiers import SpecifierSet, InvalidSpecifier +from .._download_security import ( + MAX_JSON_CATALOG_BYTES, + build_safe_download_path, + is_https_or_localhost_http, + read_response_limited, + safe_extract_zip, +) from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority from .._init_options import ( MISSING_INIT_OPTIONS_FILE, @@ -3523,18 +3529,7 @@ def install_from_zip( with tempfile.TemporaryDirectory() as tmpdir: temp_path = Path(tmpdir) - with zipfile.ZipFile(zip_path, 'r') as zf: - temp_path_resolved = temp_path.resolve() - for member in zf.namelist(): - member_path = (temp_path / member).resolve() - try: - member_path.relative_to(temp_path_resolved) - except ValueError: - raise PresetValidationError( - f"Unsafe path in ZIP archive: {member} " - "(potential path traversal)" - ) - zf.extractall(temp_path) + safe_extract_zip(zip_path, temp_path, error_type=PresetValidationError) pack_dir = temp_path manifest_path = pack_dir / "preset.yml" @@ -4270,7 +4265,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: final_url = response.geturl() if final_url != entry.url: self._validate_catalog_url(final_url) - catalog_data = json.loads(response.read()) + catalog_data = json.loads( + read_response_limited( + response, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=PresetError, + label=f"preset catalog {entry.url}", + ) + ) self._validate_catalog_payload(catalog_data, entry.url) @@ -4432,7 +4434,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: final_url = response.geturl() if final_url != catalog_url: self._validate_catalog_url(final_url) - catalog_data = json.loads(response.read()) + catalog_data = json.loads( + read_response_limited( + response, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=PresetError, + label=f"preset catalog {catalog_url}", + ) + ) # Validate catalog structure. Reuses the same helper as # ``_fetch_single_catalog`` so all three branches (root type, @@ -4593,6 +4602,10 @@ def download_pack( raise PresetError( f"Preset '{pack_id}' has no download URL" ) + if not isinstance(download_url, str): + raise PresetError( + f"Preset download URL is malformed: {download_url}" + ) from urllib.parse import urlparse @@ -4605,25 +4618,32 @@ def download_pack( try: parsed = urlparse(download_url) hostname = parsed.hostname + parsed.port except ValueError: raise PresetError( f"Preset download URL is malformed: {download_url}" ) from None - is_localhost = hostname in ("localhost", "127.0.0.1", "::1") - if parsed.scheme != "https" and not ( - parsed.scheme == "http" and is_localhost - ): + if not hostname: + raise PresetError( + f"Preset download URL is malformed: {download_url}" + ) + if not is_https_or_localhost_http(download_url): raise PresetError( f"Preset download URL must use HTTPS: {download_url}" ) if target_dir is None: target_dir = self.cache_dir / "downloads" - target_dir.mkdir(parents=True, exist_ok=True) - + target_dir = Path(target_dir) version = pack_info.get("version", "unknown") - zip_filename = f"{pack_id}-{version}.zip" - zip_path = target_dir / zip_filename + zip_path = build_safe_download_path( + target_dir, + pack_id, + version, + error_type=PresetError, + label="preset", + ) + target_dir.mkdir(parents=True, exist_ok=True) extra_headers = None resolved_download_url = self._resolve_github_release_asset_api_url(download_url) @@ -4633,7 +4653,11 @@ def download_pack( try: with self._open_url(download_url, timeout=60, extra_headers=extra_headers) as response: - zip_data = response.read() + zip_data = read_response_limited( + response, + error_type=PresetError, + label=f"preset '{pack_id}' download", + ) verify_archive_sha256( zip_data, pack_info.get("sha256"), pack_id, PresetError diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 154346209f..dfa0c41d1b 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -19,6 +19,7 @@ from .._download_security import ( is_https_or_localhost_http, is_safe_download_redirect, + read_response_limited, ) preset_app = typer.Typer( @@ -126,15 +127,15 @@ def _validate_download_redirect(old_url, new_url): if not is_https_or_localhost_http(from_url): console.print( - "[red]Error:[/red] URL must use HTTPS with a hostname, " - "or HTTP for localhost/loopback." + "[red]Error:[/red] URL must use HTTPS with a hostname and be " + "a valid URL with a host. HTTP is only allowed for localhost, " + "127.0.0.1, and ::1." ) raise typer.Exit(1) console.print(f"Installing preset from [cyan]{_escape_markup(from_url)}[/cyan]...") import urllib.error import tempfile - import shutil with tempfile.TemporaryDirectory() as tmpdir: zip_path = Path(tmpdir) / "preset.zip" @@ -162,16 +163,21 @@ def _validate_download_redirect(old_url, new_url): console.print( "[red]Error:[/red] Preset URL redirected to a disallowed URL: " f"{final_url}. Redirect targets must use HTTPS with a hostname, " - "or HTTP for localhost/loopback." + "or HTTP for localhost (127.0.0.1, ::1)." ) raise typer.Exit(1) - with zip_path.open("wb") as output: - try: - shutil.copyfileobj(response, output) - except TypeError: - output.write(response.read()) - except urllib.error.URLError as e: - console.print(f"[red]Error:[/red] Failed to download: {_escape_markup(str(e))}") + zip_path.write_bytes( + read_response_limited( + response, + error_type=PresetError, + label=f"preset {from_url}", + ) + ) + except (urllib.error.URLError, PresetError) as e: + console.print( + f"[red]Error:[/red] Failed to download: " + f"{_escape_markup(str(e))}" + ) raise typer.Exit(1) manifest = manager.install_from_zip(zip_path, speckit_version, priority) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 93505c0f30..eaacb34893 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -12,7 +12,7 @@ import os import re import sys -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Any import typer @@ -401,6 +401,12 @@ def _reject_insecure_download_redirect(old_url: str, new_url: str) -> None: # a ceiling any legitimate workflow definition should ever approach. _MAX_WORKFLOW_YAML_BYTES = 5 * 1024 * 1024 # 5 MiB _DOWNLOAD_CHUNK_SIZE = 65536 +# Custom step packages contain executable Python, metadata, and optional helper +# files downloaded one-by-one rather than as an archive. Mirror the archive +# ceilings so a catalog cannot turn individually valid files into an unbounded +# aggregate download. +_MAX_STEP_PACKAGE_FILES = 512 +_MAX_STEP_PACKAGE_BYTES = 50 * 1024 * 1024 # 50 MiB def _read_response_within_limit(response, max_bytes: int | None = None) -> bytes: @@ -2658,14 +2664,39 @@ def workflow_step_add( ) raise typer.Exit(1) - step_yml_url = info.get("step_yml_url") or info.get("url") - if not step_yml_url: + declared_step_yml_url = info.get("step_yml_url") + if declared_step_yml_url is not None and not isinstance( + declared_step_yml_url, str + ): + console.print( + f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " + "step.yml URL; expected a non-empty string" + ) + raise typer.Exit(1) + step_yml_url = declared_step_yml_url or info.get("url") + if step_yml_url is None or ( + isinstance(step_yml_url, str) and not step_yml_url.strip() + ): console.print(f"[red]Error:[/red] Catalog entry for '{step_id}' has no URL") raise typer.Exit(1) + if not isinstance(step_yml_url, str): + console.print( + f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " + "step.yml URL; expected a non-empty string" + ) + raise typer.Exit(1) # Derive __init__.py URL: replace trailing step.yml with __init__.py # or use explicit init_url if provided. init_url = info.get("init_url") + if init_url is not None and ( + not isinstance(init_url, str) or not init_url.strip() + ): + console.print( + f"[red]Error:[/red] Catalog entry for '{step_id}' has a malformed " + "__init__.py URL; expected a non-empty string" + ) + raise typer.Exit(1) if not init_url: if step_yml_url.endswith("step.yml"): init_url = step_yml_url[: -len("step.yml")] + "__init__.py" @@ -2676,6 +2707,41 @@ def workflow_step_add( ) raise typer.Exit(1) + # Preflight the declared file count before creating a staging directory or + # issuing any request. The two required files are always part of the package; + # duplicate declarations for them in extra_files are ignored below and do + # not count twice. + extra_files = info.get("extra_files") + if extra_files is not None and not isinstance(extra_files, dict): + console.print( + "[yellow]Warning:[/yellow] Catalog entry 'extra_files' is not a mapping; " + "additional package files will not be downloaded." + ) + extra_files = {} + + def _is_required_package_file(rel_path: object) -> bool: + """Match portable path/case aliases of the two required package files.""" + if not isinstance(rel_path, str): + return False + parts = PurePosixPath(rel_path.replace("\\", "/")).parts + return len(parts) == 1 and parts[0].casefold() in { + "step.yml", + "__init__.py", + } + + declared_extra_count = sum( + 1 + for rel_path in (extra_files or {}) + if not _is_required_package_file(rel_path) + ) + package_file_count = 2 + declared_extra_count + if package_file_count > _MAX_STEP_PACKAGE_FILES: + console.print( + f"[red]Error:[/red] Step package declares {package_file_count} files, " + f"exceeding the {_MAX_STEP_PACKAGE_FILES}-file limit" + ) + raise typer.Exit(1) + from specify_cli.authentication.http import open_url as _open_url def _safe_fetch(url: str) -> bytes: @@ -2732,6 +2798,14 @@ def _safe_fetch(url: str) -> bytes: console.print(f"[red]Error:[/red] Failed to download step files: {exc}") raise typer.Exit(1) + package_bytes = len(step_yml_content) + len(init_py_content) + if package_bytes > _MAX_STEP_PACKAGE_BYTES: + console.print( + f"[red]Error:[/red] Step package exceeds the " + f"{_MAX_STEP_PACKAGE_BYTES}-byte total size limit" + ) + raise typer.Exit(1) + # Validate step.yml try: import yaml as _yaml @@ -2776,13 +2850,6 @@ def _safe_fetch(url: str) -> bytes: # relative-path → URL. step.yml and __init__.py are ignored here (already # written). Paths are validated to stay within the step package directory to # prevent path-traversal attacks. - extra_files = info.get("extra_files") - if extra_files is not None and not isinstance(extra_files, dict): - console.print( - "[yellow]Warning:[/yellow] Catalog entry 'extra_files' is not a mapping; " - "additional package files will not be downloaded." - ) - extra_files = {} for rel_path, file_url in (extra_files or {}).items(): if not isinstance(rel_path, str) or not rel_path.strip(): console.print( @@ -2790,7 +2857,7 @@ def _safe_fetch(url: str) -> bytes: "empty or non-string path key" ) raise typer.Exit(1) - if rel_path in ("step.yml", "__init__.py"): + if _is_required_package_file(rel_path): continue # already written above # Reject dot-path segments ('', '.', '..') that would refer to the # package directory itself (IsADirectoryError) or escape it. @@ -2826,6 +2893,13 @@ def _safe_fetch(url: str) -> bytes: f"[red]Error:[/red] Failed to download extra file '{rel_path}': {exc}" ) raise typer.Exit(1) + package_bytes += len(file_content) + if package_bytes > _MAX_STEP_PACKAGE_BYTES: + console.print( + f"[red]Error:[/red] Step package exceeds the " + f"{_MAX_STEP_PACKAGE_BYTES}-byte total size limit" + ) + raise typer.Exit(1) try: dest.parent.mkdir(parents=True, exist_ok=True) dest.write_bytes(file_content) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 63ddc76395..46b4e81706 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -22,6 +22,8 @@ import yaml +from .._download_security import MAX_JSON_CATALOG_BYTES, read_response_limited + # --------------------------------------------------------------------------- # Errors @@ -308,7 +310,8 @@ def _validate_catalog_url(self, url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname - except ValueError: + _ = parsed.port + except (TypeError, ValueError): raise WorkflowValidationError( f"Catalog URL is malformed: {url}" ) from None @@ -505,7 +508,8 @@ def _validate_catalog_url(url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname - except ValueError: + _ = parsed.port + except (TypeError, ValueError): raise WorkflowCatalogError( f"Refusing to fetch catalog from malformed URL: {url}" ) from None @@ -538,7 +542,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: entry.url, timeout=30, redirect_validator=_validate_redirect ) as resp: _validate_catalog_url(resp.geturl()) - data = json.loads(resp.read().decode("utf-8")) + data = json.loads( + read_response_limited( + resp, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=WorkflowCatalogError, + label="workflow catalog", + ).decode("utf-8") + ) except Exception as exc: # Fall back to cache if available if cache_file.exists(): @@ -982,7 +993,8 @@ def _validate_catalog_url(self, url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname - except ValueError: + _ = parsed.port + except (TypeError, ValueError): raise StepValidationError( f"Catalog URL is malformed: {url}" ) from None @@ -1178,7 +1190,8 @@ def _validate_url(url: str) -> None: try: parsed = urlparse(url) hostname = parsed.hostname - except ValueError: + _ = parsed.port + except (TypeError, ValueError): raise StepCatalogError( f"Refusing to fetch catalog from malformed URL: {url}" ) from None @@ -1211,7 +1224,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: entry.url, timeout=30, redirect_validator=_validate_redirect ) as resp: _validate_url(resp.geturl()) - data = json.loads(resp.read().decode("utf-8")) + data = json.loads( + read_response_limited( + resp, + max_bytes=MAX_JSON_CATALOG_BYTES, + error_type=StepCatalogError, + label="step catalog", + ).decode("utf-8") + ) except Exception as exc: if cache_safe and cache_file.exists(): try: diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index 5ec3d88973..15a844118b 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -247,6 +247,25 @@ def test_catalog_entry_rejects_non_boolean_verified(): CatalogEntry.from_dict(data) +def test_catalog_entry_preserves_sha256_through_provenance(): + digest = "a" * 64 + payload = catalog_payload( + {"demo": catalog_entry_dict("demo", sha256=f"sha256:{digest}")} + ) + + entry = load_catalog_payload(payload)["demo"] + source = CatalogSource( + id="team", + url="https://example.com/catalog.json", + priority=10, + install_policy=InstallPolicy.INSTALL_ALLOWED, + scope=Scope.PROJECT, + ) + + assert entry.sha256 == f"sha256:{digest}" + assert entry.with_provenance(source).sha256 == f"sha256:{digest}" + + def test_load_payload_rejects_id_key_mismatch(): # The enclosing key is authoritative; an entry whose own id disagrees with # the key must be rejected so a catalog can't list a spoofed/unresolvable id. diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index 32972ac684..164de57006 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -7,7 +7,9 @@ from __future__ import annotations import os +import zipfile from pathlib import Path +from unittest.mock import patch import pytest import yaml @@ -171,3 +173,62 @@ def test_download_manifest_rejects_non_https_url_even_offline(tmp_path: Path): ) with pytest.raises(BundlerError, match="HTTPS"): _download_manifest(resolved, offline=True) + + +def test_local_zip_uses_bounded_archive_open(tmp_path: Path): + artifact = tmp_path / "too-many-entries.zip" + with zipfile.ZipFile(artifact, "w") as archive: + archive.writestr("bundle.yml", yaml.safe_dump(valid_manifest_dict())) + for index in range(512): + archive.writestr(f"assets/{index}.txt", "") + + with pytest.raises(BundlerError, match="too many entries"): + _local_manifest_source(str(artifact)) + + +def test_invalid_local_manifest_is_rejected_before_project_init( + tmp_path: Path, + monkeypatch, +): + bundle_dir = tmp_path / "invalid-bundle" + data = valid_manifest_dict() + data["bundle"]["author"] = "" + write_manifest(bundle_dir, data) + empty_cwd = tmp_path / "empty" + empty_cwd.mkdir() + monkeypatch.chdir(empty_cwd) + + runner = CliRunner() + with patch("specify_cli.commands.bundle._run_init") as run_init: + result = runner.invoke( + app, + ["bundle", "install", str(bundle_dir), "--offline"], + ) + + assert result.exit_code == 1 + assert "Missing required field: bundle.author" in result.output + run_init.assert_not_called() + + +def test_incompatible_local_manifest_is_rejected_before_project_init( + tmp_path: Path, + monkeypatch, +): + bundle_dir = tmp_path / "incompatible-bundle" + data = valid_manifest_dict() + data["requires"]["speckit_version"] = ">=999.0.0" + write_manifest(bundle_dir, data) + empty_cwd = tmp_path / "empty" + empty_cwd.mkdir() + monkeypatch.chdir(empty_cwd) + + runner = CliRunner() + with patch("specify_cli.commands.bundle._run_init") as run_init: + result = runner.invoke( + app, + ["bundle", "install", str(bundle_dir), "--offline"], + ) + + assert result.exit_code == 1 + assert "requires Spec Kit >=999.0.0" in result.output + run_init.assert_not_called() diff --git a/tests/test_download_security.py b/tests/test_download_security.py index f5dffe10de..75f0e90efe 100644 --- a/tests/test_download_security.py +++ b/tests/test_download_security.py @@ -1,15 +1,24 @@ -"""Tests for bounded HTTP download helpers.""" +"""Tests for bounded download and ZIP extraction helpers.""" from __future__ import annotations +import io +import stat +import struct import weakref +import zipfile +import zlib import pytest from specify_cli._download_security import ( + MAX_ZIP_CENTRAL_DIRECTORY_BYTES, + build_safe_download_path, is_https_or_localhost_http, is_loopback_url, read_response_limited, + read_zip_member_limited, + safe_extract_zip, ) @@ -112,8 +121,6 @@ class _Response: def __init__(self, data: bytes, *, chunk: int | None = None): self.data = data self.pos = 0 - # When set, never return more than *chunk* bytes per call even if more is - # requested - simulates short reads (e.g. chunked transfer encoding). self.chunk = chunk def read(self, size: int = -1) -> bytes: @@ -160,6 +167,55 @@ def read(self, _size: int = -1) -> bytes | _TrackedChunk: ) return chunk + def __enter__(self): + return self + + def __exit__(self, _exc_type, _exc, _tb): + return False + + +class _CustomZipError(ValueError): + pass + + +class _ExplodingResponse: + def read(self, _size: int = -1) -> bytes: + raise zlib.error("corrupt compressed data") + + def __enter__(self): + return self + + def __exit__(self, _exc_type, _exc, _tb): + return False + + +class _FakeZipArchive: + def __init__( + self, + response, + *, + filename: str = "extension.yml", + file_size: int = 0, + ): + self.response = response + self.info = zipfile.ZipInfo(filename) + self.info.file_size = file_size + + def __enter__(self): + return self + + def __exit__(self, _exc_type, _exc, _tb): + return False + + def getinfo(self, _name): + return self.info + + def infolist(self): + return [self.info] + + def open(self, _member, _mode="r"): + return self.response + def test_read_response_limited_rejects_oversized_download(): with pytest.raises(ValueError, match="exceeds maximum size"): @@ -171,9 +227,6 @@ def test_read_response_limited_returns_full_body_within_limit(): def test_read_response_limited_enforces_bound_under_short_reads(): - # A server that streams more than max_bytes total while every read() returns - # fewer bytes than requested (chunked encoding) must still be rejected - a - # single read(max_bytes + 1) could be fooled, the accumulating loop cannot. response = _Response(b"x" * 100, chunk=8) with pytest.raises(ValueError, match="exceeds maximum size"): read_response_limited(response, max_bytes=16) @@ -225,3 +278,794 @@ def test_read_response_limited_rejects_first_byte_at_zero_limit(): max_bytes=0, error_type=_CustomLimitError, ) + + +def test_read_response_limited_escapes_control_characters_in_label(): + with pytest.raises(ValueError) as exc_info: + read_response_limited( + _Response(b"x"), + max_bytes=0, + label="bad\x1b[2J download", + ) + + assert "\x1b" not in str(exc_info.value) + assert "\\x1b" in str(exc_info.value) + + +@pytest.mark.parametrize( + "identifier", + [ + "../outside", + "..\\outside", + "a" * 256, + "delete\x7f", + "csi\x9b[2J", + "\ud800", + ], +) +def test_build_safe_download_path_rejects_nonportable_identifiers( + tmp_path, identifier +): + with pytest.raises(ValueError, match="Unsafe archive download filename"): + build_safe_download_path( + tmp_path, + identifier, + "1.0.0", + ) + + +@pytest.mark.parametrize( + "member_name", + [ + "../evil.txt", + "nested/../../evil.txt", + "nested\\..\\evil.txt", + "C:\\Windows\\evil.txt", + "C:drive-relative.txt", + ], +) +def test_safe_extract_zip_rejects_traversal(tmp_path, member_name): + zip_path = tmp_path / "bad.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr(member_name, "nope") + + with pytest.raises(ValueError, match="Unsafe path"): + safe_extract_zip(zip_path, tmp_path / "out") + + +@pytest.mark.parametrize("member_name", [".", "./file.txt", "nested/./file.txt", "nested//file.txt"]) +def test_safe_extract_zip_rejects_dot_path_segments(tmp_path, member_name): + zip_path = tmp_path / "bad.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr(member_name, "nope") + + with pytest.raises(_CustomZipError, match="Unsafe path"): + safe_extract_zip(zip_path, tmp_path / "out", error_type=_CustomZipError) + + +def test_safe_extract_zip_rejects_symlinks(tmp_path): + zip_path = tmp_path / "bad.zip" + info = zipfile.ZipInfo("link") + info.external_attr = (stat.S_IFLNK | 0o777) << 16 + + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr(info, "target") + + with pytest.raises(ValueError, match="Unsafe symlink"): + safe_extract_zip(zip_path, tmp_path / "out") + + +def test_safe_extract_zip_rejects_symlink_without_partial_extraction(tmp_path): + zip_path = tmp_path / "mixed.zip" + link = zipfile.ZipInfo("evil-link") + link.external_attr = (stat.S_IFLNK | 0o777) << 16 + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("safe/first.txt", "hello") + zf.writestr(link, "target") + zf.writestr("safe/second.txt", "world") + + out_dir = tmp_path / "out" + with pytest.raises(ValueError, match="Unsafe symlink"): + safe_extract_zip(zip_path, out_dir) + + assert not out_dir.exists() or not any(out_dir.rglob("*")) + + +def test_safe_extract_zip_rejects_oversized_member(tmp_path): + zip_path = tmp_path / "bad.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("big.txt", "abcde") + + with pytest.raises(ValueError, match="exceeds maximum size"): + safe_extract_zip(zip_path, tmp_path / "out", max_member_bytes=4) + + +def test_safe_extract_zip_rejects_too_many_entries(tmp_path): + zip_path = tmp_path / "bad.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("one.txt", "1") + zf.writestr("two.txt", "2") + + with pytest.raises(ValueError, match="too many entries"): + safe_extract_zip(zip_path, tmp_path / "out", max_entries=1) + + +def _legacy_zip_eocd( + *, + entries: int, + central_directory_size: int, + central_directory_offset: int = 0, + comment_size: int = 0, +) -> bytes: + return struct.pack( + "<4s4H2LH", + b"PK\x05\x06", + 0, + 0, + entries, + entries, + central_directory_size, + central_directory_offset, + comment_size, + ) + + +def test_safe_extract_zip_preflights_declared_entry_count(tmp_path, monkeypatch): + zip_path = tmp_path / "too-many.zip" + zip_path.write_bytes( + _legacy_zip_eocd(entries=513, central_directory_size=0) + ) + monkeypatch.setattr( + zipfile, + "ZipFile", + lambda *_args, **_kwargs: pytest.fail("ZipFile constructor was called"), + ) + + with pytest.raises(ValueError, match="too many entries"): + safe_extract_zip(zip_path, tmp_path / "out") + + +def test_safe_extract_zip_preflights_actual_entry_count_when_eocd_lies( + tmp_path, monkeypatch +): + central_header = b"PK\x01\x02" + b"\x00" * 42 + central_directory = central_header * 513 + zip_path = tmp_path / "lying-count.zip" + zip_path.write_bytes( + central_directory + + _legacy_zip_eocd( + entries=1, + central_directory_size=len(central_directory), + ) + ) + monkeypatch.setattr( + zipfile, + "ZipFile", + lambda *_args, **_kwargs: pytest.fail("ZipFile constructor was called"), + ) + + with pytest.raises(ValueError, match="too many entries"): + safe_extract_zip(zip_path, tmp_path / "out") + + +def test_safe_extract_zip_rejects_truncated_last_eocd_comment( + tmp_path, monkeypatch +): + trailing_eocd = _legacy_zip_eocd( + entries=0, + central_directory_size=0, + comment_size=1, + ) + zip_path = tmp_path / "ambiguous-eocd.zip" + zip_path.write_bytes( + _legacy_zip_eocd( + entries=0, + central_directory_size=0, + comment_size=len(trailing_eocd), + ) + + trailing_eocd + ) + monkeypatch.setattr( + zipfile, + "ZipFile", + lambda *_args, **_kwargs: pytest.fail("ZipFile constructor was called"), + ) + + with pytest.raises(ValueError, match="Invalid ZIP archive"): + safe_extract_zip(zip_path, tmp_path / "out") + + +def test_safe_extract_zip_rejects_zip64_before_zipfile_construction( + tmp_path, monkeypatch +): + zip64_eocd = struct.pack( + "<4sQ2H2L4Q", + b"PK\x06\x06", + 44, + 45, + 45, + 0, + 0, + 0, + 0, + 0, + 0, + ) + zip64_locator = struct.pack( + "<4sLQL", + b"PK\x06\x07", + 0, + 0, + 1, + ) + zip_path = tmp_path / "zip64.zip" + zip_path.write_bytes( + zip64_eocd + + zip64_locator + + _legacy_zip_eocd( + entries=0xFFFF, + central_directory_size=0xFFFFFFFF, + central_directory_offset=0xFFFFFFFF, + ) + ) + with zipfile.ZipFile(zip_path) as zf: + assert zf.namelist() == [] + monkeypatch.setattr( + zipfile, + "ZipFile", + lambda *_args, **_kwargs: pytest.fail("ZipFile constructor was called"), + ) + + with pytest.raises(ValueError, match="ZIP64"): + safe_extract_zip(zip_path, tmp_path / "out") + + +@pytest.mark.parametrize( + "indicator", + [ + "central-sizes", + "central-offset", + "central-disk", + "local-sizes", + ], +) +def test_safe_extract_zip_rejects_entry_zip64_before_zipfile_construction( + tmp_path, monkeypatch, indicator +): + contents = b"contents" + if indicator == "central-offset": + zip64_payload = struct.pack("=0"}, + "provides": { + "commands": [ + { + "name": command_name, + "file": "commands/run.md", + } + ] + }, + } + + +def _write_update_zip(zip_path, *, version="1.1.0", manifest=None): + """Create the minimal archive required by the update preflight.""" + import zipfile + + with zipfile.ZipFile(zip_path, "w") as archive: + archive.writestr( + "extension.yml", + yaml.safe_dump( + manifest + if manifest is not None + else _valid_update_manifest(version=version) + ), + ) + + +def _stub_available_update(monkeypatch, registry_entry): + """Stub discovery for one installed extension with an available update.""" + monkeypatch.setattr( + ExtensionManager, + "list_installed", + lambda self: [ + {"id": "test-ext", "name": "Test Ext", "version": "1.0.0"} + ], + ) + monkeypatch.setattr( + ExtensionRegistry, "get", lambda self, ext_id: registry_entry + ) + monkeypatch.setattr( + ExtensionCatalog, + "get_extension_info", + lambda self, ext_id: { + "id": "test-ext", + "name": "Test Ext", + "version": "1.1.0", + "download_url": "https://example.com/ext.zip", + }, + ) + monkeypatch.setattr("typer.confirm", lambda _: True) + + +def _update_backup_dirs(project_dir): + """Return per-attempt update backups created for test-ext.""" + backup_root = ( + project_dir / ".specify" / "extensions" / ".backup" + ) + if not backup_root.is_dir(): + return [] + return list(backup_root.glob("update-*-*")) + + @pytest.fixture def project_dir(tmp_path): """Create a mock spec-kit project directory.""" @@ -85,21 +160,35 @@ def test_extension_update_rollback_corrupted_config(project_dir, monkeypatch): monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{"id": "test-ext", "name": "Test Ext", "version": "1.0.0"}]) monkeypatch.setattr(ExtensionRegistry, "get", lambda self, ext_id: {"version": "1.0.0", "enabled": True}) - # Force failure in download_extension to trigger rollback - def mock_download_fail(*args, **kwargs): - # Corrupt the config BEFORE rollback is triggered + # Reach the destructive update phase, then fail the install so rollback is + # both necessary and safe to exercise. Download/preflight failures leave + # the installation untouched and deliberately skip destructive rollback. + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + def mock_install_fail(*args, **kwargs): config_path.write_text(yaml.dump("CORRUPTED")) - raise Exception("Download failed") + raise Exception("Install failed") monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: {"id": "test-ext", "name": "Test Ext", "version": "1.1.0", "download_url": "https://example.com/ext.zip"}) - monkeypatch.setattr(ExtensionCatalog, "download_extension", mock_download_fail) + monkeypatch.setattr(ExtensionManager, "install_from_zip", mock_install_fail) monkeypatch.setattr("typer.confirm", lambda _: True) result = runner.invoke(app, ["extension", "update", "test-ext"], obj={"project_root": project_dir}) # Should handle Exception and NOT crash with AttributeError during rollback assert result.exit_code == 1 - assert "Download failed" in result.output + assert "Install failed" in result.output assert not isinstance(result.exception, AttributeError) # Verify hooks key was preserved (normalized to {} if it was null/corrupted) @@ -137,15 +226,25 @@ def test_extension_update_skills_backup_no_collision(project_dir, monkeypatch): }) monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: {"id": "test-ext", "name": "Test Ext", "version": "1.1.0", "download_url": "https://example.com/ext.zip"}) - # Fail at download (step 5, after the command backup in step 3). Delete the - # originals first to simulate an install clobbering them, forcing rollback - # to rely entirely on the backups. - def mock_download_fail(self, ext_id): + # Let download and validation succeed, then simulate remove clobbering the + # originals before install fails. Rollback must rely on the command backups. + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + def mock_remove(self, ext_id, keep_config=False): plan_file.unlink() tasks_file.unlink() - raise Exception("Download failed") - monkeypatch.setattr(ExtensionCatalog, "download_extension", mock_download_fail) + def mock_install_fail(*args, **kwargs): + raise Exception("Install failed") + + monkeypatch.setattr(ExtensionManager, "remove", mock_remove) + monkeypatch.setattr(ExtensionManager, "install_from_zip", mock_install_fail) monkeypatch.setattr("typer.confirm", lambda _: True) result = runner.invoke(app, ["extension", "update", "test-ext"], obj={"project_root": project_dir}) @@ -155,3 +254,1972 @@ def mock_download_fail(self, ext_id): assert plan_file.exists() and tasks_file.exists() assert plan_file.read_text() == "PLAN CONTENT" assert tasks_file.read_text() == "TASKS CONTENT" + + +def test_extension_update_download_failure_never_removes_live_extension( + project_dir, monkeypatch +): + """A download error happens before the destructive update boundary.""" + import shutil + + monkeypatch.chdir(project_dir) + + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + live_extension = project_dir / ".specify" / "extensions" / "test-ext" + live_extension.mkdir(parents=True) + sentinel = live_extension / "sentinel.txt" + sentinel.write_text("ORIGINAL") + + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + def mock_download_fail(self, ext_id): + raise Exception("Download failed") + + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + mock_download_fail, + ) + + removed_paths = [] + real_rmtree = shutil.rmtree + + def track_rmtree(path, *args, **kwargs): + removed_paths.append(Path(path).resolve()) + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", track_rmtree) + + remove_calls = [] + + def track_remove(self, ext_id, keep_config=False): + remove_calls.append((ext_id, keep_config)) + + monkeypatch.setattr(ExtensionManager, "remove", track_remove) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Download failed" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert live_extension.resolve() not in removed_paths + assert sentinel.read_text() == "ORIGINAL" + assert _update_backup_dirs(project_dir) == [] + + +def test_extension_update_partial_extension_backup_preserves_live_tree( + project_dir, monkeypatch +): + """A partial extension backup must never be restored over the live tree.""" + import shutil + + monkeypatch.chdir(project_dir) + + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + live_extension = project_dir / ".specify" / "extensions" / "test-ext" + live_extension.mkdir(parents=True) + first_file = live_extension / "first.txt" + second_file = live_extension / "second.txt" + first_file.write_text("FIRST ORIGINAL") + second_file.write_text("SECOND ORIGINAL") + + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + real_copytree = shutil.copytree + + def fail_partial_backup(src, dst, *args, **kwargs): + src = Path(src) + dst = Path(dst) + if src.resolve() == live_extension.resolve(): + dst.mkdir(parents=True, exist_ok=True) + shutil.copy2(first_file, dst / first_file.name) + raise OSError("Extension backup failed") + return real_copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copytree", fail_partial_backup) + + remove_calls = [] + + def track_remove(self, ext_id, keep_config=False): + remove_calls.append((ext_id, keep_config)) + + monkeypatch.setattr(ExtensionManager, "remove", track_remove) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Extension backup failed" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert first_file.read_text() == "FIRST ORIGINAL" + assert second_file.read_text() == "SECOND ORIGINAL" + assert _update_backup_dirs(project_dir) == [] + + +def test_extension_update_rejects_symlinked_backup_root_without_cleanup( + project_dir, monkeypatch +): + """A rejected backup root must never be followed during error cleanup.""" + import os + + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + outside = project_dir / "outside-backups" + outside.mkdir() + sentinel = outside / "keep.txt" + sentinel.write_text("KEEP", encoding="utf-8") + backup_root = ( + project_dir / ".specify" / "extensions" / ".backup" + ) + backup_root.parent.mkdir(parents=True) + try: + os.symlink(outside, backup_root) + except OSError: + pytest.skip("Current platform/user cannot create directory symlinks") + + download_calls = [] + remove_calls = [] + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: download_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "symlinked directory" in result.output + assert "Rolling back" not in result.output + assert download_calls == [] + assert remove_calls == [] + assert backup_root.is_symlink() + assert sentinel.read_text(encoding="utf-8") == "KEEP" + + +def test_extension_update_partial_command_backup_preserves_live_commands( + project_dir, monkeypatch +): + """An incomplete command backup must not make originals look newly added.""" + import shutil + + monkeypatch.chdir(project_dir) + + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + skills_root = project_dir / ".claude" / "skills" + plan_file = skills_root / "speckit-plan" / "SKILL.md" + tasks_file = skills_root / "speckit-tasks" / "SKILL.md" + plan_file.parent.mkdir(parents=True) + tasks_file.parent.mkdir(parents=True) + plan_file.write_text("PLAN ORIGINAL") + tasks_file.write_text("TASKS ORIGINAL") + + registry_entry = { + "version": "1.0.0", + "enabled": True, + "registered_commands": { + "claude": ["speckit.plan", "speckit.tasks"] + }, + } + _stub_available_update(monkeypatch, registry_entry) + + backup_root = ( + project_dir + / ".specify" + / "extensions" + / ".backup" + ) + real_copy2 = shutil.copy2 + command_backup_count = 0 + + def fail_second_command_backup(src, dst, *args, **kwargs): + nonlocal command_backup_count + dst = Path(dst) + try: + relative_backup = dst.resolve().relative_to( + backup_root.resolve() + ) + except ValueError: + return real_copy2(src, dst, *args, **kwargs) + if ( + len(relative_backup.parts) < 2 + or not relative_backup.parts[0].startswith("update-") + or relative_backup.parts[1] != "commands" + ): + return real_copy2(src, dst, *args, **kwargs) + command_backup_count += 1 + if command_backup_count == 2: + raise OSError("Command backup failed") + return real_copy2(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copy2", fail_second_command_backup) + + remove_calls = [] + + def track_remove(self, ext_id, keep_config=False): + remove_calls.append((ext_id, keep_config)) + + monkeypatch.setattr(ExtensionManager, "remove", track_remove) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Command backup failed" in result.output + assert "Rolling back" not in result.output + assert command_backup_count == 2 + assert remove_calls == [] + assert plan_file.read_text() == "PLAN ORIGINAL" + assert tasks_file.read_text() == "TASKS ORIGINAL" + assert _update_backup_dirs(project_dir) == [] + + +def test_extension_update_rolls_back_partial_remove(project_dir, monkeypatch): + """A remove failure after its first mutation still restores the backup.""" + monkeypatch.chdir(project_dir) + + config_path = project_dir / ".specify" / "extensions.yml" + config_path.write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + live_extension = project_dir / ".specify" / "extensions" / "test-ext" + live_extension.mkdir(parents=True) + sentinel = live_extension / "sentinel.txt" + sentinel.write_text("ORIGINAL") + + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + def fail_partial_remove(self, ext_id, keep_config=False): + sentinel.unlink() + raise OSError("Remove failed") + + monkeypatch.setattr(ExtensionManager, "remove", fail_partial_remove) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Remove failed" in result.output + assert "Rolling back" in result.output + assert sentinel.read_text() == "ORIGINAL" + assert _update_backup_dirs(project_dir) == [] + + +def test_extension_update_reports_success_when_backup_cleanup_fails( + project_dir, monkeypatch +): + """A retained backup is isolated from the next successful update.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + live_extension = ( + project_dir / ".specify" / "extensions" / "test-ext" + ) + live_extension.mkdir(parents=True) + (live_extension / "sentinel.txt").write_text( + "ORIGINAL", encoding="utf-8" + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: None, + ) + + backup_root = ( + project_dir + / ".specify" + / "extensions" + / ".backup" + ) + real_rmtree = shutil.rmtree + failed_cleanup = False + + def fail_backup_cleanup_once(path, *args, **kwargs): + nonlocal failed_cleanup + candidate = Path(path) + if ( + candidate.parent == backup_root + and candidate.name.startswith("update-") + and not failed_cleanup + ): + failed_cleanup = True + raise OSError("Backup is locked") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", fail_backup_cleanup_once) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert "Updated to v1.1.0" in result.output + assert "Could not fully remove update backup" in result.output + assert "Rolling back" not in result.output + assert failed_cleanup + + retained_backups = _update_backup_dirs(project_dir) + assert len(retained_backups) == 1 + stale_config = ( + retained_backups[0] / "config" / "stale-config.yml" + ) + stale_config.parent.mkdir(parents=True, exist_ok=True) + stale_config.write_text("stale: true\n", encoding="utf-8") + + # A second attempt receives a distinct backup directory. The retained + # config from the first completed update must never be resurrected. + _write_update_zip(mock_zip) + second_result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert second_result.exit_code == 0, second_result.output + assert not (live_extension / stale_config.name).exists() + assert _update_backup_dirs(project_dir) == retained_backups + + +def test_extension_update_reports_restored_state_when_cleanup_fails( + project_dir, monkeypatch +): + """Post-rollback cleanup errors must not contradict restored state.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + live_extension = ( + project_dir / ".specify" / "extensions" / "test-ext" + ) + live_extension.mkdir(parents=True) + sentinel = live_extension / "sentinel.txt" + sentinel.write_text("ORIGINAL", encoding="utf-8") + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + def fail_partial_remove(self, ext_id, keep_config=False): + sentinel.unlink() + raise OSError("Remove failed") + + monkeypatch.setattr( + ExtensionManager, "remove", fail_partial_remove + ) + + backup_root = ( + project_dir + / ".specify" + / "extensions" + / ".backup" + ) + real_rmtree = shutil.rmtree + failed_cleanup = False + + def fail_backup_cleanup_once(path, *args, **kwargs): + nonlocal failed_cleanup + candidate = Path(path) + if ( + candidate.parent == backup_root + and candidate.name.startswith("update-") + and not failed_cleanup + ): + failed_cleanup = True + raise OSError("Backup is locked") + return real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", fail_backup_cleanup_once) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + assert "Could not fully remove rollback backup" in result.output + assert "Rollback failed" not in result.output + assert sentinel.read_text(encoding="utf-8") == "ORIGINAL" + assert failed_cleanup + + +def test_extension_update_does_not_rollback_for_locked_download_cleanup( + project_dir, monkeypatch +): + """A locked downloaded ZIP is non-fatal after the update commits.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: None, + ) + + real_unlink = Path.unlink + failed_cleanup = False + + def fail_zip_cleanup_once(path, *args, **kwargs): + nonlocal failed_cleanup + if path == mock_zip and not failed_cleanup: + failed_cleanup = True + raise OSError("Downloaded ZIP is locked") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", fail_zip_cleanup_once) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert "Updated to v1.1.0" in result.output + assert "Could not remove downloaded update archive" in result.output + assert "Rolling back" not in result.output + assert failed_cleanup + + +def test_extension_update_preserves_install_error_when_zip_cleanup_fails( + project_dir, monkeypatch +): + """ZIP cleanup must not replace the error that caused rollback.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + def fail_install(*args, **kwargs): + raise RuntimeError("Original install failure") + + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + fail_install, + ) + + real_unlink = Path.unlink + failed_cleanup = False + + def fail_zip_cleanup_once(path, *args, **kwargs): + nonlocal failed_cleanup + if path == mock_zip and not failed_cleanup: + failed_cleanup = True + raise OSError("Downloaded ZIP is locked") + return real_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", fail_zip_cleanup_once) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Original install failure" in result.output + assert "Failed: Downloaded ZIP is locked" not in result.output + assert "Could not remove downloaded update archive" in result.output + assert "Rollback successful" in result.output + assert failed_cleanup + + +@pytest.mark.parametrize( + ("manifest", "expected_error"), + [ + ( + { + "extension": { + "id": "test-ext", + "name": "Test Ext", + "version": "1.1.0", + } + }, + "Missing required field: schema_version", + ), + ( + { + **_valid_update_manifest(), + "requires": {"speckit_version": ">=9999"}, + }, + "Extension requires spec-kit >=9999", + ), + ], +) +def test_extension_update_validates_full_manifest_before_removal( + project_dir, monkeypatch, manifest, expected_error +): + """Malformed or incompatible manifests must fail before remove().""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, manifest=manifest) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert expected_error in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + + +def test_extension_update_rejects_catalog_archive_version_mismatch( + project_dir, monkeypatch +): + """The archive version must match the normalized catalog version.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, version="1.0.1") + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert ( + "Extension version mismatch: expected '1.1.0', got '1.0.1'" + in result.output + ) + assert "Updated to v1.1.0" not in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + + +def test_extension_update_rejects_command_conflicts_before_removal( + project_dir, monkeypatch +): + """Deterministic install conflicts belong on the safe side of remove().""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest( + command_name="speckit.other.run" + ), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + normalized_output = " ".join(result.output.split()) + assert ( + "Command 'speckit.other.run' must use extension namespace 'test-ext'" + in normalized_output + ) + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + + +def test_extension_update_rejects_alias_parent_traversal_before_removal( + project_dir, monkeypatch +): + """Alias '..' segments cannot escape through a symlinked subdirectory.""" + import os + + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + commands_dir = project_dir / ".github" / "agents" + commands_dir.mkdir(parents=True) + outside_dir = project_dir / "outside" / "subdir" + outside_dir.mkdir(parents=True) + outside_target = outside_dir.parent / "victim.agent.md" + outside_target.write_text("USER CONTENT", encoding="utf-8") + try: + os.symlink(outside_dir, commands_dir / "link") + except OSError: + pytest.skip("Current platform/user cannot create directory symlinks") + + manifest = _valid_update_manifest() + manifest["provides"]["commands"][0]["aliases"] = [ + "link/../victim" + ] + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, manifest=manifest) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + normalized_output = " ".join(result.output.split()) + assert "Invalid alias 'link/../victim'" in normalized_output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + assert outside_target.read_text(encoding="utf-8") == "USER CONTENT" + + +def test_extension_update_accepts_normalized_equivalent_archive_version( + project_dir, monkeypatch +): + """Equivalent PEP 440 spellings must not cause a false mismatch.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + registry_entry = {"version": "1.0.0", "enabled": True} + _stub_available_update(monkeypatch, registry_entry) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, version="v1.1") + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert remove_calls == ["test-ext"] + assert len(install_calls) == 1 + assert "Updated to v1.1.0" in result.output + + +def test_extension_update_rejects_noncanonical_root_manifest_alias( + project_dir, monkeypatch +): + """Root aliases must not select different manifests across filesystems.""" + import zipfile + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + injected_manifest = _valid_update_manifest() + injected_manifest["extension"]["id"] = "injected" + mock_zip = project_dir / "mock.zip" + with zipfile.ZipFile(mock_zip, "w") as archive: + archive.writestr( + "EXTENSION.YML", + yaml.safe_dump(injected_manifest), + ) + archive.writestr( + "repo/extension.yml", + yaml.safe_dump(_valid_update_manifest()), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "canonical 'extension.yml' casing" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + + +def test_extension_update_rejects_multiple_nested_archive_roots_before_removal( + project_dir, monkeypatch +): + """Nested manifests must match install_from_zip's one-directory layout.""" + import zipfile + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + mock_zip = project_dir / "mock.zip" + with zipfile.ZipFile(mock_zip, "w") as archive: + archive.writestr( + "repo/extension.yml", + yaml.safe_dump(_valid_update_manifest()), + ) + archive.writestr("other/content.txt", "SECOND ROOT") + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + normalized_output = " ".join(result.output.split()) + assert ( + "must contain exactly one top-level directory" + in normalized_output + ) + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + + +def test_extension_update_accepts_root_file_beside_nested_manifest( + project_dir, monkeypatch +): + """Root files do not create another directory during ZIP extraction.""" + import zipfile + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, {"version": "1.0.0", "enabled": True} + ) + + mock_zip = project_dir / "mock.zip" + with zipfile.ZipFile(mock_zip, "w") as archive: + archive.writestr( + "repo/extension.yml", + yaml.safe_dump(_valid_update_manifest()), + ) + archive.writestr("README.md", "ROOT FILE") + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert remove_calls == ["test-ext"] + assert len(install_calls) == 1 + + +def test_extension_update_rolls_back_commands_written_before_registry_add( + project_dir, monkeypatch +): + """Rollback uses preflight paths when a failed install has no new registry.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + agents_dir = project_dir / ".github" / "agents" + prompts_dir = project_dir / ".github" / "prompts" + agents_dir.mkdir(parents=True) + prompts_dir.mkdir(parents=True) + + registry_entry = { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + } + _stub_available_update(monkeypatch, registry_entry) + + command_name = "speckit.test-ext.new" + alias_names = [ + "speckit.test-ext.group-a/shared", + "speckit.test-ext.group-b/shared", + ] + new_nested_alias = "speckit.test-ext/new-group/deep" + manifest = _valid_update_manifest(command_name=command_name) + manifest["provides"]["commands"][0]["aliases"] = [ + *alias_names, + new_nested_alias, + ] + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, manifest=manifest) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + new_command = agents_dir / f"{command_name}.agent.md" + new_prompt = prompts_dir / f"{command_name}.prompt.md" + new_nested_command = ( + agents_dir / f"{new_nested_alias}.agent.md" + ) + new_nested_prompt = ( + prompts_dir / f"{new_nested_alias}.prompt.md" + ) + existing_aliases = [ + agents_dir / f"{alias_name}.agent.md" + for alias_name in alias_names + ] + existing_alias_prompts = [ + prompts_dir / f"{alias_name}.prompt.md" + for alias_name in alias_names + ] + for index, alias_file in enumerate(existing_aliases): + alias_file.parent.mkdir(parents=True, exist_ok=True) + alias_file.write_text(f"USER COMMAND {index}", encoding="utf-8") + for index, prompt_file in enumerate(existing_alias_prompts): + prompt_file.parent.mkdir(parents=True, exist_ok=True) + prompt_file.write_text(f"USER PROMPT {index}", encoding="utf-8") + + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + def fail_before_registry_add(*args, **kwargs): + new_command.write_text("NEW COMMAND", encoding="utf-8") + new_prompt.write_text("NEW PROMPT", encoding="utf-8") + new_nested_command.parent.mkdir(parents=True) + new_nested_prompt.parent.mkdir(parents=True) + new_nested_command.write_text( + "NEW NESTED COMMAND", encoding="utf-8" + ) + new_nested_prompt.write_text( + "NEW NESTED PROMPT", encoding="utf-8" + ) + for alias_file in existing_aliases: + alias_file.write_text("OVERWRITTEN COMMAND", encoding="utf-8") + for prompt_file in existing_alias_prompts: + prompt_file.write_text("OVERWRITTEN PROMPT", encoding="utf-8") + raise RuntimeError("Hook registration failed before registry add") + + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + fail_before_registry_add, + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Hook registration failed before registry add" in result.output + assert "Rollback successful" in result.output + assert not new_command.exists() + assert not new_prompt.exists() + assert not new_nested_command.exists() + assert not new_nested_prompt.exists() + assert not (agents_dir / "speckit.test-ext").exists() + assert not (prompts_dir / "speckit.test-ext").exists() + for index, alias_file in enumerate(existing_aliases): + assert ( + alias_file.read_text(encoding="utf-8") + == f"USER COMMAND {index}" + ) + for index, prompt_file in enumerate(existing_alias_prompts): + assert ( + prompt_file.read_text(encoding="utf-8") + == f"USER PROMPT {index}" + ) + + +def test_extension_update_restores_canonical_and_legacy_commands( + project_dir, monkeypatch +): + """Rollback restores every command location cleaned by unregister.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + canonical_dir = project_dir / ".opencode" / "commands" + legacy_dir = project_dir / ".opencode" / "command" + canonical_dir.mkdir(parents=True) + legacy_dir.mkdir(parents=True) + + old_command = "speckit.test-ext.old" + canonical_file = canonical_dir / f"{old_command}.md" + legacy_file = legacy_dir / f"{old_command}.md" + canonical_file.write_text("CANONICAL USER COMMAND", encoding="utf-8") + legacy_file.write_text("LEGACY USER COMMAND", encoding="utf-8") + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {"opencode": [old_command]}, + }, + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + def remove_both_command_copies( + self, ext_id, keep_config=False + ): + canonical_file.unlink() + legacy_file.unlink() + + monkeypatch.setattr( + ExtensionManager, + "remove", + remove_both_command_copies, + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: (_ for _ in ()).throw( + RuntimeError("Install failed after legacy cleanup") + ), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + assert ( + canonical_file.read_text(encoding="utf-8") + == "CANONICAL USER COMMAND" + ) + assert ( + legacy_file.read_text(encoding="utf-8") + == "LEGACY USER COMMAND" + ) + + +def test_extension_update_removes_new_active_skills_root_on_rollback( + project_dir, monkeypatch +): + """Rollback removes an active skills hierarchy created by registration.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"claude","ai_skills":true}', + encoding="utf-8", + ) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + new_skill = ( + project_dir + / ".claude" + / "skills" + / "speckit-test-ext-run" + ) + + def fail_after_creating_skill(*args, **kwargs): + new_skill.mkdir(parents=True) + (new_skill / "SKILL.md").write_text( + "GENERATED SKILL", encoding="utf-8" + ) + raise RuntimeError("Hook registration failed after skill creation") + + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + fail_after_creating_skill, + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + assert not (project_dir / ".claude").exists() + + +def test_extension_update_removes_new_hermes_marker_on_rollback( + project_dir, monkeypatch, tmp_path +): + """Rollback removes Hermes' local marker as well as global skill output.""" + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setattr(Path, "home", lambda: fake_home) + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"hermes","ai_skills":true}', + encoding="utf-8", + ) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + local_marker = project_dir / ".hermes" / "skills" + global_skill = ( + fake_home + / ".hermes" + / "skills" + / "speckit-test-ext-run" + ) + + def fail_after_creating_hermes_skill(*args, **kwargs): + local_marker.mkdir(parents=True) + global_skill.mkdir(parents=True) + (global_skill / "SKILL.md").write_text( + "GENERATED SKILL", encoding="utf-8" + ) + raise RuntimeError("Hook registration failed after Hermes skill") + + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + fail_after_creating_hermes_skill, + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + assert not (project_dir / ".hermes").exists() + assert not (fake_home / ".hermes" / "skills").exists() + + +def test_extension_update_restores_command_symlinks( + project_dir, monkeypatch +): + """Rollback preserves valid and dangling command symlinks byte-for-byte.""" + import os + + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + agents_dir = project_dir / ".github" / "agents" + prompts_dir = project_dir / ".github" / "prompts" + agents_dir.mkdir(parents=True) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + alias_names = [ + "speckit.test-ext.valid-link", + "speckit.test-ext.dangling-link", + ] + manifest = _valid_update_manifest() + manifest["provides"]["commands"][0]["aliases"] = alias_names + mock_zip = project_dir / "mock.zip" + _write_update_zip(mock_zip, manifest=manifest) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + targets_dir = project_dir / "user-targets" + targets_dir.mkdir() + command_targets = [ + targets_dir / "command-valid.md", + targets_dir / "command-dangling.md", + ] + command_targets[0].write_text( + "USER COMMAND TARGET", encoding="utf-8" + ) + + alias_files = [ + agents_dir / f"{alias_name}.agent.md" + for alias_name in alias_names + ] + generated_prompt = ( + prompts_dir / "speckit.test-ext.run.prompt.md" + ) + command_link_texts = [] + try: + for alias_file, target in zip(alias_files, command_targets): + link_text = os.path.relpath(target, alias_file.parent) + os.symlink(link_text, alias_file) + command_link_texts.append(link_text) + except OSError: + pytest.skip("Current platform/user cannot create file symlinks") + + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: None, + ) + + def fail_before_registry_add(*args, **kwargs): + # Normal command rendering replaces command symlinks. + for alias_file in alias_files: + alias_file.unlink() + alias_file.write_text("GENERATED COMMAND", encoding="utf-8") + generated_prompt.parent.mkdir(parents=True) + generated_prompt.write_text( + "GENERATED PROMPT", encoding="utf-8" + ) + raise RuntimeError("Hook registration failed after symlink writes") + + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + fail_before_registry_add, + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + for alias_file, link_text in zip( + alias_files, command_link_texts + ): + assert alias_file.is_symlink() + assert os.readlink(alias_file) == link_text + assert ( + command_targets[0].read_text(encoding="utf-8") + == "USER COMMAND TARGET" + ) + assert not command_targets[1].exists() + assert not prompts_dir.exists() + + +def test_extension_update_rejects_symlinked_copilot_prompt_before_removal( + project_dir, monkeypatch +): + """Prompt links are rejected before rendering can write through them.""" + import os + + if not hasattr(os, "symlink"): + pytest.skip("symlinks are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"copilot","ai_skills":false}', + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + agents_dir = project_dir / ".github" / "agents" + prompts_dir = project_dir / ".github" / "prompts" + agents_dir.mkdir(parents=True) + prompts_dir.mkdir(parents=True) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + command_name = "speckit.test-ext.linked-prompt" + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest(command_name=command_name), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + target = project_dir / "user-prompt.md" + target.write_text("USER PROMPT", encoding="utf-8") + prompt_file = prompts_dir / f"{command_name}.prompt.md" + link_text = os.path.relpath(target, prompt_file.parent) + try: + os.symlink(link_text, prompt_file) + except OSError: + pytest.skip("Current platform/user cannot create file symlinks") + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Cannot safely update symlinked Copilot prompt artifact" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + assert prompt_file.is_symlink() + assert os.readlink(prompt_file) == link_text + assert target.read_text(encoding="utf-8") == "USER PROMPT" + + +@pytest.mark.parametrize( + ("active_agent", "ai_skills"), + [("gemini", False), ("copilot", True)], +) +def test_extension_update_ignores_inactive_copilot_artifacts( + project_dir, monkeypatch, active_agent, ai_skills +): + """Preflight must inspect only outputs the active install can render.""" + import json + import os + + if not hasattr(os, "link") or not hasattr(os, "symlink"): + pytest.skip("links are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "init-options.json").write_text( + json.dumps({"ai": active_agent, "ai_skills": ai_skills}), + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + if active_agent == "gemini": + (project_dir / ".gemini" / "commands").mkdir(parents=True) + + agents_dir = project_dir / ".github" / "agents" + prompts_dir = project_dir / ".github" / "prompts" + agents_dir.mkdir(parents=True) + prompts_dir.mkdir(parents=True) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + command_name = "speckit.test-ext.inactive-copilot" + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest(command_name=command_name), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + command_target = project_dir / "user-command.md" + command_target.write_text("USER COMMAND", encoding="utf-8") + command_file = agents_dir / f"{command_name}.agent.md" + prompt_target = project_dir / "user-prompt.md" + prompt_target.write_text("USER PROMPT", encoding="utf-8") + prompt_file = prompts_dir / f"{command_name}.prompt.md" + try: + os.link(command_target, command_file) + os.symlink( + os.path.relpath(prompt_target, prompt_file.parent), + prompt_file, + ) + except OSError: + pytest.skip("Current filesystem cannot create test links") + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert remove_calls == ["test-ext"] + assert len(install_calls) == 1 + assert command_file.read_text(encoding="utf-8") == "USER COMMAND" + assert prompt_file.is_symlink() + assert prompt_target.read_text(encoding="utf-8") == "USER PROMPT" + + +@pytest.mark.parametrize("broken_active_marker", [False, True]) +def test_extension_update_ignores_unreachable_global_hermes_target( + project_dir, tmp_path, monkeypatch, broken_active_marker +): + """Preflight skips Hermes when its project marker cannot be detected.""" + import os + + if not hasattr(os, "link"): + pytest.skip("hard links are unavailable") + + monkeypatch.chdir(project_dir) + home = tmp_path / "home" + home.mkdir() + monkeypatch.setattr(Path, "home", lambda: home) + if broken_active_marker: + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"hermes","ai_skills":true}', + encoding="utf-8", + ) + (project_dir / ".hermes").write_text( + "marker parent is not a directory", + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + command_name = "speckit.test-ext.unmarked-hermes" + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest(command_name=command_name), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + from specify_cli.agents import CommandRegistrar + + registrar = CommandRegistrar() + agent_config = registrar.AGENT_CONFIGS["hermes"] + commands_dir = registrar._resolve_agent_dir( + "hermes", agent_config, project_dir + ) + output_name = registrar._compute_output_name( + "hermes", command_name, agent_config + ) + artifact = commands_dir / f"{output_name}{agent_config['extension']}" + artifact.parent.mkdir(parents=True) + user_target = project_dir / "user-hermes-skill.md" + user_target.write_text("USER HERMES SKILL", encoding="utf-8") + try: + os.link(user_target, artifact) + except OSError: + pytest.skip("Current filesystem cannot create hard links") + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 0, result.output + assert remove_calls == ["test-ext"] + assert len(install_calls) == 1 + assert artifact.read_text(encoding="utf-8") == "USER HERMES SKILL" + assert not (project_dir / ".hermes" / "skills").exists() + + +@pytest.mark.parametrize("artifact_kind", ["command", "prompt"]) +def test_extension_update_rejects_hard_linked_artifacts_before_removal( + project_dir, monkeypatch, artifact_kind +): + """Rendering must not mutate another name for a shared inode.""" + import os + + if not hasattr(os, "link"): + pytest.skip("hard links are unavailable") + + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"copilot","ai_skills":false}', + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + agents_dir = project_dir / ".github" / "agents" + prompts_dir = project_dir / ".github" / "prompts" + agents_dir.mkdir(parents=True) + prompts_dir.mkdir(parents=True) + _stub_available_update( + monkeypatch, + { + "version": "1.0.0", + "enabled": True, + "registered_commands": {}, + }, + ) + + command_name = "speckit.test-ext.hard-link" + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest(command_name=command_name), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + user_target = project_dir / "user-artifact.md" + user_target.write_text("USER CONTENT", encoding="utf-8") + if artifact_kind == "command": + artifact = agents_dir / f"{command_name}.agent.md" + else: + artifact = prompts_dir / f"{command_name}.prompt.md" + try: + os.link(user_target, artifact) + except OSError: + pytest.skip("Current filesystem cannot create hard links") + original_stat = user_target.stat() + + remove_calls = [] + install_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + monkeypatch.setattr( + ExtensionManager, + "install_from_zip", + lambda *args, **kwargs: install_calls.append(args), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Cannot safely update hard-linked generated artifact" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert install_calls == [] + assert artifact.read_text(encoding="utf-8") == "USER CONTENT" + assert user_target.read_text(encoding="utf-8") == "USER CONTENT" + assert artifact.stat().st_ino == original_stat.st_ino + assert artifact.stat().st_dev == original_stat.st_dev + + +def _write_owned_extension_skill(skill_dir, extension_id, body): + """Write a SKILL.md whose metadata identifies its owning extension.""" + skill_dir.mkdir(parents=True, exist_ok=True) + frontmatter = yaml.safe_dump( + { + "name": skill_dir.name, + "description": "Extension update rollback test", + "metadata": {"source": f"extension:{extension_id}"}, + }, + sort_keys=False, + ) + (skill_dir / "SKILL.md").write_text( + f"---\n{frontmatter}---\n\n{body}\n", + encoding="utf-8", + ) + + +def test_extension_update_rolls_back_registered_skill_artifacts( + project_dir, monkeypatch +): + """Rollback restores old registered skills and removes newly created ones.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"claude","ai_skills":true}', + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + + skills_root = project_dir / ".claude" / "skills" + old_skill = skills_root / "speckit-test-ext-old" + new_skill = skills_root / "speckit-test-ext-new" + _write_owned_extension_skill(old_skill, "test-ext", "OLD SKILL") + (old_skill / "support.txt").write_text("OLD SUPPORT", encoding="utf-8") + + registry_entry = { + "version": "1.0.0", + "enabled": True, + "registered_skills": [old_skill.name], + } + _stub_available_update(monkeypatch, registry_entry) + + mock_zip = project_dir / "mock.zip" + _write_update_zip( + mock_zip, + manifest=_valid_update_manifest( + command_name="speckit.test-ext.new" + ), + ) + monkeypatch.setattr( + ExtensionCatalog, + "download_extension", + lambda self, ext_id: mock_zip, + ) + + def mock_remove(self, ext_id, keep_config=False): + shutil.rmtree(old_skill) + + def mock_install_fail(*args, **kwargs): + # Simulate a write failure before ownership frontmatter is complete. + # The normal unregistrar must preserve unverifiable directories, so + # rollback relies on the absent-before-update snapshot to remove this. + new_skill.mkdir(parents=True) + (new_skill / "SKILL.md").write_text("---\n", encoding="utf-8") + raise RuntimeError("Install failed during skill registration") + + monkeypatch.setattr(ExtensionManager, "remove", mock_remove) + monkeypatch.setattr( + ExtensionManager, "install_from_zip", mock_install_fail + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Rollback successful" in result.output + assert (old_skill / "SKILL.md").exists() + assert "OLD SKILL" in (old_skill / "SKILL.md").read_text(encoding="utf-8") + assert (old_skill / "support.txt").read_text(encoding="utf-8") == "OLD SUPPORT" + assert not new_skill.exists() + + +def test_extension_update_partial_skill_backup_preserves_live_skills( + project_dir, monkeypatch +): + """An incomplete skill backup must abort before remove() touches live data.""" + monkeypatch.chdir(project_dir) + (project_dir / ".specify" / "init-options.json").write_text( + '{"ai":"claude","ai_skills":true}', + encoding="utf-8", + ) + (project_dir / ".specify" / "extensions.yml").write_text( + yaml.safe_dump({"installed": ["test-ext"], "hooks": {}}) + ) + + first_skill = ( + project_dir / ".claude" / "skills" / "speckit-test-ext-old" + ) + second_skill = ( + project_dir / ".claude" / "skills" / "speckit-test-ext-second" + ) + _write_owned_extension_skill(first_skill, "test-ext", "FIRST SKILL") + _write_owned_extension_skill(second_skill, "test-ext", "SECOND SKILL") + first_support = first_skill / "support.txt" + second_support = second_skill / "support.txt" + first_support.write_text("FIRST SUPPORT", encoding="utf-8") + second_support.write_text("SECOND SUPPORT", encoding="utf-8") + + registry_entry = { + "version": "1.0.0", + "enabled": True, + "registered_skills": [first_skill.name, second_skill.name], + } + _stub_available_update(monkeypatch, registry_entry) + + real_copytree = shutil.copytree + skill_backup_count = 0 + + def fail_partial_skill_backup(src, dst, *args, **kwargs): + nonlocal skill_backup_count + source = Path(src).resolve() + if source in {first_skill.resolve(), second_skill.resolve()}: + skill_backup_count += 1 + if skill_backup_count == 2: + Path(dst).mkdir(parents=True, exist_ok=True) + shutil.copy2( + Path(src) / "SKILL.md", + Path(dst) / "SKILL.md", + ) + raise OSError("Skill backup failed") + return real_copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copytree", fail_partial_skill_backup) + remove_calls = [] + monkeypatch.setattr( + ExtensionManager, + "remove", + lambda self, ext_id, keep_config=False: remove_calls.append(ext_id), + ) + + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + obj={"project_root": project_dir}, + ) + + assert result.exit_code == 1 + assert "Skill backup failed" in result.output + assert "Rolling back" not in result.output + assert remove_calls == [] + assert skill_backup_count == 2 + assert (first_skill / "SKILL.md").exists() + assert (second_skill / "SKILL.md").exists() + assert first_support.read_text(encoding="utf-8") == "FIRST SUPPORT" + assert second_support.read_text(encoding="utf-8") == "SECOND SUPPORT" + assert _update_backup_dirs(project_dir) == [] diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 9f19113153..afff0a2f82 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2145,6 +2145,29 @@ def test_install_zip_force_reinstall(self, extension_dir, project_dir): ext_dir = project_dir / ".specify" / "extensions" / "test-ext" assert ext_dir.exists() + def test_install_from_zip_rejects_symlink_entry( + self, extension_dir, project_dir, temp_dir + ): + """Extension ZIPs delegate to the shared symlink-safe extractor.""" + import stat + import zipfile + + zip_path = temp_dir / "symlink-extension.zip" + link = zipfile.ZipInfo("templates/escape") + link.create_system = 3 + link.external_attr = (stat.S_IFLNK | 0o777) << 16 + with zipfile.ZipFile(zip_path, "w") as zf: + for file_path in extension_dir.rglob("*"): + if file_path.is_file(): + zf.write(file_path, file_path.relative_to(extension_dir)) + zf.writestr(link, "../../outside") + + manager = ExtensionManager(project_dir) + with pytest.raises(ValidationError, match="Unsafe symlink"): + manager.install_from_zip(zip_path, "0.1.0") + + assert not manager.registry.is_installed("test-ext") + def test_install_duplicate_error_mentions_force(self, extension_dir, project_dir): """Test that duplicate install error message suggests --force.""" manager = ExtensionManager(project_dir) @@ -4685,7 +4708,7 @@ def test_fetch_single_catalog_sends_auth_header(self, temp_dir, monkeypatch): catalog_data = {"schema_version": "1.0", "extensions": {}} mock_response = MagicMock() - mock_response.read.return_value = json.dumps(catalog_data).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(catalog_data).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://raw.githubusercontent.com/org/repo/main/catalog.json" @@ -4829,7 +4852,7 @@ def test_fetch_single_catalog_rejects_malformed_payload(self, temp_dir, payload) catalog = self._make_catalog(temp_dir) mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -4898,7 +4921,7 @@ def test_fetch_single_catalog_rejects_malformed_cached_payload( "extensions": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -4946,7 +4969,7 @@ def test_fetch_catalog_rejects_malformed_payload(self, temp_dir, payload): catalog = self._make_catalog(temp_dir) mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -4987,7 +5010,7 @@ def test_fetch_catalog_recovers_from_unreadable_cache(self, temp_dir): "extensions": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -5026,7 +5049,7 @@ def test_fetch_catalog_recovers_from_unreadable_metadata(self, temp_dir): "extensions": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -5101,7 +5124,7 @@ def test_fetch_catalog_writes_cache_as_utf8(self, temp_dir, monkeypatch): "extensions": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode("utf-8") + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode("utf-8")).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -5152,11 +5175,13 @@ def test_fetch_catalog_survives_unwritable_cache(self, temp_dir, monkeypatch): "schema_version": "1.0", "extensions": {"foo": {"name": "Foo", "version": "1.0.0"}}, } - mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() - mock_response.__enter__ = lambda s: s - mock_response.__exit__ = MagicMock(return_value=False) - mock_response.geturl.return_value = "https://example.com/catalog.json" + def make_response(): + mock_response = MagicMock() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + mock_response.geturl.return_value = "https://example.com/catalog.json" + return mock_response # Simulate an unwritable cache dir: every write_text under the # cache directory raises PermissionError (an OSError subclass). @@ -5169,7 +5194,7 @@ def failing_write_text(self, data, *args, **kwargs): monkeypatch.setattr(_PathCls, "write_text", failing_write_text) - with patch.object(catalog, "_open_url", return_value=mock_response): + with patch.object(catalog, "_open_url", side_effect=lambda *a, **kw: make_response()): # Legacy single-catalog path. assert catalog.fetch_catalog(force_refresh=True) == valid @@ -5205,7 +5230,7 @@ def test_get_merged_extensions_skips_non_mapping_entries(self, temp_dir): }, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -5303,13 +5328,98 @@ def _mock_response(self, data): from unittest.mock import MagicMock resp = MagicMock() - resp.read.return_value = data + resp.read.side_effect = io.BytesIO(data).read # Configure the context-manager protocol explicitly so `with resp` # yields `resp` itself, independent of how the protocol is invoked. resp.__enter__.return_value = resp resp.__exit__.return_value = False return resp + def test_fetch_single_catalog_rejects_oversized_body_without_cache( + self, temp_dir, monkeypatch + ): + """Catalog bounds are enforced at the extension call site.""" + from unittest.mock import patch + + catalog = self._make_catalog(temp_dir) + entry = CatalogEntry( + url="https://example.com/catalog.json", + name="default", + priority=1, + install_allowed=True, + ) + body = b'{"schema_version":"1.0","extensions":{}}' + response = self._mock_response(body) + response.geturl.return_value = entry.url + monkeypatch.setattr(_ext_module, "MAX_JSON_CATALOG_BYTES", len(body) - 1) + + with patch.object(catalog, "_open_url", return_value=response): + with pytest.raises(ExtensionError, match="exceeds maximum size"): + catalog._fetch_single_catalog(entry, force_refresh=True) + + assert not catalog.cache_dir.exists() or not any(catalog.cache_dir.iterdir()) + + def test_download_extension_rejects_oversized_body_without_output( + self, temp_dir, monkeypatch + ): + """Package bounds fail before checksum verification or disk writes.""" + from unittest.mock import patch + from specify_cli._download_security import ( + read_response_limited as real_read_response_limited, + ) + + catalog = self._make_catalog(temp_dir) + ext_info = { + "id": "test-ext", + "name": "Test Extension", + "version": "1.0.0", + "download_url": "https://example.com/test-ext.zip", + } + + def read_with_tiny_limit(response, **kwargs): + kwargs.pop("max_bytes", None) + return real_read_response_limited(response, max_bytes=4, **kwargs) + + monkeypatch.setattr( + _ext_module, + "read_response_limited", + read_with_tiny_limit, + ) + with patch.object(_ext_module, "verify_archive_sha256") as verify, \ + patch.object(catalog, "get_extension_info", return_value=ext_info), \ + patch.object( + catalog, + "_open_url", + return_value=self._mock_response(b"12345"), + ): + with pytest.raises(ExtensionError, match="exceeds maximum size"): + catalog.download_extension("test-ext", target_dir=temp_dir) + + verify.assert_not_called() + assert not (temp_dir / "test-ext-1.0.0.zip").exists() + + def test_download_extension_rejects_unsafe_output_filename(self, temp_dir): + """Catalog-controlled IDs cannot escape the requested target directory.""" + from unittest.mock import patch + + catalog = self._make_catalog(temp_dir) + outside_stem = temp_dir.parent / "outside-extension" + extension_id = str(outside_stem) + ext_info = { + "id": extension_id, + "name": "Test Extension", + "version": "1.0.0", + "download_url": "https://example.com/test-ext.zip", + } + + with patch.object(catalog, "get_extension_info", return_value=ext_info), \ + patch.object(catalog, "_open_url") as open_url: + with pytest.raises(ExtensionError, match="filename"): + catalog.download_extension(extension_id, target_dir=temp_dir) + + open_url.assert_not_called() + assert not Path(f"{outside_stem}-1.0.0.zip").exists() + def test_download_extension_accepts_matching_sha256(self, temp_dir): """A catalog ``sha256`` that matches the archive is accepted.""" import hashlib @@ -5361,16 +5471,24 @@ def test_download_extension_malformed_url_raises_extension_error(self, temp_dir) from unittest.mock import patch catalog = self._make_catalog(temp_dir) - for bad_url in ("https://[::1", "https://[not-an-ip]/x"): + for bad_url in ( + "https://[::1", + "https://[not-an-ip]/x", + "https://example.com:65536/x", + "https:///x", + 123, + ): ext_info = { "id": "test-ext", "name": "Test Extension", "version": "1.0.0", "download_url": bad_url, } - with patch.object(catalog, "get_extension_info", return_value=ext_info): + with patch.object(catalog, "get_extension_info", return_value=ext_info), \ + patch.object(catalog, "_open_url") as open_url: with pytest.raises(ExtensionError, match="malformed"): catalog.download_extension("test-ext", target_dir=temp_dir) + open_url.assert_not_called() def test_download_extension_without_sha256_still_succeeds(self, temp_dir): """Entries without ``sha256`` keep working (backwards compatible).""" @@ -5407,7 +5525,7 @@ def test_download_extension_accepts_direct_github_rest_asset_url(self, temp_dir, zip_bytes = zip_buf.getvalue() asset_response = MagicMock() - asset_response.read.return_value = zip_bytes + asset_response.read.side_effect = io.BytesIO(zip_bytes).read asset_response.__enter__ = lambda s: s asset_response.__exit__ = MagicMock(return_value=False) @@ -6962,6 +7080,38 @@ def test_add_from_malformed_ipv6_url_exits_cleanly(self, tmp_path): plain = strip_ansi(result.output) assert "Invalid URL" in plain + @pytest.mark.parametrize( + "url", + [ + "https:///ext.zip", + "https://example.com:99999/ext.zip", + ], + ) + def test_add_from_invalid_url_exits_before_prompt(self, tmp_path, url): + """Hostless URLs and invalid ports fail before prompting or downloading.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("typer.confirm") as confirm, \ + patch("specify_cli.authentication.http.open_url") as open_url: + result = runner.invoke( + app, + ["extension", "add", "my-ext", "--from", url], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "Invalid URL" in strip_ansi(result.output) + confirm.assert_not_called() + open_url.assert_not_called() + def test_add_from_bracketed_non_ip_url_exits_cleanly(self, tmp_path): """A bracketed-but-invalid IPv6 host must produce a clean error, not a ValueError traceback. "https://[not-an-ip]/ext.zip" is a malformed @@ -7207,6 +7357,62 @@ def __exit__(self, exc_type, exc, tb): assert "did not return a ZIP archive" in result.output install.assert_not_called() + def test_add_from_url_rejects_oversized_download_before_install( + self, tmp_path, monkeypatch + ): + """The direct URL path must use the same bounded reader as catalogs.""" + import io + + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + from specify_cli.extensions import _commands as extension_commands + + class FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def reject_oversized(*_args, **_kwargs): + raise ExtensionError("extension URL download exceeds maximum size") + + monkeypatch.setattr( + extension_commands, + "read_response_limited", + reject_oversized, + raising=False, + ) + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("typer.confirm", return_value=True), \ + patch( + "specify_cli.authentication.http.open_url", + return_value=FakeResponse(_MINIMAL_ZIP_BYTES), + ), \ + patch.object(ExtensionManager, "install_from_zip") as install: + result = runner.invoke( + app, + [ + "extension", + "add", + "my-ext", + "--from", + "https://example.com/ext.zip", + ], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "exceeds maximum size" in result.output + install.assert_not_called() + def test_add_from_url_resolves_ghes_release_asset(self, tmp_path): """A GHES release-download URL resolves to /api/v3 with octet-stream Accept.""" import io @@ -7401,7 +7607,7 @@ def test_download_extension_allows_bundled_with_url(self, temp_dir): } mock_response = MagicMock() - mock_response.read.return_value = b"fake zip data" + mock_response.read.side_effect = io.BytesIO(b"fake zip data").read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -7479,7 +7685,12 @@ def _create_extension_source(base_dir: Path, version: str, include_config: bool return ext_dir @staticmethod - def _create_catalog_zip(zip_path: Path, version: str): + def _create_catalog_zip( + zip_path: Path, + version: str, + manifest_path: str = "extension.yml", + extra_manifest_path: str | None = None, + ): """Create a minimal ZIP that passes extension_update ID validation.""" import zipfile import yaml @@ -7497,9 +7708,243 @@ def _create_catalog_zip(zip_path: Path, version: str): } with zipfile.ZipFile(zip_path, "w") as zf: - zf.writestr("extension.yml", yaml.dump(manifest, sort_keys=False)) + manifest_text = yaml.dump(manifest, sort_keys=False) + zf.writestr(manifest_path, manifest_text) + if extra_manifest_path is not None: + zf.writestr(extra_manifest_path, manifest_text) - def test_update_success_preserves_installed_at(self, tmp_path): + @pytest.mark.parametrize( + "manifest_path", + [ + "../extension.yml", + "/extension.yml", + "./extension.yml", + "C:/extension.yml", + ], + ) + def test_update_rejects_unsafe_manifest_path_before_removal( + self, tmp_path, manifest_path + ): + """Unsafe manifest paths fail before the installed extension is removed.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + (project_dir / ".claude" / "skills").mkdir(parents=True) + + manager = ExtensionManager(project_dir) + v1_dir = self._create_extension_source(tmp_path, "1.0.0") + manager.install_from_directory(v1_dir, "0.1.0") + installed_extension_dir = manager.extensions_dir / "test-ext" + removed_paths = [] + real_rmtree = shutil.rmtree + + def track_rmtree(path, *args, **kwargs): + removed_paths.append(Path(path).resolve()) + return real_rmtree(path, *args, **kwargs) + + zip_path = tmp_path / "unsafe-manifest.zip" + self._create_catalog_zip( + zip_path, + "2.0.0", + manifest_path=manifest_path, + ) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "test-ext", + "name": "Test Extension", + "version": "2.0.0", + "_install_allowed": True, + }), \ + patch.object( + ExtensionCatalog, + "download_extension", + return_value=zip_path, + ), \ + patch.object(shutil, "rmtree", side_effect=track_rmtree), \ + patch.object(ExtensionManager, "remove") as remove, \ + patch.object(ExtensionManager, "install_from_zip") as install: + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + input="y\n", + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "Unsafe path in ZIP archive" in result.output + remove.assert_not_called() + install.assert_not_called() + assert installed_extension_dir.resolve() not in removed_paths + assert not list( + (manager.extensions_dir / ".backup").glob( + "update-*-*" + ) + ) + assert ExtensionManager(project_dir).registry.get("test-ext")["version"] == "1.0.0" + + @pytest.mark.parametrize( + ("first_path", "second_path"), + [ + ("repo/extension.yml", "repo\\extension.yml"), + ("repo/extension.yml", "repo/EXTENSION.YML"), + ("caf\u00e9/extension.yml", "cafe\u0301/extension.yml"), + ], + ) + def test_update_rejects_normalized_manifest_collision_before_removal( + self, tmp_path, first_path, second_path + ): + """Pre-scan and extraction must agree on the manifest identity.""" + import yaml + import zipfile + + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + (project_dir / ".claude" / "skills").mkdir(parents=True) + + manager = ExtensionManager(project_dir) + v1_dir = self._create_extension_source(tmp_path, "1.0.0") + manager.install_from_directory(v1_dir, "0.1.0") + + valid_manifest = yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": "test-ext", + "name": "Test Extension", + "version": "2.0.0", + }, + } + ) + injected_manifest = yaml.safe_dump( + { + "schema_version": "1.0", + "extension": { + "id": "injected", + "name": "Injected", + "version": "2.0.0", + }, + } + ) + zip_path = tmp_path / "manifest-collision.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr(first_path, valid_manifest) + zf.writestr(second_path, injected_manifest) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "test-ext", + "name": "Test Extension", + "version": "2.0.0", + "_install_allowed": True, + }), \ + patch.object( + ExtensionCatalog, + "download_extension", + return_value=zip_path, + ), \ + patch.object(ExtensionManager, "remove") as remove, \ + patch.object(ExtensionManager, "install_from_zip") as install: + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + input="y\n", + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "multiple extension.yml" in result.output + remove.assert_not_called() + install.assert_not_called() + assert ExtensionManager(project_dir).registry.get("test-ext")["version"] == "1.0.0" + + def test_update_preflights_entry_count_before_opening_zip( + self, tmp_path + ): + """Manifest inspection must not bypass the bounded ZIP opener.""" + import struct + + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + (project_dir / ".claude" / "skills").mkdir(parents=True) + + manager = ExtensionManager(project_dir) + v1_dir = self._create_extension_source(tmp_path, "1.0.0") + manager.install_from_directory(v1_dir, "0.1.0") + + zip_path = tmp_path / "too-many.zip" + zip_path.write_bytes( + struct.pack( + "<4s4H2LH", + b"PK\x05\x06", + 0, + 0, + 513, + 513, + 0, + 0, + 0, + ) + ) + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir), \ + patch.object(ExtensionCatalog, "get_extension_info", return_value={ + "id": "test-ext", + "name": "Test Extension", + "version": "2.0.0", + "_install_allowed": True, + }), \ + patch.object( + ExtensionCatalog, + "download_extension", + return_value=zip_path, + ), \ + patch( + "specify_cli._download_security.zipfile.ZipFile", + side_effect=AssertionError("ZipFile constructor was called"), + ), \ + patch.object(ExtensionManager, "remove") as remove, \ + patch.object(ExtensionManager, "install_from_zip") as install: + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + input="y\n", + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "too many entries" in result.output + remove.assert_not_called() + install.assert_not_called() + + @pytest.mark.parametrize( + ("manifest_path", "extra_manifest_path"), + [ + ("extension.yml", None), + ("repo/extension.yml", None), + ("extension.yml", "repo/extension.yml"), + ], + ) + def test_update_success_preserves_installed_at( + self, tmp_path, manifest_path, extra_manifest_path + ): """Successful update should keep original installed_at and apply new version.""" from typer.testing import CliRunner from unittest.mock import patch @@ -7520,7 +7965,12 @@ def test_update_success_preserves_installed_at(self, tmp_path): ).read_text() zip_path = tmp_path / "test-ext-update.zip" - self._create_catalog_zip(zip_path, "2.0.0") + self._create_catalog_zip( + zip_path, + "2.0.0", + manifest_path=manifest_path, + extra_manifest_path=extra_manifest_path, + ) v2_dir = self._create_extension_source(tmp_path, "2.0.0") def fake_install_from_zip(self_obj, _zip_path, speckit_version): @@ -7619,6 +8069,167 @@ def test_update_failure_rolls_back_registry_hooks_and_commands(self, tmp_path, m for cmd_file in command_files: assert cmd_file.exists(), f"Expected command file to be restored after rollback: {cmd_file}" + def test_update_failure_after_skill_registration_restores_old_skills( + self, tmp_path, monkeypatch + ): + """Rollback must not depend on a new registry entry to restore skills.""" + import zipfile + import yaml + + from specify_cli import app + from typer.testing import CliRunner + from unittest.mock import patch + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setattr(Path, "home", lambda: fake_home) + + project_dir = tmp_path / "project" + project_dir.mkdir() + specify_dir = project_dir / ".specify" + specify_dir.mkdir() + copilot_agents_dir = project_dir / ".github" / "agents" + copilot_agents_dir.mkdir(parents=True) + (specify_dir / "init-options.json").write_text( + json.dumps( + { + "ai": "claude", + "ai_skills": True, + "script": "sh", + } + ), + encoding="utf-8", + ) + + manager = ExtensionManager(project_dir) + v1_dir = self._create_extension_source(tmp_path, "1.0.0") + manager.install_from_directory( + v1_dir, + "0.1.0", + register_commands=False, + ) + + old_registry_entry = manager.registry.get("test-ext") + skills_dir = project_dir / ".claude" / "skills" + old_skill = skills_dir / "speckit-test-ext-hello" + old_skill_content = (old_skill / "SKILL.md").read_text(encoding="utf-8") + assert old_registry_entry["registered_skills"] == [old_skill.name] + new_skill = skills_dir / "speckit-test-ext-new" + new_skill.mkdir() + user_skill_content = ( + "---\n" + "name: user-new-skill\n" + "description: User-owned skill\n" + "metadata:\n" + " source: user\n" + "---\n\nUSER SKILL\n" + ) + (new_skill / "SKILL.md").write_text( + user_skill_content, + encoding="utf-8", + ) + user_support_file = new_skill / "support.txt" + user_support_file.write_text("USER CONTENT", encoding="utf-8") + + v2_dir = self._create_extension_source(tmp_path, "2.0.0") + manifest_path = v2_dir / "extension.yml" + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + manifest["provides"]["commands"].append( + { + "name": "speckit.test-ext.new", + "file": "commands/new.md", + "description": "New command", + } + ) + manifest["provides"]["commands"].append( + { + "name": "speckit.test-ext.fresh", + "file": "commands/fresh.md", + "description": "Fresh command", + } + ) + manifest_path.write_text( + yaml.safe_dump(manifest, sort_keys=False), + encoding="utf-8", + ) + (v2_dir / "commands" / "hello.md").write_text( + "---\ndescription: New hello\n---\n\nNEW HELLO\n", + encoding="utf-8", + ) + (v2_dir / "commands" / "new.md").write_text( + "---\ndescription: New command\n---\n\nNEW COMMAND\n", + encoding="utf-8", + ) + (v2_dir / "commands" / "fresh.md").write_text( + "---\ndescription: Fresh command\n---\n\nFRESH COMMAND\n", + encoding="utf-8", + ) + + zip_path = tmp_path / "test-ext-update.zip" + with zipfile.ZipFile(zip_path, "w") as archive: + for source_path in v2_dir.rglob("*"): + if source_path.is_file(): + archive.write( + source_path, + source_path.relative_to(v2_dir), + ) + + def fail_after_skill_registration(self, manifest): + raise RuntimeError("Hook registration failed") + + runner = CliRunner() + with ( + patch.object(Path, "cwd", return_value=project_dir), + patch.object( + ExtensionCatalog, + "get_extension_info", + return_value={ + "id": "test-ext", + "name": "Test Extension", + "version": "2.0.0", + "_install_allowed": True, + }, + ), + patch.object( + ExtensionCatalog, + "download_extension", + return_value=zip_path, + ), + patch.object( + HookExecutor, + "register_hooks", + fail_after_skill_registration, + ), + ): + result = runner.invoke( + app, + ["extension", "update", "test-ext"], + input="y\n", + catch_exceptions=True, + ) + + assert result.exit_code == 1, result.output + assert "Hook registration failed" in result.output + assert "Rollback successful" in result.output + assert ExtensionManager(project_dir).registry.get("test-ext") == old_registry_entry + assert (old_skill / "SKILL.md").read_text(encoding="utf-8") == old_skill_content + assert user_support_file.read_text(encoding="utf-8") == "USER CONTENT" + assert ( + new_skill / "SKILL.md" + ).read_text(encoding="utf-8") == user_skill_content + assert not (skills_dir / "speckit-test-ext-fresh").exists() + for command_name in ("hello", "new", "fresh"): + qualified_name = f"speckit.test-ext.{command_name}" + assert not ( + copilot_agents_dir / f"{qualified_name}.agent.md" + ).exists() + assert not ( + project_dir + / ".github" + / "prompts" + / f"{qualified_name}.prompt.md" + ).exists() + @pytest.mark.parametrize( ("manifest_text", "expected_detail"), [ diff --git a/tests/test_presets.py b/tests/test_presets.py index adf6484583..05a1fa9bae 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -654,6 +654,28 @@ def test_install_from_zip_no_manifest(self, project_dir, temp_dir): with pytest.raises(PresetValidationError, match="No preset.yml found"): manager.install_from_zip(zip_path, "0.1.5") + def test_install_from_zip_rejects_symlink_entry( + self, project_dir, pack_dir, temp_dir + ): + """Preset ZIPs delegate to the shared symlink-safe extractor.""" + import stat + + zip_path = temp_dir / "symlink-preset.zip" + link = zipfile.ZipInfo("templates/escape") + link.create_system = 3 + link.external_attr = (stat.S_IFLNK | 0o777) << 16 + with zipfile.ZipFile(zip_path, "w") as zf: + for file_path in pack_dir.rglob("*"): + if file_path.is_file(): + zf.write(file_path, file_path.relative_to(pack_dir)) + zf.writestr(link, "../../outside") + + manager = PresetManager(project_dir) + with pytest.raises(PresetValidationError, match="Unsafe symlink"): + manager.install_from_zip(zip_path, "0.1.5") + + assert not manager.registry.is_installed("test-pack") + def test_remove(self, project_dir, pack_dir): """Test removing a preset.""" manager = PresetManager(project_dir) @@ -1740,7 +1762,7 @@ def test_fetch_single_catalog_sends_auth_header(self, project_dir, monkeypatch): catalog_data = {"schema_version": "1.0", "presets": {}} mock_response = MagicMock() - mock_response.read.return_value = json.dumps(catalog_data).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(catalog_data).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://raw.githubusercontent.com/org/repo/main/presets/catalog.json" @@ -1893,7 +1915,7 @@ def test_fetch_single_catalog_rejects_malformed_payload(self, project_dir, paylo catalog = PresetCatalog(project_dir) mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) # A real urllib response reports the final URL (== request URL with no @@ -1965,7 +1987,7 @@ def test_fetch_single_catalog_rejects_malformed_cached_payload( "presets": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = catalog.DEFAULT_CATALOG_URL @@ -2013,7 +2035,7 @@ def test_fetch_catalog_rejects_malformed_payload(self, project_dir, payload): catalog = PresetCatalog(project_dir) mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -2055,7 +2077,7 @@ def test_fetch_catalog_recovers_from_unreadable_cache(self, project_dir): "presets": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -2094,7 +2116,7 @@ def test_fetch_catalog_recovers_from_unreadable_metadata(self, project_dir): "presets": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -2165,7 +2187,7 @@ def test_fetch_catalog_writes_cache_as_utf8(self, project_dir, monkeypatch): "presets": {"foo": {"name": "Foo", "version": "1.0.0"}}, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode("utf-8") + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode("utf-8")).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -2214,11 +2236,13 @@ def test_fetch_catalog_survives_unwritable_cache(self, project_dir, monkeypatch) "schema_version": "1.0", "presets": {"foo": {"name": "Foo", "version": "1.0.0"}}, } - mock_response = MagicMock() - mock_response.read.return_value = json.dumps(valid).encode() - mock_response.__enter__ = lambda s: s - mock_response.__exit__ = MagicMock(return_value=False) - mock_response.geturl.return_value = catalog.DEFAULT_CATALOG_URL + def make_response(): + mock_response = MagicMock() + mock_response.read.side_effect = io.BytesIO(json.dumps(valid).encode()).read + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + mock_response.geturl.return_value = catalog.DEFAULT_CATALOG_URL + return mock_response # Simulate an unwritable cache dir: every write_text under the # cache directory raises PermissionError (an OSError subclass). @@ -2231,7 +2255,7 @@ def failing_write_text(self, data, *args, **kwargs): monkeypatch.setattr(_PathCls, "write_text", failing_write_text) - with patch.object(catalog, "_open_url", return_value=mock_response): + with patch.object(catalog, "_open_url", side_effect=lambda *a, **kw: make_response()): # Legacy single-catalog path. assert catalog.fetch_catalog(force_refresh=True) == valid @@ -2268,7 +2292,7 @@ def test_get_merged_packs_skips_non_mapping_entries(self, project_dir): }, } mock_response = MagicMock() - mock_response.read.return_value = json.dumps(payload).encode() + mock_response.read.side_effect = io.BytesIO(json.dumps(payload).encode()).read mock_response.__enter__ = lambda s: s mock_response.__exit__ = MagicMock(return_value=False) mock_response.geturl.return_value = "https://example.com/catalog.json" @@ -2361,13 +2385,109 @@ def _pack_zip_and_response(self): zip_bytes = zip_buf.getvalue() resp = MagicMock() - resp.read.return_value = zip_bytes + resp.read.side_effect = io.BytesIO(zip_bytes).read # Configure the context-manager protocol explicitly so `with resp` # yields `resp` itself, independent of how the protocol is invoked. resp.__enter__.return_value = resp resp.__exit__.return_value = False return zip_bytes, resp + def test_fetch_single_catalog_rejects_oversized_body_without_cache( + self, project_dir, monkeypatch + ): + """Catalog bounds are enforced at the preset call site.""" + import specify_cli.presets as preset_module + from unittest.mock import patch + + catalog = PresetCatalog(project_dir) + entry = PresetCatalogEntry( + url="https://example.com/catalog.json", + name="default", + priority=1, + install_allowed=True, + ) + body = b'{"schema_version":"1.0","presets":{}}' + response = MagicMock() + response.read.side_effect = io.BytesIO(body).read + response.__enter__.return_value = response + response.__exit__.return_value = False + response.geturl.return_value = entry.url + monkeypatch.setattr( + preset_module, + "MAX_JSON_CATALOG_BYTES", + len(body) - 1, + ) + + with patch.object(catalog, "_open_url", return_value=response): + with pytest.raises(PresetError, match="exceeds maximum size"): + catalog._fetch_single_catalog(entry, force_refresh=True) + + assert not catalog.cache_dir.exists() or not any(catalog.cache_dir.iterdir()) + + def test_download_pack_rejects_oversized_body_without_output( + self, project_dir, monkeypatch + ): + """Package bounds fail before checksum verification or disk writes.""" + import specify_cli.presets as preset_module + from unittest.mock import patch + from specify_cli._download_security import ( + read_response_limited as real_read_response_limited, + ) + + catalog = PresetCatalog(project_dir) + pack_info = { + "id": "test-pack", + "name": "Test Pack", + "version": "1.0.0", + "download_url": "https://example.com/test-pack.zip", + "_install_allowed": True, + } + response = MagicMock() + response.read.side_effect = io.BytesIO(b"12345").read + response.__enter__.return_value = response + response.__exit__.return_value = False + + def read_with_tiny_limit(stream, **kwargs): + kwargs.pop("max_bytes", None) + return real_read_response_limited(stream, max_bytes=4, **kwargs) + + monkeypatch.setattr( + preset_module, + "read_response_limited", + read_with_tiny_limit, + ) + with patch.object(preset_module, "verify_archive_sha256") as verify, \ + patch.object(catalog, "get_pack_info", return_value=pack_info), \ + patch.object(catalog, "_open_url", return_value=response): + with pytest.raises(PresetError, match="exceeds maximum size"): + catalog.download_pack("test-pack", target_dir=project_dir) + + verify.assert_not_called() + assert not (project_dir / "test-pack-1.0.0.zip").exists() + + def test_download_pack_rejects_unsafe_output_filename(self, project_dir): + """Catalog-controlled IDs cannot escape the requested target directory.""" + from unittest.mock import patch + + catalog = PresetCatalog(project_dir) + outside_stem = project_dir.parent / "outside-preset" + pack_id = str(outside_stem) + pack_info = { + "id": pack_id, + "name": "Test Pack", + "version": "1.0.0", + "download_url": "https://example.com/test-pack.zip", + "_install_allowed": True, + } + + with patch.object(catalog, "get_pack_info", return_value=pack_info), \ + patch.object(catalog, "_open_url") as open_url: + with pytest.raises(PresetError, match="filename"): + catalog.download_pack(pack_id, target_dir=project_dir) + + open_url.assert_not_called() + assert not Path(f"{outside_stem}-1.0.0.zip").exists() + def test_download_pack_accepts_matching_sha256(self, project_dir): """A catalog ``sha256`` that matches the preset archive is accepted.""" import hashlib @@ -2420,7 +2540,13 @@ def test_download_pack_malformed_url_raises_preset_error(self, project_dir): from unittest.mock import patch catalog = PresetCatalog(project_dir) - for bad_url in ("https://[::1", "https://[not-an-ip]/x"): + for bad_url in ( + "https://[::1", + "https://[not-an-ip]/x", + "https://example.com:65536/x", + "https:///x", + 123, + ): pack_info = { "id": "test-pack", "name": "Test Pack", @@ -2428,9 +2554,11 @@ def test_download_pack_malformed_url_raises_preset_error(self, project_dir): "download_url": bad_url, "_install_allowed": True, } - with patch.object(catalog, "get_pack_info", return_value=pack_info): + with patch.object(catalog, "get_pack_info", return_value=pack_info), \ + patch.object(catalog, "_open_url") as open_url: with pytest.raises(PresetError, match="malformed"): catalog.download_pack("test-pack", target_dir=project_dir) + open_url.assert_not_called() def test_download_pack_without_sha256_skips_verification(self, project_dir): """A catalog entry with no ``sha256`` keeps working: verification is @@ -2471,7 +2599,7 @@ def test_download_pack_accepts_direct_github_rest_asset_url(self, project_dir, m zip_bytes = zip_buf.getvalue() asset_response = MagicMock() - asset_response.read.return_value = zip_bytes + asset_response.read.side_effect = io.BytesIO(zip_bytes).read asset_response.__enter__ = lambda s: s asset_response.__exit__ = MagicMock(return_value=False) @@ -9791,8 +9919,8 @@ def geturl(self): assert "redirected to a disallowed URL" in output assert "must use HTTPS with a hostname" in output - def test_preset_add_from_url_streams_download_to_zip(self, project_dir, monkeypatch): - """URL installs stream response bytes to disk before installing the ZIP.""" + def test_preset_add_from_url_reads_in_bounded_chunks(self, project_dir, monkeypatch): + """URL installs read the response in bounded chunks.""" from specify_cli.presets._commands import preset_add class FakeResponse(io.BytesIO): @@ -9840,6 +9968,65 @@ def fake_install_from_zip(self, zip_path, speckit_version, priority=10): "priority": 7, } + def test_preset_add_from_url_rejects_oversized_download( + self, project_dir, monkeypatch, capsys + ): + """An oversized direct download fails before preset installation.""" + import typer + from specify_cli._download_security import ( + read_response_limited as real_read_response_limited, + ) + from specify_cli.presets import _commands as preset_commands + + class FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def geturl(self): + return "https://example.com/preset.zip" + + def read_with_tiny_limit(response, **kwargs): + kwargs.pop("max_bytes", None) + return real_read_response_limited(response, max_bytes=4, **kwargs) + + installed = False + + def fake_install_from_zip(*_args, **_kwargs): + nonlocal installed + installed = True + + monkeypatch.setattr( + preset_commands, + "read_response_limited", + read_with_tiny_limit, + ) + monkeypatch.setattr( + "specify_cli._require_specify_project", + lambda: project_dir, + ) + monkeypatch.setattr("specify_cli.get_speckit_version", lambda: "0.6.0") + monkeypatch.setattr( + "specify_cli.authentication.http.open_url", + lambda *_args, **_kwargs: FakeResponse(b"12345"), + ) + monkeypatch.setattr(PresetManager, "install_from_zip", fake_install_from_zip) + + with pytest.raises(typer.Exit) as exc_info: + preset_commands.preset_add( + preset_id=None, + from_url="https://example.com/preset.zip", + dev=None, + priority=10, + ) + + assert exc_info.value.exit_code == 1 + output = " ".join(strip_ansi(capsys.readouterr().out).split()) + assert "exceeds maximum size of 4 bytes" in output + assert installed is False + def test_bundled_preset_in_catalog(self): """Verify the lean preset is listed in catalog.json with bundled marker.""" catalog_path = Path(__file__).parent.parent / "presets" / "catalog.json" diff --git a/tests/test_registrar_path_traversal.py b/tests/test_registrar_path_traversal.py index 1f97a98d87..141fa05186 100644 --- a/tests/test_registrar_path_traversal.py +++ b/tests/test_registrar_path_traversal.py @@ -13,7 +13,13 @@ "../pwned", "../../etc/passwd", "subdir/../../escape", + "link/../victim", "/absolute/evil", + "NUL", + "name:stream", + "x?y", + "trailing.", + "group\\run", ] @@ -89,7 +95,8 @@ class TestAliasTraversal: @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) + commands_dir = project / ".gemini" / "commands" + commands_dir.mkdir(parents=True) registrar = CommandRegistrar() with pytest.raises(ValueError, match="escapes|outside|Invalid"): @@ -102,12 +109,15 @@ def test_gemini_rejects_traversal_in_alias(self, tmp_path, bad_alias): ) _assert_no_stray_files(tmp_path, Path(bad_alias).name.replace("/", "")) + assert list(commands_dir.rglob("*")) == [] @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) + agents_dir = project / ".github" / "agents" + prompts_dir = project / ".github" / "prompts" + agents_dir.mkdir(parents=True) + prompts_dir.mkdir(parents=True) registrar = CommandRegistrar() with pytest.raises(ValueError, match="escapes|outside|Invalid"): @@ -120,6 +130,8 @@ def test_copilot_rejects_traversal_in_alias(self, tmp_path, bad_alias): ) _assert_no_stray_files(tmp_path, Path(bad_alias).name.replace("/", "")) + assert list(agents_dir.rglob("*")) == [] + assert list(prompts_dir.rglob("*")) == [] class TestCopilotPromptTraversal: @@ -290,6 +302,13 @@ def test_safe_relative_paths_have_no_violation(self, value): "\\\\server\\share\\x.md", "../escape.md", "commands/../../escape.md", + "NUL", + "commands/CON.md", + "commands\\run.md", + "name:stream", + "x?y", + "trailing.", + "directory/", ], ) def test_unsafe_values_report_violation(self, value): @@ -380,3 +399,27 @@ def test_safe_command_and_alias_still_register(self, tmp_path): / "speckit-myext-hi" / "SKILL.md" ).exists() + + def test_copilot_nested_alias_creates_companion_prompt(self, tmp_path): + project, ext_dir = _project_and_source(tmp_path) + agents_dir = project / ".github" / "agents" + agents_dir.mkdir(parents=True) + + registrar = CommandRegistrar() + registered = registrar.register_commands( + "copilot", + [_cmd("speckit.myext.hello", ["group/run"])], + "myext", + ext_dir, + project, + ) + + assert registered == ["speckit.myext.hello", "group/run"] + assert (agents_dir / "group" / "run.agent.md").is_file() + assert ( + project + / ".github" + / "prompts" + / "group" + / "run.prompt.md" + ).is_file() diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 969daf9c1d..61c625a396 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -6398,6 +6398,7 @@ def test_validate_url_localhost_http_allowed(self, project_dir): [ "https://[::1", # unterminated IPv6 bracket "https://[not-an-ip]/x", # bracketed non-IP host + "https://example.com:notaport/catalog.json", ], ) def test_validate_url_malformed_raises_validation_error(self, project_dir, url): @@ -6507,6 +6508,62 @@ def fake_open(url, timeout=30, redirect_validator=None): catalog._fetch_single_catalog(entry, force_refresh=True) assert captured["rv"] is not None + def test_fetch_rejects_oversized_catalog_response( + self, project_dir, monkeypatch + ): + from specify_cli.authentication import http as auth_http + from specify_cli.workflows import catalog as catalog_module + from specify_cli.workflows.catalog import ( + WorkflowCatalog, + WorkflowCatalogEntry, + WorkflowCatalogError, + ) + + monkeypatch.setattr(catalog_module, "MAX_JSON_CATALOG_BYTES", 32) + requested_sizes: list[int] = [] + + class _FakeResponse: + def __init__(self): + self.body = b"x" * 64 + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def geturl(self): + return "https://example.com/catalog.json" + + def read(self, size=-1): + requested_sizes.append(size) + assert size >= 0 + chunk_size = min(size, 7) + chunk = self.body[self.offset : self.offset + chunk_size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _FakeResponse(), + ) + + catalog = WorkflowCatalog(project_dir) + entry = WorkflowCatalogEntry( + url="https://example.com/catalog.json", + name="test", + priority=1, + install_allowed=True, + ) + + with pytest.raises(WorkflowCatalogError, match="exceeds maximum size"): + catalog._fetch_single_catalog(entry, force_refresh=True) + + assert requested_sizes + assert not catalog.cache_dir.exists() + def test_add_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -7002,6 +7059,7 @@ def test_validate_url_localhost_http_allowed(self, project_dir): [ "https://[::1", # unterminated IPv6 bracket "https://[not-an-ip]/x", # bracketed non-IP host + "https://example.com:notaport/steps.json", ], ) def test_validate_url_malformed_raises_validation_error(self, project_dir, url): @@ -7103,6 +7161,62 @@ def fake_open(url, timeout=30, redirect_validator=None): catalog._fetch_single_catalog(entry, force_refresh=True) assert captured["rv"] is not None + def test_fetch_rejects_oversized_catalog_response( + self, project_dir, monkeypatch + ): + from specify_cli.authentication import http as auth_http + from specify_cli.workflows import catalog as catalog_module + from specify_cli.workflows.catalog import ( + StepCatalog, + StepCatalogEntry, + StepCatalogError, + ) + + monkeypatch.setattr(catalog_module, "MAX_JSON_CATALOG_BYTES", 32) + requested_sizes: list[int] = [] + + class _FakeResponse: + def __init__(self): + self.body = b"x" * 64 + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def geturl(self): + return "https://example.com/steps.json" + + def read(self, size=-1): + requested_sizes.append(size) + assert size >= 0 + chunk_size = min(size, 7) + chunk = self.body[self.offset : self.offset + chunk_size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _FakeResponse(), + ) + + catalog = StepCatalog(project_dir) + entry = StepCatalogEntry( + url="https://example.com/steps.json", + name="test", + priority=1, + install_allowed=True, + ) + + with pytest.raises(StepCatalogError, match="exceeds maximum size"): + catalog._fetch_single_catalog(entry, force_refresh=True) + + assert requested_sizes + assert not catalog.cache_dir.exists() + def test_add_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog @@ -8415,6 +8529,269 @@ def read(self, size=-1): project_dir / ".specify" / "workflows" / "steps" / "my-step" ).exists() + @pytest.mark.parametrize( + ("catalog_fields", "expected"), + [ + ({"url": 123}, "malformed step.yml URL"), + ( + { + "step_yml_url": [], + "url": "https://example.com/step.yml", + }, + "malformed step.yml URL", + ), + ( + { + "url": "https://example.com/step.yml", + "init_url": 123, + }, + "malformed __init__.py URL", + ), + ], + ) + def test_add_rejects_non_string_required_urls_before_network( + self, project_dir, monkeypatch, catalog_fields, expected + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "_install_allowed": True, + **catalog_fields, + }, + ) + monkeypatch.setattr( + auth_http, + "open_url", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("download should not start") + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert expected in result.output + assert not ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" + ).exists() + + @pytest.mark.parametrize( + ("alias", "protected_name"), + [ + ("./step.yml", "step.yml"), + ("step.yml/", "step.yml"), + ("STEP.YML", "step.yml"), + (".\\step.yml", "step.yml"), + ("./__init__.py", "__init__.py"), + ("__init__.py/", "__init__.py"), + ("__INIT__.PY", "__init__.py"), + (".\\__init__.py", "__init__.py"), + ], + ) + def test_add_does_not_overwrite_required_files_through_path_aliases( + self, project_dir, monkeypatch, alias, protected_name + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + alias_url = "https://example.com/overwrite" + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + "extra_files": {alias: alias_url}, + }, + ) + bodies = { + "https://example.com/step.yml": b"step:\n type_key: my-step\n", + "https://example.com/__init__.py": b"# trusted init\n", + } + requested_urls: list[str] = [] + + class _FakeResponse: + def __init__(self, url): + self.url = url + self.body = bodies[url] + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + def fake_open_url(url, timeout=30, redirect_validator=None): + requested_urls.append(url) + return _FakeResponse(url) + + monkeypatch.setattr(auth_http, "open_url", fake_open_url) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code == 0, result.output + assert alias_url not in requested_urls + installed_dir = ( + project_dir / ".specify" / "workflows" / "steps" / "my-step" + ) + assert (installed_dir / protected_name).read_bytes() == bodies[ + f"https://example.com/{protected_name}" + ] + + def test_add_rejects_too_many_package_files_before_network( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows import _commands as workflow_commands + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(workflow_commands, "_MAX_STEP_PACKAGE_FILES", 3) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + "extra_files": { + "one.py": "https://example.com/one.py", + "two.py": "https://example.com/two.py", + }, + }, + ) + monkeypatch.setattr( + auth_http, + "open_url", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("download should not start") + ), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "exceeding the 3-file limit" in result.output + steps_dir = project_dir / ".specify" / "workflows" / "steps" + assert not (steps_dir / "my-step").exists() + assert list(steps_dir.glob("speckit_step_tmp_*")) == [] + + def test_add_rejects_package_over_cumulative_size_and_cleans_staging( + self, project_dir, monkeypatch + ): + from typer.testing import CliRunner + + from specify_cli import app + from specify_cli.authentication import http as auth_http + from specify_cli.workflows import _commands as workflow_commands + from specify_cli.workflows.catalog import StepCatalog + + monkeypatch.chdir(project_dir) + monkeypatch.setattr(workflow_commands, "_MAX_STEP_PACKAGE_BYTES", 40) + monkeypatch.setattr( + StepCatalog, + "get_step_info", + lambda self, step_id: { + "id": step_id, + "name": "Test Step", + "url": "https://example.com/step.yml", + "init_url": "https://example.com/__init__.py", + "_install_allowed": True, + "extra_files": { + "helper.py": "https://example.com/helper.py", + }, + }, + ) + + bodies = { + "https://example.com/step.yml": b"step:\n type_key: my-step\n", + "https://example.com/__init__.py": b"# init\n", + "https://example.com/helper.py": b"0123456789", + } + + class _FakeResponse: + def __init__(self, url): + self.url = url + self.body = bodies[url] + self.offset = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def getheader(self, name): + return None + + def geturl(self): + return self.url + + def read(self, size=-1): + if size < 0: + size = len(self.body) - self.offset + chunk = self.body[self.offset : self.offset + size] + self.offset += len(chunk) + return chunk + + monkeypatch.setattr( + auth_http, + "open_url", + lambda url, timeout=30, redirect_validator=None: _FakeResponse(url), + ) + + result = CliRunner().invoke( + app, ["workflow", "step", "add", "my-step"] + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "40-byte total size limit" in result.output + steps_dir = project_dir / ".specify" / "workflows" / "steps" + assert not (steps_dir / "my-step").exists() + assert list(steps_dir.glob("speckit_step_tmp_*")) == [] + def test_add_rejects_non_string_extra_files_key(self, project_dir, monkeypatch): from typer.testing import CliRunner from specify_cli import app diff --git a/tests/unit/test_bundle_download_url.py b/tests/unit/test_bundle_download_url.py index 6a53b9368b..6a29423b9d 100644 --- a/tests/unit/test_bundle_download_url.py +++ b/tests/unit/test_bundle_download_url.py @@ -1,19 +1,59 @@ """Unit tests for malformed download-URL handling in bundle manifest resolution.""" from __future__ import annotations +import hashlib +import io from types import SimpleNamespace import pytest +import yaml from specify_cli.bundler import BundlerError +from specify_cli.bundler.models.catalog import CatalogEntry +from specify_cli.commands import bundle as bundle_commands from specify_cli.commands.bundle import _download_manifest, _require_https +from tests.bundler_helpers import catalog_entry_dict, valid_manifest_dict _MALFORMED_URLS = [ "https://[::1", # unclosed IPv6 bracket "https://[not-an-ip]/bundle.yml", + "https://example.com:notaport/bundle.yml", + "https://example.com:70000/bundle.yml", ] +class _Response(io.BytesIO): + def __init__(self, body: bytes, url: str) -> None: + super().__init__(body) + self._url = url + + def geturl(self) -> str: + return self._url + + +def _resolved_entry(**overrides) -> SimpleNamespace: + entry = CatalogEntry.from_dict( + catalog_entry_dict( + "demo-bundle", + download_url="https://example.com/demo-bundle.yml", + **overrides, + ) + ) + return SimpleNamespace(entry=entry) + + +def _patch_download(monkeypatch, body: bytes) -> None: + def fake_open_url( + url, + timeout=10, + extra_headers=None, + redirect_validator=None, + ): + return _Response(body, url) + + monkeypatch.setattr("specify_cli.authentication.http.open_url", fake_open_url) + + @pytest.mark.parametrize("url", _MALFORMED_URLS) def test_download_manifest_rejects_malformed_url_cleanly(url): """A malformed download_url must raise BundlerError, not a raw ValueError. @@ -40,3 +80,83 @@ def test_require_https_rejects_malformed_url_cleanly(url): """ with pytest.raises(BundlerError): _require_https("bundle 'x'", url) + + +def test_download_manifest_bounds_remote_artifact(monkeypatch): + body = yaml.safe_dump(valid_manifest_dict()).encode() + _patch_download(monkeypatch, body) + monkeypatch.setattr(bundle_commands, "MAX_DOWNLOAD_BYTES", len(body) - 1) + + with pytest.raises(BundlerError, match="exceeds maximum size"): + _download_manifest(_resolved_entry(), offline=False) + + +def test_download_manifest_accepts_matching_sha256(monkeypatch): + body = yaml.safe_dump(valid_manifest_dict()).encode() + digest = hashlib.sha256(body).hexdigest() + _patch_download(monkeypatch, body) + + manifest = _download_manifest( + _resolved_entry(sha256=f"sha256:{digest}"), + offline=False, + ) + + assert manifest.bundle.id == "demo-bundle" + + +def test_download_manifest_accepts_legacy_entry_without_sha256(monkeypatch): + body = yaml.safe_dump(valid_manifest_dict()).encode() + _patch_download(monkeypatch, body) + resolved = SimpleNamespace( + entry=SimpleNamespace( + id="demo-bundle", + version="1.2.0", + download_url="https://example.com/demo-bundle.yml", + ) + ) + + manifest = _download_manifest(resolved, offline=False) + + assert manifest.bundle.version == "1.2.0" + + +@pytest.mark.parametrize("declared", ["0" * 64, "not-a-sha256"]) +def test_download_manifest_rejects_bad_sha256(monkeypatch, declared): + body = yaml.safe_dump(valid_manifest_dict()).encode() + _patch_download(monkeypatch, body) + + with pytest.raises(BundlerError, match="sha256|Integrity check"): + _download_manifest( + _resolved_entry(sha256=declared), + offline=False, + ) + + +@pytest.mark.parametrize( + ("field", "value", "message"), + [ + ("id", "other-bundle", "id mismatch"), + ("version", "9.9.9", "version mismatch"), + ], +) +def test_download_manifest_rejects_catalog_identity_mismatch( + monkeypatch, + field, + value, + message, +): + data = valid_manifest_dict() + data["bundle"][field] = value + _patch_download(monkeypatch, yaml.safe_dump(data).encode()) + + with pytest.raises(BundlerError, match=message): + _download_manifest(_resolved_entry(), offline=False) + + +def test_download_manifest_rejects_invalid_structure(monkeypatch): + data = valid_manifest_dict() + data["bundle"]["author"] = "" + _patch_download(monkeypatch, yaml.safe_dump(data).encode()) + + with pytest.raises(BundlerError, match="invalid bundle manifest"): + _download_manifest(_resolved_entry(), offline=False) diff --git a/tests/unit/test_bundler_adapters.py b/tests/unit/test_bundler_adapters.py index 0212e850db..5ce9e10a12 100644 --- a/tests/unit/test_bundler_adapters.py +++ b/tests/unit/test_bundler_adapters.py @@ -20,6 +20,7 @@ def _source(url: str) -> CatalogSource: class _FakeResponse: def __init__(self, body: bytes, final_url: str) -> None: self._body = body + self._offset = 0 self._final_url = final_url def __enter__(self) -> "_FakeResponse": @@ -31,8 +32,12 @@ def __exit__(self, *exc) -> bool: def geturl(self) -> str: return self._final_url - def read(self) -> bytes: - return self._body + def read(self, size: int = -1) -> bytes: + if size < 0: + size = len(self._body) - self._offset + start = self._offset + self._offset = min(len(self._body), self._offset + size) + return self._body[start:self._offset] def test_http_fetch_uses_shared_client_and_rejects_redirect_downgrade(monkeypatch): @@ -71,6 +76,34 @@ def fake_open_url(url, timeout=10, extra_headers=None, redirect_validator=None): fetcher(_source("https://example.com/c.json")) +def test_http_fetch_bounds_catalog_response(monkeypatch): + body = b'{"schema_version":"1.0","bundles":{}}' + + def fake_open_url(url, timeout=10, extra_headers=None, redirect_validator=None): + return _FakeResponse(body, url) + + monkeypatch.setattr("specify_cli.authentication.http.open_url", fake_open_url) + monkeypatch.setattr(adapters, "MAX_JSON_CATALOG_BYTES", len(body) - 1) + + fetcher = adapters.make_catalog_fetcher(allow_network=True) + with pytest.raises(BundlerError, match="exceeds maximum size"): + fetcher(_source("https://example.com/c.json")) + + +@pytest.mark.parametrize( + "url", + [ + "https://[::1", + "https://example.com:notaport/catalog.json", + "https://example.com:70000/catalog.json", + ], +) +def test_fetch_rejects_malformed_source_url_cleanly(url): + fetcher = adapters.make_catalog_fetcher(allow_network=True) + with pytest.raises(BundlerError, match="URL is malformed"): + fetcher(_source(url)) + + def test_builtin_community_catalog_fetches_repository_catalog_online(monkeypatch): captured: dict = {}