From 1965da56615145718dea90ac780c180ef3af1692 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 10 Aug 2026 10:42:29 -0500 Subject: [PATCH 1/5] feat(client): stream a record's media property to a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BC publishes a record's binary attachment as a `@odata.mediaReadLink` annotation. There was no way to fetch one: every read path in the transport ends at `response.json()`, which is wrong for every byte of a PDF. `BCTransport.download` is a deliberate sibling of `_request` rather than a branch inside it — same retry policy, same error mapping, same structured log, but it streams the body instead of buffering and parsing it. The URL comes out of a response body, so it goes through `assert_bc_origin` first: a tampered mediaReadLink must not receive the bearer token. The retry semantics are the part worth reading twice. 0.7.0 stopped retrying non-idempotent requests, which widened the window in which a transient 5xx is visible rather than silently absorbed — so a download that retries has to be exactly right about bytes. One temp file is opened before the loop and truncated at the top of every attempt: a retry following a half-streamed response would otherwise append the second body to the first half of the first, producing a corrupt file that no error ever mentioned. The temp file is moved onto the destination with os.replace only after the stream completes, so a failure leaves neither a truncated destination nor a stray .part. `AsyncBCClient.get_media` reads the record through the ordinary `_resolve_url`, which is what keeps registry routing and the `disable_standard_api` lockdown applying to a media download exactly as they apply to `get` — a media stream is not a side door around the profile's allowlist. It reads without `$select`, since a projection drops the annotations it is looking for, and refuses to guess when a record advertises several media properties or none. --- src/bcli/client/_async.py | 109 +++++++++ src/bcli/client/_sync.py | 11 + src/bcli/client/_transport.py | 175 +++++++++++++- tests/test_client/test_download_media.py | 287 +++++++++++++++++++++++ 4 files changed, 581 insertions(+), 1 deletion(-) create mode 100644 tests/test_client/test_download_media.py diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index 3cde1b5..2914380 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -47,6 +47,13 @@ r"^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+$" ) +# BC advertises a streamable media property on a record as an OData annotation +# keyed ``@odata.mediaReadLink``. The value is an absolute URL to +# the raw bytes — the only place the download link is published, which is why +# ``get_media`` reads the record without ``$select`` (a projection drops +# annotations along with the fields it filters out). +_MEDIA_READ_LINK_RE = re.compile(r"^(?P.+)@odata\.mediaReadLink$") + def _parse_bound_action(entity_set_name: str) -> tuple[str, str, str] | None: """Recognise a bound-action invocation in ``entity_set_name``. @@ -454,6 +461,108 @@ async def upload_attachment( "record": bc_record or None, } + async def get_media( + self, + entity_set_name: str, + record_id: str, + dest: str | Path, + *, + media_field: str | None = None, + publisher: str | None = None, + group: str | None = None, + version: str | None = None, + ) -> dict[str, Any]: + """Download a record's media stream (PDF, image, blob) to ``dest``. + + The download counterpart of :meth:`upload_attachment`, and the one read + whose payload never reaches stdout: BC hands back raw bytes, so they go + straight to a file. + + Two requests. First the record itself, resolved through the same + ``_resolve_url`` every other read uses — so registry routing, an + explicit ``publisher``/``group``/``version`` override and the + ``disable_standard_api`` lockdown all apply here exactly as they do to + ``get``. A media download is not a side door around the profile's + endpoint allowlist. Then the media link itself, streamed to disk by + :meth:`BCTransport.download` (which re-checks the origin, because that + URL came from the response body). + + Field resolution: + + - ``media_field`` names the property explicitly. Its + ``@odata.mediaReadLink`` is used when the record carries one; + otherwise the conventional ``/`` sub-resource is + composed, which is what BC pages that omit the annotation still + serve. + - Otherwise the record's annotations are scanned. Exactly one media + property is downloaded; zero or several raise, because guessing + would quietly write the wrong stream to the caller's file. + + Returns ``{"path", "bytes_written", "media_field", "content_type", + "media_fields_discovered"}``. + + Read-only. Does not go through SafeContext. + """ + transport = self._ensure_transport() + + record_url = self._resolve_url( + entity_set_name, + record_id=record_id, + publisher=publisher, + group=group, + version=version, + ) + + # No $select: a projection drops the @odata.mediaReadLink annotations, + # which are the only thing this read is after. + record = await transport.get(record_url) + + discovered = [ + m.group("field") + for m in (_MEDIA_READ_LINK_RE.match(key) for key in record) + if m is not None + ] + + if media_field is not None: + # The field is spliced into a URL path when the record carries no + # annotation for it, so it gets the same single-path-component + # validation as a record key. + validate_record_key("media_field", media_field) + field = media_field + link = record.get(f"{media_field}@odata.mediaReadLink") + if not isinstance(link, str) or not link: + link = f"{record_url}/{media_field}" + elif len(discovered) == 1: + field = discovered[0] + link = record[f"{field}@odata.mediaReadLink"] + elif not discovered: + raise BCLIError( + f"No media stream on {entity_set_name}({record_id}): the record carries " + f"no '@odata.mediaReadLink' annotation, so there is nothing to download. " + f"Pass --media if you know the property name, and run " + f"'bcli endpoint fields {entity_set_name}' to see what this endpoint exposes." + ) + else: + candidates = ", ".join(sorted(discovered)) + raise BCLIError( + f"{entity_set_name}({record_id}) exposes {len(discovered)} media " + f"properties: {candidates}. Pass --media to pick one — writing " + f"whichever came first would be a silent guess." + ) + + dest_path = Path(dest).expanduser() + outcome = await transport.download( + link, dest_path, log_context={"endpoint": entity_set_name}, + ) + + return { + "path": str(dest_path), + "bytes_written": outcome["bytes_written"], + "media_field": field, + "content_type": outcome["content_type"], + "media_fields_discovered": discovered, + } + async def list_companies(self) -> list[dict[str, Any]]: """Discover all companies in the current environment.""" transport = self._ensure_transport() diff --git a/src/bcli/client/_sync.py b/src/bcli/client/_sync.py index 8628c04..44a0b97 100644 --- a/src/bcli/client/_sync.py +++ b/src/bcli/client/_sync.py @@ -92,6 +92,17 @@ def upload_attachment( self._async.upload_attachment(parent_type, parent_id, file_path, **kwargs) ) + def get_media( + self, + entity_set_name: str, + record_id: str, + dest: str | Path, + **kwargs, + ) -> dict[str, Any]: + return self._run( + self._async.get_media(entity_set_name, record_id, dest, **kwargs) + ) + def list_companies(self) -> list[dict[str, Any]]: return self._run(self._async.list_companies()) diff --git a/src/bcli/client/_transport.py b/src/bcli/client/_transport.py index 0d51920..13e9c7c 100644 --- a/src/bcli/client/_transport.py +++ b/src/bcli/client/_transport.py @@ -4,8 +4,11 @@ import json import logging +import os import re +import tempfile import time +from pathlib import Path from typing import Any import httpx @@ -291,7 +294,7 @@ def _emit_request_log( retry_count: int, latency_s: float, correlation_id: str | None, - context: dict[str, str] | None, + context: dict[str, Any] | None, *, error: str | None = None, ) -> None: @@ -327,6 +330,176 @@ async def get_absolute(self, url: str) -> dict[str, Any]: assert_bc_origin(url) return await self._request("GET", url) + async def download( + self, + url: str, + dest: Path, + *, + params: dict[str, str] | None = None, + log_context: dict[str, str] | None = None, + ) -> dict[str, Any]: + """Stream a GET response body to ``dest`` and return what was written. + + Read-only by construction: this issues a GET, never parses the body as + JSON, and never goes through ``SafeContext`` — there is nothing to gate + because nothing changes in BC. + + ``url`` is typically a ``@odata.mediaReadLink`` the *server* put in a + response, so it runs through :func:`bcli._url.assert_bc_origin` before + the bearer token is attached — same token-leak guard as + :meth:`get_absolute`. + + Bytes land in a ``..part`` sibling and are moved onto + ``dest`` with :func:`os.replace` once the stream completes, so a failed + download leaves neither a truncated ``dest`` nor a stray part file. + ``dest`` inherits the temp file's ``0600`` mode rather than the umask — + a downloaded invoice is the account's data, not the machine's. + + Returns ``{"bytes_written", "content_type", "correlation_id"}``. + """ + assert_bc_origin(url) + + # Deliberately a sibling of ``_request`` rather than a branch inside + # it: that method buffers the whole response and calls + # ``response.json()`` on success, which is precisely wrong for a media + # stream. The retry *policy* is the one documented above ``_request``'s + # loop; a GET can always be repeated, so this loop is unconditionally + # retry-safe and needs no ``retry_safe`` gate. Only the body handling + # differs. + backoff = INITIAL_BACKOFF + t0 = time.monotonic() + last_error: Exception | None = None + result: dict[str, Any] | None = None + + # One temp file for all attempts. Each attempt rewinds and truncates it + # first: a retry that follows a partially-streamed response would + # otherwise append the second body to the first half of the first. + tmp = tempfile.NamedTemporaryFile( + delete=False, dir=dest.parent, prefix=dest.name + ".", suffix=".part", + ) + tmp_path = Path(tmp.name) + + try: + with tmp as f: + for attempt in range(self._max_retries + 1): + f.seek(0) + f.truncate() + try: + headers = await self._inject_auth() + # The client default is application/json; a media + # stream is whatever BC says it is. + headers["Accept"] = "*/*" + + logger.debug("GET %s (stream, attempt %d)", url, attempt + 1) + + wait: float | None = None + async with self._client.stream( + "GET", url, params=params, headers=headers, + ) as response: + correlation_id = response.headers.get( + "x-ms-correlation-request-id", + ) + + if response.is_success: + written = 0 + async for chunk in response.aiter_bytes(): + f.write(chunk) + written += len(chunk) + context: dict[str, Any] = dict(log_context or {}) + context["bytes_written"] = written + self._emit_request_log( + "GET", url, response.status_code, attempt, + time.monotonic() - t0, correlation_id, context, + ) + result = { + "bytes_written": written, + "content_type": response.headers.get("content-type"), + "correlation_id": correlation_id, + } + break + + # A streamed response has no body loaded yet, so + # the error payload has to be read before it can + # be parsed. + await response.aread() + bc_message, correlation_id = _parse_bc_error(response) + status = response.status_code + + if status in _RETRYABLE and attempt < self._max_retries: + retry_after = _get_retry_after(response) + wait = retry_after if retry_after else backoff + logger.warning( + "Retryable error %d on %s, waiting %.1fs (attempt %d/%d)", + status, url, wait, attempt + 1, self._max_retries + 1, + ) + else: + self._emit_request_log( + "GET", url, status, attempt, + time.monotonic() - t0, correlation_id, log_context, + error=bc_message, + ) + error_cls = _ERROR_MAP.get(status, BCLIError) + kwargs: dict[str, Any] = { + "status_code": status, + "bc_message": bc_message, + "correlation_id": correlation_id, + } + if status == 429: + kwargs["retry_after"] = _get_retry_after(response) + message = ( + f"HTTP {status} {response.reason_phrase}: GET {url}" + ) + hint = _hint_for_bc_error(status, bc_message, url) + if hint: + message = f"{message}\n Hint: {hint}" + raise error_cls(message, **kwargs) + + # Sleeping outside the ``async with`` releases the + # connection while we wait. + import asyncio + await asyncio.sleep(wait or backoff) + backoff *= 2 + continue + + except ( + httpx.ConnectError, + httpx.ReadTimeout, + httpx.WriteTimeout, + httpx.RemoteProtocolError, + # A stream can also drop mid-body, which surfaces here + # rather than as a timeout. + httpx.ReadError, + ) as e: + last_error = e + if attempt < self._max_retries: + logger.warning( + "Network error on GET %s: %s, retrying in %.1fs", + url, e, backoff, + ) + import asyncio + await asyncio.sleep(backoff) + backoff *= 2 + continue + self._emit_request_log( + "GET", url, 0, attempt, + time.monotonic() - t0, None, log_context, + error=str(e), + ) + raise ServerError( + f"Network error after {self._max_retries + 1} attempts: {e}", + ) from e + + if result is None: + raise ServerError( + f"Download failed after {self._max_retries + 1} attempts", + ) from last_error + + os.replace(tmp_path, dest) + return result + finally: + # No-op once os.replace has moved it; cleans up every failure path. + tmp_path.unlink(missing_ok=True) + async def post( self, url: str, diff --git a/tests/test_client/test_download_media.py b/tests/test_client/test_download_media.py new file mode 100644 index 0000000..0aab222 --- /dev/null +++ b/tests/test_client/test_download_media.py @@ -0,0 +1,287 @@ +"""Tests for AsyncBCClient.get_media / BCTransport.download — media stream download. + +Two requests per download: the record (through the normal resolver, so the +registry and ``disable_standard_api`` still apply) and then the media link the +record advertises. The interesting cases are all about *which* link gets +fetched and what is left on disk when the fetch goes wrong. +""" + +from __future__ import annotations + +import httpx +import pytest + +from bcli.client._async import AsyncBCClient +from bcli.client._transport import BCTransport +from bcli.errors import BCLIError, NotFoundError + +# A BC-origin media link with placeholder ids — the host has to be on the +# allowlist or assert_bc_origin rejects it before any request goes out. +COMPANY_ID = "00000000-0000-0000-0000-000000000001" +RECORD_ID = "00000000-0000-0000-0000-000000000002" +MEDIA_URL = ( + f"https://api.businesscentral.dynamics.com/v2.0/Sandbox/api/v2.0" + f"/companies({COMPANY_ID})/incomingDocuments({RECORD_ID})/content" +) +PDF_BYTES = b"%PDF-1.4\n%fake pdf bytes for testing\n%%EOF\n" + + +class FakeAuth: + async def get_access_token(self) -> str: + return "fake-token" + + +def _client(max_retries: int = 0) -> AsyncBCClient: + c = AsyncBCClient( + tenant_id="test-tenant", + client_id="test-client", + client_secret="test-secret", + environment="Sandbox", + company_id=COMPANY_ID, + ) + c._transport = BCTransport(FakeAuth(), timeout=5, max_retries=max_retries) + return c + + +@pytest.fixture +def client() -> AsyncBCClient: + return _client() + + +def _record(**annotations: str) -> dict: + base = {"id": RECORD_ID, "description": "an incoming document"} + base.update(annotations) + return base + + +def _part_files(directory) -> list: + return sorted(directory.glob("*.part")) + + +class TestHappyPath: + async def test_downloads_the_single_advertised_media_stream( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response( + content=PDF_BYTES, headers={"content-type": "application/pdf"}, + ) + + dest = tmp_path / "invoice.pdf" + result = await client.get_media("incomingDocuments", RECORD_ID, dest) + + assert dest.read_bytes() == PDF_BYTES + assert result == { + "path": str(dest), + "bytes_written": len(PDF_BYTES), + "media_field": "content", + "content_type": "application/pdf", + "media_fields_discovered": ["content"], + } + + requests = httpx_mock.get_requests() + assert len(requests) == 2 + # Record first, through the resolver — not the media link. + assert requests[0].method == "GET" + assert str(requests[0].url).endswith(f"/incomingDocuments({RECORD_ID})") + # ...then the media link exactly as the record advertised it. + assert requests[1].method == "GET" + assert str(requests[1].url) == MEDIA_URL + assert requests[1].headers["authorization"] == "Bearer fake-token" + # The client default is application/json; a media stream is whatever + # BC decides to send. + assert requests[1].headers["accept"] == "*/*" + + async def test_no_select_on_the_record_read(self, client, tmp_path, httpx_mock): + """$select strips the annotations the download depends on.""" + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response(content=PDF_BYTES) + + await client.get_media("incomingDocuments", RECORD_ID, tmp_path / "x.pdf") + + assert "$select" not in str(httpx_mock.get_requests()[0].url) + + async def test_overwrites_an_existing_destination(self, client, tmp_path, httpx_mock): + """The SDK replaces the file; refusing to is the CLI's policy, not this layer's.""" + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response(content=PDF_BYTES) + + dest = tmp_path / "existing.pdf" + dest.write_bytes(b"stale contents") + + await client.get_media("incomingDocuments", RECORD_ID, dest) + + assert dest.read_bytes() == PDF_BYTES + + def test_sync_wrapper_delegates(self, tmp_path, httpx_mock, monkeypatch): + from bcli.client._sync import BCClient + + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response(content=PDF_BYTES) + + sync = BCClient.__new__(BCClient) + sync._async = _client() + + dest = tmp_path / "sync.pdf" + result = sync.get_media("incomingDocuments", RECORD_ID, dest) + + assert result["bytes_written"] == len(PDF_BYTES) + assert dest.read_bytes() == PDF_BYTES + + +class TestFieldResolution: + async def test_zero_media_fields_names_the_endpoint_and_suggests_media( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response(json=_record()) + + with pytest.raises(BCLIError) as exc: + await client.get_media("vendors", RECORD_ID, tmp_path / "nope.pdf") + + message = str(exc.value) + assert "vendors" in message + assert "--media" in message + assert "bcli endpoint fields vendors" in message + assert not (tmp_path / "nope.pdf").exists() + assert len(httpx_mock.get_requests()) == 1 + + async def test_two_media_fields_lists_both_candidates( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response(json=_record(**{ + "content@odata.mediaReadLink": MEDIA_URL, + "thumbnail@odata.mediaReadLink": MEDIA_URL + "Thumb", + })) + + with pytest.raises(BCLIError) as exc: + await client.get_media("incomingDocuments", RECORD_ID, tmp_path / "x.pdf") + + message = str(exc.value) + assert "content" in message + assert "thumbnail" in message + assert "--media" in message + # Nothing downloaded — the record read is the only request. + assert len(httpx_mock.get_requests()) == 1 + + async def test_explicit_media_field_wins_over_discovery( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response(json=_record(**{ + "content@odata.mediaReadLink": MEDIA_URL, + "thumbnail@odata.mediaReadLink": MEDIA_URL + "Thumb", + })) + httpx_mock.add_response(content=b"thumb") + + result = await client.get_media( + "incomingDocuments", RECORD_ID, tmp_path / "t.png", media_field="thumbnail", + ) + + assert result["media_field"] == "thumbnail" + assert str(httpx_mock.get_requests()[1].url) == MEDIA_URL + "Thumb" + assert sorted(result["media_fields_discovered"]) == ["content", "thumbnail"] + + async def test_explicit_field_without_annotation_composes_sub_resource( + self, client, tmp_path, httpx_mock, + ): + """Pages that serve a media property without annotating it still work.""" + httpx_mock.add_response(json=_record()) + httpx_mock.add_response(content=PDF_BYTES) + + result = await client.get_media( + "incomingDocuments", RECORD_ID, tmp_path / "x.pdf", media_field="attachment", + ) + + record_url = str(httpx_mock.get_requests()[0].url) + assert str(httpx_mock.get_requests()[1].url) == f"{record_url}/attachment" + assert result["media_field"] == "attachment" + assert result["media_fields_discovered"] == [] + + async def test_traversal_in_media_field_is_rejected_before_any_http( + self, client, tmp_path, httpx_mock, + ): + """A field name is a single path component — it can't retarget the URL.""" + httpx_mock.add_response(json=_record()) + + with pytest.raises(ValueError, match="media_field"): + await client.get_media( + "incomingDocuments", RECORD_ID, tmp_path / "x.pdf", + media_field="../../evil", + ) + + # Only the record read happened; no media request was ever composed. + assert len(httpx_mock.get_requests()) == 1 + assert not list(tmp_path.iterdir()) + + +class TestOriginGuard: + async def test_off_origin_media_link_is_refused(self, client, tmp_path, httpx_mock): + """A tampered mediaReadLink must not receive the bearer token.""" + evil = "https://attacker.example/leak" + httpx_mock.add_response(json=_record(**{"content@odata.mediaReadLink": evil})) + + with pytest.raises(ValueError, match="off-origin"): + await client.get_media("incomingDocuments", RECORD_ID, tmp_path / "x.pdf") + + requests = httpx_mock.get_requests() + assert len(requests) == 1 + assert all("attacker.example" not in str(r.url) for r in requests) + assert not list(tmp_path.iterdir()) + + +class TestRetryAndCleanup: + async def test_retry_after_partial_stream_writes_the_body_once( + self, tmp_path, httpx_mock, + ): + """A retried attempt truncates first, so bytes can't be written twice.""" + client = _client(max_retries=2) + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response(status_code=503, json={"error": {"message": "busy"}}) + httpx_mock.add_response(content=PDF_BYTES) + + dest = tmp_path / "retried.pdf" + result = await client.get_media("incomingDocuments", RECORD_ID, dest) + + assert dest.read_bytes() == PDF_BYTES + assert result["bytes_written"] == len(PDF_BYTES) + assert _part_files(tmp_path) == [] + + async def test_retry_after_mid_stream_network_error(self, tmp_path, httpx_mock): + client = _client(max_retries=2) + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_exception(httpx.ReadTimeout("dropped")) + httpx_mock.add_response(content=PDF_BYTES) + + dest = tmp_path / "flaky.pdf" + await client.get_media("incomingDocuments", RECORD_ID, dest) + + assert dest.read_bytes() == PDF_BYTES + assert _part_files(tmp_path) == [] + + async def test_404_on_the_media_link_leaves_no_file_and_no_litter( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response( + json=_record(**{"content@odata.mediaReadLink": MEDIA_URL}), + ) + httpx_mock.add_response( + status_code=404, json={"error": {"message": "Media not found"}}, + ) + + dest = tmp_path / "missing.pdf" + with pytest.raises(NotFoundError, match="404"): + await client.get_media("incomingDocuments", RECORD_ID, dest) + + assert not dest.exists() + assert list(tmp_path.iterdir()) == [] From e63108fe0b58d0b0158505f198f95b9afe8fe926 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 10 Aug 2026 11:04:17 -0500 Subject: [PATCH 2/5] feat(cli): --out on get and action for binary payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One flag, two transports. `bcli get --out invoice.pdf` streams the record's media property; `bcli action --out doc.pdf` decodes the base64 payload a bound action returns. Both answer the same question — "give me the bytes, not a printout" — so they share the destination policy in `_out_path`: expanduser, refuse an existing file without --overwrite, and treat a missing parent directory as an error rather than an mkdir, since a typo'd path is likelier than a wanted new tree. On `get`, --out is single-record mode, so every flag that shapes a record *list* is rejected rather than ignored — a caller who passed --filter expected records, and quietly writing one record's bytes instead would be the wrong kind of helpful. An explicitly-passed --format is a conflict for the same reason; a format inherited from config is not, because it was never about this invocation. On `action`, the destination is vetted before the POST. An action can change BC, and finding out afterwards that the payload has nowhere to go would leave the mutation applied and the bytes lost. Decoding likewise happens before the success envelope is emitted, so an undecodable response is recorded as failed rather than succeeded-with-no-file. A 204 No Content says so instead of writing a zero-byte file that looks exactly like a successful download. The help text spells out the difference from --result-out, which writes the JSON envelope *about* the invocation: both are "write output to a file", and an agent picking the wrong one gets no signal that it did. The describe test pins --out/--media on `get`, because bc_get is generated from describe — an option missing there is an option an agent cannot reach. --- src/bcli_cli/_out_path.py | 69 ++++++ src/bcli_cli/commands/action_cmd.py | 92 +++++++- src/bcli_cli/commands/get_cmd.py | 156 +++++++++++++ tests/test_cli/test_action_cmd.py | 118 ++++++++++ tests/test_cli/test_get_cmd_out.py | 206 ++++++++++++++++++ .../test_describe_positionals_limits.py | 14 ++ 6 files changed, 653 insertions(+), 2 deletions(-) create mode 100644 src/bcli_cli/_out_path.py create mode 100644 tests/test_cli/test_get_cmd_out.py diff --git a/src/bcli_cli/_out_path.py b/src/bcli_cli/_out_path.py new file mode 100644 index 0000000..2e185db --- /dev/null +++ b/src/bcli_cli/_out_path.py @@ -0,0 +1,69 @@ +"""Destination-path handling for the ``--out`` flag. + +``bcli get --out`` and ``bcli action --out`` both take a path from the user and +put bytes there. They share this module so the two verbs answer "may I write +here?" identically — and, more importantly, so both answer it *before* touching +the network. For ``action`` that ordering is the whole point: the POST it is +about to send can change BC, and discovering an unwritable destination +afterwards would leave a mutation applied with its payload nowhere to go. + +The refusal-by-default policy matches ``bcli extract`` (``_check_writeable``): +an existing file is never replaced without ``--overwrite``. +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +import typer +from rich.console import Console + +console = Console(stderr=True) + + +def prepare_out_path(path: Path, *, overwrite: bool) -> Path: + """Expand ``path`` and confirm it is safe to write, or exit non-zero. + + Returns the expanded path. Raises ``typer.Exit(1)`` when the file exists + and ``--overwrite`` was not passed, or when the parent directory is + missing — the directory is never created, because a typo'd path is far + more likely than a genuinely wanted new tree. + """ + dest = path.expanduser() + + if dest.exists() and not overwrite: + console.print( + f"[red]Refusing to overwrite[/red] {dest} — pass [bold]--overwrite[/bold] " + "to replace it." + ) + raise typer.Exit(1) + + if not dest.parent.exists(): + console.print( + f"[red]Error:[/red] directory {dest.parent} does not exist. Create it first " + "(bcli won't), then re-run." + ) + raise typer.Exit(1) + + return dest + + +def atomic_write_bytes(dest: Path, raw: bytes) -> None: + """Write ``raw`` to ``dest`` via a temp file + :func:`os.replace`. + + Same discipline as ``BCTransport.download``: a reader never sees a + half-written file, and a failed write leaves no part file behind. + """ + fd, tmp_name = tempfile.mkstemp(prefix=dest.name + ".", dir=str(dest.parent)) + try: + with os.fdopen(fd, "wb") as f: + f.write(raw) + os.replace(tmp_name, dest) + except Exception: + try: + os.unlink(tmp_name) + except OSError: + pass + raise diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py index bc40cb4..a70c6d7 100644 --- a/src/bcli_cli/commands/action_cmd.py +++ b/src/bcli_cli/commands/action_cmd.py @@ -34,7 +34,9 @@ import typer from rich.console import Console +from bcli.errors import BCLIError from bcli_cli._envelope_wrap import capture, validate_flags +from bcli_cli._out_path import atomic_write_bytes, prepare_out_path from bcli_cli._state import state from bcli_cli.output import format_output, print_context_banner @@ -87,9 +89,25 @@ def action_command( False, "--yes", "-y", help="Skip the read-only-profile warning prompt", ), + out: Optional[Path] = typer.Option( + None, "--out", + help=( + "Decode the action's base64 return value and write the raw bytes here. " + "NOT the same as --result-out: --out writes the action's decoded payload " + "bytes (a PDF, an export), --result-out writes the JSON result envelope " + "describing the invocation." + ), + ), + overwrite: bool = typer.Option( + False, "--overwrite", + help="Replace an existing --out file (refused by default)", + ), result_out: Optional[Path] = typer.Option( None, "--result-out", - help="Write a JSON result envelope to this path (atomic). See AIP §Phase 2.", + help=( + "Write a JSON result envelope to this path (atomic). See AIP §Phase 2. " + "For the action's payload bytes, see --out." + ), ), result_fd: Optional[int] = typer.Option( None, "--result-fd", @@ -107,9 +125,15 @@ def action_command( bcli action examples 42 archive bcli action items "'ALFKI'" doSomething --data '{"flag": true}' bcli action widgets 7 cancel --namespace Custom.Ns --data @payload.json + bcli action documents 42 renderPdf --out document.pdf """ validate_flags(result_out, result_fd) + # Vet the destination before the write gate and before the POST. An action + # can change BC; finding out afterwards that the payload has nowhere to go + # would leave the mutation applied and the bytes lost. + dest = prepare_out_path(out, overwrite=overwrite) if out is not None else None + # ``--data`` and ``--no-data`` may not both be set. Either alone, or # neither (defaults to ``{}``), is fine — matches ``bcli post``. if data is not None and no_data: @@ -169,8 +193,23 @@ def action_command( idempotency_key=idempotency_key, )) cap.extract_record_id_from(result) + + written: int | None = None + if dest is not None: + # Decode *before* emit_success: a return value we can't turn + # into bytes means the caller didn't get what they asked for, + # and the envelope has to say failed. The POST already + # happened either way — that's what the envelope records. + written = _write_decoded_payload(result, dest) + cap.emit_success() - if result: + + if dest is not None: + # Deliberately no format_output here: the payload is base64, + # and dumping it to stdout after writing the decoded file is + # noise at best and a wrecked pipe at worst. + console.print(f"[green]✓[/green] Decoded {written:,} bytes to {dest}") + elif result: format_output([result], output_format) else: # 204 No Content — common for actions that mutate but @@ -205,6 +244,55 @@ async def _execute_post(endpoint, body, **kwargs): return await client.post(endpoint, body, **kwargs) +def _decode_base64_payload(result: dict | str) -> bytes: + """Turn an action's return value into the raw bytes ``--out`` asked for. + + OData carries binary in an action's ``value`` property as base64, so that + is the shape we decode. Anything else is reported rather than guessed at: + an action that returned no payload, or a structured result, is a sign the + caller wanted ``--result-out`` (or nothing at all), and writing a + zero-byte file would look exactly like a successful download. + """ + import base64 + import binascii + + if not result: + raise BCLIError( + "action returned 204 No Content — nothing to write to --out. The action " + "ran; it just has no payload. Drop --out, or use --result-out to record " + "the invocation itself." + ) + + if isinstance(result, str): + payload = result + elif isinstance(result.get("value"), str): + payload = result["value"] + else: + keys = ", ".join(sorted(str(k) for k in result)) + raise BCLIError( + f"action's return value has no base64 'value' property (keys: {keys}). " + f"--out handles a base64 payload only — use --result-out for the JSON " + f"result envelope, or drop --out to print the response." + ) + + try: + return base64.b64decode(payload.strip(), validate=True) + except (binascii.Error, ValueError) as e: + preview = payload.strip()[:32] + raise BCLIError( + f"action's return value is not valid base64 (starts with: {preview!r}): {e}. " + f"If you wanted the JSON response rather than a decoded payload, use " + f"--result-out or drop --out." + ) from e + + +def _write_decoded_payload(result: dict | str, dest: Path) -> int: + """Decode the action's payload onto ``dest`` atomically; return byte count.""" + raw = _decode_base64_payload(result) + atomic_write_bytes(dest, raw) + return len(raw) + + def _parse_data(data: str) -> dict: """Parse --data argument: JSON string or @filename.""" if data.startswith("@"): diff --git a/src/bcli_cli/commands/get_cmd.py b/src/bcli_cli/commands/get_cmd.py index 92b6734..a05c895 100644 --- a/src/bcli_cli/commands/get_cmd.py +++ b/src/bcli_cli/commands/get_cmd.py @@ -3,12 +3,14 @@ from __future__ import annotations import asyncio +from pathlib import Path from typing import Optional import typer from rich.console import Console from bcli.odata._query import Query +from bcli_cli._out_path import prepare_out_path from bcli_cli._state import state from bcli_cli.output import format_output, print_context_banner @@ -26,6 +28,9 @@ def get_command( skip: Optional[int] = typer.Option(None, "--skip", help="Records to skip"), count: bool = typer.Option(False, "--count", help="Include total record count"), all_pages: bool = typer.Option(False, "--all", help="Follow pagination to get all records"), + out: Optional[Path] = typer.Option(None, "--out", help="Write the record's media stream (raw bytes) to this path instead of printing records"), + media: Optional[str] = typer.Option(None, "--media", help="Media property to download (default: auto-discover from the record's @odata.mediaReadLink annotations)"), + overwrite: bool = typer.Option(False, "--overwrite", help="Replace an existing --out file (refused by default)"), format: Optional[str] = typer.Option(None, "--format", "-f", help="Output format: table, json, csv, ndjson, raw"), publisher: Optional[str] = typer.Option(None, "--publisher", help="Custom API publisher override (escape hatch — registry resolves this automatically)"), group: Optional[str] = typer.Option(None, "--group", help="Custom API group override (escape hatch — registry resolves this automatically)"), @@ -39,7 +44,26 @@ def get_command( bcli get vendors --filter "displayName eq 'Fabrikam'" bcli get items --filter "unitPrice gt 100" --all bcli get salesInvoices --select number,totalAmountIncludingTax --orderby "number desc" + bcli get incomingDocuments --out invoice.pdf """ + _validate_out_flags( + out, media, record_id, endpoint, + query_flags={ + "--filter": filter, "--select": select, "--expand": expand, + "--orderby": orderby, "--top": top, "--skip": skip, + "--count": count, "--all": all_pages, + }, + # Only a locally-passed --format conflicts. A format inherited from + # config or a global flag is a preference about *printed records*, and + # --out prints none — silently ignoring it beats failing a command the + # user spelled correctly. + explicit_format=format is not None, + ) + + # Resolve and vet the destination before anything else: a --out run that + # can't write should cost no round trip. + dest = prepare_out_path(out, overwrite=overwrite) if out is not None else None + # Local --format overrides global output_format = format or state.format explicit_format = (format is not None) or state.format_explicit @@ -48,6 +72,14 @@ def get_command( print_context_banner() + if dest is not None: + # record_id is guaranteed non-empty by _validate_out_flags. + _run_media_download( + endpoint, record_id or "", dest, media, + publisher=publisher, group=group, version=version, + ) + return + if filter: _check_filter_fields(endpoint, filter) @@ -204,6 +236,130 @@ async def _execute_get_all_companies( return all_records +def _validate_out_flags( + out: Optional[Path], + media: Optional[str], + record_id: Optional[str], + endpoint: str, + *, + query_flags: dict[str, object], + explicit_format: bool, +) -> None: + """Enforce the ``--out`` contract before anything reaches the network. + + ``--out`` switches ``get`` from "print a list of records" to "stream one + record's media property to a file". Every flag that shapes a record *list* + is therefore evidence the caller meant the other mode, and answering with + a file they didn't expect is worse than refusing. + """ + if out is None: + if media is not None: + raise typer.BadParameter( + "--media requires --out — it names which media property to write, " + "and without --out there is nowhere to write it." + ) + return + + if not record_id: + raise typer.BadParameter( + f"--out needs a record id: bcli get {endpoint} --out . " + f"Find one first with: bcli get {endpoint} --filter \"...\" --top 1 -f json" + ) + + conflicting = sorted(name for name, value in query_flags.items() if value) + if conflicting: + raise typer.BadParameter( + f"--out streams one record's media bytes, so it cannot be combined with " + f"{', '.join(conflicting)}. Drop those to download, or drop --out to query." + ) + + if explicit_format: + raise typer.BadParameter( + "--out writes raw bytes to a file, so --format has no records to format. " + "Pass one or the other." + ) + + +def _run_media_download( + endpoint: str, + record_id: str, + dest: Path, + media_field: str | None, + *, + publisher: str | None, + group: str | None, + version: str | None, +) -> None: + """Execute the ``--out`` branch: fetch the record, stream its media to ``dest``.""" + if state.dry_run: + which = media_field or "auto-discovered from @odata.mediaReadLink" + console.print( + f"[yellow]--dry-run:[/yellow] would GET {endpoint}({record_id}), read its " + f"media property ({which}) and write the bytes to {dest}. " + f"Nothing fetched, nothing written." + ) + raise typer.Exit() + + import time as _time + + from bcli.telemetry import events as _tev + + sink = state.telemetry + started = _time.monotonic() + try: + result = asyncio.run( + _execute_get_media( + endpoint, record_id, dest, media_field, + publisher=publisher, group=group, version=version, + ) + ) + latency_ms = (_time.monotonic() - started) * 1000.0 + name, props = _tev.query( + endpoint=endpoint, + has_filter=False, + status=200, + latency_ms=latency_ms, + ) + # Additive dimension on the existing query event, so a media download + # still shows up in the same KQL as any other read but can be told + # apart from one. + props["media_download"] = True + sink.emit(name, props) + console.print( + f"[green]✓[/green] Wrote {result['bytes_written']:,} bytes to " + f"{result['path']} ({result['content_type'] or 'unknown content type'}, " + f"media field: {result['media_field']})" + ) + except Exception as e: + sink.emit(*_tev.error( + error_class=type(e).__name__, + http_status=getattr(e, "status_code", 0) or 0, + bc_message=getattr(e, "bc_message", "") or str(e), + correlation_id=getattr(e, "correlation_id", "") or "", + endpoint=endpoint, + )) + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) + + +async def _execute_get_media( + endpoint: str, + record_id: str, + dest: Path, + media_field: str | None, + *, + publisher: str | None = None, + group: str | None = None, + version: str | None = None, +) -> dict: + async with state.make_async_client() as client: + return await client.get_media( + endpoint, record_id, dest, + media_field=media_field, + publisher=publisher, group=group, version=version, + ) + + def _check_filter_fields(endpoint: str, filter_expr: str) -> None: """Pre-flight check: warn if --filter references fields the entity doesn't have. diff --git a/tests/test_cli/test_action_cmd.py b/tests/test_cli/test_action_cmd.py index d16e63c..b01f691 100644 --- a/tests/test_cli/test_action_cmd.py +++ b/tests/test_cli/test_action_cmd.py @@ -18,6 +18,7 @@ from __future__ import annotations +import base64 import json from pathlib import Path from unittest.mock import AsyncMock @@ -93,6 +94,8 @@ def _run( kwargs.setdefault("result_out", None) kwargs.setdefault("result_fd", None) kwargs.setdefault("idempotency_key", None) + kwargs.setdefault("out", None) + kwargs.setdefault("overwrite", False) return action_cmd.action_command( entity_set=entity, key=key, @@ -174,6 +177,121 @@ def test_idempotency_key_forwarded(self, cli_state, fake_client): assert kwargs.get("idempotency_key") == "k-123" +class TestOutDecode: + """``--out`` decodes the action's base64 return value to raw bytes. + + Distinct from ``--result-out``, which writes the JSON envelope *about* + the invocation. The two compose; neither implies the other. + """ + + def test_base64_payload_decoded_to_file( + self, cli_state, fake_client, tmp_path: Path, capsys, + ): + payload = b"%PDF-1.4\nfake\n%%EOF\n" + fake_client.post.return_value = { + "value": base64.b64encode(payload).decode("ascii"), + } + dest = tmp_path / "document.pdf" + + _run(out=dest) + + assert dest.read_bytes() == payload + # The base64 blob must not also land on stdout — --out means "give me + # the bytes", not "give me the bytes and dump the encoding too". + stdout = capsys.readouterr().out + assert base64.b64encode(payload).decode("ascii") not in stdout + + def test_204_no_content_fails_rather_than_writing_an_empty_file( + self, cli_state, fake_client, tmp_path: Path, + ): + fake_client.post.return_value = {} + dest = tmp_path / "document.pdf" + + with pytest.raises(typer.Exit): + _run(out=dest) + + assert not dest.exists() + + def test_204_marks_the_envelope_failed( + self, cli_state, fake_client, tmp_path: Path, + ): + """The POST happened; the caller still didn't get what they asked for.""" + fake_client.post.return_value = {} + envelope = tmp_path / "env.json" + + with pytest.raises(typer.Exit): + _run(out=tmp_path / "document.pdf", result_out=envelope) + + assert json.loads(envelope.read_text())["status"] == "failed" + + def test_dict_without_value_lists_the_keys( + self, cli_state, fake_client, tmp_path: Path, capsys, + ): + fake_client.post.return_value = {"status": "ok", "recordId": "42"} + dest = tmp_path / "document.pdf" + + with pytest.raises(typer.Exit): + _run(out=dest) + + err = capsys.readouterr().err + assert "recordId" in err and "status" in err + assert "--result-out" in err + assert not dest.exists() + + def test_invalid_base64_fails(self, cli_state, fake_client, tmp_path: Path, capsys): + fake_client.post.return_value = {"value": "not base64 at all !!!"} + dest = tmp_path / "document.pdf" + + with pytest.raises(typer.Exit): + _run(out=dest) + + assert "base64" in capsys.readouterr().err + assert not dest.exists() + + def test_existing_file_refused_before_the_post( + self, cli_state, fake_client, tmp_path: Path, + ): + """An action can mutate BC — the destination check has to come first.""" + dest = tmp_path / "document.pdf" + dest.write_bytes(b"do not clobber me") + + with pytest.raises(typer.Exit) as exc: + _run(out=dest) + + assert exc.value.exit_code == 1 + assert fake_client.post.await_count == 0 + assert dest.read_bytes() == b"do not clobber me" + + def test_overwrite_allows_replacing_the_file( + self, cli_state, fake_client, tmp_path: Path, + ): + payload = b"fresh bytes" + fake_client.post.return_value = { + "value": base64.b64encode(payload).decode("ascii"), + } + dest = tmp_path / "document.pdf" + dest.write_bytes(b"stale") + + _run(out=dest, overwrite=True) + + assert dest.read_bytes() == payload + + def test_out_and_result_out_compose( + self, cli_state, fake_client, tmp_path: Path, + ): + payload = b"both channels" + fake_client.post.return_value = { + "value": base64.b64encode(payload).decode("ascii"), + } + dest = tmp_path / "document.pdf" + envelope = tmp_path / "env.json" + + _run(out=dest, result_out=envelope) + + assert dest.read_bytes() == payload + assert json.loads(envelope.read_text())["status"] == "succeeded" + + class TestEnvelope: def test_envelope_written_on_success( self, cli_state, fake_client, tmp_path: Path, diff --git a/tests/test_cli/test_get_cmd_out.py b/tests/test_cli/test_get_cmd_out.py new file mode 100644 index 0000000..94d9c43 --- /dev/null +++ b/tests/test_cli/test_get_cmd_out.py @@ -0,0 +1,206 @@ +"""Tests for ``bcli get --out`` — media-stream download instead of printed records. + +``--out`` flips the verb into a different mode, so most of what is worth +testing is the refusal path: the flag combinations that mean the caller +expected records, and the destination checks that must happen before any +network round trip. +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock + +import pytest +import typer + +from bcli.config._model import BCConfig, BCDefaults, BCProfile +from bcli_cli._state import state +from bcli_cli.commands import get_cmd + +RECORD_ID = "00000000-0000-0000-0000-000000000002" + + +@pytest.fixture +def cli_state(): + cfg = BCConfig( + defaults=BCDefaults(profile="dev"), + profiles={ + "dev": BCProfile( + tenant_id="t1", + environment="Sandbox", + company_id="c-123", + ), + }, + ) + state._config = cfg + state._registry = None + state.profile_name = None + state.env_override = None + state.company_override = None + state.format = "table" + state.format_explicit = False + state.dry_run = False + state.quiet = True + yield state + state._config = None + state._registry = None + state.profile_name = None + state.format_explicit = False + state.dry_run = False + + +@pytest.fixture +def fake_client(monkeypatch): + c = AsyncMock() + c.__aenter__ = AsyncMock(return_value=c) + c.__aexit__ = AsyncMock(return_value=False) + c.get_media = AsyncMock(return_value={ + "path": "/tmp/invoice.pdf", + "bytes_written": 1234, + "media_field": "content", + "content_type": "application/pdf", + "media_fields_discovered": ["content"], + }) + monkeypatch.setattr(state, "make_async_client", lambda **_: c) + return c + + +def _run(*, endpoint="incomingDocuments", record_id=RECORD_ID, **kwargs): + kwargs.setdefault("filter", None) + kwargs.setdefault("select", None) + kwargs.setdefault("expand", None) + kwargs.setdefault("orderby", None) + kwargs.setdefault("top", None) + kwargs.setdefault("skip", None) + kwargs.setdefault("count", False) + kwargs.setdefault("all_pages", False) + kwargs.setdefault("out", None) + kwargs.setdefault("media", None) + kwargs.setdefault("overwrite", False) + kwargs.setdefault("format", None) + kwargs.setdefault("publisher", None) + kwargs.setdefault("group", None) + kwargs.setdefault("version", None) + return get_cmd.get_command(endpoint=endpoint, record_id=record_id, **kwargs) + + +class TestForwarding: + def test_forwards_every_argument_to_get_media( + self, cli_state, fake_client, tmp_path: Path, + ): + dest = tmp_path / "invoice.pdf" + _run( + out=dest, media="content", + publisher="acme", group="finance", version="v1.5", + ) + + args = fake_client.get_media.await_args + assert args.args[0] == "incomingDocuments" + assert args.args[1] == RECORD_ID + assert args.args[2] == dest + assert args.kwargs["media_field"] == "content" + assert args.kwargs["publisher"] == "acme" + assert args.kwargs["group"] == "finance" + assert args.kwargs["version"] == "v1.5" + + def test_expanduser_applied_before_the_client_sees_the_path( + self, cli_state, fake_client, tmp_path: Path, monkeypatch, + ): + monkeypatch.setenv("HOME", str(tmp_path)) + _run(out=Path("~/invoice.pdf")) + + assert fake_client.get_media.await_args.args[2] == tmp_path / "invoice.pdf" + + +class TestFlagValidation: + def test_media_without_out_is_rejected(self, cli_state, fake_client): + with pytest.raises(typer.BadParameter, match="--media requires --out"): + _run(media="content") + assert fake_client.get_media.await_count == 0 + + def test_out_without_record_id_is_rejected( + self, cli_state, fake_client, tmp_path: Path, + ): + with pytest.raises(typer.BadParameter, match="record id"): + _run(record_id=None, out=tmp_path / "x.pdf") + assert fake_client.get_media.await_count == 0 + + @pytest.mark.parametrize("flag,value", [ + ("filter", "number eq '1'"), + ("select", "number"), + ("expand", "lines"), + ("orderby", "number desc"), + ("top", 5), + ("skip", 5), + ("count", True), + ("all_pages", True), + ("format", "json"), + ]) + def test_query_shaping_flags_conflict_with_out( + self, cli_state, fake_client, tmp_path: Path, flag, value, + ): + with pytest.raises(typer.BadParameter): + _run(out=tmp_path / "x.pdf", **{flag: value}) + assert fake_client.get_media.await_count == 0 + + def test_global_default_format_is_ignored_not_an_error( + self, cli_state, fake_client, tmp_path: Path, + ): + """A format inherited from config shapes printed records; --out prints none.""" + cli_state.format = "json" + cli_state.format_explicit = True + + _run(out=tmp_path / "x.pdf") + + assert fake_client.get_media.await_count == 1 + + +class TestDestinationChecks: + def test_existing_file_refused_without_overwrite( + self, cli_state, fake_client, tmp_path: Path, + ): + dest = tmp_path / "invoice.pdf" + dest.write_bytes(b"do not clobber me") + + with pytest.raises(typer.Exit) as exc: + _run(out=dest) + + assert exc.value.exit_code == 1 + assert fake_client.get_media.await_count == 0 + assert dest.read_bytes() == b"do not clobber me" + + def test_existing_file_accepted_with_overwrite( + self, cli_state, fake_client, tmp_path: Path, + ): + dest = tmp_path / "invoice.pdf" + dest.write_bytes(b"stale") + + _run(out=dest, overwrite=True) + + assert fake_client.get_media.await_count == 1 + + def test_missing_parent_directory_is_an_error_not_an_mkdir( + self, cli_state, fake_client, tmp_path: Path, + ): + dest = tmp_path / "no-such-dir" / "invoice.pdf" + + with pytest.raises(typer.Exit) as exc: + _run(out=dest) + + assert exc.value.exit_code == 1 + assert fake_client.get_media.await_count == 0 + assert not dest.parent.exists() + + +class TestDryRun: + def test_dry_run_touches_nothing(self, cli_state, fake_client, tmp_path: Path): + cli_state.dry_run = True + dest = tmp_path / "invoice.pdf" + + with pytest.raises(typer.Exit) as exc: + _run(out=dest) + + assert (exc.value.exit_code or 0) == 0 + assert fake_client.get_media.await_count == 0 + assert not dest.exists() diff --git a/tests/test_describe/test_describe_positionals_limits.py b/tests/test_describe/test_describe_positionals_limits.py index 59ba506..6ce96b4 100644 --- a/tests/test_describe/test_describe_positionals_limits.py +++ b/tests/test_describe/test_describe_positionals_limits.py @@ -100,6 +100,20 @@ def test_optional_options_dont_carry_required_flag(self): assert "required" not in yes_opt or yes_opt["required"] is False +class TestMediaDownloadOptions: + def test_get_exposes_out_and_media(self): + """``bcli get --out/--media`` must reach the describe-generated MCP tools. + + ``bc_get`` is built from this document, so an option missing here is an + option an agent cannot reach — the file lands on the MCP host, which is + the user's own machine. + """ + payload = _describe_json() + get_cmd = _find(payload, ["get"]) + names = {o["name"] for o in get_cmd["options"]} + assert {"--out", "--media", "--overwrite"} <= names + + class TestLimits: def test_get_top_carries_default_and_max(self): """``bcli get --top`` is a safety-sensitive int. We pin the From 03344d45917ae26ec74ecefeaa1130cf3f078fc1 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 10 Aug 2026 11:08:22 -0500 Subject: [PATCH 3/5] feat(sdk): export filter-field helpers and the $metadata importer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `extract_field_references`, `suggest_field` and `validate_filter_fields` now come from `bcli.odata`; `import_from_metadata` from `bcli.registry`. Downstream tooling that validates a saved-query catalog against an endpoint's live fields needs all four, and was importing `bcli.odata._filter_fields` and `bcli.registry._importers` to get them — private modules whose signatures we could not have changed without breaking those consumers with no warning. Making the surface explicit is the same move as the `bcli.queries` extraction in 0.7.0. --- src/bcli/odata/__init__.py | 15 ++++++++- src/bcli/registry/__init__.py | 7 ++++- tests/test_odata/test_public_surface.py | 42 +++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 tests/test_odata/test_public_surface.py diff --git a/src/bcli/odata/__init__.py b/src/bcli/odata/__init__.py index ff365ae..ad0b92b 100644 --- a/src/bcli/odata/__init__.py +++ b/src/bcli/odata/__init__.py @@ -1,8 +1,21 @@ """OData query building and response handling.""" from bcli.odata._escape import escape_odata_string +from bcli.odata._filter_fields import ( + extract_field_references, + suggest_field, + validate_filter_fields, +) from bcli.odata._pagination import PageIterator from bcli.odata._query import Query from bcli.odata._response import ODataResponse -__all__ = ["ODataResponse", "PageIterator", "Query", "escape_odata_string"] +__all__ = [ + "ODataResponse", + "PageIterator", + "Query", + "escape_odata_string", + "extract_field_references", + "suggest_field", + "validate_filter_fields", +] diff --git a/src/bcli/registry/__init__.py b/src/bcli/registry/__init__.py index 565f500..8108009 100644 --- a/src/bcli/registry/__init__.py +++ b/src/bcli/registry/__init__.py @@ -1,6 +1,10 @@ """Endpoint registry for route resolution.""" -from bcli.registry._importers import import_from_json, import_from_postman +from bcli.registry._importers import ( + import_from_json, + import_from_metadata, + import_from_postman, +) from bcli.registry._registry import EndpointRegistry from bcli.registry._schema import EndpointMetadata @@ -8,5 +12,6 @@ "EndpointMetadata", "EndpointRegistry", "import_from_json", + "import_from_metadata", "import_from_postman", ] diff --git a/tests/test_odata/test_public_surface.py b/tests/test_odata/test_public_surface.py new file mode 100644 index 0000000..329ac4a --- /dev/null +++ b/tests/test_odata/test_public_surface.py @@ -0,0 +1,42 @@ +"""``bcli.odata`` and ``bcli.registry`` import surfaces. + +Downstream tooling (a saved-query catalog validator, for one) needs the +filter-field helpers and the ``$metadata`` importer. Re-exporting them from +their packages is what stops those consumers reaching into ``_filter_fields`` +and ``_importers`` — private modules whose signatures we'd otherwise be unable +to change without breaking them silently. +""" + +from __future__ import annotations + + +def test_odata_exports_filter_field_helpers(): + from bcli.odata import ( + extract_field_references, + suggest_field, + validate_filter_fields, + ) + + assert extract_field_references("number eq '1'") == ["number"] + assert suggest_field("numbr", ["number", "postingDate"]) == ["number"] + assert validate_filter_fields("number eq '1'", ["number"]) is None + + +def test_odata_all_lists_the_new_names(): + import bcli.odata as odata + + assert { + "extract_field_references", "suggest_field", "validate_filter_fields", + } <= set(odata.__all__) + + +def test_registry_exports_metadata_importer(): + from bcli.registry import import_from_metadata + + assert callable(import_from_metadata) + + +def test_registry_all_lists_the_metadata_importer(): + import bcli.registry as registry + + assert "import_from_metadata" in registry.__all__ From 50e84b719d2e9c10aa64920087aacc197a7f23c6 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 10 Aug 2026 11:08:22 -0500 Subject: [PATCH 4/5] docs: document --out and release 0.8.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The action verb had no entry in the command reference at all, so it gets one rather than just a --out row — including the distinction from --result-out, which is the mistake the flag most invites. The querying guide documents the two-step shape a media download really has (find the record, then fetch its stream), because --out needs a record id and the error message alone can't teach that. --- CHANGELOG.md | 54 ++++++++++++++++++++++++++++++++++++++ docs/command-reference.md | 55 +++++++++++++++++++++++++++++++++++++++ docs/querying.md | 38 +++++++++++++++++++++++++++ pyproject.toml | 2 +- uv.lock | 2 +- 5 files changed, 149 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1c0b09..ba743ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,60 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.8.0] - 2026-08-10 + +### Added + +- `--out` on `bcli get`: write a record's media stream (a scanned invoice, a + rendered document) to a file instead of printing records. BC advertises a + streamable property as a `@odata.mediaReadLink` annotation, so the + record is read first — without `$select`, which would drop the annotations — + and the single advertised link is streamed to disk. Several media properties + on one record, or none, is an error naming what was found; `--media ` + picks one explicitly, and also composes the conventional + `/` sub-resource for pages that serve a media property + without annotating it. The record read goes through the same resolver as any + other read, so the endpoint registry and `disable_standard_api` govern a media + download exactly as they govern `bcli get` — a media stream is not a side door + around the profile's allowlist. An existing file is refused without + `--overwrite` and a missing parent directory is an error, both checked before + any request goes out. + + The SDK half is `AsyncBCClient.get_media` / `BCClient.get_media` over a new + `BCTransport.download`, which streams rather than buffering and never parses + the body as JSON. `download` is a deliberate sibling of the shared `_request` + loop rather than a branch inside it, because that method's success path calls + `response.json()` — wrong for every byte of a PDF. It re-validates the URL + against the BC host allowlist first, since a `mediaReadLink` is a URL the + *server* chose and the bearer token must not follow it off-origin. Bytes land + in a `.part` sibling that is truncated at the start of every attempt and moved + onto the destination with `os.replace` only on success: a retry after a + half-streamed response would otherwise append the second body to the first + half of the first, and a failed download would leave a truncated file where a + complete one is expected. Nothing is left behind on any failure path. The + downloaded file inherits the temp file's `0600` mode rather than the umask, + on the grounds that a downloaded invoice is the account's data. + +- `--out` on `bcli action`: decode a bound action's base64 return value and + write the raw bytes. Distinct from `--result-out`, which writes the JSON + result envelope *about* the invocation — the two compose, and the help text + says so, because an agent reaching for "write the output to a file" can + otherwise pick either. The destination is vetted before the POST is sent: an + action can change BC, and discovering an unwritable path afterwards would + leave the mutation applied with its payload nowhere to go. A 204 No Content + response reports that there was no payload rather than writing a zero-byte + file, which would be indistinguishable from a successful download. Decoding + happens before the success envelope is emitted, so a payload that can't be + decoded is recorded as failed. + +- `bcli.odata` now exports `extract_field_references`, `suggest_field` and + `validate_filter_fields`; `bcli.registry` now exports `import_from_metadata`. + Downstream tooling validating a saved-query catalog against live endpoint + fields needs both, and was otherwise importing `bcli.odata._filter_fields` and + `bcli.registry._importers` directly — private modules we could not have + changed without breaking those consumers silently. Same reasoning as the + `bcli.queries` extraction in 0.7.0. + ## [0.7.0] - 2026-08-04 ### Added diff --git a/docs/command-reference.md b/docs/command-reference.md index c2fa64b..dc1b659 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -259,10 +259,65 @@ bcli get [] [options] | `--skip ` | Records to skip | | `--count` | Include total record count | | `--all` | Follow pagination for all records | +| `--out ` | Write the record's media stream (raw bytes) to this path instead of printing records. Requires a record id | +| `--media ` | Media property to download (default: auto-discovered from the record's `@odata.mediaReadLink` annotations). Requires `--out` | +| `--overwrite` | Replace an existing `--out` file (refused by default) | | `--publisher ` | Custom API publisher override | | `--group ` | Custom API group override | | `--version ` | Custom API version override | +Downloading a record's attachment: + +```bash +# Find the record, then stream its media property to a file. +bcli get incomingDocuments --filter "description eq 'March invoice'" --top 1 -f json +bcli get incomingDocuments --out invoice.pdf +``` + +`--out` is single-record mode and cannot be combined with the query-shaping +flags (`--filter`, `--select`, `--expand`, `--orderby`, `--top`, `--skip`, +`--count`, `--all`) or an explicit `--format`. The record is fetched through +normal endpoint resolution, so `disable_standard_api` and the endpoint registry +apply to a media download exactly as they do to a read. See +[querying.md](querying.md#downloading-media-streams-pdfs). + +--- + +## action + +Invoke an OData v4 bound action on a record: `POST ()/.`. + +```bash +bcli action [options] +``` + +| Option | Description | +|--------|-------------| +| `--data ` | JSON body for the action (literal or `@file`). Defaults to an empty body | +| `--no-data` | Explicitly send an empty body — same as omitting `--data` | +| `--namespace ` | Action namespace (default: `Microsoft.NAV`) | +| `--out ` | Decode the action's base64 return value and write the raw bytes here | +| `--overwrite` | Replace an existing `--out` file (refused by default) | +| `--result-out ` | Write the JSON result envelope to this path (atomic) | +| `--result-fd ` | Write the JSON result envelope to this file descriptor | +| `--idempotency-key ` | Opaque token forwarded as the `Idempotency-Key` header | +| `--yes` | Skip the read-only-profile warning prompt | + +```bash +bcli action examples 42 archive +bcli action documents 42 renderPdf --out document.pdf +``` + +**`--out` and `--result-out` write different things.** `--out` writes the +action's *decoded payload bytes* — the PDF or export the action returned as +base64 in its `value` property. `--result-out` writes the *JSON result +envelope* describing the invocation (status, exit code, correlation id). They +compose; neither implies the other. + +The destination is checked before the POST is sent, so an unwritable path fails +without invoking the action. An action that returns 204 No Content has no +payload to write, and `--out` reports that rather than creating an empty file. + --- ## post diff --git a/docs/querying.md b/docs/querying.md index 21602b9..e4a5a39 100644 --- a/docs/querying.md +++ b/docs/querying.md @@ -103,6 +103,44 @@ bcli -f json -q get customers --top 100 | jq '.[] | select(.city == "Chicago") | bcli -f csv -q get items --select number,displayName,unitPrice --all > items.csv ``` +## Downloading Media Streams (PDFs) + +Some records carry a binary attachment — a scanned invoice, a rendered +document, an image. BC advertises those as `@odata.mediaReadLink` +annotations on the record, and `--out` streams the bytes to a file instead of +printing records. + +It takes two steps, because a media download addresses exactly one record: + +```bash +# 1. Find the record's systemId. +bcli get incomingDocuments --filter "description eq 'March invoice'" --top 1 -f json + +# 2. Download its media stream. +bcli get incomingDocuments --out invoice.pdf +# ✓ Wrote 48,215 bytes to invoice.pdf (application/pdf, media field: content) +``` + +With no `--media`, the media property is auto-discovered from the record's +annotations. If the record exposes several, bcli lists them and asks you to +pick one rather than guessing: + +```bash +bcli get incomingDocuments --media attachmentContent --out invoice.pdf +``` + +Notes: + +- An existing file is never replaced without `--overwrite`, and a missing + parent directory is an error rather than an `mkdir`. +- The record is fetched through the normal endpoint resolution, so a profile + with `disable_standard_api = true` refuses a media download from an + unregistered entity exactly as it refuses a read. +- `--out` is single-record mode: it can't be combined with `--filter`, + `--select`, `--top`, `--all` and friends. Use step 1 for those. +- For a bound action that *returns* a base64 payload, the equivalent flag is + `bcli action ... --out` (see the command reference). + ## Context Banner By default, bcli shows the active profile, environment, and company before output: diff --git a/pyproject.toml b/pyproject.toml index ecd0b38..33fb0e5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,7 +9,7 @@ build-backend = "hatchling.build" # installed CLI binary (`bcli`) are unaffected — only `pip install` / # `uv tool install` use this name. name = "bc-cli" -version = "0.7.0" +version = "0.8.0" description = "Python SDK and CLI for Microsoft Dynamics 365 Business Central APIs" readme = "README.md" license = "Apache-2.0" diff --git a/uv.lock b/uv.lock index e13a74a..b57d4a2 100644 --- a/uv.lock +++ b/uv.lock @@ -321,7 +321,7 @@ wheels = [ [[package]] name = "bc-cli" -version = "0.7.0" +version = "0.8.0" source = { editable = "." } dependencies = [ { name = "httpx" }, From 08761b07c259a3248a2f050f4850d58889f599f9 Mon Sep 17 00:00:00 2001 From: igor-ctrl Date: Mon, 10 Aug 2026 12:51:28 -0500 Subject: [PATCH 5/5] fix(security): harden --out media path (https-only, encode --media, no-replace commit) Three hardenings from a pre-merge security review of the --out feature: 1. is_bc_origin (and the ETL inline copy) now require https. Accepting http for an allowlisted host would have attached the bearer token to a cleartext request if a tampered @odata.mediaReadLink/nextLink used http://. Tightens every caller of the origin guard, not just --out. 2. get_media percent-encodes an explicit --media field as a single path component. validate_record_key blocks raw separators but accepted percent escapes, so '..%2F..%2Fapi%2Fv2.0%2F...' could splice encoded traversal into the token-bearing fallback URL for a gateway that decodes %2F before routing. quote(safe="") turns %2F into %252F. 3. The --out no-overwrite promise is enforced at commit, not only at pre-flight. The existence check and the os.replace publish resolved the parent twice, so a parent-directory symlink swapped in between could clobber a same-named file without --overwrite. download() and atomic_write_bytes() gain an overwrite flag; when false they publish with os.link (no-replace), which fails if the destination appeared after the check. SDK callers keep overwrite=True; the CLI threads its real --overwrite flag through. Regression tests cover each: http BC URL rejected before the token, %2F in --media neutralized, and a parent-symlink swap between pre-flight and commit refused for both the media download and the decoded action payload. Full suite 1151 passed. --- CHANGELOG.md | 30 ++++++++ src/bcli/_url.py | 5 +- src/bcli/client/_async.py | 13 +++- src/bcli/client/_transport.py | 20 +++++- src/bcli/etl/_client.py | 5 +- src/bcli_cli/_out_path.py | 21 ++++-- src/bcli_cli/commands/action_cmd.py | 6 +- src/bcli_cli/commands/get_cmd.py | 5 ++ tests/test_cli/test_out_path.py | 42 +++++++++++ tests/test_client/test_download_media.py | 92 ++++++++++++++++++++++++ tests/test_url/test_origin_allowlist.py | 18 +++++ 11 files changed, 243 insertions(+), 14 deletions(-) create mode 100644 tests/test_cli/test_out_path.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ba743ec..6986983 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 changed without breaking those consumers silently. Same reasoning as the `bcli.queries` extraction in 0.7.0. +### Security + +Three hardenings from a security review of the `--out` media path, before it +ships: + +- **The BC-origin guard now requires `https`.** `is_bc_origin` (and the ETL + layer's inline copy) previously accepted `http` for an allowlisted host, so a + tampered `@odata.mediaReadLink` or `@odata.nextLink` of + `http://api.businesscentral.dynamics.com/…` would have received the bearer + token over cleartext. A credential-bearing request must never be plaintext; + BC only ever serves `https`. This tightens every caller of the guard — the + media download and the paginator alike — not just `--out`. +- **`--media` can no longer smuggle percent-encoded path separators.** + `validate_record_key` rejects raw `/ \ ? #` but accepted percent escapes, so a + field like `..%2F..%2Fapi%2Fv2.0%2F…` could splice encoded traversal into the + token-bearing fallback URL for any gateway that decodes `%2F` before routing. + The field is now percent-encoded as a single path component at construction + (`%2F` becomes `%252F`), so no decodable separator survives. (`disable_standard_api` + was never the real boundary — the BC permission set is — but the client should + not construct a URL it did not intend.) +- **`--out`'s no-overwrite promise is enforced at the commit, not only at + pre-flight.** The existence check and the `os.replace` publish resolved the + parent path twice, so a local attacker who swapped the output directory for a + symlink in between could overwrite a same-named victim file without + `--overwrite`. When overwrite is not requested, the download and the decoded + action payload now publish with a no-replace primitive (`os.link`) that fails + if the destination appeared after the check. The SDK's `get_media` / + `download` keep `overwrite=True` as their default for direct callers; only the + CLI opts into the strict path. + ## [0.7.0] - 2026-08-04 ### Added diff --git a/src/bcli/_url.py b/src/bcli/_url.py index 0df07b4..1425a20 100644 --- a/src/bcli/_url.py +++ b/src/bcli/_url.py @@ -181,7 +181,10 @@ def is_bc_origin(url: str) -> bool: if not parsed.scheme: # Relative URL — caller will resolve it against the BC base URL. return True - if parsed.scheme not in ("http", "https"): + if parsed.scheme != "https": + # A bearer token must never ride a cleartext request. BC always serves + # https, so an absolute http:// URL is tampering or misconfiguration — + # refuse it before the token is attached. return False host = (parsed.hostname or "").lower() if not host: diff --git a/src/bcli/client/_async.py b/src/bcli/client/_async.py index 2914380..a53050f 100644 --- a/src/bcli/client/_async.py +++ b/src/bcli/client/_async.py @@ -5,6 +5,7 @@ import re from pathlib import Path from typing import Any +from urllib.parse import quote from bcli._url import build_companies_url, build_url, validate_record_key from bcli.auth._base import AuthProvider @@ -471,6 +472,7 @@ async def get_media( publisher: str | None = None, group: str | None = None, version: str | None = None, + overwrite: bool = True, ) -> dict[str, Any]: """Download a record's media stream (PDF, image, blob) to ``dest``. @@ -531,7 +533,12 @@ async def get_media( field = media_field link = record.get(f"{media_field}@odata.mediaReadLink") if not isinstance(link, str) or not link: - link = f"{record_url}/{media_field}" + # Percent-encode the field as a single path component. + # validate_record_key blocks *raw* separators but accepts percent + # escapes, so '..%2F..%2Fapi%2Fv2.0%2F...' would otherwise splice + # encoded traversal into the token-bearing URL for any server that + # decodes %2F before routing. quote(safe="") turns %2F into %252F. + link = f"{record_url}/{quote(media_field, safe='')}" elif len(discovered) == 1: field = discovered[0] link = record[f"{field}@odata.mediaReadLink"] @@ -552,7 +559,9 @@ async def get_media( dest_path = Path(dest).expanduser() outcome = await transport.download( - link, dest_path, log_context={"endpoint": entity_set_name}, + link, dest_path, + log_context={"endpoint": entity_set_name}, + overwrite=overwrite, ) return { diff --git a/src/bcli/client/_transport.py b/src/bcli/client/_transport.py index 13e9c7c..0d390e3 100644 --- a/src/bcli/client/_transport.py +++ b/src/bcli/client/_transport.py @@ -337,6 +337,7 @@ async def download( *, params: dict[str, str] | None = None, log_context: dict[str, str] | None = None, + overwrite: bool = True, ) -> dict[str, Any]: """Stream a GET response body to ``dest`` and return what was written. @@ -494,10 +495,25 @@ async def download( f"Download failed after {self._max_retries + 1} attempts", ) from last_error - os.replace(tmp_path, dest) + if overwrite: + os.replace(tmp_path, dest) + else: + # No-replace publication. os.link raises FileExistsError if the + # destination appeared after the CLI's pre-flight check — e.g. a + # parent directory swapped to a symlink mid-download — so a race + # can't silently clobber a file the user never agreed to replace. + # The finally below removes the now-linked temp file. + try: + os.link(tmp_path, dest) + except FileExistsError: + raise FileExistsError( + f"Refusing to overwrite {dest}: it appeared after the " + f"pre-flight check. Re-run with --overwrite to replace it." + ) from None return result finally: - # No-op once os.replace has moved it; cleans up every failure path. + # No-op once os.replace has moved it; on the no-replace path this + # removes the source of the hardlink. Cleans up every failure path. tmp_path.unlink(missing_ok=True) async def post( diff --git a/src/bcli/etl/_client.py b/src/bcli/etl/_client.py index 6cf0522..b9ae652 100644 --- a/src/bcli/etl/_client.py +++ b/src/bcli/etl/_client.py @@ -40,8 +40,9 @@ def _assert_bc_origin(url: str) -> None: parsed = urlparse(url) if not parsed.scheme: return # relative URL, joined to base by httpx - if parsed.scheme not in ("http", "https"): - raise ValueError(f"Refusing non-HTTP(S) URL with auth: {url!r}") + if parsed.scheme != "https": + # Bearer tokens never ride cleartext; BC always serves https. + raise ValueError(f"Refusing non-HTTPS URL with auth: {url!r}") host = (parsed.hostname or "").lower() if not host: raise ValueError(f"Refusing URL with no host: {url!r}") diff --git a/src/bcli_cli/_out_path.py b/src/bcli_cli/_out_path.py index 2e185db..34366e3 100644 --- a/src/bcli_cli/_out_path.py +++ b/src/bcli_cli/_out_path.py @@ -50,17 +50,30 @@ def prepare_out_path(path: Path, *, overwrite: bool) -> Path: return dest -def atomic_write_bytes(dest: Path, raw: bytes) -> None: - """Write ``raw`` to ``dest`` via a temp file + :func:`os.replace`. +def atomic_write_bytes(dest: Path, raw: bytes, *, overwrite: bool = True) -> None: + """Write ``raw`` to ``dest`` via a temp file + atomic publish. Same discipline as ``BCTransport.download``: a reader never sees a - half-written file, and a failed write leaves no part file behind. + half-written file, and a failed write leaves no part file behind. When + ``overwrite`` is False the publish is no-replace (``os.link``), so a + destination that appeared after the caller's pre-flight check — e.g. a + parent directory swapped to a symlink — is refused rather than clobbered. """ fd, tmp_name = tempfile.mkstemp(prefix=dest.name + ".", dir=str(dest.parent)) try: with os.fdopen(fd, "wb") as f: f.write(raw) - os.replace(tmp_name, dest) + if overwrite: + os.replace(tmp_name, dest) + else: + try: + os.link(tmp_name, dest) + except FileExistsError: + raise FileExistsError( + f"Refusing to overwrite {dest}: it appeared after the " + f"pre-flight check. Re-run with --overwrite to replace it." + ) from None + os.unlink(tmp_name) except Exception: try: os.unlink(tmp_name) diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py index a70c6d7..33be037 100644 --- a/src/bcli_cli/commands/action_cmd.py +++ b/src/bcli_cli/commands/action_cmd.py @@ -200,7 +200,7 @@ def action_command( # into bytes means the caller didn't get what they asked for, # and the envelope has to say failed. The POST already # happened either way — that's what the envelope records. - written = _write_decoded_payload(result, dest) + written = _write_decoded_payload(result, dest, overwrite=overwrite) cap.emit_success() @@ -286,10 +286,10 @@ def _decode_base64_payload(result: dict | str) -> bytes: ) from e -def _write_decoded_payload(result: dict | str, dest: Path) -> int: +def _write_decoded_payload(result: dict | str, dest: Path, *, overwrite: bool) -> int: """Decode the action's payload onto ``dest`` atomically; return byte count.""" raw = _decode_base64_payload(result) - atomic_write_bytes(dest, raw) + atomic_write_bytes(dest, raw, overwrite=overwrite) return len(raw) diff --git a/src/bcli_cli/commands/get_cmd.py b/src/bcli_cli/commands/get_cmd.py index a05c895..445eb1b 100644 --- a/src/bcli_cli/commands/get_cmd.py +++ b/src/bcli_cli/commands/get_cmd.py @@ -77,6 +77,7 @@ def get_command( _run_media_download( endpoint, record_id or "", dest, media, publisher=publisher, group=group, version=version, + overwrite=overwrite, ) return @@ -289,6 +290,7 @@ def _run_media_download( publisher: str | None, group: str | None, version: str | None, + overwrite: bool, ) -> None: """Execute the ``--out`` branch: fetch the record, stream its media to ``dest``.""" if state.dry_run: @@ -311,6 +313,7 @@ def _run_media_download( _execute_get_media( endpoint, record_id, dest, media_field, publisher=publisher, group=group, version=version, + overwrite=overwrite, ) ) latency_ms = (_time.monotonic() - started) * 1000.0 @@ -351,12 +354,14 @@ async def _execute_get_media( publisher: str | None = None, group: str | None = None, version: str | None = None, + overwrite: bool = False, ) -> dict: async with state.make_async_client() as client: return await client.get_media( endpoint, record_id, dest, media_field=media_field, publisher=publisher, group=group, version=version, + overwrite=overwrite, ) diff --git a/tests/test_cli/test_out_path.py b/tests/test_cli/test_out_path.py new file mode 100644 index 0000000..1b4bbd0 --- /dev/null +++ b/tests/test_cli/test_out_path.py @@ -0,0 +1,42 @@ +"""Tests for the shared --out destination helper. + +``atomic_write_bytes`` publishes the decoded payload for ``bcli action --out``. +Its ``overwrite`` policy mirrors ``BCTransport.download``: a no-replace commit +so a destination that appears after the CLI's pre-flight check can't be +clobbered (#21 review, VULN-0002). +""" + +from __future__ import annotations + +import pytest + +from bcli_cli._out_path import atomic_write_bytes + + +def test_overwrite_false_refuses_an_existing_destination(tmp_path): + dest = tmp_path / "out.bin" + dest.write_bytes(b"original") + + with pytest.raises(FileExistsError, match="pre-flight"): + atomic_write_bytes(dest, b"new payload", overwrite=False) + + assert dest.read_bytes() == b"original" # untouched + assert list(tmp_path.glob("out.bin.*")) == [] # no temp litter + + +def test_overwrite_false_writes_a_new_destination(tmp_path): + dest = tmp_path / "fresh.bin" + + atomic_write_bytes(dest, b"payload", overwrite=False) + + assert dest.read_bytes() == b"payload" + assert list(tmp_path.glob("fresh.bin.*")) == [] + + +def test_overwrite_true_replaces(tmp_path): + dest = tmp_path / "out.bin" + dest.write_bytes(b"original") + + atomic_write_bytes(dest, b"replacement", overwrite=True) + + assert dest.read_bytes() == b"replacement" diff --git a/tests/test_client/test_download_media.py b/tests/test_client/test_download_media.py index 0aab222..3c8550a 100644 --- a/tests/test_client/test_download_media.py +++ b/tests/test_client/test_download_media.py @@ -220,6 +220,27 @@ async def test_traversal_in_media_field_is_rejected_before_any_http( assert len(httpx_mock.get_requests()) == 1 assert not list(tmp_path.iterdir()) + async def test_percent_encoded_field_cannot_smuggle_traversal( + self, client, tmp_path, httpx_mock, + ): + """validate_record_key blocks *raw* separators but accepts percent + escapes; a value like '..%2F..%2Fapi%2Fv2.0%2F...users' would otherwise + splice a decodable slash into the token-bearing URL for any server that + decodes %2F before routing. The field is encoded as one path component, + so %2F becomes %252F and no decodable separator survives (#21 review).""" + httpx_mock.add_response(json=_record()) + httpx_mock.add_response(content=PDF_BYTES) + + payload = "..%2F..%2Fapi%2Fv2.0%2Fcompanies(x)%2Fusers" + await client.get_media( + "incomingDocuments", RECORD_ID, tmp_path / "x.bin", media_field=payload, + ) + + media_url = str(httpx_mock.get_requests()[1].url) + assert "%252F" in media_url # the payload's % was itself encoded + assert "%2F" not in media_url # so no decodable slash remains + assert "/users" not in media_url # traversal target never resolves + class TestOriginGuard: async def test_off_origin_media_link_is_refused(self, client, tmp_path, httpx_mock): @@ -236,6 +257,77 @@ async def test_off_origin_media_link_is_refused(self, client, tmp_path, httpx_mo assert not list(tmp_path.iterdir()) +class TestNoReplaceCommit: + """#21 review (VULN-0002): the no-overwrite policy is enforced at the commit, + not only at pre-flight, so a destination that appears after the check — e.g. a + parent directory swapped to a symlink mid-download — is refused, not clobbered. + """ + + async def test_overwrite_false_refuses_a_destination_present_at_commit( + self, client, tmp_path, httpx_mock, + ): + httpx_mock.add_response(json=_record(**{"content@odata.mediaReadLink": MEDIA_URL})) + httpx_mock.add_response(content=PDF_BYTES) + + dest = tmp_path / "victim.pdf" + dest.write_bytes(b"original") + + with pytest.raises(FileExistsError, match="pre-flight"): + await client.get_media("incomingDocuments", RECORD_ID, dest, overwrite=False) + + assert dest.read_bytes() == b"original" # never clobbered + assert _part_files(tmp_path) == [] # no litter + + async def test_overwrite_true_still_replaces(self, client, tmp_path, httpx_mock): + httpx_mock.add_response(json=_record(**{"content@odata.mediaReadLink": MEDIA_URL})) + httpx_mock.add_response(content=PDF_BYTES) + + dest = tmp_path / "victim.pdf" + dest.write_bytes(b"original") + + await client.get_media("incomingDocuments", RECORD_ID, dest, overwrite=True) + + assert dest.read_bytes() == PDF_BYTES + + async def test_parent_symlink_swap_between_preflight_and_commit_is_refused( + self, client, tmp_path, httpx_mock, + ): + """Reproduces the strix PoC: after the destination is chosen, an attacker + renames the output parent and drops a symlink to a directory that holds a + same-named victim file. The no-replace commit must refuse.""" + import tempfile as _tempfile + from unittest.mock import patch + + selected = tmp_path / "selected" + victim = tmp_path / "victim" + selected.mkdir() + victim.mkdir() + dest = selected / "invoice.pdf" + victim_file = victim / "invoice.pdf" + victim_file.write_bytes(b"original-victim") + + httpx_mock.add_response(json=_record(**{"content@odata.mediaReadLink": MEDIA_URL})) + httpx_mock.add_response(content=PDF_BYTES) + + real_ntf = _tempfile.NamedTemporaryFile + + def swap_then_open(*args, **kwargs): + # The parent that passed pre-flight is swapped for a symlink to the + # victim directory, right before the temp file is created in it. + selected.rename(tmp_path / "selected-orig") + selected.symlink_to(victim, target_is_directory=True) + return real_ntf(*args, **kwargs) + + with patch( + "bcli.client._transport.tempfile.NamedTemporaryFile", swap_then_open, + ), pytest.raises(FileExistsError): + await client.get_media( + "incomingDocuments", RECORD_ID, dest, overwrite=False, + ) + + assert victim_file.read_bytes() == b"original-victim" # NOT clobbered + + class TestRetryAndCleanup: async def test_retry_after_partial_stream_writes_the_body_once( self, tmp_path, httpx_mock, diff --git a/tests/test_url/test_origin_allowlist.py b/tests/test_url/test_origin_allowlist.py index 3a6469d..90b9f8b 100644 --- a/tests/test_url/test_origin_allowlist.py +++ b/tests/test_url/test_origin_allowlist.py @@ -56,6 +56,9 @@ def test_is_bc_origin_accepts_legitimate_urls(url): ("https://attacker.example/v2.0/whatever", "different domain"), ("https://localhost:1234/leak", "localhost is not a BC host"), ("http://attacker.example/v2.0/whatever", "wrong scheme + wrong host"), + # Right host, wrong scheme: a bearer token must never ride cleartext. + (f"http://api.businesscentral.dynamics.com/v2.0/Production/api/v2.0/companies({_COMPANY_ID})/customers", + "http scheme rejected even for an allowed BC host"), # Non-HTTP schemes shouldn't slip through. ("file:///etc/passwd", "file scheme rejected"), ("javascript:alert(1)", "non-http scheme rejected"), @@ -68,6 +71,19 @@ def test_is_bc_origin_rejects_malicious_urls(url, reason): assert_bc_origin(url) +def test_https_required_for_bc_host(): + """The same BC URL is accepted over https and refused over http — a + bearer token must never be attached to a cleartext request (#21 review).""" + https_url = ( + f"https://api.businesscentral.dynamics.com/v2.0/Production/api/v2.0/companies({_COMPANY_ID})/customers" + ) + http_url = https_url.replace("https://", "http://", 1) + assert is_bc_origin(https_url) is True + assert is_bc_origin(http_url) is False + with pytest.raises(ValueError, match="off-origin"): + assert_bc_origin(http_url) + + def test_assert_message_includes_rejected_url(): """The error explicitly names the URL so operators can audit it.""" with pytest.raises(ValueError, match="attacker.example"): @@ -106,6 +122,8 @@ def test_etl_inline_check_matches_canonical(): "https://attacker.example/leak", "https://evilbusinesscentral.dynamics.com.attacker.example/leak", "file:///etc/passwd", + # https-only: cleartext to a BC host is still refused. + "http://api.businesscentral.dynamics.com/leak", ]: with pytest.raises(ValueError): etl_assert(bad)