Fix 3 current Copilot review findings: bookkeeping-aware BundlerError removal, bounded workflow downloads

1. bundle remove: BundlerError raised by the primitive installer itself
   (e.g. from a kind manager) bypassed the partial-removal bookkeeping
   message added previously via a bare `except BundlerError: raise`. Now
   routes through the same detail-construction logic as generic
   exceptions, so a mid-loop BundlerError after an earlier successful
   removal still reports that the project may be partially uninstalled,
   while a zero-removal BundlerError still reports "No components were
   removed." Both preserve the original exception message and chain
   `from exc`.

2/3. workflow add --from and catalog install/update downloads used
   unbounded `response.read()`, buffering the entire server-controlled
   body into memory before any size check, and trusted Content-Length
   alone where checked at all. Added a single shared
   `_read_response_within_limit()` helper reused by both call sites: it
   fails fast on an oversized declared Content-Length, and separately
   enforces the same cap while streaming in 64KiB chunks so a chunked or
   Content-Length-less response cannot bypass the limit by lying about or
   omitting its size. Chose 5 MiB as the cap: workflow YAML definitions
   are small step/metadata text, not binaries, so this is generous
   headroom against a malicious/misbehaving server without affecting any
   legitimate workflow definition. Both call sites already route any
   raised exception through their existing clean-error and rollback
   (`_cleanup_failed_install`) paths, so no additional error-handling
   plumbing was needed.

Tests: extended the shared `_FakeResponse` test helper (and 5 duplicate
per-test FakeResponse classes) to support `.read(amt)` chunked reads with
an internal cursor (backward compatible with existing bare `.read()`
callers) plus header simulation. Added red-first tests for: BundlerError
after partial removal reporting partial state, BundlerError with zero
removals reporting no changes, --from oversized-Content-Length rejection,
--from oversized-streamed-body-without-Content-Length rejection, and the
same two cases for the catalog install path (asserting no orphan
directory/registry mutation on rejection).

tests/integration/test_bundler_install_flow.py: 17 passed
tests/test_workflows.py: 485 passed
tests -q: 3992 passed, 110 skipped
ruff check: clean on all touched files

Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

src/specify_cli/bundler/services/installer.py

