refactor(server): simplify IP allowlist parsing and expansion

Reuse `IpAllowEntry::parse_literal` instead of duplicating `IpNet`
parsing in the resolver, and drop the unreachable defensive branch
in `expand_ip_allow_entries` that called `unwrap_or_default` on a
value that is always `Some` once an entry needs GitHub hooks.

Adds a middleware test covering X-Forwarded-For routing with a
non-zero trusted proxy count, which previously relied on
`extract_client_ip` unit tests alone.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-19 09:47:14 -04:00
parent f9279d0aca
commit 49ef284ed5
No known key found for this signature in database
2 changed files with 35 additions and 17 deletions

View file

@ -1,5 +1,3 @@
use std::net::IpAddr;
use fabro_types::settings::InterpString;
use fabro_types::settings::server::{
DiscordIntegrationSettings, GithubIntegrationSettings, IntegrationWebhooksLayer,
@ -15,7 +13,6 @@ use fabro_types::settings::server::{
TlsConfig,
};
use fabro_util::Home;
use ipnet::IpNet;
use super::{ResolveError, default_interp, parse_socket_addr, require_interp};
@ -257,8 +254,8 @@ fn resolve_ip_allow_entry(
return None;
}
match parse_ip_net(entry) {
Ok(net) => Some(IpAllowEntry::Literal(net)),
match IpAllowEntry::parse_literal(entry) {
Ok(parsed) => Some(parsed),
Err(reason) => {
errors.push(ResolveError::ParseFailure {
path: path.to_string(),
@ -269,13 +266,6 @@ fn resolve_ip_allow_entry(
}
}
fn parse_ip_net(value: &str) -> Result<IpNet, String> {
value
.parse::<IpNet>()
.or_else(|_| value.parse::<IpAddr>().map(IpNet::from))
.map_err(|error| error.to_string())
}
fn validate_ip_allowlist_for_listen(
listen: &ServerListenSettings,
ip_allowlist: &ServerIpAllowlistSettings,

View file

@ -279,18 +279,16 @@ async fn expand_ip_allow_entries(
.iter()
.any(|entry| matches!(entry, IpAllowEntry::GitHubMetaHooks))
{
Some(github_meta_resolver.resolve_hooks().await?)
github_meta_resolver.resolve_hooks().await?
} else {
None
Vec::new()
};
let mut expanded = Vec::new();
for entry in entries {
match entry {
IpAllowEntry::Literal(net) => expanded.push(*net),
IpAllowEntry::GitHubMetaHooks => {
expanded.extend(github_hooks.clone().unwrap_or_default());
}
IpAllowEntry::GitHubMetaHooks => expanded.extend(github_hooks.iter().copied()),
}
}
@ -512,6 +510,36 @@ mod tests {
assert!(config.allowlist.contains(&"192.30.252.42".parse().unwrap()));
}
#[tokio::test]
async fn middleware_reads_client_ip_from_x_forwarded_for_when_trusted_proxy_count_is_set() {
let config = Arc::new(IpAllowlistConfig {
allowlist: IpAllowlist::new(vec!["10.0.0.0/8".parse().unwrap()]),
trusted_proxy_count: 1,
});
let app = Router::new()
.route("/api/v1/runs", get(|| async { StatusCode::OK }))
.layer(middleware::from_fn_with_state(
Arc::clone(&config),
ip_allowlist_middleware,
));
let allowed_request = Request::builder()
.uri("/api/v1/runs")
.header("x-forwarded-for", "10.0.0.1, 198.51.100.1")
.body(Body::empty())
.unwrap();
let allowed_response = app.clone().oneshot(allowed_request).await.unwrap();
assert_eq!(allowed_response.status(), StatusCode::OK);
let blocked_request = Request::builder()
.uri("/api/v1/runs")
.header("x-forwarded-for", "203.0.113.1, 198.51.100.1")
.body(Body::empty())
.unwrap();
let blocked_response = app.oneshot(blocked_request).await.unwrap();
assert_eq!(blocked_response.status(), StatusCode::FORBIDDEN);
}
#[tokio::test]
async fn middleware_allows_health_and_blocks_non_allowlisted_requests() {
let config = Arc::new(IpAllowlistConfig {