fix(mcp): address 14 audit findings across MCP module by ilblackdragon · Pull Request #1094 · nearai/ironclaw

ilblackdragon · 2026-03-13T01:23:14Z

Summary

Comprehensive audit and fix of the src/tools/mcp/ module, addressing 14 findings (1 Critical, 2 High, 5 Medium, 4 Low, 1 Nit):

Critical: new_with_config panicked via assert! on non-HTTP config — now returns Result<Self, ToolError>
High: Race condition in initialize() (check-then-set with AtomicBool) — replaced with tokio::sync::OnceCell
High: Localhost check bypassed by evil.localhost.attacker.com — proper URL host parsing via is_localhost_url()
Extracted stream_transport_send() to deduplicate ~120 lines between stdio/unix transports
Atomic file writes (tmp+rename) for MCP config persistence
SSE response filtering by request_id to prevent wrong-response dispatch
Shared reqwest::Client for OAuth calls via LazyLock
Notification send errors logged instead of silently discarded
Fixed unwrap_or(0) that could incorrectly resolve id=0 pending requests
Documented Clone semantics; reset OnceCell on clone
Redirect logging in OAuth discovery responses
Consolidated duplicate is_localhost_url() logic
Added 10 new regression tests (McpToolWrapper, localhost bypass, notification dispatch, transport rejection)
URL-encoded PKCE challenge; collapsed nested if per clippy
Fixed pre-existing duplicate mod path_routing_tests in memory.rs

Test plan

cargo fmt — clean
cargo clippy --all --all-features -- -D warnings — zero warnings in MCP module
cargo test — all 3010+ tests pass (185 MCP-specific)
.githooks/pre-commit — passed
.githooks/pre-push — passed
Verify MCP stdio/unix/HTTP transports work end-to-end with a real MCP server
Verify OAuth flow with an authenticated MCP server

🤖 Generated with Claude Code

gemini-code-assist · 2026-03-13T01:23:19Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot

Pull request overview

This PR addresses multiple audit findings in the src/tools/mcp/ module, focusing on correctness and security hardening across transports, config handling, initialization, and OAuth/discovery behavior.

Changes:

