diff --git a/snix/glue/src/builtins/derivation.rs b/snix/glue/src/builtins/derivation.rs index 6586c5ba8..8de6c1981 100644 --- a/snix/glue/src/builtins/derivation.rs +++ b/snix/glue/src/builtins/derivation.rs @@ -174,6 +174,7 @@ pub(crate) mod derivation_builtins { use bstr::ByteSlice; + use nix_compat::derivation::validate_output_name; use nix_compat::store_path::hash_placeholder; use snix_eval::generators::Gen; use snix_eval::{NixContext, NixContextElement, NixString, try_cek_to_value}; @@ -189,12 +190,17 @@ pub(crate) mod derivation_builtins { return Ok(input); } - let placeholder = hash_placeholder( - input - .to_str() - .context("looking at output name in builtins.placeholder")? - .to_str()?, - ); + let nix_string = input + .to_str() + .context("looking at output name in builtins.placeholder")?; + let output_name = nix_string.to_str()?; + + // Validate the output name using the same rules as derivations + validate_output_name(output_name).map_err(|e| { + ErrorKind::Abort(format!("invalid output name in builtins.placeholder: {e}")) + })?; + + let placeholder = hash_placeholder(output_name); Ok(placeholder.into()) } diff --git a/snix/glue/src/tests/snix_tests/eval-fail-placeholder-drv-name.nix b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-drv-name.nix new file mode 100644 index 000000000..584f6b475 --- /dev/null +++ b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-drv-name.nix @@ -0,0 +1,2 @@ +# Test that builtins.placeholder fails with the reserved 'drv' output name +builtins.placeholder "drv" diff --git a/snix/glue/src/tests/snix_tests/eval-fail-placeholder-empty-name.nix b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-empty-name.nix new file mode 100644 index 000000000..1898a0c20 --- /dev/null +++ b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-empty-name.nix @@ -0,0 +1,2 @@ +# Test that builtins.placeholder fails with an empty output name +builtins.placeholder "" diff --git a/snix/glue/src/tests/snix_tests/eval-fail-placeholder-invalid-chars.nix b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-invalid-chars.nix new file mode 100644 index 000000000..525fb1346 --- /dev/null +++ b/snix/glue/src/tests/snix_tests/eval-fail-placeholder-invalid-chars.nix @@ -0,0 +1,2 @@ +# Test that builtins.placeholder fails with invalid characters in output name +builtins.placeholder "invalid/name" diff --git a/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.exp b/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.exp new file mode 100644 index 000000000..da6e7b5e6 --- /dev/null +++ b/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.exp @@ -0,0 +1 @@ +"/1rz4g4znpzjwh1xymhjpm42vipw92pr73vdgl6xs1hycac8kf2n9" diff --git a/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.nix b/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.nix new file mode 100644 index 000000000..37abd3766 --- /dev/null +++ b/snix/glue/src/tests/snix_tests/eval-okay-placeholder-valid.nix @@ -0,0 +1,2 @@ +# Test that builtins.placeholder works with a valid output name +builtins.placeholder "out" diff --git a/snix/nix-compat/src/derivation/mod.rs b/snix/nix-compat/src/derivation/mod.rs index 2b8fc1e78..8d2311e4a 100644 --- a/snix/nix-compat/src/derivation/mod.rs +++ b/snix/nix-compat/src/derivation/mod.rs @@ -22,6 +22,7 @@ pub use crate::nixhash::{CAHash, NixHash}; pub use errors::{DerivationError, OutputError}; pub use output::Output; pub use parser::Error as ParserError; +pub use validate::validate_output_name; use self::write::AtermWriteable; diff --git a/snix/nix-compat/src/derivation/validate.rs b/snix/nix-compat/src/derivation/validate.rs index e7b24d84e..b525b02a6 100644 --- a/snix/nix-compat/src/derivation/validate.rs +++ b/snix/nix-compat/src/derivation/validate.rs @@ -1,6 +1,24 @@ use crate::derivation::{Derivation, DerivationError}; use crate::store_path; +/// Validates an output name using derivation output name rules. +/// +/// Output names must: +/// - Not be empty +/// - Not be "drv" (reserved name that would conflict with the existing drvPath key in builtins.derivation) +/// - Pass the `store_path::validate_name` check +/// +/// This function is used by both derivation validation and builtins.placeholder. +pub fn validate_output_name(output_name: &str) -> Result<(), DerivationError> { + if output_name.is_empty() + || output_name == "drv" + || store_path::validate_name(output_name.as_bytes()).is_err() + { + return Err(DerivationError::InvalidOutputName(output_name.to_string())); + } + Ok(()) +} + impl Derivation { /// validate ensures a Derivation struct is properly populated, /// and returns a [DerivationError] if not. @@ -18,21 +36,7 @@ impl Derivation { // Validate all outputs for (output_name, output) in &self.outputs { - // empty output names are invalid. - // - // `drv` is an invalid output name too, as this would cause - // a `builtins.derivation` call to return an attrset with a - // `drvPath` key (which already exists) and has a different - // meaning. - // - // Other output names that don't match the name restrictions from - // [StorePathRef] will fail the [StorePathRef::validate_name] check. - if output_name.is_empty() - || output_name == "drv" - || store_path::validate_name(output_name.as_bytes()).is_err() - { - return Err(DerivationError::InvalidOutputName(output_name.to_string())); - } + validate_output_name(output_name)?; if output.is_fixed() { if self.outputs.len() != 1 { @@ -66,18 +70,10 @@ impl Derivation { } for output_name in output_names.iter() { - // empty output names are invalid. - // - // `drv` is an invalid output name too, as this would cause - // a `builtins.derivation` call to return an attrset with a - // `drvPath` key (which already exists) and has a different - // meaning. - // - // Other output names that don't match the name restrictions from - // [StorePath] will fail the [StorePathRef::validate_name] check. - if output_name.is_empty() - || output_name == "drv" - || store_path::validate_name(output_name.as_bytes()).is_err() + // For input derivation output names, we use the same validation + // but map the error to the appropriate InputDerivationOutputName variant + if let Err(DerivationError::InvalidOutputName(_)) = + validate_output_name(output_name) { return Err(DerivationError::InvalidInputDerivationOutputName( input_derivation_path.to_string(), @@ -113,7 +109,46 @@ impl Derivation { mod test { use std::collections::BTreeMap; - use crate::derivation::{CAHash, Derivation, Output}; + use super::validate_output_name; + use crate::derivation::{CAHash, Derivation, DerivationError, Output}; + + /// Test the validate_output_name function with valid names + #[test] + fn test_validate_output_name_valid() { + // Valid output names should pass + assert!(validate_output_name("out").is_ok()); + assert!(validate_output_name("dev").is_ok()); + assert!(validate_output_name("lib").is_ok()); + assert!(validate_output_name("bin").is_ok()); + assert!(validate_output_name("debug").is_ok()); + } + + /// Test the validate_output_name function with invalid names + #[test] + fn test_validate_output_name_invalid() { + // Empty name should fail + assert!(matches!( + validate_output_name(""), + Err(DerivationError::InvalidOutputName(_)) + )); + + // "drv" is reserved and should fail + assert!(matches!( + validate_output_name("drv"), + Err(DerivationError::InvalidOutputName(_)) + )); + + // Invalid characters should fail + assert!(matches!( + validate_output_name("invalid/name"), + Err(DerivationError::InvalidOutputName(_)) + )); + + assert!(matches!( + validate_output_name("invalid name"), + Err(DerivationError::InvalidOutputName(_)) + )); + } /// Regression test: produce a Derivation that's almost valid, except its /// fixed-output output has the wrong hash specified.