From db45f6c2d1a2096015de5c505851db5a6312f869 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 11:05:15 +0200 Subject: [PATCH] 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 | 2 - src/specify_cli/workflows/_commands.py | 50 +++- .../integration/test_bundler_install_flow.py | 60 +++++ tests/test_workflows.py | 220 ++++++++++++++++-- 4 files changed, 315 insertions(+), 17 deletions(-) diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 9fc7d2a96..61c882d1f 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/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 = ( diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 98fd7284f..c3ba89157 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/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: diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index 8b52d90fe..a2e34797f 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/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()) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ba1c355a0..4242146ca 100644 --- a/tests/test_workflows.py +++ b/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