From aa4ebf2f5b1eec6782ea31ddd2d9cb6d419a823a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Sun, 15 Feb 2026 22:41:59 +0300 Subject: [PATCH] various: harden input validation; add SSRF protection; fix default API key role Default API key role was "admin", which was something that I forgot to fix during testing. We change it to "read-only". Additionally repository URLs now reject `file://` scheme (another testing artifact) localhost, private IP ranges, and cloud metadata endpoints. Nix expressions reject path traversal (`..`) and absolute paths. Validation is called at the evaluator endtrypoint before command construction. Signed-off-by: NotAShelf Change-Id: I35729c6aa9ec4ff8d1ea19bd57ea93646a6a6964 --- crates/common/src/config.rs | 13 ++- crates/common/src/validate.rs | 187 ++++++++++++++++++++++++++++++++-- crates/evaluator/src/nix.rs | 4 + 3 files changed, 192 insertions(+), 12 deletions(-) diff --git a/crates/common/src/config.rs b/crates/common/src/config.rs index aaf420f..af11af8 100644 --- a/crates/common/src/config.rs +++ b/crates/common/src/config.rs @@ -402,7 +402,7 @@ const fn default_scheduling_shares() -> i32 { } fn default_role() -> String { - "admin".to_string() + "read-only".to_string() } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -764,6 +764,17 @@ mod tests { assert!(parsed.declarative.projects.is_empty()); } + #[test] + fn test_declarative_api_key_default_role_is_read_only() { + let toml_str = r#" + [[api_keys]] + name = "default-key" + key = "fc_test_123" + "#; + let config: DeclarativeConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.api_keys[0].role, "read-only"); + } + #[test] fn test_environment_override() { // Test environment variable parsing directly diff --git a/crates/common/src/validate.rs b/crates/common/src/validate.rs index 95511bc..7c38a6e 100644 --- a/crates/common/src/validate.rs +++ b/crates/common/src/validate.rs @@ -21,8 +21,6 @@ pub fn is_valid_nix_hash(hash: &str) -> bool { .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit()) } -// --- Validation trait and helpers --- - static NAME_RE: LazyLock = LazyLock::new(|| Regex::new(r"^[a-zA-Z0-9][a-zA-Z0-9_-]*$").unwrap()); @@ -36,6 +34,69 @@ const VALID_REPO_PREFIXES: &[&str] = &["https://", "http://", "git://", "ssh://", "file://"]; const VALID_FORGE_TYPES: &[&str] = &["github", "gitea", "forgejo", "gitlab"]; +/// Known internal/metadata IP ranges and hostnames to block for SSRF +/// protection. +const INTERNAL_HOSTS: &[&str] = &[ + "169.254.169.254", // AWS/GCP metadata + "metadata.google.internal", + "100.100.100.200", // Alibaba metadata +]; + +/// Extract the hostname from a URL (best-effort, no full URL parser needed). +fn extract_host_from_url(url: &str) -> Option { + // Strip scheme + let after_scheme = url.split("://").nth(1)?; + // Strip userinfo (user@host) + let after_user = after_scheme + .split_once('@') + .map_or(after_scheme, |(_, rest)| rest); + // Take host:port, strip port and path + let host_port = after_user.split('/').next()?; + let host = host_port.split(':').next()?; + if host.is_empty() { + return None; + } + Some(host.to_lowercase()) +} + +/// Check if a hostname is internal/metadata (SSRF targets). +fn is_internal_host(host: &str) -> bool { + if INTERNAL_HOSTS.iter().any(|&h| host == h) { + return true; + } + // Block localhost variants + if host == "localhost" + || host == "127.0.0.1" + || host == "::1" + || host == "[::1]" + { + return true; + } + // Block link-local 169.254.x.x + if host.starts_with("169.254.") { + return true; + } + // Block 10.x.x.x + if host.starts_with("10.") { + return true; + } + // Block 172.16-31.x.x + if host.starts_with("172.") { + if let Some(second_octet) = host.split('.').nth(1) { + if let Ok(n) = second_octet.parse::() { + if (16..=31).contains(&n) { + return true; + } + } + } + } + // Block 192.168.x.x + if host.starts_with("192.168.") { + return true; + } + false +} + /// Trait for validating request DTOs before persisting. pub trait Validate { fn validate(&self) -> Result<(), String>; @@ -67,6 +128,22 @@ fn validate_repository_url(url: &str) -> Result<(), String> { .to_string(), ); } + // Reject file:// URLs for SSRF protection (local filesystem access) + if url.starts_with("file://") { + return Err( + "repository_url must not use file:// scheme for security reasons" + .to_string(), + ); + } + // Reject URLs targeting common internal/metadata endpoints + if let Some(host) = extract_host_from_url(url) { + if is_internal_host(&host) { + return Err( + "repository_url must not target internal or metadata addresses" + .to_string(), + ); + } + } Ok(()) } @@ -77,7 +154,7 @@ fn validate_description(desc: &str) -> Result<(), String> { Ok(()) } -fn validate_nix_expression(expr: &str) -> Result<(), String> { +pub fn validate_nix_expression(expr: &str) -> Result<(), String> { if expr.is_empty() { return Err("nix_expression cannot be empty".to_string()); } @@ -87,6 +164,17 @@ fn validate_nix_expression(expr: &str) -> Result<(), String> { if expr.contains('\0') { return Err("nix_expression must not contain null bytes".to_string()); } + // Reject path traversal sequences + if expr.contains("..") { + return Err( + "nix_expression must not contain path traversal sequences (..)" + .to_string(), + ); + } + // Reject absolute paths - nix expressions should be relative attribute paths + if expr.starts_with('/') { + return Err("nix_expression must not be an absolute path".to_string()); + } Ok(()) } @@ -145,8 +233,6 @@ fn validate_forge_type(forge_type: &str) -> Result<(), String> { Ok(()) } -// --- Implementations --- - use crate::models::{ CreateBuild, CreateChannel, @@ -305,8 +391,6 @@ mod tests { use super::*; - // --- is_valid_store_path --- - #[test] fn valid_store_path() { assert!(is_valid_store_path( @@ -364,8 +448,6 @@ mod tests { assert!(!is_valid_store_path("/nix/store/abc..def")); } - // --- is_valid_nix_hash --- - #[test] fn valid_nix_hash_lowercase_alpha() { assert!(is_valid_nix_hash("abcdefghijklmnopqrstuvwxyzabcdef")); @@ -426,8 +508,6 @@ mod tests { assert!(!is_valid_nix_hash("' OR 1=1; DROP TABLE builds;--")); } - // --- Validate trait tests --- - #[test] fn test_create_project_valid() { let p = CreateProject { @@ -629,4 +709,89 @@ mod tests { }; assert!(c.validate().is_ok()); } + + #[test] + fn test_nix_expression_valid() { + assert!(validate_nix_expression("packages").is_ok()); + assert!(validate_nix_expression("checks.x86_64-linux").is_ok()); + assert!(validate_nix_expression("hydraJobs").is_ok()); + } + + #[test] + fn test_nix_expression_rejects_path_traversal() { + assert!(validate_nix_expression("../../../etc/passwd").is_err()); + assert!(validate_nix_expression("packages/..").is_err()); + assert!(validate_nix_expression("a..b").is_err()); + } + + #[test] + fn test_nix_expression_rejects_absolute_path() { + assert!(validate_nix_expression("/etc/passwd").is_err()); + assert!(validate_nix_expression("/nix/store/something").is_err()); + } + + #[test] + fn test_nix_expression_rejects_empty() { + assert!(validate_nix_expression("").is_err()); + } + + #[test] + fn test_nix_expression_rejects_null_bytes() { + assert!(validate_nix_expression("packages\0evil").is_err()); + } + + #[test] + fn test_repository_url_rejects_file_scheme() { + assert!(validate_repository_url("file:///etc/passwd").is_err()); + } + + #[test] + fn test_repository_url_rejects_localhost() { + assert!(validate_repository_url("http://localhost/repo.git").is_err()); + assert!(validate_repository_url("http://127.0.0.1/repo.git").is_err()); + } + + #[test] + fn test_repository_url_rejects_metadata_endpoint() { + assert!( + validate_repository_url("http://169.254.169.254/latest/meta-data") + .is_err() + ); + } + + #[test] + fn test_repository_url_rejects_private_networks() { + assert!(validate_repository_url("http://10.0.0.1/repo.git").is_err()); + assert!(validate_repository_url("http://192.168.1.1/repo.git").is_err()); + assert!(validate_repository_url("http://172.16.0.1/repo.git").is_err()); + } + + #[test] + fn test_repository_url_accepts_valid_https() { + assert!(validate_repository_url("https://github.com/test/repo").is_ok()); + assert!( + validate_repository_url("https://gitlab.com/test/repo.git").is_ok() + ); + assert!(validate_repository_url("git://example.com/repo.git").is_ok()); + assert!( + validate_repository_url("ssh://git@github.com/test/repo.git").is_ok() + ); + } + + #[test] + fn test_extract_host_from_url() { + assert_eq!( + extract_host_from_url("https://github.com/repo"), + Some("github.com".to_string()) + ); + assert_eq!( + extract_host_from_url("http://10.0.0.1:8080/repo"), + Some("10.0.0.1".to_string()) + ); + assert_eq!( + extract_host_from_url("ssh://user@host.com/repo"), + Some("host.com".to_string()) + ); + assert_eq!(extract_host_from_url("not-a-url"), None); + } } diff --git a/crates/evaluator/src/nix.rs b/crates/evaluator/src/nix.rs index 6b73dc9..8451944 100644 --- a/crates/evaluator/src/nix.rs +++ b/crates/evaluator/src/nix.rs @@ -114,6 +114,10 @@ pub async fn evaluate( config: &EvaluatorConfig, inputs: &[JobsetInput], ) -> Result { + // Validate nix expression before constructing any commands + fc_common::validate::validate_nix_expression(nix_expression) + .map_err(|e| CiError::NixEval(format!("Invalid nix expression: {e}")))?; + if flake_mode { evaluate_flake(repo_path, nix_expression, timeout, config, inputs).await } else {