fix(mcp): normalize server names, fix UTF-8 truncation, skip auth when header set#2379
fix(mcp): normalize server names, fix UTF-8 truncation, skip auth when header set#2379
Conversation
github-actions
bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Ethereum wallet integration via WalletConnect v2, adding tools for wallet pairing and transaction submission. It also includes several bug fixes for MCP server name normalization, UTF-8 safe string truncation, and OAuth flow logic. However, the current implementation has critical architectural issues: holding a MutexGuard across an await point in the event loop will cause deadlocks, and the wallet_transact tool is currently non-functional as it registers a callback without actually initiating the transaction request. Additionally, git dependencies should be pinned to specific commit hashes, and account address extraction should avoid relying on Debug formatting.
| if let Some(ref wc) = *client { | ||
| match wc.next().await { | ||
| Ok(event) => event, |
There was a problem hiding this comment.
The MutexGuard is held across an .await point in poll_event. Since wc.next().await can block indefinitely while waiting for events from the relay, this will prevent any other task from acquiring the lock. This effectively deadlocks other operations like request(), which also needs to acquire the same lock. Consider an architecture where the client is not locked during long-running awaits, or use a split reader/writer pattern if supported by the underlying library.
| ) -> Result<ToolOutput, ToolError> { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| // Must be paired first. | ||
| if !self.session.is_paired().await { | ||
| return Err(EthereumError::NotPaired.into()); | ||
| } | ||
|
|
||
| // Extract and validate parameters. | ||
| let to = require_str(¶ms, "to")?; | ||
| if !is_valid_eth_address(to) { | ||
| return Err(EthereumError::InvalidAddress { | ||
| address: to.to_string(), | ||
| } | ||
| .into()); | ||
| } | ||
|
|
||
| let value = require_str(¶ms, "value")?; | ||
| let data = params.get("data").and_then(|v| v.as_str()); | ||
| let chain_id = params.get("chain_id").and_then(|v| v.as_u64()); | ||
|
|
||
| // Generate a correlation ID for the async callback. | ||
| let correlation_id = uuid::Uuid::new_v4().to_string(); | ||
|
|
||
| // Extract channel/thread from context metadata for callback routing. | ||
| let channel = ctx | ||
| .metadata | ||
| .get("source_channel") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let thread_id = ctx | ||
| .metadata | ||
| .get("thread_id") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| // Register callback so the result can be routed back. | ||
| self.callback_registry | ||
| .register( | ||
| correlation_id.clone(), | ||
| CallbackMetadata { | ||
| tool_name: "wallet_transact".to_string(), | ||
| user_id: ctx.user_id.clone(), | ||
| thread_id, | ||
| channel, | ||
| }, | ||
| ) | ||
| .await; | ||
|
|
||
| let mut result = json!({ | ||
| "status": "pending", | ||
| "correlation_id": correlation_id, | ||
| "to": to, | ||
| "value": value, | ||
| "message": "Transaction submitted to wallet for approval. \ | ||
| The wallet owner must confirm on their device." | ||
| }); | ||
|
|
||
| if data.is_some() { | ||
| result["data_present"] = json!(true); | ||
| } | ||
| if let Some(cid) = chain_id { | ||
| result["chain_id"] = json!(cid); | ||
| } | ||
|
|
||
| Ok(ToolOutput::success(result, start.elapsed())) |
There was a problem hiding this comment.
The wallet_transact tool registers a callback and returns a pending status, but it never actually initiates the transaction request via self.session.request(). This makes the tool non-functional as the wallet will never receive the signature request. The tool should initiate the request (potentially in a background task if it blocks) before returning the correlation ID.
Cargo.toml
Outdated
| hex = "0.4.3" | ||
|
|
||
| # WalletConnect v2 client for Ethereum transaction approval | ||
| walletconnect-client = { git = "https://github.com/zmanian/walletconnect-client.git", branch = "main", default-features = false, features = ["native"] } |
There was a problem hiding this comment.
Pin git dependencies to a specific commit hash (rev) for reproducible builds, as per the general rules of this repository.
References
- Pin git dependencies to a specific commit hash (
rev) for reproducible builds.
| Ok(event) => event, | ||
| Err(e) => { |
There was a problem hiding this comment.
When wc.next().await returns Ok(None), it indicates that the stream has ended. The current implementation returns None but leaves the client in the Option, causing the listener loop to enter a busy-wait state (polling every 100ms and getting None immediately). The client should be cleared (*client = None) when the stream ends to stop the polling.
| if let Some(ref wc) = *client { | ||
| let address = wc | ||
| .get_account() | ||
| .map(|a| format!("{a:?}")) |
There was a problem hiding this comment.
Using format!("{a:?}") to extract the account address is unreliable as it depends on the Debug implementation of the AccountId type, which often includes the chain ID prefix (e.g., eip155:1:0x...). It is better to use a specific method to extract just the address string if available in the walletconnect-client API.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b397d21bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.callback_registry | ||
| .register( | ||
| correlation_id.clone(), | ||
| CallbackMetadata { | ||
| tool_name: "wallet_transact".to_string(), |
There was a problem hiding this comment.
wallet_transact currently records callback metadata and returns a pending success payload, but never calls WalletConnectSession::request (or any other dispatch path) to actually send eth_sendTransaction. In this commit there is no production code path that consumes the correlation ID to submit the transaction, so users can receive a “submitted for approval” response while nothing was sent and the callback only times out.
Useful? React with 👍 / 👎.
| if matches!(e, WalletConnectError::Disconnected) { | ||
| *client = None; | ||
| } |
There was a problem hiding this comment.
When wc.next() returns WalletConnectError::Disconnected, the code clears wc_client but does not update status, so the session can remain Paired even though no client exists. In that state, pairing/transaction flows can behave inconsistently (e.g., appearing paired first, then failing later), and users may be blocked from cleanly re-pairing after a disconnect.
Useful? React with 👍 / 👎.
…th when header set (#2236, #1947, #1948) - Server names are now auto-normalized (lowercase, special chars to underscores) so descriptive names like "My Twitter Server" are accepted as "my_twitter_server" instead of rejected. - Tool description truncation in `mcp test-server` uses char-safe iteration instead of byte-index slicing, preventing panics when index 57 falls inside a multi-byte UTF-8 character. - `requires_auth()` now returns false when the user has already configured a custom Authorization header, avoiding unnecessary OAuth/DCR flows for API-key-authenticated servers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
force-pushed
the
fix/mcp-client-bugs
branch
from
4b397d2 to
95c41b3
Compare
| // Truncate long descriptions (char-safe to avoid | ||
| // panicking on multi-byte UTF-8 boundaries, #1947) | ||
| let desc = if tool.description.chars().count() > 60 { | ||
| let truncated: String = tool.description.chars().take(57).collect(); |
There was a problem hiding this comment.
Critical — Pre-existing byte-slicing panic (same pattern)
This PR fixes byte-level truncation in test_server, but crates/ironclaw_tui/src/widgets/conversation.rs:863 has the same pattern: .truncate(57) which is byte-based and will panic on multi-byte UTF-8. The PR's own philosophy ("fix the pattern") demands fixing all instances.
Suggested fix: Also fix items_str.truncate(57) in conversation.rs to use chars().take() or floor_char_boundary(), matching the pattern used in this PR.
| /// | ||
| /// This allows users to supply descriptive names like "My Twitter Server" | ||
| /// which are stored as `my_twitter_server` (#2236). | ||
| pub fn normalize_server_name(name: &str) -> String { |
There was a problem hiding this comment.
High — Server-side API handler bypasses name normalization
src/channels/web/handlers/extensions.rs:143 passes req.name directly to ext_mgr.install() without normalization. The JS frontend normalizes on the client side, but direct API calls (e.g., curl) bypass the JS and send raw names. This creates client-server divergence.
Suggested fix: Apply normalize_server_name() on the server side in extensions_install_handler for mcp_server kind, or add name format validation to McpServerConfig::validate().
| anyhow::bail!("Server name '{}' contains no valid characters", raw_name); | ||
| } | ||
| if name != raw_name { | ||
| println!(" (normalized name: '{}' -> '{}')", raw_name, name); |
There was a problem hiding this comment.
Medium — Lookup commands don't normalize names
remove_server, auth_server, test_server, and toggle_server do NOT normalize the user-provided name. If a user adds "My Twitter Server" (stored as my_twitter_server), then runs ironclaw mcp test "My Twitter Server", the lookup fails with "Server not found" and no hint about the normalized name.
Suggested fix: Either normalize in lookup commands too (with a "did you mean 'my_twitter_server'?" fallback), or include the normalized form in the error message when lookup fails.
| /// byte index 57 falls inside a character boundary would panic with | ||
| /// `&tool.description[..57]`. | ||
| #[test] | ||
| fn test_tool_description_truncation_multibyte_utf8() { |
There was a problem hiding this comment.
Medium — Tests duplicate logic instead of testing production code
The truncation regression tests (test_tool_description_truncation_multibyte_utf8, test_tool_description_truncation_ascii) duplicate the truncation logic inline rather than calling the actual production code path. If someone changes the production truncation logic (e.g., changes 57 to 50), these tests would still pass.
Suggested fix: Extract the truncation logic into a helper function used by both test_server and the tests, or write the tests to call test_server with a mock client.
| // Trim trailing underscore | ||
| if result.ends_with('_') { | ||
| result.pop(); | ||
| } |
There was a problem hiding this comment.
Medium — Leading/trailing hyphens not trimmed
normalize_server_name trims leading/trailing underscores but not hyphens. Input "-hello-" produces "-hello-", and consecutive hyphens are not collapsed. Names like --- would produce ---, which could confuse downstream systems using the name in file paths or secret keys (e.g., mcp_---_access_token).
Suggested fix: Treat hyphens symmetrically: also collapse consecutive hyphens and trim leading/trailing hyphens, or document the intentional asymmetry. At minimum, add test cases for "-hello-", "---", and "a--b" to pin the behavior.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Paranoid Architect Review — REQUEST CHANGES
1 Critical, 1 High, 3 Medium findings.
The three bug fixes (normalization, UTF-8 truncation, auth skip) are solid and well-tested. However:
- Critical: A pre-existing
.truncate(57)byte-slicing panic exists inconversation.rs— the same pattern this PR fixes. Fix the pattern, not just the instance. - High: The server-side API handler bypasses name normalization, so direct API calls can inject unnormalized names.
- Medium: Lookup commands don't normalize, tests duplicate logic instead of testing production code, and hyphens are treated asymmetrically with underscores.
2 participants
Summary
Fixes three related MCP client bugs in a single PR:
my_twitter_server(lowercase, spaces/special chars replaced with underscores) instead of being rejected. Applied in CLI, web gateway frontend, and exposed asnormalize_server_name()in config.mcp test-servertool description truncation used byte-index slicing (&desc[..57]) which panics when index 57 falls inside a multi-byte character. Now useschars().take(57).collect().requires_auth()returnedtruefor all remote HTTPS servers even when the user had already configured anAuthorizationheader. Now checkshas_custom_auth_header()first and returnsfalseif set.Test plan
test_normalize_server_name_*- 5 tests covering basic, uppercase, spaces/special chars, empty/all-special, and leading/trailing underscorestest_tool_description_truncation_multibyte_utf8- regression test with CJK characters where byte index 57 falls mid-charactertest_tool_description_truncation_ascii- ASCII descriptions still truncate correctlytest_requires_auth_skipped_with_custom_auth_header- Authorization header skips authtest_requires_auth_skipped_with_lowercase_auth_header- case-insensitive checktest_requires_auth_still_true_with_non_auth_headers- non-Authorization headers don't suppress authcargo clippy --all --all-featurespasses with zero warningsCloses #2236, closes #1947, closes #1948
Generated with Claude Code