Deduplicates stdio/unix stream send logic and hardens JSON-RPC response dispatch (no-id notifications no longer resolve pending id=0).
Improves HTTP transport SSE handling by filtering events to the matching request_id, and strengthens localhost detection via proper URL parsing.
Refactors MCP client initialization to be concurrency-safe (tokio::sync::OnceCell) and adjusts config persistence to use a temp-file + rename write pattern; consolidates OAuth HTTP client usage.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/mcp/unix_transport.rs	Switches send path to shared `stream_transport_send()` helper.
src/tools/mcp/stdio_transport.rs	Switches send path to shared `stream_transport_send()` helper.
src/tools/mcp/transport.rs	Skips dispatch for responses without `id`; adds `stream_transport_send()` and regression test.
src/tools/mcp/http_transport.rs	Filters SSE events by `request_id` to avoid dispatching unrelated events.
src/tools/mcp/factory.rs	Adapts to `McpClient::new_with_config()` returning `Result` and maps errors to `InvalidConfig`.
src/tools/mcp/config.rs	Uses `is_localhost_url()` and changes config save to temp-write + rename; exports `is_localhost_url()` to crate.
src/tools/mcp/client.rs	Replaces `AtomicBool` init guard with `OnceCell`; changes `new_with_config` to return `Result`; updates tests.
src/tools/mcp/auth.rs	Introduces shared OAuth `reqwest::Client` and redirect debug logging; uses `is_localhost_url`; URL-encodes PKCE challenge.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ilblackdragon · 2026-03-13T16:25:34Z

    pub async fn initialize(&self) -> Result<InitializeResult, ToolError> {
-        // Fast path: already initialized (local flag or session manager)
-        if self.initialized.load(Ordering::Relaxed) {
+        // Fast path: OnceCell already completed.
+        if self.initialized.initialized() {
            return Ok(InitializeResult::default());
        }
-        if let Some(ref session_manager) = self.session_manager
-            && session_manager.is_initialized(&self.server_name).await
-        {
-            self.initialized.store(true, Ordering::Relaxed);
-            return Ok(InitializeResult::default());
-        }
-        if let Some(ref session_manager) = self.session_manager {
-            session_manager
-                .get_or_create(&self.server_name, &self.server_url)
-                .await;
-        }

-        let request = McpRequest::initialize(self.next_request_id());
-        let response = self.send_request(request).await?;
+        // OnceCell ensures only the first concurrent caller runs the init
+        // closure; all others await the result.
+        self.initialized
+            .get_or_try_init(|| async {
+                if let Some(ref session_manager) = self.session_manager
+                    && session_manager.is_initialized(&self.server_name).await
+                {
+                    return Ok(());
+                }
+                if let Some(ref session_manager) = self.session_manager {
+                    session_manager
+                        .get_or_create(&self.server_name, &self.server_url)
+                        .await;
+                }

-        if let Some(error) = response.error {
-            return Err(ToolError::ExternalService(format!(
-                "MCP initialization error: {} (code {})",
-                error.message, error.code
-            )));
-        }
+                let request = McpRequest::initialize(self.next_request_id());
+                let response = self.send_request(request).await?;

-        let result: InitializeResult = response
-            .result
-            .ok_or_else(|| {
-                ToolError::ExternalService("No result in initialize response".to_string())
-            })
-            .and_then(|r| {
-                serde_json::from_value(r).map_err(|e| {
-                    ToolError::ExternalService(format!("Invalid initialize result: {}", e))
-                })
-            })?;
+                if let Some(error) = response.error {
+                    return Err(ToolError::ExternalService(format!(
+                        "MCP initialization error: {} (code {})",
+                        error.message, error.code
+                    )));
+                }

-        if let Some(ref session_manager) = self.session_manager {
-            session_manager.mark_initialized(&self.server_name).await;
-        }
-        self.initialized.store(true, Ordering::Relaxed);
+                let _result: InitializeResult = response
+                    .result
+                    .ok_or_else(|| {
+                        ToolError::ExternalService("No result in initialize response".to_string())
+                    })
+                    .and_then(|r| {
+                        serde_json::from_value(r).map_err(|e| {
+                            ToolError::ExternalService(format!("Invalid initialize result: {}", e))
+                        })
+                    })?;
+
+                if let Some(ref session_manager) = self.session_manager {
+                    session_manager.mark_initialized(&self.server_name).await;
+                }

-        let notification = McpRequest::initialized_notification();
-        let _ = self.send_request(notification).await;
+                let notification = McpRequest::initialized_notification();
+                if let Err(e) = self.send_request(notification).await {
+                    tracing::debug!(
+                        "Failed to send initialized notification to '{}': {}",
+                        self.server_name,
+                        e
+                    );
+                }

-        Ok(result)
+                Ok(())
+            })
+            .await?;
+
+        Ok(InitializeResult::default())
    }


Fixed — OnceCell<InitializeResult> now stores the parsed result and returns a clone. Subsequent calls get the cached result instead of default().

ilblackdragon · 2026-03-13T16:25:37Z

+    // Write to a temporary file first, then atomically rename to avoid
+    // corrupting the config if the process crashes during the write.
+    let tmp_path = path.with_extension("json.tmp");
+    fs::write(&tmp_path, content).await?;
+    fs::rename(&tmp_path, path).await?;


Acknowledged. This is a valid concern for Windows. Since IronClaw currently targets Linux/macOS (the project uses Unix sockets, systemd, launchd), this is acceptable for now. Filed as a follow-up if Windows support becomes a priority.

ilblackdragon · 2026-03-13T16:25:39Z

+            .timeout(Duration::from_secs(30))
+            .redirect(reqwest::redirect::Policy::none())
+            .build()
+            .expect("Failed to create OAuth HTTP client")


Fixed — replaced .expect() with fallible OnceLock<Result<Client, String>> that returns AuthError::Http on build failure. All 7 call sites now use oauth_http_client()?.

- Replace panicking assert! in new_with_config with Result return (Critical) - Fix initialize() race condition using tokio::sync::OnceCell (High) - Fix localhost check bypass via proper URL parsing (High) - Extract shared stream_transport_send() to deduplicate stdio/unix send logic - Use atomic write (tmp+rename) for config file persistence - Filter SSE responses by request_id to prevent wrong-response dispatch - Share a single reqwest::Client for OAuth via fallible OnceLock - Log notification send errors instead of silently discarding - Fix unwrap_or(0) that could steal id=0 responses - Store InitializeResult in OnceCell so callers can access server capabilities - Add redirect logging in OAuth discovery - Reuse is_localhost_url() in auth.rs - Add McpToolWrapper unit tests and regression tests - URL-encode PKCE challenge for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian

Security Review

Strong security PR addressing 14 audit findings. The three highest-priority items are all correctly fixed:

assert! -> Result (Critical): No more panics on misconfigured MCP transport type.
Race condition in initialize() (High): tokio::sync::OnceCell::get_or_try_init() is the right primitive. Clone resets the cell, correct given session manager short-circuit.
Localhost bypass (High): Proper URL host parsing via is_localhost_url() replaces the vulnerable url.contains("localhost") check. Regression test for evil.localhost.attacker.com is exactly right.

Other notable improvements:

SSE response filtering by request_id prevents wrong-response dispatch
Notification id=0 stealing bug fixed with regression test
Atomic config file writes (tmp+rename)
Shared OAuth client with redirects disabled

10 regression tests covering the security-critical fixes. Well done.

Minor note: In stream_transport_send, let id = request.id.unwrap_or(0) after the is_none() early return is technically safe but slightly misleading. Not blocking.

* fix(mcp): address 14 audit findings across MCP module - Replace panicking assert! in new_with_config with Result return (Critical) - Fix initialize() race condition using tokio::sync::OnceCell (High) - Fix localhost check bypass via proper URL parsing (High) - Extract shared stream_transport_send() to deduplicate stdio/unix send logic - Use atomic write (tmp+rename) for config file persistence - Filter SSE responses by request_id to prevent wrong-response dispatch - Share a single reqwest::Client for OAuth via fallible OnceLock - Log notification send errors instead of silently discarding - Fix unwrap_or(0) that could steal id=0 responses - Store InitializeResult in OnceCell so callers can access server capabilities - Add redirect logging in OAuth discovery - Reuse is_localhost_url() in auth.rs - Add McpToolWrapper unit tests and regression tests - URL-encode PKCE challenge for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: retrigger CI with skip-regression-check label Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 13, 2026 01:23

Copilot AI reviewed Mar 13, 2026

View reviewed changes

ilblackdragon changed the base branch from main to staging March 13, 2026 01:25

github-actions bot removed the risk: high Safety, secrets, auth, or critical infrastructure label Mar 13, 2026

ilblackdragon requested a review from Copilot March 13, 2026 01:40

github-actions bot added the risk: medium Business logic, config, or moderate-risk modules label Mar 13, 2026

Copilot started reviewing on behalf of ilblackdragon March 13, 2026 01:40 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

github-actions bot mentioned this pull request Mar 13, 2026

🦞 Bản tin hàng ngày hệ sinh thái OpenClaw 2026-03-13 compasify/agents-radar#36

Open

ilblackdragon force-pushed the fix/mcp-audit-fixes branch 2 times, most recently from eb3f3f9 to c8137d6 Compare March 13, 2026 16:25

ilblackdragon added the skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir) label Mar 13, 2026

