fix(config): replace panics in parse_proxy_config with proper Result errors
parse_proxy_config was using expect()/unwrap()/panic!() which would crash the process on malformed proxy config strings instead of returning a descriptive error. Now returns ConfigError::ProxyConfigInvalid with the specific issue (bad scheme, bad address). Added tests for invalid scheme, invalid address, and end-to-end from_serve_options.
This commit is contained in:
@@ -57,7 +57,7 @@ impl StaticConfig {
|
|||||||
|
|
||||||
let dynamic = crate::config::DynamicConfig::new(auth_policy);
|
let dynamic = crate::config::DynamicConfig::new(auth_policy);
|
||||||
|
|
||||||
let proxy_config = parse_proxy_config(opts.proxy.as_deref());
|
let proxy_config = parse_proxy_config(opts.proxy.as_deref())?;
|
||||||
|
|
||||||
let listeners = if let Some(listeners) = opts.listeners {
|
let listeners = if let Some(listeners) = opts.listeners {
|
||||||
listeners
|
listeners
|
||||||
@@ -100,31 +100,41 @@ impl StaticConfig {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn parse_proxy_config(proxy: Option<&str>) -> Option<ProxyConfig> {
|
fn parse_proxy_config(
|
||||||
proxy.map(|url| {
|
proxy: Option<&str>,
|
||||||
if url.starts_with("socks5://") {
|
) -> Result<Option<ProxyConfig>, crate::error::ConfigError> {
|
||||||
let addr: SocketAddr = url
|
match proxy {
|
||||||
.strip_prefix("socks5://")
|
None => Ok(None),
|
||||||
.unwrap()
|
Some(url) => {
|
||||||
.parse()
|
if let Some(rest) = url.strip_prefix("socks5://") {
|
||||||
.expect("invalid socks5 proxy address");
|
let addr: SocketAddr = rest.parse().map_err(|e| {
|
||||||
ProxyConfig {
|
crate::error::ConfigError::ProxyConfigInvalid {
|
||||||
|
message: format!("invalid socks5 proxy address '{}': {}", rest, e),
|
||||||
|
}
|
||||||
|
})?;
|
||||||
|
Ok(Some(ProxyConfig {
|
||||||
mode: ProxyMode::Socks5(addr),
|
mode: ProxyMode::Socks5(addr),
|
||||||
|
}))
|
||||||
|
} else if let Some(rest) = url.strip_prefix("http://") {
|
||||||
|
let addr: SocketAddr = rest.parse().map_err(|e| {
|
||||||
|
crate::error::ConfigError::ProxyConfigInvalid {
|
||||||
|
message: format!(
|
||||||
|
"invalid http connect proxy address '{}': {}",
|
||||||
|
rest, e
|
||||||
|
),
|
||||||
}
|
}
|
||||||
} else if url.starts_with("http://") {
|
})?;
|
||||||
let addr: SocketAddr = url
|
Ok(Some(ProxyConfig {
|
||||||
.strip_prefix("http://")
|
|
||||||
.unwrap()
|
|
||||||
.parse()
|
|
||||||
.expect("invalid http connect proxy address");
|
|
||||||
ProxyConfig {
|
|
||||||
mode: ProxyMode::HttpConnect(addr),
|
mode: ProxyMode::HttpConnect(addr),
|
||||||
}
|
}))
|
||||||
} else {
|
} else {
|
||||||
panic!("unsupported proxy URL scheme: {url}");
|
Err(crate::error::ConfigError::ProxyConfigInvalid {
|
||||||
}
|
message: format!("unsupported proxy URL scheme: {}", url),
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
@@ -147,7 +157,7 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn parse_proxy_config_socks5() {
|
fn parse_proxy_config_socks5() {
|
||||||
let config = parse_proxy_config(Some("socks5://127.0.0.1:9050"));
|
let config = parse_proxy_config(Some("socks5://127.0.0.1:9050")).unwrap();
|
||||||
assert!(config.is_some());
|
assert!(config.is_some());
|
||||||
match config.unwrap().mode {
|
match config.unwrap().mode {
|
||||||
ProxyMode::Socks5(addr) => {
|
ProxyMode::Socks5(addr) => {
|
||||||
@@ -159,7 +169,7 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn parse_proxy_config_http() {
|
fn parse_proxy_config_http() {
|
||||||
let config = parse_proxy_config(Some("http://127.0.0.1:8080"));
|
let config = parse_proxy_config(Some("http://127.0.0.1:8080")).unwrap();
|
||||||
assert!(config.is_some());
|
assert!(config.is_some());
|
||||||
match config.unwrap().mode {
|
match config.unwrap().mode {
|
||||||
ProxyMode::HttpConnect(addr) => {
|
ProxyMode::HttpConnect(addr) => {
|
||||||
@@ -171,7 +181,31 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn parse_proxy_config_none() {
|
fn parse_proxy_config_none() {
|
||||||
assert!(parse_proxy_config(None).is_none());
|
assert!(parse_proxy_config(None).unwrap().is_none());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn parse_proxy_config_invalid_scheme() {
|
||||||
|
let result = parse_proxy_config(Some("ftp://127.0.0.1:9050"));
|
||||||
|
assert!(result.is_err());
|
||||||
|
match result.unwrap_err() {
|
||||||
|
crate::error::ConfigError::ProxyConfigInvalid { message } => {
|
||||||
|
assert!(message.contains("unsupported proxy URL scheme"));
|
||||||
|
}
|
||||||
|
e => panic!("expected ProxyConfigInvalid, got {:?}", e),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn parse_proxy_config_invalid_address() {
|
||||||
|
let result = parse_proxy_config(Some("socks5://not-an-address"));
|
||||||
|
assert!(result.is_err());
|
||||||
|
match result.unwrap_err() {
|
||||||
|
crate::error::ConfigError::ProxyConfigInvalid { message } => {
|
||||||
|
assert!(message.contains("invalid socks5 proxy address"));
|
||||||
|
}
|
||||||
|
e => panic!("expected ProxyConfigInvalid, got {:?}", e),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
@@ -206,4 +240,34 @@ mod tests {
|
|||||||
TransportKind::Tcp
|
TransportKind::Tcp
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn static_config_from_serve_options_invalid_proxy_returns_err() {
|
||||||
|
let opts = ServeOptions::new(make_key_source())
|
||||||
|
.authorized_keys(make_authorized_keys_source())
|
||||||
|
.proxy("ftp://bad-scheme");
|
||||||
|
let result = StaticConfig::from_serve_options(opts);
|
||||||
|
assert!(result.is_err());
|
||||||
|
match result.unwrap_err() {
|
||||||
|
crate::error::ConfigError::ProxyConfigInvalid { message } => {
|
||||||
|
assert!(message.contains("unsupported proxy URL scheme"));
|
||||||
|
}
|
||||||
|
e => panic!("expected ProxyConfigInvalid, got {:?}", e),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn static_config_from_serve_options_malformed_proxy_address_returns_err() {
|
||||||
|
let opts = ServeOptions::new(make_key_source())
|
||||||
|
.authorized_keys(make_authorized_keys_source())
|
||||||
|
.proxy("socks5://not-a-valid-addr");
|
||||||
|
let result = StaticConfig::from_serve_options(opts);
|
||||||
|
assert!(result.is_err());
|
||||||
|
match result.unwrap_err() {
|
||||||
|
crate::error::ConfigError::ProxyConfigInvalid { message } => {
|
||||||
|
assert!(message.contains("invalid socks5 proxy address"));
|
||||||
|
}
|
||||||
|
e => panic!("expected ProxyConfigInvalid, got {:?}", e),
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -67,6 +67,8 @@ pub enum ConfigError {
|
|||||||
},
|
},
|
||||||
#[error("incompatible options")]
|
#[error("incompatible options")]
|
||||||
IncompatibleOptions,
|
IncompatibleOptions,
|
||||||
|
#[error("invalid proxy config: {message}")]
|
||||||
|
ProxyConfigInvalid { message: String },
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, thiserror::Error)]
|
#[derive(Debug, thiserror::Error)]
|
||||||
@@ -173,6 +175,13 @@ mod tests {
|
|||||||
ConfigError::IncompatibleOptions.to_string(),
|
ConfigError::IncompatibleOptions.to_string(),
|
||||||
"incompatible options"
|
"incompatible options"
|
||||||
);
|
);
|
||||||
|
assert_eq!(
|
||||||
|
ConfigError::ProxyConfigInvalid {
|
||||||
|
message: "bad proxy".to_string()
|
||||||
|
}
|
||||||
|
.to_string(),
|
||||||
|
"invalid proxy config: bad proxy"
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
---
|
---
|
||||||
id: cleanup/panic-free-static-config
|
id: cleanup/panic-free-static-config
|
||||||
name: Replace panic/expect/unwrap with Result-based error handling in StaticConfig
|
name: Replace panic/expect/unwrap with Result-based error handling in StaticConfig
|
||||||
status: pending
|
status: completed
|
||||||
depends_on:
|
depends_on:
|
||||||
- review/phase1-core-modifications
|
- review/phase1-core-modifications
|
||||||
scope: narrow
|
scope: narrow
|
||||||
@@ -40,4 +40,4 @@ Since `StaticConfig::from_serve_options()` already returns `Result<..., ConfigEr
|
|||||||
|
|
||||||
## Summary
|
## Summary
|
||||||
|
|
||||||
> To be filled on completion
|
> Replaced all panic!/expect()/unwrap() in parse_proxy_config with Result-based error handling. Added ConfigError::ProxyConfigInvalid variant. Invalid proxy scheme or malformed address now returns clear errors instead of panicking. Added 4 new tests covering invalid scheme, invalid address, and from_serve_options error propagation.
|
||||||
Reference in New Issue
Block a user