Fix RBAC self-assertion bypass + dashboard/cache/config hardening; configurable stdio handshake timeout (#20)#21
Merged
Conversation
…tdio handshake timeout configurable (#20) Security fixes: - RBAC self-assertion bypass (HIGH): dispatch_real_tool no longer falls back to a client-supplied _mcplex_role param; role now comes exclusively from the server-verified trusted_role set by the auth middleware. - Dashboard /api/* routes now support optional gateway.dashboard_api_key auth. - Tool response cache is now partitioned by caller role to prevent cross-tenant leakage in multi-key deployments. - validate_config rejects api_key/dashboard_api_key/api_keys that resolve to an empty string (e.g. an unset ${ENV_VAR}), which previously turned "auth required" into a silent no-op. - Expanded audit log redaction list (pwd, auth, cookie, session, bearer). - Startup warning when RBAC is enabled with no api_key/api_keys configured. Fixes #20: the stdio MCP handshake (initialize) and steady-state tool calls now use independent, configurable timeouts (security.default_handshake_timeout_secs / servers[].handshake_timeout_secs vs. security.request_timeout_secs), so slow-cold-start servers no longer need to loosen the steady-state timeout. Also implements "0 = no timeout" for both, previously documented but unused. Bumps version to 0.7.1, updates README/CHANGELOG, and adds a cargo audit step to CI. 38 tests passing (up from 32). Co-Authored-By: Claude Sonnet 5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Security hardening and configuration improvements across the gateway, including closing an RBAC bypass, adding optional dashboard authentication, partitioning cache entries by caller role, rejecting empty-string secrets after ${ENV_VAR} expansion, and making the stdio MCP handshake timeout independently configurable (fixes #20).
Changes:
- Remove client-controlled role fallback (
_mcplex_role) and rely exclusively on server-verifiedtrusted_rolefor RBAC decisions. - Add
security.default_handshake_timeout_secs+servers[].handshake_timeout_secs(separate fromsecurity.request_timeout_secs), and implement0 = no timeoutfor stdio timeouts. - Add optional
gateway.dashboard_api_key, expand audit redaction keys, and addcargo auditto CI.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/security/mod.rs | Adds regression tests ensuring RBAC denies without a server-verified role. |
| src/security/audit.rs | Expands sensitive-key redaction keywords for audit logging. |
| src/security/allowlist.rs | Updates server config test fixtures for the new handshake-timeout field. |
| src/protocol/transport.rs | Removes client role fallback; partitions cache by role; exposes auth helpers for dashboard auth. |
| src/protocol/stdio.rs | Splits handshake vs steady-state request timeouts; implements 0 = no timeout. |
| src/protocol/multiplexer.rs | Wires per-server/global handshake timeout into stdio discovery/connection. |
| src/protocol/cache.rs | Partitions cache keys by caller role to prevent cross-tenant leakage. |
| src/observe/dashboard.rs | Adds optional dashboard API-key auth middleware + startup warning when unset. |
| src/main.rs | Wires handshake timeout into stdio respawn path. |
| src/config.rs | Adds new config fields + validation (reject empty secrets) + tests for parsing/defaults. |
| README.md | Documents new security boundaries, dashboard auth, role-partitioned cache, and handshake timeout config. |
| CHANGELOG.md | Adds 0.7.1 release notes covering the security and timeout changes. |
| Cargo.toml | Bumps version to 0.7.1. |
| .github/workflows/ci.yml | Adds a cargo audit supply-chain scanning job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+32
| if dashboard_api_key.is_none() { | ||
| warn!( | ||
| "📊 Dashboard has no api_key configured — it is unauthenticated. \ | ||
| Bind it to localhost only, or set gateway.dashboard_api_key." | ||
| ); | ||
| } |
Comment on lines
+61
to
+68
| async fn dashboard_auth_middleware( | ||
| State(expected_key): State<Option<String>>, | ||
| request: axum::extract::Request, | ||
| next: axum::middleware::Next, | ||
| ) -> axum::response::Response { | ||
| let Some(expected_key) = expected_key else { | ||
| return next.run(request).await; | ||
| }; |
Comment on lines
+596
to
+613
| let request_timeout = std::time::Duration::from_secs( | ||
| state_for_respawn | ||
| .config | ||
| .read() | ||
| .await | ||
| .security | ||
| .request_timeout_secs, | ||
| ); | ||
| let handshake_timeout = std::time::Duration::from_secs( | ||
| config.handshake_timeout_secs.unwrap_or( | ||
| state_for_respawn | ||
| .config | ||
| .read() | ||
| .await | ||
| .security | ||
| .default_handshake_timeout_secs, | ||
| ), | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security hardening pass on the gateway, plus a proper fix for #20 (configurable stdio handshake timeout). Verified by direct code read-through, not just an automated scan.
🔴 High
dispatch_real_toolfell back to a client-supplied_mcplex_rolerequest param whenever no server-verified role was established (i.e.enable_rbac=truewith noapi_key/api_keysconfigured — see e.g.examples/dev-team.toml). Any caller could set_mcplex_role: "admin"in the tool-call params and bypass RBAC entirely.SecurityEngine::is_tool_allowedalready denied-by-default correctly; the client-controlled fallback was the only hole. Fixed by using the server-verifiedtrusted_roleexclusively (set by the auth middleware only after a verified API-key match).🟠 Medium
/api/metrics,/api/tools,/api/servers,/api/events,/api/configwere guarded only by permissive CORS. Added optionalgateway.dashboard_api_key; when set, all dashboard routes require a matchingAuthorization: Bearer/X-API-Keyheader (constant-time compared). Unset by default (no breaking change), with a startup warning recommending a localhost-only bind in that case.${ENV_VAR}expansion silently produces""for an unset variable, which could turngateway.api_key/dashboard_api_keyinto an empty-string "secret" that any empty bearer token would match.validate_confignow rejects this at startup.🟡 Low
pwd,auth,cookie,session,bearer.enable_rbac=truebut noapi_key/api_keysconfigured (every call now correctly denies by default — this makes that visible instead of looking like a bug).✨ Fixes #20 — configurable stdio handshake timeout
The initial stdio MCP handshake (
initialize) previously shared a hardcoded 30s timeout with steady-state tool calls, so a slow cold start (e.g.npxresolving/downloading a package on first run) could only be worked around by loosening the timeout for every later tool call too. Now:security.default_handshake_timeout_secs(global default, 30s) andservers[].handshake_timeout_secs(per-server override) control the handshake only.security.request_timeout_secsapplies only to steady-state requests once connected.0now genuinely means "no timeout" for both (previously documented but never implemented anywhere in the code).Zero-risk polish
cargo auditjob to CI for supply-chain scanning.Explicitly out of scope (follow-up ideas, not code)
TLS termination, request/response transform plugin hooks, dynamic server topology reload — real ideas surfaced during review, but architectural changes that deserve their own scoped PR rather than being bundled into a security-fix PR.
Test plan
cargo build— cleancargo test— 38 passed (up from 32; new regression tests for the RBAC fix, role-partitioned cache, and handshake-timeout config parsing/defaults)cargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleantrusted_roleend-to-end (header injection in the auth middleware → dispatch) to confirm the fix is complete, not just localizedBumps version to
0.7.1; README and CHANGELOG updated accordingly.🤖 Generated with Claude Code