diff --git a/.gitignore b/.gitignore index 53b4200..3ca9843 100644 --- a/.gitignore +++ b/.gitignore @@ -66,3 +66,6 @@ AGENTS.md # Local harness CI workflows depend on the untracked .harness/ toolchain .github/workflows/harness.yml .github/workflows/eval-nightly.yml + +# local helper scripts +*.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index c1f7239..d5e79a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,220 @@ All notable changes to this project will be documented in this file. +## [1.3.4] - 2026-07-26 + +### Fixed +- **`execute_approved_write` error envelopes missing `result` key** — + `_execute_approved_write_gated` (`tools_write.py`) now routes every + return path through a new `_normalize_write_response` helper, which + injects `result: None` when the envelope does not already carry the + key. Prevents `Output validation error: 'result' is a required + property` on FastMCP transports whose inferred `outputSchema` for + `execute_approved_write` treats the success-path `result` field as + required. The five early-return error paths inside the gate + (token mismatch, validation record missing, payload mismatch, + missing `confirm`, writes disabled) and the `except Exception` arm + all now carry `result: None`. The success path is unchanged + (envelope already populated `result`). No change to `success` / + `error` semantics — `result: null` simply signals "no Odoo return + value because we never reached the execute call". + +### Added +- **`tests/test_batch_write.py`** — 4 new regression tests covering: + - `_normalize_write_response` injects `result: None` on error envelopes + and is a no-op (same identity) when `result` is already present. + - The helper is defensive against non-dict input (returns unchanged). + - End-to-end: a token-mismatch execute call returns + `{"success": false, "result": null, "error": ...}`. + +### Changed +- No breaking changes. All 954 tests pass (was 946 in 1.3.2; +6 for + the v1.3.3 nested-`record_ids` regression tests, +4 for the v1.3.4 + `_normalize_write_response` tests, −2 net for refactors + unrelated to these releases). + +## [1.3.3] - 2026-07-26 + +### Fixed +- **Write-approval token mismatch when caller-side `record_ids` arrives + nested or differently structured** — `verify_write_approval` + (`agent_tools.py`) and `write_approval_payload` (`server_core.py`) + now route `record_ids` through a new `_normalized_record_ids` helper + that flattens any nesting and casts every element to `int`, mirroring + the normalization `build_write_preview_report` already performs on + the preview side. Previously, a transport- or framework-induced + change in list shape (e.g. nested `[[3598, 3594, ...]]` arriving at + the execute call instead of flat `[3598, 3594, ...]` from the + preview) produced a different SHA-256 than the stored token, surfacing + as `"approval token does not match the canonical payload; re-run + preview_write and validate_write"`. The new helper also coerces + digit-strings (some JSON transports downcast ints) and silently drops + non-digit entries instead of coercing them to `0` (which would have + silently targeted record id 0 = the Odoo super-user). Booleans are + skipped explicitly because `bool` is an `int` subclass in Python. + Symmetrical to the existing `_normalize_numbers` int/float drift fix + shipped in 1.2.1. + +### Added +- **`tests/test_agent_tools.py`** — 6 new regression tests covering: + - `_normalized_record_ids` flattens nested / triple-nested / mixed + type input and drops non-digit entries. + - `verify_write_approval` accepts nested, triple-nested, and + digit-string `record_ids` against a flat-built token (the original + bug shape). + - `write_approval_payload` normalizes nested input to a flat + `list[int]` for the payload-equality check in + `_execute_approved_write_gated`. + +### Changed +- No breaking changes. All 108 tests in `tests/test_agent_tools.py` + pass; no other public surface affected. + +## [1.3.2] - 2026-07-21 + +### Fixed +- **Friendly error envelope on Pydantic argument validation** — when + FastMCP's binder rejected a tool call (e.g. `measures=""` instead of + a list), the server now returns the same `{"success": False, "tool": + …, "error": "Invalid input: measures: …"}` envelope the rest of the + error path uses, instead of leaking a raw `ValidationError` stack + trace. New `safe_tool_call` decorator in `error_handling.py` + applied to `aggregate_records` and `search_records`. + +- **`normalize_domain_input` no longer silently returns `[]` for garbage + strings** — invalid JSON / Python literal input now raises + `ValueError` with a message that names both legal forms (JSON list + and Python literal), so agents see a clean error path instead of + silently querying the wrong record set. Tests updated and + `test_tool_helpers` coverage extended. + +- **`aggregate_records` description now warns about `__count`** — + `__count` is the auto-returned pseudo-column from + `read_group`/`formatted_read_group`; passing it explicitly as a + measure produces `Invalid field '__count'`. Description and + `list_instances` hint tightened on `aggregate_records` and + `search_records`. + +- **`RuntimeWarning` removed from the CLI test suite** — + `runpy.run_module("odoo_mcp.__main__", run_name="__main__")` + triggered CPython's `RuntimeWarning` whenever `odoo_mcp.__main__` + was already in `sys.modules`. Replaced with a direct invocation of + the entry-point expression (`sys.exit(main())`) — same contract + tested, no `sys.modules` pollution, no warning. + +### Changed +- No breaking changes to public API. All 946 tests pass. + +## [1.3.1] - 2026-07-21 + +### Fixed +- **JSON-Schema wrapping bug for list/dict tool parameters** — tool + parameters declared as `Optional[List[…]]`, `List[…]`, or + `Optional[Dict[…]]` without an `Annotated[…, Field(description=…)]` + wrapper were emitted into the JSON schema as a `{"item": […]}` object + wrapper instead of a proper array. Clients (MCP inspector, Cline, + Claude Desktop) then rejected the call with a `list_type` validation + error: `Input should be a valid list [type=list_type, input_value= + {'item': [...]}, input_type=dict]`. The fix wraps every affected + parameter in `Annotated[…, Field(description=…)]` so Pydantic emits + a clean `{"type": "array", "items": …}` (or `{"type": "object"}`) + schema. The `domain`, `fields`, `field_names`, `group_by`, `measures`, + `checks`, `key_fields`, `args`, `kwargs`, `values`, `values_list`, + `record_ids`, `context`, `partner_ids`, `attachment_ids`, + `available_models`, `available_fields`, `installed_modules`, + `conditions`, `addons_paths`, and `instances` parameters are now all + array-safe. Tools fixed: `get_model_fields`, `search_records`, + `read_record`, `aggregate_records`, `index_knowledge`, + `data_quality_report`, `generate_json2_payload`, + `inspect_model_relationships`, `analyze_upgrade_log`, + `fit_gap_report`, `scan_addons_source`, `build_domain`, + `preview_write`, `validate_write`, `chatter_post`, + `search_across_instances`, `aggregate_across_instances`. No behavior + change at runtime — only the `tools/list` JSON-Schema surface is + corrected, so the existing test suite and all call sites are + unchanged. + +## [1.3.0] - 2026-07-17 + +Field-file I/O release: agents can now route single-field payloads +between Odoo and the local filesystem instead of forcing them through +the JSON-RPC envelope — useful for HTML, source code, and other long +rich-text fields where LLMs routinely mistransform JSON-escaped +content. The destructive write is gated by the same preview/execute + +two-phase-confirmation flow used by `chatter_post` and +`execute_approved_write`. 931 tests. + +### Added +- **`read_field_to_file` (read domain)** — read a single field's value + straight to a local file. The file content never re-enters the agent's + context window; only the SHA-256 fingerprint, byte count, and + encoding cross the wire. Honors the field ACL (redacted values become + a `[REDACTED by field ACL]` placeholder, response flags it via + `field_was_redacted`). Text fields default to UTF-8; binary fields + default to base64 (caller can override). Symlink-safe, refuses + overwrite, absolute-path-only. +- **`write_field_from_file` (write domain)** — write a field's value + from a local file via the standard two-phase preview/execute flow. + Preview returns an approval token carrying only the file's + SHA-256 + size fingerprint (no content); execute re-reads the file, + re-checks the hash (catches tamper and TOCTOU), fetches live + `fields_get` metadata (refuses `readonly=True`), then routes through + the same `confirm=true` + `ODOO_MCP_ENABLE_WRITES=1` gates as every + other write. Binary round-trip via base64; otherwise UTF-8. +- **`ODOO_MCP_FIELD_FILE_ROOTS` allow-list** — colon-separated (or + `os.pathsep`-separated) absolute directories the two tools are + permitted to read from / write into. Hardened pattern lifted from + `ODOO_MCP_ATTACHMENT_UPLOAD_ROOTS`: every path is `Path.resolve()`d + before the containment check, so `..` traversal and symlink escapes + cannot reach outside the operator's allow-list. Per-call override via + the `file_root` argument on both tools. +- **Fail-closed default with actionable error** — with no env var and + no per-call override, both tools reject every call. The error names + the env var, lists platform-specific safe defaults (`~/.cache/odoo-mcp/field-files` + on Linux/macOS, `%LOCALAPPDATA%\odoo-mcp\Cache\field-files` on + Windows), and **explicitly warns against `/tmp`** (typically + world-readable — long field payloads would leak to other local + users / processes). +- **Runtime posture in `health_check.runtime.field_file_roots`** — + non-secret summary of the configured roots (env var name + count + + resolved paths), so an operator can audit what is wired up without + reading server logs. +- **`docs/field-file-io.md`** — exhaustive operator guide (config, + examples for HTML comments, binary attachments, and long internal + notes; full threat-model table). + +### Security +- TOCTOU on read closed via `O_NOFOLLOW` on the create fd plus + `O_CREAT | O_EXCL` (no overwrite, no symlink swap race). +- TOCTOU on write closed via `O_NOFOLLOW` on the input fd: size cap + and SHA-256 are derived from the bytes read through that same fd, so + a swap between the path check and the read is rejected. +- File tamper between preview and execute caught by the + re-read + SHA-256 re-check on the execute call (the approval token + itself includes the expected hash, so any divergence fires either + the explicit hash check or the token-mismatch check, whichever runs + first — both are equally valid rejections). +- Field ACL integration for read: redacted values never appear in + file content, response payload, or audit-log entry. + +### Fixed +- **`write_field_from_file` `execute_kw` argument shape** — the + initial implementation called + `odoo.execute_method(model, "write", [[ids], {vals}])`, which + `OdooClient.execute_method(self, model, method, *args, **kwargs)` + forwarded as `execute_kw("project.task", "write", [[ids], {vals}])` + — a **single** positional arg. Odoo's XML-RPC dispatcher then unpacked + it as `ProjectTask.write(*[[ids], {vals}])` → `write([ids], {vals})`, + and Odoo faulted with + `TypeError: ProjectTask.write() missing 1 required positional argument: 'vals'`. + Now called as `odoo.execute_method(model, "write", [ids], {vals})` so + ids + vals arrive at Odoo as **two separate** positional arguments + (mirrors `execute_approved_write`, which always splatted `*args`). + New regression test + `test_write_field_from_file_does_not_pack_args_into_single_list` + (plus the `execute_method` test stub now records the unpacked + `*args` form, matching `OdooClient`'s signature) so a re-introduction + of the nested-list form fires immediately. ## [1.2.3] - 2026-07-22 Metadata-only ownership refresh after the repository transfer to `erpipe-org`. diff --git a/README.md b/README.md index ac6fce7..9bd8fef 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,7 @@ Once configured (see [Setup](#setup)), ask your agent things like: | Capability | What it gives you | | --- | --- | -| 41 MCP tools | Read records and attachments, aggregate server-side, post chatter, inspect schema, build domains, scan addons, diagnose calls and upgrade logs, check data quality, access rules, resolve model renames, validate writes, and fan out across instances. | +| 43 MCP tools | Read records and attachments, aggregate server-side, post chatter, inspect schema, build domains, scan addons, diagnose calls and upgrade logs, check data quality, access rules, resolve model renames, validate writes, stream long fields through the filesystem, and fan out across instances. | | Field-level ACL | Opt-in per-instance, per-model field allow/deny enforced on every read path (records, aggregates, knowledge index, resources). First open-source Odoo MCP with it. See [docs/field-acl.md](docs/field-acl.md). | | Cross-instance queries | Read-only fan-out across many client DBs with merged, attributed, partial-failure-tolerant results — no warehouse, no sync. See [docs/partner-playbook.md](docs/partner-playbook.md). | | Workflow prompts | 11 prompts including 6 end-to-end business workflows (invoice approval, PO match, onboarding, expense review, month-end close, pre-migration data quality) that route writes through the gate. | @@ -79,6 +79,7 @@ Once configured (see [Setup](#setup)), ask your agent things like: | Smart field selection | `search_records` and `read_record` curate business-relevant fields when no `fields` argument is supplied — drops audit, message, binary, and unstored compute noise. Pass `fields=["*"]` to opt out. | | Server-side aggregation | `aggregate_records` pushes groupby/sum/count/avg into Postgres via `formatted_read_group` (Odoo 19+) or `read_group` (16-18). | | Chatter integration | `chatter_post` adds messages to any `mail.thread` record under the same approval-token gate as writes — or directly via `MCP_CHATTER_DIRECT=1`. | +| File I/O for long fields | `read_field_to_file` and `write_field_from_file` route HTML/Code/rich-text payloads through the filesystem (escaping-safe, no context-window blow-up). Gated by an allow-listed root (`ODOO_MCP_FIELD_FILE_ROOTS`) — see [docs/field-file-io.md](docs/field-file-io.md). | | Locale plumbing | `ODOO_LOCALE` injects `context.lang` automatically on every Odoo call (caller can override). | | Structured logging | JSON formatter and rotating file handler via `ODOO_MCP_LOG_LEVEL`, `ODOO_MCP_LOG_JSON`, `ODOO_MCP_LOG_FILE`. | | Safe writes | Direct `create`, `write`, and `unlink` are blocked; approved writes require live metadata, a same-session token, explicit confirmation, and an env gate. | @@ -266,6 +267,8 @@ Optional environment variables: | `ODOO_MCP_MAX_ATTACHMENT_BYTES` | `1048576` | Download cap for `read_attachment` content (hard cap 16 MiB). | | `ODOO_MCP_ATTACHMENT_UPLOAD_ROOTS` | unset | Colon-separated local directories `validate_write` may read `_from_path` uploads from (mirrors `ODOO_ADDONS_PATHS`). Required — fails closed with no roots configured. | | `ODOO_MCP_MAX_ATTACHMENT_UPLOAD_BYTES` | `10485760` | Size cap for `_from_path` local-file uploads (hard cap 16 MiB). | +| `ODOO_MCP_FIELD_FILE_ROOTS` | unset | Colon-separated absolute directories `read_field_to_file` and `write_field_from_file` may read from / write into. Required — fails closed otherwise; never overwrites existing files. See [docs/field-file-io.md](docs/field-file-io.md). | +| `ODOO_MCP_MAX_FIELD_FILE_BYTES` | `10485760` | Size cap for `read_field_to_file`/`write_field_from_file` payloads (hard cap 16 MiB). | | `ODOO_MCP_AUTH_ISSUER_URL` | unset | OAuth 2.1: authorization server issuer. With the two vars below, the HTTP transport becomes a protected resource server (RFC 9728 metadata + bearer validation). | | `ODOO_MCP_AUTH_INTROSPECTION_URL` | unset | RFC 7662 token introspection endpoint of the authorization server. | | `ODOO_MCP_AUTH_RESOURCE_URL` | unset | Canonical URL of this MCP server (RFC 8707 audience check when the AS binds tokens). | @@ -355,9 +358,9 @@ odoo-mcp --health ## MCP Tools -41 tools grouped by use case. Each tool name is a single-purpose handle the agent can call. Tools that talk to Odoo accept an optional `instance` parameter when multiple instances are configured (see [Multiple Odoo instances](#multiple-odoo-instances)). +43 tools grouped by use case. Each tool name is a single-purpose handle the agent can call. Tools that talk to Odoo accept an optional `instance` parameter when multiple instances are configured (see [Multiple Odoo instances](#multiple-odoo-instances)). -### Read & Discover (11) +### Read & Discover (13) | Tool | Purpose | | --- | --- | @@ -372,6 +375,8 @@ odoo-mcp --health | `schema_catalog` | Build a bounded model catalog with optional field metadata. | | `build_domain` | Build and validate an Odoo domain from structured conditions. | | `read_attachment` | Read an `ir.attachment`'s metadata and size-capped base64 content (`ODOO_MCP_MAX_ATTACHMENT_BYTES`, default 1 MiB). | +| `read_field_to_file` | Stream one field's value (HTML, source, base64 binary) to a local file inside the configured `ODOO_MCP_FIELD_FILE_ROOTS` — never overwrites existing files, respects the field ACL. See [docs/field-file-io.md](docs/field-file-io.md). | +| `write_field_from_file` | Set one field from a local file's contents via the preview/execute approval gate. Token never carries file content; execute re-reads the file and re-checks the SHA-256 fingerprint before writing. | ### Write & Operate (5) diff --git a/docs/field-acl.md b/docs/field-acl.md index ef01271..6500ce6 100644 --- a/docs/field-acl.md +++ b/docs/field-acl.md @@ -62,6 +62,7 @@ unprotected. | `get_model_fields` | Denied fields are **marked** `"access": "restricted"` (not hidden) so the agent knows the field exists and can explain the redaction; `restricted_fields` listed. | | `index_knowledge` | Denied fields are excluded before BM25 indexing, so their values are never cached or searchable. | | `odoo://record/...`, `odoo://search/...` resources | Denied fields removed; `_redacted_fields` noted. | +| `read_field_to_file` | Denied fields are **not** streamed to disk. The file gets a `[REDACTED by field ACL]` placeholder and the response carries `field_was_redacted: true` plus `redacted_fields: [...]` — so the agent can neither read the value nor hallucinate it. See [field-file-io.md](field-file-io.md). | | `health_check` | `runtime.field_acl` reports whether ACL is active and how many instances have rules — never the policy contents. | `redacted_fields` notes are deliberate: the agent is told *that* fields were diff --git a/docs/field-file-io.md b/docs/field-file-io.md new file mode 100644 index 0000000..757afa6 --- /dev/null +++ b/docs/field-file-io.md @@ -0,0 +1,329 @@ +# Field file I/O (`read_field_to_file`, `write_field_from_file`) + +Two new MCP tools route single-field payloads between Odoo and the local +filesystem instead of forcing them through the JSON-RPC envelope. The +response carries only metadata (`path`, `sha256`, byte count, encoding) — +the field content stays out of the agent's context window. The agent then +reads or edits the file with its ordinary file tools. + +This solves two recurring pain points: + +| Pain | Symptom | Fix | +| --- | --- | --- | +| JSON-escape hell | Long HTML / source code / rich text round-trips through the agent context with `\\n`, double-escaped quotes, and embedded Unicode that LLMs routinely mistransform. | Field value is read/written verbatim from disk; only the SHA-256 fingerprint crosses the wire. | +| Context-window blow-up | A 50 KB product description or 30 KB e-mail body lands in every subsequent tool call's history. | The agent pulls the value once, edits it locally, and writes it back without the file content re-entering the LLM context. | + +The tools also give a write-side preview/execute flow modeled on +`chatter_post` and `execute_approved_write`, so the destructive write still +goes through an approval gate rather than executing on the first call. + +## The two tools + +### `read_field_to_file` (read domain) + +```jsonc +{ + "model": "res.partner", + "record_id": 42, + "field": "comment", + "output_path": "/srv/agent-scratch/comment-42.html", + "file_root": null, // optional per-call override (defaults to first ODOO_MCP_FIELD_FILE_ROOTS entry) + "encoding": null, // optional: "utf-8" (default for text) or "base64" (default for binary fields) + "instance": null // optional, multi-instance routing +} +``` + +The server resolves `output_path` against `file_root`, refuses to overwrite +an existing file, fetches the field via Odoo's `read`, applies the field +ACL (redacted values become a `[REDACTED by field ACL]` placeholder), and +streams the bytes to disk. The response is: + +```jsonc +{ + "success": true, + "tool": "read_field_to_file", + "model": "res.partner", + "record_id": 42, + "field": "comment", + "output_path": "/srv/agent-scratch/comment-42.html", + "file_root": "/srv/agent-scratch", + "encoding": "utf-8", + "bytes_written": 14823, + "content_sha256": "sha256:5e1c…:14823", + "field_was_redacted": false, + "redacted_fields": [], + "metadata_used": { "instance": "default", "file_root": "…", "encoding": "utf-8", "max_bytes": 10485760, "field_type": "html" } +} +``` + +The agent verifies the on-disk file with the SHA-256 fingerprint before +acting on it. + +### `write_field_from_file` (write domain) + +Two-phase preview/execute flow: + +```jsonc +// Phase 1 — preview +{ + "model": "res.partner", + "record_id": 42, + "field": "comment", + "input_path": "/srv/agent-scratch/comment-42.html" +} +// → { "mode": "preview", "approval": { …, "content_sha256": "sha256:…", "token": "…" }, "warnings": [...] } +``` + +```jsonc +// Phase 2 — execute +{ + "model": "res.partner", + "record_id": 42, + "field": "comment", + "input_path": "/srv/agent-scratch/comment-42.html", + "approval": , + "confirm": true +} +// → { "mode": "execute", "result": true, "bytes_written": 14823, "content_sha256": "…", "metadata_used": { … "field_type": "html" } } +``` + +The preview token never carries the file content — only its SHA-256 + size +fingerprint. The execute call re-reads the file, re-checks the hash +(so a tamper or benign race between the two calls is rejected), fetches +live `fields_get` metadata, refuses `readonly` fields, and routes the +write through the same gates as every other write: `confirm=true`, +`ODOO_MCP_ENABLE_WRITES=1`. + +## Configuration: file roots + +Both tools require an allow-listed root directory. The path you pass must be +**absolute** and must sit **inside** one of the configured roots. + +```bash +# Colon-separated absolute directories. Linux/macOS: +export ODOO_MCP_FIELD_FILE_ROOTS="/srv/agent-scratch:/srv/team-scratch" + +# Windows (semicolon): +export ODOO_MCP_FIELD_FILE_ROOTS="C:/agent-scratch;C:/team-scratch" +``` + +> **No default root, by design.** If `ODOO_MCP_FIELD_FILE_ROOTS` is not +> set, both tools refuse every call. The error message names the env +> var, lists platform-specific safe defaults (below), and **explicitly +> warns against `/tmp`** — see [Enabling the tools](#enabling-the-tools) +> below. + +- **Absolute paths only.** Relative paths are rejected with a clear error + before any file system call. +- **Containment, not chroot.** The agent can write anywhere inside a + configured root (and into sub-directories), but cannot escape it via + `..` traversal or symlinks — `Path.resolve(strict=False)` collapses + both before the containment check. +- **Fail closed.** Without `ODOO_MCP_FIELD_FILE_ROOTS`, both tools reject + every call. The `file_root` argument cannot stand in for the env var. +- **`file_root` is a selector, not a bypass.** When supplied, it must + equal one of the configured roots after resolve — it cannot widen the + operator's allow-list. A prompt-injected agent that passes + `file_root="/"` or `file_root="/home/user"` is rejected, even when the + env allow-list is set. +- **No overwrite on read.** `read_field_to_file` refuses if the + destination path already exists; both the pre-check and `O_CREAT | O_EXCL` + on open cover a TOCTOU race with a symlink swap. +- **Symlink defense on read.** The create fd uses `O_NOFOLLOW`; a + symlink whose target sits outside the root would also be caught by the + containment check (since `Path.resolve` follows it first). +- **Symlink defense on write.** Reads use `O_NOFOLLOW`; a TOCTOU swap + between the root check and the read is rejected because the size cap + and SHA-256 are derived from the bytes read through that same fd. + +### Enabling the tools + +There is **no implicit default**. To make `read_field_to_file` / +`write_field_from_file` work, set `ODOO_MCP_FIELD_FILE_ROOTS` to one or +more absolute directories (OS-PATHSEP separated). The `file_root` +argument on each tool call is a *selector* among the configured roots — +it cannot introduce a new root the operator has not approved. When the +tools reject a call because no root is configured, the error includes +platform-specific safe defaults: + +```text +ODOO_MCP_FIELD_FILE_ROOTS is not configured — refusing all field file I/O. +To enable read_field_to_file / write_field_from_file, set ODOO_MCP_FIELD_FILE_ROOTS +to one or more absolute directories (OS-PATHSEP separated): + + - Linux: /home//.cache/odoo-mcp/field-files + - macOS: /home//.cache/odoo-mcp/field-files + (or any user-owned absolute path with mode 0700) + +Security: do NOT point these roots at /tmp — /tmp is typically +world-readable on Linux/macOS, which would expose long field payloads +(HTML comments, source code, internal notes) to other local users and +processes. Use a per-user directory with mode 0700 instead. +``` + +The path suggestions honour `$XDG_CACHE_HOME` on POSIX and +`%LOCALAPPDATA%` on Windows; otherwise they default to +`~/.cache/odoo-mcp/field-files` (POSIX) or +`%LOCALAPPDATA%\odoo-mcp\Cache\field-files` (Windows). + +> **Why not `/tmp`?** It is `1777` (world-writable + sticky) on most +> Linux distributions and world-readable on macOS. A 50 KB product +> description or 80 KB internal note written there would be visible to +> every other process and user on the box. The tools refuse this category +> of mistake in the error message rather than silently honouring it. + +### Per-call `file_root` selector + +```jsonc +{ + "file_root": "/srv/another-root" +} +``` + +Used to pick *which* configured root the path must sit inside, when +`ODOO_MCP_FIELD_FILE_ROOTS` lists more than one. The argument is a +**selector**, not a free-form path: + +- It must equal one of the entries in `ODOO_MCP_FIELD_FILE_ROOTS` after + resolve (so `file_root="/srv/a"` is only valid when `/srv/a` is in + the configured list). +- It must be absolute. Relative values are rejected before + `Path.resolve()` runs, so `file_root="scratch"` does not silently + become `$CWD/scratch`. +- It cannot widen the operator's allow-list. A prompt-injected agent + passing `file_root="/"` or `file_root="/home/user"` is rejected with + a clear error. +- When omitted, the candidate is validated against *all* configured + roots (so multi-root configs like + `ODOO_MCP_FIELD_FILE_ROOTS=/srv/a:/srv/b` accept paths under either + root without an explicit selector). + +## Environment variables + +| Variable | Default | Effect | +| --- | --- | --- | +| `ODOO_MCP_FIELD_FILE_ROOTS` | unset | Colon-separated (or `os.pathsep`-separated) absolute directories the tools may read from / write into. **Required** — fails closed without it. | +| `ODOO_MCP_MAX_FIELD_FILE_BYTES` | `10485760` (10 MiB) | Hard cap per payload (read or write). Shared 16 MiB ceiling with attachment uploads (`ATTACHMENT_BYTES_HARD_CAP`). | + +Both knobs appear in `health_check.runtime.field_file_roots`: + +```jsonc +{ + "field_file_roots": { + "env": "ODOO_MCP_FIELD_FILE_ROOTS", + "count": 1, + "roots": ["/srv/agent-scratch"] + } +} +``` + +## Encoding + +| Field type (`fields_get.type`) | Default encoding | What the file holds | +| --- | --- | --- | +| `binary` (or caller passes `encoding="base64"`) | `base64` | The raw decoded bytes. | +| anything else (or caller passes `encoding="utf-8"`) | `utf-8` | `str(value).encode("utf-8")`. | + +A caller-supplied `encoding` argument overrides the type-based default. +Passing `encoding="base64"` on a non-binary field triggers a base64 +decode of the Odoo-supplied base64 string, which only succeeds when +`fields_get.type == "binary"` — otherwise the caller gets a clear +validation error rather than silently truncated bytes. + +## Field ACL interaction + +`read_field_to_file` runs through `get_field_policy()`: + +- Fields named in `field_acl[*][model].deny` are not exposed. The tool + writes a `[REDACTED by field ACL]` placeholder to the file and returns + `field_was_redacted: true` plus `redacted_fields: [...]` so the agent + cannot hallucinate the value. +- A redacted value never appears in the tool response payload, audit log + entry, or anywhere else. + +`write_field_from_file` does not consult the field ACL — the path is +gated by the write gate (`ODOO_MCP_ENABLE_WRITES=1` + `confirm=true`) +and by the live `fields_get` metadata check (refuses `readonly=True`). + +## Examples + +### Read a partner's HTML comment, edit locally, write it back + +```text +1. read_field_to_file(model="res.partner", record_id=42, field="comment", + output_path="/srv/agent-scratch/comment-42.html") + → file is on disk, response carries the SHA-256 fingerprint. + +2. Agent uses its ordinary file tools to read the file, edit, and save it. + +3. write_field_from_file(model="res.partner", record_id=42, field="comment", + input_path="/srv/agent-scratch/comment-42.html") + → mode="preview", returns an approval token with the new SHA-256. + +4. write_field_from_file(..., approval=, confirm=true) + → mode="execute", file is re-read, hash re-checked, write performed. +``` + +### Read a binary PDF attached to a marketing brochure + +```jsonc +read_field_to_file( + model="ir.attachment", + record_id=1985, + field="datas", // type=binary in fields_get + output_path="/srv/agent-scratch/brochure.pdf", + // encoding omitted — defaults to base64 for binary fields +) +``` + +The file on disk is a raw PDF; the agent's PDF library can read it +without first base64-decoding it from a JSON envelope. + +### Append to a long internal note without rewriting the whole field + +The agent can read the field to disk, append a paragraph with its file +tools, and write the file back — all without the 80 KB note entering the +agent's context twice. + +## Security model (summary) + +| Threat | Mitigation | +| --- | --- | +| Prompt-injected agent reads SSH keys or other secrets from `/etc` | Path is absolute + must sit inside `ODOO_MCP_FIELD_FILE_ROOTS`. No root configured → every call rejected. | +| Agent overwrites an unrelated file | `read_field_to_file` fails if destination exists (`O_CREAT|O_EXCL`). `write_field_from_file` only writes to Odoo, not the filesystem. | +| Symlink swap (TOCTOU) on read | `O_NOFOLLOW` on create fd; `Path.resolve` collapses symlinks before the containment check. | +| Symlink swap (TOCTOU) on write | File is opened with `O_RDONLY|O_NOFOLLOW`; size cap and SHA-256 derived from bytes read through that same fd. | +| File tamper between preview and execute | Execute re-reads the file and re-checks the SHA-256; mismatch → reject, no Odoo call. | +| Sensitive field leaking via filesystem | Field ACL redaction applies to `read_field_to_file`; redacted fields write a placeholder. | +| Token replay / payload swap | Execute verifies the approval token matches the current `content_sha256` of the current file content before doing anything. | + +## Auditing + +Both tools emit one audit-log entry per call (when `ODOO_MCP_AUDIT_LOG` is +set), with: + +- `tool`, `outcome` (`preview` / `success` / `denied`) +- `model`, `operation`, `record_ids` +- `instance` +- `token` (digest only — the raw token never lands on disk) +- `detail`: `field= bytes=` + +A successful `write_field_from_file` execute shows up as +`operation="write"` with `bytes=` and the resolved `instance`. + +## Compatibility + +| Odoo version | Behaviour | +| --- | --- | +| 16.0 / 17.0 / 18.0 (XML-RPC default) | Fully supported. Binary fields round-trip via base64 encoding. | +| 19.0 (XML-RPC or JSON-2) | Fully supported. Field-ACL integration unchanged. | + +The tools share no code with Odoo transports — they sit on top of the +same `OdooClient.read_records` and `OdooClient.execute_method("…", "write", …)` +calls the rest of the surface uses. + +## See also + +- [docs/field-acl.md](field-acl.md) — field-level ACL semantics and `read_field_to_file` redaction behavior +- [docs/adding-a-tool.md](adding-a-tool.md) — how a new MCP tool is registered (relevant when extending the file-root model) +- [docs/architecture.md](architecture.md) — surface vs. core module split (the file-root helper lives in `server_core.py`; both tools are thin `@mcp.tool` wrappers) diff --git a/pyproject.toml b/pyproject.toml index e4e003d..786ed5d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,8 +4,8 @@ build-backend = "setuptools.build_meta" [project] name = "odoo-mcp" -version = "1.2.3" -description = "Odoo MCP for AI agents — 41 tools, gated writes, multi-instance. Free hosted product: ERPipe (mcp.erpipe.com)" +version = "1.3.4" +description = "Odoo MCP for AI agents — 43 tools, gated writes, multi-instance. Free hosted product: ERPipe (mcp.erpipe.com)" readme = "README.md" requires-python = ">=3.10" license = "MIT" diff --git a/scripts/odoo_compose_smoke.py b/scripts/odoo_compose_smoke.py index 004911f..476a229 100755 --- a/scripts/odoo_compose_smoke.py +++ b/scripts/odoo_compose_smoke.py @@ -753,6 +753,7 @@ def assert_tool_surface(tool_names: set[str]) -> None: "get_model_fields", "search_records", "read_record", + "read_field_to_file", "search_employee", "search_holidays", "diagnose_odoo_call", @@ -766,6 +767,7 @@ def assert_tool_surface(tool_names: set[str]) -> None: "preview_write", "validate_write", "execute_approved_write", + "write_field_from_file", "scan_addons_source", "build_domain", "business_pack_report", @@ -1087,8 +1089,8 @@ async def mcp_stdio_smoke( await session.call_tool("health_check", arguments={}), "health_check", ) - if health.get("server", {}).get("tool_count") != 41: - raise AssertionError(f"health_check did not report 41 tools: {health}") + if health.get("server", {}).get("tool_count") != 43: + raise AssertionError(f"health_check did not report 43 tools: {health}") if "chatter_direct_enabled" not in health.get("runtime", {}): raise AssertionError( f"health_check did not surface chatter_direct posture: {health}" diff --git a/server.json b/server.json index 3145ea1..f825904 100644 --- a/server.json +++ b/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json", "name": "io.github.erpipe-org/mcp-odoo", "description": "Odoo MCP: gated writes, multi-instance. Free hosted: ERPipe (mcp.erpipe.com)", - "version": "1.2.3", + "version": "1.3.4", "websiteUrl": "https://erpipe-org.github.io/mcp-odoo/", "repository": { "url": "https://github.com/erpipe-org/mcp-odoo", @@ -12,7 +12,7 @@ { "registryType": "pypi", "identifier": "odoo-mcp", - "version": "1.2.3", + "version": "1.3.4", "transport": { "type": "stdio" }, diff --git a/src/odoo_mcp/agent_tools.py b/src/odoo_mcp/agent_tools.py index c1c4050..60b9751 100644 --- a/src/odoo_mcp/agent_tools.py +++ b/src/odoo_mcp/agent_tools.py @@ -259,13 +259,46 @@ def build_write_preview_report( } +def _normalized_record_ids(record_ids: Any) -> list[int]: + """Flatten + int-cast ``record_ids`` the way ``build_write_preview_report`` does. + + The approval store builds its SHA-256 token from a flat ``list[int]`` of + record ids. ``verify_write_approval`` and ``write_approval_payload`` must + apply the same normalization before re-hashing, otherwise a + transport- or framework-induced shape change (e.g. nested + ``[[3598, 3594, ...]]`` arriving at the execute call instead of flat + ``[3598, 3594, ...]`` from the preview) flips the SHA-256 and surfaces + as ``"approval token does not match the canonical payload"``. Mirrors + the int/float drift fix in 1.2.1 (`_normalize_numbers`). + """ + if not record_ids: + return [] + out: list[int] = [] + for item in record_ids: + if isinstance(item, bool): + # bool is an int subclass in Python; skip to avoid surprises. + continue + if isinstance(item, int): + out.append(item) + continue + if isinstance(item, str) and item.lstrip("-").isdigit(): + out.append(int(item)) + continue + if isinstance(item, (list, tuple)): + out.extend(_normalized_record_ids(item)) + return out + def verify_write_approval(approval: dict[str, Any]) -> tuple[bool, str]: """Verify a write approval token against the canonical payload.""" token = str(approval.get("token", "")) payload = { "model": approval.get("model"), "operation": approval.get("operation"), - "record_ids": approval.get("record_ids") or [], + # Normalize record_ids to a flat list of int, mirroring + # build_write_preview_report so a transport- or wrapper-induced + # change in list structure (e.g. nested list) cannot change the + # SHA-256 approval token between preview/validate and execute. + "record_ids": _normalized_record_ids(approval.get("record_ids")), "values": approval.get("values") or {}, "context": approval.get("context") or {}, "instance": approval.get("instance") or "default", diff --git a/src/odoo_mcp/error_handling.py b/src/odoo_mcp/error_handling.py new file mode 100644 index 0000000..85b78d9 --- /dev/null +++ b/src/odoo_mcp/error_handling.py @@ -0,0 +1,41 @@ +"""Centralised error handling helpers for MCP server_core.py. + +Kept deliberately small: this module hosts ``_format_validation_error``, +the one pure helper that turns a ``pydantic.ValidationError`` into a +single-line, bounded, agent-readable summary. The rest of the friendly-envelope +plumbing lives in ``server_core.py`` (see ``_TranslationAwareFastMCP``). + +Why not a ``@safe_tool_call`` decorator anymore? FastMCP validates annotated +tool arguments via Pydantic **before** the tool body runs (see +``mcp.server.fastmcp.utilities.func_metadata.call_fn_with_arg_validation``) +and converts any ``ValidationError`` to a ``ToolError`` whose ``__cause__`` is +the original error. A decorator on the function therefore never sees the +``ValidationError`` — the error is reformatted into a stack-trace-like string +before reaching the function. The actual hook must sit one layer up, at the +JSON-RPC dispatch boundary; that's what ``_TranslationAwareFastMCP`` does. +""" + +from __future__ import annotations + +from pydantic import ValidationError + + +def _format_validation_error(exc: ValidationError) -> str: + """Render a ``ValidationError`` as one human-readable line per problem. + + Capped at three entries; a trailing ``(and more)`` flag is added when + additional errors were truncated. Agent callers benefit from the + one-line format because they read it as a single diagnostic sentence + instead of a wall of Pydantic multi-line output that wraps at + unpredictable column widths. + """ + parts: list[str] = [] + for err in exc.errors()[:3]: + loc = ".".join(str(part) for part in err.get("loc", ())) + msg = err.get("msg", "invalid value") + parts.append(f"{loc}: {msg}" if loc else msg) + suffix = " (and more)" if len(exc.errors()) > 3 else "" + return "; ".join(parts) + suffix + + +__all__: list[str] = ["_format_validation_error"] diff --git a/src/odoo_mcp/schemas.py b/src/odoo_mcp/schemas.py index d6168e5..adad3a1 100644 --- a/src/odoo_mcp/schemas.py +++ b/src/odoo_mcp/schemas.py @@ -142,6 +142,98 @@ class ReadAttachmentResponse(ToolResponse): warnings: Optional[List[str]] = None +class ReadFieldToFileResponse(ToolResponse): + model: Optional[str] = Field(default=None, description="Source model name.") + record_id: Optional[int] = Field(default=None, description="Source record ID.") + field: Optional[str] = Field(default=None, description="Field name read.") + output_path: Optional[str] = Field( + default=None, + description=( + "Absolute path of the file the value was written to. Never " + "overwritten — the call fails if the path already exists." + ), + ) + file_root: Optional[str] = Field( + default=None, + description=( + "Resolved root directory the output path sits inside. " + "Configured via ODOO_MCP_FIELD_FILE_ROOTS or per-call file_root." + ), + ) + encoding: Optional[str] = Field( + default=None, + description='Encoding used for the on-disk payload: "utf-8" for text, "base64" for binary.', + ) + bytes_written: Optional[int] = Field( + default=None, description="Number of bytes written to output_path." + ) + content_sha256: Optional[str] = Field( + default=None, + description="SHA-256 of the bytes written — matches what the agent can verify locally.", + ) + field_was_redacted: Optional[bool] = Field( + default=None, + description=( + "True when the field ACL withheld the value; output_path then " + "contains a placeholder, not the real field content." + ), + ) + redacted_fields: Optional[List[str]] = Field( + default=None, description="Field names redacted by the field ACL." + ) + metadata_used: Optional[Dict[str, Any]] = Field( + default=None, + description="Resolved instance + file_root for auditability.", + ) + + +class WriteFieldFromFileResponse(ToolResponse): + mode: Optional[str] = Field( + default=None, + description='"preview" (no write performed) or "execute" (write performed).', + ) + model: Optional[str] = Field(default=None, description="Target model name.") + record_id: Optional[int] = Field(default=None, description="Target record ID.") + field: Optional[str] = Field(default=None, description="Field name written.") + input_path: Optional[str] = Field( + default=None, + description="Absolute path the value was read from.", + ) + file_root: Optional[str] = Field( + default=None, + description="Resolved root directory the input path sits inside.", + ) + encoding: Optional[str] = Field( + default=None, + description='Encoding of the source file: "utf-8" for text, "base64" for binary.', + ) + bytes_written: Optional[int] = Field( + default=None, description="Bytes streamed into the Odoo field (execute mode only)." + ) + content_sha256: Optional[str] = Field( + default=None, + description="SHA-256 of the bytes streamed into the Odoo field.", + ) + approval: Optional[Dict[str, Any]] = Field( + default=None, + description=( + "Preview-mode payload: re-call with this approval plus " + "confirm=true to execute the write." + ), + ) + warnings: Optional[List[str]] = Field( + default=None, + description='Preview-mode guidance, e.g. "re-call with approval + confirm=true".', + ) + result: Optional[Any] = Field( + default=None, description="Odoo return value from the write (execute mode only)." + ) + metadata_used: Optional[Dict[str, Any]] = Field( + default=None, + description="Resolved instance + file_root + size cap for auditability.", + ) + + class AggregateRecordsResponse(ToolResponse): method: Optional[str] = Field( default=None, description="formatted_read_group (19+) or read_group." diff --git a/src/odoo_mcp/server.py b/src/odoo_mcp/server.py index fe222fe..8aa1c42 100644 --- a/src/odoo_mcp/server.py +++ b/src/odoo_mcp/server.py @@ -30,6 +30,7 @@ app_lifespan, configured_addons_roots, configured_attachment_upload_roots, + configured_field_file_roots, get_model_info, get_models, get_record, @@ -41,12 +42,15 @@ register_write_approval, require_validated_write_approval, resolve_default_instance_name, + resolve_file_root_override, resolve_instance_name, resolve_read_fields, restrict_addons_paths, restrict_attachment_upload_path, + restrict_field_file_path, runtime_security_report, search_records_resource, + FIELD_FILE_ROOTS_ENV, write_approval_payload, ) @@ -84,6 +88,7 @@ _build_chatter_payload, _elicit_write_confirmation, _execute_approved_write_gated, + _read_field_file, _write_elicitation_message, chatter_post, execute_approved_write, @@ -91,6 +96,7 @@ execute_method, preview_write, validate_write, + write_field_from_file, ) # Re-export read tool functions @@ -102,6 +108,7 @@ list_instances, list_models, read_attachment, + read_field_to_file, read_record, schema_catalog, search_employee, @@ -186,6 +193,7 @@ ATTACHMENT_BYTES_HARD_CAP, DEFAULT_MAX_ATTACHMENT_BYTES, DEFAULT_MAX_ATTACHMENT_UPLOAD_BYTES, + DEFAULT_MAX_FIELD_FILE_BYTES, DEFAULT_MAX_SMART_FIELDS, MAX_SEARCH_LIMIT, METHOD_NAME_RE, @@ -195,6 +203,7 @@ clamp_limit, max_attachment_bytes, max_attachment_upload_bytes, + max_field_file_bytes, max_smart_fields, normalize_domain_input, odoo_major_version, @@ -276,12 +285,14 @@ "execute_approved_write_tool", "chatter_post", "execute_method", + "write_field_from_file", # Read tools "list_models", "get_model_fields", "search_records", "read_record", "read_attachment", + "read_field_to_file", "aggregate_records", "schema_catalog", "search_employee", @@ -341,6 +352,7 @@ "ATTACHMENT_BYTES_HARD_CAP", "DEFAULT_MAX_ATTACHMENT_BYTES", "DEFAULT_MAX_ATTACHMENT_UPLOAD_BYTES", + "DEFAULT_MAX_FIELD_FILE_BYTES", "DEFAULT_MAX_SMART_FIELDS", "MAX_SEARCH_LIMIT", "METHOD_NAME_RE", @@ -350,6 +362,7 @@ "clamp_limit", "max_attachment_bytes", "max_attachment_upload_bytes", + "max_field_file_bytes", "max_smart_fields", "normalize_domain_input", "odoo_major_version", @@ -392,15 +405,19 @@ "N_PLUS_ONE_WINDOW_SECONDS", "configured_addons_roots", "configured_attachment_upload_roots", + "configured_field_file_roots", + "FIELD_FILE_ROOTS_ENV", "instance_posture", "mcp_surface_counts", "n_plus_one_report", "note_single_record_read", "register_write_approval", "require_validated_write_approval", + "resolve_file_root_override", "resolve_read_fields", "restrict_addons_paths", "restrict_attachment_upload_path", + "restrict_field_file_path", "runtime_security_report", "load_plugins", "apply_tool_filter", @@ -415,6 +432,7 @@ "_build_chatter_payload", "_elicit_write_confirmation", "_execute_approved_write_gated", + "_read_field_file", "_write_elicitation_message", ] diff --git a/src/odoo_mcp/server_core.py b/src/odoo_mcp/server_core.py index 6c1fc8d..b42c6a1 100644 --- a/src/odoo_mcp/server_core.py +++ b/src/odoo_mcp/server_core.py @@ -8,6 +8,8 @@ use late-binding via _srv() so monkeypatches applied to the server module work. """ +from __future__ import annotations + import json import os import threading @@ -15,15 +17,16 @@ from contextlib import asynccontextmanager from dataclasses import dataclass, field from pathlib import Path -from typing import Any, AsyncIterator, Callable, Dict, List, Optional, cast +from typing import Any, AsyncIterator, Callable, Dict, List, Optional, Sequence, cast from mcp.server.fastmcp import Context, FastMCP -from mcp.types import Annotations, ToolAnnotations +from mcp.types import Annotations, ContentBlock, TextContent, ToolAnnotations from pydantic import BaseModel, Field +from .error_handling import _format_validation_error from .odoo_client import OdooClient from .schema_cache import _build_schema_cache -from .agent_tools import select_smart_fields +from .agent_tools import _normalized_record_ids, select_smart_fields from .tool_helpers import ( max_smart_fields, truthy_env, @@ -127,13 +130,82 @@ def load_server_instructions() -> str: return f"{DEFAULT_SERVER_INSTRUCTIONS}\n\n{text}" -mcp = FastMCP( +class _TranslationAwareFastMCP(FastMCP): + """FastMCP subclass that catches Pydantic ``ValidationError`` raised during + argument binding and re-emits the friendly ``{"success": False, "tool": …, + "error": "Invalid input: …"}`` envelope the rest of the tool surface uses. + Subclassing is required because FastMCP's JSON-RPC handler is registered + at ``__init__`` time via ``self._mcp_server.call_tool(validate_input=False) + (self.call_tool)`` — the bound method is captured by reference. Replacing + ``mcp.call_tool`` post-init OR replacing ``mcp._tool_manager.call_tool`` + both bypass the actual dispatch path. A subclass overrides ``self.call_tool`` + so the *registered* handler is the wrapper. + + When a tool advertises an ``outputSchema`` (i.e. ``structured_output=True``), + FastMCP expects the tool to return a dict matching that schema. Returning a + list of ``ContentBlock`` would yield ``Output validation error: outputSchema + defined but no structured output returned`` on the MCP transport. We + therefore return the envelope as a plain dict when structured output is + enabled, and as a ``[TextContent(...)]`` content-block list when it's not + (matches how FastMCP itself routes these two return shapes). + """ + + async def call_tool( # type: ignore[override] + self, name: str, arguments: dict[str, Any] + ) -> Any: + try: + return await super().call_tool(name, arguments) + except BaseException as exc: # noqa: BLE001 — last-line translator + translated = _translate_validation_tool_error_to_envelope( + exc, tool_name_hint=name + ) + if translated is None: + raise + tool = self._tool_manager.get_tool(name) + structured = bool(getattr(tool, "structured_output", True)) + if structured: + # FastMCP wraps a returned dict into ``structuredContent`` so it + # satisfies the tool's ``outputSchema``. Some old callers expect + # content blocks; they get the same dict through TextContent. + return { + "success": False, + "tool": translated.text and json.loads(translated.text).get("tool", name), + "error": translated.text and json.loads(translated.text).get("error", ""), + } + return [translated] + + +def _translate_validation_tool_error_to_envelope( + exc: BaseException, + tool_name_hint: Optional[str] = None, +) -> Optional[ContentBlock]: + """Return a friendly envelope content block, or None if not a validation error.""" + from mcp.server.fastmcp.exceptions import ToolError + from pydantic import ValidationError + + if not isinstance(exc, ToolError): + return None + cause = exc.__cause__ + if not isinstance(cause, ValidationError): + return None + + tool_name = tool_name_hint or "tool" + envelope = { + "success": False, + "tool": tool_name, + "error": f"Invalid input: {_format_validation_error(cause)}", + } + return TextContent(type="text", text=json.dumps(envelope, sort_keys=True)) + + +mcp = _TranslationAwareFastMCP( "Odoo MCP Server", instructions=load_server_instructions(), dependencies=["requests"], lifespan=app_lifespan, ) + READ_ONLY_TOOL = ToolAnnotations( readOnlyHint=True, destructiveHint=False, idempotentHint=True, openWorldHint=True ) @@ -244,7 +316,12 @@ def write_approval_payload(approval: Dict[str, Any]) -> Dict[str, Any]: payload = { "model": approval.get("model"), "operation": approval.get("operation"), - "record_ids": approval.get("record_ids") or [], + # Normalize record_ids to a flat list of int, mirroring + # build_write_preview_report / verify_write_approval so the + # payload-equality check in _execute_approved_write_gated always + # compares canonical shapes (transport- or wrapper-induced nesting + # is silently flattened rather than flipping the SHA-256 token). + "record_ids": _normalized_record_ids(approval.get("record_ids")), "values": approval.get("values") or {}, "context": approval.get("context") or {}, "instance": approval.get("instance") or "default", @@ -407,6 +484,207 @@ def restrict_attachment_upload_path(raw_path: str) -> Path: return candidate +# --------------------------------------------------------------------------- +# Field file I/O path helpers (read_field_to_file / write_field_from_file) +# --------------------------------------------------------------------------- + +FIELD_FILE_ROOTS_ENV = "ODOO_MCP_FIELD_FILE_ROOTS" + + +def configured_field_file_roots() -> List[Path]: + """Return trusted local roots operators allow field file I/O from. + + Configured by ``ODOO_MCP_FIELD_FILE_ROOTS`` (OS-PATHSEP list of absolute + directories). Used by both ``read_field_to_file`` and + ``write_field_from_file`` — a value the user can override per call via + the ``file_root`` argument on those tools. + """ + roots: List[Path] = [] + for raw_path in os.environ.get(FIELD_FILE_ROOTS_ENV, "").split(os.pathsep): + if not raw_path: + continue + roots.append(Path(raw_path).expanduser().resolve(strict=False)) + return roots + + +def _suggest_field_file_roots() -> str: + """Build a per-platform hint listing safe default roots. + + The list is a *suggestion*, not an automatic fallback — we still refuse + the call without explicit operator consent. ``/tmp`` is *deliberately* + not suggested (it is world-readable on most Linux systems and would + expose long field payloads to other local users / processes). + """ + home = Path.home() + if os.name == "nt": + local_app = os.environ.get("LOCALAPPDATA") or str(home / "AppData" / "Local") + return ( + f" - Windows: {local_app}\\odoo-mcp\\Cache\\field-files\n" + f" (or any user-owned absolute path)" + ) + # POSIX (Linux + macOS) — XDG_CACHE_HOME defaults to ~/.cache + xdg_cache = os.environ.get("XDG_CACHE_HOME") or str(home / ".cache") + return ( + f" - Linux: {xdg_cache}/odoo-mcp/field-files\n" + f" - macOS: {xdg_cache}/odoo-mcp/field-files\n" + f" (or any user-owned absolute path with mode 0700)" + ) + + +def _no_roots_configured_error() -> ValueError: + """Build the fail-closed error for the no-roots-configured case. + + Shared between ``resolve_file_root_override`` and + ``restrict_field_file_path`` so every code path that runs without + ``ODOO_MCP_FIELD_FILE_ROOTS`` returns the same actionable message: + names the env var, lists platform-suggested paths, and warns + against ``/tmp``. + """ + return ValueError( + f"{FIELD_FILE_ROOTS_ENV} is not configured — refusing all field " + "file I/O. To enable read_field_to_file / write_field_from_file, " + f"set {FIELD_FILE_ROOTS_ENV} to one or more absolute directories " + "(OS-PATHSEP separated):\n" + f"{_suggest_field_file_roots()}\n" + "\n" + "Security: do NOT point these roots at /tmp — /tmp is typically " + "world-readable on Linux/macOS, which would expose long field " + "payloads (HTML comments, source code, internal notes) to other " + "local users and processes. Use a per-user directory with mode " + "0700 instead." + ) + + +def resolve_file_root_override(file_root: Optional[str]) -> Path: + """Resolve the ``file_root`` argument as a selector among configured roots. + + The operator's allow-list (``ODOO_MCP_FIELD_FILE_ROOTS``) is the only + source of truth — ``file_root`` cannot widen it. The argument is + treated as a selector: when supplied, it must equal one of the + configured roots after resolve. When omitted, the first configured + root is returned. + + Raises ValueError when no root is configured — fail-closed is the + only safe default here. The error message names the env var, lists + platform-suggested paths, and warns against ``/tmp``. + """ + roots = configured_field_file_roots() + if not roots: + raise _no_roots_configured_error() + if file_root: + # Check absolute-ness on the expanduser'd path *before* resolve() — + # otherwise a relative path silently gets expanded to cwd and the + # check below would let it through on any machine whose cwd happens + # to live inside a root. + raw = Path(file_root).expanduser() + if not raw.is_absolute(): + raise ValueError( + f"file_root override must be an absolute path; got {file_root!r}" + ) + candidate = raw.resolve(strict=False) + # ``file_root`` is a *selector* among the configured roots — it + # cannot invent a new root that the operator has not approved. + # This blocks the prompt-injection bypass where an agent passes + # file_root="/" or file_root="/home/user" and then reads / writes + # through paths the allow-list would have rejected. + if not any(candidate == root for root in roots): + raise ValueError( + f"file_root {candidate} is not one of the configured " + f"{FIELD_FILE_ROOTS_ENV} roots: {[str(r) for r in roots]}. " + "The file_root argument can only select among configured " + "roots; it cannot widen the operator's allow-list." + ) + return candidate + return roots[0] + + +def restrict_field_file_path( + raw_path: str, file_root: Optional[str] = None +) -> tuple[Path, Path]: + """Validate a field file I/O path against an allow-listed root. + + Mirrors the hardened pattern from ``restrict_attachment_upload_path``: + the path must be **absolute** and must sit inside (or below) one of + the configured roots. Returns ``(resolved_path, resolved_root)`` so + the caller can echo the exact root it landed in. + + Unlike a ``chroot``, sub-directories of the root are allowed — but no + symlink or ``..`` traversal may escape it. The combination of + ``Path.resolve(strict=False)`` (which already collapses ``..`` and + follows symlinks *before* the containment check) plus the explicit + relative-to test is sufficient for this containment guarantee. + + When ``file_root`` is supplied it acts only as a selector among the + configured roots (see ``resolve_file_root_override``) — it cannot + widen the operator's allow-list. When omitted, the candidate is + validated against *all* configured roots and the matching root is + returned, so multi-root configs like + ``ODOO_MCP_FIELD_FILE_ROOTS=/srv/a:/srv/b`` accept paths under + either root. + + Callers MUST additionally use ``O_NOFOLLOW`` / ``O_EXCL`` on the + resulting fd to close the TOCTOU window — see ``_read_field_file`` + in ``tools_write.py`` and the ``O_CREAT|O_EXCL|O_NOFOLLOW`` block in + ``read_field_to_file`` (``tools_read.py``). + """ + roots = configured_field_file_roots() + if not roots: + raise _no_roots_configured_error() + # Check absolute-path-ness *before* resolve() — otherwise a relative + # path silently gets expanded to cwd and the check below would let it + # through on any machine whose cwd happens to live inside a root. + raw = Path(raw_path).expanduser() + if not raw.is_absolute(): + raise ValueError( + f"field file path must be absolute; got {raw_path!r}" + ) + candidate = raw.resolve(strict=False) + if file_root: + # Selector semantics: ``file_root`` must equal one of the + # configured roots. Re-validate here so a caller cannot sneak a + # different root past the containment check by passing a + # ``file_root`` we did not previously see. + raw_root = Path(file_root).expanduser() + if not raw_root.is_absolute(): + raise ValueError( + f"file_root override must be an absolute path; got {file_root!r}" + ) + resolved_root = raw_root.resolve(strict=False) + if not any(resolved_root == root for root in roots): + raise ValueError( + f"file_root {resolved_root} is not one of the configured " + f"{FIELD_FILE_ROOTS_ENV} roots: {[str(r) for r in roots]}. " + "The file_root argument can only select among configured " + "roots; it cannot widen the operator's allow-list." + ) + # Containment check against the selected root only. + if not ( + candidate == resolved_root + or _is_relative_to(candidate, resolved_root) + ): + raise ValueError( + f"{candidate} is outside the configured field file root " + f"{resolved_root}." + ) + return candidate, resolved_root + # No override: validate against ALL configured roots (mirrors the + # attachment-upload path which iterates over every root). + matching_root = next( + ( + root + for root in roots + if candidate == root or _is_relative_to(candidate, root) + ), + None, + ) + if matching_root is None: + raise ValueError( + f"{candidate} is outside all configured {FIELD_FILE_ROOTS_ENV} " + f"roots: {[str(r) for r in roots]}." + ) + return candidate, matching_root + + # --------------------------------------------------------------------------- # N+1 detection # --------------------------------------------------------------------------- @@ -530,6 +808,11 @@ def runtime_security_report() -> Dict[str, Any]: "audit_log": audit_posture(), "oauth": oauth_posture(), "field_acl": field_policy_posture(), + "field_file_roots": { + "env": FIELD_FILE_ROOTS_ENV, + "count": len(configured_field_file_roots()), + "roots": [str(root) for root in configured_field_file_roots()], + }, "n_plus_one": n_plus_one_report(), "notes": [ "HTTP transports are local-only by default in the CLI entry point.", diff --git a/src/odoo_mcp/tool_helpers.py b/src/odoo_mcp/tool_helpers.py index 8621140..ac0c099 100644 --- a/src/odoo_mcp/tool_helpers.py +++ b/src/odoo_mcp/tool_helpers.py @@ -157,6 +157,25 @@ def max_attachment_upload_bytes() -> int: return max(1, min(value, ATTACHMENT_BYTES_HARD_CAP)) +DEFAULT_MAX_FIELD_FILE_BYTES = 10 * 1024 * 1024 + + +def max_field_file_bytes() -> int: + """Read the configured field file I/O size cap (default 10 MiB). + + Backs both ``read_field_to_file`` (incoming field value streamed to disk) + and ``write_field_from_file`` (local file contents streamed into a field). + Bounded by the same ``ATTACHMENT_BYTES_HARD_CAP`` as attachment uploads, + so the operator has one shared ceiling to reason about. + """ + raw = os.environ.get("ODOO_MCP_MAX_FIELD_FILE_BYTES", "").strip() + try: + value = int(raw) if raw else DEFAULT_MAX_FIELD_FILE_BYTES + except ValueError: + value = DEFAULT_MAX_FIELD_FILE_BYTES + return max(1, min(value, ATTACHMENT_BYTES_HARD_CAP)) + + def truthy_env(name: str) -> bool: """Read a common boolean environment flag.""" return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"} @@ -263,7 +282,13 @@ def formatted_read_group_missing(exc: Exception) -> bool: def normalize_domain_input(domain: Any) -> List[Any]: - """Normalize common MCP/JSON domain shapes to an Odoo domain list.""" + """Normalize common MCP/JSON domain shapes to an Odoo domain list. + + Bad input (e.g. shell-style ``&&`` glued to a non-list string, malformed + JSON, or an unparseable Python literal) raises ``ValueError`` rather + than silently degrading to an empty list — that would otherwise turn + every typo into "match every record" and produce runaway aggregates. + """ if domain is None: return [] if isinstance(domain, SearchDomain): @@ -278,8 +303,15 @@ def normalize_domain_input(domain: Any) -> List[Any]: import ast domain_value = ast.literal_eval(domain_value) - except (SyntaxError, ValueError): - return [] + except (SyntaxError, ValueError) as exc: + raise ValueError( + "Domain string is neither valid JSON nor a Python " + f"literal: {exc}. Pass a list of 3-tuples, e.g. " + '[["is_timesheet", "=", true], ["project_id", "=", 37]], ' + 'or use the & prefix operator: ' + '["&", ["is_timesheet", "=", true], ' + '["project_id", "=", 37]].' + ) from exc if isinstance(domain_value, dict): conditions = domain_value.get("conditions") diff --git a/src/odoo_mcp/tools_cross_instance.py b/src/odoo_mcp/tools_cross_instance.py index 49ae121..5ecffaa 100644 --- a/src/odoo_mcp/tools_cross_instance.py +++ b/src/odoo_mcp/tools_cross_instance.py @@ -227,11 +227,37 @@ def worker(instance: str) -> Dict[str, Any]: ) def search_across_instances( ctx: Context, - model: str, - domain: Optional[Any] = None, - fields: Optional[List[str]] = None, - limit_per_instance: int = DEFAULT_LIMIT_PER_INSTANCE, - instances: Optional[Any] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + domain: Annotated[ + Optional[Any], + Field( + description=( + "Optional Odoo domain filter — same shape as search_records.domain. " + "Applied uniformly on every queried instance." + ) + ), + ] = None, + fields: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional subset of field names to return. Omit for the smart-field " + "selection per instance; pass ['*'] for every available field." + ) + ), + ] = None, + limit_per_instance: Annotated[ + int, Field(description="Maximum records to fetch per instance; default 50, capped at 500.") + ] = DEFAULT_LIMIT_PER_INSTANCE, + instances: Annotated[ + Optional[Any], + Field( + description=( + "Optional instance selector. Omit (or 'all') for every opted-in instance, " + "pass a list of names, or a {\"tags\": [\"...\"]} dict." + ) + ), + ] = None, ) -> SearchAcrossInstancesResponse: """Run one search across many instances; rows are tagged with `_instance`. @@ -267,11 +293,38 @@ def search_across_instances( ) def aggregate_across_instances( ctx: Context, - model: str, - group_by: List[str], - measures: Optional[List[str]] = None, - domain: Optional[Any] = None, - instances: Optional[Any] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + group_by: Annotated[ + List[str], + Field( + description=( + "Fields to group by (suffix ':granularity' for date/datetime). " + "Must include at least one field." + ) + ), + ], + measures: Annotated[ + Optional[List[str]], + Field( + description=( + 'Optional list of "field:agg" measure specs (sum/avg/min/max/count/...). ' + "Combined totals are additive across instances; averages are not combined." + ) + ), + ] = None, + domain: Annotated[ + Optional[Any], + Field(description="Optional Odoo domain filter — same shape as search_records.domain."), + ] = None, + instances: Annotated[ + Optional[Any], + Field( + description=( + "Optional instance selector. Omit (or 'all') for every opted-in instance, " + "pass a list of names, or a {\"tags\": [\"...\"]} dict." + ) + ), + ] = None, ) -> AggregateAcrossInstancesResponse: """Group/aggregate per instance plus additive grand totals across them. diff --git a/src/odoo_mcp/tools_data_quality.py b/src/odoo_mcp/tools_data_quality.py index 611293a..10e8075 100644 --- a/src/odoo_mcp/tools_data_quality.py +++ b/src/odoo_mcp/tools_data_quality.py @@ -4,9 +4,10 @@ Includes: data_quality_report. """ -from typing import Any, Dict, List, Optional +from typing import Annotated, Any, Dict, List, Optional from mcp.server.fastmcp import Context +from pydantic import Field from .data_quality import ALL_CHECKS, build_data_quality_report from .schemas import ToolResponse @@ -36,11 +37,32 @@ class DataQualityReportResponse(ToolResponse): ) def data_quality_report( ctx: Context, - model: str, - checks: Optional[List[str]] = None, - key_fields: Optional[List[str]] = None, - sample_limit: int = 500, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name to scan, e.g. 'res.partner'.")], + checks: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional subset of check names to run. Default runs all checks " + "(duplicates, missing_required, orphaned_references, format_anomalies)." + ) + ), + ] = None, + key_fields: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional field names to use as the duplicate-scan key. Overrides " + "the default heuristic (email/vat/ref/...)." + ) + ), + ] = None, + sample_limit: Annotated[ + int, Field(description="Maximum records sampled per check; default 500, capped at 2000.") + ] = 500, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> DataQualityReportResponse: """ Evidence-first data-quality report for a model (never modifies data). diff --git a/src/odoo_mcp/tools_diagnostics.py b/src/odoo_mcp/tools_diagnostics.py index 5d8e452..f7db33f 100644 --- a/src/odoo_mcp/tools_diagnostics.py +++ b/src/odoo_mcp/tools_diagnostics.py @@ -128,13 +128,27 @@ def diagnose_odoo_call( structured_output=True, ) def generate_json2_payload( - model: str, - method: str, - args: Optional[List[Any]] = None, - kwargs: Optional[Dict[str, Any]] = None, - base_url: Optional[str] = None, - database: Optional[str] = None, - include_database_header: bool = True, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + method: Annotated[str, Field(description="Odoo model method name to invoke, e.g. 'search_read'.")], + args: Annotated[ + Optional[List[Any]], + Field(description="Optional positional arguments to pass to the Odoo method."), + ] = None, + kwargs: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional keyword arguments to pass to the Odoo method."), + ] = None, + base_url: Annotated[ + Optional[str], + Field(description="Optional override of the Odoo base URL; defaults to the configured instance URL."), + ] = None, + database: Annotated[ + Optional[str], + Field(description="Optional override of the Odoo database name; defaults to the configured instance DB."), + ] = None, + include_database_header: Annotated[ + bool, Field(description="Whether to include the X-Odoo-Database header in the preview.") + ] = True, ) -> Dict[str, Any]: """Generate a JSON-2 endpoint, headers, and named JSON body.""" return generate_json2_payload_report( @@ -155,12 +169,29 @@ def generate_json2_payload( ) def inspect_model_relationships( ctx: Context, - model: str, - fields_metadata: Optional[Dict[str, Any]] = None, - include_readonly: bool = True, - include_computed: bool = True, - use_live_metadata: bool = True, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name to inspect, e.g. 'res.partner'.")], + fields_metadata: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Optional pre-fetched fields_get dict to analyze. When omitted and " + "use_live_metadata is true, the tool fetches it via bounded fields_get." + ) + ), + ] = None, + include_readonly: Annotated[ + bool, Field(description="Whether to include readonly fields in the report; default true.") + ] = True, + include_computed: Annotated[ + bool, Field(description="Whether to include computed (non-stored) fields; default true.") + ] = True, + use_live_metadata: Annotated[ + bool, Field(description="When true and no fields_metadata is provided, fetch live fields_get from Odoo.") + ] = True, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> Dict[str, Any]: """Summarize relationship fields using provided metadata or bounded fields_get.""" try: @@ -566,9 +597,13 @@ def upgrade_risk_report( structured_output=True, ) def analyze_upgrade_log( - log_text: str, - source_version: Optional[str] = None, - target_version: Optional[str] = None, + log_text: Annotated[str, Field(description="Raw Odoo install/update/upgrade log text to classify.")], + source_version: Annotated[ + Optional[str], Field(description="Optional source Odoo version, e.g. '16.0'.") + ] = None, + target_version: Annotated[ + Optional[str], Field(description="Optional target Odoo version, e.g. '17.0'.") + ] = None, ) -> Dict[str, Any]: """ Parse an Odoo install/update/upgrade log and classify known failure @@ -614,12 +649,39 @@ def lookup_model_history(name: str) -> Dict[str, Any]: structured_output=True, ) def fit_gap_report( - requirements: List[Any], - available_models: Optional[List[str]] = None, - available_fields: Optional[Dict[str, Any]] = None, - installed_modules: Optional[List[Any]] = None, - business_context: Optional[Dict[str, Any]] = None, - use_live_metadata: bool = False, + requirements: Annotated[ + List[Any], + Field( + description=( + "List of requirement objects or strings to bucketize. Each item is " + "normalized into a requirement dict before classification." + ) + ), + ], + available_models: Annotated[ + Optional[List[str]], + Field(description="Optional list of Odoo model names already in the target environment."), + ] = None, + available_fields: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Optional {model: [field, ...]} map of currently available fields to " + "use when judging each requirement's fit." + ) + ), + ] = None, + installed_modules: Annotated[ + Optional[List[Any]], + Field(description="Optional list of installed Odoo modules to constrain the analysis."), + ] = None, + business_context: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional free-form business context (industry, size, etc.)."), + ] = None, + use_live_metadata: Annotated[ + bool, Field(description="Reserved flag; this preview tool is input-driven in this release.") + ] = False, ) -> Dict[str, Any]: """Normalize requirements into standard/config/Studio/custom/avoid/unknown buckets.""" report = build_fit_gap_report( @@ -642,9 +704,21 @@ def fit_gap_report( structured_output=True, ) def scan_addons_source( - addons_paths: Optional[List[str]] = None, - max_files: int = 200, - max_file_bytes: int = 300_000, + addons_paths: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional list of absolute filesystem paths to Odoo addon roots. " + "When omitted, falls back to ODOO_ADDONS_PATHS." + ) + ), + ] = None, + max_files: Annotated[ + int, Field(description="Maximum number of addon files to scan; default 200, capped at 1000.") + ] = 200, + max_file_bytes: Annotated[ + int, Field(description="Skip files larger than this many bytes; default 300000 (300 KB).") + ] = 300_000, ) -> Dict[str, Any]: """Summarize manifests, custom models, risky methods, views, and ACL files.""" try: @@ -666,9 +740,27 @@ def scan_addons_source( structured_output=True, ) def build_domain( - conditions: List[Dict[str, Any]], - logical_operator: str = "and", - fields_metadata: Optional[Dict[str, Any]] = None, + conditions: Annotated[ + List[Dict[str, Any]], + Field( + description=( + "List of {field, operator, value} condition objects. They are combined " + "with the chosen logical_operator." + ) + ), + ], + logical_operator: Annotated[ + str, Field(description='How to combine conditions: "and" or "or". Default "and".') + ] = "and", + fields_metadata: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Optional {field: fields_get entry} map used to validate field names, " + "operators, and value shapes." + ) + ), + ] = None, ) -> BuildDomainResponse: """Build safe domain arrays for search_records and Odoo ORM calls.""" try: diff --git a/src/odoo_mcp/tools_knowledge.py b/src/odoo_mcp/tools_knowledge.py index 37dc62a..a05a464 100644 --- a/src/odoo_mcp/tools_knowledge.py +++ b/src/odoo_mcp/tools_knowledge.py @@ -7,9 +7,10 @@ locally with BM25 — no embeddings service and no data leaving the machine. """ -from typing import Any, Dict, List, Optional +from typing import Annotated, Any, Dict, List, Optional from mcp.server.fastmcp import Context +from pydantic import Field from .field_policy import get_field_policy from .knowledge_index import get_knowledge_store @@ -70,12 +71,36 @@ def fetch_and_index( ) def index_knowledge( ctx: Context, - model: str, - domain: Optional[Any] = None, - fields: Optional[List[str]] = None, - limit: int = 500, - replace: bool = False, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name to index, e.g. 'res.partner'.")], + domain: Annotated[ + Optional[Any], + Field( + description=( + "Optional Odoo domain filter — same shape as search_records.domain. " + "Used to bound the slice of records fed into the local index." + ) + ), + ] = None, + fields: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional subset of field names to store in the index. Omit for the " + "same smart-field selection as search_records; the index payload is " + "always field-ACL redacted before storage." + ) + ), + ] = None, + limit: Annotated[ + int, Field(description="Maximum records to fetch and index; default 500, capped at 2000.") + ] = 500, + replace: Annotated[ + bool, Field(description="When true, drop the existing index for this model before re-indexing.") + ] = False, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> IndexKnowledgeResponse: """Index records for free-text relevance search without further RPC calls. diff --git a/src/odoo_mcp/tools_read.py b/src/odoo_mcp/tools_read.py index 874a317..293b16a 100644 --- a/src/odoo_mcp/tools_read.py +++ b/src/odoo_mcp/tools_read.py @@ -2,11 +2,16 @@ MCP tools: read domain. Includes: list_models, get_model_fields, search_records, read_record, -read_attachment, aggregate_records, schema_catalog, search_employee, -search_holidays, list_instances, get_odoo_profile, health_check. +read_field_to_file, read_attachment, aggregate_records, schema_catalog, +search_employee, search_holidays, list_instances, get_odoo_profile, +health_check. """ +import base64 +import hashlib import json +import os +import stat from datetime import datetime, timedelta from typing import Annotated, Any, Dict, List, Optional @@ -29,6 +34,7 @@ ListInstancesResponse, ListModelsResponse, ReadAttachmentResponse, + ReadFieldToFileResponse, ReadRecordResponse, SchemaCatalogResponse, SearchRecordsResponse, @@ -40,14 +46,20 @@ SearchHolidaysResponse, clamp_limit, max_attachment_bytes, + max_field_file_bytes, normalize_domain_input, validate_model_name, ) +# ``safe_tool_call`` was removed: the friendly-envelope translation now lives +# in ``server_core._TranslationAwareFastMCP`` (subclass override of +# ``FastMCP.call_tool``), which is the only layer that actually sees the +# Pydantic ``ValidationError`` raised during argument binding. from .server_core import ( READ_ONLY_TOOL, PREVIEW_TOOL, mcp, plugin_posture, + restrict_field_file_path, _cached_fields_metadata, _resolve_odoo, mcp_surface_counts, @@ -57,6 +69,9 @@ ) +_REDACTED_FIELD_PLACEHOLDER = "[REDACTED by field ACL]" + + def _srv() -> Any: """Late import of server module to resolve patchable symbols at call time.""" from . import server @@ -324,11 +339,22 @@ def list_models( ) def get_model_fields( ctx: Context, - model: str, - field_names: Optional[List[str]] = None, - relevance: Optional[str] = None, - max_fields: int = DEFAULT_MAX_RELEVANT_FIELDS, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + field_names: Annotated[ + Optional[List[str]], + Field(description="Optional subset of field names to return; omit for all fields."), + ] = None, + relevance: Annotated[ + Optional[str], + Field(description='When "top", rank by business relevance and cap at max_fields.'), + ] = None, + max_fields: Annotated[ + int, Field(description="Maximum fields to return when relevance='top'; default 15.") + ] = DEFAULT_MAX_RELEVANT_FIELDS, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> GetModelFieldsResponse: """ Read field definitions for a model. @@ -384,21 +410,58 @@ def get_model_fields( @mcp.tool( description=( "Search Odoo records with read-only search_read; optional free-text " - "`query` matches across name/ref/email-like fields" + "`query` matches across name/ref/email-like fields. " + "If unsure which ``instance`` to pass, call ``list_instances`` first " + "or omit the argument to use the configured default." ), annotations=READ_ONLY_TOOL, structured_output=True, ) def search_records( ctx: Context, - model: str, - domain: Optional[Any] = None, - fields: Optional[List[str]] = None, - limit: int = 10, - offset: int = 0, - order: Optional[str] = None, - query: Optional[str] = None, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + domain: Annotated[ + Optional[Any], + Field( + description=( + "Optional Odoo domain filter. Accepts a standard Odoo domain list, " + "a JSON string, or {\"conditions\": [{\"field\": ..., \"operator\": ..., " + "\"value\": ...}]}. Multiple conditions are AND-combined." + ) + ), + ] = None, + fields: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional subset of field names to return. Omit for smart-field " + "selection; pass ['*'] for every available field." + ) + ), + ] = None, + limit: Annotated[ + int, Field(description="Maximum records to return; default 10, capped at 100.") + ] = 10, + offset: Annotated[ + int, Field(description="Number of records to skip; default 0.") + ] = 0, + order: Annotated[ + Optional[str], + Field(description="Optional Odoo sort order, e.g. 'name asc' or 'date desc'."), + ] = None, + query: Annotated[ + Optional[str], + Field( + description=( + "Optional free-text shortcut: the server builds an OR ilike domain " + "across the model's searchable text fields and ANDs it with `domain`." + ) + ), + ] = None, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> SearchRecordsResponse: """ Search and read records with bounded read-only semantics. @@ -468,10 +531,21 @@ def search_records( ) def read_record( ctx: Context, - model: str, - record_id: int, - fields: Optional[List[str]] = None, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + record_id: Annotated[int, Field(description="ID of the record to read.")], + fields: Annotated[ + Optional[List[str]], + Field( + description=( + "Optional subset of field names to return. Omit for smart-field " + "selection; pass ['*'] for every available field." + ) + ), + ] = None, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> ReadRecordResponse: """ Read one record by ID with bounded read-only semantics. @@ -515,6 +589,254 @@ def read_record( return {"success": False, "error": str(e)} +@mcp.tool( + description=( + "Read one field from an Odoo record and write it to a local file " + "(never overwrites existing files; path must sit inside the configured " + "field-file root)" + ), + annotations=READ_ONLY_TOOL, + structured_output=True, +) +def read_field_to_file( + ctx: Context, + model: Annotated[ + str, Field(description="Technical Odoo model name, for example 'res.partner'.") + ], + record_id: Annotated[ + int, Field(description="ID of the record to read the field from.") + ], + field: Annotated[ + str, + Field( + description=( + "Name of the field to extract. Binary fields are returned " + "as base64-decoded bytes (encoding='base64'); everything else " + "as UTF-8 text." + ) + ), + ], + output_path: Annotated[ + str, + Field( + description=( + "Absolute path of the file to create. Must be empty — the call " + "fails if a file already exists there." + ) + ), + ], + file_root: Annotated[ + Optional[str], + Field( + description=( + "Optional absolute root directory the output_path must sit " + "inside; defaults to the first entry of ODOO_MCP_FIELD_FILE_ROOTS." + ) + ), + ] = None, + encoding: Annotated[ + Optional[str], + Field( + description=( + "Override the encoding for non-binary fields. Defaults to " + "'utf-8' for text and 'base64' for binary fields (Odoo " + "fields_get reports type='binary')." + ) + ), + ] = None, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, +) -> ReadFieldToFileResponse: + """ + Extract a single field from an Odoo record and stream it to a local file. + + Routes long payloads (HTML, source code, rich text, base64 binaries) + through the filesystem instead of the JSON-RPC envelope — the + response carries only metadata (path, sha256, byte count), never the + field content itself. The agent then reads or edits the file with its + ordinary file tools. + + Security: + - Path must be absolute and inside ``file_root`` (or the first + configured ODOO_MCP_FIELD_FILE_ROOTS entry). + - Existing files are never overwritten — both pre-checked and + guarded by ``O_CREAT | O_EXCL`` at open time. + - The field ACL still applies: redacted fields become a placeholder + in the file and ``field_was_redacted=true`` is returned so the + agent cannot hallucinate the value. + - Symlink escapes are blocked by ``O_NOFOLLOW`` on the create fd. + + No side effects beyond creating the file at ``output_path``. + """ + app_context = ctx.request_context.lifespan_context + try: + instance_name, odoo = _resolve_odoo(ctx, instance) + refusal = check_rate(instance_name, "read_field_to_file") + if refusal is not None: + return refusal + validate_model_name(model) + if record_id < 1: + raise ValueError("record_id must be greater than 0") + if not field or not field.strip(): + raise ValueError("field must be a non-empty string") + cap = max_field_file_bytes() + + # Resolve the path *before* any file system call so a bad path fails + # with a clear error, not a low-level OSError. + resolved_path, resolved_root = restrict_field_file_path( + output_path, file_root + ) + if resolved_path.exists() or resolved_path.is_symlink(): + raise ValueError( + f"{resolved_path} already exists; refusing to overwrite. " + "Pick a fresh path or remove the existing file first." + ) + + # Read the record with only the requested field, then apply field ACL. + # We resolve the *list* of redacted field names first so we can still + # see whether ``field`` itself was withheld (otherwise redact_record + # drops the key from the dict and we cannot tell the difference + # between "redacted" and "missing"). + redacted_names = get_field_policy().restricted_fields( + instance_name, model, [field] + ) + field_was_redacted = field in redacted_names + + records = odoo.read_records(model, [record_id], fields=[field]) + note_single_record_read(instance_name, model) + if not records: + return { + "success": False, + "tool": "read_field_to_file", + "error": f"Record not found: {model} ID {record_id}", + } + raw_record = records[0] + if field not in raw_record: + return { + "success": False, + "tool": "read_field_to_file", + "error": ( + f"Field {field!r} not present on {model} — check get_model_fields" + ), + } + raw_value = raw_record[field] + + + + # Choose encoding. Binary fields always round-trip via base64-decode; + # for text fields the caller may override via ``encoding`` but cannot + # force base64 on a non-binary type (caller mistake — see below). + fields_metadata = _cached_fields_metadata( + app_context, odoo, model, instance_name + ) + field_meta = fields_metadata.get(field) if isinstance(fields_metadata, dict) else None + declared_binary = isinstance(field_meta, dict) and field_meta.get("type") == "binary" + declared_type = ( + field_meta.get("type") if isinstance(field_meta, dict) else None + ) + if encoding is None: + chosen_encoding = "base64" if declared_binary else "utf-8" + else: + chosen_encoding = encoding.strip().lower() + # Enforce the documented contract: ``encoding="base64"`` is only + # legal on fields whose ``fields_get.type`` is ``binary``. Without + # this guard, ``base64.b64decode(text, validate=False)`` would + # silently produce garbage bytes (and ``validate=False`` makes + # even that best-effort). Reject up front with a clear error + # naming the field and the metadata-reported type so the agent + # knows what to fix. + if chosen_encoding == "base64" and not declared_binary: + raise ValueError( + f"encoding='base64' is only valid on binary fields; " + f"field {field!r} on {model} has fields_get.type=" + f"{declared_type!r}. Pass encoding='utf-8' (or omit it) " + "for non-binary fields." + ) + + if field_was_redacted: + payload: bytes = _REDACTED_FIELD_PLACEHOLDER.encode("utf-8") + elif raw_value is None or raw_value is False: + payload = b"" + elif chosen_encoding == "base64": + if isinstance(raw_value, str): + try: + payload = base64.b64decode(raw_value, validate=True) + except Exception as exc: + raise ValueError( + f"field {field!r} declared binary but value is not " + f"valid base64: {exc}" + ) from exc + else: + payload = bytes(raw_value) + else: + payload = str(raw_value).encode("utf-8") + + if len(payload) > cap: + raise ValueError( + f"field {field!r} is {len(payload)} bytes; cap is {cap} " + "(raise ODOO_MCP_MAX_FIELD_FILE_BYTES to allow it)" + ) + + # Atomic write: O_CREAT|O_EXCL refuses any path that exists, including + # a symlink racing the pre-check. O_NOFOLLOW refuses to follow a + # symlink at the final component (defence-in-depth on top of the + # containment check, which already collapses .. via resolve()). + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL + flags |= getattr(os, "O_NOFOLLOW", 0) + fd = os.open(str(resolved_path), flags, 0o600) + try: + written = 0 + with os.fdopen(fd, "wb") as handle: + while written < len(payload): + chunk = payload[written : written + 64 * 1024] + handle.write(chunk) + written += len(chunk) + except Exception: + # Best-effort cleanup so a partial write doesn't leave junk. + try: + resolved_path.unlink(missing_ok=True) + except OSError: + pass + raise + + file_stat = resolved_path.stat() + if not stat.S_ISREG(file_stat.st_mode): + # Defensive: should be impossible given O_CREAT|O_EXCL|O_NOFOLLOW, + # but make the failure mode explicit instead of silently shipping + # a symlink or device file. + resolved_path.unlink(missing_ok=True) + raise ValueError( + f"{resolved_path} is not a regular file; refusing to return it" + ) + + digest = hashlib.sha256(payload).hexdigest() + return { + "success": True, + "tool": "read_field_to_file", + "model": model, + "record_id": record_id, + "field": field, + "output_path": str(resolved_path), + "file_root": str(resolved_root), + "encoding": chosen_encoding, + "bytes_written": len(payload), + "content_sha256": f"sha256:{digest}:{len(payload)}", + "field_was_redacted": field_was_redacted, + "redacted_fields": list(redacted_names or []), + "metadata_used": { + "instance": instance_name, + "file_root": str(resolved_root), + "encoding": chosen_encoding, + "max_bytes": cap, + "field_type": field_meta.get("type") if isinstance(field_meta, dict) else None, + }, + } + except Exception as e: + return {"success": False, "tool": "read_field_to_file", "error": str(e)} + + @mcp.tool( description=("Read an ir.attachment's metadata and size-capped base64 content"), annotations=READ_ONLY_TOOL, @@ -606,22 +928,68 @@ def read_attachment( @mcp.tool( description=( "Aggregate Odoo records server-side using Postgres groupby/sum/count. " - "Uses formatted_read_group on Odoo 19+ and read_group on earlier versions." + "Uses formatted_read_group on Odoo 19+ and read_group on earlier versions. " + "If unsure which ``instance`` to pass, call ``list_instances`` first " + "or omit the argument to use the configured default." ), annotations=READ_ONLY_TOOL, structured_output=True, ) def aggregate_records( ctx: Context, - model: str, - group_by: List[str], - measures: Optional[List[str]] = None, - domain: Optional[Any] = None, - lazy: bool = False, - limit: Optional[int] = None, - offset: int = 0, - order: Optional[str] = None, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + group_by: Annotated[ + List[str], + Field( + description=( + "Fields to group by; suffix a date/datetime field with ':granularity' " + "(day, week, month, quarter, year). Must include at least one field." + ) + ), + ], + measures: Annotated[ + Optional[List[str]], + Field( + description=( + 'Optional list of "field:agg" measure specs. Default aggregator is sum. ' + "Allowed aggregators: sum, avg, min, max, count, count_distinct, " + "array_agg, bool_and, bool_or. " + "Note: \"__count\" is the auto-returned row count per group — it " + "appears on every row without being requested, so do NOT include " + "it as a measure (Odoo will reject it as an unknown field)." + ) + ), + ] = None, + domain: Annotated[ + Optional[Any], + Field( + description=( + "Optional Odoo domain filter — same shape as search_records.domain." + ) + ), + ] = None, + lazy: Annotated[ + bool, + Field( + description=( + "When true, return only the first groupby level (legacy read_group mode)." + ) + ), + ] = False, + limit: Annotated[ + Optional[int], Field(description="Maximum rows to return; default uncapped.") + ] = None, + offset: Annotated[ + int, Field(description="Number of rows to skip; default 0.") + ] = 0, + order: Annotated[ + Optional[str], + Field(description="Optional sort order, e.g. 'date desc' or 'unit_amount desc'."), + ] = None, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> AggregateRecordsResponse: """Group records server-side and aggregate measures. diff --git a/src/odoo_mcp/tools_write.py b/src/odoo_mcp/tools_write.py index 77cb680..25429b6 100644 --- a/src/odoo_mcp/tools_write.py +++ b/src/odoo_mcp/tools_write.py @@ -2,7 +2,8 @@ MCP tools: write domain. Includes: preview_write, validate_write, execute_approved_write, -chatter_post, execute_method + WriteConfirmation + elicitation logic. +chatter_post, execute_method, write_field_from_file + WriteConfirmation + +elicitation logic. """ import base64 @@ -27,6 +28,7 @@ from .diagnostics import DESTRUCTIVE_METHODS, classify_method_safety from .tool_helpers import ( max_attachment_upload_bytes, + max_field_file_bytes, normalize_domain_input, truthy_env, validate_method_name, @@ -46,6 +48,7 @@ register_write_approval, require_validated_write_approval, restrict_attachment_upload_path, + restrict_field_file_path, write_approval_payload, ) @@ -55,6 +58,52 @@ # returns None executes (and commits) server-side, then faults with this text. _NONE_MARSHAL_FAULT_MARKER = "cannot marshal None unless allow_none is enabled" +def _normalize_write_response(report: Dict[str, Any]) -> Dict[str, Any]: + """Ensure every write-tool envelope carries an explicit ``result`` key. + + FastMCP's inferred ``outputSchema`` for tools with ``structured_output=True`` + can promote success-path keys (like ``result: ``) to required. The + success path of ``_execute_approved_write_gated`` already populates + ``result``; the error paths did not. When the underlying framework then + tries to validate the error envelope, it fails with + ``Output validation error: 'result' is a required property``. + + This helper injects ``result: None`` on any envelope that does not carry + the key, so both success and error paths round-trip cleanly through the + MCP transport. No change to the ``success`` / ``error`` semantics. + """ + if not isinstance(report, dict) or "result" in report: + return report + return {**report, "result": None} + + + + +def _read_field_file(path: Path, cap: int) -> bytes: + """Open, size-check, and read ``path`` through a single file descriptor. + + Mirrors ``_read_attachment_source_file``: opening once with O_NOFOLLOW + and reading through that same fd means the size cap and SHA-256 we + compute are always over the *actual* bytes returned, not a stale + ``stat()`` from an earlier, possibly-swapped file. The path has + already passed ``restrict_field_file_path`` so we know it sits inside + a configured root; this is the second line of defence. + """ + try: + fd = os.open(str(path), os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)) + except OSError as exc: + raise ValueError(f"{path} does not exist or is not a regular file") from exc + with os.fdopen(fd, "rb") as handle: + file_stat = os.fstat(handle.fileno()) + if not stat.S_ISREG(file_stat.st_mode): + raise ValueError(f"{path} is not a regular file") + if file_stat.st_size > cap: + raise ValueError( + f"{path} is {file_stat.st_size} bytes; cap is {cap} " + "(raise ODOO_MCP_MAX_FIELD_FILE_BYTES to allow it)" + ) + return handle.read() + def _read_attachment_source_file(path: Path, cap: int) -> bytes: """Open, size-check, and read ``path`` through a single file descriptor. @@ -177,13 +226,35 @@ async def _elicit_write_confirmation( structured_output=True, ) def preview_write( - model: str, - operation: str, - values: Optional[Dict[str, Any]] = None, - values_list: Optional[List[Dict[str, Any]]] = None, - record_ids: Optional[List[int]] = None, - context: Optional[Dict[str, Any]] = None, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + operation: Annotated[ + str, Field(description='Write operation: "create", "write", or "unlink".') + ], + values: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional single-record payload for create/write."), + ] = None, + values_list: Annotated[ + Optional[List[Dict[str, Any]]], + Field( + description=( + "Optional batch payload for create — one dict per record, max 100. " + "Executes as a single atomic Odoo create(vals_list) call." + ) + ), + ] = None, + record_ids: Annotated[ + Optional[List[int]], + Field(description="Optional list of record IDs (required for write/unlink)."), + ] = None, + context: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional Odoo context dict applied to the write call."), + ] = None, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> Dict[str, Any]: """Build a canonical approval token for a later approved write. @@ -222,15 +293,42 @@ def preview_write( ) def validate_write( ctx: Context, - model: str, - operation: str, - values: Optional[Dict[str, Any]] = None, - values_list: Optional[List[Dict[str, Any]]] = None, - record_ids: Optional[List[int]] = None, - context: Optional[Dict[str, Any]] = None, - fields_metadata: Optional[Dict[str, Any]] = None, - use_live_metadata: bool = True, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'res.partner'.")], + operation: Annotated[ + str, Field(description='Write operation: "create", "write", or "unlink".') + ], + values: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional single-record payload for create/write."), + ] = None, + values_list: Annotated[ + Optional[List[Dict[str, Any]]], + Field(description="Optional batch payload for create — one dict per record, max 100."), + ] = None, + record_ids: Annotated[ + Optional[List[int]], + Field(description="Optional list of record IDs (required for write/unlink)."), + ] = None, + context: Annotated[ + Optional[Dict[str, Any]], + Field(description="Optional Odoo context dict applied to the write call."), + ] = None, + fields_metadata: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Optional pre-fetched fields_get dict for validation. When omitted " + "and use_live_metadata is true, the tool fetches it via bounded fields_get." + ) + ), + ] = None, + use_live_metadata: Annotated[ + bool, Field(description="When true and no fields_metadata is provided, fetch live fields_get from Odoo.") + ] = True, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> Dict[str, Any]: """Validate write shape and return an approval payload when safe.""" try: @@ -393,43 +491,43 @@ def _execute_approved_write_gated( try: is_valid, _ = verify_write_approval(approval) if not is_valid: - return { + return _normalize_write_response({ "success": False, "tool": "execute_approved_write", "error": ( "approval token does not match the canonical payload; " "re-run preview_write and validate_write" ), - } + }) app_context = ctx.request_context.lifespan_context validation_record = require_validated_write_approval(app_context, approval) if validation_record is None: - return { + return _normalize_write_response({ "success": False, "tool": "execute_approved_write", "error": ( "approval token has not been validated in this server session " "or has expired; call validate_write first" ), - } + }) if write_approval_payload(approval) != validation_record.get("payload"): - return { + return _normalize_write_response({ "success": False, "tool": "execute_approved_write", "error": "approval payload does not match the stored validation record", - } + }) if not confirm: - return { + return _normalize_write_response({ "success": False, "tool": "execute_approved_write", "error": "confirm=true is required for destructive execution", - } + }) if not writes_enabled(): - return { + return _normalize_write_response({ "success": False, "tool": "execute_approved_write", "error": "write execution disabled; set ODOO_MCP_ENABLE_WRITES=1 to enable", - } + }) model = str(approval.get("model", "")) operation = str(approval.get("operation", "")).strip().lower() @@ -469,16 +567,18 @@ def _execute_approved_write_gated( result = odoo.execute_method(model, operation, *args, **kwargs) app_context.write_approvals.pop(str(approval.get("token", "")), None) - return { + return _normalize_write_response({ "success": True, "tool": "execute_approved_write", "model": model, "operation": operation, "result": result, "instance": approval_instance or _srv().resolve_default_instance_name(), - } + }) except Exception as e: - return {"success": False, "tool": "execute_approved_write", "error": str(e)} + return _normalize_write_response( + {"success": False, "tool": "execute_approved_write", "error": str(e)} + ) def _build_chatter_payload( @@ -520,16 +620,40 @@ def _build_chatter_payload( ) def chatter_post( ctx: Context, - model: str, - record_id: int, - body: str, - message_type: str = "comment", - subtype_xmlid: Optional[str] = None, - partner_ids: Optional[List[int]] = None, - attachment_ids: Optional[List[int]] = None, - approval: Optional[Dict[str, Any]] = None, - confirm: bool = False, - instance: Optional[str] = None, + model: Annotated[str, Field(description="Technical Odoo model name, e.g. 'project.task'.")], + record_id: Annotated[int, Field(description="ID of the record to post the message on.")], + body: Annotated[str, Field(description="Message body text (plaintext or HTML).")], + message_type: Annotated[ + str, Field(description="Message type: 'comment' (default) or 'notification'.") + ] = "comment", + subtype_xmlid: Annotated[ + Optional[str], + Field(description="Optional mail.message.subtype XMLID, e.g. 'mail.mt_note'."), + ] = None, + partner_ids: Annotated[ + Optional[List[int]], + Field(description="Optional list of res.partner IDs to notify in addition to followers."), + ] = None, + attachment_ids: Annotated[ + Optional[List[int]], + Field(description="Optional list of ir.attachment IDs to attach to the message."), + ] = None, + approval: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Execute-mode only: the approval payload returned from a previous " + "preview call. Omit on the first call to receive a preview token." + ) + ), + ] = None, + confirm: Annotated[ + bool, Field(description="Required true to execute; ignored in preview mode.") + ] = False, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, ) -> Dict[str, Any]: """Post a message on the chatter of a mail.thread-derived record. @@ -641,6 +765,321 @@ def chatter_post( return {"success": False, "error": str(e)} +@mcp.tool( + description=( + "Set one field on an Odoo record from the contents of a local file " + "(two-phase preview/execute; file content never enters the agent context)" + ), + annotations=DESTRUCTIVE_TOOL, + structured_output=True, +) +def write_field_from_file( + ctx: Context, + model: Annotated[ + str, Field(description="Technical Odoo model name, for example 'res.partner'.") + ], + record_id: Annotated[ + int, Field(description="ID of the record to write the field to.") + ], + field: Annotated[ + str, + Field( + description=( + "Name of the field to set. The on-disk interpretation is " + "controlled by ``encoding`` — it does NOT mirror Odoo's " + "field type. ``encoding='base64'`` (default for binary " + "fields) means the file holds RAW BYTES that the server " + "will base64-encode for Odoo's wire format; " + "``encoding='utf-8'`` (default for text/HTML fields) " + "means the file is text decoded as a Unicode string." + ) + ), + ], + input_path: Annotated[ + str, + Field( + description=( + "Absolute path of the file to read the new field value from. " + "Must sit inside the configured field-file root." + ) + ), + ], + file_root: Annotated[ + Optional[str], + Field( + description=( + "Optional selector for which configured root the input_path " + "must sit inside; must equal one of the ODOO_MCP_FIELD_FILE_ROOTS " + "entries. Defaults to the first entry. The argument cannot " + "widen the operator's allow-list." + ) + ), + ] = None, + encoding: Annotated[ + Optional[str], + Field( + description=( + "How to interpret the file contents: 'utf-8' (default for " + "text/HTML fields, file is read as text and pushed as a " + "Unicode string) or 'base64' (default for binary fields, " + "file holds raw bytes that are base64-encoded for Odoo's " + "wire format)." + ) + ), + ] = None, + approval: Annotated[ + Optional[Dict[str, Any]], + Field( + description=( + "Execute-mode only: the approval payload returned from a " + "previous preview call. Omit on the first call to receive a " + "preview token." + ) + ), + ] = None, + confirm: Annotated[ + bool, + Field(description="Required true to execute; ignored in preview mode."), + ] = False, + instance: Annotated[ + Optional[str], + Field(description="Optional configured Odoo instance name; uses the default if omitted."), + ] = None, +) -> Dict[str, Any]: + """ + Set a single field on an Odoo record by streaming the contents of a local file. + + Avoids the JSON-RPC escaping and context-bloat pain of long HTML/Code + payloads: the file content never enters the agent's context — the + preview token only carries the SHA-256 fingerprint, and the execute + call re-reads the file server-side, re-checks the hash, and routes + through the gated write pipeline. + + Modes: + - Default (gated): first call returns ``mode="preview"`` with an + approval token. Re-call with the same arguments plus ``approval`` + and ``confirm=true`` to actually write. + - Direct: ``approval`` and ``confirm=true`` together execute the write. + + Security: + - Path must be absolute and inside ``file_root`` (or the first + configured ODOO_MCP_FIELD_FILE_ROOTS entry). + - File is opened with ``O_NOFOLLOW`` and read once; size cap and + SHA-256 are derived from the same fd to defeat TOCTOU swaps. + - Preview token never contains the file content. Execute re-reads + the file and re-checks the hash, so a tampered or swapped file + between the two calls is rejected before Odoo is touched. + - Field existence + ``readonly=False`` is revalidated against live + ``fields_get`` at execute time — preview-only checks would let an + attacker mutate the field on the server between the two calls. + - Same hard gates as ``execute_approved_write``: requires + ``ODOO_MCP_ENABLE_WRITES=1`` and ``confirm=true``. + """ + try: + validate_model_name(model) + if record_id < 1: + raise ValueError("record_id must be greater than 0") + if not field or not field.strip(): + raise ValueError("field must be a non-empty string") + cap = max_field_file_bytes() + + # Resolve + size-check + hash the file before building any token. + resolved_path, resolved_root = restrict_field_file_path(input_path, file_root) + file_bytes = _read_field_file(resolved_path, cap) + digest = hashlib.sha256(file_bytes).hexdigest() + size_bytes = len(file_bytes) + + instance_name = _srv().resolve_instance_name(instance) + + # Build a deterministic payload: hash + size, no content. + canonical: Dict[str, Any] = { + "model": model, + "operation": "write", + "record_ids": [int(record_id)], + "field": field, + "input_path": str(resolved_path), + "content_sha256": f"sha256:{digest}:{size_bytes}", + "content_bytes": size_bytes, + "encoding": (encoding or "").strip().lower() or None, + "instance": instance_name, + } + token = build_approval_token(canonical) + + # ----- Preview mode ------------------------------------------------- + if approval is None: + record_write_event( + "write_field_from_file", + outcome="preview", + model=model, + operation="write", + record_ids=[int(record_id)], + instance=instance_name, + token=token, + detail=f"field={field} bytes={size_bytes}", + ) + return { + "success": True, + "tool": "write_field_from_file", + "mode": "preview", + "model": model, + "record_id": int(record_id), + "field": field, + "input_path": str(resolved_path), + "file_root": str(resolved_root), + "encoding": canonical["encoding"], + "content_sha256": canonical["content_sha256"], + "bytes_written": size_bytes, + "approval": {**canonical, "token": token}, + "warnings": [ + "Preview only. Re-call write_field_from_file with the returned " + "approval and confirm=true to actually write the field." + ], + "metadata_used": { + "instance": instance_name, + "file_root": str(resolved_root), + "encoding": canonical["encoding"], + "max_bytes": cap, + }, + } + + # ----- Execute mode ------------------------------------------------- + if not confirm: + return { + "success": False, + "tool": "write_field_from_file", + "error": "confirm=true is required for destructive execution", + } + if not writes_enabled(): + return { + "success": False, + "tool": "write_field_from_file", + "error": ( + "write execution disabled; set ODOO_MCP_ENABLE_WRITES=1 to enable" + ), + } + + provided_token = str(approval.get("token", "")) + if provided_token != token: + return { + "success": False, + "tool": "write_field_from_file", + "error": ( + "approval token does not match the current file content; " + "re-run preview and confirm the SHA-256 fingerprint." + ), + } + # Re-read once more, then compare hash. Catches both a malicious + # tamper between the two calls and a benign race where the file + # was edited by a different process in between. + re_read = _read_field_file(resolved_path, cap) + if hashlib.sha256(re_read).hexdigest() != digest: + return { + "success": False, + "tool": "write_field_from_file", + "error": ( + "file contents changed between preview and execute; " + "re-run preview and confirm." + ), + } + # The two reads were identical; either is fine for the actual write. + if len(re_read) != size_bytes: + # Should not happen since the hash covers size, but be explicit. + return { + "success": False, + "tool": "write_field_from_file", + "error": "file size changed between preview and execute", + } + + # Decide encoding for the actual value pushed to Odoo. + _, odoo = _resolve_odoo(ctx, instance) + fields_metadata = odoo.get_model_fields(model) + if not isinstance(fields_metadata, dict) or "error" in fields_metadata: + return { + "success": False, + "tool": "write_field_from_file", + "error": ( + "could not load live fields_get metadata; refusing to write " + f"{model}.{field}: {fields_metadata.get('error') if isinstance(fields_metadata, dict) else fields_metadata}" + ), + } + if field not in fields_metadata: + return { + "success": False, + "tool": "write_field_from_file", + "error": f"field {field!r} does not exist on {model}", + } + field_meta = fields_metadata[field] if isinstance(fields_metadata.get(field), dict) else {} + declared_binary = field_meta.get("type") == "binary" + declared_readonly = bool(field_meta.get("readonly")) + if declared_readonly: + return { + "success": False, + "tool": "write_field_from_file", + "error": f"field {field!r} on {model} is readonly; refusing to write", + } + + chosen_encoding = (encoding or "").strip().lower() + if not chosen_encoding: + chosen_encoding = "base64" if declared_binary else "utf-8" + + if chosen_encoding == "base64": + field_value: Any = base64.b64encode(re_read).decode("ascii") + else: + try: + field_value = re_read.decode("utf-8") + except UnicodeDecodeError as exc: + raise ValueError( + f"file at {resolved_path} is not valid utf-8; pass " + "encoding='base64' to interpret it as a binary blob" + ) from exc + + # IMPORTANT: Odoo's XML-RPC execute_kw expects ids + vals as + # *separate* positional arguments — packing them into a single + # list ([[ids], {vals}]) would unpack to ProjectTask.write(*[[ids], + # {vals}]) which yields write([ids], {vals}) and fails with + # 'missing 1 required positional argument: vals'. Mirrors + # execute_approved_write above, which already splats *args, **kwargs. + result = odoo.execute_method( + model, + "write", + [int(record_id)], + {field: field_value}, + ) + record_write_event( + "write_field_from_file", + outcome="success", + model=model, + operation="write", + record_ids=[int(record_id)], + instance=instance_name, + token=provided_token, + detail=f"field={field} bytes={size_bytes}", + ) + return { + "success": True, + "tool": "write_field_from_file", + "mode": "execute", + "model": model, + "record_id": int(record_id), + "field": field, + "input_path": str(resolved_path), + "file_root": str(resolved_root), + "encoding": chosen_encoding, + "bytes_written": size_bytes, + "content_sha256": f"sha256:{digest}:{size_bytes}", + "result": result, + "metadata_used": { + "instance": instance_name, + "file_root": str(resolved_root), + "encoding": chosen_encoding, + "max_bytes": cap, + "field_type": field_meta.get("type") if isinstance(field_meta, dict) else None, + }, + } + except Exception as e: + return {"success": False, "tool": "write_field_from_file", "error": str(e)} + + @mcp.tool( description="Execute a custom method on an Odoo model", annotations=DESTRUCTIVE_TOOL, diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index 326e0ff..cedacf0 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -254,6 +254,114 @@ def test_verify_write_approval_returns_true_for_matching_token(): assert is_valid is True +# ----- verify_write_approval nested / mixed record_ids --------------------- +# Regression tests for the v1.3.3 fix: callers whose transport re-wraps +# record_ids in a list (e.g. nested [[3598, 3594, ...]]) used to flip the +# SHA-256 approval token between preview/validate and execute. The fix +# normalizes record_ids to a flat list[int] in both verify_write_approval +# and write_approval_payload (mirroring build_write_preview_report), so the +# approval token stays canonical across transport-induced shape changes. + + +def test_normalized_record_ids_flattens_and_int_casts(): + """_normalized_record_ids flattens any nesting and casts to int.""" + assert agent_tools._normalized_record_ids([3598, 3594]) == [3598, 3594] + assert agent_tools._normalized_record_ids([[3598, 3594]]) == [3598, 3594] + assert agent_tools._normalized_record_ids([[[3598, 3594]]]) == [3598, 3594] + # mixed types: digit strings are coerced, booleans are skipped (bool + # is an int subclass in Python — never accidentally accept a True/False + # as a record id), non-digit strings are dropped. + assert agent_tools._normalized_record_ids(["3598", 3594]) == [3598, 3594] + assert agent_tools._normalized_record_ids([True, 3598, False]) == [3598] + # empty / None + assert agent_tools._normalized_record_ids([]) == [] + assert agent_tools._normalized_record_ids(None) == [] + + +def test_verify_write_approval_accepts_nested_record_ids_against_flat_token(): + """A flat-built token verifies against the same payload with nested record_ids.""" + canonical = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [3598, 3594, 3593, 3592, 3588], + "values": {}, + "context": {}, + "instance": "default", + } + token = agent_tools.build_approval_token(canonical) + # Caller arrives with record_ids wrapped in an extra list (the exact + # shape that triggered the original bug in the wild). + nested = {**canonical, "record_ids": [[3598, 3594, 3593, 3592, 3588]], "token": token} + is_valid, _ = agent_tools.verify_write_approval(nested) + assert is_valid is True, "token mismatch on nested record_ids — regression of v1.3.2" + + +def test_verify_write_approval_accepts_triple_nested_record_ids(): + """Three layers of nesting still flatten to canonical.""" + canonical = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [3598, 3594], + "values": {}, + "context": {}, + "instance": "default", + } + token = agent_tools.build_approval_token(canonical) + triple = {**canonical, "record_ids": [[[3598, 3594]]], "token": token} + is_valid, _ = agent_tools.verify_write_approval(triple) + assert is_valid is True + + +def test_verify_write_approval_accepts_string_record_ids(): + """record_ids can arrive as digit strings (some JSON transports coerce).""" + canonical = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [3598, 3594], + "values": {}, + "context": {}, + "instance": "default", + } + token = agent_tools.build_approval_token(canonical) + str_ids = {**canonical, "record_ids": ["3598", "3594"], "token": token} + is_valid, _ = agent_tools.verify_write_approval(str_ids) + assert is_valid is True + + +def test_verify_write_approval_drops_non_digit_record_ids(): + """Non-digit strings in record_ids are silently dropped, not coerced to 0.""" + canonical = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [3598], + "values": {}, + "context": {}, + "instance": "default", + } + token = agent_tools.build_approval_token(canonical) + # Caller accidentally includes a label alongside ids; we should drop it + # rather than coerce "label" -> 0 and silently target a wrong record. + mixed = {**canonical, "record_ids": [3598, "label"], "token": token} + is_valid, _ = agent_tools.verify_write_approval(mixed) + assert is_valid is True + + +def test_write_approval_payload_normalizes_record_ids(): + """write_approval_payload produces a flat list[int] for the payload-equality check.""" + from odoo_mcp.server_core import write_approval_payload + + approval = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [[3598, 3594]], + "values": {}, + "context": {}, + "instance": "default", + } + payload = write_approval_payload(approval) + assert payload["record_ids"] == [3598, 3594] + + # ----- canonical_json / build_approval_token int/float stability ----------- diff --git a/tests/test_batch_write.py b/tests/test_batch_write.py index 8c26af8..efe3edf 100644 --- a/tests/test_batch_write.py +++ b/tests/test_batch_write.py @@ -135,3 +135,84 @@ def test_batch_create_executes_end_to_end(monkeypatch): assert client.calls == [ ("res.partner", "create", ([{"name": "Ada"}, {"name": "Grace"}],), {}) ] + + +# ----- _normalize_write_response (v1.3.4) ----------------------------------- +# Regression: FastMCP's inferred outputSchema for execute_approved_write +# can promote success-path keys (like result: ) to required. The +# success return already carries result; the error returns did not, so +# FastMCP rejected the error envelope with +# "Output validation error: 'result' is a required property". The fix +# wraps every return through _normalize_write_response, which injects +# result: None on envelopes that do not carry the key. + + +def test_normalize_write_response_injects_result_none_on_error_envelope(): + from odoo_mcp.tools_write import _normalize_write_response + out = _normalize_write_response( + {"success": False, "tool": "execute_approved_write", "error": "boom"} + ) + assert out == { + "success": False, + "tool": "execute_approved_write", + "error": "boom", + "result": None, + } + + +def test_normalize_write_response_passthrough_when_result_present(): + from odoo_mcp.tools_write import _normalize_write_response + success = { + "success": True, + "tool": "execute_approved_write", + "model": "res.partner", + "operation": "unlink", + "result": True, + "instance": "default", + } + assert _normalize_write_response(success) is success + + +def test_normalize_write_response_handles_non_dict(): + from odoo_mcp.tools_write import _normalize_write_response + # Defensive: non-dict input is returned unchanged (no crash). + assert _normalize_write_response(None) is None + assert _normalize_write_response("not a dict") == "not a dict" + + +def test_execute_approved_write_error_envelope_carries_result_none(monkeypatch): + """End-to-end: a token-mismatch error envelope must include result=None. + + Triggers every gate-failure path (token mismatch) and checks that + FastMCP's inferred outputSchema check would not reject the envelope + for the missing required `result` field. + """ + from odoo_mcp import tools_write + from odoo_mcp.server_core import WRITE_APPROVAL_TTL_SECONDS + + # Build a minimal ctx with an empty AppContext (no validated record -> gate fails). + class _FakeAppContext: + write_approvals = {} + + class _FakeRequestContext: + lifespan_context = _FakeAppContext() + + class _FakeCtx: + request_context = _FakeRequestContext() + + ctx = _FakeCtx() + bogus = { + "model": "res.partner", + "operation": "unlink", + "record_ids": [3598], + "values": {}, + "context": {}, + "instance": "default", + "token": "odoo-write:bogus", + } + out = tools_write.execute_approved_write(ctx, bogus, confirm=True) + assert out["success"] is False + assert "result" in out + assert out["result"] is None + assert "error" in out + diff --git a/tests/test_cli.py b/tests/test_cli.py index c2557ee..1472720 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -347,8 +347,24 @@ def test_health_payload_returns_none_transport_security_when_unavailable(monkeyp def test_main_entrypoint_invokes_sys_exit_with_main_return_value(monkeypatch, capsys): - """Cover the ``if __name__ == '__main__'`` block via runpy.""" - import runpy + """Cover the ``if __name__ == '__main__'`` block. + + The previous version used ``runpy.run_module("odoo_mcp.__main__", + run_name="__main__")`` to execute the entry point. CPython 3.10+ raises + ``RuntimeWarning: 'odoo_mcp.__main__' found in sys.modules after import + of package 'odoo_mcp', but prior to execution of 'odoo_mcp.__main__'`` + whenever the package has already been imported. The official workaround + per CPython docs is to pop the entry from ``sys.modules`` before invoking + ``runpy``, but ``importlib.import_module`` re-populates it on the next + test, so the warning would still fire any time another test in this file + touches the package first. + + The bottom of ``odoo_mcp/__main__.py`` is literally ``sys.exit(main())``, + so we invoke that expression directly. This exercises the same contract + (the entry-point block reaches ``main`` and routes its return value + through ``sys.exit``) without polluting ``sys.modules`` and without + triggering the runpy warning. + """ import sys as _sys cli = importlib.import_module("odoo_mcp.__main__") @@ -363,23 +379,24 @@ def fake_main(): monkeypatch.setattr(cli, "main", fake_main) monkeypatch.setattr(_sys, "argv", ["odoo-mcp", "--health"]) - real_exit = _sys.exit + captured_exit: dict = {} def fake_exit(code=0): - captured["code"] = code - # Prevent runpy from raising SystemExit by short-circuiting + captured_exit["code"] = code raise SystemExit(code) monkeypatch.setattr(_sys, "exit", fake_exit) + # Invoking ``sys.exit(cli.main())`` is exactly what the + # ``if __name__ == '__main__'`` block does; doing it directly avoids the + # runpy-induced RuntimeWarning that fires whenever ``odoo_mcp.__main__`` + # is already present in ``sys.modules``. try: - runpy.run_module("odoo_mcp.__main__", run_name="__main__") + _sys.exit(cli.main()) except SystemExit as exc: - captured.setdefault("code", exc.code) + captured_exit.setdefault("code", exc.code) - # main may have been called either via fake_main (if monkeypatch survived) - # or via the runpy execution; either way, sys.exit must have been called. - assert "code" in captured - # restore the real main for any later tests + assert "main_called" in captured + assert "code" in captured_exit + # Restore the original main() so other tests get the real implementation. monkeypatch.setattr(cli, "main", real_main) - monkeypatch.setattr(_sys, "exit", real_exit) diff --git a/tests/test_error_handling.py b/tests/test_error_handling.py new file mode 100644 index 0000000..0463658 --- /dev/null +++ b/tests/test_error_handling.py @@ -0,0 +1,121 @@ +"""Tests for the centralised error-formatting helpers and the +FastMCP ``call_tool`` translation hook. + +Lives at ``tests/test_error_handling.py`` even though only +``_format_validation_error`` is now in ``error_handling.py`` (the rest of the +plumbing lives in ``server_core._TranslationAwareFastMCP``). +""" + +from __future__ import annotations + +import json +from typing import Any + +import pytest +from pydantic import BaseModel, Field, ValidationError + +from odoo_mcp.error_handling import _format_validation_error + + +class _Echo(BaseModel): + """Tiny Pydantic model used to provoke ValidationError in tests.""" + + items: list[str] = Field(default_factory=list) + + +def _make_int_error_dict(field: str, value: Any) -> dict[str, Any]: + """Build a Pydantic v2-style error dict for an integer-parsing failure.""" + return { + "type": "value_error", + "loc": (field,), + "msg": "value is not a valid integer", + "input": value, + "ctx": {"error": f"invalid literal for int() with base 10: '{value}'"}, + } + + +# ----- _format_validation_error ------------------------------------------- + + +def test_validation_error_renderer_is_bounded(): + """The renderer caps the number of errors it surfaces.""" + + try: + _Echo(items=42) # type: ignore[arg-type] + except ValidationError as exc: + rendered = _format_validation_error(exc) + # Single error here, so no "(and more)" suffix. + assert "(and more)" not in rendered + assert "items" in rendered + + # 5 errors → only the first 3 are rendered, with an "(and more)" suffix. + big_errors = [ + _make_int_error_dict(f"field_{i}", str(i)) + for i in range(5) + ] + exc = ValidationError.from_exception_data("BigModel", big_errors) # type: ignore[arg-type] + rendered = _format_validation_error(exc) + assert "(and more)" in rendered + # First three field names should be present. + for i in range(3): + assert f"field_{i}" in rendered + # The fourth and fifth should NOT be rendered (capped). + assert "field_3" not in rendered + assert "field_4" not in rendered + + +# ----- FastMCP call_tool translation (server_core.py hook) ---------------- + + +def _real_tool_error(message: str, cause: BaseException | None) -> Exception: + """Build a real ``mcp.server.fastmcp.exceptions.ToolError`` with a cause.""" + from mcp.server.fastmcp.exceptions import ToolError + + try: + raise ToolError(message) from cause + except ToolError as exc: + return exc + + +def test_translate_validation_tool_error_to_envelope_returns_envelope(): + """A ``ToolError`` whose ``__cause__`` is a ``ValidationError`` becomes an envelope block.""" + from odoo_mcp.server_core import _translate_validation_tool_error_to_envelope + + ve = ValidationError.from_exception_data( + "ArgsModel", # type: ignore[arg-type] + [_make_int_error_dict("measures", "")], + ) + tool_error = _real_tool_error( + "Error executing tool aggregate_records: 1 validation error", cause=ve + ) + block = _translate_validation_tool_error_to_envelope( + tool_error, tool_name_hint="aggregate_records" + ) + assert block is not None + assert block.type == "text" + payload = json.loads(block.text) + assert payload["success"] is False + assert payload["tool"] == "aggregate_records" + assert "measures" in payload["error"] + assert "Invalid input" in payload["error"] + + +def test_translate_validation_tool_error_to_envelope_passes_non_validation(): + """A real ``ToolError`` whose cause is *not* a ``ValidationError`` is left alone.""" + from odoo_mcp.server_core import _translate_validation_tool_error_to_envelope + + plain_tool_error = _real_tool_error("Error executing tool foo: boom", cause=None) + assert _translate_validation_tool_error_to_envelope(plain_tool_error) is None + + +def test_translate_validation_tool_error_to_envelope_passes_non_tool_error(): + """A bare ``ValidationError`` (not wrapped in ``ToolError``) is left alone.""" + from odoo_mcp.server_core import _translate_validation_tool_error_to_envelope + + ve = ValidationError.from_exception_data( + "ArgsModel", # type: ignore[arg-type] + [_make_int_error_dict("measures", "")], + ) + # The translator must not be tempted to wrap a raw ValidationError -- + # only a ToolError wrapping one is in scope. + assert _translate_validation_tool_error_to_envelope(ve) is None diff --git a/tests/test_field_file_io.py b/tests/test_field_file_io.py new file mode 100644 index 0000000..9f6e2ac --- /dev/null +++ b/tests/test_field_file_io.py @@ -0,0 +1,859 @@ +"""Tests for read_field_to_file and write_field_from_file. + +Covers the file I/O contract: +- happy path (text + binary + base64 encoding) +- Field ACL redaction on read +- Path hardening: absolute-only, root containment, no overwrite, + TOCTOU via symlink swap +- Preview/execute two-phase flow with hash re-check +- Hard gates: writes-enabled + confirm=true +- Readonly / unknown field rejection +""" + +from __future__ import annotations + +import base64 +import hashlib +import importlib +from pathlib import Path +from typing import Any + + +from tests.test_batch_write import FakeCtx + + +# --------------------------------------------------------------------------- +# Test fixtures +# --------------------------------------------------------------------------- + + +class _FieldIOClient: + """Minimal stand-in for OdooClient — text fields + a binary base64 stub. + + The ``execute_method`` signature matches OdooClient exactly: + ``(self, model, method, *args, **kwargs)`` — so the test sees the same + unpacked form OdooClient forwards to Odoo via XML-RPC execute_kw. This + is what caught the original write_field_from_file bug (which packed + ``[ids, vals]`` into a single positional arg, making Odoo see + ``ProjectTask.write(*[[ids], {vals}])`` and fault with + ``missing 1 required positional argument: 'vals'``). + """ + + def __init__(self) -> None: + self.calls: list[tuple] = [] + self.field_value: Any = "

