Read MCP secret variables through one resolver

Refs #5637 (slice 1 only; the injected-secret-store request stays open).

Four MCP sites each decided where a secret variable comes from: ${NAME}
placeholder expansion, HTTP env_headers, the bearer-token variable, and the
OAuth default-header builder. The first three already read a reviewed plugin's
captured pre-dotenv HostEnvironment; the OAuth header builder read the process
environment for every server, including reviewed plugin servers.

mcp::mcp_secret_var(reviewed_plugin, name) is now the only place that picks
the source: a reviewed plugin reads only its captured host_environment, any
other server keeps the exact std::env::var behaviour. The four sites call it,
and oauth::build_default_headers takes the server config (its four callers
already held one) so the OAuth path gets the same rule. Error context strings
are unchanged and the resolver logs and returns nothing new. Ordinary servers
still share process-environment semantics; no provider credential store or
keyring is consulted and none is added.

Verified locally (macOS; no hosted CI): the same frozen tui lib nextest batch
as the preceding #6561 commit, 426 run, 426 passed, 0 failed, 14876 skipped.
Of those, mcp:: matched 408 tests, expand_env_placeholders 2, mcp_http_auth_ 2,
reviewed_plugin_environment_uses_only_the_pre_dotenv_snapshot,
reviewed_plugin_oauth_is_disabled_without_network_or_token_mutation and
configured_mcp_credentials_refuse_remote_http_before_header_resolution 1 each.
dead-code 209, blocking 543/165 and the runtime boundary check still pass.

Not verified: no existing test passes an env-header value through the OAuth
header builder or the HTTP env_headers path, so those two sites rest on source
audit plus compilation. No running MCP server was observed. The injected store
for embedded hosts is deliberately not part of this slice.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
This commit is contained in:
CodeWhale BotandClaude Sonnet 5.5 committed 2026-10-25 05:59:14 -07:00
1 parent 7d84d0be6b
commit f98cb165a3
3 files changed
+33 -34

No files matched your search