@@ -199,8 +199,6 @@ def remove_bundle(
                 result.uninstalled.append(component)
             else:
                 result.skipped.append(component)
-    except BundlerError:
-        raise
     except Exception as exc:  # noqa: BLE001
         if result.uninstalled:
             detail = (

src/specify_cli/workflows/_commands.py

@@ -147,6 +147,52 @@ def _reject_insecure_download_redirect(old_url: str, new_url: str) -> None:
         )
 
 
+# Workflow YAML definitions are small step/metadata text, not binaries, so
+# this is generous headroom against a malicious or misbehaving server -- not
+# a ceiling any legitimate workflow definition should ever approach.
+_MAX_WORKFLOW_YAML_BYTES = 5 * 1024 * 1024  # 5 MiB
+_DOWNLOAD_CHUNK_SIZE = 65536
+
+
+def _read_response_within_limit(response, max_bytes: int = _MAX_WORKFLOW_YAML_BYTES) -> bytes:
+    """Read *response* fully, enforcing *max_bytes* via bounded streaming.
+
+    A ``Content-Length`` header is checked up front to fail fast, but it is
+    never trusted alone: the actual bytes read are also counted as they
+    stream in, so a chunked or ``Content-Length``-less response that lies
+    about (or omits) its size still cannot exceed the limit.
+    """
+    content_length = None
+    getheader = getattr(response, "getheader", None)
+    if callable(getheader):
+        try:
+            raw_length = getheader("Content-Length")
+        except Exception:
+            raw_length = None
+        if raw_length is not None:
+            try:
+                content_length = int(raw_length)
+            except (TypeError, ValueError):
+                content_length = None
+    if content_length is not None and content_length > max_bytes:
+        raise ValueError(
+            f"response declared {content_length} bytes, exceeding the "
+            f"{max_bytes}-byte workflow size limit"
+        )
+
+    chunks: list[bytes] = []
+    total = 0
+    while True:
+        chunk = response.read(_DOWNLOAD_CHUNK_SIZE)
+        if not chunk:
+            break
+        total += len(chunk)
+        if total > max_bytes:
+            raise ValueError(f"response exceeds the {max_bytes}-byte workflow size limit")
+        chunks.append(chunk)
+    return b"".join(chunks)
+
+
 def _validate_workflow_id_or_exit(workflow_id: str) -> None:
     """Validate that ``workflow_id`` is a safe installed-workflow directory name."""
     if (
@@ -887,7 +933,7 @@ def workflow_add(
                     )
                     raise typer.Exit(1)
                 with tempfile.NamedTemporaryFile(suffix=".yml", delete=False) as tmp:
-                    tmp.write(resp.read())
+                    tmp.write(_read_response_within_limit(resp))
                     tmp_path = Path(tmp.name)
         except typer.Exit:
             raise
@@ -1076,7 +1122,7 @@ def _install_workflow_from_catalog(
                     f"[red]Error:[/red] Workflow '{safe_wf_id}' redirected to non-HTTPS URL: {_escape_markup(final_url)}"
                 )
                 raise typer.Exit(1)
-            workflow_file.write_bytes(response.read())
+            workflow_file.write_bytes(_read_response_within_limit(response))
     except typer.Exit:
         raise
     except Exception as exc:

tests/integration/test_bundler_install_flow.py

@@ -153,6 +153,66 @@ def test_remove_partial_failure_message_reflects_partial_state(tmp_path: Path):
     assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"}
 
 
+def test_remove_bundlerror_from_installer_after_partial_removal_reports_partial_state(
+    tmp_path: Path,
+):
+    """If the primitive installer itself raises BundlerError (not a raw/
+    unexpected exception) after an earlier component in the same bundle was
+    already removed, the surfaced message must still carry the same
+    partial-removal detail as the generic-exception path -- a bare
+    ``except BundlerError: raise`` would re-raise the installer's original
+    message verbatim with no mention that the project may now be partially
+    uninstalled."""
+    make_project(tmp_path)
+    manifest = BundleManifest.from_dict(valid_manifest_dict())
+    installer = FakeInstaller()
+    install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest)
+
+    real_remove = installer.remove
+    calls = {"n": 0}
+
+    def remove_then_raise_bundler_error(project_root, component):
+        calls["n"] += 1
+        if calls["n"] == 1:
+            return real_remove(project_root, component)
+        raise BundlerError("kind manager refused removal")
+
+    with pytest.MonkeyPatch.context() as mp:
+        mp.setattr(installer, "remove", remove_then_raise_bundler_error)
+        with pytest.raises(BundlerError) as exc_info:
+            remove_bundle(tmp_path, "demo-bundle", installer)
+
+    message = str(exc_info.value)
+    assert "no changes were recorded" not in message.lower()
+    assert "kind manager refused removal" in message
+    assert "partially uninstalled" in message.lower()
+    assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"}
+
+
+def test_remove_bundlerror_from_installer_with_zero_removed_reports_no_changes(
+    tmp_path: Path,
+):
+    """When the installer raises BundlerError before anything was actually
+    removed, the message should not misleadingly claim partial state."""
+    make_project(tmp_path)
+    manifest = BundleManifest.from_dict(valid_manifest_dict())
+    installer = FakeInstaller()
+    install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest)
+
+    def boom(project_root, component):
+        raise BundlerError("kind manager unavailable")
+
+    with pytest.MonkeyPatch.context() as mp:
+        mp.setattr(installer, "is_installed", boom)
+        with pytest.raises(BundlerError) as exc_info:
+            remove_bundle(tmp_path, "demo-bundle", installer)
+
+    message = str(exc_info.value)
+    assert "no components were removed" in message.lower()
+    assert "kind manager unavailable" in message
+    assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"}
+
+
 def test_remove_reports_uninstalled_not_installed(tmp_path: Path):
     make_project(tmp_path)
     manifest = BundleManifest.from_dict(valid_manifest_dict())

tests/test_workflows.py

@@ -6797,8 +6797,16 @@ steps:
                 self._data = data
                 self._url = url or "https://api.github.com/repos/org/repo/releases/assets/42"
 
-            def read(self):
-                return self._data
+            def read(self, amt=None):
+                if not hasattr(self, "_pos"):
+                    self._pos = 0
+                if amt is None:
+                    chunk = self._data[self._pos :]
+                    self._pos = len(self._data)
+                    return chunk
+                chunk = self._data[self._pos : self._pos + amt]
+                self._pos += len(chunk)
+                return chunk
 
             def geturl(self):
                 return self._url
@@ -6849,8 +6857,16 @@ steps:
                 self._data = data
                 self._url = url or "https://api.github.com/repos/org/repo/releases/assets/42"
 
-            def read(self):
-                return self._data
+            def read(self, amt=None):
+                if not hasattr(self, "_pos"):
+                    self._pos = 0
+                if amt is None:
+                    chunk = self._data[self._pos :]
+                    self._pos = len(self._data)
+                    return chunk
+                chunk = self._data[self._pos : self._pos + amt]
+                self._pos += len(chunk)
+                return chunk
 
             def geturl(self):
                 return self._url
