fix(mcp): validate server names with allowlist to prevent injection (fixes #1882)

[willamhou]

Summary

• Switch MCP server name validation from blocklist (empty-only) to allowlist ([a-zA-Z0-9._-])
• Prevents shell metacharacters (;|&$), path separators (/`), null bytes, and spaces from being accepted as server names
• Server names are interpolated into secret keys (mcp_{name}_access_token), tool name prefixes, and provider tags — malicious characters in names could cause injection

Changes

src/tools/mcp/config.rs

• Added allowlist character validation in validate() (alphanumeric + dash + underscore + dot)
• Added 5 regression tests:
  • test_server_name_valid_characters_accepted — confirms legit names pass
  • test_server_name_shell_metacharacters_rejected — ;, $(), backticks, |, &, >, <, spaces
  • test_server_name_path_separators_rejected — /, \, ../
  • test_server_name_null_byte_rejected — \0
  • test_load_rejects_corrupted_server_name — validates at load time too

Test plan

• All 40 config tests pass (cargo test --lib tools::mcp::config::tests)
• cargo fmt clean
• cargo clippy zero warnings
• No .unwrap() in production code
• CI: classify + scope + full build (needs maintainer workflow approval for fork PR)

[gemini-code-assist]

Code Review

This pull request introduces validation for MCP server names to prevent shell injection and path traversal, along with a suite of unit tests. The review feedback recommends removing the dot character from the allowlist to ensure compatibility with major LLM providers' tool naming requirements and to strengthen security against path traversal. Additionally, the test cases should be updated to reflect this change by removing the dot character from the list of accepted names.

[gemini-code-assist]

The allowlist currently includes the dot ('.') character. However, server names are used as prefixes for tool names (e.g., '{server_name}{tool_name}'). Major LLM providers like OpenAI and Anthropic strictly require tool names to match the regex '^[a-zA-Z0-9-]+$'. Allowing dots in the server name will cause tool registration to fail for that server. Furthermore, excluding dots and path separators (/, ) at this boundary prevents path traversal vulnerabilities, as required by repository security rules.

Suggested change

	// Allowlist: alphanumeric, dash, underscore, dot.
	// Rejects shell metacharacters (;|&`$), path separators (/\),
	// null bytes, spaces, and other dangerous characters that could
	// cause injection when names are interpolated into secret keys,
	// tool name prefixes, or provider tags.
	if !self
	.name
	.chars()
	.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.')
	{
	return Err(ConfigError::InvalidConfig {
	reason: format!(
	"Server name '{}' contains invalid characters \
	(only alphanumeric, dash, underscore, dot are allowed)",
	self.name
	),
	});
	}
	// Allowlist: alphanumeric, dash, underscore.
	// Rejects shell metacharacters (;|& $), path separators (/\),
	// null bytes, spaces, dots, and other dangerous characters that could
	// cause injection when names are interpolated into secret keys,
	// tool name prefixes, or provider tags.
	if !self
	.name
	.chars()
	.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
	{
	return Err(ConfigError::InvalidConfig {
	reason: format!(
	"Server name '{}' contains invalid characters \
	(only alphanumeric, dash, underscore are allowed)",
	self.name
	),
	});
	}

References

1. Keep tool-specific guidance, such as parameter formats, in both the main system prompt for LLM planning and within the tool's own description (tool_info) to ensure it's exposed directly.
2. Path traversal characters (/, , .., \0) in extension names must be validated at the public API boundary for all operations (install, activate, remove) to prevent security vulnerabilities.

[gemini-code-assist]

Remove 'server.local' from the list of accepted names to align with the updated validation logic that disallows dots.

Suggested change

	for name in ["notion", "my-server", "my_server", "server.local", "MCP-1"] {
	for name in ["notion", "my-server", "my_server", "MCP-1"] {

[serrrfirat]

Worktree review note:

I reviewed the full worktree against origin/main, but PR #1941 itself only changes src/tools/mcp/config.rs against the staging base. I’m only posting the finding that is actually in this PR’s diff; the other issues I found were in stacked/base commits and would point at code that is not under review in this PR.

src/tools/mcp/config.rs

This hardening introduces a migration/regression risk for existing users with legacy MCP server names. Validation now rejects anything outside [A-Za-z0-9._-], and load_mcp_servers_from() / load_mcp_servers_from_db() fail the entire config on the first invalid entry. But the existing migration path still copies raw mcp-servers.json into DB settings without validating or rewriting names. That means a pre-existing entry like "My Server" or any other formerly-accepted name will cause the entire MCP config load to fail after upgrade, which in turn drops all MCP integrations at startup instead of quarantining or renaming the bad entry.

Suggested fix: either normalize legacy names during migration/save, or change load behavior to skip/report invalid entries individually so one historical bad name does not disable every otherwise-valid MCP server. I’d also add a regression test that migrates or loads a mixed config with one legacy invalid name plus one valid server and asserts the valid server still remains usable (or that the legacy one is deterministically renamed).

[willamhou]

Hi @henrypark133 @serrrfirat — all review feedback addressed (dot removed from allowlist, graceful load on invalid names). Rebased onto latest staging. Would appreciate a review when you get a chance. Thanks!

[willamhou]

@henrypark133 @serrrfirat @ilblackdragon Ready for review — MCP server name allowlist validation to prevent shell injection (#1882). Tests pass, fmt/clippy clean.

[willamhou]

@henrypark133 @serrrfirat @ilblackdragon Rebased onto latest staging, CI should be fresh. Would appreciate a review when you get a chance — security fix for MCP server name injection.

[henrypark133]

Review: MCP server name validation (Risk: Medium)

Good security hardening — the allowlist approach is solid and the graceful skip for legacy names is well-designed. Two issues worth addressing before merge.

Positives:

• Clean allowlist implementation blocking shell injection, path traversal, null bytes
• Graceful skip-with-warning for legacy names prevents upgrade breakage
• Good test coverage with 6 new tests

Concerning: Uppercase letters silently dropped at runtime [Logic]

File: src/tools/mcp/config.rs:172 + src/extensions/naming.rs:26
The allowlist uses is_ascii_alphanumeric() which permits uppercase (e.g., MCP-1). After factory normalization (factory.rs:40 replaces - with _), inject_mcp_client calls canonicalize_extension_name which only allows is_ascii_lowercase(). Result: MCP-1 passes validation, normalizes to MCP_1, then is silently dropped at manager.rs:1214. The test test_server_name_valid_characters_accepted lists MCP-1 as valid, but it won't work at runtime.
Suggested fix: Restrict the allowlist to lowercase: c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_', and update the test to use mcp-1 instead of MCP-1.

Minor: schema_version silently reset on load [Logic]

File: src/tools/mcp/config.rs:573, src/tools/mcp/config.rs:669
Both load functions construct McpServersFile::default() and copy only servers. The deserialized schema_version is lost (reset to 1). Low impact today but a future-compat footgun.
Suggested fix: let mut valid = McpServersFile { schema_version: config.schema_version, ..Default::default() };

Convention notes:

• The lenient-load / strict-write asymmetry is well-designed but would benefit from a brief comment in both load functions explaining the design choice.
• Missing a test for the DB load path (load_mcp_servers_from_db) with mixed valid/invalid servers — the file path has test_load_skips_invalid_server_name but no parallel DB test.

[willamhou]

@henrypark133 Both issues addressed in 0ff4e25:

1. Lowercase restriction — allowlist now requires is_ascii_lowercase() || is_ascii_digit(). Added test_server_name_uppercase_rejected covering MCP-1, MyServer, Notion. Updated existing test to use mcp-1.

2. schema_version preserved — both load_mcp_servers_from() and load_mcp_servers_from_db() now initialize the filtered McpServersFile with config.schema_version instead of defaulting to 1.

All 49 MCP config tests pass.

[ilblackdragon]

Critical — test is internally inconsistent with the loader it exercises.

This test calls load_mcp_servers_from(&path).await.unwrap() and then asserts that result.servers.len() == 1 (invalid skipped, valid kept). But the current loader at lines ~445-449 does:

for server in &config.servers {
    server.validate().map_err(|e| ConfigError::InvalidConfig {
        reason: format!("Server '{}': {}", server.name, e),
    })?;
}

It returns Err on the first invalid server — it does not skip. So .unwrap() will panic, and this test will FAIL on first run. Either:

1. Rename/rewrite the test to assert result.is_err() and that the error names bad;rm -rf / (matches the sibling test_load_rejects_corrupted_headers style + the PR body's claim of test_load_rejects_corrupted_server_name), OR
2. Intentionally change the loader to skip-and-warn (log the bad entry, drop it from the returned list) and keep the test — but then the header-validation loader path diverges in behavior, which is worse.

Option 1 matches the established pattern and this PR's own description. Please fix before merge — this is the single regression test for the core security fix, and as written it cannot run green.

[willamhou]

Fixed. The test already exercises the retain() (skip-and-warn) loader that was adopted after rebase onto staging — the for loop + ? pattern ilblackdragon referenced was from the pre-rebase version. The test runs green and confirms the expected behavior: invalid servers are skipped, valid ones kept.

[ilblackdragon]

High — allowlist is still more permissive than canonicalize_extension_name, keeping the silent-drop failure mode the comment claims to fix.

The updated comment on this line says "Uppercase names pass validation but are silently dropped at runtime" and tightens to lowercase. Good, but the same class of silent-drop remains:

canonicalize_extension_name() in src/extensions/naming.rs replaces - with _ and then rejects:

• leading or trailing _ (line 18)
• consecutive _ (line 33)

So the following names all pass this MCP validate(), yet fail (or silently-drop-at-runtime) when the extension manager canonicalizes them:

• -server, server-, _server, server_ (leading/trailing underscore after replace)
• my--server, my__server, my-_server (consecutive underscores after replace)
• a single - or _ as the entire name

If "uppercase silently dropped" is reason to reject upfront, these are the same bug and should be rejected here as well. Suggested tightening:

// Reuse the canonicalizer as the source of truth — single validation point.
if canonicalize_extension_name(&self.name).is_err() {
    return Err(ConfigError::InvalidConfig {
        reason: format!("Server name '{}' is not a valid extension identifier", self.name),
    });
}

…or inline the equivalent rules (first+last char alphanumeric, no consecutive -_). Either way, please add regression tests for -server, server-, my__server, and a bare -. Fixing the pattern, not the instance, per .claude/rules/review-discipline.md.

[willamhou]

Fixed in d6f7863. Replaced the manual char allowlist entirely with ExtensionName::new() as the single source of truth:

ExtensionName::new(&self.name).map_err(|e| ConfigError::InvalidConfig {
    reason: format!("Invalid server name '{}': {e}", self.name),
})?;

This catches all the edge cases you flagged (-server, server-, my__server, my--server, bare -/_) plus the 64-char limit from the identity layer. Added 6 regression tests covering leading/trailing separators, consecutive separators, bare separators, and length boundaries.

[ilblackdragon]

Medium — no length bound on server name.

validate() rejects empty (line 151) but imposes no upper bound. The name is interpolated into secret-store keys (mcp_{name}_access_token, {token_secret_name}_refresh_token), tool-name prefixes ({server_name}_{tool_name}), and the mcp:{name} provider tag — all of which are persisted and broadcast. A multi-kilobyte name passes every character check in this allowlist and is then stored, logged, and prefixed onto every tool call and secret key.

Suggested: cap at, say, 64 ASCII chars (matches typical DNS-label conventions and leaves headroom for tool-name suffix under the LLM provider's ^[a-zA-Z0-9_-]+$ limit — some providers also cap the full name at 64). Add a test asserting a 65-char name is rejected and a 64-char name is accepted. This is the "length limit" lens from the review checklist and the only remaining dimension not covered by the new tests.

[willamhou]

Fixed in d6f7863. The ExtensionName::new() canonicalizer enforces MAX_NAME_LEN = 64, so this is now handled by the same single-source-of-truth call. Added test_server_name_64_chars_accepted and test_server_name_65_chars_rejected to cover the boundary.

[ilblackdragon]

Review summary — security-critical

Thanks for taking on #1882. The lens is right — server names absolutely do flow into secret keys, tool-name prefixes, mcp:{name} provider tags, OAuth flow state, and spawn_stdio invocations — and an allowlist is the right primitive.

Three findings (inline comments cover detail):

1. Critical: test_load_skips_invalid_server_name asserts .unwrap() + filtered length, but load_mcp_servers_from returns Err on first invalid server. The test will panic. This is the load-time regression test for the whole PR, so please reconcile before merge (match the test_load_rejects_corrupted_headers shape that the PR body actually describes).
2. High: The allowlist is still more permissive than canonicalize_extension_name — -server, server_, my__server pass here and silently drop at runtime. Same failure mode the uppercase-tightening is supposed to eliminate. Prefer routing through canonicalize_extension_name as the single source of truth.
3. Medium: No length bound. A multi-kB name passes every char check and lands in secret keys / tool prefixes / logs.

Architectural note: the validation is only invoked on add_mcp_server / add_mcp_server_db / load_mcp_servers_from / load_mcp_servers_from_db. That covers the known ingress points today, but upsert(), get_mut(), and direct save_mcp_servers_to calls do not re-validate. Fine as-is (add-time validation is the norm), but worth a one-line note in the module doc that McpServerConfig::validate() MUST be called before any code path that materializes the name into an interpolated string — otherwise the next caller (and there will be one) re-introduces the injection path.

The fundamental direction of this PR is correct and the test matrix for shell metas / path seps / null bytes / dots / uppercase is thorough. Blocking on (1) only; (2) and (3) are strongly encouraged same-PR.

[serrrfirat]

Follow-up on ilblackdragon’s review summary (#1941 (comment)): the three findings called out there are addressed on the current HEAD.

1. Critical / load-time regression test — load_mcp_servers_from() now uses skip-and-warn behavior for invalid legacy entries, and test_load_skips_invalid_server_name matches that behavior. I also added DB-path parity in 346b811f with test_load_from_db_skips_invalid_server_name.
2. High / validator drift vs canonical naming — McpServerConfig::validate() now delegates to ExtensionName::new() in d6f7863, so MCP server names use the same canonical validator as the extension naming layer.
3. Medium / missing length bound — also handled by the same ExtensionName::new() path; d6f7863 adds the 64-char accepted / 65-char rejected regression coverage.

CI is green on the current head.

[willamhou]

@ilblackdragon @serrrfirat rebased onto latest staging — was DIRTY because staging landed McpServerName newtype + 64-byte cap + legacy-name truncation in parallel. Merged staging in (08b4d79d), keeping staging's superset implementation since it's strictly stronger than my original ExtensionName::new delegation.

CI green, all 51 MCP config tests passing. Ready for re-review / approval.

[willamhou]

@ilblackdragon brief follow-up after yesterday's rebase — current HEAD now adopts your McpServerName newtype + 64-byte cap from staging (08b4d79d). All three findings from your Apr 18 review remain resolved. Would appreciate a look when you get a chance.