fix(mcp): validate server names with strict allowlist (fixes #1882)

[serrrfirat]

Summary

• Adds allowlist-based validation for MCP server names in McpServerConfig::validate(): only [a-zA-Z0-9_-] (alphanumeric, underscore, dash) are permitted
• Prevents shell metacharacters (;, |, &, backticks, $()), path separators (/, \), dots, null bytes, spaces, and other dangerous characters that could cause injection when names are interpolated into secret keys, tool name prefixes, or provider tags
• Changes load_mcp_servers_from() and load_mcp_servers_from_db() to skip invalid entries with a tracing::warn instead of failing the entire config load -- this prevents legacy names (e.g. "My Server") from disabling all MCP integrations after an upgrade
• Adds comprehensive tests: valid names, shell injection attempts, path traversal, null bytes, dots, and a load-level test for graceful skip behavior

This supersedes #1941 and incorporates its review feedback:

• Dots removed from the allowlist (LLM providers require tool names to match ^[a-zA-Z0-9_-]+$ and server names are used as tool name prefixes)
• Graceful degradation on invalid names during config load (addresses @serrrfirat's migration/regression concern)

Test plan

• cargo check passes
• cargo clippy --all --all-features passes with zero warnings
• All 47 MCP config tests pass including 7 new tests:
  • test_server_name_valid_characters_accepted
  • test_server_name_shell_metacharacters_rejected
  • test_server_name_path_separators_rejected
  • test_server_name_null_byte_rejected
  • test_server_name_dot_rejected
  • test_load_skips_invalid_server_names
  • test_load_skips_corrupted_headers (updated to verify skip behavior)

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces stricter validation for MCP server names, allowing only alphanumeric characters, dashes, and underscores to prevent injection vulnerabilities and ensure compatibility with LLM providers. It also updates the configuration loading logic for both disk and database sources to skip invalid server entries with a warning rather than failing the entire load process. Feedback suggests using Vec::retain on the existing configuration object when filtering invalid servers to ensure that metadata, such as the schema_version, is preserved instead of being reset to default values.

[gemini-code-assist]

The current implementation of load_mcp_servers_from creates a new McpServersFile using Default::default(), which resets the schema_version to its default value (1). This loses any versioning metadata present in the original configuration file. Using retain on the original config.servers vector is a cleaner way to filter invalid entries while preserving the rest of the struct's metadata.

Suggested change

	let config: McpServersFile = serde_json::from_str(&content)?;
	// Validate every server on load so corrupted configs are caught early
	// Validate every server on load. Invalid entries are skipped with a
	// warning instead of failing the entire config — this prevents legacy
	// names (e.g. "My Server") from disabling all MCP integrations after
	// an upgrade that tightened validation.
	let mut valid = McpServersFile::default();
	for server in &config.servers {
	server.validate().map_err(|e| ConfigError::InvalidConfig {
	reason: format!("Server '{}': {}", server.name, e),
	})?;
	if let Err(e) = server.validate() {
	tracing::warn!(
	server_name = %server.name,
	"Skipping MCP server with invalid config: {e}"
	);
	continue;
	}
	valid.servers.push(server.clone());
	}
	Ok(config)
	Ok(valid)
	let mut config: McpServersFile = serde_json::from_str(&content)?;
	// Validate every server on load. Invalid entries are skipped with a
	// warning instead of failing the entire config — this prevents legacy
	// names (e.g. "My Server") from disabling all MCP integrations after
	// an upgrade that tightened validation.
	config.servers.retain(|server| {
	if let Err(e) = server.validate() {
	tracing::warn!(
	server_name = %server.name,
	"Skipping MCP server with invalid config: {e}"
	);
	false
	} else {
	true
	}
	});
	Ok(config)

References

1. In non-performance-critical code paths, prioritize code simplicity over micro-optimizations like avoiding clones, as the performance gain is often negligible.

[gemini-code-assist]

Similar to load_mcp_servers_from, this implementation resets the schema_version by creating a new McpServersFile. Using retain on the existing config.servers preserves metadata and keeps the logic straightforward.

Suggested change

	let config: McpServersFile = serde_json::from_value(value)?;
	// Validate every server on load so corrupted DB configs are caught early
	// Validate every server on load. Invalid entries are skipped
	// with a warning to avoid breaking all MCP integrations when
	// legacy names don't pass tightened validation.
	let mut valid = McpServersFile::default();
	for server in &config.servers {
	server.validate().map_err(|e| ConfigError::InvalidConfig {
	reason: format!("Server '{}': {}", server.name, e),
	})?;
	if let Err(e) = server.validate() {
	tracing::warn!(
	server_name = %server.name,
	"Skipping MCP server with invalid DB config: {e}"
	);
	continue;
	}
	valid.servers.push(server.clone());
	}
	Ok(config)
	Ok(valid)
	let mut config: McpServersFile = serde_json::from_value(value)?;
	// Validate every server on load. Invalid entries are skipped
	// with a warning to avoid breaking all MCP integrations when
	// legacy names don't pass tightened validation.
	config.servers.retain(|server| {
	if let Err(e) = server.validate() {
	tracing::warn!(
	server_name = %server.name,
	"Skipping MCP server with invalid DB config: {e}"
	);
	false
	} else {
	true
	}
	});
	Ok(config)

References

1. In non-performance-critical code paths, prioritize code simplicity over micro-optimizations like avoiding clones, as the performance gain is often negligible.

[henrypark133]

Review: Validate MCP server names with strict allowlist (Risk: Medium)

Well-done security fix with pragmatic error handling.

Positives:

• Strict name validation: only alphanumeric, dash, underscore — prevents shell metacharacters, path separators, dots, and null bytes
• Graceful degradation: invalid entries are skipped with a warning instead of failing the entire config — prevents legacy names from disabling all MCP integrations after an upgrade
• Both file and DB loading paths updated consistently
• Comprehensive test suite: valid names, shell metacharacters, path separators, null bytes, dots, and the skip-invalid-entries integration test

Convention notes:

• Good comment explaining why dots are rejected (LLM provider tool name prefix constraint)
• tracing::warn! is appropriate here since this is user-facing startup validation, not background task diagnostics

LGTM.

[serrrfirat]

Addressed review feedback from gemini-code-assist:

• Both load_mcp_servers_from and load_mcp_servers_from_db: Replaced manual for-loop + intermediate McpServersFile struct with Vec::retain on the original config. This preserves schema_version implicitly and simplifies the filtering logic (−20/+14 lines).

All 254 MCP tests pass, including test_load_preserves_schema_version.

[henrypark133]

Review: MCP server name allowlist

No verified findings in the current diff. The stricter name validation is applied on construction and on config load, and the follow-up change preserves schema_version while filtering invalid legacy entries instead of breaking the whole MCP config.

[henrypark133]

Review: MCP server name allowlist

No verified findings in the current diff. The stricter name validation is applied on construction and on config load, and the load path now filters invalid legacy entries while preserving the rest of the config.

[ilblackdragon]

@copilot resolve the merge conflicts in this pull request