+24 -16
View File
@@ -105,8 +105,18 @@ fn reject_linked_workspace_state_path(path: &Path) -> Result<()> {
.with_context(|| format!("MCP config {} is not safe to write", path.display()))
}
/// Expand `${NAME}` placeholders in an MCP config value from the process
/// environment. This lets secrets (API keys, bearer tokens, …) be supplied
fn mcp_secret_var(
reviewed_plugin: Option<&ReviewedPluginMcpSource>,
name: &str,
) -> Result<String, std::env::VarError> {
match reviewed_plugin {
Some(source) => source.host_environment.var(name),
None => std::env::var(name),
}
}
/// Expand `${NAME}` placeholders in an MCP config value from its permitted
/// environment source. This lets secrets (API keys, bearer tokens, …) be supplied
/// through environment variables instead of being written in cleartext into
/// the MCP config file on disk.
///
@@ -115,7 +125,7 @@ fn reject_linked_workspace_state_path(path: &Path) -> Result<()> {
/// echoed into logs or error output.
fn expand_env_placeholders_with(
value: &str,
environment: Option<&crate::plugins::HostEnvironment>,
reviewed_plugin: Option<&ReviewedPluginMcpSource>,
) -> Result<String> {
let mut out = String::new();
let mut rest = value;
@@ -129,11 +139,9 @@ fn expand_env_placeholders_with(
if name.is_empty() || !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
anyhow::bail!("invalid environment placeholder in MCP config value");
}
let env_value = environment
.map_or_else(|| std::env::var(name), |env| env.var(name))
.with_context(|| {
format!("environment variable {name} required by MCP config is not set")
})?;
let env_value = mcp_secret_var(reviewed_plugin, name).with_context(|| {
format!("environment variable {name} required by MCP config is not set")
})?;
out.push_str(&env_value);
rest = &after[end + 1..];
}
@@ -152,13 +160,13 @@ fn expand_env_placeholders(value: &str) -> Result<String> {
fn expand_env_placeholders_map_with_environment(
values: &HashMap<String, String>,
context: &str,
environment: Option<&crate::plugins::HostEnvironment>,
reviewed_plugin: Option<&ReviewedPluginMcpSource>,
) -> Result<HashMap<String, String>> {
let mut expanded = HashMap::with_capacity(values.len());
for (key, value) in values {
expanded.insert(
key.clone(),
expand_env_placeholders_with(value, environment)
expand_env_placeholders_with(value, reviewed_plugin)
.with_context(|| format!("failed to expand MCP {context} value for {key}"))?,
);
}
@@ -174,11 +182,11 @@ fn expand_env_placeholders_map(
}
fn expanded_mcp_stdio_env(config: &McpServerConfig) -> Result<HashMap<String, String>> {
let environment = config
.reviewed_plugin
.as_ref()
.map(|source| source.host_environment.as_ref());
expand_env_placeholders_map_with_environment(&config.env, "env", environment)
expand_env_placeholders_map_with_environment(
&config.env,
"env",
config.reviewed_plugin.as_ref(),
)
}
/// Mirror the exact expanded and sanitized environment applied by the MCP
@@ -1908,7 +1916,7 @@ async fn prepare_mcp_http_client(
let oauth_runtime = if config.reviewed_plugin.is_some() {
None
} else {
match oauth::build_default_headers(&config.headers, &config.env_headers) {
match oauth::build_default_headers(config) {
Ok(default_headers) => {
let prepared = tokio::select! {
biased;
+2 -8
View File
@@ -76,10 +76,7 @@ impl McpHttpAuth {
}
let mut headers = self.headers.clone();
for (name, env_var) in &self.env_headers {
let value = self.reviewed_plugin.as_ref().map_or_else(
|| std::env::var(env_var),
|source| source.host_environment.var(env_var),
);
let value = super::mcp_secret_var(self.reviewed_plugin.as_ref(), env_var);
if let Ok(value) = value
&& !value.trim().is_empty()
{
@@ -88,10 +85,7 @@ impl McpHttpAuth {
}
if !mcp_headers_have_authorization(&headers)
&& let Some(env_var) = self.bearer_token_env_var.as_deref()
&& let Ok(token) = self.reviewed_plugin.as_ref().map_or_else(
|| std::env::var(env_var),
|source| source.host_environment.var(env_var),
)
&& let Ok(token) = super::mcp_secret_var(self.reviewed_plugin.as_ref(), env_var)
{
let token = token.trim();
if !token.is_empty() {
+7 -10
View File
@@ -959,7 +959,7 @@ pub async fn auth_status_for_server(
}
}
let headers = match build_default_headers(&server.headers, &server.env_headers) {
let headers = match build_default_headers(server) {
Ok(headers) => headers,
Err(err) => {
tracing::warn!(target: "mcp", server = %name, error = %err, "failed to build MCP OAuth discovery headers");
@@ -989,7 +989,7 @@ pub async fn oauth_login_support(
if server_has_manual_authorization(server) {
return Ok(None);
}
let headers = build_default_headers(&server.headers, &server.env_headers)?;
let headers = build_default_headers(server)?;
discover_streamable_http_oauth_for_server(server, url, headers, network_policy).await
}
@@ -1016,7 +1016,7 @@ fn oauth_login_client(
url: &str,
network_policy: Option<&NetworkPolicyDecider>,
) -> Result<McpHttpClient> {
let headers = build_default_headers(&server.headers, &server.env_headers)?;
let headers = build_default_headers(server)?;
Ok(oauth_http_client(server, url, network_policy)?.with_default_headers(headers))
}
@@ -1423,16 +1423,13 @@ pub(crate) fn server_has_manual_authorization(server: &McpServerConfig) -> bool
|| contains_authorization_header(&server.env_headers)
}
pub fn build_default_headers(
http_headers: &HashMap<String, String>,
env_headers: &HashMap<String, String>,
) -> Result<HeaderMap> {
pub(super) fn build_default_headers(server: &McpServerConfig) -> Result<HeaderMap> {
let mut headers = HeaderMap::new();
for (name, value) in http_headers {
for (name, value) in &server.headers {
insert_header(&mut headers, name, value)?;
}
for (name, env_var) in env_headers {
if let Ok(value) = std::env::var(env_var)
for (name, env_var) in &server.env_headers {
if let Ok(value) = super::mcp_secret_var(server.reviewed_plugin.as_ref(), env_var)
&& !value.trim().is_empty()
{
insert_header(&mut headers, name, &value)?;