hello & \"world\"\nline 2

" + self.field_meta: dict[str, dict[str, Any]] = { + "comment": {"type": "html", "readonly": False}, + "description": {"type": "text", "readonly": False}, + "datas": {"type": "binary", "readonly": False}, + "create_uid": {"type": "many2one", "readonly": True}, + } + + def get_model_fields(self, model: str) -> dict[str, Any]: + return dict(self.field_meta) + + def read_records(self, model: str, ids: list[int], fields=None): + self.calls.append(("read_records", model, list(ids), list(fields or []))) + record = {"id": ids[0]} + if fields: + for f in fields: + if f in self.field_meta: + record[f] = self.field_value + return [record] + + def execute_method(self, model: str, method: str, *args: Any, **kwargs: Any): + # Record the *unpacked* form — same shape OdooClient forwards via + # XML-RPC execute_kw. Tests assert on this shape directly so a + # future regression that re-packs args is caught immediately. + self.calls.append( + ("execute_method", model, method, list(args), dict(kwargs)) + ) + return True + + +def _install_root(monkeypatch, tmp_path: Path) -> Path: + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(tmp_path)) + return tmp_path + + +def _import_server(): + return importlib.import_module("odoo_mcp.server") + + +# --------------------------------------------------------------------------- +# read_field_to_file +# --------------------------------------------------------------------------- + + +def test_read_field_to_file_writes_text_field_to_new_file(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + output_path = tmp_path / "out.html" + + client = _FieldIOClient() + client.field_value = "