@@ -6892,8 +6908,16 @@ steps:
                 self._data = data
                 self._url = url or "https://api.github.com/repos/org/repo/releases/assets/55"
 
-            def read(self):
-                return self._data
+            def read(self, amt=None):
+                if not hasattr(self, "_pos"):
+                    self._pos = 0
+                if amt is None:
+                    chunk = self._data[self._pos :]
+                    self._pos = len(self._data)
+                    return chunk
+                chunk = self._data[self._pos : self._pos + amt]
+                self._pos += len(chunk)
+                return chunk
 
             def geturl(self):
                 return self._url
@@ -6968,8 +6992,16 @@ steps:
                 self._data = data
                 self._url = url or "https://ghes.example/api/v3/repos/org/repo/releases/assets/42"
 
-            def read(self):
-                return self._data
+            def read(self, amt=None):
+                if not hasattr(self, "_pos"):
+                    self._pos = 0
+                if amt is None:
+                    chunk = self._data[self._pos :]
+                    self._pos = len(self._data)
+                    return chunk
+                chunk = self._data[self._pos : self._pos + amt]
+                self._pos += len(chunk)
+                return chunk
 
             def geturl(self):
                 return self._url
@@ -7023,8 +7055,16 @@ steps:
                 self._data = data
                 self._url = url or "https://ghes.example/api/v3/repos/org/repo/releases/assets/55"
 
-            def read(self):
-                return self._data
+            def read(self, amt=None):
+                if not hasattr(self, "_pos"):
+                    self._pos = 0
+                if amt is None:
+                    chunk = self._data[self._pos :]
+                    self._pos = len(self._data)
+                    return chunk
+                chunk = self._data[self._pos : self._pos + amt]
+                self._pos += len(chunk)
+                return chunk
 
             def geturl(self):
                 return self._url
@@ -7582,12 +7622,23 @@ steps:
     # -- add --from ----------------------------------------------------
 
     class _FakeResponse:
-        def __init__(self, data, url="https://example.com/workflow.yml"):
+        def __init__(self, data, url="https://example.com/workflow.yml", headers=None):
             self._data = data
             self._url = url
+            self._pos = 0
+            self._headers = headers or {}
 
-        def read(self):
-            return self._data
+        def read(self, amt=None):
+            if amt is None:
+                chunk = self._data[self._pos :]
+                self._pos = len(self._data)
+                return chunk
+            chunk = self._data[self._pos : self._pos + amt]
+            self._pos += len(chunk)
+            return chunk
+
+        def getheader(self, name, default=None):
+            return self._headers.get(name, default)
 
         def geturl(self):
             return self._url
@@ -7598,6 +7649,64 @@ steps:
         def __exit__(self, *a):
             return False
 
+    def test_add_from_url_rejects_oversized_content_length(self, project_dir, monkeypatch):
+        """A --from download must not trust an advertised Content-Length
+        alone by reading the whole body first -- it must reject a response
+        that declares a size over the workflow YAML limit before reading
+        the (potentially huge) body into memory at all."""
+        from unittest.mock import patch
+        from typer.testing import CliRunner
+        from specify_cli import app
+        from specify_cli.workflows import _commands as wf_commands
+
+        monkeypatch.chdir(project_dir)
+        monkeypatch.setattr(wf_commands, "_MAX_WORKFLOW_YAML_BYTES", 100)
+        small_body = b"id: align-wf\n"  # small actual body; Content-Length lies
+        runner = CliRunner()
+        with patch(
+            "specify_cli.authentication.http.open_url",
+            side_effect=lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse(
+                small_body, url, headers={"Content-Length": "1000"}
+            ),
+        ):
+            result = runner.invoke(
+                app,
+                ["workflow", "add", "align-wf", "--from", "https://example.com/workflow.yml"],
+            )
+        assert result.exit_code != 0
+        assert result.exception is None or isinstance(result.exception, SystemExit)
+        assert result.output.strip() != ""
+
+    def test_add_from_url_rejects_oversized_streamed_body_without_content_length(
+        self, project_dir, monkeypatch
+    ):
+        """A chunked/no-Content-Length response must still be capped by
+        actually counting streamed bytes -- a malicious or misbehaving
+        server cannot bypass the limit merely by omitting or lying about
+        Content-Length."""
+        from unittest.mock import patch
+        from typer.testing import CliRunner
+        from specify_cli import app
+        from specify_cli.workflows import _commands as wf_commands
+
+        monkeypatch.chdir(project_dir)
+        monkeypatch.setattr(wf_commands, "_MAX_WORKFLOW_YAML_BYTES", 100)
+        oversized_body = b"x" * 500  # no Content-Length header at all
+        runner = CliRunner()
+        with patch(
+            "specify_cli.authentication.http.open_url",
+            side_effect=lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse(
+                oversized_body, url
+            ),
+        ):
+            result = runner.invoke(
+                app,
+                ["workflow", "add", "align-wf", "--from", "https://example.com/workflow.yml"],
+            )
+        assert result.exit_code != 0
+        assert result.exception is None or isinstance(result.exception, SystemExit)
+        assert result.output.strip() != ""
+
     def test_add_from_url_installs(self, project_dir, monkeypatch):
         from unittest.mock import patch
         from typer.testing import CliRunner
