mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-02 23:56:24 +08:00
038853f8ee
* refactor(types): adopt MissionId in router + introduce McpServerName - src/bridge/router.rs response DTO now uses engine's MissionId(Uuid) instead of String - McpServerName newtype in ironclaw_common encapsulates the allowlist validation added in #2400 Closes the type gap for two identifiers flagged in the recent audit of string-typed values in the system. * refactor(mcp): address review feedback — validate server names at construction - McpClient::new and new_with_name: validate through McpServerName::new with a canonical "unknown" fallback + debug log, instead of from_trusted. Closes two HIGH-severity allowlist bypasses (e.g. IPv6 bracketed hosts and caller-supplied bad names). - McpClient:🆕 apply the same hyphen→underscore fold as the other constructors (only when a hyphen is present) for consistency. - identity.rs: extract validate_mcp_server_name() helper so TryFrom<String> consumes the owned buffer without re-allocating; MAX_MCP_SERVER_NAME_LEN aliased to MAX_NAME_LEN to prevent drift. - factory.rs: capture validated McpServerName and thread .as_str() through hottest downstream uses; TODO left for full threading. - Adds regression tests covering the IPv6-host and invalid-caller-name paths against the caller (McpClient::new / new_with_name). * fix(mcp): annotate panic-safe McpServerName::new("unknown") .expect() calls The no-panics CI check flagged two `.expect()` calls on `McpServerName::new("unknown")` fallbacks inside `McpClient::new` and `McpClient::new_with_name`. The literal `"unknown"` always satisfies the alnum-only validation rule, so the call is infallible. Document that with an inline `// safety: ...` comment per the project's panic-check suppression convention. * refactor(mcp): validate HttpMcpTransport::new server_name with safe fallback * refactor(mcp): validate McpClient constructor server names with safe fallback Address PR nearai/ironclaw#2681 Copilot review comments 3108427003, 3108427048, and 3108427073. The three remaining constructors (`new_with_transport`, `new_with_config`, `new_authenticated`) were wrapping caller-provided names with `McpServerName::from_trusted`, allowing invalid values to enter the typed field. Beyond bypassing the allowlist, the `_with_config` and `_authenticated` paths also diverged from `HttpMcpTransport::new`, which independently validates and falls back to `"unknown"` — so an invalid name could leave the client keyed one way and the transport another, silently breaking `Mcp-Session-Id` tracking. Each constructor now runs the same canonicalize-with-fallback pattern used by `McpClient::new`, `new_with_name`, and `HttpMcpTransport::new`, and threads the single validated name into both the transport and the client's typed field so the two cannot diverge. Regression tests added (per .claude/rules/testing.md, "Test Through the Caller"): invalid config/transport inputs fall back to "unknown"; valid inputs survive; client and transport names agree for both cases. * fix(mcp): migrate legacy overlong server names at load instead of dropping Address PR nearai/ironclaw#2681 review comment 3110617080. Before the `McpServerName` newtype landed, `McpServerConfig::validate()` only enforced non-empty + `[A-Za-z0-9_-]`. Delegating validation to `McpServerName::new` added a 64-byte length cap — and `load_mcp_servers_from*` silently dropped invalid configs via `retain(...)`, so a legacy persisted server name >64 bytes would vanish from the loaded config on upgrade. The load paths now in-place truncate overlong names at a char boundary (safe even if the pre-validation string contains multi-byte UTF-8), emit a `warn!` documenting the migration, and let the (now cap-satisfying) entry pass validation. Invalid-char cases still drop via `retain`, matching the pre-PR behavior for that class. Regression tests cover both the happy path (ASCII overlong name kept, truncated to exactly the cap) and char-boundary safety (multi-byte sequence straddling byte 64 must not panic).