From 2a788bcb3b715923dd00d382a97c52e22cbc9cef Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:18:28 +0100 Subject: [PATCH 01/10] feat: align MCP config with MCPG spec - container/HTTP transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace command-based MCP configuration with MCPG-native fields: - **types.rs**: Replace `command` with `container`, `entrypoint`, `entrypoint-args`, `url`, `headers`, `mounts` in McpOptions - **standalone.rs**: Update McpgServerConfig and generate_mcpg_config() to emit container/entrypointArgs (stdio) or url/headers (HTTP) - **standalone.rs**: Add generate_mcpg_docker_env() for env passthrough - Auto-maps SC_READ_TOKEN → AZURE_DEVOPS_EXT_PAT when permissions.read is configured - Forwards passthrough env vars ("") to MCPG via -e flags - **base.yml**: Add {{ mcpg_docker_env }} marker for env forwarding - **common.rs**: Update is_custom_mcp() to check container/url - **onees.rs**: Update custom MCP detection for 1ES target Tests: - New fixture: azure-devops-mcp-agent.md (container-based ADO MCP) - 4 new integration tests: fixture compilation, container config, HTTP config, env passthrough - 8 new unit tests: container/HTTP/entrypoint/mounts/env generation - Update existing tests for new field names Docs: - New example: examples/azure-devops-mcp.md (ADO work item triage) - Update AGENTS.md MCP section with container/HTTP docs and auth model Aligns with MCPG Gateway Specification v1.13.0 §3.2.1 (containerization requirement) and gh-aw front matter format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 121 ++++++--- examples/azure-devops-mcp.md | 54 ++++ src/compile/common.rs | 10 +- src/compile/onees.rs | 8 +- src/compile/standalone.rs | 316 ++++++++++++++++++++--- src/compile/types.rs | 25 +- templates/base.yml | 1 + tests/compiler_tests.rs | 205 ++++++++++++++- tests/fixtures/azure-devops-mcp-agent.md | 30 +++ tests/fixtures/complete-agent.md | 5 +- 10 files changed, 682 insertions(+), 93 deletions(-) create mode 100644 examples/azure-devops-mcp.md create mode 100644 tests/fixtures/azure-devops-mcp-agent.md diff --git a/AGENTS.md b/AGENTS.md index ef02f930..d55bac2d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,9 +128,10 @@ tools: # optional tool configuration # env: # RESERVED: workflow-level environment variables (not yet implemented) # CUSTOM_VAR: "value" mcp-servers: - my-custom-tool: # custom MCP server (requires command field) - command: "node" - args: ["path/to/mcp-server.js"] + my-custom-tool: # containerized MCP server (requires container field) + container: "node:20-slim" + entrypoint: "node" + entrypoint-args: ["path/to/mcp-server.js"] allowed: - custom_function_1 - custom_function_2 @@ -1119,61 +1120,100 @@ cargo add ## MCP Configuration -The `mcp-servers:` field configures MCP (Model Context Protocol) servers that are made available to the agent via the MCP Gateway (MCPG). All MCPs require explicit `command:` configuration — there are no built-in MCPs in the copilot CLI. -The `mcp-servers:` field configures custom MCP (Model Context Protocol) servers that the agent can use. Each entry must include a `command:` field specifying the executable to spawn. +The `mcp-servers:` field configures MCP (Model Context Protocol) servers that are made available to the agent via the MCP Gateway (MCPG). MCPs can be **containerized stdio servers** (Docker-based) or **HTTP servers** (remote endpoints). All MCP traffic flows through the MCP Gateway. -### Custom MCP Servers +### Docker Container MCP Servers (stdio) -Define MCP servers by including a `command:` field: +Run containerized MCP servers. MCPG spawns these as sibling Docker containers: ```yaml mcp-servers: - my-custom-tool: - command: "node" - args: ["path/to/mcp-server.js"] + azure-devops: + container: "node:20-slim" + entrypoint: "npx" + entrypoint-args: ["-y", "@azure-devops/mcp", "myorg", "-d", "core", "work-items"] + env: + AZURE_DEVOPS_EXT_PAT: "" allowed: - - custom_function_1 - - custom_function_2 + - core_list_projects + - wit_get_work_item + - wit_create_work_item +``` + +### HTTP MCP Servers (remote) + +Connect to remote MCP servers accessible via HTTP: + +```yaml +mcp-servers: + remote-ado: + url: "https://mcp.dev.azure.com/myorg" + headers: + X-MCP-Toolsets: "repos,wit" + X-MCP-Readonly: "true" + allowed: + - wit_get_work_item + - repo_list_repos_by_project ``` ### Configuration Properties -- `command:` - The executable to run (e.g., `"node"`, `"python"`, `"dotnet"`) -- `args:` - Array of command-line arguments passed to the command -- `allowed:` - Array of function names agents are permitted to call (required for security) -- `env:` - Optional environment variables for the MCP server process -- `service-connection:` - (1ES target only) Override the service connection name used for this MCP. If not specified, defaults to `mcp--service-connection` +**Container stdio servers:** +- `container:` - Docker image to run (e.g., `"node:20-slim"`, `"ghcr.io/org/tool:latest"`) +- `entrypoint:` - Container entrypoint override (equivalent to `docker run --entrypoint`) +- `entrypoint-args:` - Arguments passed to the entrypoint (after the image in `docker run`) +- `args:` - Additional Docker runtime arguments (inserted before the image, e.g., `["--network", "host"]`) +- `mounts:` - Volume mounts in `"source:dest:mode"` format (e.g., `["/host/data:/app/data:ro"]`) + +**HTTP servers:** +- `url:` - HTTP endpoint URL for the remote MCP server +- `headers:` - HTTP headers to include in requests (e.g., `Authorization`, `X-MCP-Toolsets`) + +**Common (both types):** +- `allowed:` - Array of tool names the agent is permitted to call (required for security) +- `env:` - Environment variables for the MCP server process. Use `""` (empty string) for passthrough from the pipeline environment. +- `service-connection:` - (1ES target only) Override the service connection name. Defaults to `mcp--service-connection` -### Example: Multiple Custom MCP Servers +### Environment Variable Passthrough + +MCP containers may need secrets from the pipeline (e.g., ADO tokens). The `env:` field supports passthrough: + +```yaml +env: + AZURE_DEVOPS_EXT_PAT: "" # Passthrough from pipeline environment + STATIC_CONFIG: "some-value" # Literal value embedded in config +``` + +When `permissions.read` is configured, the compiler automatically maps `SC_READ_TOKEN` → `AZURE_DEVOPS_EXT_PAT` on the MCPG container, so agents can access ADO APIs without manual wiring. + +### Example: Azure DevOps MCP with Authentication ```yaml mcp-servers: - # Custom Python MCP server - data-processor: - command: "python" - args: ["-m", "my_mcp_server"] + azure-devops: + container: "node:20-slim" + entrypoint: "npx" + entrypoint-args: ["-y", "@azure-devops/mcp", "myorg"] env: - DATA_DIR: "/data" + AZURE_DEVOPS_EXT_PAT: "" allowed: - - process_data - - query_database - - # Custom .NET MCP server - azure-tools: - command: "dotnet" - args: ["./tools/AzureMcp.dll"] - allowed: - - list_resources - - get_deployment_status + - core_list_projects + - wit_get_work_item +permissions: + read: my-read-arm-connection +network: + allow: + - "dev.azure.com" + - "*.dev.azure.com" ``` ### Security Notes -1. **Allow-listing**: Only functions explicitly listed in `allowed:` are accessible to agents -2. **Command Validation**: The compiler validates that commands are from a trusted set -3. **Argument Sanitization**: Arguments are validated to prevent injection attacks -4. **Environment Isolation**: MCP servers run in the same isolated sandbox as the pipeline -5. **MCPG Gateway**: All MCP traffic flows through the MCP Gateway which enforces tool-level filtering +1. **Allow-listing**: Only tools explicitly listed in `allowed:` are accessible to agents +2. **Containerization**: Stdio MCP servers run as isolated Docker containers (per MCPG spec §3.2.1) +3. **Environment Isolation**: MCP containers are spawned by MCPG with only the configured environment variables +4. **MCPG Gateway**: All MCP traffic flows through the MCP Gateway which enforces tool-level filtering +5. **Network Isolation**: MCP containers run within the same AWF-isolated network. Users must explicitly allow external domains via `network.allow` ## Network Isolation (AWF) @@ -1336,8 +1376,9 @@ The compiler generates MCPG configuration JSON from the `mcp-servers:` front mat }, "custom-tool": { "type": "stdio", - "command": "node", - "args": ["server.js"], + "container": "node:20-slim", + "entrypoint": "node", + "entrypointArgs": ["server.js"], "tools": ["process_data", "get_status"] } }, diff --git a/examples/azure-devops-mcp.md b/examples/azure-devops-mcp.md new file mode 100644 index 00000000..2388f483 --- /dev/null +++ b/examples/azure-devops-mcp.md @@ -0,0 +1,54 @@ +--- +name: "ADO Work Item Triage" +description: "Triages work items using the Azure DevOps MCP" +schedule: daily around 9:00 +engine: claude-sonnet-4.5 +mcp-servers: + azure-devops: + container: "node:20-slim" + entrypoint: "npx" + entrypoint-args: ["-y", "@azure-devops/mcp", "myorg", "-d", "core", "work", "work-items"] + env: + AZURE_DEVOPS_EXT_PAT: "" + allowed: + - core_list_projects + - wit_my_work_items + - wit_get_work_item + - wit_get_work_items_batch_by_ids + - wit_update_work_item + - wit_add_work_item_comment + - search_workitem +permissions: + read: my-read-arm-connection + write: my-write-arm-connection +safe-outputs: + update-work-item: + status: true + body: true + tags: true + max: 10 + target: "*" + comment-on-work-item: + max: 10 + target: "*" +network: + allow: + - "dev.azure.com" + - "*.dev.azure.com" + - "*.visualstudio.com" +--- + +## ADO Work Item Triage Agent + +You have access to the Azure DevOps MCP server. Use it to: + +1. Query open work items assigned to the team +2. Triage items by priority and area path +3. Add comments summarizing your analysis +4. Update tags to reflect triage status + +### Guidelines + +- Only update work items that need attention +- Add the `triaged` tag to items you've reviewed +- Add a comment explaining your triage decision diff --git a/src/compile/common.rs b/src/compile/common.rs index a75ae6bf..b0b7eb9e 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -6,11 +6,11 @@ use super::types::{FrontMatter, Repository, TriggerConfig}; use crate::compile::types::McpConfig; use crate::fuzzy_schedule; -/// Check if an MCP has a custom command (i.e., is not just a name-based reference). -/// All MCPs now require explicit command configuration — there are no built-in MCPs -/// in the copilot CLI. +/// Check if an MCP has a transport configuration (container or URL). +/// MCPs with a container are containerized stdio servers; MCPs with a URL +/// are HTTP servers. Both are routed through the MCP Gateway (MCPG). pub fn is_custom_mcp(config: &McpConfig) -> bool { - matches!(config, McpConfig::WithOptions(opts) if opts.command.is_some()) + matches!(config, McpConfig::WithOptions(opts) if opts.container.is_some() || opts.url.is_some()) } /// Parse the markdown file and extract front matter and body @@ -1006,7 +1006,7 @@ mod tests { fm.mcp_servers.insert( "my-tool".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("node".to_string()), + container: Some("node:20-slim".to_string()), ..Default::default() }), ); diff --git a/src/compile/onees.rs b/src/compile/onees.rs index 8d06d5dd..a1157669 100644 --- a/src/compile/onees.rs +++ b/src/compile/onees.rs @@ -208,10 +208,10 @@ displayName: "Finalize""#, if front_matter .mcp_servers .iter() - .any(|(_, c)| matches!(c, McpConfig::WithOptions(o) if o.command.is_some())) + .any(|(_, c)| is_custom_mcp(c)) { eprintln!( - "Warning: Custom MCP servers (with command:) are not supported in 1ES target. \ + "Warning: Custom MCP servers (with container: or url:) are not supported in 1ES target. \ They will be ignored. Use standalone target for full MCP support." ); } @@ -251,10 +251,10 @@ fn generate_mcp_configuration(mcps: &HashMap) -> String { return None; } - // Custom MCPs with command: not supported in 1ES (needs service connection) + // Custom MCPs with container/url: not supported in 1ES (needs service connection) if is_custom_mcp(config) { log::warn!( - "MCP '{}' uses custom command — not supported in 1ES target (requires service connection)", + "MCP '{}' uses custom container/url — not supported in 1ES target (requires service connection)", name ); return None; diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index bc36487c..5b3c19f5 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -205,6 +205,11 @@ impl Compiler for StandaloneCompiler { let pipeline_yaml = replace_with_indent(&pipeline_yaml, "{{ mcpg_config }}", &mcpg_config_json); + // Generate additional -e flags for MCPG Docker run (env passthrough for MCP containers) + let mcpg_docker_env = generate_mcpg_docker_env(front_matter); + let pipeline_yaml = + replace_with_indent(&pipeline_yaml, "{{ mcpg_docker_env }}", &mcpg_docker_env); + // Prepend header comment for pipeline detection let header = generate_header_comment(input_path); let pipeline_yaml = format!("{}{}", header, pipeline_yaml); @@ -372,13 +377,22 @@ fn generate_agentic_depends_on(setup_steps: &[serde_yaml::Value]) -> String { #[derive(Debug, Serialize, Clone)] #[serde(rename_all = "camelCase")] pub struct McpgServerConfig { - /// Server type: "stdio" for command-based, "http" for HTTP backends + /// Server type: "stdio" for container-based, "http" for HTTP backends #[serde(rename = "type")] pub server_type: String, - /// Command to run (for stdio type) + /// Docker container image (for stdio type, per MCPG spec §4.1.2) + #[serde(skip_serializing_if = "Option::is_none")] + pub container: Option, + /// Container entrypoint override (for stdio type) + #[serde(skip_serializing_if = "Option::is_none")] + pub entrypoint: Option, + /// Arguments passed to the container entrypoint (for stdio type) #[serde(skip_serializing_if = "Option::is_none")] - pub command: Option, - /// Command arguments (for stdio type) + pub entrypoint_args: Option>, + /// Volume mounts for containerized servers (format: "source:dest:mode") + #[serde(skip_serializing_if = "Option::is_none")] + pub mounts: Option>, + /// Additional Docker runtime arguments (inserted before image in `docker run`) #[serde(skip_serializing_if = "Option::is_none")] pub args: Option>, /// URL for HTTP backends @@ -429,7 +443,10 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { "safeoutputs".to_string(), McpgServerConfig { server_type: "http".to_string(), - command: None, + container: None, + entrypoint: None, + entrypoint_args: None, + mounts: None, args: None, url: Some("http://localhost:${SAFE_OUTPUTS_PORT}/mcp".to_string()), headers: Some(HashMap::from([( @@ -460,13 +477,23 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } if let Some(opts) = options { - if let Some(command) = &opts.command { - // Custom MCP with explicit command → stdio server + if let Some(container) = &opts.container { + // Container-based stdio MCP (MCPG-native, per spec §3.2.1) + let entrypoint_args = if opts.entrypoint_args.is_empty() { + None + } else { + Some(opts.entrypoint_args.clone()) + }; let args = if opts.args.is_empty() { None } else { Some(opts.args.clone()) }; + let mounts = if opts.mounts.is_empty() { + None + } else { + Some(opts.mounts.clone()) + }; let env = if opts.env.is_empty() { None } else { @@ -481,7 +508,10 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { name.clone(), McpgServerConfig { server_type: "stdio".to_string(), - command: Some(command.clone()), + container: Some(container.clone()), + entrypoint: opts.entrypoint.clone(), + entrypoint_args, + mounts, args, url: None, headers: None, @@ -489,12 +519,39 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { tools, }, ); + } else if let Some(url) = &opts.url { + // HTTP-based MCP (remote server) + let headers = if opts.headers.is_empty() { + None + } else { + Some(opts.headers.clone()) + }; + let tools = if opts.allowed.is_empty() { + None + } else { + Some(opts.allowed.clone()) + }; + mcp_servers.insert( + name.clone(), + McpgServerConfig { + server_type: "http".to_string(), + container: None, + entrypoint: None, + entrypoint_args: None, + mounts: None, + args: None, + url: Some(url.clone()), + headers, + env: None, + tools, + }, + ); } else { - log::warn!("MCP '{}' has no command - skipping", name); + log::warn!("MCP '{}' has no container or url — skipping", name); continue; } } else { - log::warn!("MCP '{}' has no command - skipping", name); + log::warn!("MCP '{}' has no container or url — skipping", name); } } @@ -509,6 +566,52 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } } +/// Generate additional `-e` flags for the MCPG Docker run command. +/// +/// MCP containers spawned by MCPG may need environment variables that flow from +/// the pipeline through the MCPG container (passthrough). This function: +/// 1. Auto-maps `AZURE_DEVOPS_EXT_PAT` from `SC_READ_TOKEN` when `permissions.read` is configured +/// 2. Collects all passthrough env vars (value is `""`) from MCP configs and adds `-e` flags +pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { + let mut env_flags: Vec = Vec::new(); + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + + // Auto-map AZURE_DEVOPS_EXT_PAT from SC_READ_TOKEN when permissions.read is configured + if front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() { + env_flags.push( + "-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\"".to_string(), + ); + seen.insert("AZURE_DEVOPS_EXT_PAT".to_string()); + } + + // Collect passthrough env vars from all MCP configs + for (_name, config) in &front_matter.mcp_servers { + let opts = match config { + McpConfig::WithOptions(opts) if opts.enabled.unwrap_or(true) => opts, + _ => continue, + }; + + for (var_name, var_value) in &opts.env { + if seen.contains(var_name) { + continue; + } + // Passthrough: empty string means forward from host environment + // Expansion: ${VAR} means MCPG resolves from its own environment + if var_value.is_empty() || var_value.contains("${") { + env_flags.push(format!("-e {}", var_name)); + seen.insert(var_name.clone()); + } + } + } + + env_flags.sort(); + if env_flags.is_empty() { + String::new() + } else { + format!("\\\n {}", env_flags.join(" \\\n ")) + } +} + /// Generate the steps to download agent memory from the previous successful run /// and restore it to the staging directory. fn generate_memory_download() -> String { @@ -579,8 +682,9 @@ mod tests { fm.mcp_servers.insert( "my-tool".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("node".to_string()), - args: vec!["server.js".to_string()], + container: Some("node:20-slim".to_string()), + entrypoint: Some("node".to_string()), + entrypoint_args: vec!["server.js".to_string()], allowed: vec!["do_thing".to_string()], ..Default::default() }), @@ -588,8 +692,12 @@ mod tests { let config = generate_mcpg_config(&fm); let server = config.mcp_servers.get("my-tool").unwrap(); assert_eq!(server.server_type, "stdio"); - assert_eq!(server.command.as_ref().unwrap(), "node"); - assert_eq!(server.args.as_ref().unwrap(), &vec!["server.js"]); + assert_eq!(server.container.as_ref().unwrap(), "node:20-slim"); + assert_eq!(server.entrypoint.as_ref().unwrap(), "node"); + assert_eq!( + server.entrypoint_args.as_ref().unwrap(), + &vec!["server.js"] + ); assert_eq!( server.tools.as_ref().unwrap(), &vec!["do_thing".to_string()] @@ -597,9 +705,9 @@ mod tests { } #[test] - fn test_generate_mcpg_config_mcp_without_command_skipped() { + fn test_generate_mcpg_config_mcp_without_transport_skipped() { let mut fm = minimal_front_matter(); - // An MCP with no command should be skipped (no built-in MCPs) + // An MCP with no container or url should be skipped fm.mcp_servers .insert("phantom".to_string(), McpConfig::Enabled(true)); let config = generate_mcpg_config(&fm); @@ -642,8 +750,9 @@ mod tests { fm.mcp_servers.insert( "my-tool".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("python".to_string()), - args: vec!["-m".to_string(), "server".to_string()], + container: Some("python:3.12-slim".to_string()), + entrypoint: Some("python".to_string()), + entrypoint_args: vec!["-m".to_string(), "server".to_string()], allowed: vec!["query".to_string()], ..Default::default() }), @@ -698,19 +807,23 @@ mod tests { let config = generate_mcpg_config(&fm); let so = config.mcp_servers.get("safeoutputs").unwrap(); assert_eq!(so.server_type, "http"); - assert!(so.command.is_none(), "HTTP backend should have no command"); + assert!( + so.container.is_none(), + "HTTP backend should have no container" + ); assert!(so.args.is_none(), "HTTP backend should have no args"); assert!(so.url.is_some(), "HTTP backend must have a URL"); } #[test] - fn test_generate_mcpg_config_custom_mcp_is_stdio_type() { + fn test_generate_mcpg_config_container_mcp_is_stdio_type() { let mut fm = minimal_front_matter(); fm.mcp_servers.insert( "runner".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("node".to_string()), - args: vec!["srv.js".to_string()], + container: Some("node:20-slim".to_string()), + entrypoint: Some("node".to_string()), + entrypoint_args: vec!["srv.js".to_string()], allowed: vec!["run".to_string()], ..Default::default() }), @@ -718,22 +831,22 @@ mod tests { let config = generate_mcpg_config(&fm); let srv = config.mcp_servers.get("runner").unwrap(); assert_eq!(srv.server_type, "stdio"); - assert!(srv.command.is_some(), "stdio server must have a command"); + assert!( + srv.container.is_some(), + "stdio server must have a container" + ); assert!(srv.url.is_none(), "stdio server should have no URL"); } #[test] - fn test_generate_firewall_config_unknown_non_builtin_skipped() { - // An MCP with no command should be skipped + fn test_generate_mcpg_config_container_with_env() { let mut fm = minimal_front_matter(); let mut env = std::collections::HashMap::new(); env.insert("TOKEN".to_string(), "secret".to_string()); fm.mcp_servers.insert( "with-env".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("node".to_string()), - args: vec![], - allowed: vec![], + container: Some("node:20-slim".to_string()), env, ..Default::default() }), @@ -750,22 +863,20 @@ mod tests { fm.mcp_servers.insert( "safeoutputs".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("evil".to_string()), - args: vec![], - allowed: vec![], + container: Some("evil:latest".to_string()), ..Default::default() }), ); let config = generate_mcpg_config(&fm); - // The reserved entry should still be the HTTP backend, not the user's command + // The reserved entry should still be the HTTP backend, not the user's container let so = config.mcp_servers.get("safeoutputs").unwrap(); assert_eq!( so.server_type, "http", "safeoutputs should remain HTTP backend" ); assert!( - so.command.is_none(), - "User command should not overwrite safeoutputs" + so.container.is_none(), + "User container should not overwrite safeoutputs" ); } @@ -775,8 +886,9 @@ mod tests { fm.mcp_servers.insert( "SafeOutputs".to_string(), McpConfig::WithOptions(McpOptions { - command: Some("node".to_string()), - args: vec!["evil.js".to_string()], + container: Some("node:20-slim".to_string()), + entrypoint: Some("node".to_string()), + entrypoint_args: vec!["evil.js".to_string()], allowed: vec!["hijack".to_string()], ..Default::default() }), @@ -789,4 +901,136 @@ mod tests { // No stdio entry should have been added under any casing assert_eq!(config.mcp_servers.len(), 1); } + + #[test] + fn test_generate_mcpg_config_http_mcp() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "remote".to_string(), + McpConfig::WithOptions(McpOptions { + url: Some("https://mcp.example.com/api".to_string()), + headers: { + let mut h = HashMap::new(); + h.insert("X-Custom".to_string(), "value".to_string()); + h + }, + allowed: vec!["query".to_string()], + ..Default::default() + }), + ); + let config = generate_mcpg_config(&fm); + let srv = config.mcp_servers.get("remote").unwrap(); + assert_eq!(srv.server_type, "http"); + assert_eq!( + srv.url.as_ref().unwrap(), + "https://mcp.example.com/api" + ); + assert_eq!( + srv.headers.as_ref().unwrap().get("X-Custom").unwrap(), + "value" + ); + assert!(srv.container.is_none(), "HTTP server should have no container"); + } + + #[test] + fn test_generate_mcpg_config_container_with_entrypoint() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "ado".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("node:20-slim".to_string()), + entrypoint: Some("npx".to_string()), + entrypoint_args: vec!["-y".to_string(), "@azure-devops/mcp".to_string()], + ..Default::default() + }), + ); + let config = generate_mcpg_config(&fm); + let srv = config.mcp_servers.get("ado").unwrap(); + assert_eq!(srv.server_type, "stdio"); + assert_eq!(srv.container.as_ref().unwrap(), "node:20-slim"); + assert_eq!(srv.entrypoint.as_ref().unwrap(), "npx"); + assert_eq!( + srv.entrypoint_args.as_ref().unwrap(), + &vec!["-y", "@azure-devops/mcp"] + ); + } + + #[test] + fn test_generate_mcpg_config_container_with_mounts() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "data-tool".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("data-tool:latest".to_string()), + mounts: vec!["/host/data:/app/data:ro".to_string()], + ..Default::default() + }), + ); + let config = generate_mcpg_config(&fm); + let srv = config.mcp_servers.get("data-tool").unwrap(); + assert_eq!( + srv.mounts.as_ref().unwrap(), + &vec!["/host/data:/app/data:ro"] + ); + } + + #[test] + fn test_generate_mcpg_config_no_transport_skipped() { + let mut fm = minimal_front_matter(); + // MCP with options but no container or url should be skipped + fm.mcp_servers.insert( + "no-transport".to_string(), + McpConfig::WithOptions(McpOptions { + allowed: vec!["tool".to_string()], + ..Default::default() + }), + ); + let config = generate_mcpg_config(&fm); + assert!(!config.mcp_servers.contains_key("no-transport")); + } + + #[test] + fn test_generate_mcpg_docker_env_with_permissions_read() { + let mut fm = minimal_front_matter(); + fm.permissions = Some(crate::compile::types::PermissionsConfig { + read: Some("my-read-sc".to_string()), + write: None, + }); + let env = generate_mcpg_docker_env(&fm); + assert!( + env.contains("-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\""), + "Should auto-map ADO token when permissions.read is set" + ); + } + + #[test] + fn test_generate_mcpg_docker_env_without_permissions() { + let fm = minimal_front_matter(); + let env = generate_mcpg_docker_env(&fm); + assert!( + !env.contains("AZURE_DEVOPS_EXT_PAT"), + "Should not map ADO token when permissions.read is not set" + ); + } + + #[test] + fn test_generate_mcpg_docker_env_passthrough_vars() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "tool".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("img:latest".to_string()), + env: { + let mut e = HashMap::new(); + e.insert("PASS_THROUGH".to_string(), "".to_string()); + e.insert("STATIC".to_string(), "value".to_string()); + e + }, + ..Default::default() + }), + ); + let env = generate_mcpg_docker_env(&fm); + assert!(env.contains("-e PASS_THROUGH"), "Should include passthrough var"); + assert!(!env.contains("-e STATIC"), "Should NOT include static var"); + } } diff --git a/src/compile/types.rs b/src/compile/types.rs index 1279af05..145772d9 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -355,16 +355,31 @@ pub struct McpOptions { /// Whether this MCP is enabled (default: true) #[serde(default)] pub enabled: Option, - /// Custom command (if present, it's a custom MCP - standalone only) + /// Docker container image for containerized stdio MCPs (MCPG-native) #[serde(default)] - pub command: Option, - /// Command arguments + pub container: Option, + /// Container entrypoint override (equivalent to `docker run --entrypoint`) + #[serde(default)] + pub entrypoint: Option, + /// Arguments passed to the container entrypoint + #[serde(default, rename = "entrypoint-args")] + pub entrypoint_args: Vec, + /// Additional Docker runtime arguments (inserted before the image in `docker run`) #[serde(default)] pub args: Vec, - /// Allowed tool names (for firewall filtering) + /// HTTP endpoint URL for remote MCPs + #[serde(default)] + pub url: Option, + /// HTTP headers for remote MCPs (e.g., Authorization, X-MCP-Toolsets) + #[serde(default)] + pub headers: HashMap, + /// Volume mounts for containerized MCPs (format: "source:dest:mode") + #[serde(default)] + pub mounts: Vec, + /// Allowed tool names (for MCPG tool filtering) #[serde(default)] pub allowed: Vec, - /// Environment variables + /// Environment variables for the MCP server process #[serde(default)] pub env: HashMap, /// Service connection name (1ES only, auto-generated if not specified) diff --git a/templates/base.yml b/templates/base.yml index cb85e607..005fd67f 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -264,6 +264,7 @@ jobs: --network host \ -v /var/run/docker.sock:/var/run/docker.sock \ -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" \ + {{ mcpg_docker_env }} ghcr.io/github/gh-aw-mcpg:v{{ mcpg_version }} & MCPG_PID=$! echo "MCPG started (PID: $MCPG_PID)" diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 713e976c..40c4fa62 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -346,7 +346,7 @@ fn test_fixture_complete_agent() { ); // Verify it has MCP configuration and custom MCPs - assert!(content.contains("command:"), "Should have custom MCP"); + assert!(content.contains("container:"), "Should have custom MCP"); // Verify permissions assert!( @@ -2013,3 +2013,206 @@ Vote on pull requests. let _ = fs::remove_dir_all(&temp_dir); } + +// ==================== Azure DevOps MCP Integration Tests ==================== + +/// Test that the Azure DevOps MCP fixture compiles successfully with no unreplaced markers +#[test] +fn test_fixture_azure_devops_mcp_compiled_output() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-ado-mcp-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let fixture_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests") + .join("fixtures") + .join("azure-devops-mcp-agent.md"); + + let output_path = temp_dir.join("azure-devops-mcp-agent.yml"); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + fixture_path.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed for Azure DevOps MCP fixture: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let compiled = fs::read_to_string(&output_path).expect("Should read compiled output"); + + // No unreplaced template markers (except ADO ${{ }} expressions) + for line in compiled.lines() { + let stripped = line.replace("${{", ""); + assert!( + !stripped.contains("{{ "), + "Compiled output should not contain unreplaced marker: {}", + line.trim() + ); + } + + // Should contain MCPG references + assert!( + compiled.contains("ghcr.io/github/gh-aw-mcpg"), + "Should reference MCPG Docker image" + ); + + // Should contain the container-based MCP config (container field, not command) + assert!( + compiled.contains("\"container\""), + "MCPG config should use container field" + ); + assert!( + compiled.contains("node:20-slim"), + "MCPG config should contain the container image" + ); + assert!( + compiled.contains("\"entrypoint\""), + "MCPG config should have entrypoint field" + ); + assert!( + compiled.contains("\"entrypointArgs\""), + "MCPG config should have entrypointArgs field" + ); + assert!( + !compiled.contains("\"command\""), + "MCPG config should NOT use command field" + ); + + // Should contain env passthrough for AZURE_DEVOPS_EXT_PAT + assert!( + compiled.contains("AZURE_DEVOPS_EXT_PAT"), + "Should reference AZURE_DEVOPS_EXT_PAT" + ); + + // Should contain SC_READ_TOKEN (from permissions.read) + assert!( + compiled.contains("SC_READ_TOKEN"), + "Should contain SC_READ_TOKEN" + ); + + // Should contain the MCPG docker env passthrough (auto-mapped ADO token) + assert!( + compiled.contains("-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\""), + "Should auto-map SC_READ_TOKEN to AZURE_DEVOPS_EXT_PAT on MCPG Docker run" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that container-based MCPs generate correct MCPG config JSON structure +#[test] +fn test_mcpg_config_container_based_mcp() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcpg-container-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"Container MCP Test\"\ndescription: \"Tests container-based MCP\"\nmcp-servers:\n my-tool:\n container: \"ghcr.io/example/my-tool:latest\"\n entrypoint: \"my-tool\"\n entrypoint-args: [\"--mode\", \"stdio\"]\n mounts:\n - \"/host/data:/app/data:ro\"\n env:\n API_KEY: \"test-key\"\n allowed:\n - tool_a\n - tool_b\n---\n\n## Test\n"; + + let input_path = temp_dir.join("container-mcp.md"); + let output_path = temp_dir.join("container-mcp.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args(["compile", input_path.to_str().unwrap(), "-o", output_path.to_str().unwrap()]) + .output() + .expect("Failed to run compiler"); + + assert!(output.status.success(), "Compiler should succeed: {}", String::from_utf8_lossy(&output.stderr)); + + let compiled = fs::read_to_string(&output_path).unwrap(); + + assert!(compiled.contains("\"container\": \"ghcr.io/example/my-tool:latest\"")); + assert!(compiled.contains("\"entrypoint\": \"my-tool\"")); + assert!(compiled.contains("\"entrypointArgs\"")); + assert!(compiled.contains("\"mounts\"")); + assert!(compiled.contains("/host/data:/app/data:ro")); + assert!(compiled.contains("\"API_KEY\": \"test-key\"")); + assert!(compiled.contains("\"tool_a\"")); + assert!(!compiled.contains("\"command\"")); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that HTTP-based MCPs generate correct MCPG config JSON structure +#[test] +fn test_mcpg_config_http_based_mcp() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcpg-http-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"HTTP MCP Test\"\ndescription: \"Tests HTTP MCP\"\nmcp-servers:\n remote-ado:\n url: \"https://mcp.dev.azure.com/myorg\"\n headers:\n X-MCP-Toolsets: \"repos,wit\"\n allowed:\n - wit_get_work_item\n---\n\n## Test\n"; + + let input_path = temp_dir.join("http-mcp.md"); + let output_path = temp_dir.join("http-mcp.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args(["compile", input_path.to_str().unwrap(), "-o", output_path.to_str().unwrap()]) + .output() + .expect("Failed to run compiler"); + + assert!(output.status.success(), "Compiler should succeed: {}", String::from_utf8_lossy(&output.stderr)); + + let compiled = fs::read_to_string(&output_path).unwrap(); + + assert!(compiled.contains("\"url\": \"https://mcp.dev.azure.com/myorg\"")); + assert!(compiled.contains("\"X-MCP-Toolsets\": \"repos,wit\"")); + assert!(compiled.contains("\"wit_get_work_item\"")); + assert!(!compiled.contains("\"command\"")); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// Test that env passthrough generates -e flags in MCPG Docker run +#[test] +fn test_mcpg_docker_env_passthrough() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcpg-env-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"Env Test\"\ndescription: \"Tests env passthrough\"\npermissions:\n read: my-read-sc\n write: my-write-sc\nmcp-servers:\n my-tool:\n container: \"node:20-slim\"\n env:\n MY_TOKEN: \"\"\n STATIC_VAR: \"static-value\"\nsafe-outputs:\n create-work-item:\n work-item-type: Task\n---\n\n## Test\n"; + + let input_path = temp_dir.join("env-passthrough.md"); + let output_path = temp_dir.join("env-passthrough.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args(["compile", input_path.to_str().unwrap(), "-o", output_path.to_str().unwrap()]) + .output() + .expect("Failed to run compiler"); + + assert!(output.status.success(), "Compiler should succeed: {}", String::from_utf8_lossy(&output.stderr)); + + let compiled = fs::read_to_string(&output_path).unwrap(); + + // Should auto-map AZURE_DEVOPS_EXT_PAT from SC_READ_TOKEN + assert!(compiled.contains("-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\""), "Should auto-map ADO token"); + + // Should forward passthrough env var MY_TOKEN + assert!(compiled.contains("-e MY_TOKEN"), "Should forward passthrough env var"); + + // Static var should be in config + assert!(compiled.contains("\"STATIC_VAR\": \"static-value\""), "Static env var should be in config"); + + let _ = fs::remove_dir_all(&temp_dir); +} diff --git a/tests/fixtures/azure-devops-mcp-agent.md b/tests/fixtures/azure-devops-mcp-agent.md new file mode 100644 index 00000000..43e40368 --- /dev/null +++ b/tests/fixtures/azure-devops-mcp-agent.md @@ -0,0 +1,30 @@ +--- +name: "Azure DevOps MCP Agent" +description: "Agent with Azure DevOps MCP via containerized stdio transport" +mcp-servers: + azure-devops: + container: "node:20-slim" + entrypoint: "npx" + entrypoint-args: ["-y", "@azure-devops/mcp", "myorg", "-d", "core", "work-items"] + env: + AZURE_DEVOPS_EXT_PAT: "" + allowed: + - core_list_projects + - wit_get_work_item + - wit_create_work_item + - wit_my_work_items +permissions: + read: my-read-arm-connection + write: my-write-arm-connection +safe-outputs: + create-work-item: + work-item-type: Task +network: + allow: + - "dev.azure.com" + - "*.dev.azure.com" +--- + +## Azure DevOps MCP Integration Test + +Review work items and create tasks as needed using the Azure DevOps MCP server. diff --git a/tests/fixtures/complete-agent.md b/tests/fixtures/complete-agent.md index c506e28e..731ec8d9 100644 --- a/tests/fixtures/complete-agent.md +++ b/tests/fixtures/complete-agent.md @@ -25,8 +25,9 @@ mcp-servers: allowed: - query custom-tool: - command: "node" - args: ["server.js"] + container: "node:20-slim" + entrypoint: "node" + entrypoint-args: ["server.js"] allowed: - custom_function_1 - custom_function_2 From 28edaf251a479a0a8b8405df80c03e0240d21210 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:28:40 +0100 Subject: [PATCH 02/10] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94?= =?UTF-8?q?=20env=20var=20name=20validation,=20docs,=20HTTP=20env=20warnin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate env var names against [A-Za-z_][A-Za-z0-9_]* to prevent Docker flag injection via user-controlled front matter keys - Document {{ mcpg_docker_env }} template marker in AGENTS.md - Warn when env: is configured on HTTP MCPs (silently ignored) - Add unit tests for env var name validation and injection rejection - Update {{ mcpg_config }} docs for container/url (was command) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 16 +++++++-- src/compile/standalone.rs | 70 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d55bac2d..3bc80eba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -590,13 +590,25 @@ Should be replaced with the markdown body (agent instructions) extracted from th Should be replaced with the MCP Gateway (MCPG) configuration JSON generated from the `mcp-servers:` front matter. This configuration defines the MCPG server entries and gateway settings. The generated JSON has two top-level sections: -- `mcpServers`: Maps server names to their configuration (type, command/url, tools, etc.) +- `mcpServers`: Maps server names to their configuration (type, container/url, tools, etc.) - `gateway`: Gateway settings (port, domain, apiKey, payloadDir) -SafeOutputs is always included as an HTTP backend (`type: "http"`) pointing to `localhost` (MCPG runs with `--network host`, so `localhost` is the host loopback). Custom MCPs with explicit `command:` are included as stdio servers (`type: "stdio"`). MCPs without a command are skipped (there are no built-in MCPs in the copilot CLI). +SafeOutputs is always included as an HTTP backend (`type: "http"`) pointing to `localhost` (MCPG runs with `--network host`, so `localhost` is the host loopback). Containerized MCPs with `container:` are included as stdio servers (`type: "stdio"` with `container`, `entrypoint`, `entrypointArgs`). HTTP MCPs with `url:` are included as HTTP servers. MCPs without a container or url are skipped. Runtime placeholders (`${SAFE_OUTPUTS_PORT}`, `${SAFE_OUTPUTS_API_KEY}`, `${MCP_GATEWAY_API_KEY}`) are substituted by the pipeline at runtime before passing the config to MCPG. +## {{ mcpg_docker_env }} + +Should be replaced with additional `-e` flags for the MCPG Docker run command, enabling environment variable passthrough from the pipeline to MCP containers. + +When `permissions.read` is configured, the compiler automatically adds `-e AZURE_DEVOPS_EXT_PAT="$(SC_READ_TOKEN)"` to forward the ADO access token to MCP containers that need it (e.g., Azure DevOps MCP). + +Additionally, any env vars in MCP configs with empty string values (`""`) are collected and forwarded as `-e VAR_NAME` flags, enabling passthrough from the pipeline environment through MCPG to MCP child containers. + +Environment variable names are validated against `[A-Za-z_][A-Za-z0-9_]*` to prevent Docker flag injection. + +If no passthrough env vars are needed, this marker is replaced with an empty string. + ## {{ allowed_domains }} Should be replaced with the comma-separated domain list for AWF's `--allow-domains` flag. The list includes: diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 5b3c19f5..253882e6 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -521,6 +521,13 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { ); } else if let Some(url) = &opts.url { // HTTP-based MCP (remote server) + if !opts.env.is_empty() { + log::warn!( + "MCP '{}': env vars are not supported for HTTP MCPs — they will be ignored. \ + Use headers for authentication instead.", + name + ); + } let headers = if opts.headers.is_empty() { None } else { @@ -566,6 +573,16 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } } +/// Validate that a string is a legal environment variable name (`[A-Za-z_][A-Za-z0-9_]*`). +/// Prevents injection of arbitrary Docker flags via user-controlled front matter keys. +fn is_valid_env_var_name(name: &str) -> bool { + let mut chars = name.chars(); + chars + .next() + .map_or(false, |c| c.is_ascii_alphabetic() || c == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + /// Generate additional `-e` flags for the MCPG Docker run command. /// /// MCP containers spawned by MCPG may need environment variables that flow from @@ -585,13 +602,21 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { } // Collect passthrough env vars from all MCP configs - for (_name, config) in &front_matter.mcp_servers { + for (mcp_name, config) in &front_matter.mcp_servers { let opts = match config { McpConfig::WithOptions(opts) if opts.enabled.unwrap_or(true) => opts, _ => continue, }; for (var_name, var_value) in &opts.env { + // Validate env var name to prevent Docker flag injection (e.g. "X --privileged") + if !is_valid_env_var_name(var_name) { + log::warn!( + "MCP '{}': skipping invalid env var name '{}' — must match [A-Za-z_][A-Za-z0-9_]*", + mcp_name, var_name + ); + continue; + } if seen.contains(var_name) { continue; } @@ -1033,4 +1058,47 @@ mod tests { assert!(env.contains("-e PASS_THROUGH"), "Should include passthrough var"); assert!(!env.contains("-e STATIC"), "Should NOT include static var"); } + + #[test] + fn test_generate_mcpg_docker_env_rejects_invalid_names() { + let mut fm = minimal_front_matter(); + fm.mcp_servers.insert( + "evil".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("img:latest".to_string()), + env: { + let mut e = HashMap::new(); + // Injection attempt: env var name with Docker flag + e.insert("MY_VAR --privileged".to_string(), "".to_string()); + // Valid env var for comparison + e.insert("GOOD_VAR".to_string(), "".to_string()); + e + }, + ..Default::default() + }), + ); + let env = generate_mcpg_docker_env(&fm); + assert!( + !env.contains("--privileged"), + "Should reject invalid env var name with Docker flag injection" + ); + assert!( + env.contains("-e GOOD_VAR"), + "Should include valid env var" + ); + } + + #[test] + fn test_is_valid_env_var_name() { + assert!(is_valid_env_var_name("MY_VAR")); + assert!(is_valid_env_var_name("_PRIVATE")); + assert!(is_valid_env_var_name("A")); + assert!(is_valid_env_var_name("VAR123")); + assert!(!is_valid_env_var_name("")); + assert!(!is_valid_env_var_name("123ABC")); + assert!(!is_valid_env_var_name("MY-VAR")); + assert!(!is_valid_env_var_name("MY VAR")); + assert!(!is_valid_env_var_name("X --privileged")); + assert!(!is_valid_env_var_name("X -v /etc:/etc:rw")); + } } From b939faacfe051c3d4b1722ea63fcde69ba9b412a Mon Sep 17 00:00:00 2001 From: James Devine Date: Sun, 12 Apr 2026 23:41:42 +0100 Subject: [PATCH 03/10] fix: docker run line continuation bug, mount/URL validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix broken bash line continuation when {{ mcpg_docker_env }} is non-empty or empty — restructured template so marker outputs include trailing backslash, validated with bash -n - Add validate_mount_source() — warns on sensitive host path prefixes (/etc, /root, /home, /proc, /sys, docker.sock) - Add validate_mcp_url() — warns if URL doesn't use http(s):// scheme Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 57 +++++++++++++++++++++++++++++++++++++-- templates/base.yml | 3 +-- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 253882e6..c3ff9e58 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -479,6 +479,10 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { if let Some(opts) = options { if let Some(container) = &opts.container { // Container-based stdio MCP (MCPG-native, per spec §3.2.1) + // Validate mount paths for sensitive host directories + for mount in &opts.mounts { + validate_mount_source(mount, name); + } let entrypoint_args = if opts.entrypoint_args.is_empty() { None } else { @@ -521,6 +525,7 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { ); } else if let Some(url) = &opts.url { // HTTP-based MCP (remote server) + validate_mcp_url(url, name); if !opts.env.is_empty() { log::warn!( "MCP '{}': env vars are not supported for HTTP MCPs — they will be ignored. \ @@ -573,6 +578,45 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } } +/// Sensitive host path prefixes that should not be bind-mounted into MCP containers. +const SENSITIVE_MOUNT_PREFIXES: &[&str] = &[ + "/etc", + "/root", + "/home", + "/var/run/docker.sock", + "/proc", + "/sys", +]; + +/// Validate a volume mount source path, rejecting paths that reference sensitive host directories. +fn validate_mount_source(mount: &str, mcp_name: &str) { + // Format: "source:dest:mode" + if let Some(source) = mount.split(':').next() { + let source_lower = source.to_lowercase(); + for prefix in SENSITIVE_MOUNT_PREFIXES { + if source_lower.starts_with(prefix) { + log::warn!( + "MCP '{}': mount source '{}' references a sensitive host path ({}). \ + Ensure this is intentional.", + mcp_name, source, prefix + ); + break; + } + } + } +} + +/// Validate that an MCP HTTP URL uses an allowed scheme. +fn validate_mcp_url(url: &str, mcp_name: &str) { + if !url.starts_with("https://") && !url.starts_with("http://") { + log::warn!( + "MCP '{}': URL '{}' does not use http:// or https:// scheme. \ + This may not work with MCPG.", + mcp_name, url + ); + } +} + /// Validate that a string is a legal environment variable name (`[A-Za-z_][A-Za-z0-9_]*`). /// Prevents injection of arbitrary Docker flags via user-controlled front matter keys. fn is_valid_env_var_name(name: &str) -> bool { @@ -589,6 +633,10 @@ fn is_valid_env_var_name(name: &str) -> bool { /// the pipeline through the MCPG container (passthrough). This function: /// 1. Auto-maps `AZURE_DEVOPS_EXT_PAT` from `SC_READ_TOKEN` when `permissions.read` is configured /// 2. Collects all passthrough env vars (value is `""`) from MCP configs and adds `-e` flags +/// +/// Returns flags formatted for inline insertion in the `docker run` command. +/// The marker sits after the last hardcoded `-e` flag, so the output must +/// include leading `\\\n` for line continuation when non-empty. pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { let mut env_flags: Vec = Vec::new(); let mut seen: std::collections::HashSet = std::collections::HashSet::new(); @@ -631,9 +679,14 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { env_flags.sort(); if env_flags.is_empty() { - String::new() + // No extra flags — just emit the line continuation backslash + "\\".to_string() } else { - format!("\\\n {}", env_flags.join(" \\\n ")) + // Emit each flag on its own continuation line, ending with `\` + // replace_with_indent will NOT add indentation here because the marker + // is inline (not at the start of a line), so we include indentation ourselves. + let flags = env_flags.join(" \\\n "); + format!("\\\n {} \\", flags) } } diff --git a/templates/base.yml b/templates/base.yml index 005fd67f..0f1bcdfd 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -263,8 +263,7 @@ jobs: --name mcpg \ --network host \ -v /var/run/docker.sock:/var/run/docker.sock \ - -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" \ - {{ mcpg_docker_env }} + -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" {{ mcpg_docker_env }} ghcr.io/github/gh-aw-mcpg:v{{ mcpg_version }} & MCPG_PID=$! echo "MCPG started (PID: $MCPG_PID)" From d267c41d3eb3477c2cfdbb77a9132c2ed34113d7 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 12:01:54 +0100 Subject: [PATCH 04/10] fix: skip HTTP MCPs in env passthrough, warn on container+url conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Skip HTTP MCPs in generate_mcpg_docker_env (no child container to forward env vars to) - Warn when both container and url are set on same MCP entry - Escalate docker.sock mount warning to eprintln (container escape risk) - Remove undocumented ${VAR} passthrough — only empty string ("") triggers env passthrough, matching AGENTS.md documentation - Add indentation cross-reference comment for base.yml alignment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 43 +++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index c3ff9e58..3cc94b69 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -477,6 +477,14 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } if let Some(opts) = options { + if opts.container.is_some() && opts.url.is_some() { + log::warn!( + "MCP '{}': both 'container' and 'url' are set — using 'container' (stdio). \ + Remove 'url' to silence this warning.", + name + ); + } + if let Some(container) = &opts.container { // Container-based stdio MCP (MCPG-native, per spec §3.2.1) // Validate mount paths for sensitive host directories @@ -588,15 +596,25 @@ const SENSITIVE_MOUNT_PREFIXES: &[&str] = &[ "/sys", ]; -/// Validate a volume mount source path, rejecting paths that reference sensitive host directories. +/// Validate a volume mount source path, warning on sensitive host directories. +/// Docker socket mounts are escalated to stderr warnings since they grant container escape. fn validate_mount_source(mount: &str, mcp_name: &str) { // Format: "source:dest:mode" if let Some(source) = mount.split(':').next() { let source_lower = source.to_lowercase(); + // Docker socket is especially dangerous — escalate to stderr + if source_lower.contains("docker.sock") { + eprintln!( + "Warning: MCP '{}': mount '{}' exposes the Docker socket to the MCP container. \ + This grants full host Docker access and may allow container escape.", + mcp_name, mount + ); + return; + } for prefix in SENSITIVE_MOUNT_PREFIXES { if source_lower.starts_with(prefix) { - log::warn!( - "MCP '{}': mount source '{}' references a sensitive host path ({}). \ + eprintln!( + "Warning: MCP '{}': mount source '{}' references a sensitive host path ({}). \ Ensure this is intentional.", mcp_name, source, prefix ); @@ -632,7 +650,10 @@ fn is_valid_env_var_name(name: &str) -> bool { /// MCP containers spawned by MCPG may need environment variables that flow from /// the pipeline through the MCPG container (passthrough). This function: /// 1. Auto-maps `AZURE_DEVOPS_EXT_PAT` from `SC_READ_TOKEN` when `permissions.read` is configured -/// 2. Collects all passthrough env vars (value is `""`) from MCP configs and adds `-e` flags +/// 2. Collects passthrough env vars (value is `""`) from container-based MCP configs +/// +/// Only container-based MCPs are considered — HTTP MCPs don't have child containers +/// that need env passthrough. /// /// Returns flags formatted for inline insertion in the `docker run` command. /// The marker sits after the last hardcoded `-e` flag, so the output must @@ -649,13 +670,19 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { seen.insert("AZURE_DEVOPS_EXT_PAT".to_string()); } - // Collect passthrough env vars from all MCP configs + // Collect passthrough env vars from container-based MCP configs only. + // HTTP MCPs don't have child containers — env passthrough doesn't apply. for (mcp_name, config) in &front_matter.mcp_servers { let opts = match config { McpConfig::WithOptions(opts) if opts.enabled.unwrap_or(true) => opts, _ => continue, }; + // Only container-based MCPs need env passthrough on the MCPG Docker run + if opts.container.is_none() { + continue; + } + for (var_name, var_value) in &opts.env { // Validate env var name to prevent Docker flag injection (e.g. "X --privileged") if !is_valid_env_var_name(var_name) { @@ -668,9 +695,8 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { if seen.contains(var_name) { continue; } - // Passthrough: empty string means forward from host environment - // Expansion: ${VAR} means MCPG resolves from its own environment - if var_value.is_empty() || var_value.contains("${") { + // Passthrough: empty string means forward from host/pipeline environment + if var_value.is_empty() { env_flags.push(format!("-e {}", var_name)); seen.insert(var_name.clone()); } @@ -685,6 +711,7 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { // Emit each flag on its own continuation line, ending with `\` // replace_with_indent will NOT add indentation here because the marker // is inline (not at the start of a line), so we include indentation ourselves. + // NOTE: the 12-space indentation must match the `docker run` block in base.yml let flags = env_flags.join(" \\\n "); format!("\\\n {} \\", flags) } From 9e073e851fded5e7686b1c30a92333c379adabff Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 12:12:36 +0100 Subject: [PATCH 05/10] fix: validate Docker args, warn on inline secrets, consistent eprintln MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add validate_docker_args() — scans args for --privileged, -v, --cap-add, --security-opt and other privilege escalation flags - Add warn_potential_secrets() — warns when env var names containing 'token', 'secret', 'key', 'password', 'pat' have inline values instead of passthrough; warns on Authorization headers in plaintext - Change validate_mcp_url to use eprintln! for consistency with other compile-time warnings (was log::warn, invisible without --verbose) - Change HTTP MCP env warning to eprintln! for consistency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 91 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 4 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 3cc94b69..9c49e262 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -491,6 +491,10 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { for mount in &opts.mounts { validate_mount_source(mount, name); } + // Validate Docker runtime args for privilege escalation + validate_docker_args(&opts.args, name); + // Warn about potential inline secrets + warn_potential_secrets(name, &opts.env, &HashMap::new()); let entrypoint_args = if opts.entrypoint_args.is_empty() { None } else { @@ -534,9 +538,11 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { } else if let Some(url) = &opts.url { // HTTP-based MCP (remote server) validate_mcp_url(url, name); + // Warn about potential inline secrets in headers + warn_potential_secrets(name, &HashMap::new(), &opts.headers); if !opts.env.is_empty() { - log::warn!( - "MCP '{}': env vars are not supported for HTTP MCPs — they will be ignored. \ + eprintln!( + "Warning: MCP '{}': env vars are not supported for HTTP MCPs — they will be ignored. \ Use headers for authentication instead.", name ); @@ -596,6 +602,16 @@ const SENSITIVE_MOUNT_PREFIXES: &[&str] = &[ "/sys", ]; +/// Docker runtime args that grant dangerous host access. +const DANGEROUS_DOCKER_ARGS: &[&str] = &[ + "--privileged", + "--cap-add", + "--security-opt", + "--pid=host", + "--network=host", + "--ipc=host", +]; + /// Validate a volume mount source path, warning on sensitive host directories. /// Docker socket mounts are escalated to stderr warnings since they grant container escape. fn validate_mount_source(mount: &str, mcp_name: &str) { @@ -624,17 +640,84 @@ fn validate_mount_source(mount: &str, mcp_name: &str) { } } +/// Validate Docker runtime args for dangerous flags that could escalate privileges. +/// Also detects volume mounts smuggled via `-v`/`--volume` that bypass `mounts` validation. +fn validate_docker_args(args: &[String], mcp_name: &str) { + for (i, arg) in args.iter().enumerate() { + let arg_lower = arg.to_lowercase(); + // Check for dangerous Docker flags + for dangerous in DANGEROUS_DOCKER_ARGS { + if arg_lower == *dangerous || arg_lower.starts_with(&format!("{}=", dangerous)) { + eprintln!( + "Warning: MCP '{}': Docker arg '{}' grants elevated privileges. \ + Ensure this is intentional.", + mcp_name, arg + ); + } + } + // Check for volume mounts smuggled via args (bypasses mounts validation) + if arg == "-v" || arg == "--volume" { + if let Some(mount_spec) = args.get(i + 1) { + eprintln!( + "Warning: MCP '{}': volume mount '{}' in args bypasses mounts validation. \ + Use the 'mounts:' field instead.", + mcp_name, mount_spec + ); + validate_mount_source(mount_spec, mcp_name); + } + } else if arg_lower.starts_with("-v=") || arg_lower.starts_with("--volume=") { + let mount_spec = arg.splitn(2, '=').nth(1).unwrap_or(""); + eprintln!( + "Warning: MCP '{}': volume mount '{}' in args bypasses mounts validation. \ + Use the 'mounts:' field instead.", + mcp_name, mount_spec + ); + validate_mount_source(mount_spec, mcp_name); + } + } +} + /// Validate that an MCP HTTP URL uses an allowed scheme. fn validate_mcp_url(url: &str, mcp_name: &str) { if !url.starts_with("https://") && !url.starts_with("http://") { - log::warn!( - "MCP '{}': URL '{}' does not use http:// or https:// scheme. \ + eprintln!( + "Warning: MCP '{}': URL '{}' does not use http:// or https:// scheme. \ This may not work with MCPG.", mcp_name, url ); } } +/// Warn when env values or headers look like they contain inline secrets. +/// Secrets should use pipeline variables and passthrough ("") instead. +fn warn_potential_secrets(mcp_name: &str, env: &HashMap, headers: &HashMap) { + for (key, value) in env { + if !value.is_empty() && (key.to_lowercase().contains("token") + || key.to_lowercase().contains("secret") + || key.to_lowercase().contains("key") + || key.to_lowercase().contains("password") + || key.to_lowercase().contains("pat")) + { + eprintln!( + "Warning: MCP '{}': env var '{}' has an inline value that may be a secret. \ + Use an empty string (\"\") for passthrough from pipeline variables instead.", + mcp_name, key + ); + } + } + for (key, value) in headers { + if value.to_lowercase().contains("bearer ") + || key.to_lowercase() == "authorization" + { + eprintln!( + "Warning: MCP '{}': header '{}' may contain inline credentials. \ + These will appear in plaintext in the compiled pipeline YAML.", + mcp_name, key + ); + } + } +} + /// Validate that a string is a legal environment variable name (`[A-Za-z_][A-Za-z0-9_]*`). /// Prevents injection of arbitrary Docker flags via user-controlled front matter keys. fn is_valid_env_var_name(name: &str) -> bool { From ebc87cc8c47a6c0d4be9e0e866c155220a7fbaa6 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 12:26:06 +0100 Subject: [PATCH 06/10] fix: tighten validation, scope ADO token auto-map to requesting MCPs - Remove dead /var/run/docker.sock from SENSITIVE_MOUNT_PREFIXES (already caught by contains("docker.sock") early return) - Fix prefix check: /etc no longer matches /etc-configs (require exact match or trailing /) - Fix Docker args: handle split-form flags (--pid host, --network host, --ipc host) in addition to --flag=value form - Scope ADO token auto-map: only inject AZURE_DEVOPS_EXT_PAT when a container MCP actually requests it via env passthrough, not whenever permissions.read is set - Add test for dedup edge case: auto-mapped SC_READ_TOKEN form wins over bare passthrough, appears exactly once - Add test for no-request case: permissions.read without MCP requesting AZURE_DEVOPS_EXT_PAT does not inject the token Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 108 +++++++++++++++++++++++++++++++++----- tests/compiler_tests.rs | 2 +- 2 files changed, 96 insertions(+), 14 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 9c49e262..bed79a9a 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -597,19 +597,19 @@ const SENSITIVE_MOUNT_PREFIXES: &[&str] = &[ "/etc", "/root", "/home", - "/var/run/docker.sock", "/proc", "/sys", ]; -/// Docker runtime args that grant dangerous host access. -const DANGEROUS_DOCKER_ARGS: &[&str] = &[ +/// Docker runtime flag names that grant dangerous host access. +/// Checked both as `--flag=value` and as `--flag value` (split across two args). +const DANGEROUS_DOCKER_FLAGS: &[&str] = &[ "--privileged", "--cap-add", "--security-opt", - "--pid=host", - "--network=host", - "--ipc=host", + "--pid", + "--network", + "--ipc", ]; /// Validate a volume mount source path, warning on sensitive host directories. @@ -618,7 +618,6 @@ fn validate_mount_source(mount: &str, mcp_name: &str) { // Format: "source:dest:mode" if let Some(source) = mount.split(':').next() { let source_lower = source.to_lowercase(); - // Docker socket is especially dangerous — escalate to stderr if source_lower.contains("docker.sock") { eprintln!( "Warning: MCP '{}': mount '{}' exposes the Docker socket to the MCP container. \ @@ -628,7 +627,9 @@ fn validate_mount_source(mount: &str, mcp_name: &str) { return; } for prefix in SENSITIVE_MOUNT_PREFIXES { - if source_lower.starts_with(prefix) { + // Match exact path or path with trailing separator to avoid false positives + // (e.g. /etc matches /etc and /etc/shadow, but not /etc-configs) + if source_lower == *prefix || source_lower.starts_with(&format!("{}/", prefix)) { eprintln!( "Warning: MCP '{}': mount source '{}' references a sensitive host path ({}). \ Ensure this is intentional.", @@ -642,12 +643,15 @@ fn validate_mount_source(mount: &str, mcp_name: &str) { /// Validate Docker runtime args for dangerous flags that could escalate privileges. /// Also detects volume mounts smuggled via `-v`/`--volume` that bypass `mounts` validation. +/// Handles both `--flag=value` and `--flag value` (split) forms. fn validate_docker_args(args: &[String], mcp_name: &str) { for (i, arg) in args.iter().enumerate() { let arg_lower = arg.to_lowercase(); - // Check for dangerous Docker flags - for dangerous in DANGEROUS_DOCKER_ARGS { - if arg_lower == *dangerous || arg_lower.starts_with(&format!("{}=", dangerous)) { + // Check for dangerous Docker flags (both --flag=value and --flag value) + for dangerous in DANGEROUS_DOCKER_FLAGS { + if arg_lower == *dangerous + || arg_lower.starts_with(&format!("{}=", dangerous)) + { eprintln!( "Warning: MCP '{}': Docker arg '{}' grants elevated privileges. \ Ensure this is intentional.", @@ -745,8 +749,19 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { let mut env_flags: Vec = Vec::new(); let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + // Check if any container MCP requests AZURE_DEVOPS_EXT_PAT passthrough + let any_mcp_needs_ado_token = front_matter.mcp_servers.values().any(|config| { + matches!(config, McpConfig::WithOptions(opts) + if opts.enabled.unwrap_or(true) + && opts.container.is_some() + && opts.env.contains_key("AZURE_DEVOPS_EXT_PAT")) + }); + // Auto-map AZURE_DEVOPS_EXT_PAT from SC_READ_TOKEN when permissions.read is configured - if front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() { + // AND at least one container MCP requests it via env passthrough + if any_mcp_needs_ado_token + && front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() + { env_flags.push( "-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\"".to_string(), ); @@ -1184,11 +1199,78 @@ mod tests { read: Some("my-read-sc".to_string()), write: None, }); + // A container MCP must request AZURE_DEVOPS_EXT_PAT for the auto-map to trigger + fm.mcp_servers.insert( + "ado-tool".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("node:20-slim".to_string()), + env: { + let mut e = HashMap::new(); + e.insert("AZURE_DEVOPS_EXT_PAT".to_string(), "".to_string()); + e + }, + ..Default::default() + }), + ); + let env = generate_mcpg_docker_env(&fm); + assert!( + env.contains("-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\""), + "Should auto-map ADO token when permissions.read is set and MCP requests it" + ); + } + + #[test] + fn test_generate_mcpg_docker_env_permissions_read_no_mcp_request() { + let mut fm = minimal_front_matter(); + fm.permissions = Some(crate::compile::types::PermissionsConfig { + read: Some("my-read-sc".to_string()), + write: None, + }); + // No MCP requests AZURE_DEVOPS_EXT_PAT — auto-map should NOT trigger + fm.mcp_servers.insert( + "unrelated-tool".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("node:20-slim".to_string()), + ..Default::default() + }), + ); + let env = generate_mcpg_docker_env(&fm); + assert!( + !env.contains("AZURE_DEVOPS_EXT_PAT"), + "Should NOT auto-map ADO token when no MCP requests it" + ); + } + + #[test] + fn test_generate_mcpg_docker_env_dedup_auto_map_and_passthrough() { + // When permissions.read is set AND MCP has AZURE_DEVOPS_EXT_PAT: "", + // the auto-mapped form (with SC_READ_TOKEN) should win — no duplicate + let mut fm = minimal_front_matter(); + fm.permissions = Some(crate::compile::types::PermissionsConfig { + read: Some("my-read-sc".to_string()), + write: None, + }); + fm.mcp_servers.insert( + "ado-tool".to_string(), + McpConfig::WithOptions(McpOptions { + container: Some("node:20-slim".to_string()), + env: { + let mut e = HashMap::new(); + e.insert("AZURE_DEVOPS_EXT_PAT".to_string(), "".to_string()); + e + }, + ..Default::default() + }), + ); let env = generate_mcpg_docker_env(&fm); + // Should have the SC_READ_TOKEN form (auto-mapped), not bare passthrough assert!( env.contains("-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\""), - "Should auto-map ADO token when permissions.read is set" + "Auto-mapped form should be present" ); + // Should appear exactly once + let count = env.matches("AZURE_DEVOPS_EXT_PAT").count(); + assert_eq!(count, 1, "AZURE_DEVOPS_EXT_PAT should appear exactly once, got {}", count); } #[test] diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 40c4fa62..500f22a5 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -2189,7 +2189,7 @@ fn test_mcpg_docker_env_passthrough() { )); fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); - let input = "---\nname: \"Env Test\"\ndescription: \"Tests env passthrough\"\npermissions:\n read: my-read-sc\n write: my-write-sc\nmcp-servers:\n my-tool:\n container: \"node:20-slim\"\n env:\n MY_TOKEN: \"\"\n STATIC_VAR: \"static-value\"\nsafe-outputs:\n create-work-item:\n work-item-type: Task\n---\n\n## Test\n"; + let input = "---\nname: \"Env Test\"\ndescription: \"Tests env passthrough\"\npermissions:\n read: my-read-sc\n write: my-write-sc\nmcp-servers:\n my-tool:\n container: \"node:20-slim\"\n env:\n AZURE_DEVOPS_EXT_PAT: \"\"\n MY_TOKEN: \"\"\n STATIC_VAR: \"static-value\"\nsafe-outputs:\n create-work-item:\n work-item-type: Task\n---\n\n## Test\n"; let input_path = temp_dir.join("env-passthrough.md"); let output_path = temp_dir.join("env-passthrough.yml"); From 9727e77861ce50af121b5b6bd5da70c85557b558 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 12:42:51 +0100 Subject: [PATCH 07/10] refactor: use replace_with_indent for mcpg_docker_env indentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move {{ mcpg_docker_env }} to its own indented line in base.yml so replace_with_indent handles alignment automatically. Removes hardcoded 12-space indentation from generate_mcpg_docker_env() — future template reformats won't silently break bash syntax. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 13 ++++++------- templates/base.yml | 3 ++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index f5871d85..2b582428 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -808,15 +808,14 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { env_flags.sort(); if env_flags.is_empty() { - // No extra flags — just emit the line continuation backslash + // No extra flags — the template line is replaced with just a line continuation "\\".to_string() } else { - // Emit each flag on its own continuation line, ending with `\` - // replace_with_indent will NOT add indentation here because the marker - // is inline (not at the start of a line), so we include indentation ourselves. - // NOTE: the 12-space indentation must match the `docker run` block in base.yml - let flags = env_flags.join(" \\\n "); - format!("\\\n {} \\", flags) + // Emit each flag on its own line with `\` continuation. + // replace_with_indent handles indentation from the template (base.yml), + // so we only emit the content without hardcoded spaces. + let flags = env_flags.join(" \\\n"); + format!("{} \\", flags) } } diff --git a/templates/base.yml b/templates/base.yml index c49b94f1..a99118f0 100644 --- a/templates/base.yml +++ b/templates/base.yml @@ -267,7 +267,8 @@ jobs: --name mcpg \ --network host \ -v /var/run/docker.sock:/var/run/docker.sock \ - -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" {{ mcpg_docker_env }} + -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" \ + {{ mcpg_docker_env }} ghcr.io/github/gh-aw-mcpg:v{{ mcpg_version }} & MCPG_PID=$! echo "MCPG started (PID: $MCPG_PID)" From 9cc9caea4b632662ff4cd78af3a6dd85080c1b06 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 12:56:25 +0100 Subject: [PATCH 08/10] fix: container image validation, expand Docker flag list, fix headers arg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add validate_container_image() — rejects image names with unexpected characters (defense-in-depth against injection) - Expand DANGEROUS_DOCKER_FLAGS with --user/-u, --add-host, --entrypoint - Fix warn_potential_secrets call: pass &opts.headers instead of empty HashMap for container MCPs - Add inline comment explaining lone backslash in empty env case - Add case-insensitivity note on validate_mount_source Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 2b582428..dd1c5706 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -492,14 +492,15 @@ pub fn generate_mcpg_config(front_matter: &FrontMatter) -> McpgConfig { if let Some(container) = &opts.container { // Container-based stdio MCP (MCPG-native, per spec §3.2.1) + validate_container_image(container, name); // Validate mount paths for sensitive host directories for mount in &opts.mounts { validate_mount_source(mount, name); } // Validate Docker runtime args for privilege escalation validate_docker_args(&opts.args, name); - // Warn about potential inline secrets - warn_potential_secrets(name, &opts.env, &HashMap::new()); + // Warn about potential inline secrets (check headers too in case user set both) + warn_potential_secrets(name, &opts.env, &opts.headers); let entrypoint_args = if opts.entrypoint_args.is_empty() { None } else { @@ -615,10 +616,31 @@ const DANGEROUS_DOCKER_FLAGS: &[&str] = &[ "--pid", "--network", "--ipc", + "--user", + "-u", + "--add-host", + "--entrypoint", ]; +/// Validate a container image name for injection attempts. +/// Allows `[a-zA-Z0-9./_:-]` which covers standard Docker image references. +fn validate_container_image(image: &str, mcp_name: &str) { + if image.is_empty() { + eprintln!("Warning: MCP '{}': container image name is empty.", mcp_name); + return; + } + if !image.chars().all(|c| c.is_ascii_alphanumeric() || "._/:-@".contains(c)) { + eprintln!( + "Warning: MCP '{}': container image '{}' contains unexpected characters. \ + Image names should only contain [a-zA-Z0-9./_:-@].", + mcp_name, image + ); + } +} + /// Validate a volume mount source path, warning on sensitive host directories. /// Docker socket mounts are escalated to stderr warnings since they grant container escape. +/// Note: paths are lowercased for comparison to catch cross-platform casing (e.g. `/ETC/shadow`). fn validate_mount_source(mount: &str, mcp_name: &str) { // Format: "source:dest:mode" if let Some(source) = mount.split(':').next() { @@ -808,7 +830,10 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { env_flags.sort(); if env_flags.is_empty() { - // No extra flags — the template line is replaced with just a line continuation + // No extra flags — emit a lone `\` so the bash line continuation from the + // preceding `-e MCP_GATEWAY_API_KEY=...` flag connects to the image name on + // the next line. This is valid bash: a backslash at end-of-line continues + // the command. replace_with_indent preserves this on its own indented line. "\\".to_string() } else { // Emit each flag on its own line with `\` continuation. From 9f75801f585385530e70a70c48dacb4cb9113221 Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 13:08:23 +0100 Subject: [PATCH 09/10] fix: warn on missing permissions.read when MCP requests ADO token - Emit eprintln warning when container MCP has AZURE_DEVOPS_EXT_PAT passthrough but permissions.read is not configured (token would be empty at runtime, causing silent auth failure) - Remove --network host from AGENTS.md args example (contradicts DANGEROUS_DOCKER_FLAGS warning, bypasses AWF proxy) - Add entrypoint: field hint to --entrypoint Docker args warning Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 2 +- src/compile/standalone.rs | 29 ++++++++++++++++++++--------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f3b1b75e..011591f7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1184,7 +1184,7 @@ mcp-servers: - `container:` - Docker image to run (e.g., `"node:20-slim"`, `"ghcr.io/org/tool:latest"`) - `entrypoint:` - Container entrypoint override (equivalent to `docker run --entrypoint`) - `entrypoint-args:` - Arguments passed to the entrypoint (after the image in `docker run`) -- `args:` - Additional Docker runtime arguments (inserted before the image, e.g., `["--network", "host"]`) +- `args:` - Additional Docker runtime arguments (inserted before the image in `docker run`). **Security note**: dangerous flags like `--privileged`, `--network host` will trigger compile-time warnings. - `mounts:` - Volume mounts in `"source:dest:mode"` format (e.g., `["/host/data:/app/data:ro"]`) **HTTP servers:** diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index dd1c5706..044620b7 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -679,10 +679,15 @@ fn validate_docker_args(args: &[String], mcp_name: &str) { if arg_lower == *dangerous || arg_lower.starts_with(&format!("{}=", dangerous)) { + let extra_hint = if *dangerous == "--entrypoint" { + " Use the 'entrypoint:' field instead of passing --entrypoint in args." + } else { + "" + }; eprintln!( "Warning: MCP '{}': Docker arg '{}' grants elevated privileges. \ - Ensure this is intentional.", - mcp_name, arg + Ensure this is intentional.{}", + mcp_name, arg, extra_hint ); } } @@ -786,13 +791,19 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { // Auto-map AZURE_DEVOPS_EXT_PAT from SC_READ_TOKEN when permissions.read is configured // AND at least one container MCP requests it via env passthrough - if any_mcp_needs_ado_token - && front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() - { - env_flags.push( - "-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\"".to_string(), - ); - seen.insert("AZURE_DEVOPS_EXT_PAT".to_string()); + if any_mcp_needs_ado_token { + if let Some(_) = front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()) { + env_flags.push( + "-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\"".to_string(), + ); + seen.insert("AZURE_DEVOPS_EXT_PAT".to_string()); + } else { + eprintln!( + "Warning: one or more container MCPs request AZURE_DEVOPS_EXT_PAT passthrough \ + but permissions.read is not configured. The token will be empty at runtime. \ + Add `permissions: {{ read: }}` to enable auto-mapping." + ); + } } // Collect passthrough env vars from container-based MCP configs only. From 7db2a44e18d9ae35d139206668331aa7b364757d Mon Sep 17 00:00:00 2001 From: James Devine Date: Mon, 13 Apr 2026 13:19:39 +0100 Subject: [PATCH 10/10] style: simplify if-let Some(_) to .is_some() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/compile/standalone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compile/standalone.rs b/src/compile/standalone.rs index 044620b7..f38761c1 100644 --- a/src/compile/standalone.rs +++ b/src/compile/standalone.rs @@ -792,7 +792,7 @@ pub fn generate_mcpg_docker_env(front_matter: &FrontMatter) -> String { // Auto-map AZURE_DEVOPS_EXT_PAT from SC_READ_TOKEN when permissions.read is configured // AND at least one container MCP requests it via env passthrough if any_mcp_needs_ado_token { - if let Some(_) = front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()) { + if front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() { env_flags.push( "-e AZURE_DEVOPS_EXT_PAT=\"$(SC_READ_TOKEN)\"".to_string(), );