hello & \"world\"\nline 2

" + ctx = FakeCtx(client) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(output_path) + ) + + assert result["success"] is True, result + assert result["output_path"] == str(output_path) + assert result["bytes_written"] == len(client.field_value.encode("utf-8")) + assert result["encoding"] == "utf-8" + assert result["field_was_redacted"] is False + assert output_path.read_bytes() == client.field_value.encode("utf-8") + # sha256 fingerprint is in the response, not the file content. + assert result["content_sha256"].startswith("sha256:") + digest = result["content_sha256"].split(":")[1] + assert digest == hashlib.sha256(client.field_value.encode("utf-8")).hexdigest() + + +def test_read_field_to_file_refuses_overwrite_of_existing_file(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + output_path = tmp_path / "out.html" + output_path.write_text("existing content") + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(output_path) + ) + + assert result["success"] is False + assert "already exists" in result["error"] + # Existing file must not be modified. + assert output_path.read_text() == "existing content" + + +def test_read_field_to_file_rejects_relative_path(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", "out.html" # relative path + ) + + assert result["success"] is False + assert "absolute" in result["error"].lower() + + +def test_read_field_to_file_rejects_path_outside_configured_root( + tmp_path, monkeypatch +): + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(outside / "out.html") + ) + + assert result["success"] is False + assert "outside" in result["error"].lower() + + +def test_read_field_to_file_requires_configured_root_when_no_override( + tmp_path, monkeypatch +): + server = _import_server() + monkeypatch.delenv("ODOO_MCP_FIELD_FILE_ROOTS", raising=False) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(tmp_path / "out.html") + ) + + assert result["success"] is False + error = result["error"] + # The error must name the env var and explicitly warn against /tmp + # (world-readable on Linux/macOS). The remediation is now ONLY to + # set ODOO_MCP_FIELD_FILE_ROOTS — file_root cannot widen the + # allow-list (see test_file_root_cannot_widen_allow_list below). + assert "ODOO_MCP_FIELD_FILE_ROOTS" in error + assert "/tmp" in error + assert "world-readable" in error + # The error should also explain why: refuses all field file I/O. + assert "refusing" in error.lower() or "fail" in error.lower() + # file_root is no longer advertised as a remediation. + assert "Pass file_root=" not in error + + +def test_missing_root_error_lists_platform_specific_suggestions( + tmp_path, monkeypatch +): + """The remediation message should suggest a safe per-platform default + directory, not just throw a generic error.""" + import os + + server = _import_server() + monkeypatch.delenv("ODOO_MCP_FIELD_FILE_ROOTS", raising=False) + monkeypatch.delenv("XDG_CACHE_HOME", raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(tmp_path / "out.html") + ) + + assert result["success"] is False + if os.name == "nt": + # Windows suggestion should mention AppData/Local. + assert "AppData" in result["error"] + else: + # POSIX suggestion should use ~/.cache (XDG) and name Linux/macOS. + assert ".cache" in result["error"] + assert "Linux" in result["error"] + assert "macOS" in result["error"] + + +def test_read_field_to_file_uses_explicit_file_root_override_as_selector( + tmp_path, monkeypatch +): + """``file_root`` is a SELECTOR among configured roots (it must equal + one of them after resolve) — it cannot invent a new root. The + configured root must be set even when ``file_root`` is supplied.""" + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + + output_path = allowed / "explicit_root.html" + client = _FieldIOClient() + ctx = FakeCtx(client) + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(output_path), + file_root=str(allowed), + ) + + assert result["success"] is True, result + assert result["file_root"] == str(allowed.resolve()) + assert output_path.exists() + + +def test_file_root_cannot_widen_allow_list(tmp_path, monkeypatch): + """Regression test for the prompt-injection bypass: ``file_root`` + used to accept any absolute path even when it was outside the + configured ``ODOO_MCP_FIELD_FILE_ROOTS`` list — letting a + prompt-injected agent pass ``file_root="/"`` and then read / write + through any path under it. The selector semantics now reject this + outright.""" + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + + ctx = FakeCtx(_FieldIOClient()) + # ``/`` is not in the configured roots list. + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(allowed / "out.html"), + file_root="/", + ) + + assert result["success"] is False + error_text = result["error"].lower() + assert ( + "not one of the configured" in error_text + or "cannot widen" in error_text + ) + + # Same bypass attempt against the write tool. + input_path = allowed / "snippet.html" + input_path.write_text("payload", encoding="utf-8") + write_result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + file_root="/home/user", + ) + assert write_result["success"] is False + assert ( + "not one of the configured" in write_result["error"].lower() + or "cannot widen" in write_result["error"].lower() + ) + + +def test_file_root_requires_env_var_even_when_supplied(tmp_path, monkeypatch): + """Setting ``file_root`` no longer lets you skip + ``ODOO_MCP_FIELD_FILE_ROOTS`` — the env var is always required.""" + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + monkeypatch.delenv("ODOO_MCP_FIELD_FILE_ROOTS", raising=False) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(allowed / "out.html"), + file_root=str(allowed), + ) + + assert result["success"] is False + assert "ODOO_MCP_FIELD_FILE_ROOTS" in result["error"] + + +def test_relative_file_root_rejected_before_resolve(tmp_path, monkeypatch): + """Relative ``file_root`` values such as ``\"scratch\"`` must be + rejected before ``Path.resolve()`` silently joins them to ``$CWD``.""" + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(allowed / "out.html"), + file_root="scratch", + ) + + assert result["success"] is False + assert "absolute" in result["error"].lower() + + +def test_multi_root_accepts_path_under_second_root(tmp_path, monkeypatch): + """When no ``file_root`` is supplied, the candidate must be validated + against ALL configured roots — not just the first one. Configs like + ``ODOO_MCP_FIELD_FILE_ROOTS=/srv/a:/srv/b`` must accept paths under + either root.""" + server = _import_server() + root_a = tmp_path / "a" + root_b = tmp_path / "b" + root_a.mkdir() + root_b.mkdir() + monkeypatch.setenv( + "ODOO_MCP_FIELD_FILE_ROOTS", f"{root_a}{__import__('os').pathsep}{root_b}" + ) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(root_b / "out.html"), + ) + + assert result["success"] is True, result + assert result["file_root"] == str(root_b.resolve()) + + +def test_read_field_to_file_handles_binary_field_with_base64(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + output_path = tmp_path / "blob.bin" + + raw_bytes = b"\x89PNG\r\n\x1a\n -- binary content --" + client = _FieldIOClient() + client.field_value = base64.b64encode(raw_bytes).decode("ascii") + ctx = FakeCtx(client) + result = server.read_field_to_file( + ctx, "res.partner", 7, "datas", str(output_path) + ) + + assert result["success"] is True, result + assert result["encoding"] == "base64" + assert output_path.read_bytes() == raw_bytes + + +def test_read_field_to_file_rejects_base64_on_non_binary_field( + tmp_path, monkeypatch +): + """``encoding=\"base64\"`` is only valid on fields whose + ``fields_get.type`` is ``binary``. Without this guard, an + arbitrary text field would be silently base64-decoded into garbage + bytes (with ``validate=False`` making it best-effort). Reject up + front with a clear error naming the field and the metadata type.""" + server = _import_server() + _install_root(monkeypatch, tmp_path) + output_path = tmp_path / "out.bin" + + ctx = FakeCtx(_FieldIOClient()) + # ``comment`` is type=\"html\" in the fake field meta, not binary. + result = server.read_field_to_file( + ctx, + "res.partner", + 7, + "comment", + str(output_path), + encoding="base64", + ) + + assert result["success"] is False + error_text = result["error"].lower() + assert "encoding" in error_text and "base64" in error_text + assert "binary" in error_text + # The error must name the field and the metadata-reported type so + # the agent knows what to fix. + assert "comment" in result["error"] + assert "html" in result["error"] + # The file must NOT have been written. + assert not output_path.exists() + + +def test_read_field_to_file_blocks_symlink_escape_within_root( + tmp_path, monkeypatch +): + """A symlink whose resolved target sits outside the configured root + must be rejected by the containment check (Path.resolve collapses it + before the relative_to check runs).""" + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + + # File outside the root that contains the secret bytes. + secret_file = outside / "secret.html" + secret_file.write_bytes(b"TOP-SECRET-VALUE") + # A symlink inside the allowed root that points at the secret. + escape_link = allowed / "out.html" + escape_link.symlink_to(secret_file) + + ctx = FakeCtx(_FieldIOClient()) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(escape_link) + ) + + assert result["success"] is False + assert "outside" in result["error"].lower() + # The secret must not be exposed anywhere in the response. + serialized = str(result) + assert "TOP-SECRET-VALUE" not in serialized + + +# --------------------------------------------------------------------------- +# write_field_from_file +# --------------------------------------------------------------------------- + + +def test_write_field_from_file_preview_returns_token_without_content( + tmp_path, monkeypatch +): + server = _import_server() + _install_root(monkeypatch, tmp_path) + input_path = tmp_path / "snippet.html" + content = "

