Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions crates/openshell-sandbox/src/child_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ pub(crate) fn tls_env_vars(
]
}

/// Serializes tests that read or write `OPENSHELL_DIRECT_TCP_*`.
///
/// `std::env::set_var` / `remove_var` are unsafe because a concurrent read or
/// write of the environment is undefined behavior. Tests run in parallel by
/// default, and the tests in this module and in `sandbox::linux::netns` touch
/// the same variables, so every such test holds this lock for its whole body.
#[cfg(test)]
pub(crate) fn lock_direct_tcp_env() -> std::sync::MutexGuard<'static, ()> {
static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner)
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -79,8 +91,10 @@ mod tests {

#[test]
fn apply_proxy_env_includes_node_proxy_opt_in_and_local_bypass() {
let _env = lock_direct_tcp_env();
// Ensure no leftover env from other tests affects NO_PROXY
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };

let mut cmd = Command::new("/usr/bin/env");
cmd.stdin(Stdio::null())
Expand All @@ -102,11 +116,14 @@ mod tests {

#[test]
fn no_proxy_includes_direct_tcp_hosts() {
std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
std::env::set_var(
let _env = lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var(
"OPENSHELL_DIRECT_TCP_HOSTS",
"oauth2.googleapis.com,gmail.googleapis.com",
);
) };

let no_proxy = build_no_proxy();
assert_eq!(
Expand All @@ -115,24 +132,29 @@ mod tests {
);

// Clean up
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };
}

#[test]
fn no_proxy_includes_direct_tcp_endpoints() {
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
std::env::set_var(
let _env = lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var(
"OPENSHELL_DIRECT_TCP_ENDPOINTS",
"10.0.1.215:5432, 10.0.1.215:6379 , db.internal:1025,",
);
) };

let no_proxy = build_no_proxy();
assert_eq!(
no_proxy,
"127.0.0.1,localhost,::1,10.0.1.215,10.0.1.215,db.internal"
);

std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
}

#[test]
Expand Down
62 changes: 60 additions & 2 deletions crates/openshell-sandbox/src/proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1916,10 +1916,15 @@ fn rewrite_forward_request(
output.extend_from_slice(&raw[header_end..used]);
}

// Fail-closed: scan for any remaining unresolved placeholders
// Fail-closed: scan for any remaining unresolved placeholders or
// provider-shaped aliases, including the percent-decoded request line.
if secret_resolver.is_some() {
let output_str = String::from_utf8_lossy(&output);
if output_str.contains(crate::secrets::PLACEHOLDER_PREFIX_PUBLIC) {
let request_line = output_str.split("\r\n").next().unwrap_or("");
if crate::secrets::request_line_has_credential_marker(request_line) {
return Err(crate::secrets::UnresolvedPlaceholderError { location: "path" });
}
if crate::secrets::contains_credential_marker(&output_str) {
return Err(crate::secrets::UnresolvedPlaceholderError { location: "header" });
}
}
Expand Down Expand Up @@ -3170,6 +3175,59 @@ mod tests {
assert!(!result_str.contains("openshell:resolve:env:ANTHROPIC_API_KEY"));
}

fn github_alias_resolver() -> Option<SecretResolver> {
SecretResolver::from_provider_env(
[("GITHUB_TOKEN".to_string(), "github_pat_REAL".to_string())]
.into_iter()
.collect(),
)
.1
}

#[test]
fn test_rewrite_resolves_provider_alias_auth_header() {
let resolver = github_alias_resolver();
let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN\r\n\r\n";
let result = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref())
.expect("should succeed");
let result_str = String::from_utf8_lossy(&result);
assert!(result_str.contains("Authorization: token github_pat_REAL"));
assert!(!result_str.contains("OPENSHELL-RESOLVE-ENV-"));
}

#[test]
fn test_rewrite_rejects_unknown_provider_alias() {
let resolver = github_alias_resolver();
let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-OTHER_TOKEN\r\n\r\n";
let err = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref())
.expect_err("unresolved alias must fail closed");
assert_eq!(err.location, "header");
}

#[test]
fn test_rewrite_rejects_provider_alias_in_path() {
let resolver = github_alias_resolver();
let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\n\r\n";
assert!(
rewrite_forward_request(
raw,
raw.len(),
"/p?t=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN",
resolver.as_ref(),
)
.is_err()
);
assert!(
rewrite_forward_request(
raw,
raw.len(),
"/p?t=github_pat_OPENSHELL%2DRESOLVE%2DENV%2DGITHUB_TOKEN",
resolver.as_ref(),
)
.is_err()
);
}

// --- Forward proxy SSRF defence tests ---
//
// The forward proxy handler uses the same SSRF logic as the CONNECT path:
Expand Down
47 changes: 32 additions & 15 deletions crates/openshell-sandbox/src/sandbox/linux/netns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1152,31 +1152,40 @@ mod tests {

#[test]
fn test_parse_direct_tcp_hosts() {
std::env::set_var(
let _env = crate::child_env::lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var(
"OPENSHELL_DIRECT_TCP_HOSTS",
"oauth2.googleapis.com, gmail.googleapis.com , ",
);
) };
let hosts = parse_direct_tcp_hosts();
assert_eq!(hosts, vec!["oauth2.googleapis.com", "gmail.googleapis.com"]);
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };
}

#[test]
fn test_parse_direct_tcp_hosts_empty() {
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
let _env = crate::child_env::lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };
assert!(parse_direct_tcp_hosts().is_empty());

std::env::set_var("OPENSHELL_DIRECT_TCP_HOSTS", "");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var("OPENSHELL_DIRECT_TCP_HOSTS", "") };
assert!(parse_direct_tcp_hosts().is_empty());
std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") };
}

#[test]
fn test_parse_direct_tcp_endpoints_basic() {
std::env::set_var(
let _env = crate::child_env::lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var(
"OPENSHELL_DIRECT_TCP_ENDPOINTS",
"10.0.1.215:5432, 10.0.1.215:6379 , db.internal:1025,",
);
) };
let eps = parse_direct_tcp_endpoints();
assert_eq!(
eps,
Expand All @@ -1186,30 +1195,38 @@ mod tests {
DirectTcpEndpoint { host: "db.internal".into(), port: 1025 },
]
);
std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
}

#[test]
fn test_parse_direct_tcp_endpoints_invalid_entries_skipped() {
std::env::set_var(
let _env = crate::child_env::lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var(
"OPENSHELL_DIRECT_TCP_ENDPOINTS",
"host-no-port, :5432, host:abc, good.internal:8025",
);
) };
let eps = parse_direct_tcp_endpoints();
assert_eq!(
eps,
vec![DirectTcpEndpoint { host: "good.internal".into(), port: 8025 }]
);
std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
}

#[test]
fn test_parse_direct_tcp_endpoints_empty() {
std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
let _env = crate::child_env::lock_direct_tcp_env();
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
assert!(parse_direct_tcp_endpoints().is_empty());
std::env::set_var("OPENSHELL_DIRECT_TCP_ENDPOINTS", "");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::set_var("OPENSHELL_DIRECT_TCP_ENDPOINTS", "") };
assert!(parse_direct_tcp_endpoints().is_empty());
std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS");
// SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env().
unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") };
}

#[test]
Expand Down
Loading
Loading