@@ -8061,6 +8170,91 @@ steps:
         assert not dest_dir.exists()
         assert not WorkflowRegistry(project_dir).is_installed("align-wf")
 
+    def test_add_catalog_rejects_oversized_content_length(self, project_dir, monkeypatch):
+        """Catalog installs must share the same size cap as --from: a
+        response that declares an oversized Content-Length is rejected
+        before its body is read into memory, and no orphan directory or
+        registry mutation is left behind."""
+        from typer.testing import CliRunner
+        from specify_cli import app
+        from specify_cli.workflows import _commands as wf_commands
+        from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowRegistry
+
+        monkeypatch.chdir(project_dir)
+        monkeypatch.setattr(wf_commands, "_MAX_WORKFLOW_YAML_BYTES", 100)
+        monkeypatch.setattr(
+            WorkflowCatalog,
+            "get_workflow_info",
+            lambda self, wid: {
+                "id": wid,
+                "name": "Align Workflow",
+                "version": "1.0.0",
+                "url": "https://example.com/workflow.yml",
+                "_install_allowed": True,
+                "_catalog_name": "test-catalog",
+            },
+        )
+        small_body = b"id: align-wf\n"  # actual body is small; header lies
+        runner = CliRunner()
+        with pytest.MonkeyPatch.context() as mp:
+            mp.setattr(
+                "specify_cli.authentication.http.open_url",
+                lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse(
+                    small_body, url, headers={"Content-Length": "1000"}
+                ),
+            )
+            result = runner.invoke(app, ["workflow", "add", "align-wf"])
+
+        assert result.exit_code != 0
+        assert result.exception is None or isinstance(result.exception, SystemExit)
+        assert result.output.strip() != ""
+        dest_dir = project_dir / ".specify" / "workflows" / "align-wf"
+        assert not dest_dir.exists()
+        assert not WorkflowRegistry(project_dir).is_installed("align-wf")
+
+    def test_add_catalog_rejects_oversized_streamed_body_without_content_length(
+        self, project_dir, monkeypatch
+    ):
+        """Catalog installs must also cap actual streamed bytes when
+        Content-Length is absent or understated, leaving no orphan
+        directory or registry mutation behind."""
+        from typer.testing import CliRunner
+        from specify_cli import app
+        from specify_cli.workflows import _commands as wf_commands
+        from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowRegistry
+
+        monkeypatch.chdir(project_dir)
+        monkeypatch.setattr(wf_commands, "_MAX_WORKFLOW_YAML_BYTES", 100)
+        monkeypatch.setattr(
+            WorkflowCatalog,
+            "get_workflow_info",
+            lambda self, wid: {
+                "id": wid,
+                "name": "Align Workflow",
+                "version": "1.0.0",
+                "url": "https://example.com/workflow.yml",
+                "_install_allowed": True,
+                "_catalog_name": "test-catalog",
+            },
+        )
+        oversized_body = b"x" * 500  # no Content-Length header at all
+        runner = CliRunner()
+        with pytest.MonkeyPatch.context() as mp:
+            mp.setattr(
+                "specify_cli.authentication.http.open_url",
+                lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse(
+                    oversized_body, url
+                ),
+            )
+            result = runner.invoke(app, ["workflow", "add", "align-wf"])
+
+        assert result.exit_code != 0
+        assert result.exception is None or isinstance(result.exception, SystemExit)
+        assert result.output.strip() != ""
+        dest_dir = project_dir / ".specify" / "workflows" / "align-wf"
+        assert not dest_dir.exists()
+        assert not WorkflowRegistry(project_dir).is_installed("align-wf")
+
     def test_add_catalog_reinstall_save_failure_restores_prior_file(self, project_dir, monkeypatch):
         """Re-adding an already-installed catalog workflow downloads the new
         version over the existing install directory. If registry.add() then