diff --git a/CHANGELOG.md b/CHANGELOG.md index 580f6c174..e0d813e9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ ### Changes +- **An n8n MCP Client Tool that exposes every tool of its server is read as doing so.** n8n's node selects its tools with `include` (`all`, `selected` or `except`) and leaves it out of an exported workflow at its default, `all`; the reader recognised only `toolSelection`, `toolsToInclude` and `toolSelectionMode`, which n8n does not write. So the default export, `include: all` and `include: except` each entered the catalog as one concretely named tool, with no wildcard, no dynamic surface and no `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED`. Each is now one `.*` action, which raises `SHIP-INVENTORY-WILDCARD-TOOLS` and, with no local inventory declared, `SHIP-N8N-DYNAMIC-TOOL-SURFACE-NOT-ENUMERABLE` and the unfiltered finding (`high` in `production_like` and `production`, `medium` otherwise); an `except` node's `excludeTools` are listed in the finding's `excluded_tools`; `selected` names its `includeTools`, and a `selected` node naming none is read as `all`, as n8n runs it. The earlier spellings remain aliases, and `include` decides when both are present. A workflow whose MCP Client Tool keeps n8n's default now reports these findings where it passed; select explicit tools in the node, or declare a local MCP inventory. Migration: [STABILITY.md](STABILITY.md#mcp-reader-conformance-1025). (#1025) +- **`SHIP-MCP-ENV-SECRET-PASSTHROUGH` counts a secret an MCP server forwards by name, not only an `env` entry.** A secret-like variable in Codex's `env_vars` list or `env_http_headers`, any `bearer_token_env_var`, and a `${NAME}` reference in a `headers` value (`.mcp.json`) now fire the finding (an editor's `${env:NAME}` is read as `NAME` there and in `env`, and a prompted `${input:…}` names no variable) and add to the server's risk score as an `env` entry always did. A credential-named header (`Authorization`) counts the variable it carries whatever that variable is called, and a value written where a variable name belongs is redacted in the evidence, not echoed. (#1025) +- **A `codex_config` source names the other hosts' MCP config files it does not read.** Its coverage said it read `.mcp.json`, `.codex/config.toml` "and the other host MCP config files" under its root; it reads only the first two. A `.cursor/mcp.json` or `.vscode/mcp.json` there is now named in a source warning, so a workspace whose servers live only in one no longer passes silently, and the coverage text in [`docs/determinism-boundary.md`](docs/determinism-boundary.md) names exactly the files read and the files named. Reading those files is #663. (#1025) - **`verify` stops when it has no base, and a deleted manifest is a removed gate.** With neither `--base` nor `--no-base`, a run whose detection found nothing (a default branch other than `main` or `master` with no `origin/HEAD`, or a repository with no remote) read no committed change, published that empty change set as `complete` and granted merge: a pull request deleting `.github/workflows/agents-shipgate.yml` on a `develop` repository exited 0 as `mergeable`, and exited 20 as `blocked` once `--base origin/develop` was named. Detection keeps every base it found before and otherwise follows `check`'s ladder (#649): a default branch already at the head is an empty comparison, and a repository with no remote uses its local `main` or `master`. When nothing resolves, the run exits 2 with `base_status: ref_missing`, a `diff_status` of `unavailable`/`refs_missing` whose detail begins `base_unresolved`, a `fetch_base` next action naming `--base` and `--no-base`, and every permission denied. A change deleting the configured manifest that its merge base (or `HEAD`, for a working-tree run without a base) holds is a human `stop` naming the removal, exit 2, instead of the `configure` and `init --write` route; a missing manifest whose base cannot be resolved fails closed as above, one whose base holds it but cannot be compared (a shallow merge base), or whose named `--base` is not available locally (the Action's `diff_base: target` without `fetch-depth: 0`), fails closed with that diff-input `fetch_base` stop in both CI modes, and a repository that never had one keeps the setup route (with an unavailable named base, only where complete history, neither shallow nor grafted, shows the manifest never existed; a depth-1 checkout with an unfetched `--base` therefore stops in advisory mode too, where it used to exit 0 on the host route). `SHIP-VERIFY-CI-GATE-REMOVED` resolves changed paths against the evaluated tree's root instead of the manifest's directory, so `apps/agent/shipgate.yaml` no longer turns a comment in the root workflow into a critical blocker, nor lets a nested copy hide a root deletion. Every Git archive packs the commit's tree rather than its history, so a depth-1 checkout archives `--head` instead of failing an integrity check that agent mode routed to editing `shipgate.yaml`. `check` leaves verify's default reports directory out of a working-tree change set when it holds only uncommitted Shipgate artifacts, so a check, verify, check loop no longer counts the reports as changed files. **A run that used to report `mergeable` with no base now exits 2**: pass `--base` (in the Action, `diff_base: target` with `fetch-depth: 0`, or `base_ref`) or `--no-base`. No schema, contract or check ID moves. See the `verify stops without a base` migration note in `STABILITY.md`. (#1023) - **`attest`, `org bundle` and the Action validate the verification receipt of current verify output.** All three gated receipt validation on version literals the engine had stopped emitting (verifier `0.5`–`0.7`, `shipgate.verify_run/v3`). On every completed `1.x` verify run, `attest --from agents-shipgate-reports/verifier.json` and the Action's `attestation: true` step crashed with a traceback and exit 1, `org bundle` published a rewritten `receipt_id` or `decision_id` with exit 0, and the Action exported empty `request_id`, `receipt_id`, `decision_id` and `artifact_set_id` beside a `verification_receipt_json` nothing had checked. One predicate, `requires_verification_receipt` in `agents_shipgate.schemas.verification_identity`, now decides for all three: it names the verifier versions written before receipts (`0.1`–`0.4`) and reads every other version, newer ones included, as owing a receipt whenever the verifier states an outcome; the Action, which also publishes `report.json`'s decision, owes one whenever `verifier.json` names a `report.json` as its own, and publishes nothing from a report it does not name (the manifest-free host route leaves an earlier run's in place). A `0.1`–`0.4` label on a payload carrying a receipt-era identity field (`request_id`, `decision_id` and the four others verifier `0.5` added) is not believed. A failed or not-run execution that states none (a missing manifest, the manifest-free host route) writes no receipt, and still publishes, with empty identities. A receipt that is present is always validated: its content addressing, every artifact it binds and the request and decision `verifier.json` names. A missing, tampered or mismatched one exits `3` in `attest` and `org bundle` with one line naming the receipt (plus the agent-mode `next_action` line), and fails the Action's output step. `org bundle` also refuses an `attestation.json` whose receipt identity is not the validated receipt's, or that names one when none was validated. The Action's `verification_receipt_json` is empty unless the receipt validated. No schema version changes. See the [migration note](STABILITY.md#receipt-predicate-1022). (#1022) - **An OpenAPI operation behind a path-item `$ref` is read, and one that cannot be read is named.** The loader skipped every key of a path item that was not an HTTP method, so `/accounts/{id}: {$ref: '#/components/pathItems/Account'}` contributed nothing: its `deleteAccount` was absent from `report.json` and a strict scan passed. A `$ref` to another path item in the same document is now followed (chains included, with the fields written beside the `$ref` read first), and each operation's `source_pointer` and line name where it is written, such as `/components/pathItems/Account/delete`. A path item whose `$ref` leaves the document (a split spec's `./paths/users.yaml`, which is reported and never opened), names nothing, or loops is a source warning and an `adapter_parse` row (`unresolved_path_item_ref`) in `surface_exclusions`, so the scan no longer passes; a referenced operation replaced by one of the same method beside the `$ref` is one too (`shadowed_path_item_operation`). Schema `$ref`s the document resolves no longer warn: the resolver also descends `additionalProperties`, `patternProperties`, `not`, `prefixItems` and list-form `items`, follows a schema that is only a reference, and an unresolved reference is reported once per target rather than once per tool, so four read operations sharing an `additionalProperties` reference no longer read as `insufficient_evidence`. Unquoted YAML status codes (`200:`) select the lowest 2xx response as the output schema instead of the first response listed, and the DELETE proof's pure-Python YAML parse runs only when a document has a DELETE, stopping once the document is past its node budget. **Tool inventories can grow** where a path-item `$ref` hid operations, and a verdict that rested on the hidden part can move from `passed` to `review_required` or `blocked`; one held back only by false unresolved-reference warnings can move to `passed`. The DELETE proof still refuses a path item written as a `$ref`. (#1021) diff --git a/STABILITY.md b/STABILITY.md index ec225c48b..f418662b6 100644 --- a/STABILITY.md +++ b/STABILITY.md @@ -347,6 +347,61 @@ the Action tag) for reproducible CI. --- + + +## Migration Note: Unreleased — n8n's `include`, MCP secrets forwarded by name, and the host MCP files a `codex_config` source does not read (#1025) + +**What was wrong.** Three readers did not match what the tools they read +write. n8n's MCP Client Tool selects its tools with `include` (`all`, +`selected` or `except`) and leaves it out of an export at its default, `all`; +the n8n reader read only `toolSelection`, `toolsToInclude` and +`toolSelectionMode`, so a node exposing every tool of its server was one +concretely named tool and `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED` never fired +on a real export. `SHIP-MCP-ENV-SECRET-PASSTHROUGH` read only a server's `env` +table, not a secret forwarded by name. And a `codex_config` source's coverage +said it read "the other host MCP config files" while it read only `.mcp.json` +and `.codex/config.toml`, so a workspace whose servers lived in +`.cursor/mcp.json` or `.vscode/mcp.json` passed with nothing said. + +**What changes.** + +- **Check ids.** None is added, removed or renamed. Per + [Check IDs](#check-ids), the conditions under which these fire broaden, and + this note records it: + `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED`, + `SHIP-N8N-DYNAMIC-TOOL-SURFACE-NOT-ENUMERABLE` and + `SHIP-INVENTORY-WILDCARD-TOOLS` fire for an MCP Client Tool with no + `include`, `include: all`, `include: except`, or `include: selected` naming + no `includeTools`; `SHIP-MCP-ENV-SECRET-PASSTHROUGH` fires for a + secret-like name in `env_vars` or `env_http_headers` (or one a credential + header carries), any `bearer_token_env_var`, and a `${NAME}` reference in + `headers`. +- **Fingerprints.** Such an MCP Client Tool's action is `.*` rather than + the node's name, so a finding keyed on the old tool name (for example + `SHIP-N8N-AI-TOOL-METADATA-MISSING`) reads as new. An + `SHIP-MCP-ENV-SECRET-PASSTHROUGH` finding on a server that also forwards a + secret in one of the new forms lists more `env_secret_names`, so it reads as + new too. An unfiltered finding on an `except` node gains `excluded_tools` + only when the node names one, so an existing finding with none keeps its + fingerprint. +- **Decisions.** A workflow whose MCP Client Tool keeps n8n's default, and a + `codex_config` workspace holding `.cursor/mcp.json` or `.vscode/mcp.json` + under its root, no longer reach `passed`: the first raises the findings + above, and the second a source warning naming each such file. + +**Who must act.** An n8n workflow author selects explicit tools in the MCP +Client Tool, or declares a local MCP inventory. A `codex_config` adopter +reviews the servers in the named file; until #663 reads those files, the +warning stays while the file is under the source's root. A suppression or +baseline entry for a finding that reads as new is re-recorded. + +**What does not change.** The earlier n8n spellings are still read, as +aliases; `include` decides when both are present. A `selected` node naming +its tools lists them as before. No CLI flag, schema, control state or +permission is added, and `minimum_control_contract_version` stays `21`. + +--- + ## Migration Note: Unreleased — `verify` stops without a base, and a deleted manifest is a removed gate (#1023) diff --git a/docs/checks.json b/docs/checks.json index 486467096..657dd5e33 100644 --- a/docs/checks.json +++ b/docs/checks.json @@ -2514,7 +2514,7 @@ "source_id", "env_secret_names" ], - "fires_when": "A statically parsed MCP server declares secret-like env vars or passes through secret-like host env references.", + "fires_when": "A statically parsed MCP server declares secret-like env vars, passes through secret-like host env references, or forwards a secret-like variable by name (`env_vars`, `bearer_token_env_var`, `env_http_headers`, a `headers` reference).", "floor_severity": "medium", "id": "SHIP-MCP-ENV-SECRET-PASSTHROUGH", "mvp_tier": "lifecycle", @@ -2686,9 +2686,10 @@ "source_ref", "node_id", "selection_mode", - "explicit_inventory" + "explicit_inventory", + "excluded_tools" ], - "fires_when": "An n8n MCP Client Tool uses All or All Except selection and no explicit local inventory is declared.", + "fires_when": "An n8n MCP Client Tool uses All or All Except selection (`include` of `all`, the default an export leaves out, or `except`) and no explicit local inventory is declared.", "floor_severity": null, "id": "SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED", "mvp_tier": "adapter", diff --git a/docs/checks.md b/docs/checks.md index 64fe1f5b4..78a07f12a 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -923,7 +923,11 @@ inventory. ### SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED An n8n MCP Client Tool exposes `All` or `All Except` tools without a local -inventory. Select explicit MCP tools or provide a local MCP inventory for +inventory. The node selects with `include`: `all`, which is n8n's default and +is left out of an exported workflow, so a node with no `include` exposes every +tool; `except`, whose `excludeTools` the finding lists as `excluded_tools`; or +`selected`, which names its `includeTools` and does not fire this check unless +it names none. Select explicit MCP tools or provide a local MCP inventory for release review. The severity is environment-sensitive because the selector is easy to narrow before production, while production-like use increases blast radius. @@ -1234,9 +1238,14 @@ a changed file — env- or settings-bound factories never guess a match. Fires when a statically parsed MCP server passes through secret-like environment variables, such as token, password, API key, or credential -names. Secret pass-through changes the credential boundary available to the -server, so Shipgate routes the change to human review without exposing raw -secret values in evidence. +names. Besides an `env` entry, a server can forward a variable by name: in +Codex's `env_vars` list or `env_http_headers`, as a `bearer_token_env_var` +(always counted, since it holds a bearer token), or as a `${NAME}` reference in +a `headers` value. A forwarded variable counts when its name, or the header +carrying it, is secret-like. An editor's `${env:NAME}` is read as `NAME`; a +prompted `${input:…}` names no environment variable. Secret pass-through changes the credential +boundary available to the server, so Shipgate routes the change to human +review without exposing raw secret values in evidence. ### SHIP-MCP-AUTO-APPROVE-SIDE-EFFECT diff --git a/docs/checks/mcp_permissions.yaml b/docs/checks/mcp_permissions.yaml index 88c04033f..e929e4ed3 100644 --- a/docs/checks/mcp_permissions.yaml +++ b/docs/checks/mcp_permissions.yaml @@ -10,8 +10,9 @@ checks: description: MCP server passes through secret environment variables. rationale: Secret-bearing environment pass-through expands the credential boundary available to a tool server. - fires_when: A statically parsed MCP server declares secret-like env vars or passes - through secret-like host env references. + fires_when: A statically parsed MCP server declares secret-like env vars, passes + through secret-like host env references, or forwards a secret-like variable by + name (`env_vars`, `bearer_token_env_var`, `env_http_headers`, a `headers` reference). evidence_fields: [source_id, env_secret_names] recommendation: Review the secret boundary or replace pass-through with a narrower credential mechanism. diff --git a/docs/checks/n8n.yaml b/docs/checks/n8n.yaml index 259da1c19..3f7368378 100644 --- a/docs/checks/n8n.yaml +++ b/docs/checks/n8n.yaml @@ -70,12 +70,14 @@ checks: description: n8n MCP Client Tool exposes an unfiltered toolset. rationale: |- All-tools and all-except MCP client selections can expose more tools than reviewers expect. The check is environment-sensitive because the selector is straightforward to narrow before production, while production-like use increases blast radius. - fires_when: An n8n MCP Client Tool uses All or All Except selection and no explicit - local inventory is declared. + fires_when: An n8n MCP Client Tool uses All or All Except selection (`include` + of `all`, the default an export leaves out, or `except`) and no explicit local + inventory is declared. evidence_fields: - source_ref - node_id - selection_mode - explicit_inventory + - excluded_tools recommendation: Select an explicit MCP tool allowlist in n8n or provide a local MCP inventory. diff --git a/docs/determinism-boundary.json b/docs/determinism-boundary.json index 2cda606db..004f31e9b 100644 --- a/docs/determinism-boundary.json +++ b/docs/determinism-boundary.json @@ -1073,7 +1073,7 @@ "SHIP-INVENTORY-WILDCARD-TOOLS", "SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED" ], - "reads": "An MCP client node whose tool selection is `all`, `all_except`, or unreadable: one `.*` action stands in for a selection the workflow does not enumerate.", + "reads": "An MCP client node whose `include` is `all` \u2014 n8n's default, which an export leaves out \u2014 or `except`, or whose selection is unreadable: one `.*` action stands in for a selection the workflow does not enumerate, and an `except` node's `excludeTools` are recorded.", "shape": "dynamic_construction", "status": "extracted", "surface": null, @@ -1524,7 +1524,7 @@ "label": "Codex / MCP host config", "manifest_section": null, "manifest_section_role": "activates", - "reads": "`.mcp.json`, `.codex/config.toml`, and the other host MCP config files under the root a `tool_sources[]` entry names \u2014 including the servers a plugin block declares." + "reads": "`.mcp.json` and `.codex/config.toml`, at any depth under the root a `tool_sources[]` entry names \u2014 including the servers a plugin block of `.codex/config.toml` declares. Other hosts' MCP config files there (`.cursor/mcp.json` and `.vscode/mcp.json`) are not read; each one present is named in a source warning." }, { "adapter": "codex_plugin", diff --git a/docs/determinism-boundary.md b/docs/determinism-boundary.md index 2892b1162..8918b8f00 100644 --- a/docs/determinism-boundary.md +++ b/docs/determinism-boundary.md @@ -235,7 +235,7 @@ Where an input shows more than one answer for a shape, the table shows the best | `literal_registration` | A tool node present in the workflow JSON — workflow, code, HTTP, MCP client, or another AI tool node — read for its parameters and credentials. The export names the node; the called tool's own contract is not in the file. | `n8n_workflow_tool`, `n8n_code_tool`, `n8n_http_tool`, `n8n_mcp_client_tool`, `n8n_ai_tool`, `mcp` | `medium` | `low_confidence` | `low_confidence_tool`, `unattested_surface` | — | | `factory` | A workflow JSON has no construction step: a node is in the file or it is not. | — | — | `not_applicable` | — | — | | `dynamic_construction — expression-backed tool name` | A tool node whose `toolName` (or workflow target) is an n8n expression. The node still enters the catalog under the expression text at `medium`, and the unresolved name is recorded as a dynamic fact — the action is not hidden, its identity is. | `n8n_workflow_tool` | `medium` | `low_confidence` | `low_confidence_tool`, `unattested_surface` | `SHIP-N8N-DYNAMIC-TOOL-SURFACE-NOT-ENUMERABLE` | -| `dynamic_construction — MCP client wildcard` | An MCP client node whose tool selection is `all`, `all_except`, or unreadable: one `.*` action stands in for a selection the workflow does not enumerate. | `n8n_mcp_client_tool` | `medium` | `low_confidence` | `low_confidence_tool`, `incomplete_surface` | `SHIP-INVENTORY-WILDCARD-TOOLS`, `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED` | +| `dynamic_construction — MCP client wildcard` | An MCP client node whose `include` is `all` — n8n's default, which an export leaves out — or `except`, or whose selection is unreadable: one `.*` action stands in for a selection the workflow does not enumerate, and an `except` node's `excludeTools` are recorded. | `n8n_mcp_client_tool` | `medium` | `low_confidence` | `low_confidence_tool`, `incomplete_surface` | `SHIP-INVENTORY-WILDCARD-TOOLS`, `SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED` | @@ -308,7 +308,7 @@ Where an input shows more than one answer for a shape, the table shows the best ### Codex / MCP host config -*Configured as a `tool_sources[]` entry of type `codex_config`.* `.mcp.json`, `.codex/config.toml`, and the other host MCP config files under the root a `tool_sources[]` entry names — including the servers a plugin block declares. +*Configured as a `tool_sources[]` entry of type `codex_config`.* `.mcp.json` and `.codex/config.toml`, at any depth under the root a `tool_sources[]` entry names — including the servers a plugin block of `.codex/config.toml` declares. Other hosts' MCP config files there (`.cursor/mcp.json` and `.vscode/mcp.json`) are not read; each one present is named in a source warning. **Can a verdict rest on it?** diff --git a/llms-full.txt b/llms-full.txt index 97bc20648..1ddb9a685 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -4341,7 +4341,11 @@ inventory. ### SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED An n8n MCP Client Tool exposes `All` or `All Except` tools without a local -inventory. Select explicit MCP tools or provide a local MCP inventory for +inventory. The node selects with `include`: `all`, which is n8n's default and +is left out of an exported workflow, so a node with no `include` exposes every +tool; `except`, whose `excludeTools` the finding lists as `excluded_tools`; or +`selected`, which names its `includeTools` and does not fire this check unless +it names none. Select explicit MCP tools or provide a local MCP inventory for release review. The severity is environment-sensitive because the selector is easy to narrow before production, while production-like use increases blast radius. @@ -4652,9 +4656,14 @@ a changed file — env- or settings-bound factories never guess a match. Fires when a statically parsed MCP server passes through secret-like environment variables, such as token, password, API key, or credential -names. Secret pass-through changes the credential boundary available to the -server, so Shipgate routes the change to human review without exposing raw -secret values in evidence. +names. Besides an `env` entry, a server can forward a variable by name: in +Codex's `env_vars` list or `env_http_headers`, as a `bearer_token_env_var` +(always counted, since it holds a bearer token), or as a `${NAME}` reference in +a `headers` value. A forwarded variable counts when its name, or the header +carrying it, is secret-like. An editor's `${env:NAME}` is read as `NAME`; a +prompted `${input:…}` names no environment variable. Secret pass-through changes the credential +boundary available to the server, so Shipgate routes the change to human +review without exposing raw secret values in evidence. ### SHIP-MCP-AUTO-APPROVE-SIDE-EFFECT diff --git a/src/agents_shipgate/checks/n8n.py b/src/agents_shipgate/checks/n8n.py index fac6e2537..02e4cd53b 100644 --- a/src/agents_shipgate/checks/n8n.py +++ b/src/agents_shipgate/checks/n8n.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import Any + from agents_shipgate.checks._framework_common import ( DynamicSurfaceConfig, collect_dynamic_surface_findings, @@ -56,6 +58,16 @@ def run(context: ScanContext) -> list[Finding]: for toolset in artifacts.mcp_client_tools: if toolset.get("selection_mode") not in {"all", "all_except"}: continue + evidence: dict[str, Any] = { + "source_ref": toolset.get("source_ref"), + "node_id": toolset.get("node_id"), + "selection_mode": toolset.get("selection_mode"), + "explicit_inventory": False, + } + # Only when it names a tool, so an `except` node that excludes + # nothing keeps the evidence, and the fingerprint, it had. + if toolset.get("excluded_tools"): + evidence["excluded_tools"] = list(toolset["excluded_tools"]) findings.append( Finding( check_id="SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED", @@ -66,12 +78,7 @@ def run(context: ScanContext) -> list[Finding]: else "medium", category="n8n", agent_id=context.agent.id, - evidence={ - "source_ref": toolset.get("source_ref"), - "node_id": toolset.get("node_id"), - "selection_mode": toolset.get("selection_mode"), - "explicit_inventory": False, - }, + evidence=evidence, confidence="high", provenance_kind="static_declaration", source=_n8n_source(toolset), diff --git a/src/agents_shipgate/core/source_warnings.py b/src/agents_shipgate/core/source_warnings.py index 888104944..79848f606 100644 --- a/src/agents_shipgate/core/source_warnings.py +++ b/src/agents_shipgate/core/source_warnings.py @@ -316,6 +316,26 @@ def duplicate_mcp_server_declaration_warning( ) +def unread_host_mcp_config_warning( + path: str, host: str, read_files: Sequence[str] +) -> str: + """Warning for another host's MCP config file a ``codex_config`` source does not read. + + The source reads ``read_files`` and nothing else, so a server declared only + in ``path`` is not in the scan. Passing quietly over it read as a scan that + had seen every MCP server in the tree (#1025); reading these files is #663. + """ + + quoted = [f"`{name}`" for name in read_files] + read = " and ".join([_join(quoted[:-1]), quoted[-1]] if len(quoted) > 1 else quoted) + return ( + f"{host} MCP config {path!r} was not read: a codex_config entry in " + f"shipgate.yaml#tool_sources[] reads only {read}, so an MCP server " + "declared only in that file is missing from this scan. Review that " + "file's servers before relying on this result." + ) + + def ambiguous_inventory_merge_warning( inventory_path: str, source_id: str, tools: Sequence[str] ) -> str: @@ -762,6 +782,7 @@ def withdraw_completed_adk_tool_warnings( "unbound_inventory_duplicate_warning", "unknown_inventory_source_warning", "unmatched_binding_member", + "unread_host_mcp_config_warning", "unresolved_adk_tool_symbols", "withdraw_completed_adk_tool_warnings", "zero_observation_binding_member", diff --git a/src/agents_shipgate/inputs/codex_config.py b/src/agents_shipgate/inputs/codex_config.py index cfc67c932..a16bbac5d 100644 --- a/src/agents_shipgate/inputs/codex_config.py +++ b/src/agents_shipgate/inputs/codex_config.py @@ -10,11 +10,31 @@ walk_input_tree, ) from agents_shipgate.inputs.coverage import BoundaryCell, SourceCoverage -from agents_shipgate.inputs.mcp_manifest import load_codex_config_mcp_sources +from agents_shipgate.inputs.mcp_manifest import ( + READ_MCP_CONFIG_FILES, + UNREAD_HOST_MCP_CONFIG_FILES, + load_codex_config_mcp_sources, +) from agents_shipgate.inputs.protocol import LoadedAdapterResult from agents_shipgate.schemas.manifest import AgentsShipgateManifest, ToolSourceConfig +def _code_list(names: tuple[str, ...]) -> str: + quoted = [f"`{name}`" for name in names] + return quoted[0] if len(quoted) == 1 else f"{', '.join(quoted[:-1])} and {quoted[-1]}" + + +#: What the coverage says this adapter reads, written from the tuples the +#: reader matches files with, so the two cannot drift apart (#1025). +_READS = ( + f"{_code_list(READ_MCP_CONFIG_FILES)}, at any depth under the root a " + "`tool_sources[]` entry names — including the servers a plugin block of " + "`.codex/config.toml` declares. Other hosts' MCP config files there " + f"({_code_list(tuple(path for path, _host in UNREAD_HOST_MCP_CONFIG_FILES))}) " + "are not read; each one present is named in a source warning." +) + + class CodexConfigAdapter: source_type: ClassVar[str] = "codex_config" scope: ClassVar[Literal["per_source"]] = "per_source" @@ -23,11 +43,7 @@ class CodexConfigAdapter: coverage: ClassVar[SourceCoverage] = SourceCoverage( adapter="codex_config", label="Codex / MCP host config", - reads=( - "`.mcp.json`, `.codex/config.toml`, and the other host MCP config " - "files under the root a `tool_sources[]` entry names — including the " - "servers a plugin block declares." - ), + reads=_READS, cells=( BoundaryCell( shape="export_artifact", diff --git a/src/agents_shipgate/inputs/mcp_manifest.py b/src/agents_shipgate/inputs/mcp_manifest.py index c29e2ad26..c93b0a73f 100644 --- a/src/agents_shipgate/inputs/mcp_manifest.py +++ b/src/agents_shipgate/inputs/mcp_manifest.py @@ -15,8 +15,10 @@ ) from agents_shipgate.core.domain import LoadedToolSource, SourceSurfaceOmission, Tool from agents_shipgate.core.errors import InputParseError +from agents_shipgate.core.privacy import redact_text from agents_shipgate.core.source_warnings import ( duplicate_mcp_server_declaration_warning, + unread_host_mcp_config_warning, ) from agents_shipgate.inputs.common import ( list_input_directory, @@ -39,7 +41,15 @@ _TOOLS_KEYS = ("tools", "tool_overrides") _SCHEMA_KEYS = ("inputSchema", "input_schema", "schema") _OUTPUT_SCHEMA_KEYS = ("outputSchema", "output_schema") -_ENV_REF_RE = re.compile(r"\$[{(]?([A-Za-z_][A-Za-z0-9_]*)[})]?") +#: An environment-variable reference in a value: `$NAME`, `${NAME}`, the +#: shell's `${NAME:-default}`, `${NAME:=default}` and `${NAME:?message}`, +#: `$(NAME)`, or the editors' `${env:NAME}`, read as `NAME`. Any other +#: `${prefix:id}`, such as VS Code's `${input:id}`, names no environment +#: variable, so its prefix is not read as one. +_ENV_REF_RE = re.compile( + r"\$(?:\{env:|[{(])?([A-Za-z_][A-Za-z0-9_]*)(?![A-Za-z0-9_])(?!:[A-Za-z_])" +) +_ENV_VAR_NAME_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") _SKIPPED_SCAN_DIRS = { ".git", ".hg", @@ -55,6 +65,23 @@ } +#: The MCP config files a ``codex_config`` source reads, at any depth under the +#: root its ``tool_sources[]`` entry names. ``CodexConfigAdapter.coverage`` is +#: written from this and :data:`UNREAD_HOST_MCP_CONFIG_FILES`, so what the +#: adapter says it reads is what :func:`_mcp_config_files` matches (#1025). +READ_MCP_CONFIG_FILES: tuple[str, ...] = (".mcp.json", ".codex/config.toml") + +#: Other hosts' MCP config files, with the host each is for: the ones the host +#: readers recognise (``core.boundary_registry``) that this reader does not +#: read. One present under the root is named in a source warning instead of +#: passed over, so servers declared only there never pass silently. Reading +#: them is #663. +UNREAD_HOST_MCP_CONFIG_FILES: tuple[tuple[str, str], ...] = ( + (".cursor/mcp.json", "Cursor"), + (".vscode/mcp.json", "VS Code"), +) + + #: ``SourceSurfaceOmission.reason`` for a server declaration that did not #: enter the catalog as a surface of its own. Rendered by #: ``core.surface_exclusions.EXCLUSION_REASON_PHRASES``, which is where the @@ -183,9 +210,10 @@ def load_codex_config_mcp_sources(root: Path, base_dir: Path) -> list[LoadedTool root = root.resolve() if not root.exists(): return sources - for path in _iter_mcp_candidate_files(root): + read_files, unread_files = _mcp_config_files(root) + for path in read_files: rel = _relative(path, root) - if rel == ".codex/config.toml" or rel.endswith("/.codex/config.toml"): + if _is_config_path(rel, ".codex/config.toml"): source_ref = _relative(path, base_dir) try: data = tomllib.loads(load_text_file(path)) @@ -210,6 +238,19 @@ def load_codex_config_mcp_sources(root: Path, base_dir: Path) -> list[LoadedTool if unreadable is not None: sources.append(unreadable) servers.extend(file_servers) + for path, host in unread_files: + # Warning-only, like a file that did not parse: it carries no tools, + # so it never collides with a per-server source, and it names the file. + source_ref = _relative(path, base_dir) + sources.append( + LoadedToolSource( + source_id=f"unread_host_mcp_config:{source_ref}", + source_type="codex_config_mcp", + warnings=[ + unread_host_mcp_config_warning(source_ref, host, READ_MCP_CONFIG_FILES) + ], + ) + ) sources.extend(_loaded_sources_from_servers(servers)) return sources @@ -302,7 +343,7 @@ def _servers_from_mapping( # is this invariant that makes one name in two files one server to # reconcile rather than two surfaces to tell apart. source_id = f"{source_id_prefix}:{server_name}" - env_secret_names = tuple(_env_secret_names(raw_server.get("env"))) + env_secret_names = tuple(_env_secret_names(raw_server)) transport = _transport(raw_server) command = raw_server.get("command") if isinstance(raw_server.get("command"), str) else None url_present = isinstance(raw_server.get("url"), str) or transport in { @@ -996,18 +1037,60 @@ def _first_dict(data: dict[str, Any], names: tuple[str, ...]) -> dict[str, Any]: return {} -def _env_secret_names(raw_env: Any) -> list[str]: - if not isinstance(raw_env, dict): - return [] +def _env_secret_names(raw_server: dict[str, Any]) -> list[str]: + """The secret-shaped environment variables one server declaration forwards. + + An ``env`` entry is named by its key, as it always was. A server can also + forward a variable by name, in the forms ``core.host_grants`` models for + the same declarations (#795, #1025): Codex's ``env_vars`` list and + ``bearer_token_env_var``, Codex's ``env_http_headers`` (header name to + variable name), and a ``${NAME}`` reference in a ``headers`` value. A + forwarded name counts when it, or the header carrying it, is secret-shaped; + ``bearer_token_env_var`` always does, since it holds a bearer token. + + This is detection, so it does not reuse ``host_grants``' publication filter, + which withholds a name its digest redaction rewrites — ``API_KEY`` after + ``TOKEN`` in one ``env_vars`` list. A name is still never published as + written when it looks like a secret value: :func:`redact_text` replaces it. + """ + names: set[str] = set() - for key, value in raw_env.items(): - key_text = str(key) - if is_secret_env_name(key_text): - names.add(key_text) - if isinstance(value, str): - for match in _ENV_REF_RE.findall(value): - if is_secret_env_name(match): - names.add(key_text) + raw_env = raw_server.get("env") + if isinstance(raw_env, dict): + for key, value in raw_env.items(): + key_text = str(key) + if is_secret_env_name(key_text): + names.add(key_text) + if isinstance(value, str): + for match in _ENV_REF_RE.findall(value): + if is_secret_env_name(match): + names.add(key_text) + forwarded: list[tuple[str, bool]] = [] + env_vars = raw_server.get("env_vars") + if isinstance(env_vars, list): + forwarded.extend((item, False) for item in env_vars if isinstance(item, str)) + bearer = raw_server.get("bearer_token_env_var") + if isinstance(bearer, str): + forwarded.append((bearer, True)) + env_headers = raw_server.get("env_http_headers") + if isinstance(env_headers, dict): + forwarded.extend( + (value, is_secret_env_name(str(header))) + for header, value in env_headers.items() + if isinstance(value, str) + ) + headers = raw_server.get("headers") + if isinstance(headers, dict): + forwarded.extend( + (match, is_secret_env_name(str(header))) + for header, value in headers.items() + if isinstance(value, str) + for match in _ENV_REF_RE.findall(value) + ) + for name, credential_carrier in forwarded: + name = name.strip() + if _ENV_VAR_NAME_RE.fullmatch(name) and (credential_carrier or is_secret_env_name(name)): + names.add(redact_text(name) or name) return sorted(names) @@ -1025,7 +1108,7 @@ def _transport(server: dict[str, Any]) -> str | None: def _looks_like_local_documentation(server_name: str, server: dict[str, Any]) -> bool: if isinstance(server.get("url"), str): return False - if _env_secret_names(server.get("env")): + if _env_secret_names(server): return False text = " ".join( [ @@ -1038,8 +1121,20 @@ def _looks_like_local_documentation(server_name: str, server: dict[str, Any]) -> return bool(tokens & {"doc", "docs", "documentation", "readme"}) -def _iter_mcp_candidate_files(root: Path) -> list[Path]: +def _is_config_path(rel: str, config_path: str) -> bool: + return rel == config_path or rel.endswith(f"/{config_path}") + + +def _mcp_config_files(root: Path) -> tuple[list[Path], list[tuple[Path, str]]]: + """The MCP config files under ``root`` this reader reads, and the ones it names. + + The first are :data:`READ_MCP_CONFIG_FILES`; the second, with the host each + is for, :data:`UNREAD_HOST_MCP_CONFIG_FILES`. Both are found at any depth, + in one walk, in path order. + """ + candidates: list[Path] = [] + unread: list[tuple[Path, str]] = [] stack = [root] while stack: current = stack.pop() @@ -1054,13 +1149,16 @@ def _iter_mcp_candidate_files(root: Path) -> list[Path]: if not child.is_file(): continue rel = _relative(child, root) - if ( - rel == ".codex/config.toml" - or rel.endswith("/.codex/config.toml") - or child.name == ".mcp.json" - ): + if any(_is_config_path(rel, config) for config in READ_MCP_CONFIG_FILES): candidates.append(child) - return sorted(candidates) + continue + host = next( + (host for config, host in UNREAD_HOST_MCP_CONFIG_FILES if _is_config_path(rel, config)), + None, + ) + if host is not None: + unread.append((child, host)) + return sorted(candidates), sorted(unread) def _text_or_none(value: Any) -> str | None: @@ -1083,6 +1181,8 @@ def _json_pointer_escape(value: str) -> str: __all__ = [ "DUPLICATE_SERVER_DECLARATION", + "READ_MCP_CONFIG_FILES", + "UNREAD_HOST_MCP_CONFIG_FILES", "NormalizedMcpServer", "NormalizedMcpTool", "load_codex_config_mcp_sources", diff --git a/src/agents_shipgate/inputs/n8n/_adapter.py b/src/agents_shipgate/inputs/n8n/_adapter.py index ea06f6bf7..ec93ef9ee 100644 --- a/src/agents_shipgate/inputs/n8n/_adapter.py +++ b/src/agents_shipgate/inputs/n8n/_adapter.py @@ -324,9 +324,11 @@ class N8nAdapter: variant="MCP client wildcard", status="extracted", reads=( - "An MCP client node whose tool selection is `all`, `all_except`, " - "or unreadable: one `.*` action stands in for a selection " - "the workflow does not enumerate." + "An MCP client node whose `include` is `all` — n8n's default, " + "which an export leaves out — or `except`, or whose selection " + "is unreadable: one `.*` action stands in for a " + "selection the workflow does not enumerate, and an `except` " + "node's `excludeTools` are recorded." ), emits=("n8n_mcp_client_tool",), ceiling="medium", diff --git a/src/agents_shipgate/inputs/n8n/_tools.py b/src/agents_shipgate/inputs/n8n/_tools.py index 37bcbe9f2..387460b66 100644 --- a/src/agents_shipgate/inputs/n8n/_tools.py +++ b/src/agents_shipgate/inputs/n8n/_tools.py @@ -254,15 +254,18 @@ def _mcp_client_tools( ) -> list[Tool]: from agents_shipgate.inputs.n8n._workflows import _dynamic, _node_record - mode = _selection_mode(item.parameters) - selected = _selected_mcp_tools(item.parameters) - artifacts.mcp_client_tools.append( - { - **_node_record(item, source_path, workflow_id), - "selection_mode": mode, - "selected_tool_count": len(selected), - } - ) + # n8n leaves `include` out of an export at its default, `all`, so a node + # that names no selection exposes every tool its server lists (#1025). + mode = _selection_mode(item.parameters, client=True) + selected = _selected_mcp_tools(item.parameters) if mode == "selected" else [] + record: dict[str, Any] = { + **_node_record(item, source_path, workflow_id), + "selection_mode": mode, + "selected_tool_count": len(selected), + } + if mode == "all_except": + record["excluded_tools"] = _excluded_mcp_tools(item.parameters) + artifacts.mcp_client_tools.append(record) if mode in {"all", "all_except"} and not artifacts.tool_inventory_files: _dynamic( artifacts, @@ -284,11 +287,9 @@ def _mcp_client_tools( ), warnings=warnings, ) - names = selected or [ - f"{_redact_text(item.name) or item.name}.*" - if mode in {"all", "all_except", "unknown"} - else _tool_name(item) - ] + # Every mode but `selected` names no tool, so one `.*` action stands + # in for the selection the workflow does not enumerate. + names = selected or [f"{_redact_text(item.name) or item.name}.*"] tools = [ _base_tool( item, @@ -444,22 +445,55 @@ def _from_ai_parameters(value: Any) -> list[dict[str, str | None]]: # --- MCP Client Tool selection-mode helpers --------------------------------- -def _selection_mode(parameters: dict[str, Any]) -> str: - value = _top_level_string( - parameters, - {"toolSelection", "toolsToInclude", "toolSelectionMode"}, +#: The spellings this reader read a selection mode from before #1025. n8n +#: writes none of them; they stay aliases on the MCP Client Tool and remain +#: the only ones read for any other node, such as the MCP Server Trigger. +#: Ordered, so the first one present decides under any hash seed. +_LEGACY_SELECTION_MODE_KEYS = ("toolSelection", "toolsToInclude", "toolSelectionMode") +#: The parameter n8n's MCP Client Tool selects its tools with, `include` — +#: `all` (the default, which an export leaves out), `selected` or `except` — +#: read before the earlier spellings (#1025). +_CLIENT_SELECTION_MODE_KEYS = ("include", *_LEGACY_SELECTION_MODE_KEYS) +#: Where a node lists the tools `selected` keeps: n8n's `includeTools`, then +#: the earlier spellings. +_INCLUDED_TOOL_KEYS = ("includeTools", "tools", "toolNames", "selectedTools", "toolName") +#: Where a node lists the tools `except` leaves out: n8n's `excludeTools`. +_EXCLUDED_TOOL_KEYS = ("excludeTools",) + + +def _selection_mode(parameters: dict[str, Any], *, client: bool = False) -> str: + """How an n8n MCP node selects the tools it exposes. + + ``client`` is n8n's MCP Client Tool, read as n8n runs it (#1025): its + `include` first, whose `except` is all-except; `all` when it names neither + a mode nor a tool, since an export leaves the default out; and `all` for a + `selected` that lists no tool, since n8n filters by `includeTools` only + when it names one. Any other node, such as the MCP Server Trigger, which + has no selection of its own, is read by the earlier spellings only, as it + was before. + """ + + keys = _CLIENT_SELECTION_MODE_KEYS if client else _LEGACY_SELECTION_MODE_KEYS + value = next( + ( + raw.strip() + for key in keys + if isinstance(raw := parameters.get(key), str) and raw.strip() + ), + None, ) normalized = (value or "").lower().replace(" ", "_").replace("-", "_") if normalized in {"all", "all_tools", "alltools"}: return "all" - if normalized in {"all_except", "allexcept"}: + if normalized in {"all_except", "allexcept"} or (client and normalized == "except"): return "all_except" if normalized in {"selected", "selected_tools", "specific"}: + return "selected" if not client or _selected_mcp_tools(parameters) else "all" + if value: + return "unknown" + if _selected_mcp_tools(parameters): return "selected" - selected = _selected_mcp_tools(parameters) - if selected: - return "selected" - return "unknown" if value else "unspecified" + return "all" if client else "unspecified" def _is_unfiltered_mode(parameters: dict[str, Any]) -> bool: @@ -467,8 +501,16 @@ def _is_unfiltered_mode(parameters: dict[str, Any]) -> bool: def _selected_mcp_tools(parameters: dict[str, Any]) -> list[str]: + return _mcp_tool_names(parameters, _INCLUDED_TOOL_KEYS) + + +def _excluded_mcp_tools(parameters: dict[str, Any]) -> list[str]: + return _mcp_tool_names(parameters, _EXCLUDED_TOOL_KEYS) + + +def _mcp_tool_names(parameters: dict[str, Any], keys: tuple[str, ...]) -> list[str]: values: list[str] = [] - for key in ("tools", "toolNames", "selectedTools", "includeTools", "toolName"): + for key in keys: raw = parameters.get(key) if isinstance(raw, str): if raw.strip(): diff --git a/tests/test_adopter_vocabulary.py b/tests/test_adopter_vocabulary.py index d695bce03..2f9b058fa 100644 --- a/tests/test_adopter_vocabulary.py +++ b/tests/test_adopter_vocabulary.py @@ -757,6 +757,9 @@ def test_an_unknown_agent_falls_back_to_something_readable() -> None: "ambiguous_inventory_merge_warning": sw.ambiguous_inventory_merge_warning( "tools/inventory.json", "orders_a", ["process_order"] ), + "unread_host_mcp_config_warning": sw.unread_host_mcp_config_warning( + "packages/api/.cursor/mcp.json", "Cursor", [".mcp.json", ".codex/config.toml"] + ), } # Public names in `core.source_warnings` that do not write a message: the diff --git a/tests/test_mcp_manifest.py b/tests/test_mcp_manifest.py index 38ddc85e3..95fe21fdb 100644 --- a/tests/test_mcp_manifest.py +++ b/tests/test_mcp_manifest.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import re from pathlib import Path import pytest @@ -13,6 +14,9 @@ _RISK_MONOTONE_ANNOTATIONS, _RISK_UNION_ANNOTATIONS, DUPLICATE_SERVER_DECLARATION, + READ_MCP_CONFIG_FILES, + UNREAD_HOST_MCP_CONFIG_FILES, + _mcp_config_files, load_codex_config_mcp_sources, normalize_codex_config_mcp_servers, normalize_mcp_json_servers, @@ -1091,3 +1095,309 @@ def test_two_servers_in_one_file_may_expose_the_same_tool_name(tmp_path: Path) - ) assert sorted(row["name"] for row in report.tool_catalog) == ["query", "query"] + + +# --- secret forwarding by name, and the files a codex_config row reads (#1025) --- + + +def _reviewed_codex_manifest_text() -> str: + """The codex row bound to the agent with its authority declared. + + An unbound tool is outside the surface the checks read, so a finding about + one could never fire; and a passing decision needs the authority stated. + """ + + return _codex_manifest_text().replace( + " path: .\n", + " path: .\n" + " binding:\n" + " complete: true\n" + " reason: reviewed MCP server surface\n" + " authority:\n" + " mode: none\n", + ) + + +def _passthrough_findings(tmp_path: Path, filename: str, body: str): + target = tmp_path / filename + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(body, encoding="utf-8") + (tmp_path / "shipgate.yaml").write_text(_reviewed_codex_manifest_text(), encoding="utf-8") + report, _exit_code = run_scan( + config_path=tmp_path / "shipgate.yaml", + output_dir=tmp_path / "agents-shipgate-reports", + ) + return [ + finding + for finding in report.findings + if finding.check_id == "SHIP-MCP-ENV-SECRET-PASSTHROUGH" + ] + + +@pytest.mark.parametrize( + ("filename", "body"), + [ + ( + ".codex/config.toml", + '[mcp_servers.github]\ncommand = "github-mcp"\n' + 'env_vars = ["GITHUB_TOKEN"]\nenabled_tools = ["read_issue"]\n', + ), + ( + ".codex/config.toml", + '[mcp_servers.github]\nurl = "https://mcp.example.com/github"\n' + 'bearer_token_env_var = "GITHUB_TOKEN"\nenabled_tools = ["read_issue"]\n', + ), + ( + ".codex/config.toml", + '[mcp_servers.github]\nurl = "https://mcp.example.com/github"\n' + 'env_http_headers = { "X-GitHub-Token" = "GITHUB_TOKEN" }\n' + 'enabled_tools = ["read_issue"]\n', + ), + ( + ".mcp.json", + json.dumps( + { + "mcpServers": { + "github": { + "type": "http", + "url": "https://mcp.example.com/github", + "headers": {"Authorization": "Bearer ${GITHUB_TOKEN}"}, + "tools": {"read_issue": {}}, + } + } + } + ), + ), + ], + ids=["env_vars", "bearer_token_env_var", "env_http_headers", "headers"], +) +def test_a_secret_forwarded_by_name_fires_the_passthrough_finding( + tmp_path: Path, filename: str, body: str +) -> None: + """Only the `env` table was read, so these forms of the same exposure passed.""" + + findings = _passthrough_findings(tmp_path, filename, body) + + assert [finding.evidence["env_secret_names"] for finding in findings] == [["GITHUB_TOKEN"]] + + +@pytest.mark.parametrize( + ("header", "variable"), + [("Authorization", "GITHUB_PAT_VALUE"), ("X-Request-Source", "GITHUB_TOKEN")], + ids=["credential_header", "secret_variable"], +) +def test_a_forwarded_header_counts_when_the_header_or_the_variable_is_secret( + tmp_path: Path, header: str, variable: str +) -> None: + (tmp_path / ".codex").mkdir() + (tmp_path / ".codex" / "config.toml").write_text( + '[mcp_servers.github]\nurl = "https://mcp.example.com/github"\n' + f'env_http_headers = {{ "{header}" = "{variable}" }}\n' + 'enabled_tools = ["read_issue"]\n', + encoding="utf-8", + ) + + tool = load_codex_config_mcp_sources(tmp_path, tmp_path)[0].tools[0] + + assert tool.annotations["mcp_env_secret_names"] == [variable] + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("Bearer ${env:GITHUB_TOKEN}", ["GITHUB_TOKEN"]), + ("Bearer ${GITHUB_TOKEN:-}", ["GITHUB_TOKEN"]), + ("Bearer ${GITHUB_TOKEN:?x}", ["GITHUB_TOKEN"]), + ("Bearer ${GITHUB_TOKEN:=d}", ["GITHUB_TOKEN"]), + # A prompted input is not an environment variable, so it names none. + ("Bearer ${input:githubToken}", None), + ], + ids=["env_prefix", "default_value", "required_value", "assigned_default", "input_prefix"], +) +def test_a_prefixed_reference_names_the_variable_not_its_prefix( + tmp_path: Path, value: str, expected: list[str] | None +) -> None: + """`${env:NAME}` was read as a variable named `env`, `${input:id}` as `input` (#1072 review).""" + + (tmp_path / ".mcp.json").write_text( + json.dumps( + { + "mcpServers": { + "github": { + "type": "http", + "url": "https://mcp.example.com/github", + "headers": {"Authorization": value}, + "tools": {"read_issue": {}}, + } + } + } + ), + encoding="utf-8", + ) + + tool = load_codex_config_mcp_sources(tmp_path, tmp_path)[0].tools[0] + + assert tool.annotations.get("mcp_env_secret_names") == expected + + +def test_a_forwarded_name_that_is_not_secret_is_not_counted(tmp_path: Path) -> None: + (tmp_path / ".codex").mkdir() + (tmp_path / ".codex" / "config.toml").write_text( + '[mcp_servers.tools]\nurl = "https://mcp.example.com/tools"\n' + 'env_vars = ["PATH", "HOME", "NAME=value"]\n' + 'env_http_headers = { "X-Request-Source" = "HOSTNAME" }\n' + 'headers = { "X-Trace" = "${TRACE_ID}" }\n' + 'enabled_tools = ["read_issue"]\n', + encoding="utf-8", + ) + + tool = load_codex_config_mcp_sources(tmp_path, tmp_path)[0].tools[0] + + assert "mcp_env_secret_names" not in tool.annotations + + +def test_a_bearer_token_written_where_its_variable_belongs_is_not_published( + tmp_path: Path, +) -> None: + """`bearer_token_env_var` always counts, and a value in it is redacted, not echoed.""" + + token = "ghp_" + "a" * 36 + findings = _passthrough_findings( + tmp_path, + ".codex/config.toml", + '[mcp_servers.github]\nurl = "https://mcp.example.com/github"\n' + f'bearer_token_env_var = "{token}"\nenabled_tools = ["read_issue"]\n', + ) + + assert len(findings) == 1 + assert token not in json.dumps(findings[0].evidence) + report_text = (tmp_path / "agents-shipgate-reports" / "report.json").read_text( + encoding="utf-8" + ) + assert token not in report_text + + +def _write_tree(root: Path, paths: list[str]) -> None: + for rel in paths: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("{}", encoding="utf-8") + + +def test_the_files_a_codex_config_row_reads_are_the_files_its_coverage_names() -> None: + """The coverage text said it read "the other host MCP config files" too. + + It is now written from the two tuples the walk matches with. This pins the + rest: the text names nothing else, the published page carries it, and every + MCP config file the host readers recognise is one of the two. + """ + + from agents_shipgate.core.boundary_registry import BOUNDARY_ADAPTERS + from agents_shipgate.inputs.codex_config import CodexConfigAdapter + + reads = CodexConfigAdapter.coverage.reads + unread = tuple(path for path, _host in UNREAD_HOST_MCP_CONFIG_FILES) + named = set(re.findall(r"`([^`]+\.(?:json|toml))`", reads)) + assert named == {*READ_MCP_CONFIG_FILES, *unread} + assert not set(READ_MCP_CONFIG_FILES) & set(unread) + + page = (Path(__file__).resolve().parents[1] / "docs" / "determinism-boundary.md").read_text( + encoding="utf-8" + ) + assert reads in page + + recognised = { + path + for adapter in BOUNDARY_ADAPTERS + if adapter.id != "shared" + for path in adapter.exact_paths + if path.endswith("mcp.json") or path == ".codex/config.toml" + } + assert recognised == {*READ_MCP_CONFIG_FILES, *unread} + + +def test_the_walk_matches_exactly_the_files_the_coverage_names(tmp_path: Path) -> None: + unread = [path for path, _host in UNREAD_HOST_MCP_CONFIG_FILES] + named = [*READ_MCP_CONFIG_FILES, *unread] + _write_tree( + tmp_path, + [ + *named, + *(f"pkg/{path}" for path in named), + # Near misses: none of them is an MCP config file the reader names. + "mcp.json", + "config.toml", + ".cursor/cli.json", + ".vscode/settings.json", + "node_modules/pkg/.cursor/mcp.json", + ], + ) + + read_files, unread_files = _mcp_config_files(tmp_path) + + def rel(path: Path) -> str: + return path.relative_to(tmp_path).as_posix() + + assert [rel(path) for path in read_files] == sorted( + [*READ_MCP_CONFIG_FILES, *(f"pkg/{path}" for path in READ_MCP_CONFIG_FILES)] + ) + assert [(rel(path), host) for path, host in unread_files] == sorted( + [ + *((path, host) for path, host in UNREAD_HOST_MCP_CONFIG_FILES), + *((f"pkg/{path}", host) for path, host in UNREAD_HOST_MCP_CONFIG_FILES), + ] + ) + + +def test_a_workspace_whose_servers_live_in_another_hosts_file_does_not_pass( + tmp_path: Path, +) -> None: + """The reported workspace: a reviewed Codex config passed with nothing said. + + `.codex/config.toml` declares one reviewed read-only server; Cursor's and + VS Code's files declare a token-forwarding server and a shell server. The + scan read the first, passed, and named neither of the others. + """ + + (tmp_path / ".codex").mkdir() + (tmp_path / ".codex" / "config.toml").write_text( + '[mcp_servers.docs]\ncommand = "docs-mcp"\n' + "[mcp_servers.docs.tools.read_docs]\nread_only = true\n" + 'description = "Read local documentation."\n' + 'input_schema = { type = "object", properties = { path = { type = "string" } } }\n', + encoding="utf-8", + ) + _mcp_workspace( + tmp_path, + {".cursor/mcp.json": {"github": {"command": "npx", "env": {"GITHUB_TOKEN": "x"}}}}, + ) + (tmp_path / ".vscode").mkdir() + (tmp_path / ".vscode" / "mcp.json").write_text( + json.dumps({"servers": {"bash": {"type": "stdio", "command": "bash-mcp"}}}), + encoding="utf-8", + ) + (tmp_path / "shipgate.yaml").write_text(_reviewed_codex_manifest_text(), encoding="utf-8") + + report, _exit_code = run_scan( + config_path=tmp_path / "shipgate.yaml", + output_dir=tmp_path / "agents-shipgate-reports", + ) + + assert [row["name"] for row in report.tool_catalog] == ["read_docs"] + unread = [warning for warning in report.source_warnings if "was not read" in warning] + assert [warning.split(" was not read")[0] for warning in unread] == [ + "Cursor MCP config '.cursor/mcp.json'", + "VS Code MCP config '.vscode/mcp.json'", + ] + assert report.release_decision.decision != "passed" + + # The same workspace without the other hosts' files is the one that passed, + # so the warnings are what moved the decision. + (tmp_path / ".cursor" / "mcp.json").unlink() + (tmp_path / ".vscode" / "mcp.json").unlink() + report, _exit_code = run_scan( + config_path=tmp_path / "shipgate.yaml", + output_dir=tmp_path / "agents-shipgate-reports", + ) + assert report.source_warnings == [] + assert report.release_decision.decision == "passed" diff --git a/tests/test_n8n.py b/tests/test_n8n.py index f8297fe67..2f6bcc78b 100644 --- a/tests/test_n8n.py +++ b/tests/test_n8n.py @@ -20,6 +20,7 @@ from agents_shipgate.core.risk_hints import enrich_tools_with_risk_hints, risk_tags from agents_shipgate.inputs import common as inputs_common from agents_shipgate.inputs.n8n import load_n8n_artifacts +from agents_shipgate.inputs.n8n._tools import _is_unfiltered_mode, _selection_mode def test_n8n_top_level_config_is_accepted_and_tool_source_type_is_rejected(tmp_path): @@ -406,15 +407,42 @@ def test_n8n_node_id_identity_survives_prepended_node(tmp_path): assert finding.evidence.get("source_ref") is None -def test_n8n_mcp_operation_field_does_not_create_wildcard_selection(tmp_path): +#: `parameters` of n8n's MCP Client Tool node (`@n8n/n8n-nodes-langchain.mcpClientTool`) +#: as n8n exports them. `include` selects the tools — `all`, `selected` or +#: `except` — and is left out at its default, `all`, so the default export +#: names no selection at all (#1025). +_MCP_CLIENT_ENDPOINT = {"endpointUrl": "https://mcp.example.com/github", "options": {}} +_MCP_CLIENT_EXPORTS = { + "default": dict(_MCP_CLIENT_ENDPOINT), + "include_all": {**_MCP_CLIENT_ENDPOINT, "include": "all"}, + "include_except": { + **_MCP_CLIENT_ENDPOINT, + "include": "except", + "excludeTools": ["list_issues", "delete_repository"], + }, + "include_selected": { + **_MCP_CLIENT_ENDPOINT, + "include": "selected", + "includeTools": ["get_issue", "list_issues"], + # The node's `options` collection is not a tool selection, whatever a + # key in it says. + "options": {"operation": "all"}, + }, + # n8n filters by `includeTools` only when it names a tool. + "include_selected_empty": {**_MCP_CLIENT_ENDPOINT, "include": "selected"}, +} + + +def _scan_mcp_client_export(tmp_path, parameters): project = tmp_path / "project" project.mkdir() workflow = _minimal_agent_workflow( { "id": "mcp-client", - "name": "MCP Client", - "type": "n8n-nodes-langchain.toolMcp", - "parameters": {"include": "All", "options": {"operation": "all"}}, + "name": "GitHub MCP", + "type": "@n8n/n8n-nodes-langchain.mcpClientTool", + "typeVersion": 1.1, + "parameters": parameters, } ) (project / "workflow.json").write_text(json.dumps(workflow), encoding="utf-8") @@ -422,7 +450,7 @@ def test_n8n_mcp_operation_field_does_not_create_wildcard_selection(tmp_path): """ version: "0.1" project: - name: n8n-mcp-operation + name: n8n-mcp-client-export agent: name: n8n-agent environment: @@ -433,20 +461,123 @@ def test_n8n_mcp_operation_field_does_not_create_wildcard_selection(tmp_path): """, encoding="utf-8", ) - report, _ = run_scan( config_path=project / "shipgate.yaml", output_dir=tmp_path / "reports", formats=["json"], ci_mode="advisory", ) + return report + + +@pytest.mark.parametrize( + ("export", "selection_mode"), + [ + ("default", "all"), + ("include_all", "all"), + ("include_except", "all_except"), + ("include_selected_empty", "all"), + ], +) +def test_n8n_mcp_client_export_exposing_every_tool_is_unfiltered( + tmp_path, export, selection_mode +): + """Each export n8n runs with every tool, or every tool but some, of the server. + + The reader read only spellings n8n never wrote, so the default export — + which carries no `include` — entered the catalog as one concretely named + tool: no wildcard, no dynamic surface and no UNFILTERED finding. + """ + + report = _scan_mcp_client_export(tmp_path, _MCP_CLIENT_EXPORTS[export]) + + assert [tool["name"] for tool in report.tool_inventory] == ["GitHub MCP.*"] + assert report.frameworks["n8n"]["dynamic_tool_surface_count"] == 1 + dynamic = [ + finding + for finding in report.findings + if finding.check_id == "SHIP-N8N-DYNAMIC-TOOL-SURFACE-NOT-ENUMERABLE" + ] + assert [finding.evidence["surface"]["kind"] for finding in dynamic] == [ + "mcp_client_wildcard" + ] + unfiltered = [ + finding + for finding in report.findings + if finding.check_id == "SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED" + ] + assert len(unfiltered) == 1 + assert unfiltered[0].severity == "high" + assert unfiltered[0].evidence["selection_mode"] == selection_mode + if selection_mode == "all_except": + assert unfiltered[0].evidence["excluded_tools"] == [ + "delete_repository", + "list_issues", + ] + else: + assert "excluded_tools" not in unfiltered[0].evidence + assert report.release_decision.decision != "passed" + +def test_n8n_mcp_client_export_selecting_tools_names_them(tmp_path): + report = _scan_mcp_client_export(tmp_path, _MCP_CLIENT_EXPORTS["include_selected"]) + + assert sorted(tool["name"] for tool in report.tool_inventory) == [ + "get_issue", + "list_issues", + ] assert report.frameworks["n8n"]["dynamic_tool_surface_count"] == 0 assert "SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED" not in { finding.check_id for finding in report.findings } +def test_n8n_mcp_client_include_outranks_a_legacy_spelling(tmp_path): + """`include` decides when an earlier spelling is also present, under any hash seed.""" + + report = _scan_mcp_client_export( + tmp_path, + { + **_MCP_CLIENT_EXPORTS["include_selected"], + "toolSelection": "all", + "toolsToInclude": "All", + }, + ) + + assert sorted(tool["name"] for tool in report.tool_inventory) == [ + "get_issue", + "list_issues", + ] + assert report.frameworks["n8n"]["dynamic_tool_surface_count"] == 0 + + +@pytest.mark.parametrize( + ("parameters", "trigger_unfiltered", "client_mode"), + [ + # Shapes n8n gives only the MCP Client Tool: the trigger reads none. + ({"include": "all"}, False, "all"), + ({"include": "except"}, False, "all_except"), + ({"toolSelection": "selected"}, False, "all"), + ({}, False, "all"), + # The earlier spellings both have always read. + ({"toolsToInclude": "All"}, True, "all"), + ({"toolSelection": "All Except"}, True, "all_except"), + ], +) +def test_n8n_mcp_server_trigger_keeps_the_selection_it_read( + parameters, trigger_unfiltered, client_mode +): + """n8n's `include` and its defaults belong to the client node (#1025 review). + + The MCP Server Trigger has no selection of its own; it is read by the + earlier spellings only, as before, so a shape n8n gives only the client + adds no `mcp_server_wildcard` surface to a trigger. + """ + + assert _is_unfiltered_mode(parameters) is trigger_unfiltered + assert _selection_mode(parameters, client=True) == client_mode + + def test_n8n_nested_send_and_wait_text_does_not_count_as_human_review(tmp_path): project = tmp_path / "project" project.mkdir()