From 5a27daa3324d7e3133ed3bafbb26cfb0d99d93ea Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 16 Jul 2026 22:59:51 -0700 Subject: [PATCH] refactor(download): extract per-URL loop into _download_one_url / _copy_local_one / _stream_http_one (BE-3273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit execute_download's per-URL loop body had grown to ~220 lines spanning two download branches (local copy + HTTP stream), four cap/truncation checks, part-file lifecycle, and entry construction — all inline at four levels of nesting. Lift it into three helpers placed with the other _-helpers: - _download_one_url: naming, collision-safe path, symlink-dest refusal, branch dispatch, entry construction. - _copy_local_one: the local-output copy branch. - _stream_http_one: the HTTP stream branch. Strictly behavior-preserving: every guard, guard ORDER, renderer.error code /message/details key, and typer.Exit(code=1) is carried over verbatim. The _local_source_path call stays pinned behind the is_local_job SSRF gate (not hoisted), and _assert_download_url still precedes the Request build. execute_download's loop is now 13 lines of orchestration. All 51 pre-existing tests in test_transfer_download.py pass unmodified; 11 new direct unit tests pin each extracted guard. --- comfy_cli/command/transfer.py | 491 ++++++++++-------- .../command/test_transfer_download.py | 265 ++++++++++ 2 files changed, 537 insertions(+), 219 deletions(-) diff --git a/comfy_cli/command/transfer.py b/comfy_cli/command/transfer.py index 08e971fa..fd308ba3 100644 --- a/comfy_cli/command/transfer.py +++ b/comfy_cli/command/transfer.py @@ -465,6 +465,266 @@ def execute_upload( # --------------------------------------------------------------------------- +def _copy_local_one(url: str, idx: int, local_source: Path, local_path: Path, renderer) -> None: + """Copy a LOCAL run's on-disk output into ``local_path`` (no HTTP fetch). + + Extracted verbatim from ``execute_download``'s per-URL loop: the + ``local_source is not None`` branch. Every guard, envelope, and exit is + byte-identical to the inline version. + """ + # On-disk output (local run): copy it in. No HTTP fetch, so the + # SSRF guard doesn't apply — a bare path/file:// URL has no host to + # forge a request to. `copyfile` follows a symlinked SOURCE but + # writes a plain file at `local_path` (the dest-symlink guard above + # already refused a symlinked destination). + if not local_source.is_file(): + renderer.error( + code="download_failed", + message=f"Local output not found on disk: {local_source}", + hint="ensure the job completed and its output files still exist", + details={"url": url, "path": str(local_source), "index": idx}, + ) + raise typer.Exit(code=1) + # Mirror the HTTP branch's safety cap so a pathological source + # (e.g. an unbounded pseudo-file that still reports as regular) can't + # exhaust the disk. stat() follows the symlinked source, matching + # what copyfile actually reads. + try: + source_size = local_source.stat().st_size + except OSError as e: + renderer.error( + code="download_failed", + message=f"Failed to stat local output {idx}: {e}", + hint="ensure the output file is readable", + details={"url": url, "path": str(local_source), "index": idx}, + ) + raise typer.Exit(code=1) + if source_size > _MAX_DOWNLOAD_BYTES: + renderer.error( + code="download_failed", + message=f"Local output {idx} exceeds {_MAX_DOWNLOAD_BYTES} byte safety limit", + hint="the source file is too large to copy", + details={"url": url, "path": str(local_source), "size": source_size, "index": idx}, + ) + raise typer.Exit(code=1) + # Wrap the copy like the HTTP branch's failure handling: an OSError + # (permission denied, full/read-only dest) or the mid-copy size cap + # must surface as a structured envelope, not an unhandled traceback + # that breaks machine-mode/NDJSON consumers. + try: + _copy_local_output_capped(local_source, local_path) + except (OSError, ValueError) as e: + renderer.error( + code="download_failed", + message=f"Failed to copy local output {idx}: {e}", + hint="check filesystem permissions and free space in the out-dir", + details={"url": url, "path": str(local_source), "index": idx}, + ) + raise typer.Exit(code=1) + + +def _stream_http_one(url: str, idx: int, local_path: Path, auth_hdrs: dict[str, str], renderer) -> None: + """Stream one HTTP(S) output into ``local_path`` via a verified part-file. + + Extracted verbatim from ``execute_download``'s per-URL loop: the + ``else`` (non-local) branch. The SSRF assert precedes the ``Request`` + build, and the four cap/truncation checks keep their original order and + the part-file ``finally`` cleanup its original semantics. + """ + try: + _assert_download_url(url) + except ValueError as e: + renderer.error( + code="download_failed", + message=str(e), + hint="output URLs should be http or https", + details={"url": url, "index": idx}, + ) + raise typer.Exit(code=1) + + req = urllib.request.Request(url) + for hdr, val in auth_hdrs.items(): + req.add_header(hdr, val) + + # Stream into an exclusively-created temp file and rename it into + # place only once the body is complete and verified, so + # `local_path` never holds a partial file no matter how the + # transfer dies. + part_path: Path | None = None + try: + with _DOWNLOAD_OPENER.open(req, timeout=_DOWNLOAD_TIMEOUT_S) as resp: + expected = _declared_content_length(resp) + if expected is not None and expected > _MAX_DOWNLOAD_BYTES: + renderer.error( + code="download_failed", + message=f"Output {idx} declares {expected} bytes, over the {_MAX_DOWNLOAD_BYTES} byte safety limit", + hint="the output is too large to download", + details={"url": url, "index": idx, "declared_bytes": expected}, + ) + raise typer.Exit(code=1) + part_file, part_path = _open_part_file(local_path) + total = 0 + with part_file as fp: + while True: + chunk = resp.read(_DOWNLOAD_CHUNK) + if not chunk: + break + total += len(chunk) + if expected is not None and total > expected: + # http.client clips plain Content-Length bodies, + # but ignores Content-Length when the response + # is chunked — that pairing could stream far + # past the declared size before the post-loop + # check fires. + renderer.error( + code="download_failed", + message=( + f"Download of output {idx} exceeds its declared Content-Length of {expected} bytes" + ), + hint="the server sent more data than it declared", + details={ + "url": url, + "index": idx, + "declared_bytes": expected, + "received_bytes": total, + }, + ) + raise typer.Exit(code=1) + if total > _MAX_DOWNLOAD_BYTES: + renderer.error( + code="download_failed", + message=f"Download of output {idx} exceeds {_MAX_DOWNLOAD_BYTES} byte safety limit", + hint="the output is too large to download", + details={"url": url, "index": idx, "received_bytes": total}, + ) + raise typer.Exit(code=1) + fp.write(chunk) + # http.client returns EOF instead of raising IncompleteRead when + # a Content-Length body is cut short and read in chunks, so a + # dropped connection otherwise looks like a completed download — + # verify the byte count explicitly. + if expected is not None and total != expected: + renderer.error( + code="download_failed", + message=f"Download of output {idx} truncated: received {total} of {expected} bytes", + hint="the connection dropped mid-transfer; retry the download", + details={"url": url, "index": idx, "declared_bytes": expected, "received_bytes": total}, + ) + raise typer.Exit(code=1) + part_path.replace(local_path) + part_path = None + except urllib.error.HTTPError as e: + renderer.error( + code="download_failed", + message=f"Failed to download output {idx}: HTTP {e.code}", + hint="check that the job completed successfully and the server is reachable", + details={"status": e.code, "url": url, "index": idx}, + ) + raise typer.Exit(code=1) + except (OSError, http.client.HTTPException) as e: + # Everything that isn't an HTTP status lands here: URLError + # (refused/DNS/TLS — a subclass of OSError), a socket timeout + # or reset mid-read, filesystem errors from the temp-file + # create/write/rename, and a truncated *chunked* body — which + # raises http.client.IncompleteRead (an HTTPException, not an + # OSError) rather than the silent EOF a Content-Length body + # gives. Emit the envelope instead of a traceback so + # machine-mode consumers keep their contract. + reason = getattr(e, "reason", None) or e + # A bare TimeoutError()/IncompleteRead can stringify to "", + # which would emit a reason-less envelope — fall back to the + # exception's type name so the cause is always diagnosable. + reason_text = str(reason) or type(e).__name__ + renderer.error( + code="download_failed", + message=f"Failed to download output {idx}: {reason_text}", + hint="check that the server is reachable and the out-dir is writable", + details={"url": url, "index": idx, "reason": reason_text}, + ) + raise typer.Exit(code=1) + finally: + # Cleared after the success rename; on every failure path this + # removes the partial download. + if part_path is not None: + try: + part_path.unlink(missing_ok=True) + except OSError: + pass + + +def _download_one_url( + url: str, + idx: int, + *, + dest: Path, + annotations: list[tuple[str | None, str | None]], + item_counters: dict[str, int], + short_id: str, + is_local_job: bool, + auth_hdrs: dict[str, str], + renderer, +) -> dict[str, Any]: + """Fetch or copy one output URL and return its saved-file entry dict. + + Extracted verbatim from ``execute_download``'s per-URL loop body: derive + the item-aware name, refuse a symlinked destination, dispatch to the local + copy or HTTP stream branch, then build the entry. ``item_counters`` is + mutated in place across calls exactly as the inline loop did. + """ + # A LOCAL run emits a bare on-disk path / file:// URL for an output + # that already exists — copy it instead of HTTP-fetching it. + local_source = _local_source_path(url) if is_local_job else None + # Derive the extension from the source. A bare path has no + # `?filename=` query param, so read the real suffix off the on-disk + # file rather than mislabeling everything `.png`; real URLs carry the + # name in the query param. + if local_source is not None: + ext = local_source.suffix or ".png" + else: + parsed = urllib.parse.urlparse(url) + qs = urllib.parse.parse_qs(parsed.query) + remote_name = qs.get("filename", ["output.png"])[0] + ext = Path(remote_name).suffix or ".png" + node_id, item = annotations[idx] + if item is not None: + safe_item = _sanitize_item_name(item) + n = item_counters.get(safe_item, 0) + item_counters[safe_item] = n + 1 + local_name = f"{safe_item}_{n:03d}{ext}" + else: + local_name = f"{short_id}_{idx:03d}{ext}" + # Suffix deterministically instead of overwriting a prior attempt. + local_path = _collision_safe_path(dest / local_name) + + # Refuse to overwrite symlinks (could be pointed at arbitrary files). + if local_path.is_symlink(): + renderer.error( + code="download_failed", + message=f"Refusing to write to symlink: {local_path}", + hint="remove the symlink and retry", + details={"path": str(local_path), "index": idx}, + ) + raise typer.Exit(code=1) + + if local_source is not None: + _copy_local_one(url, idx, local_source, local_path, renderer) + else: + _stream_http_one(url, idx, local_path, auth_hdrs, renderer) + + file_size = local_path.stat().st_size + entry: dict[str, Any] = { + "url": url, + "path": str(local_path.resolve()), + "size": file_size, + } + # Optional provenance keys — present only when known (no nulls). + if node_id is not None: + entry["node_id"] = node_id + if item is not None: + entry["item"] = item + return entry + + def execute_download( prompt_id: str | None = None, *, @@ -615,226 +875,19 @@ def execute_download( is_local_job = state is not None and getattr(state, "where", None) == "local" for idx, url in enumerate(output_urls): - # A LOCAL run emits a bare on-disk path / file:// URL for an output - # that already exists — copy it instead of HTTP-fetching it. - local_source = _local_source_path(url) if is_local_job else None - # Derive the extension from the source. A bare path has no - # `?filename=` query param, so read the real suffix off the on-disk - # file rather than mislabeling everything `.png`; real URLs carry the - # name in the query param. - if local_source is not None: - ext = local_source.suffix or ".png" - else: - parsed = urllib.parse.urlparse(url) - qs = urllib.parse.parse_qs(parsed.query) - remote_name = qs.get("filename", ["output.png"])[0] - ext = Path(remote_name).suffix or ".png" - node_id, item = annotations[idx] - if item is not None: - safe_item = _sanitize_item_name(item) - n = item_counters.get(safe_item, 0) - item_counters[safe_item] = n + 1 - local_name = f"{safe_item}_{n:03d}{ext}" - else: - local_name = f"{short_id}_{idx:03d}{ext}" - # Suffix deterministically instead of overwriting a prior attempt. - local_path = _collision_safe_path(dest / local_name) - - # Refuse to overwrite symlinks (could be pointed at arbitrary files). - if local_path.is_symlink(): - renderer.error( - code="download_failed", - message=f"Refusing to write to symlink: {local_path}", - hint="remove the symlink and retry", - details={"path": str(local_path), "index": idx}, - ) - raise typer.Exit(code=1) - - if local_source is not None: - # On-disk output (local run): copy it in. No HTTP fetch, so the - # SSRF guard doesn't apply — a bare path/file:// URL has no host to - # forge a request to. `copyfile` follows a symlinked SOURCE but - # writes a plain file at `local_path` (the dest-symlink guard above - # already refused a symlinked destination). - if not local_source.is_file(): - renderer.error( - code="download_failed", - message=f"Local output not found on disk: {local_source}", - hint="ensure the job completed and its output files still exist", - details={"url": url, "path": str(local_source), "index": idx}, - ) - raise typer.Exit(code=1) - # Mirror the HTTP branch's safety cap so a pathological source - # (e.g. an unbounded pseudo-file that still reports as regular) can't - # exhaust the disk. stat() follows the symlinked source, matching - # what copyfile actually reads. - try: - source_size = local_source.stat().st_size - except OSError as e: - renderer.error( - code="download_failed", - message=f"Failed to stat local output {idx}: {e}", - hint="ensure the output file is readable", - details={"url": url, "path": str(local_source), "index": idx}, - ) - raise typer.Exit(code=1) - if source_size > _MAX_DOWNLOAD_BYTES: - renderer.error( - code="download_failed", - message=f"Local output {idx} exceeds {_MAX_DOWNLOAD_BYTES} byte safety limit", - hint="the source file is too large to copy", - details={"url": url, "path": str(local_source), "size": source_size, "index": idx}, - ) - raise typer.Exit(code=1) - # Wrap the copy like the HTTP branch's failure handling: an OSError - # (permission denied, full/read-only dest) or the mid-copy size cap - # must surface as a structured envelope, not an unhandled traceback - # that breaks machine-mode/NDJSON consumers. - try: - _copy_local_output_capped(local_source, local_path) - except (OSError, ValueError) as e: - renderer.error( - code="download_failed", - message=f"Failed to copy local output {idx}: {e}", - hint="check filesystem permissions and free space in the out-dir", - details={"url": url, "path": str(local_source), "index": idx}, - ) - raise typer.Exit(code=1) - else: - try: - _assert_download_url(url) - except ValueError as e: - renderer.error( - code="download_failed", - message=str(e), - hint="output URLs should be http or https", - details={"url": url, "index": idx}, - ) - raise typer.Exit(code=1) - - req = urllib.request.Request(url) - for hdr, val in auth_hdrs.items(): - req.add_header(hdr, val) - - # Stream into an exclusively-created temp file and rename it into - # place only once the body is complete and verified, so - # `local_path` never holds a partial file no matter how the - # transfer dies. - part_path: Path | None = None - try: - with _DOWNLOAD_OPENER.open(req, timeout=_DOWNLOAD_TIMEOUT_S) as resp: - expected = _declared_content_length(resp) - if expected is not None and expected > _MAX_DOWNLOAD_BYTES: - renderer.error( - code="download_failed", - message=f"Output {idx} declares {expected} bytes, over the {_MAX_DOWNLOAD_BYTES} byte safety limit", - hint="the output is too large to download", - details={"url": url, "index": idx, "declared_bytes": expected}, - ) - raise typer.Exit(code=1) - part_file, part_path = _open_part_file(local_path) - total = 0 - with part_file as fp: - while True: - chunk = resp.read(_DOWNLOAD_CHUNK) - if not chunk: - break - total += len(chunk) - if expected is not None and total > expected: - # http.client clips plain Content-Length bodies, - # but ignores Content-Length when the response - # is chunked — that pairing could stream far - # past the declared size before the post-loop - # check fires. - renderer.error( - code="download_failed", - message=( - f"Download of output {idx} exceeds its declared " - f"Content-Length of {expected} bytes" - ), - hint="the server sent more data than it declared", - details={ - "url": url, - "index": idx, - "declared_bytes": expected, - "received_bytes": total, - }, - ) - raise typer.Exit(code=1) - if total > _MAX_DOWNLOAD_BYTES: - renderer.error( - code="download_failed", - message=f"Download of output {idx} exceeds {_MAX_DOWNLOAD_BYTES} byte safety limit", - hint="the output is too large to download", - details={"url": url, "index": idx, "received_bytes": total}, - ) - raise typer.Exit(code=1) - fp.write(chunk) - # http.client returns EOF instead of raising IncompleteRead when - # a Content-Length body is cut short and read in chunks, so a - # dropped connection otherwise looks like a completed download — - # verify the byte count explicitly. - if expected is not None and total != expected: - renderer.error( - code="download_failed", - message=f"Download of output {idx} truncated: received {total} of {expected} bytes", - hint="the connection dropped mid-transfer; retry the download", - details={"url": url, "index": idx, "declared_bytes": expected, "received_bytes": total}, - ) - raise typer.Exit(code=1) - part_path.replace(local_path) - part_path = None - except urllib.error.HTTPError as e: - renderer.error( - code="download_failed", - message=f"Failed to download output {idx}: HTTP {e.code}", - hint="check that the job completed successfully and the server is reachable", - details={"status": e.code, "url": url, "index": idx}, - ) - raise typer.Exit(code=1) - except (OSError, http.client.HTTPException) as e: - # Everything that isn't an HTTP status lands here: URLError - # (refused/DNS/TLS — a subclass of OSError), a socket timeout - # or reset mid-read, filesystem errors from the temp-file - # create/write/rename, and a truncated *chunked* body — which - # raises http.client.IncompleteRead (an HTTPException, not an - # OSError) rather than the silent EOF a Content-Length body - # gives. Emit the envelope instead of a traceback so - # machine-mode consumers keep their contract. - reason = getattr(e, "reason", None) or e - # A bare TimeoutError()/IncompleteRead can stringify to "", - # which would emit a reason-less envelope — fall back to the - # exception's type name so the cause is always diagnosable. - reason_text = str(reason) or type(e).__name__ - renderer.error( - code="download_failed", - message=f"Failed to download output {idx}: {reason_text}", - hint="check that the server is reachable and the out-dir is writable", - details={"url": url, "index": idx, "reason": reason_text}, - ) - raise typer.Exit(code=1) - finally: - # Cleared after the success rename; on every failure path this - # removes the partial download. - if part_path is not None: - try: - part_path.unlink(missing_ok=True) - except OSError: - pass - - file_size = local_path.stat().st_size - entry: dict[str, Any] = { - "url": url, - "path": str(local_path.resolve()), - "size": file_size, - } - # Optional provenance keys — present only when known (no nulls). - if node_id is not None: - entry["node_id"] = node_id - if item is not None: - entry["item"] = item + entry = _download_one_url( + url, + idx, + dest=dest, + annotations=annotations, + item_counters=item_counters, + short_id=short_id, + is_local_job=is_local_job, + auth_hdrs=auth_hdrs, + renderer=renderer, + ) saved_files.append(entry) - saved_paths.append(str(local_path.resolve())) + saved_paths.append(entry["path"]) # Human progress line + inline previews are pretty-mode-only: machine # consumers read the envelope, and `comfy --json download | jq` requires diff --git a/tests/comfy_cli/command/test_transfer_download.py b/tests/comfy_cli/command/test_transfer_download.py index 0a395830..161ef27b 100644 --- a/tests/comfy_cli/command/test_transfer_download.py +++ b/tests/comfy_cli/command/test_transfer_download.py @@ -880,6 +880,271 @@ def test_loopback_file_uris_are_local(self, url): assert transfer._local_source_path(url) == Path("/abs/path.png") +class TestDownloadHelperExtraction: + """Direct, guard-pinning unit tests for the per-URL helpers extracted from + ``execute_download`` (BE-3273): ``_download_one_url`` orchestrates naming + + the symlink-dest refusal + branch dispatch; ``_copy_local_one`` and + ``_stream_http_one`` carry the two download branches. The behavior-preserving + extraction must keep every guard, envelope code, and exit intact — and the + ``_local_source_path`` gate must stay pinned to ``is_local_job``. + """ + + def _json_renderer(self): + # Pass the renderer explicitly (the helpers take it as an argument), so + # no process-wide singleton state leaks between direct-call tests. + return Renderer(mode=OutputMode.JSON, command="download") + + def _error(self, capsys) -> dict: + lines = [ln for ln in capsys.readouterr().out.splitlines() if ln.strip()] + envelope = json.loads(lines[-1]) + assert envelope["type"] == "envelope" + assert envelope["ok"] is False + return envelope["error"] + + # -- _download_one_url --------------------------------------------------- + + def test_non_local_job_never_calls_local_source_path_and_ssrf_rejects(self, tmp_path, capsys, monkeypatch): + # With is_local_job=False the local-copy path is never consulted: + # _local_source_path must NOT be called (monkeypatched to explode), and a + # bare filesystem path flows to the HTTP branch where the SSRF assert + # rejects it as a non-HTTP URL. + import typer + + def _boom(url): + raise AssertionError("_local_source_path must not be called for a non-local job") + + monkeypatch.setattr(transfer, "_local_source_path", _boom) + dest = tmp_path / "out" + dest.mkdir() + with pytest.raises(typer.Exit) as excinfo: + transfer._download_one_url( + "/etc/passwd", + 0, + dest=dest, + annotations=[(None, None)], + item_counters={}, + short_id="prompt-d", + is_local_job=False, + auth_hdrs={}, + renderer=self._json_renderer(), + ) + assert excinfo.value.exit_code == 1 + err = self._error(capsys) + assert err["code"] == "download_failed" + assert "non-HTTP URL" in err["message"] + assert err["details"] == {"url": "/etc/passwd", "index": 0} + # Nothing was written. + assert list(dest.iterdir()) == [] + + def test_local_job_copies_and_shares_per_item_counter(self, tmp_path, capsys): + # A real local source file is copied in, the entry reports the written + # path + true size, and a shared item_counters dict advances the per-item + # index across two calls. + src_dir = tmp_path / "src" + src_dir.mkdir() + src_a = src_dir / "a.png" + src_a.write_bytes(b"\x89PNG-a-bytes") + src_b = src_dir / "b.png" + src_b.write_bytes(b"\x89PNG-longer-b-bytes") + dest = tmp_path / "out" + dest.mkdir() + + annotations = [("5", "s1"), ("5", "s1")] + item_counters: dict[str, int] = {} + renderer = self._json_renderer() + + entry0 = transfer._download_one_url( + str(src_a), + 0, + dest=dest, + annotations=annotations, + item_counters=item_counters, + short_id="prompt-d", + is_local_job=True, + auth_hdrs={}, + renderer=renderer, + ) + entry1 = transfer._download_one_url( + str(src_b), + 1, + dest=dest, + annotations=annotations, + item_counters=item_counters, + short_id="prompt-d", + is_local_job=True, + auth_hdrs={}, + renderer=renderer, + ) + + assert Path(entry0["path"]).name == "s1_000.png" + assert Path(entry1["path"]).name == "s1_001.png" + assert item_counters == {"s1": 2} + assert Path(entry0["path"]).read_bytes() == b"\x89PNG-a-bytes" + assert entry0["size"] == len(b"\x89PNG-a-bytes") + assert entry1["size"] == len(b"\x89PNG-longer-b-bytes") + assert entry0["node_id"] == "5" and entry0["item"] == "s1" + # Source files are copied, not moved. + assert src_a.is_file() and src_b.is_file() + + def test_symlink_destination_refused_before_copy_or_fetch(self, tmp_path, capsys, monkeypatch): + # The dest-symlink guard fires before either branch runs. _collision_safe_path + # normally skips symlinks, so pin it to the planted symlink to reach the guard, + # and make both branch sinks explode to prove neither is entered. + import typer + + dest = tmp_path / "out" + dest.mkdir() + target = tmp_path / "elsewhere.png" + target.write_bytes(b"attacker-target") + link = dest / "s1_000.png" + link.symlink_to(target) + + monkeypatch.setattr(transfer, "_collision_safe_path", lambda p: link) + monkeypatch.setattr( + transfer, + "_copy_local_output_capped", + lambda *a: (_ for _ in ()).throw(AssertionError("copy must not run")), + ) + monkeypatch.setattr( + transfer._DOWNLOAD_OPENER, + "open", + lambda *a, **k: (_ for _ in ()).throw(AssertionError("fetch must not run")), + ) + + src = tmp_path / "src.png" + src.write_bytes(b"local") + with pytest.raises(typer.Exit) as excinfo: + transfer._download_one_url( + str(src), + 0, + dest=dest, + annotations=[("5", "s1")], + item_counters={}, + short_id="prompt-d", + is_local_job=True, + auth_hdrs={}, + renderer=self._json_renderer(), + ) + assert excinfo.value.exit_code == 1 + err = self._error(capsys) + assert err["code"] == "download_failed" + assert "Refusing to write to symlink" in err["message"] + # The symlink and its target are untouched. + assert link.is_symlink() + assert target.read_bytes() == b"attacker-target" + + # -- _stream_http_one ---------------------------------------------------- + + def _stream(self, tmp_path, capsys, resp, *, expect_exit=True): + import typer + + dest = tmp_path / "out" + dest.mkdir(exist_ok=True) + local_path = dest / "img.png" + renderer = self._json_renderer() + with patch.object(transfer._DOWNLOAD_OPENER, "open", side_effect=lambda req, timeout=None: resp): + if expect_exit: + with pytest.raises(typer.Exit) as excinfo: + transfer._stream_http_one("https://x/view?filename=a.png", 0, local_path, {}, renderer) + assert excinfo.value.exit_code == 1 + return self._error(capsys), dest, local_path + transfer._stream_http_one("https://x/view?filename=a.png", 0, local_path, {}, renderer) + return None, dest, local_path + + def test_stream_declared_over_cap_refused_before_body_read(self, tmp_path, capsys, monkeypatch): + monkeypatch.setattr(transfer, "_MAX_DOWNLOAD_BYTES", 100) + resp = _FakeResp(b"x" * 40, content_length=500) + err, dest, _ = self._stream(tmp_path, capsys, resp) + assert err["code"] == "download_failed" + assert err["details"]["declared_bytes"] == 500 + assert resp.reads == 0 # refused before the first body read + assert list(dest.glob("*")) == [] + + def test_stream_over_declared_midstream_aborts(self, tmp_path, capsys): + resp = _FakeResp(b"x" * 400, content_length=100, chunk_size=32) + err, dest, _ = self._stream(tmp_path, capsys, resp) + assert err["code"] == "download_failed" + assert err["details"]["declared_bytes"] == 100 + assert err["details"]["received_bytes"] > 100 + assert resp.reads > 1 # aborted mid-stream, not only post-loop + assert list(dest.glob("*")) == [] + + def test_stream_running_cap_aborts(self, tmp_path, capsys, monkeypatch): + monkeypatch.setattr(transfer, "_MAX_DOWNLOAD_BYTES", 100) + resp = _FakeResp(b"x" * 400, content_length=None, chunk_size=32) + err, dest, _ = self._stream(tmp_path, capsys, resp) + assert err["code"] == "download_failed" + assert err["details"]["received_bytes"] > 100 + assert list(dest.glob("*")) == [] + + def test_stream_truncation_aborts(self, tmp_path, capsys): + resp = _FakeResp(b"x" * 400, content_length=1000) + err, dest, _ = self._stream(tmp_path, capsys, resp) + assert err["code"] == "download_failed" + assert err["details"]["declared_bytes"] == 1000 + assert err["details"]["received_bytes"] == 400 + assert list(dest.glob("*")) == [] + + def test_stream_success_renames_part_to_final_no_sibling(self, tmp_path, capsys): + _, dest, local_path = self._stream(tmp_path, capsys, _FakeResp(), expect_exit=False) + assert local_path.read_bytes() == b"\x89PNG-fake" + assert list(dest.glob("*.part")) == [] + assert [p.name for p in dest.iterdir()] == ["img.png"] + + # -- _copy_local_one ----------------------------------------------------- + + def test_copy_missing_source_envelope(self, tmp_path, capsys): + import typer + + dest = tmp_path / "out" + dest.mkdir() + missing = tmp_path / "gone.png" + with pytest.raises(typer.Exit) as excinfo: + transfer._copy_local_one(str(missing), 0, missing, dest / "img.png", self._json_renderer()) + assert excinfo.value.exit_code == 1 + err = self._error(capsys) + assert err["code"] == "download_failed" + assert "not found on disk" in err["message"] + assert list(dest.iterdir()) == [] + + def test_copy_stat_cap_breach_envelope(self, tmp_path, capsys, monkeypatch): + import typer + + monkeypatch.setattr(transfer, "_MAX_DOWNLOAD_BYTES", 100) + src = tmp_path / "big.png" + src.write_bytes(b"x" * 300) + dest = tmp_path / "out" + dest.mkdir() + with pytest.raises(typer.Exit) as excinfo: + transfer._copy_local_one(str(src), 0, src, dest / "img.png", self._json_renderer()) + assert excinfo.value.exit_code == 1 + err = self._error(capsys) + assert err["code"] == "download_failed" + assert "safety limit" in err["message"] + assert err["details"]["size"] == 300 + assert list(dest.iterdir()) == [] + + @pytest.mark.parametrize("exc", [OSError("disk full"), ValueError("mid-copy cap")]) + def test_copy_capped_failure_surfaces_as_envelope(self, tmp_path, capsys, monkeypatch, exc): + import typer + + src = tmp_path / "src.png" + src.write_bytes(b"\x89PNG-local") + dest = tmp_path / "out" + dest.mkdir() + + def _raise(src_, dst_): + raise exc + + monkeypatch.setattr(transfer, "_copy_local_output_capped", _raise) + with pytest.raises(typer.Exit) as excinfo: + transfer._copy_local_one(str(src), 0, src, dest / "img.png", self._json_renderer()) + assert excinfo.value.exit_code == 1 + err = self._error(capsys) + assert err["code"] == "download_failed" + assert "Failed to copy local output" in err["message"] + + # --------------------------------------------------------------------------- # _default_out_dir — project/1 root wins over the legacy config key # ---------------------------------------------------------------------------