From 3fa8c78aa040ddbb507de19c495647193c5b40c6 Mon Sep 17 00:00:00 2001 From: Cody De Arkland Date: Fri, 21 Aug 2026 13:43:24 -0700 Subject: [PATCH] fix(mcp): only complete the project, and only when nothing was scoped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live testing of the injection that #1115 carried onto master found it corrupts explicitly cross-project calls, which is one of the things the remote MCP is for. Reproduced by hand: ask get-logs for another project's service and pass that project's id, but omit the environment. The proxy filled the environment from the directory link, the server checked the linked environment against the caller's project, and denied the request with "You don't have the required role (viewer) on this resource." Supplying the environment explicitly made the same call succeed. Injection, not permissions. A second, quieter shape: a serviceId from elsewhere with no projectId got the linked project stapled on, so the failure came back as "No deployment found for this service and environment" — which reads to a model as "that service does not exist" rather than "you did not say which project". Two changes: - Only projectId is completed. environmentId and serviceId are subordinate to a project; the server defaults the environment where that is safe and requires both explicitly on the destructive tools, where guessing is worse than asking. LinkContext collapses to the one field, so there is no longer a way to pair one project's id with another's resources. - Nothing is completed when the call already names a projectId, environmentId, serviceId or deploymentId. A caller that named a resource has its own intent; it should get a plain "projectId Required" and correct itself, not a project we guessed. This costs nothing measurable. Of ~80,000 successful local MCP calls on 2026-08-19, the project-omitted/service-supplied combination occurred exactly zero times — callers supply full context or none — and the ~39,800 that supply none still get served, which is the population the injection exists for. Requests the proxy completed now carry x-railway-mcp-injected. Injection is otherwise indistinguishable server-side from a caller's own argument, so nothing could measure how often it fires or recognise it in a report of "the agent looked at the wrong project". Co-Authored-By: Claude Opus 5 (1M context) --- src/commands/mcp/proxy.rs | 441 ++++++++++++++++++-------------------- 1 file changed, 213 insertions(+), 228 deletions(-) diff --git a/src/commands/mcp/proxy.rs b/src/commands/mcp/proxy.rs index efc6f81d3..dbdfbe6f9 100644 --- a/src/commands/mcp/proxy.rs +++ b/src/commands/mcp/proxy.rs @@ -73,53 +73,59 @@ struct SessionMeta { /// accepts, so this has to come from the server rather than a list baked /// into the CLI that would drift as tools change. tool_params: HashMap>, + /// Request ids whose arguments the proxy completed, so the outgoing request + /// can declare it. Cleared as each is sent. + injected_ids: HashSet, } -/// The project/environment/service this invocation targets. +/// The project this invocation targets, used only to complete a tool call that +/// named no scope of its own. /// -/// The local MCP server resolves these from `railway link`; the remote server -/// is a different machine and never can. Roughly 44% of successful local tool -/// calls pass no projectId and rely on exactly this, so without it the remote -/// path is not a drop-in replacement. +/// Deliberately just the project. `environmentId` and `serviceId` are +/// subordinate to a project, and filling them from the directory link is what +/// broke explicit cross-project calls: a linked environment paired with a +/// caller-supplied project from somewhere else fails the server's auth gate +/// with "you don't have the required role". The server resolves the +/// environment itself where that is safe, and requires both explicitly on the +/// destructive tools, where guessing is worse than asking. /// -/// Covers the two sources `get_linked_project` resolves without I/O — the -/// RAILWAY_PROJECT_ID/ENVIRONMENT_ID/SERVICE_ID env vars and the directory -/// link. Deliberately NOT covered: resolving a project from a RAILWAY_TOKEN, -/// which costs a GraphQL round trip. Doing that here would put a network call -/// (and a 15s connect timeout on a bad one) in front of proxy startup, which -/// the harness is waiting on. Project-token users without a directory link or -/// env vars get no injection and must pass ids explicitly. +/// Resolved from RAILWAY_PROJECT_ID or the `railway link` directory, whichever +/// is present — the two sources `get_linked_project` reads without I/O. +/// Deliberately NOT covered: resolving a project from a RAILWAY_TOKEN, which +/// costs a GraphQL round trip and would put a network call (and a 15s connect +/// timeout on a bad one) in front of proxy startup while the harness waits. #[derive(Clone, Default)] struct LinkContext { project_id: Option, - environment_id: Option, - service_id: Option, } -impl LinkContext { - fn value_for(&self, param: &str) -> Option<&str> { - match param { - "projectId" => self.project_id.as_deref(), - "environmentId" => self.environment_id.as_deref(), - "serviceId" => self.service_id.as_deref(), - _ => None, - } - } - - fn is_empty(&self) -> bool { - self.project_id.is_none() && self.environment_id.is_none() && self.service_id.is_none() - } -} - -/// Context parameters the proxy will fill in. Ordered widest-first purely for -/// readable logs; injection is per-parameter and independent. -const INJECTABLE_PARAMS: [&str; 3] = ["projectId", "environmentId", "serviceId"]; +/// Parameters that scope a tool call to a resource. Any one of these means the +/// caller expressed intent about *what* it is acting on, so the proxy leaves +/// the call alone entirely. +/// +/// This is what keeps cross-project work intact. A caller holding a serviceId +/// from another project and no projectId gets the plain "projectId Required" +/// back and can correct itself. Injecting a project there would answer a +/// question nobody asked — the server would look for that service in the +/// linked project, fail, and report "not found in this project", which reads +/// to a model as "this service does not exist". +/// +/// Measured 2026-08-19: of ~80,000 successful local MCP calls, the +/// project-omitted/service-supplied combination occurred exactly zero times. +/// Callers supply full context or none, so gating on this costs nothing. +const SCOPING_PARAMS: [&str; 4] = ["projectId", "environmentId", "serviceId", "deploymentId"]; /// Marks traffic as coming through `railway mcp proxy` so remote MCP telemetry /// can separate it from editor OAuth and other direct clients. const MCP_TRANSPORT_HEADER: &str = "x-railway-mcp-transport"; const MCP_TRANSPORT_VALUE: &str = "cli-proxy"; const MCP_CLIENT_HEADER: &str = "x-railway-mcp-client"; +/// Names the context the proxy filled in on this call. Injection is otherwise +/// invisible — the server cannot tell an injected projectId from one the +/// caller chose — so without this there is no way to measure how often it +/// fires, or to recognise it in a report of "the agent looked at the wrong +/// project". +const MCP_INJECTED_HEADER: &str = "x-railway-mcp-injected"; type Out = mpsc::UnboundedSender; @@ -188,8 +194,12 @@ pub async fn serve_proxy() -> Result<()> { session.init_request = Some(msg.clone()); session.client_name = extract_mcp_client_header(&msg); } else if method_of(&msg) == Some("tools/call") { - let session = state.session.lock().await; - inject_link_context(&state.link, &session.tool_params, &mut msg); + let mut session = state.session.lock().await; + if inject_link_context(&state.link, &session.tool_params, &mut msg) { + for id in ids_of(&msg) { + session.injected_ids.insert(id.to_string()); + } + } } if handshake_done { @@ -224,43 +234,19 @@ fn method_of(msg: &JsonValue) -> Option<&str> { /// surfaces resolve the same project. Absent link (or an unreadable config) is /// normal — injection simply does nothing. fn read_link_context(configs: &Configs) -> LinkContext { - let linked = configs.get_local_linked_project().ok(); - - // Env-var targeting wins over the directory link, matching - // `get_linked_project`. Mixing the two would silently pair project A with - // project B's environment, so an explicit RAILWAY_PROJECT_ID discards the - // directory link unless both name the same project. - let env_project = Configs::get_railway_project_id().filter(|s| !s.is_empty()); - let linked_for_env = linked - .as_ref() - .filter(|p| env_project.as_ref().is_none_or(|id| &p.project == id)); - - let project_id = env_project - .clone() - .or_else(|| linked.as_ref().map(|p| p.project.clone())) - .filter(|s| !s.is_empty()); - - // Env-var environment/service ids count only alongside RAILWAY_PROJECT_ID, - // matching `Configs::resolve_env_var_project`, which refuses - // RAILWAY_ENVIRONMENT_ID without RAILWAY_PROJECT_ID outright. Without this - // guard, a stray RAILWAY_ENVIRONMENT_ID from another project's tooling - // would silently pair project A (the link) with project B's environment. - // Silently ignoring the orphaned var beats silently mispairing it. - let environment_id = Configs::get_railway_environment_id() - .filter(|_| env_project.is_some()) - .or_else(|| linked_for_env.and_then(|p| p.environment.clone())) - .filter(|s| !s.is_empty()); - - let service_id = Configs::get_railway_service_id() - .filter(|_| env_project.is_some()) - .or_else(|| linked_for_env.and_then(|p| p.service.clone())) + // RAILWAY_PROJECT_ID wins over the directory link, matching + // `get_linked_project`. With only the project in play there is no longer a + // way to pair one project's id with another's environment. + let project_id = Configs::get_railway_project_id() + .or_else(|| { + configs + .get_local_linked_project() + .ok() + .map(|linked| linked.project) + }) .filter(|s| !s.is_empty()); - LinkContext { - project_id, - environment_id, - service_id, - } + LinkContext { project_id } } /// Learn each tool's declared parameters from a `tools/list` result. @@ -295,51 +281,56 @@ fn inject_link_context( link: &LinkContext, tool_params: &HashMap>, msg: &mut JsonValue, -) { - if link.is_empty() || method_of(msg) != Some("tools/call") { - return; +) -> bool { + let Some(project_id) = link.project_id.as_deref() else { + return false; + }; + if method_of(msg) != Some("tools/call") { + return false; } let Some(tool_name) = msg .pointer("/params/name") .and_then(JsonValue::as_str) .map(str::to_owned) else { - return; + return false; }; + // No schema yet (tools/list not seen): forward untouched rather than send + // a parameter the tool may not accept. let Some(declared) = tool_params.get(&tool_name) else { - return; + return false; }; + if !declared.contains("projectId") { + return false; + } - let missing: Vec<(&str, String)> = INJECTABLE_PARAMS - .iter() - .filter(|param| declared.contains(**param)) - .filter_map(|param| { - let already_set = msg - .pointer(&format!("/params/arguments/{param}")) - .is_some_and(|v| !v.is_null()); - if already_set { - return None; - } - link.value_for(param).map(|v| (*param, v.to_string())) - }) - .collect(); - - if missing.is_empty() { - return; + // The caller named a resource, so it has its own intent about scope. + let supplied = |param: &str| { + msg.pointer(&format!("/params/arguments/{param}")) + .is_some_and(|v| !v.is_null()) + }; + if SCOPING_PARAMS.iter().any(|param| supplied(param)) { + return false; } + // Only projectId. environmentId and serviceId are subordinate to a project + // and the server resolves or requires them itself: it defaults the + // environment where that is safe, and demands both explicitly on the + // destructive tools, where guessing is worse than asking. let Some(params) = msg.get_mut("params").and_then(JsonValue::as_object_mut) else { - return; + return false; }; let arguments = params .entry("arguments") .or_insert_with(|| JsonValue::Object(serde_json::Map::new())); let Some(arguments) = arguments.as_object_mut() else { - return; + return false; }; - for (param, value) in missing { - arguments.insert(param.to_string(), JsonValue::String(value)); - } + arguments.insert( + "projectId".to_string(), + JsonValue::String(project_id.to_string()), + ); + true } /// Request ids awaiting a response in this message — one for a plain request, @@ -510,7 +501,13 @@ async fn post_message( token: &str, session_id: Option<&str>, ) -> Result { - let client_name = state.session.lock().await.client_name.clone(); + let (client_name, injected) = { + let mut session = state.session.lock().await; + let injected = ids_of(msg) + .iter() + .any(|id| session.injected_ids.remove(&id.to_string())); + (session.client_name.clone(), injected) + }; let mut req = state .http .post(&state.url) @@ -521,6 +518,9 @@ async fn post_message( if let Some(client) = client_name.as_deref() { req = req.header(MCP_CLIENT_HEADER, client); } + if injected { + req = req.header(MCP_INJECTED_HEADER, "projectId"); + } if let Some(sid) = session_id { req = req.header("mcp-session-id", sid); } @@ -975,12 +975,9 @@ mod link_context_tests { fn link() -> LinkContext { LinkContext { project_id: Some("proj-1".into()), - environment_id: Some("env-1".into()), - service_id: Some("svc-1".into()), } } - /// What the server reports for a project-scoped tool. fn params_for(tool: &str, declared: &[&str]) -> HashMap> { let mut m = HashMap::new(); m.insert( @@ -999,48 +996,92 @@ mod link_context_tests { }) } + const CTX: &[&str] = &["projectId", "environmentId", "serviceId"]; + #[test] - fn fills_the_context_a_tool_declares_but_the_caller_omitted() { - // The gap this closes: ~44% of successful local MCP calls pass no - // projectId and rely on `railway link`, which the remote server cannot - // see. - let params = params_for("list-services", &["projectId", "environmentId"]); + fn fills_the_project_when_the_caller_gave_no_scope_at_all() { + // The case worth serving: ~39,800 successful local calls a day supply + // no context and lean on `railway link`. let mut msg = call("list-services", json!({})); - - inject_link_context(&link(), ¶ms, &mut msg); - + assert!(inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert_eq!( msg.pointer("/params/arguments/projectId").unwrap(), "proj-1" ); - assert_eq!( - msg.pointer("/params/arguments/environmentId").unwrap(), - "env-1" - ); - // Not declared by this tool, so not invented. + } + + #[test] + fn never_fills_a_subordinate_id() { + // environmentId and serviceId belong to a project. Filling them from + // the link is what corrupted an explicitly cross-project call: the + // server saw the linked environment against another project and denied + // the request. + let mut msg = call("list-services", json!({})); + inject_link_context(&link(), ¶ms_for("list-services", CTX), &mut msg); + assert!(msg.pointer("/params/arguments/environmentId").is_none()); assert!(msg.pointer("/params/arguments/serviceId").is_none()); } #[test] - fn never_overwrites_what_the_caller_supplied() { - let params = params_for("list-services", &["projectId"]); - let mut msg = call("list-services", json!({ "projectId": "explicit" })); + fn leaves_an_explicit_project_alone() { + let mut msg = call("list-services", json!({ "projectId": "other" })); + assert!(!inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); + assert_eq!(msg.pointer("/params/arguments/projectId").unwrap(), "other"); + } - inject_link_context(&link(), ¶ms, &mut msg); + #[test] + fn stays_out_of_a_call_that_named_any_resource() { + // Cross-project work lives here. A caller holding a serviceId from + // another project must get "projectId Required", not a project we + // guessed — otherwise the failure reads as "that service is gone". + for scoped in [ + json!({ "serviceId": "svc-from-another-project" }), + json!({ "environmentId": "env-from-another-project" }), + json!({ "deploymentId": "dep-from-another-project" }), + ] { + let mut msg = call("get-logs", scoped.clone()); + let declared = params_for( + "get-logs", + &["projectId", "environmentId", "serviceId", "deploymentId"], + ); + assert!( + !inject_link_context(&link(), &declared, &mut msg), + "should not inject over {scoped}" + ); + assert!(msg.pointer("/params/arguments/projectId").is_none()); + } + } + #[test] + fn treats_an_explicit_null_scope_as_absent() { + let mut msg = call("list-services", json!({ "projectId": null })); + assert!(inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert_eq!( msg.pointer("/params/arguments/projectId").unwrap(), - "explicit" + "proj-1" ); } #[test] - fn leaves_tools_that_declare_no_context_alone() { - let params = params_for("search-docs", &["query"]); + fn leaves_tools_that_do_not_take_a_project_alone() { let mut msg = call("search-docs", json!({ "query": "volumes" })); - - inject_link_context(&link(), ¶ms, &mut msg); - + assert!(!inject_link_context( + &link(), + ¶ms_for("search-docs", &["query"]), + &mut msg + )); assert_eq!( msg.pointer("/params/arguments").unwrap(), &json!({ "query": "volumes" }) @@ -1049,37 +1090,33 @@ mod link_context_tests { #[test] fn does_nothing_before_tools_list_has_been_seen() { - // An unknown tool means no schema yet; guessing could send a parameter - // the tool does not accept. let mut msg = call("list-services", json!({})); - - inject_link_context(&link(), &HashMap::new(), &mut msg); - + assert!(!inject_link_context(&link(), &HashMap::new(), &mut msg)); assert_eq!(msg.pointer("/params/arguments").unwrap(), &json!({})); } #[test] - fn does_nothing_without_a_directory_link() { - let params = params_for("list-services", &["projectId"]); + fn does_nothing_without_a_linked_project() { let mut msg = call("list-services", json!({})); - - inject_link_context(&LinkContext::default(), ¶ms, &mut msg); - + assert!(!inject_link_context( + &LinkContext::default(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert_eq!(msg.pointer("/params/arguments").unwrap(), &json!({})); } #[test] fn creates_the_arguments_object_when_the_caller_sent_none() { - let params = params_for("list-services", &["projectId"]); let mut msg = json!({ - "jsonrpc": "2.0", - "id": 1, - "method": "tools/call", + "jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { "name": "list-services" }, }); - - inject_link_context(&link(), ¶ms, &mut msg); - + assert!(inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert_eq!( msg.pointer("/params/arguments/projectId").unwrap(), "proj-1" @@ -1088,126 +1125,74 @@ mod link_context_tests { #[test] fn ignores_messages_that_are_not_tool_calls() { - let params = params_for("list-services", &["projectId"]); let mut msg = json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list" }); - - inject_link_context(&link(), ¶ms, &mut msg); - + assert!(!inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert!(msg.pointer("/params").is_none()); } - #[test] - fn learns_declared_parameters_from_a_tools_list_result() { - let mut session = SessionMeta::default(); - record_tool_params( - &mut session, - &json!({ - "jsonrpc": "2.0", - "id": 1, - "result": { "tools": [ - { "name": "list-services", "inputSchema": { "properties": { - "projectId": {}, "environmentId": {} - }}}, - { "name": "whoami", "inputSchema": { "properties": {} } } - ]} - }), - ); - - assert!(session.tool_params["list-services"].contains("projectId")); - assert!(session.tool_params["whoami"].is_empty()); - } - - /// Serialized: these mutate process env, which is global. - #[test] - fn env_var_targeting_overrides_and_does_not_mix_with_a_stale_link() { - use std::sync::Mutex as StdMutex; - static ENV_LOCK: StdMutex<()> = StdMutex::new(()); - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - - // RAILWAY_PROJECT_ID naming a different project than the directory - // link must not inherit that link's environment — pairing project A - // with project B's environment is exactly the silent-wrong-target bug - // injection is supposed to avoid. - let linked = LinkContext { - project_id: Some("proj-from-dir".into()), - environment_id: Some("env-from-dir".into()), - service_id: Some("svc-from-dir".into()), - }; - assert_eq!(linked.value_for("projectId"), Some("proj-from-dir")); - assert_eq!(linked.value_for("unknownParam"), None); - } - #[test] fn does_not_inject_into_a_jsonrpc_batch() { - // Known, deliberate gap: method_of() sees no method on a top-level - // array, so a batched tools/call is forwarded untouched. Fail-closed - // is the right side to err on, and batches are vanishingly rare. - let params = params_for("list-services", &["projectId"]); let mut msg = json!([ { "jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { "name": "list-services", "arguments": {} } } ]); - - inject_link_context(&link(), ¶ms, &mut msg); - + assert!(!inject_link_context( + &link(), + ¶ms_for("list-services", CTX), + &mut msg + )); assert_eq!(msg.pointer("/0/params/arguments").unwrap(), &json!({})); } #[test] fn survives_malformed_params_and_arguments() { - let params = params_for("list-services", &["projectId"]); - - // params is not an object + let declared = params_for("list-services", CTX); let mut a = json!({ "method": "tools/call", "params": "nope" }); - inject_link_context(&link(), ¶ms, &mut a); - assert_eq!(a.pointer("/params").unwrap(), "nope"); + assert!(!inject_link_context(&link(), &declared, &mut a)); - // arguments is an array rather than an object let mut b = json!({ "method": "tools/call", "params": { "name": "list-services", "arguments": [1, 2] } }); - inject_link_context(&link(), ¶ms, &mut b); + assert!(!inject_link_context(&link(), &declared, &mut b)); assert_eq!(b.pointer("/params/arguments").unwrap(), &json!([1, 2])); - // no tool name at all let mut c = json!({ "method": "tools/call", "params": { "arguments": {} } }); - inject_link_context(&link(), ¶ms, &mut c); - assert_eq!(c.pointer("/params/arguments").unwrap(), &json!({})); + assert!(!inject_link_context(&link(), &declared, &mut c)); } #[test] - fn treats_an_explicit_null_as_absent() { - let params = params_for("list-services", &["projectId"]); - let mut msg = call("list-services", json!({ "projectId": null })); - - inject_link_context(&link(), ¶ms, &mut msg); - - assert_eq!( - msg.pointer("/params/arguments/projectId").unwrap(), - "proj-1" - ); - } - - #[test] - fn injects_only_what_the_link_actually_has() { - // A directory can be linked to a project without an environment. - let partial = LinkContext { + fn resolves_only_a_project_so_there_is_nothing_to_mix() { + // The earlier shape carried environment and service too, which is how + // a linked environment ended up attached to another project's id. + let ctx = LinkContext { project_id: Some("proj-1".into()), - environment_id: None, - service_id: None, }; - let params = params_for("list-services", &["projectId", "environmentId"]); - let mut msg = call("list-services", json!({})); - - inject_link_context(&partial, ¶ms, &mut msg); + assert_eq!(ctx.project_id.as_deref(), Some("proj-1")); + assert_eq!(LinkContext::default().project_id, None); + } - assert_eq!( - msg.pointer("/params/arguments/projectId").unwrap(), - "proj-1" + #[test] + fn learns_declared_parameters_from_a_tools_list_result() { + let mut session = SessionMeta::default(); + record_tool_params( + &mut session, + &json!({ + "jsonrpc": "2.0", "id": 1, + "result": { "tools": [ + { "name": "list-services", "inputSchema": { "properties": { + "projectId": {}, "environmentId": {} + }}}, + { "name": "whoami", "inputSchema": { "properties": {} } } + ]} + }), ); - // Left for the server to default rather than invented here. - assert!(msg.pointer("/params/arguments/environmentId").is_none()); + assert!(session.tool_params["list-services"].contains("projectId")); + assert!(session.tool_params["whoami"].is_empty()); } #[test]