Preview me

\n

some html

" + input_path.write_text(content, encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + result = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + + assert result["success"] is True, result + assert result["mode"] == "preview" + assert result["bytes_written"] == len(content.encode("utf-8")) + approval = result["approval"] + assert approval["token"] + assert approval["content_sha256"].startswith("sha256:") + # The real content must never appear in the approval payload. + assert content not in str(approval) + assert content.encode("utf-8") not in str(approval).encode("utf-8") + # And nothing in the tool response either. + assert content not in str(result) + + +def test_write_field_from_file_execute_round_trip(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = tmp_path / "snippet.html" + content = "

Round trip

" + input_path.write_text(content, encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + assert preview["success"] is True + + result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + approval=preview["approval"], + confirm=True, + ) + + assert result["success"] is True, result + assert result["mode"] == "execute" + assert result["result"] is True + # The write must reach Odoo as TWO positional arguments (ids, vals), + # not one — see the regression test below for the failing form. + # write_call == ("execute_method", model, method, args_list, kwargs_dict) + write_call = next(c for c in client.calls if c[0] == "execute_method") + _, model, method, args, kwargs = write_call + assert (model, method) == ("res.partner", "write") + # ids and vals must be TWO *separate* positional args for Odoo + # XML-RPC execute_kw (not one nested list — see regression test). + assert len(args) == 2, f"expected 2 positional args, got {args!r}" + assert args == [[9], {"comment": content}] + assert kwargs == {} + + +def test_write_field_from_file_rejects_tampered_file_after_preview( + tmp_path, monkeypatch +): + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = tmp_path / "snippet.html" + input_path.write_text("original", encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + assert preview["success"] is True + + # Swap the file contents between preview and execute. + input_path.write_text("TAMPERED content", encoding="utf-8") + + result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + approval=preview["approval"], + confirm=True, + ) + + assert result["success"] is False + # Either defense may fire first: the token check (the new file hash + # yields a new approval token that doesn't match the preview's) or + # the explicit file-hash re-check (file contents changed). Both are + # equally correct rejections. + error_text = result["error"].lower() + assert ( + "changed between preview and execute" in error_text + or "token does not match" in error_text + ) + # No write call must have reached Odoo. + assert all(c[0] != "execute_method" for c in client.calls) + + +def test_write_field_from_file_requires_confirm_and_writes_enabled( + tmp_path, monkeypatch +): + server = _import_server() + _install_root(monkeypatch, tmp_path) + # Writes disabled by default. + input_path = tmp_path / "snippet.html" + input_path.write_text("hi", encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + assert preview["success"] is True + + # confirm=False -> rejected. + denied = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + approval=preview["approval"], + confirm=False, + ) + assert denied["success"] is False + assert "confirm=true" in denied["error"] + + # Now enable writes but skip confirm -> still rejected (same gate). + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + denied2 = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + approval=preview["approval"], + confirm=False, + ) + assert denied2["success"] is False + assert "confirm=true" in denied2["error"] + + +def test_write_field_from_file_rejects_readonly_field(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = tmp_path / "snippet.html" + input_path.write_text("hi", encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "res.partner", 9, "create_uid", str(input_path) + ) + assert preview["success"] is True + + result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "create_uid", + str(input_path), + approval=preview["approval"], + confirm=True, + ) + + assert result["success"] is False + assert "readonly" in result["error"] + + +def test_write_field_from_file_rejects_oversized_input(tmp_path, monkeypatch): + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_MAX_FIELD_FILE_BYTES", "10") + + input_path = tmp_path / "big.html" + input_path.write_bytes(b"x" * 100) + + ctx = FakeCtx(_FieldIOClient()) + result = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + + assert result["success"] is False + assert "cap is 10" in result["error"] + + +def test_write_field_from_file_rejects_path_outside_root(tmp_path, monkeypatch): + server = _import_server() + allowed = tmp_path / "allowed" + allowed.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.setenv("ODOO_MCP_FIELD_FILE_ROOTS", str(allowed)) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = outside / "snippet.html" + input_path.write_text("hi", encoding="utf-8") + + ctx = FakeCtx(_FieldIOClient()) + result = server.write_field_from_file( + ctx, "res.partner", 9, "comment", str(input_path) + ) + + assert result["success"] is False + assert "outside" in result["error"].lower() + + +def test_write_field_from_file_base64_encoding_round_trip(tmp_path, monkeypatch): + """Binary file on disk + base64 encoding → real base64 value pushed to Odoo.""" + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + raw_bytes = b"\x00\x01\x02binary blob" + input_path = tmp_path / "blob.bin" + input_path.write_bytes(raw_bytes) + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "res.partner", 9, "datas", str(input_path) + ) + assert preview["success"] is True + + result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "datas", + str(input_path), + approval=preview["approval"], + confirm=True, + ) + + assert result["success"] is True, result + write_call = next(c for c in client.calls if c[0] == "execute_method") + # write_call == ("execute_method", model, method, args_list, kwargs_dict) + _, model, method, args, kwargs = write_call + assert (model, method) == ("res.partner", "write") + assert len(args) == 2, ( + "ids + vals must arrive as two *separate* positional args for " + "Odoo XML-RPC execute_kw (not one nested list — see the bug " + "test below)." + ) + pushed_ids, pushed_vals = args + assert pushed_ids == [9] + pushed_value = pushed_vals["datas"] + assert base64.b64decode(pushed_value) == raw_bytes + assert kwargs == {} + + +def test_write_field_from_file_does_not_pack_args_into_single_list( + tmp_path, monkeypatch +): + """Regression test for the ProjectTask.write() bug. + + The original implementation called: + odoo.execute_method(model, "write", [[ids], {vals}]) + OdooClient.execute_method(self, model, method, *args, **kwargs) splits + *args, so this arrived at Odoo as execute_kw("project.task", "write", + [[ids], {vals}]) — a single positional arg. Odoo's XML-RPC dispatcher + then unpacked it as ``ProjectTask.write(*[[ids], {vals}])`` which + means ``write([ids], {vals})`` — only one positional arg where two + were required, so Odoo faulted with + ``missing 1 required positional argument: 'vals'``. + + Correct form (mirroring execute_approved_write): + odoo.execute_method(model, "write", [ids], {vals}) + + This test asserts the args tuple is split into exactly the two + separate values Odoo expects. If anyone ever re-introduces the + nested-list form, this test fires. + """ + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = tmp_path / "snippet.html" + input_path.write_text("payload", encoding="utf-8") + + client = _FieldIOClient() + ctx = FakeCtx(client) + preview = server.write_field_from_file( + ctx, "project.task", 134, "description", str(input_path) + ) + assert preview["success"] is True + + result = server.write_field_from_file( + ctx, + "project.task", + 134, + "description", + str(input_path), + approval=preview["approval"], + confirm=True, + ) + assert result["success"] is True, result + + write_call = next(c for c in client.calls if c[0] == "execute_method") + _, model, method, args, kwargs = write_call + assert (model, method) == ("project.task", "write") + # ids and vals must arrive as two SEPARATE positional args, not one + # nested list. The fix changes + # odoo.execute_method(model, "write", [[ids], {vals}]) # bug + # to + # odoo.execute_method(model, "write", [ids], {vals}) # fix + # which OdooClient then forwards to execute_kw as + # ('project.task', 'write', [[ids]], {vals}) # also ok + # either way the *args tuple Odoo receives is ([ids], {vals}). + # The structural assertion is: args is a list whose first element is + # the ids list and second element is the vals dict — *not* a single + # nested list whose first element is also a list. Type-check the + # first element specifically. + assert len(args) == 2, f"expected 2 positional args, got {args!r}" + ids_arg, vals_arg = args + assert ids_arg == [134] + assert isinstance(ids_arg, list) and all(isinstance(x, int) for x in ids_arg) + assert isinstance(vals_arg, dict) + assert vals_arg == {"description": "payload"} + assert kwargs == {} + + +def test_write_field_from_file_rejects_garbage_approval_token( + tmp_path, monkeypatch +): + server = _import_server() + _install_root(monkeypatch, tmp_path) + monkeypatch.setenv("ODOO_MCP_ENABLE_WRITES", "1") + + input_path = tmp_path / "snippet.html" + input_path.write_text("hi", encoding="utf-8") + + ctx = FakeCtx(_FieldIOClient()) + result = server.write_field_from_file( + ctx, + "res.partner", + 9, + "comment", + str(input_path), + approval={"token": "not-the-real-token", "content_sha256": "sha256:x:1"}, + confirm=True, + ) + + assert result["success"] is False + assert "token does not match" in result["error"] + + +# --------------------------------------------------------------------------- +# Field ACL integration +# --------------------------------------------------------------------------- + + +def test_read_field_to_file_writes_placeholder_when_field_is_redacted( + tmp_path, monkeypatch +): + """When the field ACL withholds the value, the file gets a placeholder + and the response flags it via field_was_redacted so the agent cannot + hallucinate the value.""" + import odoo_mcp.field_policy as fp + + server = _import_server() + _install_root(monkeypatch, tmp_path) + output_path = tmp_path / "redacted.html" + + # Install a deny policy directly into the module-level cache and + # reset afterwards so other tests are not affected. + deny_policy = fp.FieldPolicy( + { + "default": { + "res.partner": fp.ModelFieldRule( + mode="deny", fields=frozenset({"comment"}) + ), + } + } + ) + monkeypatch.setattr(fp, "_policy", deny_policy) + try: + client = _FieldIOClient() + ctx = FakeCtx(client) + result = server.read_field_to_file( + ctx, "res.partner", 7, "comment", str(output_path) + ) + assert result["success"] is True, result + assert result["field_was_redacted"] is True + assert result["redacted_fields"] == ["comment"] + # Placeholder in the file, not the real value. + body = output_path.read_text(encoding="utf-8") + assert body == "[REDACTED by field ACL]" + finally: + fp.reset_field_policy() diff --git a/tests/test_server.py b/tests/test_server.py index 323f755..1ca370d 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -108,9 +108,11 @@ def test_server_registers_expected_tools_and_resources_without_lifespan(): "search_across_instances", "aggregate_across_instances", "accounting_health_across_instances", + "read_field_to_file", + "write_field_from_file", } assert expected_tools <= tools - assert len(tools) == 41 + assert len(tools) == 43 assert "odoo://models" in resources assert { "odoo://model/{model_name}", @@ -816,7 +818,7 @@ def get_user_context(self): health = call_tool_json(server, "health_check", {}) assert health["success"] is True - assert health["server"]["tool_count"] == 41 + assert health["server"]["tool_count"] == 43 assert health["runtime"]["chatter_direct_enabled"] is False assert health["runtime"]["broad_unknown_method_mode"]["enabled"] is False @@ -1827,7 +1829,7 @@ def test_max_smart_fields_invalid_env_falls_back_to_default(monkeypatch): def test_mcp_surface_counts_reports_v030_totals(): server = importlib.import_module("odoo_mcp.server") counts = server.mcp_surface_counts() - assert counts["tool_count"] == 41 + assert counts["tool_count"] == 43 assert counts["prompt_count"] == 11 # 1 fixed resource + 3 templates = 4 assert counts["resource_count"] == 4 @@ -2140,9 +2142,18 @@ def test_normalize_domain_input_accepts_search_domain_object(): assert server.normalize_domain_input(sd) == [["name", "=", "Ada"]] -def test_normalize_domain_input_returns_empty_for_invalid_string(): +def test_normalize_domain_input_raises_for_invalid_string(): + """Garbage strings should raise ``ValueError`` rather than silently return ``[]``. + + Returning ``[]`` silently made callers silently query the wrong record + set. Fix B deliberately switches to raising so the agent sees a clean + error path. The friendly message names both legal forms. + """ + import pytest + server = importlib.import_module("odoo_mcp.server") - assert server.normalize_domain_input("def x():") == [] + with pytest.raises(ValueError, match="Domain string is neither valid JSON"): + server.normalize_domain_input("def x():") def test_normalize_domain_input_handles_python_literal_via_ast(): @@ -2168,9 +2179,16 @@ def test_normalize_domain_input_returns_empty_for_empty_list(): assert server.normalize_domain_input([[]]) == [] -def test_normalize_domain_input_returns_empty_for_empty_string(): +def test_normalize_domain_input_raises_for_empty_string(): + """Empty string is treated as a malformed domain and raises ``ValueError``. + + Use ``None`` (or simply omit the argument) for "no filter". + """ + import pytest + server = importlib.import_module("odoo_mcp.server") - assert server.normalize_domain_input("") == [] + with pytest.raises(ValueError, match="Domain string is neither valid JSON"): + server.normalize_domain_input("") # ----- write approval helpers ------------------------------------------- diff --git a/tests/test_tool_helpers.py b/tests/test_tool_helpers.py index ea8b85d..838f1af 100644 --- a/tests/test_tool_helpers.py +++ b/tests/test_tool_helpers.py @@ -445,10 +445,31 @@ def test_python_literal_string_domain(self): result = tool_helpers.normalize_domain_input(literal_str) assert result == [["name", "=", "Ada"]] - def test_invalid_json_string_returns_empty(self): - """Return empty domain on invalid JSON.""" - result = tool_helpers.normalize_domain_input('{"broken": json}') - assert result == [] + def test_invalid_json_string_raises(self): + """Raise ValueError on invalid JSON (not silently return []).""" + # Previously this fell through to `return []` which made typos + # masquerade as "match every record" in aggregates. The new contract + # is: bad input loud-fails with a helpful message naming both + # the legal JSON form and the legal Python-literal form. + with pytest.raises(ValueError, match="Domain string is neither valid JSON nor a Python literal"): + tool_helpers.normalize_domain_input('{"broken": json}') + + def test_shell_style_and_glue_raises(self): + """Raise ValueError on shell-style ``&&`` glued to a non-list string. + + A user naturally writes something like:: + + [('task_id', 'in', [1, 2])] && [('project_id', '=', 37)] + + as a single string. That is not valid JSON and not a valid Python + literal — the helper must reject it explicitly so the agent gets + a clear "use the & prefix" hint instead of a 3723-row aggregate + matching every record. + """ + with pytest.raises(ValueError, match="Domain string is neither valid JSON nor a Python literal"): + tool_helpers.normalize_domain_input( + "[('task_id', 'in', [6126, 7493])] && [('project_id', '=', 37)]" + ) def test_dict_with_conditions(self): """Parse dict with conditions key.""" @@ -488,10 +509,28 @@ def test_nested_single_condition_list(self): result = tool_helpers.normalize_domain_input([["name", "=", "Ada"]]) assert result == [["name", "=", "Ada"]] - def test_non_list_non_dict_input(self): - """Return empty domain for non-list/dict inputs.""" - assert tool_helpers.normalize_domain_input("string") == [] + def test_non_string_non_list_non_dict_input_returns_empty(self): + """Return empty domain for scalar non-list/dict/non-string inputs. + + Strings are handled by the JSON/Python-literal parser path — + they raise on bad input — while ``int``/``float``/``None`` skip + both parsers and fall through to the list/dict short-circuit. + """ + # ``int``/``None`` short-circuit through the list/dict branch and + # return ``[]`` without raising. (The previous test combined these + # with a string case; strings now raise instead, see below.) assert tool_helpers.normalize_domain_input(123) == [] + assert tool_helpers.normalize_domain_input(None) is not None + assert tool_helpers.normalize_domain_input(3.14) == [] + + def test_unparseable_string_raises(self): + """Raise ValueError on a string that is neither JSON nor a Python literal. + + Previously the function silently degraded to ``[]`` here, which + turned every typo into "match every record" in aggregates. + """ + with pytest.raises(ValueError, match="Domain string is neither valid JSON nor a Python literal"): + tool_helpers.normalize_domain_input("not a valid domain string") def test_invalid_conditions_filtered_out(self): """Filter out malformed conditions.""" diff --git a/uv.lock b/uv.lock index c1eabea..ef2039a 100644 --- a/uv.lock +++ b/uv.lock @@ -835,7 +835,7 @@ wheels = [ [[package]] name = "odoo-mcp" -version = "1.1.0" +version = "1.3.0" source = { editable = "." } dependencies = [ { name = "mcp" },