diff --git a/CHANGELOG.md b/CHANGELOG.md index a1c0b09..6986983 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,90 @@ 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. + +### 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/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/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 3cde1b5..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 @@ -47,6 +48,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 +462,116 @@ 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, + overwrite: bool = True, + ) -> 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: + # 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"] + 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}, + overwrite=overwrite, + ) + + 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..0d390e3 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,192 @@ 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, + overwrite: bool = True, + ) -> 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 + + 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; 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( self, url: str, 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/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/src/bcli_cli/_out_path.py b/src/bcli_cli/_out_path.py new file mode 100644 index 0000000..34366e3 --- /dev/null +++ b/src/bcli_cli/_out_path.py @@ -0,0 +1,82 @@ +"""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, *, 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. 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) + 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) + except OSError: + pass + raise diff --git a/src/bcli_cli/commands/action_cmd.py b/src/bcli_cli/commands/action_cmd.py index bc40cb4..33be037 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, overwrite=overwrite) + 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, *, overwrite: bool) -> int: + """Decode the action's payload onto ``dest`` atomically; return byte count.""" + raw = _decode_base64_payload(result) + atomic_write_bytes(dest, raw, overwrite=overwrite) + 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..445eb1b 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,15 @@ 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, + overwrite=overwrite, + ) + return + if filter: _check_filter_fields(endpoint, filter) @@ -204,6 +237,134 @@ 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, + overwrite: bool, +) -> 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, + overwrite=overwrite, + ) + ) + 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, + 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, + ) + + 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_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 new file mode 100644 index 0000000..3c8550a --- /dev/null +++ b/tests/test_client/test_download_media.py @@ -0,0 +1,379 @@ +"""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()) + + 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): + """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 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, + ): + """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()) == [] 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 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__ 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) 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" },