chore: retrigger CI with skip-regression-check label

5276b3d

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian approved these changes Mar 13, 2026

View reviewed changes

ilblackdragon merged commit f53c1bb into staging Mar 13, 2026
13 checks passed

ilblackdragon deleted the fix/mcp-audit-fixes branch March 13, 2026 17:37

ironclaw-ci bot mentioned this pull request Mar 13, 2026

chore: promote staging to staging-promote/bc672520-23062088162 (2026-03-13 18:08 UTC) #1137

Merged

github-actions bot mentioned this pull request Mar 16, 2026

chore: promote staging to main (2026-03-13 16:09 UTC) #1132

Merged

ironclaw-ci bot mentioned this pull request Mar 17, 2026

chore: release v0.19.0 #973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): address 14 audit findings across MCP module#1094

fix(mcp): address 14 audit findings across MCP module#1094
ilblackdragon merged 2 commits intostagingfrom
fix/mcp-audit-fixes

ilblackdragon commented Mar 13, 2026

Uh oh!

gemini-code-assist bot commented Mar 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

ilblackdragon Mar 13, 2026

Uh oh!

ilblackdragon Mar 13, 2026

Uh oh!

ilblackdragon Mar 13, 2026

Uh oh!

zmanian left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ilblackdragon commented Mar 13, 2026

Summary

Test plan

Uh oh!

gemini-code-assist bot commented Mar 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

ilblackdragon Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ilblackdragon Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ilblackdragon Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

zmanian left a comment

Choose a reason for hiding this comment

Security Review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants