diff --git a/docs/MCP_TOOLS.md b/docs/MCP_TOOLS.md index 1736a2697..76995a531 100644 --- a/docs/MCP_TOOLS.md +++ b/docs/MCP_TOOLS.md @@ -166,7 +166,6 @@ Synthesize whole-repository symbol dependency and reference graph. |---|---|---|---|---| | `target_dir` | `string` | No | - | Target source directory to verify or analyze. | | `max_files` | `integer` | No | `100` | Maximum source files to include. | -| `output_path` | `string` | No | - | Destination file path for output report or artifacts. | | `fmt` | `string` | No | `json` | Output format for synthesized code graph: 'json' or 'dot'. | ### `ai_ast_parse` @@ -1381,7 +1380,6 @@ Stream runtime security anomaly events from Kubernetes Falco eBPF probes. | `simulate` | `boolean` | No | `False` | Generate simulated kernel eBPF security anomalies for testing. | | `dry_run` | `boolean` | No | `False` | Preview execution plan without mutating external state. | | `json_output` | `boolean` | No | `False` | Output findings or metrics as JSON. | -| `output` | `string` | No | - | Export discovered alerts to JSON file | ### `k8s_status` diff --git a/openspec/changes/3236-read-only-tools-take-paths-that/design.md b/openspec/changes/3236-read-only-tools-take-paths-that/design.md deleted file mode 100644 index 5d7ef5bfe..000000000 --- a/openspec/changes/3236-read-only-tools-take-paths-that/design.md +++ /dev/null @@ -1,18 +0,0 @@ -# Design - -## Decisions - -- **Keep `_PATH_PARAM_RESTRICTIONS` as the only place that decides which options a tool publishes.**: SECURITY.md rule 11 names it as the schema gate, and `generate_mcp_tools` builds every published schema from it. -- **Put the confinement data in a new `src/devops_cli/ai/mcp/path_confinement.py`: `PATH_CONFINEMENTS` (tool name to the parameters that it confines) and `NOT_A_FILE_PATH` (tool name to a parameter with its reason).**: The audit needs one place to read. A parameter that is a host and not a file, such as `target` of `security_intel_network`, then has a recorded reason and no silent exemption. -- **Remove `output_path` from `ai_ast_graph` and `output` from `k8s_security_stream`. Keep `target_dir` of `ai_ast_graph` and `root_dir` of `gh_pages_verify`, confined under the work tree.**: Both commands print their result when no file is given, so the model needs no output option. The two directory options select what the command reads, and a relative path under the work tree is useful. -- **Confine in `_check_path_confinement`, called in `_create_tool_function` beside `_check_oidc_token_reference`, before `_build_command_argv`.**: The existing guards of #2638 and #3222 use this pattern. A refusal reaches the caller as a validation error, and no command starts with a path outside the root. -- **Resolve the root from the work tree of the server process, never from a tool argument.**: A root that a model gives would move the boundary that confines the model. -- **The audit reads the tools of every job role through `tools_for_role`, matches a text parameter whose name holds a keyword of `_PATH_PARAM_KEYWORDS`, and requires an entry in one of the two tables.**: The issue asks for the role tools. A person who adds a tool name to a role table gets a failing test that names the tool and the parameter. -- **Do not audit the `ops` role in this change.**: The census in Feasibility shows 60 tools with a path-like parameter. A test over them needs a decision for each one, and that work is a separate item. The `ops` role is never served over the network. - -## Rejected alternatives - -- **Leave the path options as they are and rely on the read-only annotation.**: `generate_mcp_tools` sets `annotations={"readOnlyHint": read_only}` and nothing else checks the argument, so a model value reaches the command unchanged. -- **Drop every path option, including `target_dir` and `root_dir`.**: `safe_resolve_subpath` already confines a path under a base with `resolved.is_relative_to(base)`, so a kept option is confined without losing the use of the command. -- **Audit the whole exposure table in this change.**: `tools_for_role("ops", ...)` holds 60 tools with a path-like parameter. One pull request cannot decide them all, and a test with a long exempt list hides the defect that it should show. -- **Add the path tables to `src/devops_cli/ai/mcp/roles.py`.**: That file decides which tools a role holds, and the path decision is about a parameter. Mixing them makes `roles.py` own two reasons to change. diff --git a/openspec/changes/3236-read-only-tools-take-paths-that/proposal.md b/openspec/changes/3236-read-only-tools-take-paths-that/proposal.md deleted file mode 100644 index 3a64c49ac..000000000 --- a/openspec/changes/3236-read-only-tools-take-paths-that/proposal.md +++ /dev/null @@ -1,45 +0,0 @@ -# Proposal - -## Why - -SECURITY.md rule 11 requires a path helper to confine a file path that a model gives to an MCP tool. #3236 asks that no tool of a role table accepts a path outside a confined root, and that a test finds an unconfined path parameter. The role tables of #3219 hold no path tool today, so the risk is a later edit: a person adds a tool name to a table and the tool carries a path option. Three tools that the tables leave out show the class: `ai_ast_graph` (`--output`, `--dir`), `k8s_security_stream` (`--output`) and `gh_pages_verify` (`--dir`). The `ops` role serves them, and a person on the command line chooses their paths. - -## What Changes - -- `_PATH_PARAM_RESTRICTIONS` in `src/devops_cli/ai/mcp/generator.py` gains an entry for `("ai", "ast", "graph")` and for `("k8s", "security-stream")`. The `output_path` option of the first and the `output` option of the second leave the published schema. The command of the first prints its graph, and the second prints its alerts, so the model needs no file. -- A new module `src/devops_cli/ai/mcp/path_confinement.py` holds two tables keyed by tool name. `PATH_CONFINEMENTS` names each kept path parameter (`target_dir` of `ai_ast_graph`, `root_dir` of `gh_pages_verify`). `NOT_A_FILE_PATH` names a text parameter whose name looks like a path and that is not a file path, with the reason (`target` of `security_intel_network` is a host or an address). -- `_create_tool_function` in `generator.py` calls a new `_check_path_confinement(tool_name, kwargs)` beside the other `_check_*` guards. It resolves each kept path argument through `safe_resolve_subpath` against the work tree of the server process, writes the resolved value back, and refuses an escape with a `ValidationError` that names the parameter. The root comes from the process and never from an argument. -- A new test `tests/ai/mcp/test_path_confinement.py` reads the published schema of every tool of every job role and fails on a text parameter whose name holds a keyword of `_PATH_PARAM_KEYWORDS` and that neither table names. It also covers the three tools above. -- The change carries a spec delta for the `mcp-tool-path-confinement` capability, because the published schema and the tool behavior are observable to a consumer. -- `docs/MCP_TOOLS.md` is regenerated: the `output_path` row of `ai_ast_graph` and the `output` row of `k8s_security_stream` go. -- The other tools of the `ops` role publish about 55 path parameters. Confining them is a separate item (see Workflow follow-up of `tasks.md`). -- The change fits one pull request. - -## Capabilities - -### New Capabilities - -- `mcp-tool-path-confinement`: A tool publishes no path option that the server can choose, A kept path option resolves under a server root, The schema audit names an unconfined path parameter of a role tool - -### Modified Capabilities - -None. - -## Impact - -- Code: `src/devops_cli/ai/mcp/path_confinement.py`, `src/devops_cli/ai/mcp/generator.py` -- Documents: `docs/MCP_TOOLS.md` -- Tests: `tests/ai/mcp/test_path_confinement.py`, `tests/ai/mcp/test_mcp.py`, `tests/ai/mcp/test_fastmcp_contracts.py` - -## Feasibility - -- Command output, `git cat-file -e 4571d5f3:src/devops_cli/ai/mcp/roles.py` (the base of this branch) exits 0, and `git show --stat 69234732` shows #3233 adding it. The role tables exist, and `tools_for_role` in that file gives the tools of a role. -- Output of a script that lists the published schema of `tools_for_role(role, catalogue)` for each of `JOB_ROLES`: the planning role holds 28 tools, the implementation role 17 and the review role 16. One text parameter of them has a path-like name: `target` of `security_intel_network`, which is a host. No role tool takes a file path today, so the audit passes at once, and it fails when a later edit adds a path tool. -- The same script over the whole catalogue lists 60 tools that publish a path-like parameter, for example `ai_ingest_library.output_dir`, `scan_sbom.output_file`, `tf_apply.plan_file` and `vault_get.path`. The change confines only the three tools of the issue. It does not claim that every tool passes. -- `src/devops_cli/commands/ai_ast.py` shows `ast_graph_cmd` with `--dir` (`target_dir`) and `--output` (`output_path`); without `--output` it prints the graph. `src/devops_cli/commands/k8s/diagnostics.py` shows `security_stream_cmd` with `--output`. `src/devops_cli/commands/gh.py` shows `pages_verify_cmd` with `--dir` (`root_dir`). The command names are `ai ast graph`, `k8s security-stream` and `gh pages verify`; the issue text `ai ast-graph` is wrong. -- `_PATH_PARAM_RESTRICTIONS` holds no entry for the first two commands and `("gh", "pages", "verify")` needs none, because its option stays. `_introspect_command_parameters` skips every option that an entry does not allow, and `("ai", "benchmark")` already drops `output`, `document` and `code` this way (#2704). -- `safe_resolve_subpath` in `src/devops_cli/core/paths.py` raises on `..`, on an absolute path outside the base and on a symlink escape. The helper the fix needs exists. - -## Existing Tool Considered - -No open, maintained library confines the path parameters of an MCP server's tools: FastMCP publishes a tool schema and runs the function, and pydantic validates types, not containment. The change reuses `safe_resolve_subpath` in `src/devops_cli/core/paths.py` and the `_PATH_PARAM_RESTRICTIONS` table of `src/devops_cli/ai/mcp/generator.py`, so it adds no dependency. The written cost of the small module and the test is about 15,000 tokens to build, about 4,000 tokens per year to keep and fix, and one day elapsed. The change deletes no internal code: no tool function, registry entry or test is removed. diff --git a/openspec/changes/3236-read-only-tools-take-paths-that/specs/mcp-tool-path-confinement/spec.md b/openspec/changes/3236-read-only-tools-take-paths-that/specs/mcp-tool-path-confinement/spec.md deleted file mode 100644 index 0567827bc..000000000 --- a/openspec/changes/3236-read-only-tools-take-paths-that/specs/mcp-tool-path-confinement/spec.md +++ /dev/null @@ -1,49 +0,0 @@ -## Purpose -Which path options the MCP tools publish, and how each kept path value is confined to a root that the server sets, so a model that calls a tool reaches no path outside the server work tree. The audit covers the tools of the job roles. - -## ADDED Requirements - -### Requirement: A tool publishes no path option that the server can choose -A tool MUST publish no path option when the server can choose the path that the command writes or reads. - -#### Scenario: The graph tool prints its graph -- **WHEN** A caller reads the published schema of the `ai_ast_graph` tool. -- **THEN** The schema holds no `output_path` property, and the command prints its graph. - -#### Scenario: The stream tool writes no file -- **WHEN** A caller reads the published schema of the `k8s_security_stream` tool. -- **THEN** The schema holds no `output` property, and the tool prints the stream. - -### Requirement: A kept path option resolves under a server root -Every path parameter that a tool publishes MUST resolve through `safe_resolve_subpath` against a root that the server sets before the server opens or writes the path. - -#### Scenario: A traversal value is refused -- **WHEN** A caller gives `../../etc/passwd` to the `root_dir` parameter of `gh_pages_verify`. -- **THEN** The tool returns a validation error that names the parameter, and no command starts. - -#### Scenario: A directory of the graph tool is confined -- **WHEN** A caller gives `/etc` to the `target_dir` parameter of `ai_ast_graph`. -- **THEN** The tool returns a validation error that names the parameter, and no command starts. - -#### Scenario: A relative value stays in the work tree -- **WHEN** A caller gives `docs` to the `root_dir` parameter of `gh_pages_verify`. -- **THEN** The tool resolves the value under the work tree of the server and runs the command with the resolved path. - -#### Scenario: The root is not a tool argument -- **WHEN** A caller reads the published schema of a tool that keeps a path parameter. -- **THEN** The schema holds no property for the confinement root. - -### Requirement: The schema audit names an unconfined path parameter of a role tool -A test SHALL read the published schema of every tool of every job role and MUST fail when a text parameter with a path-like name is in neither the confinement table nor the table of parameters that are not file paths. - -#### Scenario: An unconfined path parameter fails the audit -- **WHEN** A tool of a job role publishes a text parameter named like a path and neither table names it. -- **THEN** The test fails with the tool name and the parameter name. - -#### Scenario: A parameter that is not a file path carries a reason -- **WHEN** A tool of a job role publishes `target` and the tool takes a host or an address. -- **THEN** The table of parameters that are not file paths names the tool and the parameter with a reason, and the audit passes. - -#### Scenario: Every role tool passes the audit -- **WHEN** The test reads the published schema of the tools of the planning, implementation and review roles. -- **THEN** The test passes. diff --git a/openspec/changes/3236-read-only-tools-take-paths-that/tasks.md b/openspec/changes/3236-read-only-tools-take-paths-that/tasks.md deleted file mode 100644 index 148221ee6..000000000 --- a/openspec/changes/3236-read-only-tools-take-paths-that/tasks.md +++ /dev/null @@ -1,43 +0,0 @@ -# Tasks - -## 1. Add the confinement tables and the guard - -Reads: `src/devops_cli/core/paths.py`, `src/devops_cli/ai/mcp/generator.py`. -Changes: `src/devops_cli/ai/mcp/path_confinement.py` (new), `src/devops_cli/ai/mcp/generator.py`, `tests/ai/mcp/test_mcp.py`. - -- [ ] 1.1 In `tests/ai/mcp/test_mcp.py`, write tests that the `gh_pages_verify` tool refuses `../../etc/passwd` and the `ai_ast_graph` tool refuses `/etc` with a `ValidationError` that names the parameter and starts no command, and that `docs` resolves under the work tree. Verify `uv run devops ci test tests/ai/mcp/test_mcp.py` fails on the base -- [ ] 1.2 Write `src/devops_cli/ai/mcp/path_confinement.py` with `PATH_CONFINEMENTS` (`ai_ast_graph`: `target_dir`; `gh_pages_verify`: `root_dir`) and `NOT_A_FILE_PATH` (`security_intel_network`: `target`, with the reason). In `generator.py`, add `_check_path_confinement(tool_name, kwargs)`, which calls `safe_resolve_subpath` with `error_cls=ValidationError` against the work tree of the process and writes the resolved value back, and call it in `_create_tool_function` beside `_check_oidc_token_reference`. Verify `uv run devops ci test tests/ai/mcp/test_mcp.py` passes - -## 2. Publish no output path for two tools - -Reads: `src/devops_cli/ai/mcp/generator.py`, `tests/ai/mcp/test_fastmcp_contracts.py`. -Changes: `src/devops_cli/ai/mcp/generator.py`, `tests/ai/mcp/test_fastmcp_contracts.py`. - -- [ ] 2.1 In `tests/ai/mcp/test_fastmcp_contracts.py`, assert that the schema of `ai_ast_graph` holds no `output_path` and that of `k8s_security_stream` holds no `output`. Verify `uv run devops ci test tests/ai/mcp/test_fastmcp_contracts.py` fails on the base -- [ ] 2.2 Add `_PATH_PARAM_RESTRICTIONS` entries for `("ai", "ast", "graph")` and `("k8s", "security-stream")` that allow every option of the command but the output option. Verify `uv run devops ci test tests/ai/mcp/test_fastmcp_contracts.py` passes - -## 3. Audit the path parameters of the role tools - -Reads: `src/devops_cli/ai/mcp/roles.py`, `src/devops_cli/ai/mcp/path_confinement.py`, `src/devops_cli/core/paths.py`. -Changes: `tests/ai/mcp/test_path_confinement.py` (new). - -- [ ] 3.1 In `tests/ai/mcp/test_path_confinement.py`, take the catalogue of `mcp`, apply `tools_for_role` for each of `JOB_ROLES`, and match each text property whose name holds a keyword of `_PATH_PARAM_KEYWORDS`. Require an entry in `PATH_CONFINEMENTS` or `NOT_A_FILE_PATH`, and fail with the tool and parameter name. Add a case that a synthetic tool with `output_path` fails. Verify `uv run devops ci test tests/ai/mcp/test_path_confinement.py` passes - -## 4. Document the published options - -Reads: `docs/MCP_TOOLS.md`. -Changes: `docs/MCP_TOOLS.md`. - -- [ ] 4.1 Run `uv run devops docs generate --sync-readme`, and verify `uv run devops ci docs` passes - -## Criteria - -- No tool of a role table accepts a path outside a confined root.: group 3 -- A test reads the published schema of every role tool and fails on an unconfined path parameter.: group 3 -- Tests cover the path options of `ai_ast_graph`, `k8s_security_stream` and `gh_pages_verify`.: group 1, group 2 -- The confinement root is not a tool argument.: group 1 - -## Workflow follow-up - -- Confine or drop the path parameters of the other 60 tools of the `ops` role, and extend the audit to them: #3300. -- Pending a person: review the diff in tuicr (tuicr -r ..HEAD). diff --git a/openspec/changes/3236-read-only-tools-take-paths-that/.openspec.yaml b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/.openspec.yaml similarity index 50% rename from openspec/changes/3236-read-only-tools-take-paths-that/.openspec.yaml rename to openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/.openspec.yaml index a7587c5f0..4ccb2b41a 100644 --- a/openspec/changes/3236-read-only-tools-take-paths-that/.openspec.yaml +++ b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/.openspec.yaml @@ -1,2 +1,2 @@ schema: spec-driven -created: 2026-11-02 +created: 2026-11-04 diff --git a/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/design.md b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/design.md new file mode 100644 index 000000000..113dc95d4 --- /dev/null +++ b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/design.md @@ -0,0 +1,20 @@ +# Design + +## Decisions + +- **Fold the unbuilt plan of #3236 into this change.** Its tables and guard are the prerequisite of the full audit, and its active folder would otherwise race the archive of this change for one capability. The folder leaves the tree in this pull request. +- **Keep `_PATH_PARAM_RESTRICTIONS` as the only place that decides which options a tool publishes.** SECURITY.md rule 11 names it as the schema gate, and `generate_mcp_tools` builds every published schema from it. +- **Put the path data in `src/devops_cli/ai/mcp/path_confinement.py`: `PATH_CONFINEMENTS` maps a tool name to the parameters that the guard confines, and `NOT_A_FILE_PATH` maps a tool name to a parameter with its reason.** The audit needs one place to read, and a parameter that is a host or a model name has a recorded reason instead of a silent exemption. +- **Confine every kept file path under the work tree of the server process, never under a path that a model gives.** `safe_resolve_subpath` already refuses `..`, an absolute path outside the base and a symlink escape, so the guard reuses it instead of a new resolver. A caller that omits the option gets the command default, because the guard reads only the arguments that the caller supplies. +- **Remove `output_path` from `ai_ast_graph` and `output` from `k8s_security_stream`.** Both commands print their result when no file is given, so the model needs no output option. Keep `target_dir` of `ai_ast_graph` and `root_dir` of `gh_pages_verify`, confined. +- **Record a parameter that is not a file path in `NOT_A_FILE_PATH` with a reason of at least 30 characters.** `target_provider` and `target_model` name a provider and a model, `max_files` is a count, `file_filter` is a glob, `filed_by` is an enum, and `destroy` is a flag that a keyword matches by substring. `vault_get.path` and `vault_sync.path` name a secret in Vault, not a file. +- **Audit `tools_for_role(ROLE_OPS, catalogue)`, which is every published tool.** The issue covers the whole exposure table, and the hand-written tools share the catalogue, so one audit leaves no tool outside it. The audit fails on an unconfined parameter, a short reason and a stale entry, and names the tool and the parameter. +- **Put the audit in the new module and call it from the test.** The test stays a thin assertion, and a later caller can run the same audit without importing the test. + +## Rejected alternatives + +- **Leave the path options as they are and rely on the read-only annotation.** `generate_mcp_tools` sets `annotations={"readOnlyHint": read_only}` and nothing else checks the argument, so a model value reaches the command unchanged. +- **Drop every path option, including `target_dir` and `root_dir`.** `safe_resolve_subpath` already confines a path under a base, so a kept option is confined without losing the use of the command. +- **Confine under the home directory or a data directory for the tools that default outside the work tree.** A model could then read every file of the home directory through those tools. The guard checks only supplied arguments, so the command default stays and a model-chosen path stays in the work tree. +- **Audit the exposure table literal only.** `CONST_MCP_EXPOSURE_TABLE` omits the hand-written tools, so `scan_uv_audit.directory` would escape the audit. The catalogue through `tools_for_role(ROLE_OPS, ...)` holds both. +- **A bare exempt list.** A bare entry hides why the parameter is safe and cannot fail the test. A reason of at least 30 characters and the stale check keep the list honest. diff --git a/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/proposal.md b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/proposal.md new file mode 100644 index 000000000..9432f8552 --- /dev/null +++ b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/proposal.md @@ -0,0 +1,48 @@ +# Proposal + +## Why + +#3300 reports that the `ops` role publishes 60 tools whose path-like text parameters a model sets, and that SECURITY.md rule 11 requires removal or confinement for every path that a command reads or writes. #3236 planned the confinement tables and audited the job roles, but that plan is not built: `src/devops_cli/ai/mcp/path_confinement.py` does not exist on `main`, and the generated tools pass a model value to the command unchanged. This change delivers the whole class in one pull request: it adds the tables, confines each kept path parameter to the work tree of the server process, removes the two output options that the server can choose, and audits every tool that the server publishes, the `ops` role included. + +The plan of #3236 is folded into this change because its tables and its guard are the prerequisite of the full audit. The active folder `openspec/changes/3236-read-only-tools-take-paths-that/` leaves the tree in the same pull request. + +## What Changes + +- A new module `src/devops_cli/ai/mcp/path_confinement.py` holds `PATH_CONFINEMENTS`, the tool name and the path parameters that the guard confines, and `NOT_A_FILE_PATH`, the tool name and the parameter whose name looks like a path but that is not a file path, with a reason. It also holds `audit_path_parameters(tools)`, which returns one problem for each unconfined path-like parameter, each short reason and each entry that matches no published parameter. +- `_PATH_PARAM_RESTRICTIONS` in `src/devops_cli/ai/mcp/generator.py` gains an entry for `("ai", "ast", "graph")` and one for `("k8s", "security-stream")`. The `output_path` option of the first and the `output` option of the second leave the published schema. The commands print their result, so the model needs no file. +- `_create_tool_function` calls a new `_check_path_confinement` before the command starts. It resolves each kept path argument through `safe_resolve_subpath` against the work tree of the server process, writes the resolved value back, and refuses an escape with a `ValidationError` that names the parameter. The root comes from the process and never from an argument. +- The native `scan_uv_audit` tool confines its `directory` parameter with the same helper. +- A new test `tests/ai/mcp/test_path_confinement.py` audits the published schema of every tool of the `ops` role, which is the whole catalogue, and fails with the tool and parameter name when a path-like parameter has no entry. It also tests that a synthetic tool with an unconfined `output_path` fails, that a short or stale reason fails, that an escape is refused, and that a relative path resolves under the work tree. +- `docs/MCP_TOOLS.md` and the generated command documents are regenerated. +- The change carries a spec delta for the `mcp-tool-path-confinement` capability, because the published schema and the tool behavior are observable to a consumer. +- The change fits one pull request. + +## Capabilities + +### New Capabilities + +- `mcp-tool-path-confinement`: every published path-like parameter of every tool is removed, confined under a server root, or recorded as not a file path with a reason. + +### Modified Capabilities + +None. + +## Impact + +- Code: `src/devops_cli/ai/mcp/path_confinement.py` (new), `src/devops_cli/ai/mcp/generator.py`, `src/devops_cli/ai/mcp/server.py`, `src/devops_cli/lang/en/errors.py` +- Documents: `docs/MCP_TOOLS.md`, `docs/ERRORS.md`, `docs/CLI_REFERENCE.md` +- Tests: `tests/ai/mcp/test_path_confinement.py` (new), `tests/ai/mcp/test_fastmcp_contracts.py` +- Plans: `openspec/changes/3300-the-ops-role-publishes-60-tools-with/` (new), `openspec/changes/3236-read-only-tools-take-paths-that/` (removed, folded into this change) +- It deletes no internal code beyond the folded plan folder. + +## Feasibility + +- `git cat-file -e origin/main:src/devops_cli/ai/mcp/path_confinement.py` fails, and `git log --all -- src/devops_cli/ai/mcp/path_confinement.py` prints no commit. #3236 is still an active change with no code, so its tables do not exist. +- A script over `CONST_MCP_EXPOSURE_TABLE` applies `_introspect_command_parameters` and matches `_PATH_PARAM_KEYWORDS` from `src/devops_cli/core/paths.py`. It lists 59 tools, among them `ai_ingest_library.output_dir`, `scan_sbom.output_file`, `scan_aibom.output_file`, `tf_apply.plan_file`, `k8s_create_tls_secret.cert_path`, `vault_get.path` and `vault_sync.path`. The count differs from the issue by one tool because `k8s_security_stream.output` does not hold a keyword; this change removes that option as #3236 planned. +- The same script prints the Click default of each parameter: `k8s_create_tls_secret` and `k8s_enable_tls` default to absolute paths under `~/.config/devops-cli`, `ssh audit` and `ssh status` default to `None`, and the scan and Terraform tools default to `.`. The guard reads only the arguments that a caller supplies, so a server default never trips it. +- `safe_resolve_subpath` in `src/devops_cli/core/paths.py` resolves a relative path against a base, refuses `..`, an absolute path outside the base and a symlink escape, and takes `error_cls`. The helper the guard needs exists. +- `tools_for_role(ROLE_OPS, catalogue)` returns every tool but `hydrate_tool_domain` in `src/devops_cli/ai/mcp/roles.py`, so one audit over that role covers the exposure table and the hand-written tools. + +## Existing Tool Considered + +No open, maintained library confines the path parameters of an MCP server's tools: FastMCP publishes a schema and runs the function, and pydantic validates types, not containment. The change reuses `pathlib`, `safe_resolve_subpath` and the work-tree helper `find_worktree_root` in `src/devops_cli/core/repo.py`. It builds no path library and adds no dependency. The written cost is about 20,000 tokens to build the tables, the guard and the audit, about 5,000 tokens per year to keep and fix them, and two days elapsed. diff --git a/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/specs/mcp-tool-path-confinement/spec.md b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/specs/mcp-tool-path-confinement/spec.md new file mode 100644 index 000000000..f9f8df5c0 --- /dev/null +++ b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/specs/mcp-tool-path-confinement/spec.md @@ -0,0 +1,84 @@ +## Purpose +Which path options the MCP tools publish, and how each kept path value is confined to a root that the server sets, so a model that calls a tool reaches no path outside the server work tree. The audit covers every published tool, the `ops` role included. + +## ADDED Requirements + +### Requirement: A tool publishes no path option that the server can choose +A tool MUST publish no path option when the server can choose the path that the command writes or reads. + +#### Scenario: The graph tool prints its graph +- **WHEN** A caller reads the published schema of the `ai_ast_graph` tool. +- **THEN** The schema holds no `output_path` property, and the command prints its graph. + +#### Scenario: The stream tool writes no file +- **WHEN** A caller reads the published schema of the `k8s_security_stream` tool. +- **THEN** The schema holds no `output` property, and the tool prints the stream. + +### Requirement: A kept path option resolves under a server root +Every path parameter that a tool publishes MUST resolve through `safe_resolve_subpath` against a root that the server sets before the server opens or writes the path. + +#### Scenario: A traversal value is refused +- **WHEN** A caller gives `../../etc/passwd` to the `root_dir` parameter of `gh_pages_verify`. +- **THEN** The tool returns a validation error that names the parameter, and no command starts. + +#### Scenario: A directory of the graph tool is confined +- **WHEN** A caller gives `/etc` to the `target_dir` parameter of `ai_ast_graph`. +- **THEN** The tool returns a validation error that names the parameter, and no command starts. + +#### Scenario: A relative value stays in the work tree +- **WHEN** A caller gives `docs` to the `root_dir` parameter of `gh_pages_verify`. +- **THEN** The tool resolves the value under the work tree of the server and runs the command with the resolved path. + +#### Scenario: The root is not a tool argument +- **WHEN** A caller reads the published schema of a tool that keeps a path parameter. +- **THEN** The schema holds no property for the confinement root. + +### Requirement: Write path confinement +Every MCP tool that writes a file SHALL remove each path parameter from its schema or SHALL confine it with `PATH_CONFINEMENTS`. + +#### Scenario: Unconfined write path +- **WHEN** A tool publishes a write-path parameter that matches `_PATH_PARAM_KEYWORDS`. +- **THEN** The audit fails unless the parameter has a `PATH_CONFINEMENTS` entry. + +#### Scenario: Listed write tool schema +- **WHEN** The test reads the published schema of each listed write tool. +- **THEN** Each path-like write parameter is absent or has a `PATH_CONFINEMENTS` entry. + +### Requirement: Read path confinement +Every MCP tool that reads a file SHALL remove each path parameter from its schema or SHALL confine it with `PATH_CONFINEMENTS`. + +#### Scenario: Unconfined read path +- **WHEN** A tool publishes a read-path parameter that matches `_PATH_PARAM_KEYWORDS`. +- **THEN** The audit fails unless the parameter has a `PATH_CONFINEMENTS` entry. + +### Requirement: Full exposure audit +The path-parameter audit SHALL inspect every tool that the `ops` role publishes, including a tool outside the job roles. + +#### Scenario: Tool outside the job roles +- **WHEN** A tool that the `ops` role publishes is not in a job role and publishes a path-like parameter. +- **THEN** The audit checks that parameter and fails when no entry exists. + +#### Scenario: Synthetic unconfined tool +- **WHEN** The audit reads a synthetic tool with an `output_path` property. +- **THEN** The audit returns one problem that names the tool and the parameter. + +### Requirement: Reasoned non-file exemption +Each `NOT_A_FILE_PATH` entry SHALL hold a reason of at least 30 characters, and the audit MUST fail on a missing or short reason. + +#### Scenario: Bare or short exemption +- **WHEN** A path-like parameter has a `NOT_A_FILE_PATH` entry with no reason or a reason shorter than 30 characters. +- **THEN** The audit fails and names the tool and parameter. + +### Requirement: Stale exemption +The audit MUST fail when a `PATH_CONFINEMENTS` or `NOT_A_FILE_PATH` entry names no published path-like parameter. + +#### Scenario: Stale non-file entry +- **WHEN** An entry matches no published parameter. +- **THEN** The audit fails and names the stale entry. + +### Requirement: Vault secret paths +The `vault_get.path` and `vault_sync.path` parameters SHALL be recorded in `NOT_A_FILE_PATH` as Vault secret paths, not filesystem paths. + +#### Scenario: Vault path exemption +- **WHEN** The audit reads `vault_get.path` or `vault_sync.path`. +- **THEN** The audit accepts the parameter only through its `NOT_A_FILE_PATH` reason and not through a filesystem confinement. diff --git a/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/tasks.md b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/tasks.md new file mode 100644 index 000000000..accefd5a8 --- /dev/null +++ b/openspec/changes/archive/2026-11-04-3300-the-ops-role-publishes-60-tools-with/tasks.md @@ -0,0 +1,39 @@ +# Tasks + +## 1. Confine every kept path parameter + +Reads: `src/devops_cli/core/paths.py`, `src/devops_cli/ai/mcp/generator.py`, `src/devops_cli/ai/mcp/server.py`. +Changes: `tests/ai/mcp/test_path_confinement.py` (new), `src/devops_cli/ai/mcp/path_confinement.py` (new), `src/devops_cli/ai/mcp/generator.py`, `src/devops_cli/ai/mcp/server.py`, `src/devops_cli/lang/en/errors.py`. + +- [x] 1.1 In `tests/ai/mcp/test_path_confinement.py`, assert that `ai_ast_parse` refuses `/etc/passwd` with a `ValidationError` that names `file_path` and starts no command, and that `docs` resolves under the work tree. Verify `uv run devops ci test tests/ai/mcp/test_path_confinement.py` fails on the base + - Verification: `uv run devops ci test tests/ai/mcp/test_path_confinement.py` +- [x] 1.2 Write `src/devops_cli/ai/mcp/path_confinement.py` with `PATH_CONFINEMENTS` and `NOT_A_FILE_PATH`. Add `_check_path_confinement` in `generator.py`, call it in `_create_tool_function` before the command starts, and confine `scan_uv_audit.directory` in `server.py`. Add the refusal message to `src/devops_cli/lang/en/errors.py`. Verify `uv run devops ci test tests/ai/mcp/test_path_confinement.py` passes + - Verification: `uv run devops ci test tests/ai/mcp/test_path_confinement.py` + +## 2. Remove the output options and audit the whole catalogue + +Reads: `src/devops_cli/ai/mcp/path_confinement.py`, `src/devops_cli/ai/mcp/roles.py`. +Changes: `tests/ai/mcp/test_fastmcp_contracts.py`, `tests/ai/mcp/test_path_confinement.py`, `src/devops_cli/ai/mcp/generator.py`, `src/devops_cli/ai/mcp/path_confinement.py`. + +- [x] 2.1 In `tests/ai/mcp/test_fastmcp_contracts.py`, assert that the schema of `ai_ast_graph` holds no `output_path` and that of `k8s_security_stream` holds no `output`. In `tests/ai/mcp/test_path_confinement.py`, add the audit cases: every path-like parameter of the `ops` role has an entry, a synthetic `output_path` fails, a short reason fails and a stale entry fails. Verify `uv run devops ci test tests/ai/mcp/test_fastmcp_contracts.py tests/ai/mcp/test_path_confinement.py` fails on the base + - Verification: `uv run devops ci test tests/ai/mcp/test_fastmcp_contracts.py tests/ai/mcp/test_path_confinement.py` +- [x] 2.2 Add the two `_PATH_PARAM_RESTRICTIONS` entries and `audit_path_parameters` in `path_confinement.py`. Verify the same command passes + - Verification: `uv run devops ci test tests/ai/mcp/test_fastmcp_contracts.py tests/ai/mcp/test_path_confinement.py` + +## 3. Regenerate the documents + +Reads: `docs/MCP_TOOLS.md`, `docs/ERRORS.md`. +Changes: `docs/MCP_TOOLS.md`, `docs/ERRORS.md`, `docs/CLI_REFERENCE.md`, `docs/commands/*.md`. + +- [x] 3.1 Run `uv run devops docs generate --sync-readme`, and verify `uv run devops ci docs` passes + - Verification: `uv run devops ci docs` + +## Criteria + +- Every published path-like parameter is removed, confined or recorded as not a file path with a reason.: group 1, group 2 +- The audit covers every tool of the `ops` role, and fails on a short reason and a stale entry.: group 2 +- The confinement root is the work tree of the server process and is not a tool argument.: group 1 + +## Workflow follow-up + +- Pending a person: review the diff in tuicr (tuicr -r ..HEAD). diff --git a/openspec/specs/mcp-tool-path-confinement/spec.md b/openspec/specs/mcp-tool-path-confinement/spec.md new file mode 100644 index 000000000..47fa2abb4 --- /dev/null +++ b/openspec/specs/mcp-tool-path-confinement/spec.md @@ -0,0 +1,86 @@ +# mcp-tool-path-confinement Specification + +## Purpose +Which path options the MCP tools publish, and how each kept path value is confined to a root that the server sets, so a model that calls a tool reaches no path outside the server work tree. The audit covers every published tool, the `ops` role included. + +## Requirements + +### Requirement: A tool publishes no path option that the server can choose +A tool MUST publish no path option when the server can choose the path that the command writes or reads. + +#### Scenario: The graph tool prints its graph +- **WHEN** A caller reads the published schema of the `ai_ast_graph` tool. +- **THEN** The schema holds no `output_path` property, and the command prints its graph. + +#### Scenario: The stream tool writes no file +- **WHEN** A caller reads the published schema of the `k8s_security_stream` tool. +- **THEN** The schema holds no `output` property, and the tool prints the stream. + +### Requirement: A kept path option resolves under a server root +Every path parameter that a tool publishes MUST resolve through `safe_resolve_subpath` against a root that the server sets before the server opens or writes the path. + +#### Scenario: A traversal value is refused +- **WHEN** A caller gives `../../etc/passwd` to the `root_dir` parameter of `gh_pages_verify`. +- **THEN** The tool returns a validation error that names the parameter, and no command starts. + +#### Scenario: A directory of the graph tool is confined +- **WHEN** A caller gives `/etc` to the `target_dir` parameter of `ai_ast_graph`. +- **THEN** The tool returns a validation error that names the parameter, and no command starts. + +#### Scenario: A relative value stays in the work tree +- **WHEN** A caller gives `docs` to the `root_dir` parameter of `gh_pages_verify`. +- **THEN** The tool resolves the value under the work tree of the server and runs the command with the resolved path. + +#### Scenario: The root is not a tool argument +- **WHEN** A caller reads the published schema of a tool that keeps a path parameter. +- **THEN** The schema holds no property for the confinement root. + +### Requirement: Write path confinement +Every MCP tool that writes a file SHALL remove each path parameter from its schema or SHALL confine it with `PATH_CONFINEMENTS`. + +#### Scenario: Unconfined write path +- **WHEN** A tool publishes a write-path parameter that matches `_PATH_PARAM_KEYWORDS`. +- **THEN** The audit fails unless the parameter has a `PATH_CONFINEMENTS` entry. + +#### Scenario: Listed write tool schema +- **WHEN** The test reads the published schema of each listed write tool. +- **THEN** Each path-like write parameter is absent or has a `PATH_CONFINEMENTS` entry. + +### Requirement: Read path confinement +Every MCP tool that reads a file SHALL remove each path parameter from its schema or SHALL confine it with `PATH_CONFINEMENTS`. + +#### Scenario: Unconfined read path +- **WHEN** A tool publishes a read-path parameter that matches `_PATH_PARAM_KEYWORDS`. +- **THEN** The audit fails unless the parameter has a `PATH_CONFINEMENTS` entry. + +### Requirement: Full exposure audit +The path-parameter audit SHALL inspect every tool that the `ops` role publishes, including a tool outside the job roles. + +#### Scenario: Tool outside the job roles +- **WHEN** A tool that the `ops` role publishes is not in a job role and publishes a path-like parameter. +- **THEN** The audit checks that parameter and fails when no entry exists. + +#### Scenario: Synthetic unconfined tool +- **WHEN** The audit reads a synthetic tool with an `output_path` property. +- **THEN** The audit returns one problem that names the tool and the parameter. + +### Requirement: Reasoned non-file exemption +Each `NOT_A_FILE_PATH` entry SHALL hold a reason of at least 30 characters, and the audit MUST fail on a missing or short reason. + +#### Scenario: Bare or short exemption +- **WHEN** A path-like parameter has a `NOT_A_FILE_PATH` entry with no reason or a reason shorter than 30 characters. +- **THEN** The audit fails and names the tool and parameter. + +### Requirement: Stale exemption +The audit MUST fail when a `PATH_CONFINEMENTS` or `NOT_A_FILE_PATH` entry names no published path-like parameter. + +#### Scenario: Stale non-file entry +- **WHEN** An entry matches no published parameter. +- **THEN** The audit fails and names the stale entry. + +### Requirement: Vault secret paths +The `vault_get.path` and `vault_sync.path` parameters SHALL be recorded in `NOT_A_FILE_PATH` as Vault secret paths, not filesystem paths. + +#### Scenario: Vault path exemption +- **WHEN** The audit reads `vault_get.path` or `vault_sync.path`. +- **THEN** The audit accepts the parameter only through its `NOT_A_FILE_PATH` reason and not through a filesystem confinement. diff --git a/src/devops_cli/ai/mcp/generator.py b/src/devops_cli/ai/mcp/generator.py index 0d00ba971..eea2985d2 100644 --- a/src/devops_cli/ai/mcp/generator.py +++ b/src/devops_cli/ai/mcp/generator.py @@ -12,6 +12,7 @@ from fastmcp import FastMCP from pydantic import Field +from devops_cli.ai.mcp.path_confinement import PATH_CONFINEMENTS, confine_value, confinement_root from devops_cli.config.constants import CONST_MCP_EXPOSURE_TABLE from devops_cli.core.command_resolver import module_click_command from devops_cli.docs.generator import _clean_text @@ -96,6 +97,29 @@ "allowed": frozenset({"path", "key", "dry_run"}), "required": frozenset(), }, + # No `output_path`: the command prints its graph, so the model needs no file (#3300). + ("ai", "ast", "graph"): { + "allowed": frozenset({"target_dir", "max_files", "fmt"}), + "required": frozenset(), + }, + # No `output`: the tool prints the alerts, so the model needs no file (#3300). + ("k8s", "security-stream"): { + "allowed": frozenset( + { + "namespace", + "label", + "severity", + "duration", + "tail", + "follow", + "simulate", + "dry_run", + "output_format", + "json_output", + } + ), + "required": frozenset(), + }, } @@ -307,6 +331,25 @@ def _check_model_endpoints(path: tuple[str, ...], kwargs: dict[str, Any]) -> Non raise ValidationError(ERRORS.mcp.model_names_endpoint, field="models") +def _check_path_confinement(tool_name: str, kwargs: dict[str, Any]) -> None: + """Resolve each kept path argument under the work tree before the command starts. + + The root comes from the server process, never from an argument, so a model that calls the + tool cannot move the boundary that confines it (#3300). + """ + names = PATH_CONFINEMENTS.get(tool_name) + if not names: + return + root = confinement_root() + for name in sorted(names): + if name not in kwargs: + continue + value = kwargs[name] + if value is None or value == "": + continue + kwargs[name] = confine_value(root, name, value) + + def _check_oidc_token_reference(path: tuple[str, ...], kwargs: dict[str, Any]) -> None: """Refuse an OIDC token of the sign tool that is not a `keyring:` reference. @@ -401,6 +444,7 @@ def tool_fn(*args: Any, **kwargs: Any) -> str: from devops_cli.ai.mcp.server import _run_mcp_cmd bound = sig.bind(*args, **kwargs) + _check_path_confinement(tool_name, bound.arguments) bound.apply_defaults() _check_sandbox_preconditions(path, bound.arguments) _check_model_endpoints(path, bound.arguments) diff --git a/src/devops_cli/ai/mcp/path_confinement.py b/src/devops_cli/ai/mcp/path_confinement.py new file mode 100644 index 000000000..2b5c07cab --- /dev/null +++ b/src/devops_cli/ai/mcp/path_confinement.py @@ -0,0 +1,177 @@ +"""The path parameters of the MCP tools: which ones the guard confines, and which are no file. + +`PATH_CONFINEMENTS` names the path parameters that `_check_path_confinement` resolves under the +work tree of the server process, so a model that calls a tool reaches no path outside it. +`NOT_A_FILE_PATH` names a parameter whose name holds a path keyword and whose value is no +filesystem path, with the reason. `audit_path_parameters` fails on an unconfined parameter, a +reason shorter than `MIN_NOT_A_FILE_REASON` and an entry that matches no published parameter. +""" + +from __future__ import annotations + +from collections.abc import Iterable +from pathlib import Path +from typing import Any, Final + +from devops_cli.core.paths import _PATH_PARAM_KEYWORDS + +MIN_NOT_A_FILE_REASON: Final[int] = 30 + +PATH_CONFINEMENTS: Final[dict[str, frozenset[str]]] = { + "ai_ast_graph": frozenset({"target_dir"}), + "ai_ast_parse": frozenset({"file_path"}), + "ai_diagram": frozenset({"target_dir"}), + "ai_ingest_library": frozenset({"output_dir"}), + "ai_ingest_query_library": frozenset({"contracts_dir"}), + "ai_pack_context": frozenset({"target_path"}), + "ai_rag_drift": frozenset({"path"}), + "ai_rag_index": frozenset({"path"}), + "ai_read": frozenset({"target_path"}), + "ai_repomap": frozenset({"target_dir"}), + "ai_test_gen": frozenset({"target_file"}), + "branches_list": frozenset({"base_dir"}), + "docker_sandbox": frozenset({"seccomp_profile"}), + "gh_issues_close": frozenset({"comment_file"}), + "gh_labels_sync": frozenset({"file"}), + "gh_pages_verify": frozenset({"root_dir"}), + "gh_project_audit": frozenset({"template_file"}), + "gh_project_status": frozenset({"template_file"}), + "gh_project_sync": frozenset({"template_file"}), + "gh_views_audit": frozenset({"template_file"}), + "gh_views_spec": frozenset({"template_file"}), + "gh_views_sync": frozenset({"template_file"}), + "k8s_bootstrap": frozenset({"k8s_dir"}), + "k8s_create_tls_secret": frozenset({"cert_path", "key_path"}), + "k8s_deploy_stack": frozenset({"k8s_dir"}), + "k8s_diff_helm": frozenset({"chart_path"}), + "k8s_enable_tls": frozenset({"tls_dir"}), + "k8s_lint": frozenset({"target"}), + "k8s_teardown_stack": frozenset({"k8s_dir"}), + "k8s_validate": frozenset({"manifest_path"}), + "repos_list": frozenset({"base_dir"}), + "repos_sync": frozenset({"base_dir"}), + "review_branch": frozenset({"repo_path"}), + "review_export_feedback": frozenset({"reviews_dir"}), + "review_path": frozenset({"targets"}), + "review_stats": frozenset({"reviews_dir"}), + "roadmap_intake": frozenset({"body_file"}), + "sandbox_deploy": frozenset({"seccomp_profile"}), + "sandbox_exec": frozenset({"workdir"}), + "scan_aibom": frozenset({"output_file", "target"}), + "scan_complexity": frozenset({"target"}), + "scan_fix": frozenset({"target"}), + "scan_injection": frozenset({"target"}), + "scan_sast": frozenset({"target"}), + "scan_sbom": frozenset({"output_file", "target"}), + "scan_secrets": frozenset({"target"}), + "scan_trivy": frozenset({"target"}), + "scan_uv_audit": frozenset({"directory"}), + "ssh_audit": frozenset({"key_dir"}), + "ssh_status": frozenset({"key_dir"}), + "tf_apply": frozenset({"directory", "plan_file", "var_file"}), + "tf_cost_breakdown": frozenset({"directory"}), + "tf_notify_plan": frozenset({"plan_file"}), + "tf_output": frozenset({"directory"}), + "tf_plan": frozenset({"directory", "var_file"}), + "tls_inspect": frozenset({"cert_path"}), +} + +NOT_A_FILE_PATH: Final[dict[str, dict[str, str]]] = { + "ai_ast_graph": { + "max_files": "The value is a count of files, not a path that the command opens.", + }, + "ai_failover": { + "target_provider": "The value names an AI provider, not a path that the command opens.", + "target_model": "The value names an AI model, not a path that the command opens.", + }, + "ai_rag_query": { + "file_filter": "The value is a glob that filters indexed paths, not a file opened on disk.", + }, + "ai_repomap": { + "max_files": "The value is a count of files, not a path that the command opens.", + }, + "security_intel_network": { + "target": "The value is a host name or an address, not a path that the tool opens.", + }, + "roadmap_intake": { + "filed_by": "The value names who files the issue, an enum, not a filesystem path.", + }, + "tf_plan": { + "destroy": "The value is a boolean flag; the keyword 'dest' matches its name by substring.", + }, + "vault_get": { + "path": "The value selects a Vault secret by its secret path; no filesystem file is opened.", + }, + "vault_sync": { + "path": "The value selects a Vault secret by its secret path; no filesystem file is opened.", + }, +} + + +def confinement_root() -> Path: + """The work tree of the server process, never a value that a tool argument carries.""" + from devops_cli.core.repo import find_worktree_root + + return find_worktree_root(Path.cwd()) + + +def confine_value(root: Path, name: str, value: Any) -> Any: + """Resolve one path argument under `root`, and refuse an escape with a named error.""" + if isinstance(value, (list, tuple)): + return [confine_value(root, name, item) for item in value] + if not isinstance(value, (str, Path)): + return value + from devops_cli.core.paths import safe_resolve_subpath + from devops_cli.exceptions import ValidationError + from devops_cli.lang import ERRORS + + try: + resolved = safe_resolve_subpath(root, str(value), error_cls=ValidationError) + except ValidationError as exc: + raise ValidationError( + ERRORS.mcp.path_outside_work_tree.format(name=name, root=root), field=name + ) from exc + return str(resolved) + + +def _path_like_names(tool: Any) -> set[str]: + properties = getattr(tool, "parameters", {}).get("properties", {}) + return { + name + for name in properties + if any(keyword in name.lower() for keyword in _PATH_PARAM_KEYWORDS) + } + + +def audit_path_parameters(tools: Iterable[Any]) -> list[str]: + """Return one problem for each path parameter that no entry confines or explains.""" + published: dict[str, set[str]] = { + tool.name: _path_like_names(tool) for tool in tools if _path_like_names(tool) + } + problems: list[str] = [] + for tool_name, names in published.items(): + confined = PATH_CONFINEMENTS.get(tool_name, frozenset()) + reasons = NOT_A_FILE_PATH.get(tool_name, {}) + for name in sorted(names): + if name in confined: + continue + reason = reasons.get(name) + if reason is None: + problems.append( + f"{tool_name}.{name} is a path-like parameter with no confinement or reason." + ) + elif len(reason.strip()) < MIN_NOT_A_FILE_REASON: + problems.append( + f"{tool_name}.{name} has a reason shorter than {MIN_NOT_A_FILE_REASON} characters." + ) + for tool_name, confined_names in PATH_CONFINEMENTS.items(): + stale = sorted(confined_names - published.get(tool_name, set())) + problems.extend( + f"{tool_name}.{name} is confined but is not a published path." for name in stale + ) + for tool_name, reasons in NOT_A_FILE_PATH.items(): + stale = sorted(reasons.keys() - published.get(tool_name, set())) + problems.extend( + f"{tool_name}.{name} is exempt but is not a published path." for name in stale + ) + return problems diff --git a/src/devops_cli/ai/mcp/server.py b/src/devops_cli/ai/mcp/server.py index fcf268b4d..c6abc4f36 100644 --- a/src/devops_cli/ai/mcp/server.py +++ b/src/devops_cli/ai/mcp/server.py @@ -22,6 +22,7 @@ schema_violations, ) from devops_cli.ai.mcp.generator import generate_mcp_tools +from devops_cli.ai.mcp.path_confinement import confine_value, confinement_root from devops_cli.ai.task_loader import load_task_prompt from devops_cli.config.constants import ( CONST_FASTMCP_OPERATIONS_LOGGER, @@ -144,7 +145,8 @@ def scan_uv_audit(directory: str = ".") -> str: _validate_mcp_arg("directory", directory) from devops_cli.ai.tools.builtin_tools import scan_uv_audit as _native_uv_audit - return _native_uv_audit(directory=directory) + confined = confine_value(confinement_root(), "directory", directory) + return _native_uv_audit(directory=str(confined)) # ── FastMCP Dynamic System State Resources ───────────────────────────────────── diff --git a/src/devops_cli/lang/en/errors.py b/src/devops_cli/lang/en/errors.py index 1c4fd62cc..79159faf5 100644 --- a/src/devops_cli/lang/en/errors.py +++ b/src/devops_cli/lang/en/errors.py @@ -258,6 +258,10 @@ class MCPErrorMessages: "Invalid value for 'oidc_token': use a reference of the form keyring:. " "A token in an argument passes through the model, so the tool never takes the token itself." ) + path_outside_work_tree: str = ( + "Invalid value for '{name}': the path must resolve under the work tree at '{root}'. " + "A model that calls a tool never chooses a path outside it." + ) conflicting_symbol_and_pattern: str = ( "Cannot specify both 'symbol' and 'pattern'; provide one or the other." ) diff --git a/tests/ai/mcp/test_fastmcp_contracts.py b/tests/ai/mcp/test_fastmcp_contracts.py index d07fc9c88..86cc660c9 100644 --- a/tests/ai/mcp/test_fastmcp_contracts.py +++ b/tests/ai/mcp/test_fastmcp_contracts.py @@ -2,6 +2,8 @@ from __future__ import annotations +from pathlib import Path + import pytest from devops_cli.ai.mcp.server import list_mcp_tools @@ -450,7 +452,7 @@ def test_fastmcp_ast_tools() -> None: "ai", "ast", "parse", - "src/main.py", + str(Path("src/main.py").resolve()), "--query", "(function_definition) @fn", "--format", @@ -470,7 +472,7 @@ def test_fastmcp_ast_tools() -> None: "ast", "graph", "--dir", - "src", + str(Path("src").resolve()), "--max-files", "20", "--format", @@ -505,7 +507,7 @@ def test_fastmcp_context_packing_tool() -> None: "devops", "ai", "pack-context", - "src/main.py", + str(Path("src/main.py").resolve()), "--referenced", "main,app", "--max-tokens", @@ -545,7 +547,7 @@ def test_fastmcp_ai_read_tool() -> None: "devops", "ai", "read", - "src/main.py", + str(Path("src/main.py").resolve()), "--inspect", "--level", "0", @@ -580,7 +582,7 @@ def test_fastmcp_ai_rag_drift_tool() -> None: "ai", "rag", "drift", - "src", + str(Path("src").resolve()), "--auto-sync", "--format", "json", @@ -1680,3 +1682,18 @@ def test_fastmcp_k8s_security_stream_validation() -> None: with pytest.raises(ToolError, match="Invalid severity"): k8s_security_stream(severity="NonExistent") + + +def test_the_graph_and_stream_tools_publish_no_output_option() -> None: + """The server chooses the print path, so neither tool publishes an output option.""" + import asyncio + + from devops_cli.ai.mcp.server import mcp + + async def schemas() -> tuple[dict[str, object], dict[str, object]]: + graph = await mcp.get_tool("ai_ast_graph") + stream = await mcp.get_tool("k8s_security_stream") + return graph.parameters["properties"], stream.parameters["properties"] + + graph_props, stream_props = asyncio.run(schemas()) + assert ("output_path" in graph_props, "output" in stream_props) == (False, False) diff --git a/tests/ai/mcp/test_path_confinement.py b/tests/ai/mcp/test_path_confinement.py new file mode 100644 index 000000000..10db71524 --- /dev/null +++ b/tests/ai/mcp/test_path_confinement.py @@ -0,0 +1,130 @@ +"""Every published path-like parameter is removed, confined, or recorded with a reason (#3300).""" + +from __future__ import annotations + +import asyncio +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +import devops_cli.ai.mcp.path_confinement as path_confinement +from devops_cli.ai.mcp.generator import _create_tool_function, resolve_click_command +from devops_cli.ai.mcp.path_confinement import audit_path_parameters +from devops_cli.ai.mcp.roles import ROLE_OPS, tools_for_role +from devops_cli.ai.mcp.server import mcp +from devops_cli.config.constants import CONST_MCP_EXPOSURE_TABLE +from devops_cli.core.paths import _PATH_PARAM_KEYWORDS +from devops_cli.core.repo import find_worktree_root +from devops_cli.exceptions import ValidationError + + +def _catalogue() -> list[Any]: + return asyncio.run(mcp._list_tools()) + + +def _tool_fn(path: tuple[str, ...]) -> Any: + _, timeout, _read_only, format_json = next( + entry for entry in CONST_MCP_EXPOSURE_TABLE if entry[0] == path + ) + tool_name = "_".join(path).replace("-", "_") + return _create_tool_function(path, timeout, format_json, tool_name, resolve_click_command(path)) + + +def _path_like_parameters() -> set[str]: + names: set[str] = set() + for tool in tools_for_role(ROLE_OPS, _catalogue()): + for name in tool.parameters.get("properties", {}): + if any(keyword in name.lower() for keyword in _PATH_PARAM_KEYWORDS): + names.add(f"{tool.name}.{name}") + return names + + +def test_every_path_like_parameter_is_confined_or_explained() -> None: + problems = audit_path_parameters(tools_for_role(ROLE_OPS, _catalogue())) + assert problems == [] + + +def test_the_audit_covers_a_tool_outside_the_job_roles() -> None: + queued = { + f"{tool.name}.{name}" + for tool in tools_for_role(ROLE_OPS, _catalogue()) + for name in tool.parameters.get("properties", {}) + if any(keyword in name.lower() for keyword in _PATH_PARAM_KEYWORDS) + } + assert {"scan_aibom.output_file", "ai_ingest_library.output_dir"} <= queued + + +def test_a_synthetic_unconfined_path_fails_the_audit( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(path_confinement, "PATH_CONFINEMENTS", {}) + monkeypatch.setattr(path_confinement, "NOT_A_FILE_PATH", {}) + tool = SimpleNamespace( + name="synthetic_tool", + parameters={"properties": {"output_path": {"type": "string"}}}, + ) + problems = audit_path_parameters([tool]) + assert len(problems) == 1 + assert "synthetic_tool.output_path" in problems[0] + + +def test_a_short_reason_fails_the_audit(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(path_confinement, "PATH_CONFINEMENTS", {}) + monkeypatch.setattr( + path_confinement, "NOT_A_FILE_PATH", {"synthetic_tool": {"target": "a host"}} + ) + tool = SimpleNamespace( + name="synthetic_tool", + parameters={"properties": {"target": {"type": "string"}}}, + ) + problems = audit_path_parameters([tool]) + assert len(problems) == 1 + assert "synthetic_tool.target" in problems[0] + + +def test_a_stale_entry_fails_the_audit(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + path_confinement, "PATH_CONFINEMENTS", {"gone_tool": frozenset({"file_path"})} + ) + monkeypatch.setattr(path_confinement, "NOT_A_FILE_PATH", {}) + problems = audit_path_parameters([]) + assert len(problems) == 1 + assert "gone_tool.file_path" in problems[0] + + +def test_a_path_outside_the_work_tree_is_refused(monkeypatch: pytest.MonkeyPatch) -> None: + started: list[list[str]] = [] + monkeypatch.setattr( + "devops_cli.ai.mcp.server._run_mcp_cmd", + lambda cmd, timeout: started.append(cmd) or "ok", + ) + tool = _tool_fn(("ai", "ast", "parse")) + with pytest.raises(ValidationError, match="file_path"): + tool(file_path="/etc/passwd") + assert started == [] + + +def test_a_traversal_value_is_refused(monkeypatch: pytest.MonkeyPatch) -> None: + started: list[list[str]] = [] + monkeypatch.setattr( + "devops_cli.ai.mcp.server._run_mcp_cmd", + lambda cmd, timeout: started.append(cmd) or "ok", + ) + tool = _tool_fn(("gh", "pages", "verify")) + with pytest.raises(ValidationError, match="root_dir"): + tool(root_dir="../../etc/passwd") + assert started == [] + + +def test_a_relative_path_resolves_under_the_work_tree(monkeypatch: pytest.MonkeyPatch) -> None: + root = find_worktree_root(Path.cwd()) + started: list[list[str]] = [] + monkeypatch.setattr( + "devops_cli.ai.mcp.server._run_mcp_cmd", + lambda cmd, timeout: started.append(cmd) or "ok", + ) + tool = _tool_fn(("ai", "ast", "parse")) + assert tool(file_path="README.md") == "ok" + assert str(root / "README.md") in started[0] diff --git a/tests/roadmap/test_roadmap_intake.py b/tests/roadmap/test_roadmap_intake.py index 8c56f0e16..6fc16195d 100644 --- a/tests/roadmap/test_roadmap_intake.py +++ b/tests/roadmap/test_roadmap_intake.py @@ -1730,8 +1730,11 @@ def run(cmd: list[str], **_: object) -> str: def test_the_mcp_mirror_plans_by_default_and_builds_a_command_line_the_cli_accepts( - opened: Roadmap, fake_model: FakeModel, tmp_path: Path + opened: Roadmap, fake_model: FakeModel, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: + from devops_cli.ai.mcp import generator + + monkeypatch.setattr(generator, "confinement_root", lambda: tmp_path) opened.issue("feat: from mcp") tool = asyncio.run(mcp_server.mcp.get_tool("roadmap_intake")) names = {listed.name for listed in asyncio.run(mcp_server.mcp._list_tools())} diff --git a/tests/tf/test_tf_cost.py b/tests/tf/test_tf_cost.py index 2289e7af6..a236cd7c3 100644 --- a/tests/tf/test_tf_cost.py +++ b/tests/tf/test_tf_cost.py @@ -215,10 +215,12 @@ def test_tf_cost_command_dry_run(tmp_path: Path) -> None: assert "infracost breakdown" in result.output -def test_tf_cost_breakdown_mcp_tool(tmp_path: Path) -> None: +def test_tf_cost_breakdown_mcp_tool(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Verify FastMCP tool tf_cost_breakdown produces valid JSON output.""" + from devops_cli.ai.mcp import generator from devops_cli.ai.mcp.server import tf_cost_breakdown + monkeypatch.setattr(generator, "confinement_root", lambda: tmp_path) out = tf_cost_breakdown(directory=str(tmp_path), mock=True) assert "total_monthly_cost" in out