From b9503b866313e8e8fe9ee82c3945148843a52947 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Tue, 18 Aug 2026 12:22:10 +0200 Subject: [PATCH] feat(server): let the environment set allowed hosts and the auth header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Container deployments configure bugwarden entirely through the environment (#31, #32), and two flags were still command line only: --allowed-hosts deliberately so, --use-auth-header by omission. Both now carry an env fallback, MCP_ALLOWED_HOSTS and BUGZILLA_USE_AUTH_HEADER. One variable has to carry the whole host list, so Cli::resolved_allowed_hosts splits every entry on commas and whitespace and drops the empty ones. That drop is load-bearing: rmcp's host_is_allowed skips an entry it cannot parse and matches against what is left, so the single empty entry MCP_ALLOWED_HOSTS= produces would switch validation on with nothing matchable and refuse every Host. Dropping it makes MCP_ALLOWED_HOSTS= read as unset instead, the same convention BUGZILLA_API_KEY_FILE= already follows. Environment-backed flags leak into test processes, so the http harness now neutralises allowed_hosts like the two key fields, and the binary_user_agent scrub list is pinned by a sweep test that fails when any env-bearing flag is missing from it — which promptly exposed MCP_HOST, MCP_PORT and MCP_API_KEY_HEADER as pre-existing gaps, now closed. DESIGN.md records the reversal of the command-line-only decision rather than deleting it, and its Testing list gains the new env_config binary (one test by construction: it mutates the process environment). A unit test fails the moment any flag ships without an env fallback. Man page and completions regenerated. --- README.md | 15 +-- crates/bugwarden/completions/_bugwarden | 4 +- crates/bugwarden/completions/bugwarden.fish | 4 +- crates/bugwarden/man/bugwarden.1 | 10 +- crates/bugwarden/src/config.rs | 102 +++++++++++++++- crates/bugwarden/src/server.rs | 13 +- crates/bugwarden/tests/binary_user_agent.rs | 58 +++++++-- crates/bugwarden/tests/env_config.rs | 111 ++++++++++++++++++ .../tests/http_transport_wiremock.rs | 53 ++++++++- docs/DESIGN.md | 37 +++++- 10 files changed, 363 insertions(+), 44 deletions(-) create mode 100644 crates/bugwarden/tests/env_config.rs diff --git a/README.md b/README.md index 98068f2..33a5348 100644 --- a/README.md +++ b/README.md @@ -353,21 +353,22 @@ Command-line arguments take precedence over environment variables. | `--transport ` | `MCP_TRANSPORT` | `http` | MCP transport. `stdio` is for subprocess launches by an MCP client; `http` exposes a network endpoint at `/mcp` | | `--host
` | `MCP_HOST` | `127.0.0.1` | Listen address (http transport only) | | `--port ` | `MCP_PORT` | `8000` | Listen port (http transport only) | -| `--allowed-hosts ` | — | — | Hostname or `host:port` authority accepted in an inbound `Host` header (http transport only). Repeatable; each occurrence adds one host. Without it no `Host` validation happens, so a client may address the server by any name | +| `--allowed-hosts ` | `MCP_ALLOWED_HOSTS` | — | Hostname or `host:port` authority accepted in an inbound `Host` header (http transport only). Repeatable, and each value may list several hosts separated by commas and/or whitespace. Without any host no `Host` validation happens, so a client may address the server by any name | | `--api-key-header ` | `MCP_API_KEY_HEADER` | `ApiKey` | HTTP header name in which clients send the Bugzilla API key (http transport only). Not consulted in server-held key mode | | `--api-key ` | `BUGZILLA_API_KEY` | — | Bugzilla API key. **Required** for `--transport stdio` unless `--api-key-file` provides it; with `http` it is ignored with a warning (clients send the key per request — use `--api-key-file` for a server-held key) | | `--api-key-file ` | `BUGZILLA_API_KEY_FILE` | — | Path to a file holding the Bugzilla API key (container secret, systemd `LoadCredential` path). Mutually exclusive with `--api-key`; an empty value counts as unset. Over `http` this selects server-held key mode: every request is served with this key and the per-request header is not consulted | -| `--use-auth-header` | — | `false` | Authenticate to Bugzilla with `Authorization: Bearer ` instead of the `api_key` query parameter | +| `--use-auth-header` | `BUGZILLA_USE_AUTH_HEADER` | `false` | Authenticate to Bugzilla with `Authorization: Bearer ` instead of the `api_key` query parameter. As an environment variable it takes the literal `true` or `false`, exactly like `MCP_READ_ONLY` | | `--read-only` | `MCP_READ_ONLY` | `false` | Disable all write tools. Tighten-only: ORed with the policy's `global.read_only`; cannot re-enable writes a policy forbids. As an environment variable it takes the literal `true` or `false` — `1`, `yes` and an empty value are a usage error, not a synonym | | `--policy ` | `BUGWARDEN_POLICY` | — | Path to the guard policy TOML. Without it, an allow-all policy applies (with private comments off and the 2 MiB attachment cap still in force) | | `--audit-config ` | `BUGWARDEN_AUDIT_CONFIG` | — | Path to the audit stream configuration TOML (worked example in [`examples/audit.toml`](examples/audit.toml)). Without it, no audit stream is written. Records carry W3C trace ids when the client sends a `traceparent` in the request's `_meta`, enabling correlation with client-side traces | | — | `RUST_LOG` | `info` | Tracing filter for the diagnostic log, which always goes to **stderr** — stdout belongs to the stdio transport. An unparsable value falls back to `info` | -An empty value counts as unset for `--api-key` and `--api-key-file` only, so -`BUGZILLA_API_KEY_FILE=` in a unit file leaves the two key modes unaffected -rather than erroring (under stdio it then leaves no key source at all, which -is a startup error of its own). An empty `BUGWARDEN_POLICY` or -`BUGWARDEN_AUDIT_CONFIG` is a usage error. +An empty value counts as unset for `--api-key`, `--api-key-file` and +`--allowed-hosts`, so `BUGZILLA_API_KEY_FILE=` in a unit file leaves the two +key modes unaffected rather than erroring (under stdio it then leaves no key +source at all, which is a startup error of its own), and `MCP_ALLOWED_HOSTS=` +names no host, leaving `Host` validation off as if it were never set. An empty +`BUGWARDEN_POLICY` or `BUGWARDEN_AUDIT_CONFIG` is a usage error. Exit status: `0` on clean shutdown, `1` on a startup or runtime failure (an unreadable policy or audit configuration, a key misconfiguration, a Bugzilla diff --git a/crates/bugwarden/completions/_bugwarden b/crates/bugwarden/completions/_bugwarden index 654a935..c335dab 100644 --- a/crates/bugwarden/completions/_bugwarden +++ b/crates/bugwarden/completions/_bugwarden @@ -20,13 +20,13 @@ _bugwarden() { stdio\:"Stdio transport. The API key comes from \`--api-key\` / \`BUGZILLA_API_KEY\` or \`--api-key-file\` at startup"))' \ '--host=[Host address for the MCP server to listen on (http transport only). Defaults to 127.0.0.1 or the MCP_HOST environment variable]:HOST:_default' \ '--port=[Port for the MCP server to listen on (http transport only). Defaults to 8000 or the MCP_PORT environment variable]:PORT:_default' \ -'*--allowed-hosts=[Hostname or '\''host\:port'\'' authority accepted in an inbound Host header (http transport only). Repeat the flag to allow further hosts. Command line only, no environment variable; without it Host validation stays off and any Host header is served]:HOST:_default' \ +'*--allowed-hosts=[Hostname or '\''host\:port'\'' authority accepted in an inbound Host header (http transport only). Repeat the flag, or separate entries with commas or whitespace; environment variable MCP_ALLOWED_HOSTS can also be used. Empty entries are dropped, so \`MCP_ALLOWED_HOSTS=\` is an unset (like \`BUGZILLA_API_KEY_FILE=\`); without a host, Host validation stays off and any Host header is served]:HOST:_default' \ '--api-key-header=[HTTP header for clients to send the Bugzilla API key. Defaults to '\''ApiKey'\'' or the MCP_API_KEY_HEADER environment variable. Not consulted in server-held key mode (--api-key-file over http)]:API_KEY_HEADER:_default' \ '--api-key=[Bugzilla API key. Required for --transport stdio (no HTTP headers exist there) unless --api-key-file provides it. Environment variable BUGZILLA_API_KEY can also be used. Ignored for --transport http (clients send the key per-request via the API key header; use --api-key-file for a server-held key)]:API_KEY:_default' \ '--api-key-file=[Path to a file holding the Bugzilla API key (e.g. a container secret or systemd LoadCredential path). Mutually exclusive with --api-key. Over http this selects server-held key mode\: every request is served with this key and the per-request API key header is not consulted. An empty value counts as absent, like --api-key (so \`BUGZILLA_API_KEY_FILE=\` is an unset, not an error)]:API_KEY_FILE:_files' \ '--policy=[Path to the guard policy TOML file. Environment variable BUGWARDEN_POLICY can also be used. Without it an allow-all default policy is used]:POLICY:_files' \ '--audit-config=[Path to the audit configuration TOML file (see examples/audit.toml). Environment variable BUGWARDEN_AUDIT_CONFIG can also be used. Without it no audit stream is written]:AUDIT_CONFIG:_files' \ -'--use-auth-header[Use '\''Authorization\: Bearer'\'' header instead of the api_key query parameter (required for some Bugzilla instances)]' \ +'--use-auth-header[Use '\''Authorization\: Bearer'\'' header instead of the api_key query parameter (required for some Bugzilla instances). Environment variable BUGZILLA_USE_AUTH_HEADER=true can also be used]' \ '--read-only[Disables all tools which modify the state of a bug. Environment variable MCP_READ_ONLY=true can also be used. Can only tighten the guard policy, never loosen it]' \ '-h[Print help (see more with '\''--help'\'')]' \ '--help[Print help (see more with '\''--help'\'')]' \ diff --git a/crates/bugwarden/completions/bugwarden.fish b/crates/bugwarden/completions/bugwarden.fish index d478e98..c1adcf3 100644 --- a/crates/bugwarden/completions/bugwarden.fish +++ b/crates/bugwarden/completions/bugwarden.fish @@ -3,13 +3,13 @@ complete -c bugwarden -l transport -d 'Transport for the MCP server: \'http\' (d stdio\t'Stdio transport. The API key comes from `--api-key` / `BUGZILLA_API_KEY` or `--api-key-file` at startup'" complete -c bugwarden -l host -d 'Host address for the MCP server to listen on (http transport only). Defaults to 127.0.0.1 or the MCP_HOST environment variable' -r complete -c bugwarden -l port -d 'Port for the MCP server to listen on (http transport only). Defaults to 8000 or the MCP_PORT environment variable' -r -complete -c bugwarden -l allowed-hosts -d 'Hostname or \'host:port\' authority accepted in an inbound Host header (http transport only). Repeat the flag to allow further hosts. Command line only, no environment variable; without it Host validation stays off and any Host header is served' -r +complete -c bugwarden -l allowed-hosts -d 'Hostname or \'host:port\' authority accepted in an inbound Host header (http transport only). Repeat the flag, or separate entries with commas or whitespace; environment variable MCP_ALLOWED_HOSTS can also be used. Empty entries are dropped, so `MCP_ALLOWED_HOSTS=` is an unset (like `BUGZILLA_API_KEY_FILE=`); without a host, Host validation stays off and any Host header is served' -r complete -c bugwarden -l api-key-header -d 'HTTP header for clients to send the Bugzilla API key. Defaults to \'ApiKey\' or the MCP_API_KEY_HEADER environment variable. Not consulted in server-held key mode (--api-key-file over http)' -r complete -c bugwarden -l api-key -d 'Bugzilla API key. Required for --transport stdio (no HTTP headers exist there) unless --api-key-file provides it. Environment variable BUGZILLA_API_KEY can also be used. Ignored for --transport http (clients send the key per-request via the API key header; use --api-key-file for a server-held key)' -r complete -c bugwarden -l api-key-file -d 'Path to a file holding the Bugzilla API key (e.g. a container secret or systemd LoadCredential path). Mutually exclusive with --api-key. Over http this selects server-held key mode: every request is served with this key and the per-request API key header is not consulted. An empty value counts as absent, like --api-key (so `BUGZILLA_API_KEY_FILE=` is an unset, not an error)' -r -F complete -c bugwarden -l policy -d 'Path to the guard policy TOML file. Environment variable BUGWARDEN_POLICY can also be used. Without it an allow-all default policy is used' -r -F complete -c bugwarden -l audit-config -d 'Path to the audit configuration TOML file (see examples/audit.toml). Environment variable BUGWARDEN_AUDIT_CONFIG can also be used. Without it no audit stream is written' -r -F -complete -c bugwarden -l use-auth-header -d 'Use \'Authorization: Bearer\' header instead of the api_key query parameter (required for some Bugzilla instances)' +complete -c bugwarden -l use-auth-header -d 'Use \'Authorization: Bearer\' header instead of the api_key query parameter (required for some Bugzilla instances). Environment variable BUGZILLA_USE_AUTH_HEADER=true can also be used' complete -c bugwarden -l read-only -d 'Disables all tools which modify the state of a bug. Environment variable MCP_READ_ONLY=true can also be used. Can only tighten the guard policy, never loosen it' complete -c bugwarden -s h -l help -d 'Print help (see more with \'--help\')' complete -c bugwarden -s V -l version -d 'Print version' diff --git a/crates/bugwarden/man/bugwarden.1 b/crates/bugwarden/man/bugwarden.1 index 7c103d6..c3a7df0 100644 --- a/crates/bugwarden/man/bugwarden.1 +++ b/crates/bugwarden/man/bugwarden.1 @@ -50,7 +50,7 @@ Host address for the MCP server to listen on (http transport only). Defaults to Port for the MCP server to listen on (http transport only). Defaults to 8000 or the MCP_PORT environment variable .TP \fB\-\-allowed\-hosts\fR \fI\fR -Hostname or \*(Aqhost:port\*(Aq authority accepted in an inbound Host header (http transport only). Repeat the flag to allow further hosts. Command line only, no environment variable; without it Host validation stays off and any Host header is served +Hostname or \*(Aqhost:port\*(Aq authority accepted in an inbound Host header (http transport only). Repeat the flag, or separate entries with commas or whitespace; environment variable MCP_ALLOWED_HOSTS can also be used. Empty entries are dropped, so `MCP_ALLOWED_HOSTS=` is an unset (like `BUGZILLA_API_KEY_FILE=`); without a host, Host validation stays off and any Host header is served .TP \fB\-\-api\-key\-header\fR \fI\fR [default: ApiKey] HTTP header for clients to send the Bugzilla API key. Defaults to \*(AqApiKey\*(Aq or the MCP_API_KEY_HEADER environment variable. Not consulted in server\-held key mode (\-\-api\-key\-file over http) @@ -62,7 +62,7 @@ Bugzilla API key. Required for \-\-transport stdio (no HTTP headers exist there) Path to a file holding the Bugzilla API key (e.g. a container secret or systemd LoadCredential path). Mutually exclusive with \-\-api\-key. Over http this selects server\-held key mode: every request is served with this key and the per\-request API key header is not consulted. An empty value counts as absent, like \-\-api\-key (so `BUGZILLA_API_KEY_FILE=` is an unset, not an error) .TP \fB\-\-use\-auth\-header\fR -Use \*(AqAuthorization: Bearer\*(Aq header instead of the api_key query parameter (required for some Bugzilla instances) +Use \*(AqAuthorization: Bearer\*(Aq header instead of the api_key query parameter (required for some Bugzilla instances). Environment variable BUGZILLA_USE_AUTH_HEADER=true can also be used .TP \fB\-\-read\-only\fR Disables all tools which modify the state of a bug. Environment variable MCP_READ_ONLY=true can also be used. Can only tighten the guard policy, never loosen it @@ -106,6 +106,9 @@ Fallback for \fB\-\-host\fR. .B MCP_PORT Fallback for \fB\-\-port\fR. .TP +.B MCP_ALLOWED_HOSTS +Fallback for \fB\-\-allowed\-hosts\fR. +.TP .B MCP_API_KEY_HEADER Fallback for \fB\-\-api\-key\-header\fR. .TP @@ -115,6 +118,9 @@ Fallback for \fB\-\-api\-key\fR. .B BUGZILLA_API_KEY_FILE Fallback for \fB\-\-api\-key\-file\fR. .TP +.B BUGZILLA_USE_AUTH_HEADER +Fallback for \fB\-\-use\-auth\-header\fR. +.TP .B MCP_READ_ONLY Fallback for \fB\-\-read\-only\fR. .TP diff --git a/crates/bugwarden/src/config.rs b/crates/bugwarden/src/config.rs index 9a99876..68ee96b 100644 --- a/crates/bugwarden/src/config.rs +++ b/crates/bugwarden/src/config.rs @@ -52,10 +52,12 @@ pub struct Cli { pub port: u16, /// Hostname or 'host:port' authority accepted in an inbound Host header - /// (http transport only). Repeat the flag to allow further hosts. Command - /// line only, no environment variable; without it Host validation stays + /// (http transport only). Repeat the flag, or separate entries with + /// commas or whitespace; environment variable MCP_ALLOWED_HOSTS can also + /// be used. Empty entries are dropped, so `MCP_ALLOWED_HOSTS=` is an unset + /// (like `BUGZILLA_API_KEY_FILE=`); without a host, Host validation stays /// off and any Host header is served. - #[arg(long, value_name = "HOST")] + #[arg(long, env = "MCP_ALLOWED_HOSTS", value_name = "HOST")] pub allowed_hosts: Vec, /// HTTP header for clients to send the Bugzilla API key. Defaults to @@ -87,8 +89,9 @@ pub struct Cli { pub api_key_file: Option, /// Use 'Authorization: Bearer' header instead of the api_key query - /// parameter (required for some Bugzilla instances). - #[arg(long)] + /// parameter (required for some Bugzilla instances). Environment + /// variable BUGZILLA_USE_AUTH_HEADER=true can also be used. + #[arg(long, env = "BUGZILLA_USE_AUTH_HEADER")] pub use_auth_header: bool, /// Disables all tools which modify the state of a bug. Environment @@ -153,6 +156,26 @@ pub enum KeyCustody { } impl Cli { + /// The Host authorities this deployment answers to: every + /// `--allowed-hosts` occurrence, or `MCP_ALLOWED_HOSTS` when the flag is + /// absent — clap takes one source, not both — split on commas and + /// whitespace so one variable can carry a list, with empty entries + /// dropped. + /// + /// Dropping them is what makes `MCP_ALLOWED_HOSTS=` read as unset (the + /// `BUGZILLA_API_KEY_FILE=` convention) instead of naming one unmatchable + /// authority: rmcp validates against a non-empty list, and an entry it + /// cannot parse is skipped, so a lone empty entry would refuse every + /// Host. Naming a host only ever narrows what the disabled state serves + /// (I9); dropping an empty one restores exactly the documented default. + pub fn resolved_allowed_hosts(&self) -> Vec<&str> { + self.allowed_hosts + .iter() + .flat_map(|entry| entry.split(|c: char| c == ',' || c.is_whitespace())) + .filter(|host| !host.is_empty()) + .collect() + } + /// Resolve who holds the Bugzilla API key — the whole table, once, at /// startup. /// @@ -423,6 +446,75 @@ mod tests { assert!(msg.contains("--transport stdio requires"), "{msg}"); } + #[test] + fn every_flag_declares_an_environment_fallback() { + // Container deployments configure bugwarden entirely through the + // environment (issues #31/#32), so a flag without an `env` fallback + // is a hole in that contract — this fails the moment one appears. + let mut cmd = command(); + cmd.build(); + let env_less: Vec = cmd + .get_arguments() + .filter(|arg| !matches!(arg.get_id().as_str(), "help" | "version")) + .filter(|arg| arg.get_env().is_none()) + .map(|arg| arg.get_id().to_string()) + .collect(); + assert!( + env_less.is_empty(), + "every flag needs an environment fallback, these have none: {env_less:?}" + ); + let env_of = |id: &str| { + cmd.get_arguments() + .find(|arg| arg.get_id().as_str() == id) + .and_then(clap::Arg::get_env) + .map(|env| env.to_string_lossy().into_owned()) + }; + assert_eq!( + env_of("allowed_hosts").as_deref(), + Some("MCP_ALLOWED_HOSTS") + ); + assert_eq!( + env_of("use_auth_header").as_deref(), + Some("BUGZILLA_USE_AUTH_HEADER") + ); + } + + #[test] + fn allowed_hosts_split_on_commas_and_whitespace_dropping_empties() { + // One environment variable has to be able to carry the whole list, + // and both sources feed the one field — so the normalization the + // environment needs is proven here, on the flag. + let cli = Cli::parse_from([ + "bugwarden", + "--bugzilla-server", + "https://bugzilla.example.com", + "--allowed-hosts", + "a.example:8000, b.example", + "--allowed-hosts", + " c.example\td.example ", + "--allowed-hosts", + "", + ]); + assert_eq!( + cli.resolved_allowed_hosts(), + ["a.example:8000", "b.example", "c.example", "d.example"] + ); + + // An entry naming no host leaves validation off — `MCP_ALLOWED_HOSTS=` + // is unset, not "allow nothing" (see `resolved_allowed_hosts`). + let cli = Cli::parse_from([ + "bugwarden", + "--bugzilla-server", + "https://bugzilla.example.com", + "--allowed-hosts", + " ,, ", + ]); + assert!( + cli.resolved_allowed_hosts().is_empty(), + "an entry naming no host must leave Host validation off" + ); + } + #[test] fn cli_debug_never_prints_the_startup_key_i12() { let mut cli = base_cli("stdio"); diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index 1fb9b76..dfa9179 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -1344,9 +1344,9 @@ impl BugWarden { /// deployment not addressed as `localhost`, containers included. /// Disabled deliberately when the operator names no host: the access /// control here is the network boundary, and per-caller - /// authentication when it lands (issue #32). `--allowed-hosts` names - /// the authorities this deployment answers to, which only ever - /// narrows what the disabled state serves (I9). + /// authentication when it lands (issue #32). `--allowed-hosts` / + /// `MCP_ALLOWED_HOSTS` names the authorities this deployment answers + /// to, which only ever narrows what the disabled state serves (I9). /// * `max_request_body_bytes` is a POST cap with no rmcp 2.2 /// equivalent, worth keeping as a memory bound — but it also ceilings /// `add_attachment`, so a fixed value silently overrides the @@ -1386,9 +1386,12 @@ impl BugWarden { .with_max_request_body_bytes(max_request_body_bytes( self.guard.policy.global.max_attachment_bytes, )); - match self.cfg.allowed_hosts.as_slice() { + // `resolved_allowed_hosts` is what decides on/off: an empty list is + // the disabled state, and an entry rmcp cannot parse would otherwise + // leave validation on with nothing matchable (see config.rs). + match self.cfg.resolved_allowed_hosts().as_slice() { [] => config, - hosts => config.with_allowed_hosts(hosts.iter().map(String::as_str)), + hosts => config.with_allowed_hosts(hosts.iter().copied()), } } diff --git a/crates/bugwarden/tests/binary_user_agent.rs b/crates/bugwarden/tests/binary_user_agent.rs index a7b13ec..1dddde6 100644 --- a/crates/bugwarden/tests/binary_user_agent.rs +++ b/crates/bugwarden/tests/binary_user_agent.rs @@ -20,6 +20,51 @@ use wiremock::{Mock, MockServer, ResponseTemplate}; /// hanging the suite until CI's own timeout kills it. const REPLY_TIMEOUT: Duration = Duration::from_secs(20); +/// The ambient environment must not reach the child: every one of these is +/// read by `Cli` (`RUST_LOG` only muddies the captured stderr). Scrubbed as +/// one set rather than per test — the http-only knobs are inert for a stdio +/// run today, and pruning them is how the list falls behind `Cli` again. +/// `the_scrub_list_covers_every_environment_fallback` holds it to every +/// `env`-backed flag, so adding one cannot quietly leave a hole here. +const SCRUBBED_ENV: [&str; 13] = [ + "BUGZILLA_SERVER", + "BUGZILLA_API_KEY", + "BUGZILLA_API_KEY_FILE", + "BUGZILLA_USE_AUTH_HEADER", + "BUGWARDEN_POLICY", + "BUGWARDEN_AUDIT_CONFIG", + "MCP_TRANSPORT", + "MCP_HOST", + "MCP_PORT", + "MCP_ALLOWED_HOSTS", + "MCP_API_KEY_HEADER", + "MCP_READ_ONLY", + "RUST_LOG", +]; + +/// The scrub list above is only as good as its coverage of `Cli`, and a +/// flag added with an `env` fallback would otherwise go on reaching the +/// child from the developer's or the runner's environment. +#[test] +fn the_scrub_list_covers_every_environment_fallback() { + let mut cmd = bugwarden::config::command(); + cmd.build(); + let unscrubbed: Vec = cmd + .get_arguments() + .filter_map(clap::Arg::get_env) + .map(|env| env.to_string_lossy().into_owned()) + .filter(|env| !SCRUBBED_ENV.contains(&env.as_str())) + .collect(); + assert!( + !SCRUBBED_ENV.is_empty() && cmd.get_arguments().any(|arg| arg.get_env().is_some()), + "the check is only evidence while both lists are non-empty" + ); + assert!( + unscrubbed.is_empty(), + "these environment fallbacks reach the spawned binary: {unscrubbed:?}" + ); +} + /// The identity this build must present. Spelled out rather than read from /// the manifest the code reads: a comparison against `CARGO_PKG_REPOSITORY` /// agrees with whatever that field says, including a repository belonging @@ -54,18 +99,7 @@ async fn upstream_requests_of_a_real_run( .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::null()); - // The ambient environment must not reach the child: every one of these - // is read by `Cli` and would change what is under test. - for var in [ - "BUGZILLA_SERVER", - "BUGZILLA_API_KEY", - "BUGZILLA_API_KEY_FILE", - "BUGWARDEN_POLICY", - "BUGWARDEN_AUDIT_CONFIG", - "MCP_TRANSPORT", - "MCP_READ_ONLY", - "RUST_LOG", - ] { + for var in SCRUBBED_ENV { cmd.env_remove(var); } let mut child = cmd.spawn().expect("the built binary must start"); diff --git a/crates/bugwarden/tests/env_config.rs b/crates/bugwarden/tests/env_config.rs new file mode 100644 index 0000000..76cb8e7 --- /dev/null +++ b/crates/bugwarden/tests/env_config.rs @@ -0,0 +1,111 @@ +//! Configuration read from the REAL process environment. +//! +//! ONE test on purpose: it mutates the process environment, which is only +//! safe while no other thread reads it, and libtest runs a binary's tests in +//! parallel. Put new environment cases inside this test rather than beside +//! it, and keep this file free of anything else. +//! +//! Coverage contract (each of these mutations must fail this test): +//! - dropping `env = "MCP_ALLOWED_HOSTS"` or `env = "BUGZILLA_USE_AUTH_HEADER"`; +//! - keeping the empty entry `MCP_ALLOWED_HOSTS=` produces, which would turn +//! Host validation ON with nothing matchable and refuse every request; +//! - letting the environment override the command line. + +use bugwarden::config::Cli; +use clap::error::ErrorKind; +use clap::Parser as _; + +const VARS: [&str; 3] = [ + "MCP_ALLOWED_HOSTS", + "BUGZILLA_USE_AUTH_HEADER", + "MCP_READ_ONLY", +]; + +fn clear() { + for var in VARS { + std::env::remove_var(var); + } +} + +/// Parse a minimal command line, so only the variable under test decides. +fn parse(args: &[&str]) -> Result { + let mut argv = vec![ + "bugwarden", + "--bugzilla-server", + "https://bugzilla.example.com", + ]; + argv.extend_from_slice(args); + Cli::try_parse_from(argv) +} + +/// What `var=value` alone makes `read` see: the parsed flag, or the usage +/// error clap refused the value with. +fn outcome(var: &str, value: &str, read: fn(&Cli) -> bool) -> Result { + clear(); + std::env::set_var(var, value); + let outcome = parse(&[]).map(|cli| read(&cli)).map_err(|e| e.kind()); + clear(); + outcome +} + +#[test] +fn every_flag_is_settable_from_the_environment() { + clear(); + + // One variable carries the whole allowed-hosts list, comma- and/or + // whitespace-separated, trimmed. + std::env::set_var("MCP_ALLOWED_HOSTS", "a.example:8000, b.example"); + let cli = parse(&[]).expect("a host list parses"); + assert_eq!( + cli.resolved_allowed_hosts(), + ["a.example:8000", "b.example"], + "MCP_ALLOWED_HOSTS must reach --allowed-hosts as separate entries" + ); + + // `MCP_ALLOWED_HOSTS=` is the set-but-empty "unset" idiom of unit files + // and container specs (as for BUGZILLA_API_KEY_FILE): it names no host, + // so Host validation stays off. Keeping the empty entry would instead + // switch validation on with nothing rmcp can match, refusing every Host. + std::env::set_var("MCP_ALLOWED_HOSTS", ""); + let cli = parse(&[]).expect("an empty value parses"); + assert!( + cli.resolved_allowed_hosts().is_empty(), + "MCP_ALLOWED_HOSTS= must read as unset, leaving Host validation off" + ); + + // Precedence: the command line wins over the environment (I9 is not at + // stake either way — naming hosts only ever narrows what is served). + std::env::set_var("MCP_ALLOWED_HOSTS", "env.example"); + let cli = parse(&["--allowed-hosts", "cli.example"]).expect("the flag parses"); + assert_eq!( + cli.resolved_allowed_hosts(), + ["cli.example"], + "--allowed-hosts must win over MCP_ALLOWED_HOSTS" + ); + + // BUGZILLA_USE_AUTH_HEADER is read, and read exactly as MCP_READ_ONLY is: + // clap's bool flags take the literal `true` or `false` from an + // environment variable and make everything else a usage error. + assert_eq!( + outcome("BUGZILLA_USE_AUTH_HEADER", "true", |cli| cli + .use_auth_header), + Ok(true), + "BUGZILLA_USE_AUTH_HEADER=true must select the Authorization header" + ); + assert_eq!( + outcome("BUGZILLA_USE_AUTH_HEADER", "false", |cli| cli + .use_auth_header), + Ok(false) + ); + for value in [ + "true", "false", "TRUE", "True", "1", "0", "yes", "no", "on", "off", "", + ] { + assert_eq!( + outcome("BUGZILLA_USE_AUTH_HEADER", value, |cli| cli.use_auth_header), + outcome("MCP_READ_ONLY", value, |cli| cli.read_only), + "BUGZILLA_USE_AUTH_HEADER={value:?} must be read exactly like MCP_READ_ONLY" + ); + } + + clear(); +} diff --git a/crates/bugwarden/tests/http_transport_wiremock.rs b/crates/bugwarden/tests/http_transport_wiremock.rs index 20dac5d..65dbbfb 100644 --- a/crates/bugwarden/tests/http_transport_wiremock.rs +++ b/crates/bugwarden/tests/http_transport_wiremock.rs @@ -22,6 +22,9 @@ //! reading only `context.meta`, is behavior-preserving over every //! serialized transport and is killed by the direct in-process call //! test in server.rs instead); +//! - keeping the empty entry `MCP_ALLOWED_HOSTS=` produces instead of +//! dropping it, which turns Host validation on with nothing rmcp can +//! match and refuses every request; //! - the POST body cap going back to a fixed value, which refuses uploads //! the operator's `global.max_attachment_bytes` permits, or losing its //! 4 MiB floor, which would let a policy shrink the transport's memory @@ -51,9 +54,13 @@ use wiremock::matchers::{any, method, path, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; /// Build the http-transport `Cli` against `mock`, with `key_file` when the -/// test runs in server-held mode. Both key fields are then set explicitly, -/// so the ambient environment (`BUGZILLA_API_KEY`, `BUGZILLA_API_KEY_FILE`) -/// cannot leak into what a test resolves. +/// test runs in server-held mode. Every field an ambient environment +/// variable could set behind the fixed arguments below is then assigned +/// explicitly — the two key sources (`BUGZILLA_API_KEY`, +/// `BUGZILLA_API_KEY_FILE`) and the Host allowlist (`MCP_ALLOWED_HOSTS`, +/// which would otherwise refuse the loopback authority the harness dials). +/// A new environment-backed flag that changes what a test resolves belongs +/// here too. fn http_cli(mock: &MockServer, key_file: Option<&std::path::Path>) -> Arc { let mut cli = Cli::parse_from([ "bugwarden", @@ -66,6 +73,7 @@ fn http_cli(mock: &MockServer, key_file: Option<&std::path::Path>) -> Arc { ]); cli.api_key = None; cli.api_key_file = key_file.map(std::path::Path::to_path_buf); + cli.allowed_hosts = Vec::new(); Arc::new(cli) } @@ -457,6 +465,45 @@ async fn allowed_hosts_serve_the_named_authority_and_refuse_the_others() { } } +#[tokio::test] +async fn an_allowed_hosts_entry_naming_no_host_leaves_validation_off() { + // `MCP_ALLOWED_HOSTS=` (the set-but-empty "unset" idiom of unit files and + // container specs) reaches the server as one empty entry. rmcp validates + // against a non-empty list and silently skips the entries it cannot + // parse, so carrying that entry through would refuse EVERY Host and brick + // the deployment; dropping it restores the documented default instead. + let mock = MockServer::start().await; + let file = key_file("srv-key\n"); + let mut cli = Arc::into_inner(http_cli(&mock, Some(file.path()))).expect("the sole owner"); + cli.allowed_hosts = vec![String::new()]; + let addr = serve_http(Arc::new(cli), "", &mock, None).await; + + let response = reqwest::Client::new() + .post(format!("http://{addr}/mcp")) + .header("Host", "bugwarden.example:8080") + .header("Accept", "application/json, text/event-stream") + .header("Content-Type", "application/json") + .json(&json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-11-25", + "capabilities": {}, + "clientInfo": { "name": "host-probe", "version": "1" } + } + })) + .send() + .await + .expect("the request must reach the server"); + let status = response.status(); + let body = response.text().await.expect("a body"); + assert!( + status.is_success(), + "an entry naming no host must leave every Host served, got {status}: {body}" + ); +} + #[tokio::test] async fn a_handshake_free_call_is_refused_and_never_names_a_client() { // rmcp routes a request to its handshake-free lifecycle on the mere diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 8ed4831..19c1903 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -984,10 +984,11 @@ clap derive `Cli`, with env fallbacks: | --transport | MCP_TRANSPORT | http | http \| stdio (clap ValueEnum) | | --host | MCP_HOST | 127.0.0.1 | http only | | --port | MCP_PORT | 8000 | http only | +| --allowed-hosts | MCP_ALLOWED_HOSTS | — | http only; Host authorities served, comma/whitespace-separated and repeatable, empty entries dropped so an empty value reads as unset (see rmcp usage notes) | | --api-key-header | MCP_API_KEY_HEADER | ApiKey | http per-request key header | | --api-key | BUGZILLA_API_KEY | — | required for stdio unless --api-key-file provides it; warn-and-ignore for http (never a silent upgrade to server-held) | | --api-key-file | BUGZILLA_API_KEY_FILE | — | file holding the key (container secret / systemd LoadCredential path); mutually exclusive with --api-key; over http selects server-held key mode (see Key custody) | -| --use-auth-header | — | false | Bearer to Bugzilla instead of api_key query param | +| --use-auth-header | BUGZILLA_USE_AUTH_HEADER | false | Bearer to Bugzilla instead of api_key query param | | --read-only | MCP_READ_ONLY | false | tighten-only (I9) | | --policy | BUGWARDEN_POLICY | — | path to guard policy TOML | | --audit-config | BUGWARDEN_AUDIT_CONFIG | — | path to audit configuration TOML; without it no audit stream is written | @@ -1305,7 +1306,7 @@ wired, `server.rs` and `main.rs` are the reference. | field | rmcp 3.1 default | this build | |---|---|---| - | `allowed_hosts` | `localhost`, `127.0.0.1`, `::1` | **set** — `disable_allowed_hosts()`, or the operator's `--allowed-hosts` list when given | + | `allowed_hosts` | `localhost`, `127.0.0.1`, `::1` | **set** — `disable_allowed_hosts()`, or the operator's `--allowed-hosts` / `MCP_ALLOWED_HOSTS` list when given | | `max_request_body_bytes` | 4 MiB | **set** — derived from `global.max_attachment_bytes`, floored at that same 4 MiB (see below) | | `cancellation_token` | fresh token | **set** (main.rs) — a child of the process token | | `allowed_origins` | `[]`, i.e. validation off | inherited, deliberately | @@ -1373,11 +1374,18 @@ wired, `server.rs` and `main.rs` are the reference. shutdown. An operator who does know the authorities their deployment answers to names - them with a repeated `--allowed-hosts`, which turns that validation back on + them with a repeated `--allowed-hosts`, or with `MCP_ALLOWED_HOSTS` as one + comma- and/or whitespace-separated list, which turns that validation back on for exactly that list. Tighten-only like every other CLI knob (I9), since - the disabled state serves every `Host`; command line only, and deliberately - without an environment variable, because it is a per-deployment network fact - stated where the bind address is stated. + the disabled state serves every `Host`. **SUPERSEDED 2026-08-18:** the flag + was command line only, on the argument that the hosts are a per-deployment + network fact stated where the bind address is stated — the #31/#32 decision + that a container configures bugwarden entirely through the environment + overrides that, and `--host`/`--port` state the bind address from the + environment too. Empty entries are dropped in `Cli::resolved_allowed_hosts`, + so `MCP_ALLOWED_HOSTS=` reads as unset (the `BUGZILLA_API_KEY_FILE=` + convention) rather than as a list of one authority `host_is_allowed` skips, + which would refuse every `Host`. `allowed_origins` is the browser-facing sibling of `allowed_hosts`, and the #32 argument covers it identically. It is inherited rather than named because @@ -1684,6 +1692,23 @@ wired, `server.rs` and `main.rs` are the reference. decoded lands exactly on the 64 MiB ceiling, one quantum below it still derives and one above clamps; and `u64::MAX` clamps to the ceiling rather than panicking, wrapping, or saturating into an unbounded body. +- Configuration tests (crates/bugwarden/tests/env_config.rs, the REAL + process environment): every flag is settable from the environment, which + is what a container deployment configures through (#31/#32). ONE test by + construction — it mutates process-global state, which is safe only while + no other thread reads it, so the file holds exactly one and says so. It + pins `MCP_ALLOWED_HOSTS` carrying a comma- and/or whitespace-separated + list into separate trimmed entries, an empty value reading as unset + (validation off, not a list of one authority `host_is_allowed` skips, + which would refuse every `Host`), `--allowed-hosts` beating the variable + since clap takes one source and not the union, and + `BUGZILLA_USE_AUTH_HEADER` being read exactly as `MCP_READ_ONLY` is — + compared value for value over `true`/`false` and the nine spellings clap + refuses, with `Ok(true)`/`Ok(false)` anchors so the parity assertion + cannot pass on two flags that are both unset. Two structural siblings + keep it from rotting: a `config.rs` unit test fails when any argument + lacks an `env` fallback, and `binary_user_agent.rs` holds its child-env + scrub list to that same set. - Audit tests (crates/bugwarden/tests/audit_wiremock.rs + #[cfg(test)] in server.rs and audit.rs): one record per call for EVERY routed tool, refusal paths and protocol errors included; the refusal map is total