mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-08 08:04:25 -10:00
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:
1 parent
7d84d0be6b
commit
f98cb165a3
3 files changed
+33
-34
No files matched your search
+24
-16
@@ -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;
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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)?;
|
||||
|
||||
Reference in new issue
Block a user