From 0651e7e6e681eaeefb912ba20c3b5ccc99f1c551 Mon Sep 17 00:00:00 2001 From: pengfei-threemoonslab Date: Tue, 13 Oct 2026 12:57:14 -0700 Subject: [PATCH 1/3] fix(readers): read n8n include/excludeTools and the MCP keys and files hosts export The n8n MCP Client Tool reader read only spellings n8n never writes, so the default export (no `include`), `include: all` and `include: except` entered the catalog as one named tool and SHIP-N8N-MCP-CLIENT-TOOLSET-UNFILTERED never fired. It now reads `include` (absent means `all`), `includeTools` and `excludeTools`, keeping the earlier spellings as ordered aliases. SHIP-MCP-ENV-SECRET-PASSTHROUGH read only a server's `env` table; it now also counts secrets forwarded through `env_vars`, `bearer_token_env_var`, `env_http_headers` and `${NAME}` references in `headers`. The codex_config coverage claimed to read other hosts' MCP config files. The read set and the recognised-but-unread set are now constants the walk and the coverage text are both built from, and a .cursor/mcp.json or .vscode/mcp.json under the source root is named in a source warning instead of passing silently. Refs #1025 --- CHANGELOG.md | 4 + STABILITY.md | 55 ++++ docs/checks.json | 7 +- docs/checks.md | 16 +- docs/checks/mcp_permissions.yaml | 5 +- docs/checks/n8n.yaml | 6 +- docs/determinism-boundary.json | 4 +- docs/determinism-boundary.md | 4 +- llms-full.txt | 16 +- src/agents_shipgate/checks/n8n.py | 19 +- src/agents_shipgate/core/source_warnings.py | 21 ++ src/agents_shipgate/inputs/codex_config.py | 28 +- src/agents_shipgate/inputs/mcp_manifest.py | 137 ++++++++-- src/agents_shipgate/inputs/n8n/_adapter.py | 8 +- src/agents_shipgate/inputs/n8n/_tools.py | 82 ++++-- tests/test_adopter_vocabulary.py | 3 + tests/test_mcp_manifest.py | 272 ++++++++++++++++++++ tests/test_n8n.py | 115 ++++++++- 18 files changed, 716 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 668e32685..25f88aa67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ ### 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 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) + - **`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 --application` reads bounded local factories returning OpenAI Agents SDK and Google ADK tool lists. It keeps the returned members and their construction locations, and reports unread constructor identity, import effects, escaping values, or unresolved callers as coverage gaps rather than treating them as established wiring. This is static source analysis; it does not execute a factory or establish runtime enforcement. (#874, #964) diff --git a/STABILITY.md b/STABILITY.md index 49be617ea..df0d232e7 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 — `attest`, `org bundle` and the Action validate the verification receipt (#1022) 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..f1bdafd5e 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,13 @@ 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. 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..7de53be58 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,13 @@ 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. 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..355d9dc7c 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, @@ -40,6 +42,7 @@ _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_]*)[})]?") +_ENV_VAR_NAME_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") _SKIPPED_SCAN_DIRS = { ".git", ".hg", @@ -55,6 +58,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 +203,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 +231,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 +336,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 +1030,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 +1101,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 +1114,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 +1142,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 +1174,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..cfb751ca0 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, absent="all") + 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,47 @@ 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 parameter n8n's MCP Client Tool selects its tools with, `include` — +#: `all` (the default, which an export leaves out), `selected` or `except` — +#: then the spellings this reader read before it, kept as aliases (#1025). +#: Ordered, so the first one present decides under any hash seed. +_SELECTION_MODE_KEYS = ("include", "toolSelection", "toolsToInclude", "toolSelectionMode") +#: 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], *, absent: str = "unspecified") -> str: + """How an n8n MCP node selects the tools it exposes. + + ``absent`` is the mode of a node that names neither a mode nor a tool: + ``all`` for an MCP Client Tool, whose `include` an export leaves out at + its default. A `selected` node that lists no tool is ``all``, because n8n + filters by `includeTools` only when it names one. + """ + + value = next( + ( + raw.strip() + for key in _SELECTION_MODE_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 {"except", "all_except", "allexcept"}: return "all_except" if normalized in {"selected", "selected_tools", "specific"}: + return "selected" if _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 absent def _is_unfiltered_mode(parameters: dict[str, Any]) -> bool: @@ -467,8 +493,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..ce8cc3909 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,271 @@ 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] + + +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..1f700d66c 100644 --- a/tests/test_n8n.py +++ b/tests/test_n8n.py @@ -406,15 +406,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 +449,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 +460,96 @@ 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 + + def test_n8n_nested_send_and_wait_text_does_not_count_as_human_review(tmp_path): project = tmp_path / "project" project.mkdir() From 227a21d32730c0a5b8b2266fae4cffa285912f4d Mon Sep 17 00:00:00 2001 From: pengfei-threemoonslab Date: Tue, 13 Oct 2026 13:14:22 -0700 Subject: [PATCH 2/3] fix(readers): keep the trigger's selection reading and read ${env:NAME} as NAME The MCP Server Trigger shared the client node's new selection reading, so include: all/except and an empty selected added an mcp_server_wildcard surface to a trigger, a node n8n gives no selection. Only the MCP Client Tool now reads include and its defaults; the trigger reads the earlier spellings as before. The env reference pattern stopped at ':', so ${env:GITHUB_TOKEN} published 'env' and ${input:x} 'input' as forwarded variables. ${env:NAME} is now read as NAME, ${NAME:-default} still as NAME, and any other ${prefix:...} names no variable. Refs #1025 --- CHANGELOG.md | 2 +- docs/checks.md | 3 +- llms-full.txt | 3 +- src/agents_shipgate/inputs/mcp_manifest.py | 8 ++++- src/agents_shipgate/inputs/n8n/_tools.py | 34 ++++++++++++-------- tests/test_mcp_manifest.py | 36 ++++++++++++++++++++++ tests/test_n8n.py | 28 +++++++++++++++++ 7 files changed, 97 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b51835ca8..e0d813e9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ ### 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 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) +- **`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) diff --git a/docs/checks.md b/docs/checks.md index f1bdafd5e..78a07f12a 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -1242,7 +1242,8 @@ 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. Secret pass-through changes the credential +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. diff --git a/llms-full.txt b/llms-full.txt index 7de53be58..1ddb9a685 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -4660,7 +4660,8 @@ 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. Secret pass-through changes the credential +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. diff --git a/src/agents_shipgate/inputs/mcp_manifest.py b/src/agents_shipgate/inputs/mcp_manifest.py index 355d9dc7c..05c8fb0c6 100644 --- a/src/agents_shipgate/inputs/mcp_manifest.py +++ b/src/agents_shipgate/inputs/mcp_manifest.py @@ -41,7 +41,13 @@ _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}`, +#: `${NAME:-default}`, `$(NAME)`, or the editors' `${env:NAME}`, read as +#: `NAME`. Any other `${prefix:…}`, 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_])(?!:(?!-))" +) _ENV_VAR_NAME_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") _SKIPPED_SCAN_DIRS = { ".git", diff --git a/src/agents_shipgate/inputs/n8n/_tools.py b/src/agents_shipgate/inputs/n8n/_tools.py index cfb751ca0..387460b66 100644 --- a/src/agents_shipgate/inputs/n8n/_tools.py +++ b/src/agents_shipgate/inputs/n8n/_tools.py @@ -256,7 +256,7 @@ def _mcp_client_tools( # 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, absent="all") + 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), @@ -445,11 +445,15 @@ def _from_ai_parameters(value: Any) -> list[dict[str, str | None]]: # --- MCP Client Tool selection-mode helpers --------------------------------- +#: 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` — -#: then the spellings this reader read before it, kept as aliases (#1025). -#: Ordered, so the first one present decides under any hash seed. -_SELECTION_MODE_KEYS = ("include", "toolSelection", "toolsToInclude", "toolSelectionMode") +#: 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") @@ -457,19 +461,23 @@ def _from_ai_parameters(value: Any) -> list[dict[str, str | None]]: _EXCLUDED_TOOL_KEYS = ("excludeTools",) -def _selection_mode(parameters: dict[str, Any], *, absent: str = "unspecified") -> str: +def _selection_mode(parameters: dict[str, Any], *, client: bool = False) -> str: """How an n8n MCP node selects the tools it exposes. - ``absent`` is the mode of a node that names neither a mode nor a tool: - ``all`` for an MCP Client Tool, whose `include` an export leaves out at - its default. A `selected` node that lists no tool is ``all``, because n8n - filters by `includeTools` only when it names one. + ``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 _SELECTION_MODE_KEYS + for key in keys if isinstance(raw := parameters.get(key), str) and raw.strip() ), None, @@ -477,15 +485,15 @@ def _selection_mode(parameters: dict[str, Any], *, absent: str = "unspecified") normalized = (value or "").lower().replace(" ", "_").replace("-", "_") if normalized in {"all", "all_tools", "alltools"}: return "all" - if normalized in {"except", "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 _selected_mcp_tools(parameters) else "all" + return "selected" if not client or _selected_mcp_tools(parameters) else "all" if value: return "unknown" if _selected_mcp_tools(parameters): return "selected" - return absent + return "all" if client else "unspecified" def _is_unfiltered_mode(parameters: dict[str, Any]) -> bool: diff --git a/tests/test_mcp_manifest.py b/tests/test_mcp_manifest.py index ce8cc3909..f94016071 100644 --- a/tests/test_mcp_manifest.py +++ b/tests/test_mcp_manifest.py @@ -1202,6 +1202,42 @@ def test_a_forwarded_header_counts_when_the_header_or_the_variable_is_secret( assert tool.annotations["mcp_env_secret_names"] == [variable] +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("Bearer ${env:GITHUB_TOKEN}", ["GITHUB_TOKEN"]), + ("Bearer ${GITHUB_TOKEN:-}", ["GITHUB_TOKEN"]), + # A prompted input is not an environment variable, so it names none. + ("Bearer ${input:githubToken}", None), + ], + ids=["env_prefix", "default_value", "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( diff --git a/tests/test_n8n.py b/tests/test_n8n.py index 1f700d66c..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): @@ -550,6 +551,33 @@ def test_n8n_mcp_client_include_outranks_a_legacy_spelling(tmp_path): 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() From 53d9ac8ca42041ff89c8ac60bf19686f732d33a7 Mon Sep 17 00:00:00 2001 From: pengfei-threemoonslab Date: Tue, 13 Oct 2026 13:18:07 -0700 Subject: [PATCH 3/3] fix(readers): keep counting ${NAME:?msg} and ${NAME:=default} references The cycle-1 lookahead rejected every ':' but ':-', so the POSIX ${NAME:?msg} and ${NAME:=default} forms named no variable where main counted NAME. Only a ':' followed by an identifier character, a ${prefix:id} such as ${input:id}, is now skipped. Refs #1025 --- src/agents_shipgate/inputs/mcp_manifest.py | 11 ++++++----- tests/test_mcp_manifest.py | 4 +++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/agents_shipgate/inputs/mcp_manifest.py b/src/agents_shipgate/inputs/mcp_manifest.py index 05c8fb0c6..c93b0a73f 100644 --- a/src/agents_shipgate/inputs/mcp_manifest.py +++ b/src/agents_shipgate/inputs/mcp_manifest.py @@ -41,12 +41,13 @@ _TOOLS_KEYS = ("tools", "tool_overrides") _SCHEMA_KEYS = ("inputSchema", "input_schema", "schema") _OUTPUT_SCHEMA_KEYS = ("outputSchema", "output_schema") -#: An environment-variable reference in a value: `$NAME`, `${NAME}`, -#: `${NAME:-default}`, `$(NAME)`, or the editors' `${env:NAME}`, read as -#: `NAME`. Any other `${prefix:…}`, such as VS Code's `${input:id}`, names no -#: environment variable, so its prefix is not read as one. +#: 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_])(?!:(?!-))" + 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 = { diff --git a/tests/test_mcp_manifest.py b/tests/test_mcp_manifest.py index f94016071..95fe21fdb 100644 --- a/tests/test_mcp_manifest.py +++ b/tests/test_mcp_manifest.py @@ -1207,10 +1207,12 @@ def test_a_forwarded_header_counts_when_the_header_or_the_variable_is_secret( [ ("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", "input_prefix"], + 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