Full-project review + fixes: fail-open security, quiche driver OOM/stall, TUIC lifecycle leaks, vacuous tests

crates/tuic-client/src/socks5/handle_task.rs

@@ -175,7 +175,14 @@ impl Server {
 
 		debug!("[socks5] [{peer_addr}] [associate] [{assoc_id:#06x}] stopped associating");
 
-		outbound_handle.abort();
+		// Let the outbound UDP handler wind down on its own instead of aborting
+		// it. By this point `local_incoming` has ended (or been cancelled by the
+		// select), so its `local_to_remote_tx` is dropped and the handler's input
+		// channel is closed — `handle_udp` then exits gracefully and sends the
+		// `Dissociate` to the server. `abort()` here would kill it mid-teardown,
+		// leaking the server-side association until its GC timeout. Detaching (via
+		// drop) lets the self-terminating task finish that teardown.
+		drop(outbound_handle);
 		remote_handle.abort();
 	}
 }

crates/tuic-client/src/socks5/mod.rs

@@ -118,7 +118,7 @@ impl Server {
 	pub async fn start(cancel: CancellationToken) {
 		let server = SERVER.get().unwrap();
 
-		warn!(
+		info!(
 			"[socks5] server started, listening on {}",
 			server.listener.local_addr().unwrap()
 		);

crates/tuic-client/src/wind_adapter.rs

@@ -46,7 +46,13 @@ impl TuicOutboundAdapter {
 		let sni = match relay.sni.clone() {
 			Some(s) => s,
 			None => {
-				if relay.server.0.parse::<std::net::IpAddr>().is_ok() {
+				// Strip any surrounding brackets so a bracketed IPv6 literal
+				// (`[::1]`) is recognized as an IP literal too -- otherwise the
+				// bracketed form fails `IpAddr::parse` and gets announced
+				// verbatim as the SNI, which rustls rejects ("invalid server
+				// name").
+				let host = relay.server.0.trim_start_matches('[').trim_end_matches(']');
+				if host.parse::<std::net::IpAddr>().is_ok() {
 					tracing::warn!(
 						"relay server `{}` is an IP literal but no `sni` was configured; TLS verification will likely fail. \
 						 Set `sni = \"<hostname>\"` in the relay config to fix.",

crates/tuic-core/Cargo.toml

@@ -23,7 +23,6 @@ tokio-util = { version = "0.7", features = ["codec"] }
 num_enum = "0.7"
 snafu = "0.9"
 uuid = { version = "1", features = ["v4"] }
-hex = { package = "const-hex", version = "1" }
 
 # UDP fragment reassembly state machine
 moka = { version = "0.12", features = ["future"] }
@@ -39,3 +38,4 @@ futures-util = { version = "0.3", default-features = false, features = ["sink"]
 tokio-stream = "0.1"
 test-log = { version = "0.2", features = ["trace"] }
 eyre = "0.6"
+hex = { package = "const-hex", version = "1" }

crates/tuic-core/src/proto/error.rs

@@ -45,13 +45,13 @@ pub enum ProtoError {
 
 impl From<std::io::Error> for ProtoError {
 	#[inline(always)]
-	fn from(_source: std::io::Error) -> Self {
-		#[cfg(debug_assertions)]
-		panic!("IO error should not be created by From<io::Error>");
-		#[cfg(not(debug_assertions))]
-		{
-			use snafu::IntoError as _;
-			IoSnafu.into_error(_source)
-		}
+	fn from(source: std::io::Error) -> Self {
+		// `Decoder::Error: From<io::Error>` is required by tokio-util's `Framed*`,
+		// and the underlying transport's IO errors (e.g. a peer reset) flow
+		// through here. The previous debug-build `panic!` turned any such error
+		// into a crash the moment these codecs were driven over real IO, so map
+		// it to the `Io` variant in all builds instead.
+		use snafu::IntoError as _;
+		IoSnafu.into_error(source)
 	}
 }

crates/tuic-server/src/legacy/acl.pest

@@ -24,10 +24,20 @@ address = {
     | any_addr 
 }
 
-// Special keywords
-localhost_kw = { "localhost" }
-private_kw = { "private" }
-suffix_localhost = { "suffix:localhost" }
+// Special keywords.
+//
+// These must be atomic (`@{}`) and carry a trailing boundary assertion, like
+// `ipv6` does. Without the boundary, a bare literal like `"private"` matches
+// the prefix of a longer token: `proxy privatetracker.org` would parse as
+// `addr = private, hijack = "tracker.org"`, silently routing the whole RFC1918
+// range to `proxy` while ignoring the intended domain. The boundary forces the
+// keyword to be a complete token (followed by whitespace or end-of-input);
+// `privatetracker.org` then falls through to the `domain` rule as intended.
+// (A non-atomic rule would let pest consume the following whitespace before the
+// lookahead runs, defeating it -- hence `@{}`.)
+localhost_kw = @{ "localhost" ~ &(WHITESPACE | EOI) }
+private_kw = @{ "private" ~ &(WHITESPACE | EOI) }
+suffix_localhost = @{ "suffix:localhost" ~ &(WHITESPACE | EOI) }
 any_addr = { "*" }
 
 // IP addresses

crates/tuic-server/src/legacy/mod.rs

@@ -666,10 +666,17 @@ fn address_to_rule_types(addr: &AclAddress) -> Vec<wrule::RuleType> {
 			}
 		}
 		AclAddress::Cidr(cidr_str) => {
-			if let Ok(net) = cidr_str.parse::<ipnet::IpNet>() {
-				vec![wrule::RuleType::IpCidr(net)]
-			} else {
-				vec![]
+			// The grammar accepts prefixes like `/999` (acl.pest only bounds the
+			// digit count), so a malformed CIDR such as `10.0.0.0/99` can reach
+			// here. Warn on failure -- like the `Ip` arm above -- instead of
+			// dropping the rule silently, which would fail-open (e.g. a `reject`
+			// rule vanishing and its traffic being allowed).
+			match cidr_str.parse::<ipnet::IpNet>() {
+				Ok(net) => vec![wrule::RuleType::IpCidr(net)],
+				Err(e) => {
+					tracing::warn!("ACL entry {cidr_str:?} could not be parsed as a CIDR ({e}); rule dropped");
+					vec![]
+				}
 			}
 		}
 		AclAddress::Domain(domain) => {
@@ -927,6 +934,26 @@ mod tests {
 		assert!(result.ports.is_some());
 	}
 
+	#[tokio::test]
+	async fn keyword_prefix_of_domain_is_not_swallowed() {
+		// `private`/`localhost` must be complete tokens, not prefixes. Before the
+		// boundary assertion, `proxy privatetracker.org` parsed as
+		// addr=Private + hijack="tracker.org", silently rerouting all of RFC1918.
+		let result = parse_acl_rule("proxy privatetracker.org").unwrap();
+		assert_eq!(result.outbound, "proxy");
+		assert_eq!(result.addr, AclAddress::Domain("privatetracker.org".into()));
+		assert_eq!(result.hijack, None);
+
+		let result = parse_acl_rule("allow localhost5.com").unwrap();
+		assert_eq!(result.addr, AclAddress::Domain("localhost5.com".into()));
+		assert_eq!(result.ports, None);
+		assert_eq!(result.hijack, None);
+
+		// The bare keywords still parse as keywords.
+		assert_eq!(parse_acl_rule("allow private").unwrap().addr, AclAddress::Private);
+		assert_eq!(parse_acl_rule("allow localhost").unwrap().addr, AclAddress::Localhost);
+	}
+
 	#[tokio::test]
 	async fn any_match() {
 		let rule = AclRule {

crates/tuic-server/src/wind_adapter.rs

@@ -77,26 +77,51 @@ impl wind_core::AbstractInbound for ServerInbound {
 }
 
 /// Build an [`OutboundAction`] for a single configured outbound rule.
-fn make_outbound_action(rule: &OutboundRule, resolver: Arc<dyn Resolver>) -> Arc<dyn OutboundAction> {
+///
+/// `stream_timeout` is the configured half-close idle timeout for relays; it
+/// was previously hardcoded to `Duration::ZERO`, silently disabling the feature
+/// and ignoring the operator's `stream_timeout` setting.
+fn make_outbound_action(rule: &OutboundRule, resolver: Arc<dyn Resolver>, stream_timeout: Duration) -> Arc<dyn OutboundAction> {
 	match rule.kind.as_str() {
 		"socks5" => Arc::new(Socks5Action::new(Socks5ActionOpts {
 			addr: rule.addr.clone().unwrap_or_default(),
 			username: rule.username.clone(),
 			password: rule.password.clone(),
 			allow_udp: rule.allow_udp,
-			stream_timeout: Duration::ZERO,
+			stream_timeout,
 			tcp_keepalive: Some(wind_core::tcp::TcpKeepalive::default()),
 		})),
-		_ => Arc::new(DirectOutbound::new(
+		"direct" => Arc::new(DirectOutbound::new(
 			DirectOutboundOpts {
 				bind_ipv4: rule.bind_ipv4,
 				bind_ipv6: rule.bind_ipv6,
 				bind_device: rule.bind_device.clone(),
-				stream_timeout: Duration::ZERO,
+				stream_timeout,
 				tcp_keepalive: Some(wind_core::tcp::TcpKeepalive::default()),
 			},
 			resolver,
 		)),
+		// An unknown outbound type (typically a typo such as "Socks5" or
+		// "sock5") must NOT silently become a direct outbound: that would send
+		// traffic out via the server's own IP instead of the intended tunnel.
+		// Warn loudly and fall back to direct so the misconfiguration is
+		// visible in logs rather than being a silent security downgrade.
+		other => {
+			tracing::warn!(
+				outbound_type = %other,
+				"unknown outbound type; falling back to DIRECT. Expected \"direct\" or \"socks5\""
+			);
+			Arc::new(DirectOutbound::new(
+				DirectOutboundOpts {
+					bind_ipv4: rule.bind_ipv4,
+					bind_ipv6: rule.bind_ipv6,
+					bind_device: rule.bind_device.clone(),
+					stream_timeout,
+					tcp_keepalive: Some(wind_core::tcp::TcpKeepalive::default()),
+				},
+				resolver,
+			))
+		}
 	}
 }
 
@@ -196,10 +221,14 @@ fn build_dispatcher(ctx: Arc<TuicAppContext>, resolver: Arc<dyn Resolver>) -> Di
 	let router = TuicRouter::new(ctx.clone(), resolver.clone());
 	let mut dispatcher = Dispatcher::new(router);
 
-	dispatcher.add_handler("default", make_outbound_action(&cfg.outbound.default, resolver.clone()));
+	let stream_timeout = cfg.stream_timeout;
+	dispatcher.add_handler(
+		"default",
+		make_outbound_action(&cfg.outbound.default, resolver.clone(), stream_timeout),
+	);
 
 	for (name, rule) in &cfg.outbound.named {
-		dispatcher.add_handler(name.clone(), make_outbound_action(rule, resolver.clone()));
+		dispatcher.add_handler(name.clone(), make_outbound_action(rule, resolver.clone(), stream_timeout));
 	}
 
 	dispatcher

crates/tuic-tests/tests/integration_tests.rs

@@ -275,10 +275,12 @@ async fn test_server_client_integration() -> eyre::Result<()> {
 
 	info!("[Integration Test] Starting TUIC server on 127.0.0.1:8443...");
 	let server_handle = tokio::spawn(async move {
-		match timeout(Duration::from_secs(10), tuic_server::run(server_config)).await {
+		// Must outlast the whole test body (TCP + UDP + concurrent phases with
+		// their own timeouts); a 10s cap could kill the server mid-test.
+		match timeout(Duration::from_secs(30), tuic_server::run(server_config)).await {
 			Ok(Ok(())) => info!("[Integration Test] Server completed successfully"),
 			Ok(Err(e)) => error!("[Integration Test] Server error: {}", e),
-			Err(_) => error!("[Integration Test] Server timeout"),
+			Err(_) => info!("[Integration Test] Server timed out (expected at test end)"),
 		}
 	});
 
@@ -358,16 +360,20 @@ async fn test_server_client_integration() -> eyre::Result<()> {
 		tokio::time::sleep(Duration::from_millis(200)).await;
 
 		let test_data = b"Hello, TUIC!";
-		test_tcp_through_socks5("127.0.0.1:1080", echo_addr, test_data, "TCP Test").await;
+		let ok = test_tcp_through_socks5("127.0.0.1:1080", echo_addr, test_data, "TCP Test").await;
 
 		info!("[TCP Test] Waiting for echo server to finish...");
 		tokio::time::sleep(Duration::from_millis(500)).await;
 
 		echo_task.abort();
 		info!("[TCP Test] TCP test completed\n");
+		ok
 	};
 
-	let _ = timeout(Duration::from_secs(6), tcp_test).await;
+	let tcp_ok = timeout(Duration::from_secs(6), tcp_test)
+		.await
+		.expect("TCP relay test timed out");
+	assert!(tcp_ok, "TCP relay through SOCKS5/TUIC failed");
 
 	let udp_test = async {
 		use std::net::{IpAddr, Ipv4Addr};
@@ -382,13 +388,17 @@ async fn test_server_client_integration() -> eyre::Result<()> {
 
 		let test_data = b"Hello, UDP through TUIC!";
 		let client_bind_addr = std::net::SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 0);
-		test_udp_through_socks5("127.0.0.1:1080", echo_addr, test_data, "UDP Test", client_bind_addr).await;
+		let ok = test_udp_through_socks5("127.0.0.1:1080", echo_addr, test_data, "UDP Test", client_bind_addr).await;
 
 		echo_task.abort();
 		info!("[UDP Test] UDP test completed\n");
+		ok
 	};
 
-	let _ = timeout(Duration::from_secs(3), udp_test).await;
+	let udp_ok = timeout(Duration::from_secs(3), udp_test)
+		.await
+		.expect("UDP relay test timed out");
+	assert!(udp_ok, "UDP relay through SOCKS5/TUIC failed");
 
 	let concurrent_test = async {
 		use fast_socks5::client::{Config, Socks5Stream};
@@ -514,6 +524,14 @@ async fn test_ipv6_server_client_integration() -> eyre::Result<()> {
 	info!("[IPv6 Test] Starting IPv6 Integration Test");
 	info!("[IPv6 Test] ========================================\n");
 
+	// Skip (rather than silently pass) when the environment has no IPv6
+	// loopback -- common on constrained CI runners. If [::1] is available we
+	// require the relay to actually work below.
+	if tokio::net::TcpListener::bind("[::1]:0").await.is_err() {
+		info!("[IPv6 Test] no IPv6 loopback available; skipping");
+		return Ok(());
+	}
+
 	let server_config = tuic_server::Config {
 		log_level: tuic_server::config::LogLevel::Debug,
 		server: "[::1]:8444".parse::<SocketAddr>()?,
@@ -547,6 +565,14 @@ async fn test_ipv6_server_client_integration() -> eyre::Result<()> {
 		max_external_packet_size: 1500,
 		stream_timeout: Duration::from_secs(60),
 		outbound: tuic_server::config::OutboundConfig::default(),
+		// The echo target is `[::1]` (loopback), so the loopback guard must be
+		// off or the relay is rejected before it reaches the outbound. The IPv4
+		// test sets this too; the IPv6 test previously relied on the default
+		// (guard on) and silently passed only because it made no assertions.
+		experimental: ExperimentalConfig {
+			drop_loopback: false,
+			..Default::default()
+		},
 		// Allow localhost connections for testing
 		acl: vec![tuic_server::legacy::AclRule {
 			outbound: "allow".to_string(),
@@ -559,10 +585,11 @@ async fn test_ipv6_server_client_integration() -> eyre::Result<()> {
 
 	info!("[IPv6 Test] Starting TUIC server on [::1]:8444...");
 	let server_handle = tokio::spawn(async move {
-		match timeout(Duration::from_secs(10), tuic_server::run(server_config)).await {
+		// Must outlast the whole test body; a 10s cap could kill it mid-test.
+		match timeout(Duration::from_secs(30), tuic_server::run(server_config)).await {
 			Ok(Ok(())) => info!("[IPv6 Test] Server completed successfully"),
 			Ok(Err(e)) => error!("[IPv6 Test] Server error: {}", e),
-			Err(_) => error!("[IPv6 Test] Server timeout"),
+			Err(_) => info!("[IPv6 Test] Server timed out (expected at test end)"),
 		}
 	});
 
@@ -648,13 +675,17 @@ async fn test_ipv6_server_client_integration() -> eyre::Result<()> {
 		tokio::time::sleep(Duration::from_millis(200)).await;
 
 		let test_data = b"Hello IPv6 TUIC!";
-		test_tcp_through_socks5("[::1]:1081", echo_addr, test_data, "IPv6 TCP Test").await;
+		let ok = test_tcp_through_socks5("[::1]:1081", echo_addr, test_data, "IPv6 TCP Test").await;
 
 		echo_task.abort();
 		info!("[IPv6 TCP Test] TCP test completed\n");
+		ok
 	};
 
-	let _ = timeout(Duration::from_secs(6), tcp_test).await;
+	let tcp_ok = timeout(Duration::from_secs(6), tcp_test)
+		.await
+		.expect("IPv6 TCP relay test timed out");
+	assert!(tcp_ok, "IPv6 TCP relay through SOCKS5/TUIC failed");
 
 	let udp_test = async {
 		use std::net::{IpAddr, Ipv6Addr};
@@ -667,13 +698,17 @@ async fn test_ipv6_server_client_integration() -> eyre::Result<()> {
 
 		let test_data = b"Hello, IPv6 UDP through TUIC!";
 		let client_bind_addr = std::net::SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 0);
-		test_udp_through_socks5("[::1]:1081", echo_addr, test_data, "IPv6 UDP Test", client_bind_addr).await;
+		let ok = test_udp_through_socks5("[::1]:1081", echo_addr, test_data, "IPv6 UDP Test", client_bind_addr).await;
 
 		echo_task.abort();
 		info!("[IPv6 UDP Test] UDP test completed\n");
+		ok
 	};
 
-	let _ = timeout(Duration::from_secs(3), udp_test).await;
+	let udp_ok = timeout(Duration::from_secs(3), udp_test)
+		.await
+		.expect("IPv6 UDP relay test timed out");
+	assert!(udp_ok, "IPv6 UDP relay through SOCKS5/TUIC failed");
 
 	client_handle.abort();
 	server_handle.abort();