[codex] Tighten auth flows and unify live canary coverage - #2367
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 a comprehensive live canary regression system for auth flows, including new browser-based and seeded-token runners. It also refactors the authentication manager and MCP client architecture to support multi-user isolation, ensuring that MCP sessions and tokens are correctly partitioned by user. My review highlights a potential issue with the McpClient::for_user implementation regarding shared state for Stdio transports, which could lead to request ID collisions and redundant handshakes.
| *.png|*.jpg|*.jpeg|*.gif|*.webp|*.sqlite|*.db|*.wasm|*.zip) continue ;; | ||
| esac | ||
| for pattern in "${patterns[@]}"; do | ||
| if grep -nIEi "${pattern}" "${file}" >> "${matches_file}" 2>/dev/null; then |
There was a problem hiding this comment.
Critical Severity
This scrubber persists raw secret matches inside the same artifact directory it is supposed to sanitize. grep appends matching lines to artifacts/live-canary/scrub-matches.txt, and the workflow later uploads artifacts/live-canary/; in non-strict lanes it even continues after matches are found. That means a leaked bearer token/PAT in any log gets copied into a new artifact file and uploaded. The console redaction also only handles some prefix forms, so raw ghp_, github_pat_, ya29, etc. matches can still be printed unredacted.
Please write matches to a temp file outside the upload tree, ensure every persisted/printed match is redacted before it exists under the artifact directory, and make secret matches fail or remove the affected upload payload.
There was a problem hiding this comment.
Fixed in d59426ee.
The scrubber now writes raw grep matches only to temp files outside the artifact upload tree, persists scrub-matches.txt with redacted values only, and redacts direct token forms like ghp_, github_pat_, ya29, Slack tokens, and Anthropic keys before printing or saving. In strict mode it now removes matched payload files before exiting non-zero so they cannot be uploaded after a secret hit.
Validation: bash -n scripts/live-canary/scrub-artifacts.sh, strict scrubber smoke test with a fake ghp_... token confirming exit 1, source payload removal, and redacted-only match output.
| "provider": "github", | ||
| "oauth": { | ||
| "authorization_url": "https://github.com/login/oauth/authorize", | ||
| "token_url": "https://github.com/login/oauth/access_token", |
There was a problem hiding this comment.
High Severity
The new GitHub OAuth descriptor points at https://github.com/login/oauth/access_token, but the shared OAuth exchanger parses the token response as JSON and does not set Accept: application/json. GitHub returns a form-encoded access-token response by default and only returns JSON when that Accept header is present, so this browser OAuth path will fail after the user consents.
Please add provider token request headers or a urlencoded fallback for this descriptor, and cover it with a mock GitHub token endpoint regression test.
There was a problem hiding this comment.
Fixed in d59426ee.
The shared OAuth token exchange now sends Accept: application/json and falls back to parsing application/x-www-form-urlencoded style token responses. That covers GitHub's default token response shape even if JSON is not returned.
Validation: added test_github_form_encoded_token_response_parses; ran cargo test auth::oauth::tests --lib.
| // the same credential should reuse a single pending entry rather than | ||
| // accumulate stale flows. This logic used to live in | ||
| // bridge::auth_manager and was lost when the call moved here; without | ||
| // crate::auth::extension and was lost when the call moved here; without |
There was a problem hiding this comment.
High Severity
This pending-flow path was made user-aware here, but the surrounding pending-auth lifecycle is still extension-scoped: pending_auth is keyed only by extension name, and clear_pending_extension_auth(name) / remove() retain gateway flows only by flow.extension_name != name. That breaks multi-user auth isolation: if user A has a pending OAuth flow for github/notion and user B starts auth for the same extension, B's auth call clears A's pending callback state, so A's eventual callback can no longer complete.
Please key pending auth cleanup by (user_id, extension) or thread user_id into clear_pending_extension_auth, and add a two-user same-extension pending OAuth regression test.
There was a problem hiding this comment.
Fixed in d59426ee.
Pending auth cleanup is now keyed by runtime user plus extension. clear_pending_extension_auth() and remove() only clear pending OAuth flows matching both extension_name and user_id, so user B starting or removing github no longer clears user A's pending callback state.
Validation: added test_clear_pending_extension_auth_only_clears_matching_user_flow; existing test_remove_wasm_tool_clears_pending_oauth_state_and_activation_error was updated for the user-scoped key and passes.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code review covering multi-user MCP isolation, credential handling in live canary infrastructure, and auth URL sanitization.
| let url: String = server_url.into(); | ||
| let name = extract_server_name(&url); | ||
| let transport = Arc::new(HttpMcpTransport::new(url.clone(), name.clone())); | ||
| let runtime_state = Self::new_runtime_state(); |
There was a problem hiding this comment.
Critical Severity — Multi-user MCP isolation fragility for stdio transports
When for_user() is called on stdio transports, it shares initialized (OnceCell), tools_cache, and next_id across all users (lines 316-321). The first user's initialization wins and InitializeResult (server capabilities) is frozen from that user's perspective. If a future MCP server returns user-scoped capabilities, they leak across users.
The test test_stdio_for_user_shares_initialize_cache_and_request_ids validates this as intentional behavior, which is fine for current MCP servers. However, this shared-state assumption should be documented as a contract on McpClient itself (not just in the Clone impl doc), so future maintainers know adding user-scoped capabilities to stdio transports would require breaking this sharing.
Suggested fix: Add a doc comment on for_user() explicitly stating that stdio transports share initialization/tool-cache across all users by design, and that changing this would require per-user OnceCells.
There was a problem hiding this comment.
Fixed in d59426ee.
Added the McpClient::for_user() contract doc directly on the method: HTTP user views keep per-user initialization/session/tool-cache state, while stdio/UDS intentionally share initialization, request IDs, and tool cache because they address a single underlying process. The comment also calls out that user-scoped stdio capabilities would require revisiting this and moving to per-user OnceCells/tool caches.
Validation: cargo test tools::mcp::client::tests --lib.
| -e 's/(secret[[:space:]]*[:=][[:space:]]*)[^[:space:]]+/\1<REDACTED>/Ig' \ | ||
| "${matches_file}" | head -200 | ||
| if [[ "${STRICT_ARTIFACT_SCRUB}" == "true" || "${STRICT_ARTIFACT_SCRUB}" == "1" ]]; then | ||
| exit 1 |
There was a problem hiding this comment.
High Severity — STRICT_ARTIFACT_SCRUB not set for lanes handling real credentials
STRICT_ARTIFACT_SCRUB is only set to true for the private-oauth lane (visible in .github/workflows/live-canary.yml). However, auth-live-seeded and auth-browser-consent lanes handle real provider tokens (Google OAuth, GitHub PAT) but upload artifacts with only the best-effort regex scrub here. If the regex misses a token format, it flows into uploaded CI artifacts.
Line 47: if [[ "${STRICT_ARTIFACT_SCRUB}" == "true" ...]] — the fallthrough prints a warning and continues.
Suggested fix: Set STRICT_ARTIFACT_SCRUB=true for all lanes that handle real provider credentials (auth-live-seeded, auth-browser-consent), not just private-oauth. The regex scrub is best-effort and should not be the only gate for lanes with real tokens.
There was a problem hiding this comment.
Fixed in d59426ee.
Set STRICT_ARTIFACT_SCRUB=true for both real-credential GitHub Actions lanes: auth-live-seeded and auth-browser-consent.
Validation: bash -n scripts/live-canary/run.sh scripts/live-canary/scrub-artifacts.sh and the strict scrubber smoke test.
| DEFAULT_VENV = E2E_DIR / ".venv" | ||
| DEFAULT_SECRETS_MASTER_KEY = ( | ||
| "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" | ||
| ) |
There was a problem hiding this comment.
High Severity — Hardcoded predictable master key in committed source
DEFAULT_SECRETS_MASTER_KEY = (
"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
)This key is committed to the repo. Canary databases created with it (which include real provider tokens for auth-live-seeded) can be trivially decrypted by anyone reading this source. While the canary databases are ephemeral, there is a window during which real OAuth tokens are encrypted with a publicly-known key.
Suggested fix: Use os.urandom(32).hex() per canary run to generate a unique master key. This ensures that even if a canary database leaks (e.g., via CI artifacts), the tokens inside cannot be decrypted. Verify cleanup covers all failure modes so the DB+key are never persisted together.
There was a problem hiding this comment.
Fixed in d59426ee.
Removed the committed default canary master key from scripts/live_canary/common.py. start_gateway_stack() now generates a fresh os.urandom(32).hex() key per run unless a caller explicitly passes a key, and the seeded/browser canary runners no longer pass a shared constant.
Validation: PYTHONPYCACHEPREFIX=/tmp/ironclaw-pr2367-pycache python3 -m py_compile scripts/live_canary/common.py scripts/auth_live_canary/run_live_canary.py scripts/auth_browser_canary/run_browser_canary.py.
| return None; | ||
| } | ||
| url::Url::parse(u) | ||
| .ok() |
There was a problem hiding this comment.
High Severity — Auth URL sanitization returns original string, not parsed URL
sanitize_auth_url parses with url::Url::parse() but returns the original string u.to_owned(). Percent-encoded CRLF sequences (%0d%0a) pass the char::is_control check (which only catches literal control chars in the input string) and flow through to the caller. After the URL is later used in an HTTP context (e.g., Location header), the server or browser may decode the percent-encoding, enabling header injection.
The existing test rejects_invalid_or_control_character_urls only tests literal \n and \r, not their percent-encoded forms.
Suggested fix: Return parsed.to_string() instead of u.to_owned() to get the normalized, canonicalized URL. Also add test cases for %0d%0a sequences.
There was a problem hiding this comment.
Fixed in d59426ee.
sanitize_auth_url() now rejects percent-decoded control characters before accepting an auth URL and returns the parsed canonical URL instead of the original input string. Added %0d%0a regression cases.
Validation: cargo test sanitize_tests --lib.
| } | ||
|
|
||
| /// Get or create a session for a server. | ||
| pub async fn get_or_create(&self, server_name: &str, server_url: &str) -> McpSession { |
There was a problem hiding this comment.
Medium Severity — Unbounded session growth
Session map is now keyed by (user_id, server_name) which increases the cardinality compared to the previous server-only key. cleanup_stale() exists but there is no evidence it is called periodically (no timer, no background task scheduling it). In a multi-user deployment, the session map grows without bound until the process restarts.
Suggested fix: Either call cleanup_stale() on a timer (e.g., every 5 minutes via a tokio interval task) or add a max capacity with LRU eviction. The unbounded growth is more impactful now that the key space is O(users * servers) rather than O(servers).
There was a problem hiding this comment.
Fixed in d59426ee.
McpSessionManager now has a hard capacity limit in addition to stale cleanup. get_or_create() opportunistically removes stale sessions and evicts the oldest active session when the (user_id, server_name) map reaches capacity, bounding growth across users and servers.
Validation: added test_session_manager_evicts_oldest_when_capacity_is_reached; ran cargo test tools::mcp::session::tests --lib.
|
|
||
| /// Server name (for logging and session management). | ||
| server_name: String, | ||
|
|
There was a problem hiding this comment.
Medium Severity — No user_id validation in for_user()
for_user() accepts any impl Into<String>. An empty string or a string containing path separators could cause issues in session keys, secret paths, or log parsing. Given that the user_id flows into McpSessionKey, secret lookups, and header values, basic validation would prevent subtle bugs.
Suggested fix: Validate that user_id is non-empty and does not contain path separators (/, \) or control characters. Return a Result or panic-in-debug to surface misuse early.
There was a problem hiding this comment.
Fixed in d59426ee.
McpClient::for_user() now validates user_id and returns ToolError::InvalidParameters for empty IDs, path separators, or control characters before building a user-scoped client view.
Validation: added test_for_user_rejects_invalid_user_ids; ran cargo test tools::mcp::client::tests --lib.
|
|
||
| Ok(Self { | ||
| transport, | ||
| server_url: config.url.clone(), |
There was a problem hiding this comment.
Medium Severity — New HTTP transport created per tool call
In McpToolWrapper::execute(), self.client.for_user(&ctx.user_id) is called on every tool execution. For HTTP transports, for_user() creates a new HttpMcpTransport with a fresh OnceCell, meaning the MCP initialize handshake is attempted on every single tool call. This adds latency and unnecessary server load.
For stdio transports this is fine (shared runtime state), but HTTP callers pay a per-call initialization tax.
Suggested fix: Cache the per-user client in McpToolWrapper or ExtensionManager so that repeated calls from the same user reuse the same transport and initialization state.
There was a problem hiding this comment.
Fixed in d59426ee.
McpClient::for_user() now caches HTTP user-specific client views by runtime user id, and McpToolWrapper::execute() reuses that cached view. That preserves per-user HTTP session/initialize/tool-cache state across repeated calls instead of creating a fresh transport and OnceCell every execution. Stdio/UDS still share the underlying process runtime state by design.
Validation: added test_mcp_tool_wrapper_reuses_http_user_client_between_calls; ran cargo test tools::mcp::client::tests --lib.
…anary-unification # Conflicts: # deny.toml # src/bridge/effect_adapter.rs # src/channels/web/server.rs
…ressions
Addresses PR 2367 review feedback. Two workstreams.
Canary consolidation (addresses "5 top-level canary dirs" review nit):
- Collapse scripts/auth_browser_canary/ into scripts/auth_live_canary/
with a --mode {seeded,browser} flag. The two runners shared 93% of
their CLI, bootstrap, and stack orchestration.
- Delete scripts/auth_browser_canary/ (4 files, ~684 lines).
- Update run.sh dispatch so auth-live-seeded → --mode seeded and
auth-browser-consent → --mode browser. Lane names unchanged; workflow
YAML needs no edit.
- Fold browser-mode env vars into auth_live_canary/config.example.env
and merge ACCOUNTS.md references.
- Document the live-canary/ (shell) vs live_canary/ (Python package)
split inline so the naming isn't a trap.
Restore regressions dropped in the earlier origin/staging merge:
- ExtensionManager.pending_auth: re-key by (user_id, name) via a
PendingAuthKey struct instead of the bare extension name. Threaded
user_id through clear_pending_extension_auth + all insert/remove
sites. Without this, user A and user B collided on the same
extension's pending-auth state.
- McpSessionManager: re-add DEFAULT_MAX_SESSIONS + max_sessions field
+ with_limits() constructor + oldest-by-last_activity eviction in
get_or_create. Unbounded growth would have leaked one HashMap entry
per unique (user, server) forever.
- McpClient::for_user: re-add is_valid_mcp_user_id validation, bounded
UserClientCache (256-entry FIFO), and Result<Arc<Self>, ToolError>
return type. Cache means repeated tool calls from the same user skip
the initialize handshake.
Follow-up nits from the same review:
- MCP_MAX_SESSIONS env knob in app.rs so operators can raise the cap
without rebuilding (B4).
- Extract drop_pending_oauth_flows_for helper; two retain sites in
manager.rs now share one predicate (B5).
- Annotate the 5 cron schedules in .github/workflows/live-canary.yml
with which lanes each drives (B6).
Collateral: fix two stale crate::bridge::auth_manager::AuthManager
references in src/channels/web/server.rs left over from the earlier
module rename; without this, cargo test didn't compile.
Regression tests:
- test_session_manager_evicts_oldest_when_capacity_is_reached
- test_for_user_rejects_invalid_user_ids
- test_mcp_tool_wrapper_reuses_http_user_client_between_calls
All three assert on the specific class of bug the respective fix
prevents.
Verification:
- cargo check --no-default-features --features libsql: clean
- cargo clippy --no-default-features --features libsql --lib --tests:
zero warnings
- cargo fmt --check: clean
- cargo test tools::mcp -- --test-threads=1: 225 pass
- cargo test extensions::manager::tests: 109 pass
- cargo test --test mcp_multi_tenant_integration: both pass
- Both canary --mode {seeded,browser} --list-cases work
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolve parallel regression fix (d59426e by firat) against local f4015be. Both branches independently addressed the same PR 2367 review findings; the merge keeps the stronger design choices from each side. Conflict resolutions: - src/extensions/manager.rs: keep PendingAuthKey tuple struct (mine) over the `\u{1f}`-joined string (theirs). Named struct is collision-proof. Merge in their new regression test `test_clear_pending_extension_auth_only_clears_matching_user_flow`, rewriting it to use PendingAuthKey::new. Keep both drop_pending_oauth_flows_for helper and the retain site refactor. - src/tools/mcp/client.rs: keep bounded UserClientCache with 256-entry FIFO eviction (mine) over uncapped HashMap (theirs). Adopt their cleaner `new_user_client_cache()` helper pattern at all insert sites. Keep shared cache across clones. - src/tools/mcp/session.rs: take mine; content is identical after deduplicating the `test_session_manager_evicts_oldest_when_capacity_is_reached` test both branches added. - scripts/auth_live_canary/run_live_canary.py: keep mine (unified --mode {seeded,browser}). Drop the stale DEFAULT_SECRETS_MASTER_KEY call-site arg — their common.py change made secrets_master_key optional with an auto-generator. - scripts/auth_browser_canary/run_browser_canary.py: force-delete (modify/delete conflict — mine deleted, they made minor edits). - .github/workflows/live-canary.yml: auto-merged (my cron schedule comments + their job `if:` edits). - src/auth/oauth.rs, scripts/live-canary/scrub-artifacts.sh, scripts/live_canary/common.py: their changes only; taken as-is. Verification: - cargo check --no-default-features --features libsql --tests: clean - cargo clippy --no-default-features --features libsql --lib --tests: zero warnings - cargo fmt --check: clean - cargo test tools::mcp -- --test-threads=1: 225 pass - cargo test extensions::manager::tests -- --test-threads=1: 110 pass (includes the new per-user pending_auth regression test from theirs) - cargo test --test mcp_multi_tenant_integration: both pass
In bash strict mode (set -u), the run_python_lane() function would fail when case_args or passthrough_args arrays were empty due to unquoted array expansion. Temporarily disable strict mode for these expansions to allow empty arrays to expand to no arguments (rather than an empty string). This fixes all three auth canary lanes: - LANE=auth-live-seeded - LANE=auth-browser-consent - LANE=auth-smoke Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 62 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two follow-up reviewer findings on top of 0df70e4. tests/e2e/mock_llm.py: the `AUTH_LIVE_GOOGLE_*` override in both `oauth_exchange` and `oauth_refresh` was gated only on "not an MCP request" (`not code.startswith("mock_mcp_code")` / `not provider.startswith("mcp:")`). GitHub and Notion flows would have fallen into the override branch and received Google tokens, masking real provider-specific failures in the auth-live-seeded canary. Gate strictly on the Google `token_url` host via a new `_is_google_token_url` helper; non-Google providers now fall through to their real mock validation path. docs/extensions/github.md: "remember then when creating issues" → "remember them". Typo spotted in the same file the earlier commit was correcting.
Follow-up on review of 0df70e4. The previous fix moved one way — declared `environment: auth-live-canary` / `auth-browser-canary` on the two lanes — because `ACCOUNTS.md` claimed those Environments were in use. In fact no such GitHub Environments are configured; secrets live at repo scope and the jobs read them directly. Revert the `environment:` declarations on auth-live-seeded and auth-browser-consent (they would have required operators to create empty Environments on GitHub before scheduled runs could start) and update `ACCOUNTS.md` to describe the actual repo-scope setup, plus a migration note for operators who later want real env isolation.
| // tool wrappers are already registered. We still need to insert | ||
| // *this* user's client below so per-user dispatch routes to the | ||
| // right credential. | ||
| if self.mcp_clients.contains(user_id, name).await { |
There was a problem hiding this comment.
[P2] Reusing server names across users leaks stdio MCP child processes
The old global already active short-circuit is now per-user, so a second user activating the same stdio MCP server will fall through and spawn a fresh child process. McpProcessManager::spawn_stdio() is still keyed only by server_name, which means the new transport overwrites the old handle without shutting it down. On shared stdio servers this leaves orphaned MCP processes behind, and shutdown_all() or any later restart logic can only manage the most recently activated user's child.
There was a problem hiding this comment.
Fixed in 13b7638. McpProcessManager's transports/configs maps are now keyed by a new composite McpProcessKey(user_id, server_name)
instead of just server_name. A second user activating the same stdio server now gets its own tracked entry instead of overwriting the
first user's handle — spawn_stdio, shutdown, try_restart, get, and managed_servers all take user_id. Same-user re-activation also shuts
the old child down before spawning the replacement so orphans can't accumulate. Mirrors the existing McpClientStore partitioning from
d93243b.
| // partitioning of the client store addressed the runtime | ||
| // dispatch leak, but the registry surface was still global and | ||
| // susceptible to the same cross-tenant leak. | ||
| let surface_signature = crate::tools::mcp::surface_signature(&mcp_tools); |
There was a problem hiding this comment.
[P2] Don't persist conflicting cached_tools on a rejected activation
The new surface-conflict check returns an error here, but updated_server.cached_tools has already been written just above. latent_provider_actions() intentionally exposes server.cached_tools for inactive MCP servers, so after this rejection the affected user will still see tool names and schemas from a backend that cannot be activated while the other user owns the shared server name. Moving the cache write after the conflict check, or rolling it back on this branch, avoids advertising impossible actions.
There was a problem hiding this comment.
Fixed in 13b7638. In activate_mcp, the tool-surface conflict check now runs before updated_server.cached_tools = mcp_tools.clone() is
persisted via update_mcp_server. Previously the cache write happened first, so a subsequent rejection left latent_provider_actions()
advertising tools from a backend that couldn't actually be activated for that user. Reordering keeps the persisted cache consistent
with what's actually activatable.
Reviewer flagged that libSQL runs in WAL mode (see
`src/db/libsql/mod.rs` line 334 — `PRAGMA journal_mode=WAL`), so
recent committed writes may live in `ironclaw.db-wal` rather than
the main file. `cp "${DB_PATH}" ...` alone can silently drop those
writes — a stale OAuth refresh token on the runner even though the
local DB looks current.
In practice the current workflow (stop ironclaw → run this script)
keeps the main file authoritative because SQLite checkpoints on
clean shutdown. But a future operator running the script while
ironclaw is up would hit the bug. Run `PRAGMA wal_checkpoint(TRUNCATE)`
before `cp` — cheap (~10 ms on an idle DB), works on a busy DB too,
and makes the script correct regardless of whether ironclaw is
running.
Also add sqlite3 to the dependency preflight check.
…aths 1. McpProcessManager now partitions stdio children by (user_id, server_name). Previously `transports` and `configs` were keyed by `server_name` only, so a second user activating the same stdio MCP server would overwrite the prior user's transport handle in the map, leaving the prior child process orphaned. The Arc in the prior user's `McpClient` kept the process alive for dispatch, but `shutdown_all` / `try_restart` / `managed_servers` all lost visibility of it. Added `McpProcessKey(user_id, server_name)` + threaded `user_id` through spawn / shutdown / restart / get / managed_servers, mirroring the `McpClientStore` partitioning from d93243b. Factory.rs and the single main.rs caller updated. 2. Startup MCP client injection in src/app.rs was passing the raw config-row `server.name` (hyphens preserved) while the created client and wrappers had already been normalized to underscores by `create_client_from_config`. Result: the client landed in McpClientStore under "my-mcp-server" while wrappers looked up "my_mcp_server" at dispatch — every tool call failed with "MCP server '…' is not active for this user" until manual reactivation. Source the name from `client.server_name()` (the already-normalized canonical field) so the insert key matches the dispatch-time lookup key. 3. activate_mcp in src/extensions/manager.rs now performs the tool-surface conflict check BEFORE persisting `updated_server.cached_tools`. Previously the cache write happened first; if the conflict check then rejected, the server's persisted `cached_tools` still contained the new surface, and `latent_provider_actions()` advertised tools from a backend that couldn't actually be activated for this user.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Review: MCP isolation still misses approval-policy divergence
The branch has clearly closed several real multi-user auth/MCP issues, and the targeted MCP integration coverage is in much better shape. I still don't think this is ready to merge because the new same-name MCP surface-conflict guard does not include tool annotations, so approval policy can still leak across users.
What looks good:
- The per-user MCP client/session/process partitioning is the right direction and the concurrency guard around activate/remove materially improves the lifecycle safety.
- The follow-up fixes for cached tool persistence ordering, startup MCP name normalization, and stdio process ownership address real bugs from earlier review rounds.
- The targeted multi-tenant verification is meaningful:
cargo test --test mcp_multi_tenant_integration --features libsqlpasses on this branch.
Critical: same-name MCP conflict detection still ignores approval annotations
src/tools/mcp/client_store.rs::surface_signature() now decides whether two users are allowed to share one global wrapper surface for the same server_name, and both inject_mcp_client() and activate_mcp() rely on that hash to reject divergent backends. But the hash currently includes only tool name, description, and input schema.
That is not the full runtime surface. Approval policy comes from MCP annotations: McpTool::requires_approval() reads annotations.destructive_hint, and McpToolWrapper::requires_approval() uses that to decide whether the globally-registered wrapper is gated. If two users get the same tool names/schemas/descriptions but different annotations, the conflict check will treat them as identical and allow the second activation, even though one user's backend may require approval while the other's does not. Because the tool registry is global per tool name, one user's approval semantics then become the shared wrapper policy for both users.
Please include annotations (at least the approval-relevant ones) in the surface fingerprint and add a regression test that proves same-name cross-user activation is rejected when only the MCP annotations differ.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 63 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn surface_signature(tools: &[McpTool]) -> String { | ||
| let mut entries: Vec<(String, String, String)> = tools | ||
| .iter() | ||
| .map(|t| { | ||
| ( | ||
| t.name.clone(), | ||
| t.description.clone(), | ||
| serde_json::to_string(&t.input_schema).unwrap_or_default(), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
surface_signature uses serde_json::to_string(&t.input_schema) as part of the hash. JSON object key order is not semantically meaningful and can legitimately vary across responses, so two equivalent schemas could produce different strings and incorrectly trip check_surface_conflict, blocking multi-user activation for the same server. Consider hashing a canonicalized form of the schema (e.g., recursively sort object keys before serialization, or serialize via a stable canonical JSON routine) so the signature is order-insensitive.
There was a problem hiding this comment.
Include canonicalized annotations in the hash + integration test activate_rejects_divergent_annotations_on_shared_server_name + unit tests
| def reserve_loopback_port() -> int: | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: | ||
| sock.bind(("127.0.0.1", 0)) | ||
| return sock.getsockname()[1] | ||
|
|
There was a problem hiding this comment.
reserve_loopback_port() binds to port 0 to pick a free port, then immediately closes the socket and later starts the child process on that port. This has a TOCTOU race where another process can claim the port between reservation and spawn, causing flaky canary runs. Prefer letting the child bind to port 0 itself and report the chosen port (as mock_llm.py already does), or keep the reserving socket open until the child is ready, or add retry-on-bind-failure logic when starting subprocesses.
There was a problem hiding this comment.
canonicalize_json() recursively sorts object keys before hashing + regression test surface_signature_is_object_key_order_insensitive
| // Same-user re-activation: shut the previous child down before | ||
| // the new one takes its slot so the old process doesn't become | ||
| // an orphan. | ||
| if let Some(old_transport) = self.transports.write().await.remove(&key) | ||
| && let Err(e) = old_transport.shutdown().await | ||
| { | ||
| tracing::warn!( | ||
| user_id = %user_id, | ||
| server = %name, | ||
| error = %e, | ||
| "Failed to shut down previous stdio MCP child before replacement" | ||
| ); |
There was a problem hiding this comment.
spawn_stdio removes an existing transport under a RwLock write guard and then awaits old_transport.shutdown(). Because the write-guard is created as a temporary (self.transports.write().await.remove(...)), it can be held across the .await, blocking other users from spawning/getting/shutting down transports and risking deadlocks if shutdown paths ever need the same lock. Refactor to fetch/remove the old transport in a separate scope (drop the lock), then perform the async shutdown afterward.
There was a problem hiding this comment.
Scoped the remove inside a block so the guard drops before .await; comments explain the invariant
…mock_llm port race Four follow-up review findings on top of 13b7638. 1. `surface_signature` now includes MCP tool annotations in the fingerprint, not just name/description/input_schema. Annotations drive `McpTool::requires_approval` (via `destructive_hint`), and ToolRegistry keys wrappers by tool name only — without this dimension in the hash, two tenants whose backends returned the same schema but different `destructive_hint` would be treated as identical surfaces and the globally-registered wrapper's approval policy would leak across users. Integration test `activate_rejects_divergent_annotations_on_shared_server_name` drives two mock MCP servers through the full ExtensionManager path with annotation-only divergence and asserts the second user's activation is rejected. 2. `surface_signature` now canonicalizes JSON values by sorting object keys recursively before hashing. `serde_json::to_string` preserves input key order, so a spec-compliant backend that emits `{"a":1,"b":2}` on one call and `{"b":2,"a":1}` on the next — both legal — would have falsely tripped the cross-tenant conflict check. Unit test `surface_signature_is_object_key_order_insensitive` proves equivalent-but-reordered schemas now fingerprint identically. 3. `McpProcessManager::spawn_stdio` and `try_restart` were holding the `transports` RwLock write guard across a `.await`. Because the guard was created as a temporary inside `if let ...` / compound expressions, Rust extended its lifetime through the shutdown `.await`, blocking every other caller (spawn/get/ shutdown for any other user, any other server) for the duration of the child's shutdown. Refactored both sites to remove the entry inside a scoped block (guard dropped at the end of the block) and perform the async shutdown afterward, with a comment explaining the invariant. 4. `scripts/live_canary/common.py::_start_gateway_stack` used `reserve_loopback_port()` for the mock LLM subprocess, which bound port 0 and closed the socket before the child bound — opening a TOCTOU window where another process could claim the port. `mock_llm.py` already supports `--port 0` + prints `MOCK_LLM_PORT=<N>` on startup (which `wait_for_port_line` already reads), so switched to that race-free pattern. The gateway/http port sites still use `reserve_loopback_port` because ironclaw's gateway reads `GATEWAY_PORT` as a fixed u16 and doesn't support port-0 discovery; documented the residual (low-probability) race and the recommended retry pattern in the helper's docstring. Mock MCP server (`tests/support/mock_mcp_server.rs`) gained a parallel `start_mock_mcp_server_with_specs` + `MockToolSpec` that lets a test override annotations on advertised tools. The existing `start_mock_mcp_server` + 9 existing call sites are untouched.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Review: auth and MCP isolation fixes look ready to merge
I reviewed this in two passes: first as a normal PR review across the auth/MCP/live-canary surfaces, then again as a diff-focused /review pass against origin/staging...HEAD. I also ran the new targeted MCP isolation integration coverage locally.
What looks good:
- The per-user MCP client, session, and stdio-process partitioning is now coherent end-to-end. The runtime no longer shares client/session/process state across users for the same server name, which closes the real multi-user isolation bug this branch set out to fix.
- The global MCP wrapper surface is now defended in the right place. Rejecting same-name cross-user activations when the reported surface diverges, including annotation-only divergence that changes approval policy, is the right fix for the shared
ToolRegistryshape. - The follow-up hardening in the auth path is solid: auth URLs are sanitized before surfacing, token/validation requests are SSRF-checked and redirect-disabled, and error bodies are truncated before being threaded into logs or user-visible errors.
- The canary follow-ups improved the operational story materially. The scheduled seeded lane no longer defaults to the mutating lifecycle probes, and the workflow/docs mismatch around environment-scoped secrets has been corrected toward the actual current repo-scope behavior.
- The regression coverage is meaningful rather than helper-only. In particular, the multi-tenant MCP integration tests exercise the real
ExtensionManagercall path for per-user token binding, session isolation, divergent-surface rejection, annotation-aware approval-policy isolation, and concurrent activate/remove registry consistency.
No verified findings.
Low-priority notes:
- The startup MCP injection path still does not mirror the normal activation path's remove-on-wrapper-construction-failure cleanup, so I would keep an eye on that if wrapper construction ever becomes more fallible. I do not think it is a blocker on the current implementation because the injection path is reusing the already-cached
list_tools()result.
Validation I ran locally:
cargo test --test mcp_multi_tenant_integration --features libsqlcargo test sanitize_tests --lib
Summary
This branch tightens extension and tool auth handling and unifies the auth live-canary system so future provider coverage follows one path.
It includes:
src/auth/scripts/live_canary/framework modules and one canonical account/setup guideWhy
We had auth regressions and open MCP multi-user isolation issues, and the live-canary work had started to split across multiple runner shapes. This branch fixes the highest-risk auth isolation bug, expands end-to-end auth coverage, and gives future auth/provider canaries a single setup and extension path.
Validation
Ran targeted checks during the branch work, including:
cargo testcoverage for auth URL sanitization, redirect handling, MCP session partitioning, factory partitioning, runtime-user MCP wrapper execution, and MCP multi-tenant integrationpytestcoverage for auth/browser scenarios intests/e2e/scenarios/test_extensions.pyandtests/e2e/scenarios/test_v2_auth_oauth_matrix.pypython3 -m py_compilefor the shared live-canary modules and auth runnersbash -n scripts/live-canary/run.sh scripts/live-canary/scrub-artifacts.shscripts/live-canary/run.sh --list-testsand--list-casesImpact
scripts/live_canary/auth_registry.pyandscripts/live-canary/ACCOUNTS.md