mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Restrict MCP HTTP redirects to the configured origin (#39046)
## Why MCP requests can contain sensitive headers and tool-call bodies. Following a cross-origin redirect could disclose them to another server. ## What changed - Follow redirects only when every hop remains on the configured server's origin, and require HTTPS for redirects on non-loopback hostnames. - Preserve standard redirect method and body handling while sharing the original timeout and enforcing a 10-hop limit. - Avoid replaying plaintext proxy credentials across redirects, while leaving OAuth requests that stop at redirects unchanged. ## Testing Add coverage for same-origin and cross-origin redirects, redirect methods, headers and bodies, proxy credentials, timeouts, hop limits, and legacy MCP tool calls. GitOrigin-RevId: 3e267911a1665ad8c2fc63825099a758770f4d68
This commit is contained in:
@@ -56,6 +56,7 @@ use sse_stream::SseStream;
|
||||
use tokio::sync::oneshot;
|
||||
|
||||
use crate::event_notification_transport::MAX_EVENT_NOTIFICATION_BYTES;
|
||||
use crate::http_client_redirect::SameOriginRedirectHttpClient;
|
||||
use crate::incoming_jsonrpc::deserialize_incoming_jsonrpc_message;
|
||||
use crate::incoming_jsonrpc::normalize_sse_jsonrpc_message;
|
||||
use crate::local_stdio_transport::MAX_MCP_STDIO_LINE_BYTES;
|
||||
@@ -124,7 +125,7 @@ impl StreamableHttpClientAdapter {
|
||||
initialize_deadline: Arc<Mutex<Option<Instant>>>,
|
||||
) -> Self {
|
||||
Self {
|
||||
http_client,
|
||||
http_client: Arc::new(SameOriginRedirectHttpClient::new(http_client)),
|
||||
default_headers,
|
||||
auth_provider,
|
||||
event_stream_cancellations: Arc::default(),
|
||||
|
||||
@@ -0,0 +1,221 @@
|
||||
//! Restricts MCP HTTP redirects to the configured server's origin.
|
||||
//!
|
||||
//! MCP requests can carry sensitive headers and tool-call bodies. Following a
|
||||
//! cross-origin redirect would send them to another server or an internal
|
||||
//! service, so this transport validates each redirect before following it.
|
||||
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use codex_exec_server::ExecServerError;
|
||||
use codex_exec_server::HttpClient;
|
||||
use codex_exec_server::HttpHeader;
|
||||
use codex_exec_server::HttpRedirectPolicy;
|
||||
use codex_exec_server::HttpRequestParams;
|
||||
use codex_exec_server::HttpRequestResponse;
|
||||
use codex_exec_server::HttpResponseBodyStream;
|
||||
use futures::FutureExt;
|
||||
use futures::future::BoxFuture;
|
||||
use http::HeaderValue;
|
||||
use http::StatusCode;
|
||||
use url::Url;
|
||||
|
||||
const MAX_REDIRECTS: usize = 10;
|
||||
|
||||
pub(crate) struct SameOriginRedirectHttpClient {
|
||||
inner: Arc<dyn HttpClient>,
|
||||
}
|
||||
|
||||
enum RedirectResponse {
|
||||
Buffered(HttpRequestResponse),
|
||||
Streaming(HttpRequestResponse, HttpResponseBodyStream),
|
||||
}
|
||||
|
||||
impl SameOriginRedirectHttpClient {
|
||||
pub(crate) fn new(inner: Arc<dyn HttpClient>) -> Self {
|
||||
Self { inner }
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
mut params: HttpRequestParams,
|
||||
) -> Result<RedirectResponse, ExecServerError> {
|
||||
// Inspect each Location ourselves before the underlying client can send
|
||||
// sensitive request data to the redirect destination.
|
||||
params.redirect_policy = HttpRedirectPolicy::Stop;
|
||||
let mut current_url = Url::parse(¶ms.url)
|
||||
.map_err(|error| ExecServerError::HttpRequest(error.to_string()))?;
|
||||
let original_origin = current_url.origin();
|
||||
// Redirect hops share the original timeout instead of restarting it.
|
||||
let deadline = params
|
||||
.timeout_ms
|
||||
.map(|timeout_ms| Instant::now() + Duration::from_millis(timeout_ms));
|
||||
let mut redirects = 0;
|
||||
|
||||
loop {
|
||||
if let Some(deadline) = deadline {
|
||||
let Some(remaining) = deadline.checked_duration_since(Instant::now()) else {
|
||||
return Err(ExecServerError::HttpRequest(
|
||||
"MCP HTTP request timed out".to_string(),
|
||||
));
|
||||
};
|
||||
params.timeout_ms = Some(
|
||||
u64::try_from(remaining.as_millis())
|
||||
.unwrap_or(u64::MAX)
|
||||
.max(1),
|
||||
);
|
||||
}
|
||||
|
||||
let result = if params.stream_response {
|
||||
let (response, stream) = self.inner.http_request_stream(params.clone()).await?;
|
||||
RedirectResponse::Streaming(response, stream)
|
||||
} else {
|
||||
RedirectResponse::Buffered(self.inner.http_request(params.clone()).await?)
|
||||
};
|
||||
let response = match &result {
|
||||
RedirectResponse::Buffered(response) | RedirectResponse::Streaming(response, _) => {
|
||||
response
|
||||
}
|
||||
};
|
||||
let status = StatusCode::from_u16(response.status).ok();
|
||||
if !matches!(
|
||||
status,
|
||||
Some(
|
||||
StatusCode::MOVED_PERMANENTLY
|
||||
| StatusCode::FOUND
|
||||
| StatusCode::SEE_OTHER
|
||||
| StatusCode::TEMPORARY_REDIRECT
|
||||
| StatusCode::PERMANENT_REDIRECT
|
||||
)
|
||||
) {
|
||||
return Ok(result);
|
||||
}
|
||||
|
||||
let Some(location) = response
|
||||
.headers
|
||||
.iter()
|
||||
.find(|header| header.name.eq_ignore_ascii_case("location"))
|
||||
.map(|header| header.value.as_str())
|
||||
else {
|
||||
return Ok(result);
|
||||
};
|
||||
let next_url = current_url
|
||||
.join(location)
|
||||
.map_err(|error| ExecServerError::HttpRequest(error.to_string()))?;
|
||||
// Compare every hop with the configured origin so redirect chains
|
||||
// cannot escape it through a later cross-origin Location.
|
||||
if next_url.origin() != original_origin {
|
||||
return Err(ExecServerError::HttpRequest(
|
||||
"MCP HTTP redirect to a different origin is not allowed".to_string(),
|
||||
));
|
||||
}
|
||||
// A hostname can resolve to a different address on the next HTTP
|
||||
// connection. HTTPS authenticates the hostname; localhost and IP
|
||||
// literals do not depend on attacker-controlled DNS.
|
||||
if next_url.scheme() == "http"
|
||||
&& matches!(next_url.host(), Some(url::Host::Domain(host)) if host != "localhost")
|
||||
{
|
||||
return Err(ExecServerError::HttpRequest(
|
||||
"MCP HTTP redirects for non-loopback hostnames require HTTPS".to_string(),
|
||||
));
|
||||
}
|
||||
if redirects >= MAX_REDIRECTS {
|
||||
return Err(ExecServerError::HttpRequest(
|
||||
"MCP HTTP request exceeded the redirect limit".to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
// Preserve normal HTTP behavior: 307/308 retain the method and body;
|
||||
// 303 and legacy 301/302 POST redirects switch to a bodyless GET.
|
||||
let drop_body = matches!(
|
||||
status,
|
||||
Some(StatusCode::MOVED_PERMANENTLY | StatusCode::FOUND)
|
||||
) && params.method.eq_ignore_ascii_case("POST")
|
||||
|| status == Some(StatusCode::SEE_OTHER);
|
||||
if drop_body {
|
||||
if !params.method.eq_ignore_ascii_case("HEAD") {
|
||||
params.method = "GET".to_string();
|
||||
}
|
||||
params.body = None;
|
||||
params.headers.retain(|header| {
|
||||
![
|
||||
"content-type",
|
||||
"content-length",
|
||||
"content-encoding",
|
||||
"transfer-encoding",
|
||||
]
|
||||
.iter()
|
||||
.any(|name| header.name.eq_ignore_ascii_case(name))
|
||||
});
|
||||
}
|
||||
|
||||
// HTTPS keeps application proxy credentials inside the tunnel to
|
||||
// the already-validated origin. Plaintext routes can change across
|
||||
// requests, so their proxy credentials must never be replayed.
|
||||
params.headers.retain(|header| {
|
||||
(current_url.scheme() == "https"
|
||||
|| !header.name.eq_ignore_ascii_case("proxy-authorization"))
|
||||
&& !header.name.eq_ignore_ascii_case("referer")
|
||||
});
|
||||
let mut referer = current_url.clone();
|
||||
let _ = referer.set_username("");
|
||||
let _ = referer.set_password(None);
|
||||
referer.set_fragment(None);
|
||||
if HeaderValue::from_str(referer.as_str()).is_ok() {
|
||||
params.headers.push(HttpHeader {
|
||||
name: "referer".to_string(),
|
||||
value: referer.to_string(),
|
||||
});
|
||||
}
|
||||
|
||||
params.url = next_url.to_string();
|
||||
current_url = next_url;
|
||||
redirects += 1;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl HttpClient for SameOriginRedirectHttpClient {
|
||||
fn http_request(
|
||||
&self,
|
||||
mut params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>> {
|
||||
async move {
|
||||
params.stream_response = false;
|
||||
if params.redirect_policy == HttpRedirectPolicy::Stop {
|
||||
return self.inner.http_request(params).await;
|
||||
}
|
||||
match self.execute(params).await? {
|
||||
RedirectResponse::Buffered(response) => Ok(response),
|
||||
RedirectResponse::Streaming(_, _) => Err(ExecServerError::Protocol(
|
||||
"MCP buffered HTTP request returned a streamed response".to_string(),
|
||||
)),
|
||||
}
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
|
||||
fn http_request_stream(
|
||||
&self,
|
||||
mut params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> {
|
||||
async move {
|
||||
params.stream_response = true;
|
||||
if params.redirect_policy == HttpRedirectPolicy::Stop {
|
||||
return self.inner.http_request_stream(params).await;
|
||||
}
|
||||
match self.execute(params).await? {
|
||||
RedirectResponse::Streaming(response, stream) => Ok((response, stream)),
|
||||
RedirectResponse::Buffered(_) => Err(ExecServerError::Protocol(
|
||||
"MCP streamed HTTP request returned a buffered response".to_string(),
|
||||
)),
|
||||
}
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "http_client_redirect_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,390 @@
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::HttpHeader;
|
||||
use pretty_assertions::assert_eq;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
use crate::http_headers::with_http_headers_helper;
|
||||
|
||||
use super::*;
|
||||
|
||||
const PROXY_HEADERS_HELPER: &str = if cfg!(windows) {
|
||||
r#"echo {"Proxy-Authorization":"Bearer proxy-token"}"#
|
||||
} else {
|
||||
r#"printf '{"Proxy-Authorization":"Bearer proxy-token"}'"#
|
||||
};
|
||||
|
||||
fn request(url: impl Into<String>) -> HttpRequestParams {
|
||||
HttpRequestParams {
|
||||
method: "POST".to_string(),
|
||||
url: url.into(),
|
||||
headers: Vec::new(),
|
||||
body: Some(b"sensitive-body".to_vec().into()),
|
||||
timeout_ms: Some(5_000),
|
||||
redirect_policy: HttpRedirectPolicy::Follow,
|
||||
request_id: "redirect-test".to_string(),
|
||||
stream_response: false,
|
||||
}
|
||||
}
|
||||
|
||||
fn headers<const N: usize>(headers: [(&str, &str); N]) -> Vec<HttpHeader> {
|
||||
headers
|
||||
.into_iter()
|
||||
.map(|(name, value)| HttpHeader {
|
||||
name: name.to_string(),
|
||||
value: value.to_string(),
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct RecordingRedirectHttpClient {
|
||||
requests: Mutex<Vec<HttpRequestParams>>,
|
||||
delay: Duration,
|
||||
loop_redirects: bool,
|
||||
}
|
||||
|
||||
impl HttpClient for RecordingRedirectHttpClient {
|
||||
fn http_request(
|
||||
&self,
|
||||
params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>> {
|
||||
let mut requests = self.requests.lock().expect("request recorder lock");
|
||||
let redirect = requests.is_empty() || self.loop_redirects;
|
||||
let delay = self.delay;
|
||||
requests.push(params);
|
||||
async move {
|
||||
if !delay.is_zero() {
|
||||
tokio::time::sleep(delay).await;
|
||||
}
|
||||
Ok(HttpRequestResponse {
|
||||
status: if redirect { 307 } else { 200 },
|
||||
headers: if redirect {
|
||||
headers([("location", "/final")])
|
||||
} else {
|
||||
Vec::new()
|
||||
},
|
||||
body: Vec::new().into(),
|
||||
})
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
|
||||
fn http_request_stream(
|
||||
&self,
|
||||
_params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> {
|
||||
async {
|
||||
Err(ExecServerError::HttpRequest(
|
||||
"unexpected streaming request".to_string(),
|
||||
))
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plaintext_hostname_redirects_are_rejected_before_dns_can_rebind() -> anyhow::Result<()> {
|
||||
for (url, expected_error) in [
|
||||
(
|
||||
"http://mcp.example/start",
|
||||
Some("non-loopback hostnames require HTTPS"),
|
||||
),
|
||||
("http://localhost/start", None),
|
||||
("http://127.0.0.1/start", None),
|
||||
("http://[::1]/start", None),
|
||||
("https://mcp.example/start", None),
|
||||
] {
|
||||
let recorder = Arc::new(RecordingRedirectHttpClient::default());
|
||||
let client = SameOriginRedirectHttpClient::new(recorder.clone());
|
||||
let response = client
|
||||
.http_request(HttpRequestParams {
|
||||
headers: headers([("authorization", "Bearer sensitive-token")]),
|
||||
..request(url)
|
||||
})
|
||||
.await;
|
||||
|
||||
if let Some(expected_error) = expected_error {
|
||||
let error = response.expect_err("plaintext hostname redirects must fail");
|
||||
assert!(error.to_string().contains(expected_error), "{error}");
|
||||
} else {
|
||||
assert_eq!(response?.status, 200);
|
||||
}
|
||||
let requests = recorder.requests.lock().expect("request recorder lock");
|
||||
assert_eq!(requests.len(), if expected_error.is_some() { 1 } else { 2 });
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn same_origin_redirects_enforce_shared_timeout_and_hop_limit() {
|
||||
for (delay, timeout_ms, expected_error, expected_requests) in [
|
||||
(Duration::from_millis(100), 175, "timed out", 2),
|
||||
(Duration::ZERO, 5_000, "redirect limit", MAX_REDIRECTS + 1),
|
||||
] {
|
||||
let recorder = Arc::new(RecordingRedirectHttpClient {
|
||||
delay,
|
||||
loop_redirects: true,
|
||||
..Default::default()
|
||||
});
|
||||
let client = SameOriginRedirectHttpClient::new(recorder.clone());
|
||||
let error = client
|
||||
.http_request(HttpRequestParams {
|
||||
method: "GET".to_string(),
|
||||
body: None,
|
||||
timeout_ms: Some(timeout_ms),
|
||||
..request("https://mcp.example/loop")
|
||||
})
|
||||
.await
|
||||
.expect_err("redirect loops must respect both limits");
|
||||
|
||||
assert!(error.to_string().contains(expected_error), "{error}");
|
||||
let requests = recorder.requests.lock().expect("request recorder lock");
|
||||
assert_eq!(requests.len(), expected_requests);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn same_origin_redirects_preserve_method_body_and_headers() -> anyhow::Result<()> {
|
||||
for (method, redirect_status, redirected_method) in [
|
||||
("POST", 301, "GET"),
|
||||
("POST", 302, "GET"),
|
||||
("POST", 303, "GET"),
|
||||
("HEAD", 303, "HEAD"),
|
||||
("POST", 307, "POST"),
|
||||
("GET", 307, "GET"),
|
||||
("DELETE", 307, "DELETE"),
|
||||
("POST", 308, "POST"),
|
||||
] {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(path("/start"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(redirect_status).insert_header("location", "/final"),
|
||||
)
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(path("/final"))
|
||||
.respond_with(ResponseTemplate::new(200))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let client =
|
||||
SameOriginRedirectHttpClient::new(Environment::default_for_tests().get_http_client());
|
||||
let expected_content_length = b"sensitive-body".len().to_string();
|
||||
let mut params = HttpRequestParams {
|
||||
method: method.to_string(),
|
||||
headers: headers([
|
||||
("x-api-key", "sensitive-key"),
|
||||
("proxy-authorization", "sensitive-proxy-credentials"),
|
||||
("referer", "https://stale.example/"),
|
||||
]),
|
||||
body: (method == "POST").then(|| b"sensitive-body".to_vec().into()),
|
||||
stream_response: method != "DELETE",
|
||||
..request(format!("{}/start", server.uri()))
|
||||
};
|
||||
if method == "POST" {
|
||||
params.headers.extend(headers([
|
||||
("content-type", "application/json"),
|
||||
("content-encoding", "identity"),
|
||||
("content-length", &expected_content_length),
|
||||
]));
|
||||
}
|
||||
let status = if method == "DELETE" {
|
||||
client.http_request(params).await?.status
|
||||
} else {
|
||||
client.http_request_stream(params).await?.0.status
|
||||
};
|
||||
assert_eq!(status, 200);
|
||||
|
||||
let requests = server.received_requests().await.expect("recorded requests");
|
||||
let redirected = &requests[1];
|
||||
let expected_referer = format!("{}/start", server.uri());
|
||||
let body_preserved = redirected_method == "POST";
|
||||
for (name, expected) in [
|
||||
("x-api-key", Some("sensitive-key")),
|
||||
("proxy-authorization", None),
|
||||
("referer", Some(expected_referer.as_str())),
|
||||
("content-type", body_preserved.then_some("application/json")),
|
||||
("content-encoding", body_preserved.then_some("identity")),
|
||||
(
|
||||
"content-length",
|
||||
body_preserved.then_some(expected_content_length.as_str()),
|
||||
),
|
||||
] {
|
||||
assert_eq!(
|
||||
redirected
|
||||
.headers
|
||||
.get(name)
|
||||
.and_then(|value| value.to_str().ok()),
|
||||
expected,
|
||||
"unexpected redirected {name} header"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
(redirected.method.as_str(), redirected.body.as_slice()),
|
||||
(
|
||||
redirected_method,
|
||||
if body_preserved {
|
||||
b"sensitive-body".as_slice()
|
||||
} else {
|
||||
b"".as_slice()
|
||||
}
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn https_redirects_preserve_configured_and_helper_proxy_authorization() -> anyhow::Result<()>
|
||||
{
|
||||
for helper_enabled in [false, true] {
|
||||
let recorder = Arc::new(RecordingRedirectHttpClient::default());
|
||||
let inner: Arc<dyn HttpClient> = recorder.clone();
|
||||
let directory = tempfile::tempdir()?;
|
||||
let url = "https://mcp.example/start";
|
||||
let inner = if helper_enabled {
|
||||
with_http_headers_helper(
|
||||
inner,
|
||||
url,
|
||||
PROXY_HEADERS_HELPER,
|
||||
directory.path().to_path_buf(),
|
||||
)?
|
||||
} else {
|
||||
inner
|
||||
};
|
||||
let client = SameOriginRedirectHttpClient::new(inner);
|
||||
let response = client
|
||||
.http_request(HttpRequestParams {
|
||||
headers: if helper_enabled {
|
||||
Vec::new()
|
||||
} else {
|
||||
headers([("Proxy-Authorization", "Bearer proxy-token")])
|
||||
},
|
||||
..request(url)
|
||||
})
|
||||
.await?;
|
||||
|
||||
let requests = recorder.requests.lock().expect("request recorder lock");
|
||||
assert_eq!(
|
||||
(
|
||||
response.status,
|
||||
requests.len(),
|
||||
requests[1].url.as_str(),
|
||||
requests[1]
|
||||
.headers
|
||||
.iter()
|
||||
.find(|header| header.name.eq_ignore_ascii_case("proxy-authorization"))
|
||||
.map(|header| header.value.as_str()),
|
||||
),
|
||||
(
|
||||
200,
|
||||
2,
|
||||
"https://mcp.example/final",
|
||||
Some("Bearer proxy-token"),
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plaintext_helper_redirects_block_mcp_but_preserve_oauth_stop() -> anyhow::Result<()> {
|
||||
for (request_id, redirect_policy) in [
|
||||
("mcp-request-1", HttpRedirectPolicy::Follow),
|
||||
("oauth-request-1", HttpRedirectPolicy::Stop),
|
||||
] {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(path("/start"))
|
||||
.respond_with(ResponseTemplate::new(307).insert_header("location", "/final"))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let directory = tempfile::tempdir()?;
|
||||
let url = format!("{}/start", server.uri());
|
||||
let helper = with_http_headers_helper(
|
||||
Environment::default_for_tests().get_http_client(),
|
||||
&url,
|
||||
PROXY_HEADERS_HELPER,
|
||||
directory.path().to_path_buf(),
|
||||
)?;
|
||||
let client = SameOriginRedirectHttpClient::new(helper);
|
||||
let response = client
|
||||
.http_request_stream(HttpRequestParams {
|
||||
request_id: request_id.to_string(),
|
||||
redirect_policy,
|
||||
stream_response: true,
|
||||
..request(url)
|
||||
})
|
||||
.await;
|
||||
|
||||
match (redirect_policy, response) {
|
||||
(HttpRedirectPolicy::Stop, Ok((response, _))) => assert_eq!(response.status, 307),
|
||||
(HttpRedirectPolicy::Follow, Err(error)) => {
|
||||
assert!(error.to_string().contains("Proxy-Authorization"));
|
||||
}
|
||||
_ => panic!("unexpected plaintext proxy-credential redirect behavior"),
|
||||
}
|
||||
assert_eq!(server.received_requests().await.unwrap().len(), 1);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn cross_origin_redirects_never_reach_their_destination() -> anyhow::Result<()> {
|
||||
for status in [307, 308] {
|
||||
for method in ["POST", "GET", "DELETE"] {
|
||||
let destination = MockServer::start().await;
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(path("/start"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(status)
|
||||
.insert_header("location", format!("{}/private", destination.uri())),
|
||||
)
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let client = SameOriginRedirectHttpClient::new(
|
||||
Environment::default_for_tests().get_http_client(),
|
||||
);
|
||||
let params = HttpRequestParams {
|
||||
method: method.to_string(),
|
||||
headers: headers([("x-api-key", "sensitive-key")]),
|
||||
body: (method == "POST").then(|| b"sensitive-body".to_vec().into()),
|
||||
stream_response: method != "DELETE",
|
||||
..request(format!("{}/start", server.uri()))
|
||||
};
|
||||
let error = if method == "DELETE" {
|
||||
client
|
||||
.http_request(params)
|
||||
.await
|
||||
.expect_err("cross-origin DELETE redirect must fail")
|
||||
} else {
|
||||
match client.http_request_stream(params).await {
|
||||
Ok(_) => panic!("cross-origin {method} redirect must fail"),
|
||||
Err(error) => error,
|
||||
}
|
||||
};
|
||||
|
||||
assert!(
|
||||
error.to_string().contains("different origin"),
|
||||
"cross-origin {method} redirect must explain its rejection: {error}"
|
||||
);
|
||||
assert!(destination.received_requests().await.unwrap().is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -197,6 +197,23 @@ impl HttpHeadersClient {
|
||||
}
|
||||
Ok(params)
|
||||
}
|
||||
|
||||
fn reject_proxy_authorization_redirect(
|
||||
response: &HttpRequestResponse,
|
||||
) -> Result<(), ExecServerError> {
|
||||
if (300..400).contains(&response.status)
|
||||
&& response
|
||||
.headers
|
||||
.iter()
|
||||
.any(|header| header.name.eq_ignore_ascii_case("location"))
|
||||
{
|
||||
return Err(ExecServerError::HttpRequest(
|
||||
"MCP HTTP redirect cannot safely replay Proxy-Authorization credentials"
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl HttpClient for HttpHeadersClient {
|
||||
@@ -205,8 +222,21 @@ impl HttpClient for HttpHeadersClient {
|
||||
params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>> {
|
||||
async move {
|
||||
// OAuth validates redirect responses itself; only intercept MCP replay.
|
||||
let mcp_redirect_was_stopped = params.redirect_policy == HttpRedirectPolicy::Stop
|
||||
&& !params.request_id.starts_with("oauth-request-");
|
||||
let params = self.prepare_request(params).await?;
|
||||
self.inner.http_request(params).await
|
||||
let prevent_proxy_authorization_redirect = mcp_redirect_was_stopped
|
||||
&& Url::parse(¶ms.url).is_ok_and(|url| url.scheme() == "http")
|
||||
&& params
|
||||
.headers
|
||||
.iter()
|
||||
.any(|header| header.name.eq_ignore_ascii_case("proxy-authorization"));
|
||||
let response = self.inner.http_request(params).await?;
|
||||
if prevent_proxy_authorization_redirect {
|
||||
Self::reject_proxy_authorization_redirect(&response)?;
|
||||
}
|
||||
Ok(response)
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
@@ -216,8 +246,20 @@ impl HttpClient for HttpHeadersClient {
|
||||
params: HttpRequestParams,
|
||||
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> {
|
||||
async move {
|
||||
let mcp_redirect_was_stopped = params.redirect_policy == HttpRedirectPolicy::Stop
|
||||
&& !params.request_id.starts_with("oauth-request-");
|
||||
let params = self.prepare_request(params).await?;
|
||||
self.inner.http_request_stream(params).await
|
||||
let prevent_proxy_authorization_redirect = mcp_redirect_was_stopped
|
||||
&& Url::parse(¶ms.url).is_ok_and(|url| url.scheme() == "http")
|
||||
&& params
|
||||
.headers
|
||||
.iter()
|
||||
.any(|header| header.name.eq_ignore_ascii_case("proxy-authorization"));
|
||||
let (response, stream) = self.inner.http_request_stream(params).await?;
|
||||
if prevent_proxy_authorization_redirect {
|
||||
Self::reject_proxy_authorization_redirect(&response)?;
|
||||
}
|
||||
Ok((response, stream))
|
||||
}
|
||||
.boxed()
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ mod elicitation_client_service;
|
||||
mod event_notification_transport;
|
||||
mod executor_process_transport;
|
||||
mod http_client_adapter;
|
||||
mod http_client_redirect;
|
||||
mod http_headers;
|
||||
mod in_process_transport;
|
||||
mod incoming_jsonrpc;
|
||||
|
||||
@@ -230,8 +230,9 @@ async fn modern_discovery_does_not_follow_redirects_with_sensitive_headers() ->
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn legacy_mcp_requests_follow_redirects_with_configured_headers() -> anyhow::Result<()> {
|
||||
let redirect_target = MockServer::start().await;
|
||||
async fn legacy_mcp_requests_follow_same_origin_redirects_with_configured_headers()
|
||||
-> anyhow::Result<()> {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/forwarded"))
|
||||
.and(header("x-api-key", "sensitive-key"))
|
||||
@@ -244,23 +245,19 @@ async fn legacy_mcp_requests_follow_redirects_with_configured_headers() -> anyho
|
||||
}
|
||||
})
|
||||
.expect(2)
|
||||
.mount(&redirect_target)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let resource_server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/mcp"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(307)
|
||||
.insert_header("location", format!("{}/forwarded", redirect_target.uri())),
|
||||
)
|
||||
.respond_with(ResponseTemplate::new(307).insert_header("location", "/forwarded"))
|
||||
.expect(2)
|
||||
.mount(&resource_server)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let client = RmcpClient::new_streamable_http_client_with_protocol_mode(
|
||||
"legacy-redirect-test",
|
||||
&format!("{}/mcp", resource_server.uri()),
|
||||
&format!("{}/mcp", server.uri()),
|
||||
/*bearer_token*/ None,
|
||||
Some(HashMap::from([(
|
||||
"x-api-key".to_string(),
|
||||
@@ -275,12 +272,66 @@ async fn legacy_mcp_requests_follow_redirects_with_configured_headers() -> anyho
|
||||
)
|
||||
.await?;
|
||||
initialize_client(&client).await?;
|
||||
redirect_target.verify().await;
|
||||
resource_server.verify().await;
|
||||
server.verify().await;
|
||||
client.shutdown().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn legacy_fallback_does_not_follow_cross_origin_tool_redirects() -> anyhow::Result<()> {
|
||||
for mode in [McpProtocolMode::Legacy, McpProtocolMode::V20260728] {
|
||||
let destination = MockServer::start().await;
|
||||
let server = MockServer::start().await;
|
||||
let destination_url = format!("{}/private", destination.uri());
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/redirected"))
|
||||
.respond_with(ResponseTemplate::new(307).insert_header("location", destination_url))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/mcp"))
|
||||
.respond_with(move |request: &Request| {
|
||||
let body: Value = request.body_json().expect("valid JSON-RPC request");
|
||||
match body["method"].as_str() {
|
||||
Some("server/discover") => ResponseTemplate::new(200).set_body_json(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": body["id"],
|
||||
"error": {"code": -32601, "message": "method not found"},
|
||||
})),
|
||||
Some("initialize") => legacy_initialize_response(&body),
|
||||
Some("notifications/initialized") => ResponseTemplate::new(202),
|
||||
Some("tools/call") => {
|
||||
ResponseTemplate::new(307).insert_header("location", "/redirected")
|
||||
}
|
||||
other => panic!("unexpected legacy MCP method: {other:?}"),
|
||||
}
|
||||
})
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let client = create_client(&server, mode).await?;
|
||||
initialize_client(&client).await?;
|
||||
let error = client
|
||||
.call_tool(
|
||||
"query_logs_sql".to_string(),
|
||||
Some(json!({"query": "sensitive-query"})),
|
||||
/*meta*/ None,
|
||||
Some(Duration::from_secs(5)),
|
||||
)
|
||||
.await
|
||||
.expect_err("cross-origin tools/call redirect must fail");
|
||||
assert!(
|
||||
format!("{error:#}").contains("different origin"),
|
||||
"cross-origin tools/call redirect must explain its rejection: {error:#}"
|
||||
);
|
||||
assert!(destination.received_requests().await.unwrap().is_empty());
|
||||
client.shutdown().await;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn modern_discovery_accepts_metadata_namespaced_server_identity() -> anyhow::Result<()> {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
Reference in New Issue
Block a user