From c2e4f81791167bb9071e62a0f5d9c66c42bcb530 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 10:47:01 +0000 Subject: [PATCH 1/5] Initial plan From 50bcf63ae34e5492b19e03f4666bbd1e00bc0ca6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 10:51:45 +0000 Subject: [PATCH 2/5] Fix extension add --from review-thread issues --- src/specify_cli/__init__.py | 16 ++----- src/specify_cli/extensions.py | 13 +++++- tests/test_extensions.py | 81 +++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 13 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 41fb994726..b613b1cc1a 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3604,6 +3604,9 @@ def extension_add( if priority < 1: console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") raise typer.Exit(1) + if dev and from_url: + console.print("[red]Error:[/red] --dev and --from cannot be used together.") + raise typer.Exit(1) manager = ExtensionManager(project_root) speckit_version = get_speckit_version() @@ -3625,7 +3628,6 @@ def extension_add( elif from_url: # Install from URL (ZIP file) - import urllib.request import urllib.error from urllib.parse import urlparse @@ -3643,27 +3645,17 @@ def extension_add( console.print("Only install extensions from sources you trust.\n") console.print(f"Downloading from {from_url}...") - # Download ZIP to temp location - download_dir = project_root / ".specify" / "extensions" / ".cache" / "downloads" - download_dir.mkdir(parents=True, exist_ok=True) - zip_path = download_dir / f"{extension}-url-download.zip" - try: from specify_cli.authentication.http import open_url as _open_url with _open_url(from_url, timeout=60) as response: zip_data = response.read() - zip_path.write_bytes(zip_data) # Install from downloaded ZIP - manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority) + manifest = manager.install_from_zip_bytes(zip_data, speckit_version, priority=priority) except urllib.error.URLError as e: console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}") raise typer.Exit(1) - finally: - # Clean up downloaded ZIP - if zip_path.exists(): - zip_path.unlink() else: # Try bundled extensions first (shipped with spec-kit) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 871503f0ae..a91e9b2f9a 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -8,6 +8,7 @@ import json import hashlib +import io import os import tempfile import zipfile @@ -1224,6 +1225,16 @@ def install_from_zip( ValidationError: If manifest is invalid or priority is invalid CompatibilityError: If extension is incompatible """ + with zip_path.open("rb") as zip_file: + return self.install_from_zip_bytes(zip_file.read(), speckit_version, priority=priority) + + def install_from_zip_bytes( + self, + zip_bytes: bytes, + speckit_version: str, + priority: int = 10, + ) -> ExtensionManifest: + """Install extension from ZIP bytes.""" # Validate priority early if priority < 1: raise ValidationError("Priority must be a positive integer (1 or higher)") @@ -1232,7 +1243,7 @@ def install_from_zip( temp_path = Path(tmpdir) # Extract ZIP safely (prevent Zip Slip attack) - with zipfile.ZipFile(zip_path, 'r') as zf: + with zipfile.ZipFile(io.BytesIO(zip_bytes), 'r') as zf: # Validate all paths first before extracting anything temp_path_resolved = temp_path.resolve() for member in zf.namelist(): diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 153388a541..a15dcaa0da 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3458,6 +3458,87 @@ def test_extensionignore_negation_pattern(self, temp_dir, valid_manifest_data): class TestExtensionAddCLI: """CLI integration tests for extension add command.""" + def test_add_rejects_dev_and_from_together(self, tmp_path): + """extension add should reject mutually exclusive --dev and --from flags.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + extension_dir = tmp_path / "extension" + extension_dir.mkdir() + (extension_dir / "extension.yml").write_text( + "id: test-ext\nname: Test Extension\nversion: 1.0.0\ncommands: []\n", + encoding="utf-8", + ) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke( + app, + ["extension", "add", str(extension_dir), "--dev", "--from", "https://example.com/ext.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 1 + assert "--dev and --from cannot be used together" in result.output + + def test_add_from_url_installs_from_downloaded_bytes(self, tmp_path): + """extension add --from should install from in-memory bytes.""" + from types import SimpleNamespace + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + from specify_cli.extensions import ExtensionManager + + runner = CliRunner() + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + fake_manifest = SimpleNamespace( + id="test-ext", + name="Test Extension", + version="1.0.0", + description="desc", + warnings=[], + commands=[], + ) + zip_payload = b"fake-zip-bytes" + install_args = {} + + class _Resp: + def __enter__(self): + return self + + def __exit__(self, *_args): + return False + + def read(self): + return zip_payload + + def _install_from_zip_bytes(self_obj, payload, _speckit_version, priority=10): + install_args["payload"] = payload + install_args["priority"] = priority + return fake_manifest + + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("specify_cli.authentication.http.open_url", return_value=_Resp()), \ + patch.object(ExtensionManager, "install_from_zip_bytes", _install_from_zip_bytes), \ + patch.object(ExtensionManager, "install_from_zip", side_effect=AssertionError("legacy path install should not be used")): + result = runner.invoke( + app, + ["extension", "add", "../../evil", "--from", "https://example.com/ext.zip"], + catch_exceptions=True, + ) + + assert result.exit_code == 0, result.output + assert install_args["payload"] == zip_payload + assert install_args["priority"] == 10 + def test_add_by_display_name_uses_resolved_id_for_download(self, tmp_path): """extension add by display name should use resolved ID for download_extension().""" from typer.testing import CliRunner From 3ff1ecf8a204ef3faa4f9a9a20a5c2a395523977 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 20 May 2026 10:55:27 +0000 Subject: [PATCH 3/5] Polish extension add URL-install tests and docs --- src/specify_cli/__init__.py | 4 ++-- src/specify_cli/extensions.py | 22 +++++++++++++++++++--- tests/test_extensions.py | 10 +++++----- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index b613b1cc1a..f6804a908f 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3649,10 +3649,10 @@ def extension_add( from specify_cli.authentication.http import open_url as _open_url with _open_url(from_url, timeout=60) as response: - zip_data = response.read() + zip_bytes = response.read() # Install from downloaded ZIP - manifest = manager.install_from_zip_bytes(zip_data, speckit_version, priority=priority) + manifest = manager.install_from_zip_bytes(zip_bytes, speckit_version, priority=priority) except urllib.error.URLError as e: console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}") raise typer.Exit(1) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index a91e9b2f9a..6b1431a75b 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -1225,8 +1225,11 @@ def install_from_zip( ValidationError: If manifest is invalid or priority is invalid CompatibilityError: If extension is incompatible """ - with zip_path.open("rb") as zip_file: - return self.install_from_zip_bytes(zip_file.read(), speckit_version, priority=priority) + try: + with zip_path.open("rb") as zip_file: + return self.install_from_zip_bytes(zip_file.read(), speckit_version, priority=priority) + except OSError as e: + raise ValidationError(f"Failed to read ZIP file {zip_path}: {e}") from e def install_from_zip_bytes( self, @@ -1234,7 +1237,20 @@ def install_from_zip_bytes( speckit_version: str, priority: int = 10, ) -> ExtensionManifest: - """Install extension from ZIP bytes.""" + """Install extension from ZIP bytes. + + Args: + zip_bytes: ZIP archive content. + speckit_version: Current spec-kit version. + priority: Resolution priority (lower = higher precedence, default 10). + + Returns: + Installed extension manifest. + + Raises: + ValidationError: If manifest is invalid or priority is invalid. + CompatibilityError: If extension is incompatible. + """ # Validate priority early if priority < 1: raise ValidationError("Priority must be a positive integer (1 or higher)") diff --git a/tests/test_extensions.py b/tests/test_extensions.py index a15dcaa0da..34c8366d4c 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3510,28 +3510,28 @@ def test_add_from_url_installs_from_downloaded_bytes(self, tmp_path): zip_payload = b"fake-zip-bytes" install_args = {} - class _Resp: + class _MockHTTPResponse: def __enter__(self): return self - def __exit__(self, *_args): + def __exit__(self, exc_type, exc_val, exc_tb): return False def read(self): return zip_payload - def _install_from_zip_bytes(self_obj, payload, _speckit_version, priority=10): + def _install_from_zip_bytes(_self, payload, _speckit_version, priority=10): install_args["payload"] = payload install_args["priority"] = priority return fake_manifest with patch.object(Path, "cwd", return_value=project_dir), \ - patch("specify_cli.authentication.http.open_url", return_value=_Resp()), \ + patch("specify_cli.authentication.http.open_url", return_value=_MockHTTPResponse()), \ patch.object(ExtensionManager, "install_from_zip_bytes", _install_from_zip_bytes), \ patch.object(ExtensionManager, "install_from_zip", side_effect=AssertionError("legacy path install should not be used")): result = runner.invoke( app, - ["extension", "add", "../../evil", "--from", "https://example.com/ext.zip"], + ["extension", "add", "ignored-extension-name", "--from", "https://example.com/ext.zip"], catch_exceptions=True, ) From c8a1b8f1e02f138bb7f5c5deaf8dabb755aa3a9e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 21 May 2026 13:49:21 +0000 Subject: [PATCH 4/5] Address Copilot feedback: refactor install_from_zip and cap URL download size - Refactor install_from_zip/install_from_zip_bytes to share a private _install_from_zip_file() helper that accepts an open ZipFile object. install_from_zip() now uses ZipFile(path) directly (no full file read into memory); install_from_zip_bytes() wraps bytes in BytesIO as before. - Add 50 MB streaming read cap in --from URL download path to prevent memory exhaustion from unbounded responses. - Update existing --from URL test mock to support chunked read(n) calls. - Add new test covering oversized download rejection. --- src/specify_cli/__init__.py | 18 ++++++++++- src/specify_cli/extensions.py | 59 +++++++++++++++++++++++++---------- tests/test_extensions.py | 59 +++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 19 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index f6804a908f..1b3efa0575 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3648,11 +3648,27 @@ def extension_add( try: from specify_cli.authentication.http import open_url as _open_url + _MAX_ZIP_BYTES = 50 * 1024 * 1024 # 50 MB hard cap with _open_url(from_url, timeout=60) as response: - zip_bytes = response.read() + chunks: list[bytes] = [] + total = 0 + while True: + chunk = response.read(65536) + if not chunk: + break + total += len(chunk) + if total > _MAX_ZIP_BYTES: + raise ValueError( + f"Download exceeded maximum allowed size of {_MAX_ZIP_BYTES // (1024 * 1024)} MB" + ) + chunks.append(chunk) + zip_bytes = b"".join(chunks) # Install from downloaded ZIP manifest = manager.install_from_zip_bytes(zip_bytes, speckit_version, priority=priority) + except ValueError as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) except urllib.error.URLError as e: console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}") raise typer.Exit(1) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 6b1431a75b..c6ca5a03e7 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -1226,8 +1226,10 @@ def install_from_zip( CompatibilityError: If extension is incompatible """ try: - with zip_path.open("rb") as zip_file: - return self.install_from_zip_bytes(zip_file.read(), speckit_version, priority=priority) + with zipfile.ZipFile(zip_path, 'r') as zf: + return self._install_from_zip_file(zf, speckit_version, priority=priority) + except zipfile.BadZipFile as e: + raise ValidationError(f"Invalid ZIP file {zip_path}: {e}") from e except OSError as e: raise ValidationError(f"Failed to read ZIP file {zip_path}: {e}") from e @@ -1247,6 +1249,32 @@ def install_from_zip_bytes( Returns: Installed extension manifest. + Raises: + ValidationError: If manifest is invalid or priority is invalid. + CompatibilityError: If extension is incompatible. + """ + try: + with zipfile.ZipFile(io.BytesIO(zip_bytes), 'r') as zf: + return self._install_from_zip_file(zf, speckit_version, priority=priority) + except zipfile.BadZipFile as e: + raise ValidationError(f"Invalid ZIP data: {e}") from e + + def _install_from_zip_file( + self, + zf: "zipfile.ZipFile", + speckit_version: str, + priority: int = 10, + ) -> ExtensionManifest: + """Shared implementation: install extension from an already-open ZipFile. + + Args: + zf: Open ZipFile object (path-backed or in-memory). + speckit_version: Current spec-kit version. + priority: Resolution priority (lower = higher precedence, default 10). + + Returns: + Installed extension manifest. + Raises: ValidationError: If manifest is invalid or priority is invalid. CompatibilityError: If extension is incompatible. @@ -1259,20 +1287,19 @@ def install_from_zip_bytes( temp_path = Path(tmpdir) # Extract ZIP safely (prevent Zip Slip attack) - with zipfile.ZipFile(io.BytesIO(zip_bytes), '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) + # 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) # Find extension directory (may be nested) extension_dir = temp_path diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 34c8366d4c..ed4c787369 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3511,14 +3511,24 @@ def test_add_from_url_installs_from_downloaded_bytes(self, tmp_path): install_args = {} class _MockHTTPResponse: + def __init__(self): + self._data = zip_payload + self._pos = 0 + def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): return False - def read(self): - return zip_payload + def read(self, n=-1): + if n == -1: + chunk = self._data[self._pos:] + self._pos = len(self._data) + else: + chunk = self._data[self._pos:self._pos + n] + self._pos += len(chunk) + return chunk def _install_from_zip_bytes(_self, payload, _speckit_version, priority=10): install_args["payload"] = payload @@ -3539,6 +3549,51 @@ def _install_from_zip_bytes(_self, payload, _speckit_version, priority=10): assert install_args["payload"] == zip_payload assert install_args["priority"] == 10 + def test_add_from_url_rejects_oversized_download(self, tmp_path): + """extension add --from should reject downloads exceeding the 50 MB size cap.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + _50MB_PLUS_1 = 50 * 1024 * 1024 + 1 + large_payload = b"X" * _50MB_PLUS_1 + + class _LargeHTTPResponse: + def __init__(self): + self._data = large_payload + self._pos = 0 + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, exc_tb): + return False + + def read(self, n=-1): + if n == -1: + chunk = self._data[self._pos:] + self._pos = len(self._data) + else: + chunk = self._data[self._pos:self._pos + n] + self._pos += len(chunk) + return chunk + + with patch.object(Path, "cwd", return_value=project_dir), \ + patch("specify_cli.authentication.http.open_url", return_value=_LargeHTTPResponse()): + result = runner.invoke( + app, + ["extension", "add", "ignored-extension-name", "--from", "https://example.com/large.zip"], + catch_exceptions=True, + ) + + assert result.exit_code != 0 + assert "50" in result.output # error message mentions size limit + def test_add_by_display_name_uses_resolved_id_for_download(self, tmp_path): """extension add by display name should use resolved ID for download_extension().""" from typer.testing import CliRunner From 934351b5f62870acd45419594901f582cd98b62a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 21 May 2026 13:51:23 +0000 Subject: [PATCH 5/5] Extract _MAX_ZIP_BYTES/_DOWNLOAD_CHUNK_SIZE as module constants; strengthen test assertion - Move size cap and chunk size to module-level named constants - Update oversized-download test to import _MAX_ZIP_BYTES from production code (no duplicated magic number) and assert on specific error substring --- src/specify_cli/__init__.py | 5 +++-- tests/test_extensions.py | 7 +++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 1b3efa0575..dbaf7a4387 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -117,6 +117,8 @@ def _build_agent_config() -> dict[str, dict[str, Any]]: # Agents that use TOML command format (others use Markdown) _TOML_AGENTS = frozenset({"gemini", "tabnine"}) +_MAX_ZIP_BYTES = 50 * 1024 * 1024 # 50 MB hard cap for --from URL downloads +_DOWNLOAD_CHUNK_SIZE = 64 * 1024 # 64 KB read chunks def _build_ai_assistant_help() -> str: """Build the --ai help text from AGENT_CONFIG so it stays in sync with runtime config.""" @@ -3648,12 +3650,11 @@ def extension_add( try: from specify_cli.authentication.http import open_url as _open_url - _MAX_ZIP_BYTES = 50 * 1024 * 1024 # 50 MB hard cap with _open_url(from_url, timeout=60) as response: chunks: list[bytes] = [] total = 0 while True: - chunk = response.read(65536) + chunk = response.read(_DOWNLOAD_CHUNK_SIZE) if not chunk: break total += len(chunk) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index ed4c787369..c6ca76891f 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3553,15 +3553,14 @@ def test_add_from_url_rejects_oversized_download(self, tmp_path): """extension add --from should reject downloads exceeding the 50 MB size cap.""" from typer.testing import CliRunner from unittest.mock import patch - from specify_cli import app + from specify_cli import app, _MAX_ZIP_BYTES runner = CliRunner() project_dir = tmp_path / "test-project" project_dir.mkdir() (project_dir / ".specify").mkdir() - _50MB_PLUS_1 = 50 * 1024 * 1024 + 1 - large_payload = b"X" * _50MB_PLUS_1 + large_payload = b"X" * (_MAX_ZIP_BYTES + 1) class _LargeHTTPResponse: def __init__(self): @@ -3592,7 +3591,7 @@ def read(self, n=-1): ) assert result.exit_code != 0 - assert "50" in result.output # error message mentions size limit + assert "exceeded maximum allowed size" in result.output def test_add_by_display_name_uses_resolved_id_for_download(self, tmp_path): """extension add by display name should use resolved ID for download_extension()."""