diff --git a/CHANGELOG.md b/CHANGELOG.md index b8ef1e7..e4a53c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `plenum mcp` connection binding at server registration: `--project-path`, `--name`, and `--dsn-env` decouple resolution from the launcher's cwd ([REF-297](/REF/issues/REF-297)) +- MCP `connect` save mode (`save`: `"local"` | `"global"`, `project_path`, `name`) that persists connections by credential reference; `password_env` and `keychain_service`/`keychain_account` accepted on `connect`, `introspect`, and `query` ([REF-298](/REF/issues/REF-298)) +- `plenum connect` wizard offers plaintext, env var, shell command, and OS keychain credential sources ([REF-299](/REF/issues/REF-299)) +- `mcp_stanza` in `plenum connect --save` output: a ready-to-paste `mcpServers` block for the saved connection ([REF-299](/REF/issues/REF-299)) +- JSON Schema `schemas/connect_save_success.json` for the `connect --save` envelope, covered by schema drift ([REF-304](/REF/issues/REF-304)) - `--diff-against` flag for `plenum introspect` — read-only schema diff against a prior snapshot ([REF-281](/REF/issues/REF-281)) - Live-DB test harness: Docker Compose fixtures, vendor seed SQL per engine, gated test suites, and `scripts/test-live.sh` orchestration ([REF-275](/REF/issues/REF-275)) - MySQL 8.0 and 8.4 live test matrix: connect, introspect, query, safety, and envelope coverage ([REF-276](/REF/issues/REF-276)) @@ -27,11 +32,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- DuckDB: compile against `duckdb` 1.10506.0, where `Value` is `#[non_exhaustive]`; `UHUGEINT` converts to a string, `GEOMETRY` (WKB) to Base64, and an unknown value type is a structured `QUERY_FAILED` error instead of a guess +- `plenum connect --project-path X --save local` reported success but persisted nothing when run from another directory; it now writes `X/.plenum/config.json` ([REF-303](/REF/issues/REF-303)) - PostgreSQL: correct NULL detection, composite foreign-key introspection, and view definitions ([REF-277](/REF/issues/REF-277)) - MySQL: route text-protocol statements correctly; classify `EXPLAIN`; surface timeout as a first-class error ([REF-258](/REF/issues/REF-258)) - Build: vendor OpenSSL for hermetic release builds ([REF-258](/REF/issues/REF-258)) - Capability: reject `WITH`-CTE DML bypasses across all engines ([REF-41](/REF/issues/REF-41)) +### Security + +- MCP `connect` rejects inline plaintext `password` with `CAPABILITY_VIOLATION` ([REF-298](/REF/issues/REF-298)) +- All MCP tools reject `password_command` with `CAPABILITY_VIOLATION`: it executes a shell command, so accepting it from tool arguments gave agents arbitrary command execution. The CLI flag and wizard still offer it. + ### Changed - SQLite: tighten read-only `PRAGMA` allowlist ([REF-44](/REF/issues/REF-44)) diff --git a/CLAUDE.md b/CLAUDE.md index d462053..dde3deb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -266,6 +266,20 @@ Plenum is exposed via a local MCP server. The Plenum CLI remains the execution boundary. +**Connection binding (`plenum mcp` flags):** +- `--project-path ` — resolve config for that project, not the server's cwd +- `--name ` — select a named connection within that project +- `--dsn-env ` — read the DSN from exactly that env var; never from ambient vars such as `DATABASE_URL` +- `--dsn-env` conflicts with `--project-path` and `--name` +- Connection fields in a tool call override the binding +- No binding and no cwd config → structured error naming the flags to add + +**Credential references on MCP tools:** +- Accepted: `password_env`, `keychain_service` + `keychain_account` (at most one source per call) +- `connect` rejects inline plaintext `password` with `CAPABILITY_VIOLATION` +- Every tool rejects `password_command` with `CAPABILITY_VIOLATION` — it executes a shell command, so it is CLI-only (a human types it) +- `connect` with `save` (`"local"` | `"global"`) stores the reference, never the secret + --- ## Rust Architecture Expectations diff --git a/Cargo.toml b/Cargo.toml index 43849ad..39c33a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,7 +65,7 @@ postgres-native-tls = { version = "0.5", optional = true } # TLS for tokio-post native-tls = { version = "0.2", optional = true, features = ["vendored"] } # Platform TLS (OpenSSL/Secure Transport/SChannel) mysql_async = { version = "0.34", default-features = false, features = ["default-rustls"], optional = true } # MySQL native async driver rusqlite = { version = "0.32", features = ["bundled"], optional = true } # SQLite native driver with bundled lib -duckdb = { version = "1.10504.0", features = ["bundled"], optional = true } # DuckDB native driver with bundled lib +duckdb = { version = "1.10506.0", features = ["bundled"], optional = true } # DuckDB native driver with bundled lib # BLOB encoding (Base64) - used by SQLite and PostgreSQL base64 = "0.22" diff --git a/README.md b/README.md index 174468f..3bd8008 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,70 @@ cargo build --release - Rust 1.70 or later - Supported platforms: Linux, macOS, Windows +## Zero to Query + +Two paths from nothing to a working query result. + +### Path A — CLI-first (recommended for most setups) + +**1. Save a connection:** + +```bash +# Save a PostgreSQL connection to the current project (password via env var — never plaintext) +plenum connect --name dev --engine postgres \ + --host localhost --port 5432 --user myuser \ + --password-env PGPASSWORD --database mydb --save local +``` + +**2. Query immediately:** + +```bash +plenum query --sql "SELECT COUNT(*) FROM users" +# => {"ok":true,"engine":"postgres","command":"query","data":{"rows":[{"count":42}]},"meta":{...}} +``` + +Plenum resolves the `dev` connection automatically from `.plenum/config.json` because it is the default for this project directory. + +--- + +### Path B — MCP-first (for AI coding agents via Claude Desktop or editors) + +Register Plenum in your MCP client config with the project path bound at registration time. **You must supply `--project-path`** — see the [cwd-Coupling Warning](#cwd-coupling-warning) below. + +**Option 1 — use a saved named connection:** + +```json +{ + "mcpServers": { + "plenum": { + "command": "plenum", + "args": ["mcp", "--project-path", "/abs/path/to/your/project", "--name", "dev"] + } + } +} +``` + +Run `plenum connect` first (Path A) to save the connection, then add the stanza above. + +**Option 2 — use a DSN from a named env var (no saved config needed):** + +```json +{ + "mcpServers": { + "plenum": { + "command": "plenum", + "args": ["mcp", "--dsn-env", "DATABASE_URL"] + } + } +} +``` + +`DATABASE_URL` is read at connection time from that env var only. No ambient credential scanning. + +Once the MCP server is running the agent can call the `query` tool without any further setup. + +--- + ## Usage Plenum provides exactly three commands: @@ -355,29 +419,63 @@ Agents should: ## MCP Integration -Plenum exposes functionality via MCP (Model Context Protocol) server: +Plenum exposes all three CLI commands as MCP tools via a local MCP server. The server is started with `plenum mcp` and connection binding is controlled at registration time: ```bash -# Start MCP server (hidden command, for AI agent use) -plenum mcp +plenum mcp --project-path /abs/path/to/project # pin to a project (recommended) +plenum mcp --project-path /abs/path/to/project --name prod # pin to a named connection +plenum mcp --dsn-env DATABASE_URL # read DSN from a named env var ``` -Configure in your MCP client: +### cwd-Coupling Warning + +When `plenum mcp` is started **without** `--project-path` or `--dsn-env`, connection resolution falls back to the server process's working directory. MCP clients (Claude Desktop, editors, daemons) launch `plenum mcp` from arbitrary directories — often not your project root. The result is a failed resolution, not a wrong connection: the tool call fails with a structured error that names the missing flag to add. + +**Always supply `--project-path` or `--dsn-env` in your `mcpServers` stanza.** + +### mcpServers configuration + ```json { "mcpServers": { "plenum": { "command": "plenum", - "args": ["mcp"] + "args": ["mcp", "--project-path", "/abs/path/to/your/project", "--name", "dev"] } } } ``` -Each CLI command maps to an MCP tool: -- `connect` → Validate and save database connections -- `introspect` → Retrieve schema information -- `query` → Execute constrained SQL queries +### MCP tools + +Each CLI command maps to one MCP tool. Tool-call arguments mirror the CLI flags and take precedence over the binding set at server start. + +#### `connect` tool + +Validates and persists a database connection **by credential reference**. Inline plaintext passwords and `password_command` are rejected with `CAPABILITY_VIOLATION` at the MCP boundary — never pass raw secrets through tool arguments. + +| Field | Description | +|-------|-------------| +| `engine` | `postgres`, `mysql`, `sqlite`, or `duckdb` | +| `host`, `port`, `user`, `database` | Server coordinates (postgres/mysql) | +| `file` | File path (sqlite/duckdb) | +| `password_env` | Name of env var holding the password | +| `keychain_service` + `keychain_account` | OS keychain lookup pair (both required together) | +| `save` | `"local"` (`.plenum/config.json`) or `"global"` (`~/.config/plenum/connections.json`) | +| `project_path` | Absolute path of the project to save the connection under | +| `name` | Connection name (defaults to `"default"`) | + +At most one credential-reference source (`password_env` or the keychain pair) may be supplied per call. + +`password_command` is rejected on every MCP tool because it executes a shell command. Accepting it from tool arguments would let an agent run arbitrary commands on the host. Use `--password-command` from the CLI (`plenum connect`) instead — a human types it there. + +#### `introspect` tool + +Retrieves schema information. Accepts the same credential-reference fields (`password_env`, `keychain_service`/`keychain_account`) for one-off explicit connections — secrets are resolved at connect time and never appear in tool arguments or logs. + +#### `query` tool + +Executes a constrained, read-only SQL query. Accepts the same credential-reference fields as `introspect`. ## Architecture @@ -402,6 +500,15 @@ Credentials are stored as **plaintext JSON** in config files: - Secure config files with OS-level permissions (`chmod 600`) - Avoid `--password` CLI flag (visible in process listings) +#### Credential Sourcing Safety Rules + +Plenum's credential sourcing is deterministic and reference-based. These rules hold unconditionally: + +1. **Explicit reference only** — The MCP `connect` tool rejects inline plaintext passwords with `CAPABILITY_VIOLATION`. Credentials flow through Plenum only as named references (`password_env`, `keychain_service`/`keychain_account`). MCP tools reject `password_command` because it executes a shell command; it is available only from the CLI. The CLI `--password` flag is available but strongly discouraged (visible in process listings and shell history). +2. **No ambient pickup** — `--dsn-env` reads exactly the one env var you name. Plenum never scans `DATABASE_URL`, `PGPASSWORD`, or any other ambient variable on its own. +3. **No scanning** — Plenum reads only the two fixed config file paths (`.plenum/config.json` and `~/.config/plenum/connections.json`). No directory walking, no inference from arbitrary paths. +4. **Deterministic resolution** — Given the same `--project-path`, `--name`, and env vars, the same connection is always resolved. The precedence order is: explicit CLI flags → local config → global config → structured error. + #### Security Reporting For detailed security documentation, threat model, and vulnerability reporting, see **[SECURITY.md](SECURITY.md)**. diff --git a/SECURITY.md b/SECURITY.md index 921398d..6d4e777 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -172,6 +172,27 @@ When using the MCP server, credentials are passed per-invocation via JSON-RPC: **Security Note**: MCP communication over stdio is local-only (no network exposure). +### MCP Credential References + +The MCP `connect` tool rejects an inline plaintext `password` with `CAPABILITY_VIOLATION`. Agents reference the secret instead: + +- `password_env` — the name of an environment variable that holds the password. +- `keychain_service` + `keychain_account` — an OS keychain entry (both are required together). + +**`password_command` is rejected on every MCP tool** (`connect`, `introspect`, `query`) with `CAPABILITY_VIOLATION`. It executes a shell command via `sh -c`. Accepting it from tool arguments would give any connected agent arbitrary command execution on the host. The CLI flag `--password-command` and the `plenum connect` wizard still offer it, because a human types the command there. + +**Residual risk — saved configs:** a connection already saved with `password_command` still runs that command when the MCP server resolves it. The command comes from a config file, not from tool arguments. Review `password_command` entries in `.plenum/config.json` before you commit or pull that file, because it is team-shareable. + +### MCP Connection Binding + +`plenum mcp` binds its default connection at server start: + +- `--project-path ` — resolve config for that project, independent of the server's working directory. +- `--name ` — select a named connection within that project. +- `--dsn-env ` — read the DSN from exactly the named environment variable. Plenum never reads `DATABASE_URL`, `PGPASSWORD`, or any other ambient variable on its own. + +`--dsn-env` cannot be combined with `--project-path` or `--name`. Connection fields in a tool call override the binding. + --- ## SQL Injection & Query Validation diff --git a/schemas/connect_save_success.json b/schemas/connect_save_success.json new file mode 100644 index 0000000..c9ddc44 --- /dev/null +++ b/schemas/connect_save_success.json @@ -0,0 +1,178 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SuccessEnvelope_for_ConnectSaveResult", + "description": "Success envelope for operation results\n\nGeneric over the data type to support different operation return values.", + "type": "object", + "required": [ + "command", + "data", + "engine", + "meta", + "ok" + ], + "properties": { + "command": { + "description": "Command that was executed (connect, introspect, query)", + "type": "string" + }, + "data": { + "description": "Operation-specific data", + "allOf": [ + { + "$ref": "#/definitions/ConnectSaveResult" + } + ] + }, + "engine": { + "description": "Database engine used for this operation (postgres, mysql, sqlite)", + "type": "string" + }, + "meta": { + "description": "Execution metadata", + "allOf": [ + { + "$ref": "#/definitions/Metadata" + } + ] + }, + "ok": { + "description": "Always true for success envelopes", + "type": "boolean" + } + }, + "definitions": { + "ConnectSaveResult": { + "description": "Success `data` payload emitted by `plenum connect --save` after a connection is persisted.\n\nTyping this envelope — rather than assembling it inline via `serde_json::json!` at the call site — keeps the emitted shape, including the nested [`McpStanza`], under `generate-schemas` + `tests/schema_drift.rs` coverage so the stanza cannot drift without a failing test.", + "type": "object", + "required": [ + "connection_name", + "engine", + "mcp_stanza", + "message", + "saved_to" + ], + "properties": { + "connection_name": { + "description": "Name the connection was saved under.", + "type": "string" + }, + "engine": { + "description": "Database engine of the saved connection (postgres, mysql, sqlite).", + "type": "string" + }, + "mcp_stanza": { + "description": "Ready-to-paste MCP server stanza for the saved connection.", + "allOf": [ + { + "$ref": "#/definitions/McpStanza" + } + ] + }, + "message": { + "description": "Human-readable confirmation message.", + "type": "string" + }, + "saved_to": { + "description": "Where the connection was persisted: `\"local\"` or `\"global\"`.", + "type": "string" + } + } + }, + "McpServerConfig": { + "description": "A single MCP server entry within an [`McpStanza`].\n\nMirrors the shape MCP hosts expect under `mcpServers.`: an executable plus the exact argv used to launch Plenum's MCP server for the saved connection.", + "type": "object", + "required": [ + "args", + "command" + ], + "properties": { + "args": { + "description": "Arguments passed to the executable. Reflects the W1 launch flags (`mcp --project-path --name `) for the connection just saved.", + "type": "array", + "items": { + "type": "string" + } + }, + "command": { + "description": "Executable to launch (always `\"plenum\"`).", + "type": "string" + } + } + }, + "McpStanza": { + "description": "Ready-to-paste `mcpServers` configuration block emitted after `plenum connect` saves a connection.\n\nThe agent operator can copy this verbatim into an MCP host config (e.g. `claude_desktop_config.json`) to wire up the Plenum MCP server for the saved connection without consulting setup documentation.\n\nOnly the connection's project path and name are encoded — never a credential or secret. Indirect credential sources remain resolved at connection time by the stored connection reference.", + "type": "object", + "required": [ + "mcpServers" + ], + "properties": { + "mcpServers": { + "description": "Map of MCP server name to launch config. Always contains a single `\"plenum\"` entry.", + "type": "object", + "additionalProperties": { + "$ref": "#/definitions/McpServerConfig" + } + } + } + }, + "Metadata": { + "description": "Execution metadata included in all responses", + "type": "object", + "required": [ + "contract_version", + "execution_ms" + ], + "properties": { + "contract_version": { + "description": "Output contract version — agents use this to guard against silent breaking changes", + "type": "string" + }, + "execution_ms": { + "description": "Execution time in milliseconds", + "type": "integer", + "format": "uint64", + "minimum": 0.0 + }, + "has_more": { + "description": "Whether more rows are available beyond this page (query results only)", + "type": [ + "boolean", + "null" + ] + }, + "next_offset": { + "description": "Offset to pass as --offset for the next page (present only when `has_more` is true)", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, + "rows_returned": { + "description": "Number of rows returned (for query results, None for other operations)", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, + "rows_truncated": { + "description": "Whether the result set was capped by `max_rows` (query results only)", + "type": [ + "boolean", + "null" + ] + }, + "truncated_by": { + "description": "Why the result was truncated: \"bytes\" when `max_bytes` triggered the cut; absent for row-count truncation or when rows are not truncated", + "type": [ + "string", + "null" + ] + } + } + } + } +} diff --git a/src/bin/generate_schemas.rs b/src/bin/generate_schemas.rs index 4491764..5c94594 100644 --- a/src/bin/generate_schemas.rs +++ b/src/bin/generate_schemas.rs @@ -6,7 +6,10 @@ //! Run this whenever output types change to keep the checked-in schemas in sync. //! The drift test in `tests/schema_drift.rs` fails if schemas are stale. -use plenum::{ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, SuccessEnvelope}; +use plenum::{ + ConnectSaveResult, ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, + SuccessEnvelope, +}; use schemars::schema_for; use std::fs; @@ -14,6 +17,7 @@ fn main() { let schemas: &[(&str, schemars::schema::RootSchema)] = &[ ("schemas/error_envelope.json", schema_for!(ErrorEnvelope)), ("schemas/connect_success.json", schema_for!(SuccessEnvelope)), + ("schemas/connect_save_success.json", schema_for!(SuccessEnvelope)), ("schemas/introspect_success.json", schema_for!(SuccessEnvelope)), ("schemas/query_success.json", schema_for!(SuccessEnvelope)), ]; diff --git a/src/config/mod.rs b/src/config/mod.rs index 0822d83..8fe7a06 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -457,55 +457,88 @@ pub fn load_with_precedence() -> Result { } } -/// Resolve a connection by project path and name +/// Resolve a connection for a specific project directory. /// -/// Searches in merged view (both local and global configs). -/// Returns an error if the connection is not found. -/// Returns a tuple of (`ConnectionConfig`, `is_readonly`). -/// -/// # Parameters -/// - `project_path`: Optional project path. If None, uses current working directory. -/// - `name`: Optional connection name. If None, uses the project's default connection. -pub fn resolve_connection( - project_path: Option<&str>, +/// Unlike [`resolve_connection`], this loads the local config from +/// `/.plenum/config.json` rather than from the current working +/// directory. Required when `plenum mcp --project-path ` is invoked from +/// a different directory than the project (e.g. MCP clients that start the +/// server from `$HOME`). +pub fn resolve_connection_in_project( + project_path: &str, name: Option<&str>, ) -> Result<(ConnectionConfig, bool)> { - // Determine project path (use provided or get current) - let path = match project_path { - Some(p) => p.to_string(), - None => get_current_project_path()?, - }; + // Load local config from the project directory itself, not from CWD. + let local_path = PathBuf::from(project_path).join(".plenum").join("config.json"); + let global_path = global_config_path()?; + let registry = load_with_explicit_local(&local_path, &global_path)?; - // Search in merged view (both local and global) - let registry = load_with_precedence()?; + // The project key stored by load_registry is the canonicalized parent of + // `.plenum/config.json`. Canonicalize the given path to match. + let canon = PathBuf::from(project_path) + .canonicalize() + .ok() + .and_then(|p| p.to_str().map(str::to_string)) + .unwrap_or_else(|| project_path.to_string()); - // Look up project in registry - let project = registry.projects.get(&path).ok_or_else(|| { + resolve_from_registry(®istry, &canon, name) +} + +/// Load a merged registry using an explicit local config path instead of CWD. +/// +/// Merging semantics are identical to [`load_with_precedence`]: local wins for +/// the same project path, global provides everything else. +fn load_with_explicit_local(local_path: &Path, global_path: &Path) -> Result { + match (local_path.exists(), global_path.exists()) { + (false, false) => Ok(ConnectionRegistry::default()), + (true, false) => load_registry(local_path), + (false, true) => load_registry(global_path), + (true, true) => { + let global_registry = load_registry(global_path)?; + let local_registry = load_registry(local_path)?; + let mut merged = global_registry; + for (project_path, local_project) in local_registry.projects { + let project_entry = merged.projects.entry(project_path).or_default(); + for (conn_name, conn) in local_project.connections { + project_entry.connections.insert(conn_name, conn); + } + if local_project.default.is_some() { + project_entry.default = local_project.default; + } + } + Ok(merged) + } + } +} + +/// Look up a named (or default) connection from a registry for a given project path. +fn resolve_from_registry( + registry: &ConnectionRegistry, + path: &str, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + let project = registry.projects.get(path).ok_or_else(|| { PlenumError::config_error(format!( "No connections found for project path '{path}'. Run 'plenum connect' to create one." )) })?; - // Determine connection name (use provided or project's default) let conn_name = match name { Some(n) => n.to_string(), - None => { - // Use project's default - project - .default - .as_ref() - .ok_or_else(|| { - let available: Vec<_> = project.connections.keys().collect(); - PlenumError::config_error(format!( - "No default connection set for project '{path}'. Available connections: {available:?}. \ - Specify one with --name or set a default in the config." + None => project + .default + .as_ref() + .ok_or_else(|| { + let available: Vec<_> = project.connections.keys().collect(); + PlenumError::config_error(format!( + "No default connection set for project '{path}'. \ + Available connections: {available:?}. \ + Specify one with --name or set a default in the config." )) - })? - .clone() - } + })? + .clone(), }; - // Look up connection by name within project let stored = project.connections.get(&conn_name).ok_or_else(|| { let available: Vec<_> = project.connections.keys().collect(); let default_info = match &project.default { @@ -513,14 +546,39 @@ pub fn resolve_connection( None => String::new(), }; PlenumError::config_error(format!( - "Connection '{conn_name}' not found for project '{path}'. Available connections: {available:?}{default_info}" + "Connection '{conn_name}' not found for project '{path}'. \ + Available connections: {available:?}{default_info}" )) })?; - // Resolve environment variables and get readonly flag stored.resolve() } +/// Resolve a connection by project path and name +/// +/// Searches in merged view (both local and global configs). +/// Returns an error if the connection is not found. +/// Returns a tuple of (`ConnectionConfig`, `is_readonly`). +/// +/// # Parameters +/// - `project_path`: Optional project path. If None, uses current working directory. +/// - `name`: Optional connection name. If None, uses the project's default connection. +pub fn resolve_connection( + project_path: Option<&str>, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + // Determine project path (use provided or get current) + let path = match project_path { + Some(p) => p.to_string(), + None => get_current_project_path()?, + }; + + // Search in merged view (both local and global) + let registry = load_with_precedence()?; + + resolve_from_registry(®istry, &path, name) +} + /// Save a connection to a config file /// /// # Parameters @@ -587,6 +645,66 @@ pub fn save_connection( Ok(()) } +/// Save a connection scoped to an explicit project directory. +/// +/// Unlike [`save_connection`], the local config file is written to +/// `/.plenum/config.json` rather than `/.plenum/config.json`. +/// This is required by the MCP server, which is long-lived and started from an +/// arbitrary cwd (Claude Desktop, editors, daemons) — the project path must be +/// explicit, never inferred from cwd. +/// +/// Credentials are persisted only as references (`password_env`, +/// `password_command`, `keychain_entry`); no plaintext secret is ever written. +/// +/// Only one of `password_env`, `password_command`, or `keychain_entry` may be `Some`. +pub fn save_connection_in_project( + project_path: &str, + name: Option, + config: ConnectionConfig, + password_env: Option, + password_command: Option, + keychain_entry: Option, + location: ConfigLocation, +) -> Result<()> { + let conn_name = name.unwrap_or_else(|| "default".to_string()); + + // Local config lives under the project directory itself; global is per-user. + let config_path = match location { + ConfigLocation::Local => PathBuf::from(project_path).join(".plenum").join("config.json"), + ConfigLocation::Global => global_config_path()?, + }; + + // Registry key: `save_registry` (local path) and `load_registry` both key by + // the canonicalized project root. Match that so lookups line up. + let key = PathBuf::from(project_path) + .canonicalize() + .ok() + .and_then(|p| p.to_str().map(String::from)) + .unwrap_or_else(|| project_path.to_string()); + + let mut registry = if config_path.exists() { + load_registry(&config_path)? + } else { + ConnectionRegistry::default() + }; + + let project = registry.projects.entry(key).or_default(); + let is_first_connection = project.connections.is_empty(); + + project.connections.insert( + conn_name.clone(), + StoredConnection { config, password_env, password_command, keychain_entry, readonly: None }, + ); + + if is_first_connection { + project.default = Some(conn_name); + } + + save_registry(&config_path, ®istry)?; + + Ok(()) +} + /// List all available connections /// /// Returns a Vec of tuples: (`project_path`, `connection_name`, config) @@ -1596,4 +1714,62 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().message().contains("Only one password source")); } + + // --- save_connection_in_project tests --- + + #[test] + fn test_save_connection_in_project_local_writes_by_reference() { + // Local save must write to /.plenum/config.json and persist the + // credential reference (password_env) with NO plaintext password. + let project = std::env::temp_dir().join(format!( + "plenum-save-itest-{}-{:p}", + std::process::id(), + &0u8 + )); + let _ = fs::remove_dir_all(&project); + fs::create_dir_all(&project).unwrap(); + let project_str = project.to_str().unwrap().to_string(); + + let config = ConnectionConfig { + engine: DatabaseType::Postgres, + host: Some("db.example.com".to_string()), + port: Some(5432), + user: Some("agent".to_string()), + password: None, + database: Some("app".to_string()), + file: None, + tls: None, + }; + + save_connection_in_project( + &project_str, + Some("prod".to_string()), + config, + Some("DB_PASSWORD".to_string()), + None, + None, + ConfigLocation::Local, + ) + .unwrap(); + + let written = fs::read_to_string(project.join(".plenum").join("config.json")).unwrap(); + assert!(written.contains("\"password_env\""), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD"), "env var name stored: {written}"); + assert!(!written.contains("\"password\":"), "no plaintext password: {written}"); + // First connection becomes the default pointer. + assert!(written.contains("\"default\"")); + assert!(written.contains("prod")); + + // Round-trip: load the file back and confirm the connection is keyed by + // the canonicalized project root with its reference intact. + let canon = project.canonicalize().unwrap(); + let registry = load_registry(&project.join(".plenum").join("config.json")).unwrap(); + let stored_project = registry.projects.get(canon.to_str().unwrap()).unwrap(); + assert_eq!(stored_project.default.as_deref(), Some("prod")); + let stored = stored_project.connections.get("prod").unwrap(); + assert_eq!(stored.password_env.as_deref(), Some("DB_PASSWORD")); + assert!(stored.config.password.is_none()); + + let _ = fs::remove_dir_all(&project); + } } diff --git a/src/diff.rs b/src/diff.rs index 9acf39a..0d72e3a 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -450,8 +450,8 @@ mod tests { fn test_identical_tables_produce_empty_diff() { let tables = vec![make_table("users", vec![make_col("id", "int", false)])]; let (added, removed, changed) = diff_tables(&tables, &tables); - assert!(added.is_empty()); - assert!(removed.is_empty()); + assert_eq!(added, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(changed.is_empty()); } @@ -464,7 +464,7 @@ mod tests { ]; let (added, removed, _) = diff_tables(&base, &target); assert_eq!(added, vec!["orders"]); - assert!(removed.is_empty()); + assert_eq!(removed, Vec::::new()); } #[test] @@ -475,7 +475,7 @@ mod tests { ]; let target = vec![make_table("users", vec![make_col("id", "int", false)])]; let (added, removed, _) = diff_tables(&base, &target); - assert!(added.is_empty()); + assert_eq!(added, Vec::::new()); assert_eq!(removed, vec!["orders"]); } @@ -592,8 +592,8 @@ mod tests { let views = vec![make_view("v_active", Some("SELECT 1"), vec![make_col("id", "int", false)])]; let (added, removed, changed) = diff_views(&views, &views); - assert!(added.is_empty()); - assert!(removed.is_empty()); + assert_eq!(added, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(changed.is_empty()); } diff --git a/src/engine/duckdb/mod.rs b/src/engine/duckdb/mod.rs index 3ef9037..6246b35 100644 --- a/src/engine/duckdb/mod.rs +++ b/src/engine/duckdb/mod.rs @@ -39,6 +39,12 @@ pub struct DuckDbEngine; /// Default schema used when no `--schema` is provided. const DEFAULT_SCHEMA: &str = "main"; +// `DatabaseEngine` declares these methods as returning futures, so each impl must +// be `async` whether or not its body awaits. The DuckDB driver is synchronous, so +// no method here awaits. `clippy::unused_async_trait_impl` (Rust 1.98+) flags +// that; `unknown_lints` keeps older toolchains from rejecting the new lint name. +#[allow(unknown_lints)] +#[allow(clippy::unused_async_trait_impl)] impl DatabaseEngine for DuckDbEngine { async fn validate_connection(config: &ConnectionConfig) -> Result { let file_path = extract_file_path(config)?; @@ -689,7 +695,7 @@ fn execute_query( let value_ref = row.get_ref(idx).map_err(|e| { PlenumError::query_failed(format!("Failed to read column {idx}: {e}")) })?; - values.push(duckdb_value_to_json(&value_ref.to_owned())); + values.push(duckdb_value_to_json(&value_ref.to_owned())?); } rows_data.push(values); pos += 1; @@ -748,14 +754,17 @@ fn f64_to_json(f: f64) -> serde_json::Value { /// represent natively are stringified deterministically: /// - `HUGEINT` / `UHUGEINT` and `DECIMAL` → string (preserves precision) /// - `TIMESTAMP` / `DATE` / `TIME` → ISO-8601 string -/// - `BLOB` → Base64 string +/// - `BLOB` and `GEOMETRY` (WKB bytes) → Base64 string /// - `INTERVAL` → object with `months` / `days` / `nanos` /// - Nested types (`LIST`, `ARRAY`, `STRUCT`, `MAP`, `UNION`, `ENUM`) convert /// recursively to JSON arrays / objects. -fn duckdb_value_to_json(value: &Value) -> serde_json::Value { +/// +/// `duckdb::types::Value` is `#[non_exhaustive]`. A variant this function does +/// not know is a structured error, never a guessed representation. +fn duckdb_value_to_json(value: &Value) -> Result { use serde_json::Value as Json; - match value { + let json = match value { Value::Null => Json::Null, Value::Boolean(b) => Json::Bool(*b), Value::TinyInt(i) => Json::Number((*i).into()), @@ -764,6 +773,7 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { Value::BigInt(i) => Json::Number((*i).into()), // HUGEINT exceeds JSON's i64 range; preserve precision as a string Value::HugeInt(i) => Json::String(i.to_string()), + Value::UHugeInt(i) => Json::String(i.to_string()), Value::UTinyInt(i) => Json::Number((*i).into()), Value::USmallInt(i) => Json::Number((*i).into()), Value::UInt(i) => Json::Number((*i).into()), @@ -774,7 +784,7 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { Value::Decimal(d) => Json::String(d.to_string()), Value::Timestamp(unit, v) => format_timestamp(*unit, *v), Value::Text(s) | Value::Enum(s) => Json::String(s.clone()), - Value::Blob(b) => { + Value::Blob(b) | Value::Geometry(b) => { use base64::Engine; Json::String(base64::engine::general_purpose::STANDARD.encode(b)) } @@ -786,31 +796,40 @@ fn duckdb_value_to_json(value: &Value) -> serde_json::Value { "nanos": nanos, }), Value::List(items) | Value::Array(items) => { - Json::Array(items.iter().map(duckdb_value_to_json).collect()) + Json::Array(items.iter().map(duckdb_value_to_json).collect::>()?) } Value::Struct(map) => { - let obj: serde_json::Map = - map.iter().map(|(k, v)| (k.clone(), duckdb_value_to_json(v))).collect(); + let obj = map + .iter() + .map(|(k, v)| Ok((k.clone(), duckdb_value_to_json(v)?))) + .collect::>>()?; Json::Object(obj) } Value::Map(map) => { - let obj: serde_json::Map = map + let obj = map .iter() .map(|(k, v)| { let key = match k { Value::Text(s) | Value::Enum(s) => s.clone(), - other => match duckdb_value_to_json(other) { + other => match duckdb_value_to_json(other)? { Json::String(s) => s, j => j.to_string(), }, }; - (key, duckdb_value_to_json(v)) + Ok((key, duckdb_value_to_json(v)?)) }) - .collect(); + .collect::>>()?; Json::Object(obj) } - Value::Union(inner) => duckdb_value_to_json(inner), - } + Value::Union(inner) => duckdb_value_to_json(inner)?, + other => { + return Err(PlenumError::query_failed(format!( + "Unsupported DuckDB value type in result set: {:?}", + other.data_type() + ))); + } + }; + Ok(json) } #[cfg(test)] @@ -1181,6 +1200,44 @@ mod tests { let _ = std::fs::remove_file(&temp_file); } + #[test] + fn uhugeint_converts_to_a_string() { + // UHUGEINT exceeds JSON's number range; preserve precision as a string. + let json = duckdb_value_to_json(&Value::UHugeInt(u128::MAX)).expect("convert"); + assert_eq!(json, serde_json::json!("340282366920938463463374607431768211455")); + } + + #[test] + fn geometry_converts_to_base64_wkb() { + // GEOMETRY arrives as WKB bytes; encode like BLOB. + let json = + duckdb_value_to_json(&Value::Geometry(vec![0xDE, 0xAD, 0xBE, 0xEF])).expect("convert"); + assert_eq!(json, serde_json::json!("3q2+7w==")); + } + + #[tokio::test] + async fn test_execute_uhugeint_max_value() { + let temp_file = fixture_path("uhugeint"); + let _ = std::fs::remove_file(&temp_file); + { + let conn = Connection::open(&temp_file).expect("create"); + conn.execute_batch( + "CREATE TABLE big (v UHUGEINT); + INSERT INTO big VALUES (340282366920938463463374607431768211455)", + ) + .expect("seed"); + } + + let config = ConnectionConfig::duckdb(temp_file.clone()); + let caps = Capabilities::default(); + let qr = DuckDbEngine::execute(&config, "SELECT v FROM big", &[], &caps) + .await + .expect("query failed"); + assert_eq!(qr.rows[0][0], serde_json::json!("340282366920938463463374607431768211455")); + + let _ = std::fs::remove_file(&temp_file); + } + #[tokio::test] async fn test_introspect_foreign_keys() { let temp_file = fixture_path("fk"); @@ -1410,7 +1467,7 @@ mod tests { assert!(result.is_ok(), "native explain failed: {:?}", result.err()); let qr = result.unwrap(); assert!(qr.plan.is_none()); - assert!(!qr.rows.is_empty()); + assert_ne!(qr.rows.len(), 0); } // ========================================================================= diff --git a/src/engine/mysql/mod.rs b/src/engine/mysql/mod.rs index 329e884..4724cce 100644 --- a/src/engine/mysql/mod.rs +++ b/src/engine/mysql/mod.rs @@ -1317,7 +1317,7 @@ mod tests { assert!(result.is_ok(), "Connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("MySQL") || info.server_info.contains("MariaDB")); } @@ -1337,7 +1337,7 @@ mod tests { assert!(result.is_ok(), "Wildcard connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("MySQL") || info.server_info.contains("MariaDB")); assert_eq!(info.connected_database, "(no database selected)"); } @@ -1360,7 +1360,7 @@ mod tests { let query_result = result.unwrap(); assert_eq!(query_result.columns.len(), 1); - assert!(!query_result.rows.is_empty()); + assert_ne!(query_result.rows.len(), 0); } #[tokio::test] diff --git a/src/engine/postgres/mod.rs b/src/engine/postgres/mod.rs index a28d276..3c829ce 100644 --- a/src/engine/postgres/mod.rs +++ b/src/engine/postgres/mod.rs @@ -1308,7 +1308,7 @@ mod tests { assert!(result.is_ok(), "Connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("PostgreSQL")); assert_eq!(info.connected_database, "postgres"); assert_eq!(info.user, "postgres"); @@ -1330,7 +1330,7 @@ mod tests { assert!(result.is_ok(), "Wildcard connection validation failed: {:?}", result.err()); let info = result.unwrap(); - assert!(!info.database_version.is_empty()); + assert_ne!(info.database_version, ""); assert!(info.server_info.contains("PostgreSQL")); assert!(info.connected_database.contains("wildcard mode")); assert!(info.connected_database.contains("postgres")); @@ -1360,7 +1360,7 @@ mod tests { let query_result = result.unwrap(); assert_eq!(query_result.columns.len(), 1); - assert!(!query_result.rows.is_empty()); + assert_ne!(query_result.rows.len(), 0); } #[tokio::test] diff --git a/src/engine/sqlite/mod.rs b/src/engine/sqlite/mod.rs index 038b50b..7279951 100644 --- a/src/engine/sqlite/mod.rs +++ b/src/engine/sqlite/mod.rs @@ -31,6 +31,12 @@ use crate::error::{PlenumError, Result}; /// `SQLite` database engine implementation pub struct SqliteEngine; +// `DatabaseEngine` declares these methods as returning futures, so each impl must +// be `async` whether or not its body awaits. The SQLite driver is synchronous, so +// no method here awaits. `clippy::unused_async_trait_impl` (Rust 1.98+) flags +// that; `unknown_lints` keeps older toolchains from rejecting the new lint name. +#[allow(unknown_lints)] +#[allow(clippy::unused_async_trait_impl)] impl DatabaseEngine for SqliteEngine { async fn validate_connection(config: &ConnectionConfig) -> Result { // Validate config is for SQLite diff --git a/src/lib.rs b/src/lib.rs index b800d32..cfb43d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,7 +42,8 @@ pub mod output; // JSON output envelopes (Phase 1.2) // MCP server (Phase 7) - M pub use capability::validate_query; pub use config::{ list_connections, list_connections_for_project, list_connections_raw, resolve_connection, - save_connection, ConfigLocation, ConnectionRegistry, KeychainEntry, StoredConnection, + resolve_connection_in_project, save_connection, save_connection_in_project, ConfigLocation, + ConnectionRegistry, KeychainEntry, StoredConnection, }; pub use dsn::{parse_dsn, redact_dsn}; pub use engine::{ @@ -52,7 +53,10 @@ pub use engine::{ SchemaInfo, TableDiff, TableInfo, TimeOnlyResult, ViewDiff, ViewInfo, }; pub use error::{PlenumError, Result}; -pub use output::{ErrorEnvelope, ErrorInfo, Metadata, SuccessEnvelope, CONTRACT_VERSION}; +pub use output::{ + ConnectSaveResult, ErrorEnvelope, ErrorInfo, McpServerConfig, McpStanza, Metadata, + SuccessEnvelope, CONTRACT_VERSION, +}; #[cfg(test)] mod tests { diff --git a/src/main.rs b/src/main.rs index ec2d7c0..2454f2e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,17 +30,24 @@ use plenum::engine::postgres::PostgresEngine; #[cfg(feature = "sqlite")] use plenum::engine::sqlite::SqliteEngine; -/// Resolved arguments for `plenum connect`: -/// connection name, project path, config, `password_env`, `password_command`, `keychain_entry`, save location. -type ConnectArgs = ( - String, - Option, - ConnectionConfig, - Option, - Option, - Option, - ConfigLocation, -); +/// Resolved arguments for `plenum connect`, produced by the interactive picker, +/// interactive wizard, or non-interactive flag path and consumed by the save step. +struct ConnectArgs { + /// Connection name to save under. + name: String, + /// Explicit project path, or `None` to default to the current directory. + project_path: Option, + /// Resolved connection configuration. + config: ConnectionConfig, + /// Indirect credential source: environment variable name. + password_env: Option, + /// Indirect credential source: shell command producing the password. + password_command: Option, + /// Indirect credential source: OS keychain entry. + keychain_entry: Option, + /// Where to persist the connection (local or global). + location: ConfigLocation, +} /// Plenum - Agent-First Database Control CLI #[derive(Parser)] @@ -371,7 +378,20 @@ enum Commands { /// Start MCP server (hidden from help, for AI agent integration) #[command(hide = true)] - Mcp, + Mcp { + /// Pin config resolution to this project path (overrides the launcher's cwd) + #[arg(long)] + project_path: Option, + + /// Pin to a named connection within the resolved project + #[arg(long)] + name: Option, + + /// Read the DSN from this named environment variable at connection time. + /// Only the named variable is read — never an ambient fallback. + #[arg(long, conflicts_with_all = ["project_path", "name"])] + dsn_env: Option, + }, } #[tokio::main] @@ -571,7 +591,9 @@ async fn main() { ) .await } - Some(Commands::Mcp) => handle_mcp().await, + Some(Commands::Mcp { project_path, name, dsn_env }) => { + handle_mcp(project_path, name, dsn_env).await + } None => { // No subcommand provided let error_envelope = ErrorEnvelope::new( @@ -734,22 +756,43 @@ async fn handle_connect( .await } else { // Interactive mode: show picker - interactive_connect_picker().await.map(|(n, p, c, l)| (n, p, c, None, None, None, l)) + interactive_connect_picker().await }; match result { - Ok(( - conn_name, - proj_path, + Ok(ConnectArgs { + name: conn_name, + project_path: proj_path, config, password_env, password_command, keychain_entry, location, - )) => { - // Save connection - match plenum::save_connection( - proj_path, + }) => { + // Resolve the effective project path up front so the saved connection + // and the emitted mcp_stanza reference the exact same path. When + // proj_path is None, save_connection would fall back to the current + // directory; resolve it here so both stay in lockstep. + let effective_project_path = match proj_path { + Some(p) => p, + None => match plenum::config::get_current_project_path() { + Ok(p) => p, + Err(e) => { + let envelope = ErrorEnvelope::from_error("", "connect", &e); + output_error(&envelope); + return Err(1); + } + }, + }; + + // Save connection. Use the project-scoped writer so `--save local` + // targets `/.plenum/config.json` rather than the + // current working directory. The local config format is + // single-project and keyed by its own directory, so writing under + // cwd while keying the entry under an explicit `--project-path` + // would silently drop the connection. + match plenum::save_connection_in_project( + &effective_project_path, Some(conn_name.clone()), config.clone(), password_env, @@ -760,15 +803,20 @@ async fn handle_connect( Ok(()) => { // Build success response let elapsed_ms = start.elapsed().as_millis() as u64; - let data = serde_json::json!({ - "connection_name": conn_name, - "engine": config.engine.as_str(), - "saved_to": match location { - ConfigLocation::Local => "local", - ConfigLocation::Global => "global", + let mcp_stanza = plenum::McpStanza::for_saved_connection( + &effective_project_path, + &conn_name, + ); + let data = plenum::ConnectSaveResult { + message: format!("Connection '{conn_name}' saved successfully"), + connection_name: conn_name, + engine: config.engine.as_str().to_string(), + saved_to: match location { + ConfigLocation::Local => "local".to_string(), + ConfigLocation::Global => "global".to_string(), }, - "message": format!("Connection '{}' saved successfully", conn_name), - }); + mcp_stanza, + }; let envelope = SuccessEnvelope::new( config.engine.as_str(), @@ -956,8 +1004,7 @@ async fn handle_connect_test( } /// Interactive connection picker (when no args provided) -async fn interactive_connect_picker( -) -> Result<(String, Option, ConnectionConfig, ConfigLocation)> { +async fn interactive_connect_picker() -> Result { use dialoguer::Select; // Get current project path @@ -1017,13 +1064,150 @@ async fn interactive_connect_picker( // Ask for save location let location = prompt_save_location()?; - Ok((name.clone(), None, config.clone(), location)) + // Re-saving an existing connection preserves its stored credential + // reference as-is; the picker never re-prompts for or scans credentials. + Ok(ConnectArgs { + name: name.clone(), + project_path: None, + config: config.clone(), + password_env: None, + password_command: None, + keychain_entry: None, + location, + }) + } +} + +/// A credential source chosen in the interactive wizard. +/// +/// Each variant carries only what the user explicitly typed — an env var name, +/// a command string, or a keychain service/account. The wizard never scans the +/// environment, keychain, or filesystem to populate these; there is no ambient +/// pickup or discovery. +#[derive(Debug, Clone, PartialEq, Eq)] +enum WizardCredential { + /// Plaintext password typed directly. + Plaintext(String), + /// Name of an environment variable to read at connection time. + Env(String), + /// Shell command whose stdout yields the password at connection time. + Command(String), + /// OS keychain service + account reference. + Keychain { service: String, account: String }, +} + +/// Split a wizard credential choice into the persisted fields +/// `(password, password_env, password_command, keychain_entry)`. +/// +/// Only the user-typed reference is propagated. For indirect sources the inline +/// `password` is `None` (resolved at use time); nothing here reads the +/// environment, keychain, or filesystem. +fn wizard_credential_fields( + cred: WizardCredential, +) -> (Option, Option, Option, Option) { + match cred { + WizardCredential::Plaintext(p) => (Some(p), None, None, None), + WizardCredential::Env(var) => (None, Some(var), None, None), + WizardCredential::Command(cmd) => (None, None, Some(cmd), None), + WizardCredential::Keychain { service, account } => { + (None, None, None, Some(KeychainEntry { service, account })) + } + } +} + +/// Validate that an indirect credential source resolves now, so the wizard fails +/// fast instead of persisting a broken reference. +/// +/// Error messages name the exact field the user must fix and never suggest a +/// discovered alternative (no "did you mean" from a scan of the environment or +/// keychain). +fn validate_wizard_credential(cred: &WizardCredential) -> Result<()> { + match cred { + WizardCredential::Plaintext(_) => Ok(()), + WizardCredential::Env(var) => match std::env::var(var) { + Ok(v) if v.is_empty() => Err(PlenumError::invalid_input(format!( + "Environment variable {var} is set but empty — set {var} or choose a different credential source" + ))), + Ok(_) => Ok(()), + Err(_) => Err(PlenumError::invalid_input(format!( + "Environment variable {var} is not set — set {var} or choose a different credential source" + ))), + }, + WizardCredential::Command(cmd) => plenum::config::run_password_command_pub(cmd) + .map(|_| ()) + .map_err(|e| { + PlenumError::invalid_input(format!( + "password command failed validation: {} — fix the command or choose a different credential source", + e.message() + )) + }), + WizardCredential::Keychain { service, account } => { + plenum::config::lookup_keychain_password_pub(service, account).map(|_| ()).map_err(|e| { + PlenumError::invalid_input(format!( + "keychain lookup for service '{service}' / account '{account}' failed: {} — fix the service/account or choose a different credential source", + e.message() + )) + }) + } + } +} + +/// Prompt for how the password should be provided. The user always *types* the +/// source reference; the wizard never lists or suggests discovered credentials. +fn prompt_wizard_credential() -> Result { + use dialoguer::{Input, Password, Select}; + + let choices = vec![ + "Enter password directly", + "Environment variable (you type the name)", + "Shell command (you type the command)", + "OS keychain (you type service + account)", + ]; + let idx = Select::new() + .with_prompt("How should the password be provided?") + .items(&choices) + .default(0) + .interact() + .map_err(|e| PlenumError::invalid_input(format!("Selection failed: {e}")))?; + + match idx { + 0 => { + let pw: String = Password::new() + .with_prompt("Password") + .interact() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Plaintext(pw)) + } + 1 => { + let var: String = Input::new() + .with_prompt("Environment variable name (e.g. PGPASSWORD)") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Env(var)) + } + 2 => { + let cmd: String = Input::new() + .with_prompt("Shell command to produce the password") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Command(cmd)) + } + _ => { + let service: String = Input::new() + .with_prompt("Keychain service name") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + let account: String = Input::new() + .with_prompt("Keychain account name") + .interact_text() + .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + Ok(WizardCredential::Keychain { service, account }) + } } } /// Interactive connection wizard -async fn interactive_connect_wizard( -) -> Result<(String, Option, ConnectionConfig, ConfigLocation)> { +async fn interactive_connect_wizard() -> Result { use dialoguer::{Input, Select}; eprintln!("\n=== Create New Database Connection ===\n"); @@ -1037,8 +1221,8 @@ async fn interactive_connect_wizard( .map_err(|e| PlenumError::invalid_input(format!("Selection failed: {e}")))?; let engine = parse_engine(engine_choices[engine_idx])?; - // Build config based on engine type - let config = match engine { + // Build config and resolve any indirect credential reference based on engine type. + let (config, password_env, password_command, keychain_entry) = match engine { DatabaseType::Postgres | DatabaseType::MySQL => { let host: String = Input::new() .with_prompt("Host") @@ -1057,21 +1241,30 @@ async fn interactive_connect_wizard( .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - let password: String = dialoguer::Password::new() - .with_prompt("Password") - .interact() - .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; + // Offer plaintext or an indirect credential source, validating that + // the chosen source resolves before we persist the reference. + let cred = prompt_wizard_credential()?; + validate_wizard_credential(&cred)?; let database: String = Input::new() .with_prompt("Database name") .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - if engine == DatabaseType::Postgres { - ConnectionConfig::postgres(host, port, user, password, database) - } else { - ConnectionConfig::mysql(host, port, user, password, database) - } + let (password, p_env, p_cmd, keychain) = wizard_credential_fields(cred); + + let config = ConnectionConfig { + engine, + host: Some(host), + port: Some(port), + user: Some(user), + // None for indirect sources — resolved at use time, never stored inline. + password, + database: Some(database), + file: None, + tls: None, + }; + (config, p_env, p_cmd, keychain) } DatabaseType::SQLite | DatabaseType::DuckDB => { let file: String = Input::new() @@ -1079,11 +1272,13 @@ async fn interactive_connect_wizard( .interact_text() .map_err(|e| PlenumError::invalid_input(format!("Input failed: {e}")))?; - if engine == DatabaseType::DuckDB { + let config = if engine == DatabaseType::DuckDB { ConnectionConfig::duckdb(PathBuf::from(file)) } else { ConnectionConfig::sqlite(PathBuf::from(file)) - } + }; + // File-based engines have no authentication credential. + (config, None, None, None) } }; @@ -1098,7 +1293,15 @@ async fn interactive_connect_wizard( let location = prompt_save_location()?; // Use None for project_path (will default to current directory) - Ok((name, None, config, location)) + Ok(ConnectArgs { + name, + project_path: None, + config, + password_env, + password_command, + keychain_entry, + location, + }) } /// Non-interactive connect (with CLI args) @@ -1260,7 +1463,15 @@ async fn non_interactive_connect( } }; - Ok((conn_name, project_path, config, password_env, password_command, keychain_entry, location)) + Ok(ConnectArgs { + name: conn_name, + project_path, + config, + password_env, + password_command, + keychain_entry, + location, + }) } /// Prompt user for save location @@ -1357,7 +1568,7 @@ async fn handle_introspect( // --diff-against-project-path overrides; otherwise fall back to the primary project path // (or current directory when neither is specified). let target_proj = diff_against_project_path.as_deref().or(project_path.as_deref()); - let target_config = match plenum::resolve_connection(target_proj, Some(&target_name)) { + let target_config = match resolve_connection_cli(target_proj, Some(&target_name)) { Ok((cfg, _)) => cfg, Err(e) => { let envelope = ErrorEnvelope::from_error(config.engine.as_str(), "introspect", &e); @@ -1787,10 +1998,25 @@ async fn handle_query( } #[allow(clippy::future_not_send)] -async fn handle_mcp() -> std::result::Result<(), i32> { +async fn handle_mcp( + project_path: Option, + name: Option, + dsn_env: Option, +) -> std::result::Result<(), i32> { // Phase 7: MCP server using manual JSON-RPC 2.0 implementation // Follows the proven pattern from reflex-search (no unstable rmcp dependency) - match plenum::mcp::serve().await { + // + // Bind the connection source at server registration so resolution is + // decoupled from the launcher's cwd (REF-297). + let binding = match plenum::mcp::McpBinding::new(project_path, name, dsn_env) { + Ok(binding) => binding, + Err(e) => { + eprintln!("MCP server error: {e}"); + return Err(1); + } + }; + + match plenum::mcp::serve(binding).await { Ok(()) => Ok(()), Err(e) => { // MCP server errors go to stderr (not stdout, which is for JSON-RPC) @@ -1862,6 +2088,23 @@ fn build_tls_config( /// This helper resolves a connection from config or builds one from CLI arguments. /// Precedence: Named connection at project path → CLI arguments only /// Returns a tuple of (`ConnectionConfig`, `is_readonly`). +/// Resolve a saved connection honoring an explicit `--project-path`. +/// +/// When a project path is given, the connection is resolved from that project's +/// own `.plenum/config.json` (merged with the global registry), matching the +/// documented `--project-path /other/project` semantics and the way +/// `plenum mcp --project-path` binds. When no project path is given, resolution +/// falls back to the current working directory's local config. +fn resolve_connection_cli( + project_path: Option<&str>, + name: Option<&str>, +) -> Result<(ConnectionConfig, bool)> { + match project_path { + Some(path) => plenum::resolve_connection_in_project(path, name), + None => plenum::resolve_connection(None, name), + } +} + fn build_connection_config( name: Option<&str>, project_path: Option<&str>, @@ -1885,7 +2128,7 @@ fn build_connection_config( let mut resolved_connection: Option<(ConnectionConfig, bool)> = if should_try_resolve { // Try to load connection from config - match plenum::resolve_connection(project_path, name) { + match resolve_connection_cli(project_path, name) { Ok(cfg_tuple) => Some(cfg_tuple), Err(_) if has_explicit_args => None, // Ignore error if explicit args provided as fallback Err(e) => return Err(e), // Propagate error if no fallback @@ -1987,3 +2230,90 @@ fn parse_engine(engine: &str) -> Result { ))), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn wizard_env_credential_stores_only_typed_name() { + // Capability test: the typed env var name is stored verbatim; no inline + // password value is produced, and nothing is read from the environment. + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Env("PGPASSWORD".to_string())); + assert_eq!(p_env.as_deref(), Some("PGPASSWORD")); + assert!(password.is_none(), "indirect source must not store an inline password"); + assert!(p_cmd.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_command_credential_stores_only_typed_command() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Command("op read secret".to_string())); + assert_eq!(p_cmd.as_deref(), Some("op read secret")); + assert!(password.is_none()); + assert!(p_env.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_keychain_credential_stores_only_typed_service_account() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Keychain { + service: "plenum-db".to_string(), + account: "app".to_string(), + }); + let entry = keychain.expect("keychain entry stored"); + assert_eq!(entry.service, "plenum-db"); + assert_eq!(entry.account, "app"); + assert!(password.is_none()); + assert!(p_env.is_none()); + assert!(p_cmd.is_none()); + } + + #[test] + fn wizard_plaintext_credential_stores_inline_password() { + let (password, p_env, p_cmd, keychain) = + wizard_credential_fields(WizardCredential::Plaintext("hunter2".to_string())); + assert_eq!(password.as_deref(), Some("hunter2")); + assert!(p_env.is_none()); + assert!(p_cmd.is_none()); + assert!(keychain.is_none()); + } + + #[test] + fn wizard_validate_env_missing_names_the_field_to_fix() { + // A missing env var must fail fast and name the exact variable — never + // suggest a discovered alternative. + let var = "PLENUM_WIZARD_TEST_MISSING_XYZ"; + // Ensure it is unset for the duration of this assertion. + assert!(std::env::var(var).is_err(), "test precondition: var must be unset"); + let err = validate_wizard_credential(&WizardCredential::Env(var.to_string())) + .expect_err("missing env var must fail validation"); + let msg = err.message(); + assert!(msg.contains(var), "error should name the missing variable, got {msg:?}"); + assert!(msg.contains("not set"), "error should explain it is unset, got {msg:?}"); + } + + #[test] + fn wizard_validate_env_present_passes() { + let var = "PLENUM_WIZARD_TEST_PRESENT_ABC"; + std::env::set_var(var, "resolved-secret"); + let result = validate_wizard_credential(&WizardCredential::Env(var.to_string())); + std::env::remove_var(var); + assert!(result.is_ok(), "present env var should pass validation"); + } + + #[test] + fn wizard_validate_env_empty_names_the_field_to_fix() { + let var = "PLENUM_WIZARD_TEST_EMPTY_DEF"; + std::env::set_var(var, ""); + let result = validate_wizard_credential(&WizardCredential::Env(var.to_string())); + std::env::remove_var(var); + let err = result.expect_err("empty env var must fail"); + let msg = err.message(); + assert!(msg.contains(var), "error should name the empty variable, got {msg:?}"); + assert!(msg.contains("empty"), "error should explain it is empty, got {msg:?}"); + } +} diff --git a/src/mcp.rs b/src/mcp.rs index 449395d..8d6b9a3 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -46,6 +46,7 @@ use serde_json::Value; use std::io::{self, BufRead, Write}; use std::path::PathBuf; +use crate::config::{ConfigLocation, KeychainEntry, StoredConnection}; use crate::{parse_dsn, redact_dsn, Capabilities, ConnectionConfig, DatabaseEngine, DatabaseType}; // Import database engines @@ -58,6 +59,57 @@ use crate::engine::postgres::PostgresEngine; #[cfg(feature = "sqlite")] use crate::engine::sqlite::SqliteEngine; +// ============================================================================ +// Connection Binding (captured at server registration) +// ============================================================================ + +/// Connection binding captured from `plenum mcp` flags at server registration. +/// +/// The MCP server is long-lived and its launcher's cwd is not a meaningful +/// signal (Claude Desktop, editors, and daemons start it from arbitrary +/// directories). This binding pins config resolution to an explicit source so +/// resolution never silently depends on cwd. +/// +/// Per-call tool arguments (`dsn`, `connection`, `engine`, ...) still take +/// precedence; the binding only supplies the default a call omits. +#[derive(Debug, Clone, Default)] +pub struct McpBinding { + /// `--project-path`: pins config resolution to this project path. + project_path: Option, + /// `--name`: pins to a named connection within the resolved project. + name: Option, + /// `--dsn-env`: reads the DSN from this named environment variable at + /// connection time. Only the named variable is ever read — never an ambient + /// fallback such as `DATABASE_URL` or `PGPASSWORD`. + dsn_env: Option, +} + +impl McpBinding { + /// Construct a binding from `plenum mcp` flags, rejecting conflicting + /// combinations. + /// + /// `--dsn-env` names a complete one-off connection source, so it cannot be + /// combined with the saved-config selectors `--project-path` / `--name`. + /// + /// # Errors + /// Returns an error when `--dsn-env` is combined with `--project-path` or + /// `--name`. + pub fn new( + project_path: Option, + name: Option, + dsn_env: Option, + ) -> Result { + if dsn_env.is_some() && (project_path.is_some() || name.is_some()) { + return Err(anyhow!( + "'--dsn-env' cannot be combined with '--project-path' or '--name': \ + a DSN environment variable is a complete connection source, \ + not a saved-config selector" + )); + } + Ok(Self { project_path, name, dsn_env }) + } +} + // ============================================================================ // JSON-RPC 2.0 Structures // ============================================================================ @@ -156,7 +208,7 @@ impl CallToolResult { /// /// Returns an error if stdio communication fails or if there's a fatal error. #[allow(clippy::future_not_send)] -pub async fn serve() -> Result<()> { +pub async fn serve(binding: McpBinding) -> Result<()> { let stdin = io::stdin(); let reader = stdin.lock(); let mut stdout = io::stdout(); @@ -183,7 +235,7 @@ pub async fn serve() -> Result<()> { continue; } - let response = handle_request(request).await; + let response = handle_request(request, &binding).await; let response_json = serde_json::to_string(&response)?; writeln!(stdout, "{response_json}")?; stdout.flush()?; @@ -195,11 +247,11 @@ pub async fn serve() -> Result<()> { /// Handle a JSON-RPC request /// /// Routes the request to the appropriate handler based on the method name. -async fn handle_request(request: JsonRpcRequest) -> JsonRpcResponse { +async fn handle_request(request: JsonRpcRequest, binding: &McpBinding) -> JsonRpcResponse { let result = match request.method.as_str() { "initialize" => handle_initialize(request.params), "tools/list" => handle_list_tools(), - "tools/call" => handle_call_tool(request.params).await, + "tools/call" => handle_call_tool(request.params, binding).await, _ => Err(anyhow!("Unknown method: {}", request.method)), }; @@ -292,6 +344,18 @@ fn handle_list_tools() -> Result { "type": "string", "description": "DISCOURAGED: SQLite/DuckDB database file path. Only for one-off sqlite/duckdb explicit connections or as override. Prefer using saved connections." }, + "password_env": { + "type": "string", + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with keychain_service/keychain_account." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain account name. Must be paired with keychain_service." + }, "list_databases": { "type": "boolean", "description": "Operation: List all databases. Returns {\"type\": \"database_list\", \"databases\": [\"db1\", \"db2\", ...]}. Requires wildcard connection (database=\"*\"). MySQL/Postgres only. Mutually exclusive with other operations." @@ -402,6 +466,18 @@ fn handle_list_tools() -> Result { "type": "string", "description": "DISCOURAGED: File path to SQLite/DuckDB database file. Only for one-off sqlite/duckdb explicit connections. Can be relative or absolute path. Example: './app.db', '/var/lib/data.duckdb'. Prefer using saved connections." }, + "password_env": { + "type": "string", + "description": "Credential reference for one-off explicit connections: name of the environment variable holding the password. The secret value never passes through Plenum. Combine with engine + host/port/user/database. Mutually exclusive with keychain_service/keychain_account." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain service name. Must be paired with keychain_account. Mutually exclusive with password_env." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference for one-off explicit connections: OS keychain account name. Must be paired with keychain_service." + }, "max_rows": { "type": "number", "description": "CRITICAL: Maximum number of rows to return from SELECT queries. Due to MCP's 25k token response limit, this parameter is effectively REQUIRED for all queries against tables of unknown size. Omitting this will cause tool failure on large tables. Start small and increase if needed: Use 10 for initial exploration/preview, 50-100 for small known tables, 100-500 for medium tables (only after confirming size with COUNT(*) query). Even with columnar format (30-50% token reduction), a 100-row result with 10+ columns can approach token limits. Always prefer smaller limits initially." @@ -433,7 +509,7 @@ fn handle_list_tools() -> Result { }, { "name": "connect", - "description": "Test a database connection and return server metadata (version, database name, user). Opens a connection, verifies liveness, returns ConnectionInfo, then disconnects immediately. Stateless and read-only — no config is saved or mutated. Use to health-check a saved connection before running queries, or to verify credentials after setup. IMPORTANT CONNECTION WORKFLOW: (1) RECOMMENDED: Auto-resolve (omit all connection params) — uses project's default saved connection, (2) COMMON: Named connection (use 'connection' param only) — references saved connection by name, (3) DISCOURAGED: Explicit credentials (engine + host/user/password) — ONLY for one-off checks. Possible error codes: CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", + "description": "Test a database connection OR save a connection config by reference. TWO MODES: (A) TEST (default, no 'save'): opens a connection, verifies liveness, returns ConnectionInfo, then disconnects. Stateless — no config mutated. (B) SAVE ('save': \"local\"|\"global\"): persists the connection config BY REFERENCE to .plenum/config.json (local) or ~/.config/plenum/connections.json (global), then returns a saved confirmation. No live connection required — provisions connections offline. CREDENTIAL SAFETY (non-negotiable): inline plaintext 'password' is REJECTED with CAPABILITY_VIOLATION. Source secrets ONLY by reference: 'password_env' (env var name) or 'keychain_service'+'keychain_account' (OS keychain). 'password_command' is REJECTED with CAPABILITY_VIOLATION because it executes a shell command. The plaintext secret NEVER passes through Plenum — only the reference string is stored. Use TEST mode to health-check a saved connection; use SAVE mode to register a new one. Possible error codes: CAPABILITY_VIOLATION (inline plaintext password or password_command), CONNECTION_FAILED (unreachable host, bad credentials, missing file), INVALID_INPUT (missing required params), CONFIG_ERROR (no saved connection found).", "inputSchema": { "type": "object", "properties": { @@ -469,6 +545,31 @@ fn handle_list_tools() -> Result { "file": { "type": "string", "description": "DISCOURAGED: SQLite/DuckDB database file path. Only for explicit one-off tests." + }, + "password_env": { + "type": "string", + "description": "Credential reference: name of the environment variable that holds the password (e.g. \"DB_PASSWORD\"). The reference string is stored/used, never the secret value. Mutually exclusive with keychain_service/keychain_account. Use this instead of inline 'password' (which is rejected)." + }, + "keychain_service": { + "type": "string", + "description": "Credential reference: OS keychain service name. Must be paired with keychain_account. The password is looked up from the platform keychain at connection time. Mutually exclusive with password_env." + }, + "keychain_account": { + "type": "string", + "description": "Credential reference: OS keychain account name. Must be paired with keychain_service." + }, + "save": { + "type": "string", + "enum": ["local", "global"], + "description": "SAVE MODE: persist this connection config by reference. \"local\" writes .plenum/config.json (team-shareable, per-project); \"global\" writes ~/.config/plenum/connections.json (per-user). Requires explicit 'engine' + connection params. Credentials are stored only as references (password_env/keychain_*) — never plaintext. Omit to run in TEST mode instead." + }, + "project_path": { + "type": "string", + "description": "SAVE MODE modifier: project path to key the saved connection under. Defaults to the server's bound --project-path, or the current working directory. Local saves are written to /.plenum/config.json." + }, + "name": { + "type": "string", + "description": "SAVE MODE modifier: name to store the connection under (e.g. \"prod\", \"staging\"). Defaults to \"default\". The first connection saved for a project becomes its default." } } } @@ -480,15 +581,26 @@ fn handle_list_tools() -> Result { /// Handle tools/call request /// /// Routes the tool call to the appropriate tool implementation. -async fn handle_call_tool(params: Option) -> Result { +async fn handle_call_tool(params: Option, binding: &McpBinding) -> Result { let params = params.ok_or_else(|| anyhow!("Missing params"))?; let name = params["name"].as_str().ok_or_else(|| anyhow!("Missing tool name"))?; let arguments = ¶ms["arguments"]; + // Capability boundary: `password_command` runs a shell command, so accepting + // it from tool arguments would let an agent execute arbitrary commands on the + // host. Reject it for every tool before anything runs. + if arguments.get("password_command").is_some() { + return Err(anyhow!( + "CAPABILITY_VIOLATION: 'password_command' is not permitted on MCP tools because \ + it executes a shell command. Reference the secret via 'password_env' or \ + 'keychain_service' + 'keychain_account'." + )); + } + match name { - "connect" => tool_connect(arguments).await, - "introspect" => tool_introspect(arguments).await, - "query" => tool_query(arguments).await, + "connect" => tool_connect(arguments, binding).await, + "introspect" => tool_introspect(arguments, binding).await, + "query" => tool_query(arguments, binding).await, _ => Err(anyhow!("Unknown tool: {name}")), } } @@ -499,10 +611,33 @@ async fn handle_call_tool(params: Option) -> Result { /// MCP Tool: connect /// -/// Tests a database connection and returns server metadata. -/// Stateless and read-only — no config is saved or mutated. -async fn tool_connect(args: &Value) -> Result { - let (config, _is_readonly) = resolve_connection_from_args(args)?; +/// Two modes: +/// - Without `save`: tests a database connection and returns server metadata. +/// Stateless — no config is mutated. +/// - With `save` (`"local"` | `"global"`): persists the connection config **by +/// reference** to `.plenum/config.json` or the global registry, then returns a +/// saved confirmation. No live connection is required, so agents can provision +/// connections offline. +/// +/// Inline plaintext passwords are rejected unconditionally: callers must source +/// secrets via `password_env` or `keychain_service` + `keychain_account`. This +/// guarantees the plaintext secret never passes through Plenum. +async fn tool_connect(args: &Value, binding: &McpBinding) -> Result { + // Capability boundary: the MCP connect tool never handles plaintext secrets. + if args.get("password").and_then(|v| v.as_str()).is_some() { + return Err(anyhow!( + "CAPABILITY_VIOLATION: inline plaintext 'password' is not permitted on the MCP \ + connect tool. Reference the secret instead via 'password_env' or \ + 'keychain_service' + 'keychain_account'." + )); + } + + // Save mode: persist by reference and return without opening a connection. + if let Some(save_raw) = args.get("save") { + return tool_connect_save(args, binding, save_raw); + } + + let (config, _is_readonly) = resolve_connection_from_args(args, binding)?; let connection_info = match config.engine { #[cfg(feature = "sqlite")] @@ -545,13 +680,84 @@ async fn tool_connect(args: &Value) -> Result { CallToolResult::success(connection_info) } +/// Persist a connection config by reference for the `connect` tool's save mode. +/// +/// The stored config carries no plaintext password (inline `password` was already +/// rejected by the caller); credentials are recorded only as references. +fn tool_connect_save(args: &Value, binding: &McpBinding, save_raw: &Value) -> Result { + let location = match save_raw.as_str() { + Some("local") => ConfigLocation::Local, + Some("global") => ConfigLocation::Global, + _ => { + return Err(anyhow!("Invalid 'save' value. Must be \"local\" or \"global\"")); + } + }; + + // Saving requires explicit connection parameters — you cannot re-save a + // connection selected purely by name, and a DSN would carry inline secrets. + let engine_str = args.get("engine").and_then(|v| v.as_str()).ok_or_else(|| { + anyhow!("'save' requires explicit connection params, starting with 'engine'") + })?; + + let (password_env, keychain_entry) = parse_credential_refs(args)?; + let has_ref = password_env.is_some() || keychain_entry.is_some(); + + // Build the config to store. `allow_missing_password` keeps the config free of + // any inline secret — the reference is the sole password authority. + let config = build_connection_config_from_args(args, engine_str, true)?; + + // Project path: explicit arg wins, else the server-registration binding, else cwd. + let project_path = match args.get("project_path").and_then(|v| v.as_str()) { + Some(p) => p.to_string(), + None => match &binding.project_path { + Some(p) => p.clone(), + None => { + crate::config::get_current_project_path().map_err(|e| anyhow!("{}", e.message()))? + } + }, + }; + + let name = args.get("name").and_then(|v| v.as_str()).map(String::from); + + crate::config::save_connection_in_project( + &project_path, + name.clone(), + config, + password_env.clone(), + None, + keychain_entry.clone(), + location, + ) + .map_err(|e| anyhow!("Failed to save connection: {}", e.message()))?; + + let credential_source = if password_env.is_some() { + "password_env" + } else if keychain_entry.is_some() { + "keychain" + } else { + "none" + }; + + CallToolResult::success(serde_json::json!({ + "saved": true, + "location": match location { + ConfigLocation::Local => "local", + ConfigLocation::Global => "global", + }, + "name": name.unwrap_or_else(|| "default".to_string()), + "project_path": project_path, + "credential_source": credential_source, + "has_reference": has_ref, + })) +} + /// MCP Tool: introspect /// /// Introspects database schema and returns table/column information. /// When `diff_against` is provided, computes a structural schema diff instead. -async fn tool_introspect(args: &Value) -> Result { +async fn tool_introspect(args: &Value, binding: &McpBinding) -> Result { // Resolve base connection config - let (config, _is_readonly) = resolve_connection_from_args(args)?; + let (config, _is_readonly) = resolve_connection_from_args(args, binding)?; // Get optional database and schema modifiers (shared by both paths) let database = args.get("target_database").and_then(|v| v.as_str()); @@ -703,12 +909,12 @@ fn parse_introspect_operation(args: &Value) -> Result Result { +async fn tool_query(args: &Value, binding: &McpBinding) -> Result { // Extract SQL let sql = args["sql"].as_str().ok_or_else(|| anyhow!("Missing required field: sql"))?; // Resolve connection config - let (mut config, _is_readonly) = resolve_connection_from_args(args)?; + let (mut config, _is_readonly) = resolve_connection_from_args(args, binding)?; // Apply target_database override if provided if let Some(target_db) = args.get("target_database").and_then(|v| v.as_str()) { @@ -769,8 +975,56 @@ async fn tool_query(args: &Value) -> Result { // Helper Functions (Stateless) // ============================================================================ -/// Build `ConnectionConfig` from JSON arguments -fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result { +/// Extract credential-reference arguments from a tool call. +/// +/// Recognizes `password_env` and the keychain pair `keychain_service` + +/// `keychain_account`. (`password_command` is rejected earlier, in +/// `handle_call_tool`, because it executes a shell command.) These name where +/// a secret lives; the plaintext secret itself never passes through Plenum. +/// +/// Enforces the same invariants the config layer does: +/// - at most one credential source may be specified; +/// - `keychain_service` and `keychain_account` must be supplied together. +/// +/// Returns `(password_env, keychain_entry)` — both `None` when no reference +/// was provided. +fn parse_credential_refs(args: &Value) -> Result<(Option, Option)> { + let password_env = args.get("password_env").and_then(|v| v.as_str()).map(String::from); + let service = args.get("keychain_service").and_then(|v| v.as_str()).map(String::from); + let account = args.get("keychain_account").and_then(|v| v.as_str()).map(String::from); + + let keychain_entry = match (service, account) { + (Some(service), Some(account)) => Some(KeychainEntry { service, account }), + (None, None) => None, + _ => { + return Err(anyhow!( + "'keychain_service' and 'keychain_account' must be provided together" + )); + } + }; + + if password_env.is_some() && keychain_entry.is_some() { + return Err(anyhow!( + "Only one credential reference is allowed: \ + password_env or keychain_service/keychain_account" + )); + } + + Ok((password_env, keychain_entry)) +} + +/// Build `ConnectionConfig` from JSON arguments. +/// +/// When `allow_missing_password` is true the inline `password` field is +/// optional — the caller is expected to supply a credential reference +/// (`password_env` / `keychain_service`+`keychain_account`) +/// that resolves the secret at connection time. The resulting config carries +/// `password: None` so the reference is the sole authority. +fn build_connection_config_from_args( + args: &Value, + engine_str: &str, + allow_missing_password: bool, +) -> Result { let engine_type = match engine_str { "postgres" => DatabaseType::Postgres, "mysql" => DatabaseType::MySQL, @@ -793,20 +1047,30 @@ fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result { let file_str = args["file"] @@ -823,16 +1087,21 @@ fn build_connection_config_from_args(args: &Value, engine_str: &str) -> Result Result<(ConnectionConfig, bool)> { +fn resolve_connection_from_args( + args: &Value, + binding: &McpBinding, +) -> Result<(ConnectionConfig, bool)> { // Scenario 0: DSN one-off URL (mutually exclusive with connection and engine) if let Some(dsn_str) = args.get("dsn").and_then(|v| v.as_str()) { if args.get("connection").and_then(|v| v.as_str()).is_some() { @@ -853,9 +1122,14 @@ fn resolve_connection_from_args(args: &Value) -> Result<(ConnectionConfig, bool) if has_connection { let connection = args["connection"].as_str().unwrap(); - // Use None for project_path (defaults to current directory) - let (mut config, is_readonly) = crate::resolve_connection(None, Some(connection)) - .map_err(|e| anyhow!("Failed to resolve connection '{connection}': {e}"))?; + // When --project-path is bound, look up the named connection inside that + // project's local config (not CWD). Without a bound path, fall back to CWD. + let (mut config, is_readonly) = if let Some(path) = &binding.project_path { + crate::config::resolve_connection_in_project(path, Some(connection)) + } else { + crate::resolve_connection(None, Some(connection)) + } + .map_err(|e| anyhow!("Failed to resolve connection '{connection}': {e}"))?; // Apply overrides if let Some(eng) = args.get("engine").and_then(|v| v.as_str()) { @@ -892,20 +1166,82 @@ fn resolve_connection_from_args(args: &Value) -> Result<(ConnectionConfig, bool) // Scenario 2: Explicit connection parameters if has_engine { let engine_str = args["engine"].as_str().unwrap(); - let config = build_connection_config_from_args(args, engine_str)?; + let (password_env, keychain_entry) = parse_credential_refs(args)?; + let has_ref = password_env.is_some() || keychain_entry.is_some(); + + let config = build_connection_config_from_args(args, engine_str, has_ref)?; + + // A credential reference sources the secret at connection time; resolve it + // now so the one-off connection carries the real password. The plaintext + // never came through Plenum — only the reference string did. + if has_ref { + let stored = StoredConnection { + config, + password_env, + password_command: None, + keychain_entry, + readonly: None, + }; + let (resolved, is_readonly) = + stored.resolve().map_err(|e| anyhow!("{}", e.message()))?; + return Ok((resolved, is_readonly)); + } + return Ok((config, false)); // Explicit connections are never readonly } - // Scenario 3: Auto-resolve default connection for current project - // Use None for both project_path (current directory) and connection_name (use default) - let (config, is_readonly) = crate::resolve_connection(None, None).map_err(|e| { - anyhow!( - "No connection or engine specified, and failed to auto-resolve default connection: {e}. \ - Either provide 'connection' (named), 'engine' (explicit), or ensure a default connection exists for this project." - ) - })?; + // Scenario 3: Fall back to the server-registration binding. + resolve_from_binding(binding) +} - Ok((config, is_readonly)) +/// Resolve a connection from the server-registration binding alone (no per-call +/// connection selectors were provided). +/// +/// Priority: `--dsn-env` (explicit env-sourced DSN) → `--project-path` / `--name` +/// (or the cwd default when neither is bound). When nothing resolves and no +/// binding flags were given, returns a structured error naming exactly which +/// flags to add — never a silent fallback. +fn resolve_from_binding(binding: &McpBinding) -> Result<(ConnectionConfig, bool)> { + // `--dsn-env`: read the DSN from ONLY the named variable. Plenum never reads + // an ambient credential source (DATABASE_URL, PGPASSWORD, ...) that the + // caller did not explicitly name. + if let Some(var) = &binding.dsn_env { + let dsn = std::env::var(var).map_err(|_| { + anyhow!( + "--dsn-env variable '{var}' is not set in the environment. \ + Plenum reads only the named variable and never falls back to \ + other environment variables." + ) + })?; + let config = + parse_dsn(&dsn).map_err(|e| anyhow!("{} (from --dsn-env {})", e.message(), var))?; + return Ok((config, false)); + } + + // `--project-path`: load the local config from the project directory itself, + // not from the launcher's cwd — that's the whole point of the flag. + if let Some(path) = &binding.project_path { + return crate::config::resolve_connection_in_project(path, binding.name.as_deref()) + .map_err(|e| anyhow!("Failed to resolve bound connection: {e}")); + } + + // `--name` only (no --project-path): use cwd as project, pick the named connection. + // No flags at all: use cwd + project default. + crate::resolve_connection(None, binding.name.as_deref()).map_err(|e| { + if binding.name.is_none() { + // Nothing was bound and cwd has no usable config — name the flags exactly. + anyhow!( + "No connection could be resolved: no connection selectors were passed to this \ + tool call and 'plenum mcp' was started without a connection binding. \ + Restart the server with one of: '--project-path ' to pin the project, \ + '--name ' to select a saved connection, or \ + '--dsn-env ' to read a DSN from a named environment variable. \ + Underlying error: {e}" + ) + } else { + anyhow!("Failed to resolve bound connection: {e}") + } + }) } /// Validate database connection @@ -999,3 +1335,239 @@ async fn execute_query( } } } + +// ============================================================================ +// Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + + // ── McpBinding construction / conflict rules ──────────────────────────── + + #[test] + fn binding_rejects_dsn_env_with_project_path() { + let err = McpBinding::new(Some("/p".to_string()), None, Some("PLENUM_DSN".to_string())) + .unwrap_err() + .to_string(); + assert!(err.contains("--dsn-env"), "error should name --dsn-env: {err}"); + assert!(err.contains("--project-path"), "error should name --project-path: {err}"); + } + + #[test] + fn binding_rejects_dsn_env_with_name() { + let err = McpBinding::new(None, Some("prod".to_string()), Some("PLENUM_DSN".to_string())) + .unwrap_err() + .to_string(); + assert!(err.contains("--dsn-env"), "error should name --dsn-env: {err}"); + assert!(err.contains("--name"), "error should name --name: {err}"); + } + + #[test] + fn binding_allows_project_path_and_name_together() { + let binding = + McpBinding::new(Some("/p".to_string()), Some("prod".to_string()), None).unwrap(); + assert_eq!(binding.project_path.as_deref(), Some("/p")); + assert_eq!(binding.name.as_deref(), Some("prod")); + } + + #[test] + fn binding_allows_dsn_env_alone_and_empty() { + assert!(McpBinding::new(None, None, Some("PLENUM_DSN".to_string())).is_ok()); + assert!(McpBinding::default().dsn_env.is_none()); + } + + // ── --dsn-env resolution (credential-sourcing safety) ─────────────────── + + #[test] + fn dsn_env_reads_only_the_named_variable() { + // A named var holding a valid DSN resolves; nothing else is consulted. + std::env::set_var("PLENUM_TEST_DSN_ENV_OK", "sqlite::memory:"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_OK".to_string())).unwrap(); + let (config, is_readonly) = resolve_from_binding(&binding).unwrap(); + assert_eq!(config.engine, DatabaseType::SQLite); + assert!(!is_readonly); + std::env::remove_var("PLENUM_TEST_DSN_ENV_OK"); + } + + #[test] + fn dsn_env_never_falls_back_to_ambient_credentials() { + // Set a plausible ambient credential source, then point --dsn-env at a + // DIFFERENT, unset variable. Plenum must NOT pick up the ambient one. + std::env::set_var("PLENUM_TEST_AMBIENT_DSN", "postgres://u:p@host:5432/db"); + std::env::remove_var("PLENUM_TEST_DSN_ENV_UNSET"); + + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_UNSET".to_string())).unwrap(); + let err = resolve_from_binding(&binding).unwrap_err().to_string(); + + assert!( + err.contains("PLENUM_TEST_DSN_ENV_UNSET"), + "error should name the missing var: {err}" + ); + assert!( + !err.contains("PLENUM_TEST_AMBIENT_DSN"), + "must not reference the ambient var: {err}" + ); + assert!(!err.contains("host"), "must not leak the ambient DSN value: {err}"); + std::env::remove_var("PLENUM_TEST_AMBIENT_DSN"); + } + + #[test] + fn dsn_env_with_invalid_dsn_is_rejected() { + std::env::set_var("PLENUM_TEST_DSN_ENV_BAD", "not-a-valid-dsn"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_BAD".to_string())).unwrap(); + let err = resolve_from_binding(&binding).unwrap_err().to_string(); + assert!(err.contains("PLENUM_TEST_DSN_ENV_BAD"), "error names the var: {err}"); + std::env::remove_var("PLENUM_TEST_DSN_ENV_BAD"); + } + + // ── Per-call args still take precedence over the binding ──────────────── + + #[test] + fn per_call_dsn_overrides_binding_and_is_used_directly() { + // Even with a --dsn-env binding, an explicit per-call `dsn` wins. + std::env::set_var("PLENUM_TEST_DSN_ENV_BINDING", "sqlite::memory:"); + let binding = + McpBinding::new(None, None, Some("PLENUM_TEST_DSN_ENV_BINDING".to_string())).unwrap(); + let args = serde_json::json!({ "dsn": "sqlite:/tmp/explicit.db" }); + let (config, _) = resolve_connection_from_args(&args, &binding).unwrap(); + assert_eq!(config.engine, DatabaseType::SQLite); + assert_eq!(config.file.as_deref(), Some(std::path::Path::new("/tmp/explicit.db"))); + std::env::remove_var("PLENUM_TEST_DSN_ENV_BINDING"); + } + + #[test] + fn per_call_dsn_and_connection_are_mutually_exclusive() { + let args = serde_json::json!({ "dsn": "sqlite::memory:", "connection": "prod" }); + let err = + resolve_connection_from_args(&args, &McpBinding::default()).unwrap_err().to_string(); + assert!(err.contains("mutually exclusive"), "got: {err}"); + } + + // ── Credential-reference parsing (REF-298) ────────────────────────────── + + #[test] + fn parse_credential_refs_none_when_absent() { + let (env, kc) = parse_credential_refs(&serde_json::json!({})).unwrap(); + assert!(env.is_none() && kc.is_none()); + } + + #[test] + fn parse_credential_refs_env() { + let (env, kc) = + parse_credential_refs(&serde_json::json!({ "password_env": "DB_PASSWORD" })).unwrap(); + assert_eq!(env.as_deref(), Some("DB_PASSWORD")); + assert!(kc.is_none()); + } + + #[test] + fn parse_credential_refs_keychain_requires_both() { + let err = parse_credential_refs(&serde_json::json!({ "keychain_service": "svc" })) + .unwrap_err() + .to_string(); + assert!(err.contains("keychain_service") && err.contains("keychain_account"), "got: {err}"); + } + + #[test] + fn parse_credential_refs_rejects_multiple_sources() { + let args = serde_json::json!({ + "password_env": "A", + "keychain_service": "svc", + "keychain_account": "acct" + }); + let err = parse_credential_refs(&args).unwrap_err().to_string(); + assert!(err.contains("Only one credential reference"), "got: {err}"); + } + + // ── One-off explicit connection sourced from a credential reference ────── + + #[test] + fn one_off_explicit_resolves_password_from_env_reference() { + // The reference string is passed; Plenum reads the secret from the env + // var at resolution time. No plaintext ever appeared in the args. + std::env::set_var("PLENUM_TEST_ONEOFF_PWD", "s3cret-from-env"); + let args = serde_json::json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "PLENUM_TEST_ONEOFF_PWD" + }); + let (config, _ro) = resolve_connection_from_args(&args, &McpBinding::default()).unwrap(); + assert_eq!(config.engine, DatabaseType::Postgres); + assert_eq!(config.password.as_deref(), Some("s3cret-from-env")); + std::env::remove_var("PLENUM_TEST_ONEOFF_PWD"); + } + + #[test] + fn one_off_explicit_missing_env_reference_is_error() { + std::env::remove_var("PLENUM_TEST_ONEOFF_MISSING"); + let args = serde_json::json!({ + "engine": "postgres", + "host": "h", "port": 5432, "user": "u", "database": "d", + "password_env": "PLENUM_TEST_ONEOFF_MISSING" + }); + let err = + resolve_connection_from_args(&args, &McpBinding::default()).unwrap_err().to_string(); + assert!(err.contains("PLENUM_TEST_ONEOFF_MISSING"), "names missing var: {err}"); + } + + // ── connect save mode persists by reference (no plaintext) ────────────── + + #[test] + fn connect_save_local_writes_reference_no_plaintext() { + let project = + std::env::temp_dir().join(format!("plenum-mcp-save-{}-{:p}", std::process::id(), &0u8)); + let _ = std::fs::remove_dir_all(&project); + std::fs::create_dir_all(&project).unwrap(); + + let args = serde_json::json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "DB_PASSWORD", + "save": "local", + "name": "prod", + "project_path": project.to_str().unwrap(), + }); + + let result = tool_connect_save(&args, &McpBinding::default(), &args["save"]).unwrap(); + // Tool result envelope wraps a JSON text block; assert the confirmation shape. + let text = result["content"][0]["text"].as_str().unwrap(); + assert!(text.contains("\"saved\": true"), "confirmation: {text}"); + assert!(text.contains("\"location\": \"local\""), "location: {text}"); + + let written = std::fs::read_to_string(project.join(".plenum").join("config.json")).unwrap(); + assert!(written.contains("password_env"), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD")); + assert!(!written.contains("\"password\":"), "no plaintext: {written}"); + assert!(written.contains("prod")); + + let _ = std::fs::remove_dir_all(&project); + } + + #[test] + fn connect_save_rejects_missing_engine() { + let args = serde_json::json!({ "save": "local", "connection": "prod" }); + let err = tool_connect_save(&args, &McpBinding::default(), &args["save"]) + .unwrap_err() + .to_string(); + assert!(err.contains("engine"), "got: {err}"); + } + + #[test] + fn connect_save_rejects_invalid_location() { + let args = serde_json::json!({ "save": "cloud", "engine": "sqlite", "file": "/tmp/x.db" }); + let err = tool_connect_save(&args, &McpBinding::default(), &args["save"]) + .unwrap_err() + .to_string(); + assert!(err.contains("local") && err.contains("global"), "got: {err}"); + } +} diff --git a/src/output.rs b/src/output.rs index 60a11a7..79fb1d7 100644 --- a/src/output.rs +++ b/src/output.rs @@ -9,6 +9,8 @@ //! //! Output is stable, versioned, and suitable for programmatic parsing by agents. +use std::collections::BTreeMap; + use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -95,6 +97,88 @@ impl ErrorEnvelope { } } +/// A single MCP server entry within an [`McpStanza`]. +/// +/// Mirrors the shape MCP hosts expect under `mcpServers.`: an executable +/// plus the exact argv used to launch Plenum's MCP server for the saved connection. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct McpServerConfig { + /// Executable to launch (always `"plenum"`). + pub command: String, + + /// Arguments passed to the executable. Reflects the W1 launch flags + /// (`mcp --project-path --name `) for the connection just saved. + pub args: Vec, +} + +/// Ready-to-paste `mcpServers` configuration block emitted after `plenum connect` +/// saves a connection. +/// +/// The agent operator can copy this verbatim into an MCP host config (e.g. +/// `claude_desktop_config.json`) to wire up the Plenum MCP server for the saved +/// connection without consulting setup documentation. +/// +/// Only the connection's project path and name are encoded — never a credential +/// or secret. Indirect credential sources remain resolved at connection time by +/// the stored connection reference. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct McpStanza { + /// Map of MCP server name to launch config. Always contains a single + /// `"plenum"` entry. + #[serde(rename = "mcpServers")] + pub mcp_servers: BTreeMap, +} + +impl McpStanza { + /// Build the stanza for a connection that was just saved under + /// `project_path` with the given `name`. + /// + /// The MCP server binds resolution explicitly to `--project-path`/`--name` + /// (never inferring from cwd), so the emitted argv reproduces exactly that + /// binding. + #[must_use] + pub fn for_saved_connection(project_path: &str, name: &str) -> Self { + let server = McpServerConfig { + command: "plenum".to_string(), + args: vec![ + "mcp".to_string(), + "--project-path".to_string(), + project_path.to_string(), + "--name".to_string(), + name.to_string(), + ], + }; + let mut mcp_servers = BTreeMap::new(); + mcp_servers.insert("plenum".to_string(), server); + Self { mcp_servers } + } +} + +/// Success `data` payload emitted by `plenum connect --save` after a connection +/// is persisted. +/// +/// Typing this envelope — rather than assembling it inline via +/// `serde_json::json!` at the call site — keeps the emitted shape, including the +/// nested [`McpStanza`], under `generate-schemas` + `tests/schema_drift.rs` +/// coverage so the stanza cannot drift without a failing test. +#[derive(Debug, Clone, Serialize, JsonSchema)] +pub struct ConnectSaveResult { + /// Name the connection was saved under. + pub connection_name: String, + + /// Database engine of the saved connection (postgres, mysql, sqlite). + pub engine: String, + + /// Where the connection was persisted: `"local"` or `"global"`. + pub saved_to: String, + + /// Human-readable confirmation message. + pub message: String, + + /// Ready-to-paste MCP server stanza for the saved connection. + pub mcp_stanza: McpStanza, +} + /// Error information structure #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct ErrorInfo { @@ -289,6 +373,59 @@ mod tests { assert!(!envelope.ok); } + #[test] + fn test_mcp_stanza_shape_reflects_project_path_and_name() { + let stanza = McpStanza::for_saved_connection("/home/user/project1", "staging"); + let json = serde_json::to_value(&stanza).unwrap(); + + // Ready-to-paste block is keyed by `mcpServers` -> `plenum`. + let server = &json["mcpServers"]["plenum"]; + assert_eq!(server["command"], "plenum"); + assert_eq!( + server["args"], + serde_json::json!([ + "mcp", + "--project-path", + "/home/user/project1", + "--name", + "staging" + ]) + ); + } + + #[test] + fn test_mcp_stanza_encodes_only_path_and_name_no_secret() { + // The stanza must never carry a credential — only the connection reference. + let stanza = McpStanza::for_saved_connection("/tmp/proj", "prod"); + let json = serde_json::to_string(&stanza).unwrap(); + assert!(!json.contains("password")); + assert!(!json.contains("dsn")); + // Single server entry named "plenum". + assert_eq!(stanza.mcp_servers.len(), 1); + assert!(stanza.mcp_servers.contains_key("plenum")); + } + + #[test] + fn test_connect_save_result_shape() { + let data = ConnectSaveResult { + connection_name: "staging".to_string(), + engine: "postgres".to_string(), + saved_to: "local".to_string(), + message: "Connection 'staging' saved successfully".to_string(), + mcp_stanza: McpStanza::for_saved_connection("/home/user/project1", "staging"), + }; + let json = serde_json::to_value(&data).unwrap(); + + assert_eq!(json["connection_name"], "staging"); + assert_eq!(json["engine"], "postgres"); + assert_eq!(json["saved_to"], "local"); + assert_eq!(json["message"], "Connection 'staging' saved successfully"); + // Nested stanza is carried through under the same `mcp_stanza` key the + // inline `serde_json::json!` builder used, keyed by `mcpServers` -> `plenum`. + assert_eq!(json["mcp_stanza"]["mcpServers"]["plenum"]["command"], "plenum"); + assert!(!json.to_string().contains("password")); + } + #[test] fn test_contract_version_constant() { assert_eq!(CONTRACT_VERSION, "1"); diff --git a/tests/cli_connect.rs b/tests/cli_connect.rs index f0cea98..1663c5e 100644 --- a/tests/cli_connect.rs +++ b/tests/cli_connect.rs @@ -194,6 +194,133 @@ fn connect_rejects_password_and_password_env_together() { cleanup(&dir); } +#[test] +fn connect_save_emits_ready_to_paste_mcp_stanza() { + let dir = unique_tmp_dir("stanza"); + let db_path = create_sqlite_db(&dir); + let canonical = std::fs::canonicalize(&dir).expect("canonicalize scratch dir"); + let canonical = canonical.to_str().expect("utf-8 path").to_string(); + + let (code, stdout, _stderr) = run_connect( + &dir, + &[], + &[ + "--name", + "staging", + "--engine", + "sqlite", + "--file", + db_path.to_str().unwrap(), + "--save", + "local", + ], + ); + + assert_eq!(code, 0, "expected success, stdout={stdout}"); + + let envelope: Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("non-JSON stdout {stdout:?}: {e}")); + assert_eq!(envelope.get("ok").and_then(Value::as_bool), Some(true)); + + // A ready-to-paste mcpServers block must be present in the connect data. + let server = envelope + .pointer("/data/mcp_stanza/mcpServers/plenum") + .expect("mcp_stanza.mcpServers.plenum present in connect output"); + assert_eq!(server.get("command").and_then(Value::as_str), Some("plenum")); + + let args: Vec = server + .get("args") + .and_then(Value::as_array) + .expect("args array present") + .iter() + .map(|v| v.as_str().unwrap_or_default().to_string()) + .collect(); + + // Reflects the W1 launch flags for the connection just saved. + assert_eq!( + args, + vec![ + "mcp".to_string(), + "--project-path".to_string(), + canonical, + "--name".to_string(), + "staging".to_string(), + ], + "stanza args should reproduce the exact --project-path/--name binding" + ); + + cleanup(&dir); +} + +#[test] +fn connect_save_local_with_explicit_project_path_persists_to_that_project() { + // Regression: `connect --project-path X --save local` run from a DIFFERENT + // cwd must persist the connection under X's own `.plenum/config.json`, not + // silently drop it. The local config format is single-project and keyed by + // its own directory, so a save that keys the entry under X while writing the + // file under cwd loses the connection entirely (save reports ok, nothing + // persisted). The save must target /.plenum/config.json. + let cwd = unique_tmp_dir("proj_cwd"); + let project = unique_tmp_dir("proj_target"); + let db_path = create_sqlite_db(&project); + + let (code, stdout, _stderr) = run_connect( + &cwd, + &[], + &[ + "--name", + "smoke", + "--project-path", + project.to_str().unwrap(), + "--engine", + "sqlite", + "--file", + db_path.to_str().unwrap(), + "--save", + "local", + ], + ); + + assert_eq!(code, 0, "expected success, stdout={stdout}"); + let envelope: Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("non-JSON stdout {stdout:?}: {e}")); + assert_eq!(envelope.get("ok").and_then(Value::as_bool), Some(true)); + + // The connection must land in the TARGET project's local config, keyed under + // its own directory (single-project local format). + let target_cfg = project.join(".plenum").join("config.json"); + let contents = std::fs::read_to_string(&target_cfg).unwrap_or_else(|e| { + panic!( + "connection must persist to /.plenum/config.json ({target_cfg:?}): {e}" + ) + }); + let parsed: Value = serde_json::from_str(&contents).expect("local config is valid JSON"); + assert!( + parsed.pointer("/connections/smoke").is_some(), + "saved connection 'smoke' must be present in target config, got {contents}" + ); + + // It must NOT have been written under the unrelated cwd. + assert!( + !cwd.join(".plenum").join("config.json").exists(), + "connection must not leak into the working directory's config" + ); + + // And it must be resolvable via the same explicit project path. + let (test_code, test_stdout, _) = run_connect( + &cwd, + &[], + &["--test", "--project-path", project.to_str().unwrap(), "--name", "smoke"], + ); + assert_eq!( + test_code, 0, + "saved connection must resolve by project-path+name, stdout={test_stdout}" + ); + + cleanup(&cwd); + cleanup(&project); +} + // ============================================================================ // --test (connection ping) tests // ============================================================================ diff --git a/tests/mcp_bound_connection.rs b/tests/mcp_bound_connection.rs new file mode 100644 index 0000000..53d0859 --- /dev/null +++ b/tests/mcp_bound_connection.rs @@ -0,0 +1,605 @@ +//! End-to-end coverage for the `plenum mcp` connection-binding flags (REF-297): +//! `--project-path`, `--name`, and `--dsn-env`. +//! +//! The MCP server is decoupled from the launcher's cwd: the binding captured at +//! `plenum mcp` startup supplies the default connection source for tool calls +//! that omit their own connection selectors. Every test drives the compiled +//! `plenum mcp` binary over stdio. +//! +//! Contract notes exercised here: +//! - Errors from a tool call surface as JSON-RPC error responses (the server's +//! existing error surface); the `error.message` is what callers read. +//! - `--dsn-env` reads the DSN at connection time and consults ONLY the named +//! variable — never an ambient source such as `DATABASE_URL`. +//! - `--project-path` re-keys config resolution; global config is keyed by +//! project path, so a global connection resolves regardless of cwd. + +use serde_json::{json, Value}; +use std::io::{BufRead, BufReader, Write}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +const TIMEOUT: Duration = Duration::from_secs(10); + +// ─── helpers ──────────────────────────────────────────────────────────────── + +/// Create a unique, empty scratch directory with no `.plenum` config. +fn scratch(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("plenum-bind-{}-{tag}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create scratch dir"); + dir +} + +/// Spawn `plenum mcp ` with the given env overrides and cwd. +/// +/// `xdg_config_home` isolates global-config lookup to a per-test directory so +/// the developer's real `~/.config/plenum` never influences the result. +fn spawn_mcp( + extra_args: &[&str], + envs: &[(&str, &str)], + cwd: &Path, + xdg_config_home: &Path, +) -> Child { + let bin = env!("CARGO_BIN_EXE_plenum"); + let mut cmd = Command::new(bin); + cmd.arg("mcp").args(extra_args); + cmd.env("XDG_CONFIG_HOME", xdg_config_home); + for (k, v) in envs { + cmd.env(k, v); + } + cmd.current_dir(cwd) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn plenum mcp") +} + +fn read_line_json(reader: &mut BufReader) -> Option { + let deadline = Instant::now() + TIMEOUT; + while Instant::now() < deadline { + let mut buf = String::new(); + match reader.read_line(&mut buf) { + Ok(0) | Err(_) => return None, + Ok(_) => { + let t = buf.trim(); + if t.is_empty() { + continue; + } + return serde_json::from_str(t).ok(); + } + } + } + None +} + +/// Drive the handshake and a single `connect` tool call (no connection args), +/// returning the JSON-RPC response for the call. +fn handshake_then_connect(child: &mut Child) -> Value { + let mut stdin = child.stdin.take().expect("stdin"); + let stdout = child.stdout.take().expect("stdout"); + let mut reader = BufReader::new(stdout); + + let init = json!({ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { "protocolVersion": "2024-11-05", "capabilities": {}, + "clientInfo": { "name": "test", "version": "0" } } + }); + let notif = json!({ "jsonrpc": "2.0", "method": "notifications/initialized" }); + let call = json!({ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": { "name": "connect", "arguments": {} } + }); + + writeln!(stdin, "{init}").unwrap(); + writeln!(stdin, "{notif}").unwrap(); + writeln!(stdin, "{call}").unwrap(); + stdin.flush().unwrap(); + drop(stdin); + + let mut response = None; + while let Some(v) = read_line_json(&mut reader) { + if v.get("id") == Some(&json!(2)) { + response = Some(v); + break; + } + } + response.expect("no response for connect tool call") +} + +/// Drive the handshake and a single tool call with explicit `arguments`, +/// returning the JSON-RPC response for the call. +fn handshake_then_call(child: &mut Child, tool: &str, arguments: &Value) -> Value { + let mut stdin = child.stdin.take().expect("stdin"); + let stdout = child.stdout.take().expect("stdout"); + let mut reader = BufReader::new(stdout); + + let init = json!({ + "jsonrpc": "2.0", "id": 1, "method": "initialize", + "params": { "protocolVersion": "2024-11-05", "capabilities": {}, + "clientInfo": { "name": "test", "version": "0" } } + }); + let notif = json!({ "jsonrpc": "2.0", "method": "notifications/initialized" }); + let call = json!({ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": { "name": tool, "arguments": arguments.clone() } + }); + + writeln!(stdin, "{init}").unwrap(); + writeln!(stdin, "{notif}").unwrap(); + writeln!(stdin, "{call}").unwrap(); + stdin.flush().unwrap(); + drop(stdin); + + let mut response = None; + while let Some(v) = read_line_json(&mut reader) { + if v.get("id") == Some(&json!(2)) { + response = Some(v); + break; + } + } + response.expect("no response for tool call") +} + +/// Extract the JSON-decoded text payload from a successful tool result. +fn tool_result_json(resp: &Value) -> Value { + let text = resp["result"]["content"][0]["text"] + .as_str() + .unwrap_or_else(|| panic!("expected a tool result text block, got: {resp}")); + serde_json::from_str(text).expect("tool result text is valid JSON") +} + +/// Create an on-disk `SQLite` database at `path`. Plenum opens connections +/// read-only, so the file must already exist for `connect` to succeed. +fn create_sqlite_db(path: &Path) { + let conn = rusqlite::Connection::open(path).expect("create sqlite db"); + conn.execute_batch("CREATE TABLE IF NOT EXISTS probe (id INTEGER PRIMARY KEY)") + .expect("seed sqlite db"); +} + +/// Write a global config registry (`{ "projects": { : {...} } }`) into +/// `/plenum/connections.json`. +fn write_global_config(xdg: &Path, registry: &Value) { + let dir = xdg.join("plenum"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("connections.json"), registry.to_string()).unwrap(); +} + +fn error_message(resp: &Value) -> String { + resp["error"]["message"] + .as_str() + .unwrap_or_else(|| panic!("expected a JSON-RPC error response, got: {resp}")) + .to_string() +} + +fn assert_no_error(resp: &Value) { + assert!( + resp.get("error").is_none(), + "expected a successful tool result, got JSON-RPC error: {:?}", + resp.get("error") + ); +} + +// ─── Criterion 4: no binding + no config → structured error naming the flags ── + +#[test] +fn no_binding_and_no_config_names_the_required_flags() { + let cwd = scratch("noflags"); + let xdg = scratch("noflags-xdg"); // empty → no global config + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("--project-path"), "must name --project-path: {msg}"); + assert!(msg.contains("--name"), "must name --name: {msg}"); + assert!(msg.contains("--dsn-env"), "must name --dsn-env: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Credential safety: no ambient pickup of DATABASE_URL ───────────────────── + +#[test] +fn database_url_is_never_used_as_an_ambient_source() { + let cwd = scratch("ambient"); + let xdg = scratch("ambient-xdg"); + // A valid ambient DSN is present but no --dsn-env flag names it. + let mut child = spawn_mcp(&[], &[("DATABASE_URL", "sqlite::memory:")], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + // Must fail (not silently resolve from DATABASE_URL), and the error must not + // reference the ambient variable it was never told to read. + let msg = error_message(&resp); + assert!(!msg.contains("DATABASE_URL"), "must not reference the ambient var: {msg}"); + assert!(msg.contains("--dsn-env"), "should point the operator at the flags: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 3: --dsn-env resolves the DSN from ONLY the named variable ───── + +#[test] +fn dsn_env_resolves_connection_from_named_variable() { + let cwd = scratch("dsnenv-ok"); + let xdg = scratch("dsnenv-ok-xdg"); + let db = cwd.join("app.db"); + create_sqlite_db(&db); + let dsn = format!("sqlite:{}", db.display()); + + let mut child = + spawn_mcp(&["--dsn-env", "PLENUM_ITEST_DSN"], &[("PLENUM_ITEST_DSN", &dsn)], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + let content = resp["result"]["content"][0]["text"].as_str().unwrap_or(""); + assert!(!content.is_empty(), "expected ConnectionInfo content, got empty: {resp}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn dsn_env_unset_variable_errors_and_names_the_variable() { + let cwd = scratch("dsnenv-unset"); + let xdg = scratch("dsnenv-unset-xdg"); + // The var is deliberately not provided to the child. + let mut child = spawn_mcp(&["--dsn-env", "PLENUM_ITEST_UNSET"], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("PLENUM_ITEST_UNSET"), "error must name the missing var: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 1: --project-path pins resolution regardless of launcher cwd ─── + +#[test] +fn project_path_pins_resolution_regardless_of_cwd() { + let cwd = scratch("projpath-cwd"); // empty: no local config here + let xdg = scratch("projpath-xdg"); + let db = xdg.join("proj.db"); + create_sqlite_db(&db); + let proj = format!("/plenum-itest-proj-{}", std::process::id()); + + // Global config keyed by the project path — resolvable from any cwd. + write_global_config( + &xdg, + &json!({ + "projects": { + proj.clone(): { + "connections": { + "default": { "engine": "sqlite", "file": db.to_str().unwrap() } + }, + "default": "default" + } + } + }), + ); + + let mut child = spawn_mcp(&["--project-path", &proj], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn project_path_loads_local_config_from_the_project_directory() { + // The core of `resolve_connection_in_project`: a LOCAL `.plenum/config.json` + // living inside the pinned project is loaded even when the server is launched + // from an unrelated cwd with no config of its own. + let proj = scratch("localcfg-proj"); + let launch_cwd = scratch("localcfg-launch"); // different, config-less cwd + let xdg = scratch("localcfg-xdg"); // empty global config + let db = proj.join("app.db"); + create_sqlite_db(&db); + + let plenum_dir = proj.join(".plenum"); + std::fs::create_dir_all(&plenum_dir).unwrap(); + std::fs::write( + plenum_dir.join("config.json"), + json!({ + "connections": { "default": { "engine": "sqlite", "file": db.to_str().unwrap() } }, + "default": "default" + }) + .to_string(), + ) + .unwrap(); + + let proj_str = proj.to_str().unwrap(); + let mut child = spawn_mcp(&["--project-path", proj_str], &[], &launch_cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&proj); + let _ = std::fs::remove_dir_all(&launch_cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criteria 2 + 5: --name selects a connection within the bound project ───── + +#[test] +fn name_selects_connection_and_combines_with_project_path() { + let cwd = scratch("name-cwd"); + let xdg = scratch("name-xdg"); + let good_db = xdg.join("prod.db"); + create_sqlite_db(&good_db); + let proj = format!("/plenum-itest-named-{}", std::process::id()); + + // Default ("staging") points at an unopenable path; "prod" is valid. + // Selecting --name prod must succeed, proving the name overrides the default. + write_global_config( + &xdg, + &json!({ + "projects": { + proj.clone(): { + "connections": { + "staging": { + "engine": "sqlite", + "file": "/plenum-nonexistent-dir/staging.db" + }, + "prod": { "engine": "sqlite", "file": good_db.to_str().unwrap() } + }, + "default": "staging" + } + } + }), + ); + + let mut child = spawn_mcp(&["--project-path", &proj, "--name", "prod"], &[], &cwd, &xdg); + let resp = handshake_then_connect(&mut child); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── Criterion 5: --dsn-env conflicts with saved-config selectors ───────────── + +#[test] +fn dsn_env_conflicts_with_project_path_at_cli() { + let bin = env!("CARGO_BIN_EXE_plenum"); + let output = Command::new(bin) + .args(["mcp", "--dsn-env", "PLENUM_DSN", "--project-path", "/tmp/x"]) + .stdin(Stdio::null()) + .output() + .expect("run plenum mcp"); + + assert!(!output.status.success(), "conflicting flags must be rejected"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("dsn-env") || stderr.contains("dsn_env"), + "conflict error should mention --dsn-env: {stderr}" + ); +} + +#[test] +fn dsn_env_conflicts_with_name_at_cli() { + let bin = env!("CARGO_BIN_EXE_plenum"); + let output = Command::new(bin) + .args(["mcp", "--dsn-env", "PLENUM_DSN", "--name", "prod"]) + .stdin(Stdio::null()) + .output() + .expect("run plenum mcp"); + + assert!(!output.status.success(), "conflicting flags must be rejected"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("dsn-env") || stderr.contains("dsn_env"), + "conflict error should mention --dsn-env: {stderr}" + ); +} + +// ─── REF-298: credential safety on the MCP connect tool ────────────────────── + +#[test] +fn connect_rejects_inline_plaintext_password() { + // Inline plaintext 'password' must be rejected with a capability error, + // directing the caller to reference-based credential sourcing. + let cwd = scratch("inline-pw"); + let xdg = scratch("inline-pw-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "localhost", + "port": 5432, + "user": "u", + "database": "d", + "password": "hunter2" + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("CAPABILITY_VIOLATION"), "must be a capability error: {msg}"); + assert!(msg.contains("password_env"), "must point to references: {msg}"); + // The plaintext secret must never be echoed back. + assert!(!msg.contains("hunter2"), "must not leak the secret: {msg}"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_save_local_persists_by_reference() { + // save: "local" writes /.plenum/config.json storing only the + // credential reference — never the plaintext secret. + let cwd = scratch("save-local"); + let xdg = scratch("save-local-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "db.example.com", + "port": 5432, + "user": "agent", + "database": "app", + "password_env": "DB_PASSWORD", + "save": "local", + "name": "prod", + "project_path": cwd.to_str().unwrap() + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + assert_no_error(&resp); + let data = tool_result_json(&resp); + assert_eq!(data["saved"], json!(true)); + assert_eq!(data["location"], json!("local")); + assert_eq!(data["name"], json!("prod")); + assert_eq!(data["credential_source"], json!("password_env")); + + let written = + std::fs::read_to_string(cwd.join(".plenum").join("config.json")).expect("config written"); + assert!(written.contains("password_env"), "reference stored: {written}"); + assert!(written.contains("DB_PASSWORD"), "env var name stored: {written}"); + assert!(!written.contains("\"password\":"), "no plaintext password: {written}"); + assert!(written.contains("prod")); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_save_requires_a_credential_reference_shape() { + // A keychain reference needs BOTH service and account; supplying only one + // is rejected before anything is written. + let cwd = scratch("save-badkc"); + let xdg = scratch("save-badkc-xdg"); + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call( + &mut child, + "connect", + &json!({ + "engine": "postgres", + "host": "h", "port": 5432, "user": "u", "database": "d", + "keychain_service": "svc", + "save": "local", + "project_path": cwd.to_str().unwrap() + }), + ); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("keychain_account"), "must name the missing field: {msg}"); + assert!(!cwd.join(".plenum").join("config.json").exists(), "nothing should be written"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +// ─── password_command is not accepted from MCP tool arguments ──────────────── +// +// `password_command` runs a shell command (`sh -c`). Accepting it from tool +// arguments would let any agent execute arbitrary commands on the host, so +// every MCP tool rejects it before anything runs. The CLI flag and wizard +// remain available to humans. + +/// Call `tool` with `password_command` set to a command that creates a marker +/// file, then assert a capability error and that the command never ran. +fn assert_password_command_rejected(tool: &str, tag: &str, mut arguments: Value) { + let cwd = scratch(tag); + let xdg = scratch(&format!("{tag}-xdg")); + let marker = cwd.join("executed"); + arguments["password_command"] = json!(format!("touch {}", marker.display())); + + let mut child = spawn_mcp(&[], &[], &cwd, &xdg); + let resp = handshake_then_call(&mut child, tool, &arguments); + let _ = child.kill(); + let _ = child.wait(); + + let msg = error_message(&resp); + assert!(msg.contains("CAPABILITY_VIOLATION"), "must be a capability error: {msg}"); + assert!(msg.contains("password_command"), "must name the rejected field: {msg}"); + assert!(!marker.exists(), "password_command must never execute"); + assert!(!cwd.join(".plenum").exists(), "nothing should be written"); + + let _ = std::fs::remove_dir_all(&cwd); + let _ = std::fs::remove_dir_all(&xdg); +} + +#[test] +fn connect_rejects_password_command() { + assert_password_command_rejected( + "connect", + "pwcmd-connect", + json!({ "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d" }), + ); +} + +#[test] +fn connect_save_rejects_password_command() { + let cwd = scratch("pwcmd-save-path"); + assert_password_command_rejected( + "connect", + "pwcmd-save", + json!({ + "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d", + "save": "global", "project_path": cwd.to_str().unwrap() + }), + ); + let _ = std::fs::remove_dir_all(&cwd); +} + +#[test] +fn introspect_rejects_password_command() { + assert_password_command_rejected( + "introspect", + "pwcmd-introspect", + json!({ "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d" }), + ); +} + +#[test] +fn query_rejects_password_command() { + assert_password_command_rejected( + "query", + "pwcmd-query", + json!({ + "engine": "postgres", "host": "h", "port": 5432, "user": "u", "database": "d", + "sql": "SELECT 1" + }), + ); +} + +#[test] +fn password_command_is_rejected_even_with_a_named_connection() { + // A named-connection call must not silently ignore the field either. + assert_password_command_rejected( + "query", + "pwcmd-named", + json!({ "name": "dev", "sql": "SELECT 1" }), + ); +} diff --git a/tests/schema_drift.rs b/tests/schema_drift.rs index 9842d96..1219603 100644 --- a/tests/schema_drift.rs +++ b/tests/schema_drift.rs @@ -3,7 +3,10 @@ //! //! When this test fails, run `cargo run --bin generate-schemas` to regenerate. -use plenum::{ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, SuccessEnvelope}; +use plenum::{ + ConnectSaveResult, ConnectionInfo, ErrorEnvelope, IntrospectResult, QueryResult, + SuccessEnvelope, +}; use schemars::schema_for; fn expected_schema(schema: &schemars::schema::RootSchema) -> String { @@ -37,6 +40,16 @@ fn connect_success_schema_not_stale() { ); } +#[test] +fn connect_save_success_schema_not_stale() { + let generated = expected_schema(&schema_for!(SuccessEnvelope)); + let on_disk = on_disk("connect_save_success.json"); + assert_eq!( + on_disk, generated, + "schemas/connect_save_success.json is stale — run: cargo run --bin generate-schemas" + ); +} + #[test] fn introspect_success_schema_not_stale() { let generated = expected_schema(&schema_for!(SuccessEnvelope));