feat(tvix/glue): emit a warning in case of bad SRI hashes r/7311

And include a test to ensure we show the warning.

Change-Id: Ib6a436dbba2592b398b54e44f15a48d1aa345099
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10470
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>

6 files changed, 50 insertions, 3 deletions

diff --git a/tvix/Cargo.lock b/tvix/Cargo.lock
index d9f0f3f5be15..abddd5b4a553 100644
--- a/tvix/Cargo.lock
+++ b/tvix/Cargo.lock
@@ -3405,6 +3405,7 @@ version = "0.1.0"
 dependencies = [
  "bytes",
  "criterion",
+ "data-encoding",
  "lazy_static",
  "nix-compat",
  "tempfile",
diff --git a/tvix/Cargo.nix b/tvix/Cargo.nix
index f92cc0b57d15..69b9cc9a8d97 100644
--- a/tvix/Cargo.nix
+++ b/tvix/Cargo.nix
@@ -10720,6 +10720,10 @@ rec {
             packageId = "bytes";
           }
           {
+            name = "data-encoding";
+            packageId = "data-encoding";
+          }
+          {
             name = "nix-compat";
             packageId = "nix-compat";
           }
diff --git a/tvix/eval/src/warnings.rs b/tvix/eval/src/warnings.rs
index e007d9c34f2d..f537aa913e40 100644
--- a/tvix/eval/src/warnings.rs
+++ b/tvix/eval/src/warnings.rs
@@ -18,6 +18,7 @@ pub enum WarningKind {
     EmptyInherit,
     EmptyLet,
     ShadowedOutput(String),
+    SRIHashWrongPadding,
 
     /// Tvix internal warning for features triggered by users that are
     /// not actually implemented yet, but do not cause runtime failures.
@@ -105,6 +106,7 @@ impl EvalWarning {
                 "this derivation's environment shadows the output name {}",
                 out
             ),
+            WarningKind::SRIHashWrongPadding => "SRI hash has wrong padding".to_string(),
 
             WarningKind::NotImplemented(what) => {
                 format!("feature not yet implemented in tvix: {}", what)
@@ -127,6 +129,7 @@ impl EvalWarning {
             WarningKind::EmptyInherit => "W009",
             WarningKind::EmptyLet => "W010",
             WarningKind::ShadowedOutput(_) => "W011",
+            WarningKind::SRIHashWrongPadding => "W012",
 
             WarningKind::NotImplemented(_) => "W999",
         }
diff --git a/tvix/glue/Cargo.toml b/tvix/glue/Cargo.toml
index b0902b5c9983..6b0edb5742e0 100644
--- a/tvix/glue/Cargo.toml
+++ b/tvix/glue/Cargo.toml
@@ -4,6 +4,7 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
+data-encoding = "2.3.3"
 nix-compat = { path = "../nix-compat" }
 tvix-build = { path = "../build", default-features = false, features = []}
 tvix-eval = { path = "../eval" }
diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs
index 993e7612aed4..57abe11b1912 100644
--- a/tvix/glue/src/builtins/derivation.rs
+++ b/tvix/glue/src/builtins/derivation.rs
@@ -106,12 +106,14 @@ fn populate_inputs<I: IntoIterator<Item = PathName>>(
 /// (lowercase) hex encoding of the digest.
 ///
 /// These values are only rewritten for the outputs, not what's passed to env.
+///
+/// The return value may optionally contain a warning.
 fn handle_fixed_output(
     drv: &mut Derivation,
     hash_str: Option<String>,      // in nix: outputHash
     hash_algo_str: Option<String>, // in nix: outputHashAlgo
     hash_mode_str: Option<String>, // in nix: outputHashmode
-) -> Result<(), ErrorKind> {
+) -> Result<Option<WarningKind>, ErrorKind> {
     // If outputHash is provided, ensure hash_algo_str is compatible.
     // If outputHash is not provided, do nothing.
     if let Some(hash_str) = hash_str {
@@ -125,6 +127,7 @@ fn handle_fixed_output(
         // construct a NixHash.
         let nixhash = nixhash::from_str(&hash_str, hash_algo_str.as_deref())
             .map_err(DerivationError::InvalidOutputHash)?;
+        let algo = nixhash.algo();
 
         // construct the fixed output.
         drv.outputs.insert(
@@ -140,8 +143,18 @@ fn handle_fixed_output(
                 },
             },
         );
+
+        // Peek at hash_str once more.
+        // If it was a SRI hash, but is not using the correct length, this means
+        // the padding was wrong. Emit a warning in that case.
+        let sri_prefix = format!("{}-", algo);
+        if let Some(rest) = hash_str.strip_prefix(&sri_prefix) {
+            if data_encoding::BASE64.encode_len(algo.digest_length()) != rest.len() {
+                return Ok(Some(WarningKind::SRIHashWrongPadding));
+            }
+        }
     }
-    Ok(())
+    Ok(None)
 }
 
 /// Handles derivation parameters which are not just forwarded to
@@ -349,7 +362,12 @@ pub(crate) mod derivation_builtins {
                 Err(cek) => return Ok(Value::Catchable(cek)),
                 Ok(s) => s,
             };
-            handle_fixed_output(&mut drv, output_hash, output_hash_algo, output_hash_mode)?;
+
+            if let Some(warning) =
+                handle_fixed_output(&mut drv, output_hash, output_hash_algo, output_hash_mode)?
+            {
+                emit_warning_kind(&co, warning).await;
+            }
         }
 
         // Scan references in relevant attributes to detect any build-references.
diff --git a/tvix/glue/src/builtins/mod.rs b/tvix/glue/src/builtins/mod.rs
index 86924cefef7f..aeae8a7c3baa 100644
--- a/tvix/glue/src/builtins/mod.rs
+++ b/tvix/glue/src/builtins/mod.rs
@@ -154,4 +154,24 @@ mod tests {
             "/171rf4jhx57xqz3p7swniwkig249cif71pa08p80mgaf0mqz5bmr"
         );
     }
+
+    /// constructs calls to builtins.derivation that should succeed, but produce warnings
+    #[test_case(r#"(builtins.derivation { name = "foo"; builder = "/bin/sh"; system = "x86_64-linux"; outputHashMode = "recursive"; outputHashAlgo = "sha256"; outputHash = "sha256-fgIr3TyFGDAXP5+qoAaiMKDg/a1MlT6Fv/S/DaA24S8===="; }).outPath"#, "/nix/store/xm1l9dx4zgycv9qdhcqqvji1z88z534b-foo"; "r:sha256 wrong padding")]
+    fn builtins_derivation_hash_wrong_padding_warn(code: &str, expected_path: &str) {
+        let eval_result = eval(code);
+
+        let value = eval_result.value.expect("must succeed");
+
+        match value {
+            tvix_eval::Value::String(s) => {
+                assert_eq!(expected_path, s.as_str());
+            }
+            _ => panic!("unexpected value type: {:?}", value),
+        }
+
+        assert!(
+            !eval_result.warnings.is_empty(),
+            "warnings should not be empty"
+        );
+    }
 }