fix: Consistent caching and checksumming (#361)

adamspofford-dfinity · web-flow · commit 825f6f52aed1 · 2026-02-13T12:23:47.000-08:00
diff --git a/crates/icp/src/canister/recipe/handlebars.rs b/crates/icp/src/canister/recipe/handlebars.rs
@@ -16,7 +16,10 @@ use crate::{
         canister::{BuildSteps, SyncSteps},
         recipe::{Recipe, RecipeType},
     },
-    package::{PackageCache, cache_recipe, read_cached_recipe},
+    package::{
+        PackageCache, cache_registry_recipe, cache_uri_recipe, read_cached_registry_recipe,
+        read_cached_uri_recipe,
+    },
     prelude::*,
 };
 
@@ -78,12 +81,6 @@ pub enum HandlebarsError {
 
     #[snafu(display("failed to acquire lock on package cache"))]
     LockCache { source: crate::fs::lock::LockError },
-
-    #[snafu(display("failed to resolve git tag '{tag}' for recipe"))]
-    ResolveGitTag { source: reqwest::Error, tag: String },
-
-    #[snafu(display("failed to parse git tag response for '{tag}'"))]
-    ParseGitTag { tag: String },
 }
 
 impl Handlebars {
@@ -92,7 +89,7 @@ impl Handlebars {
         recipe: &Recipe,
     ) -> Result<(BuildSteps, SyncSteps), HandlebarsError> {
         // Determine the template source
-        let tmpl = match &recipe.recipe_type {
+        let tmpl_source = match &recipe.recipe_type {
             RecipeType::File(path) => TemplateSource::LocalPath(Path::new(&path).into()),
             RecipeType::Url(url) => TemplateSource::RemoteUrl(url.to_owned()),
             RecipeType::Registry {
@@ -103,18 +100,30 @@ impl Handlebars {
         };
 
         // Retrieve the template, using cache for remote/registry sources
-        let tmpl = match tmpl {
+        let (tmpl, should_cache) = match &tmpl_source {
             TemplateSource::LocalPath(path) => {
-                let bytes = read(&path).context(ReadFileSnafu)?;
-                if let Some(expected) = &recipe.sha256 {
-                    verify_checksum(&bytes, expected)?;
-                }
-                parse_bytes_to_string(bytes)?
+                let bytes = read(path).context(ReadFileSnafu)?;
+                (parse_bytes_to_string(bytes)?, false)
             }
 
             TemplateSource::RemoteUrl(u) => {
-                self.fetch_remote_template(&u, recipe.sha256.as_deref())
-                    .await?
+                // Check cache
+                let maybe_cached = self
+                    .pkg_cache
+                    .with_read(async |r| {
+                        read_cached_uri_recipe(r, u, recipe.sha256.as_deref())
+                            .context(ReadCacheSnafu)
+                    })
+                    .await
+                    .context(LockCacheSnafu)?;
+                if let Some(cached) = maybe_cached? {
+                    debug!("Using cached recipe template for {u}");
+                    (parse_bytes_to_string(cached)?, false)
+                } else {
+                    // Download the template
+                    let tmpl = self.fetch_remote_bytes(u).await?;
+                    (parse_bytes_to_string(tmpl)?, true)
+                }
             }
 
             // TMP(or.ricon): Temporarily hardcode a dfinity registry
@@ -130,43 +139,31 @@ impl Handlebars {
                 let maybe_cached = self
                     .pkg_cache
                     .with_read(async |r| {
-                        read_cached_recipe(r, &package, &version).context(ReadCacheSnafu)
+                        read_cached_registry_recipe(r, &package, version).context(ReadCacheSnafu)
                     })
                     .await
                     .context(LockCacheSnafu)?;
                 if let Some(cached) = maybe_cached? {
                     debug!("Using cached recipe template for {package}@{version}");
-                    parse_bytes_to_string(cached)?
+                    (parse_bytes_to_string(cached)?, false)
                 } else {
                     // Download the template
                     let url = format!(
                         "https://github.com/dfinity/icp-cli-recipes/releases/download/{release_tag}/recipe.hbs"
                     );
                     let bytes = self.fetch_remote_bytes(&url).await?;
 
-                    if let Some(expected) = &recipe.sha256 {
-                        verify_checksum(&bytes, expected)?;
-                    }
-
-                    // Resolve the git tag to a commit SHA for caching
-                    let git_sha = self
-                        .resolve_git_tag_sha("dfinity", "icp-cli-recipes", &release_tag)
-                        .await?;
-
-                    // Cache the template keyed by the git SHA
-                    self.pkg_cache
-                        .with_write(async |w| {
-                            cache_recipe(w, &package, &version, &git_sha, &bytes)
-                                .context(CacheRecipeSnafu)
-                        })
-                        .await
-                        .context(LockCacheSnafu)??;
-
-                    parse_bytes_to_string(bytes)?
+                    (parse_bytes_to_string(bytes)?, true)
                 }
             }
         };
 
+        let hash = if let Some(sha256) = &recipe.sha256 {
+            verify_checksum(tmpl.as_bytes(), sha256)?
+        } else {
+            Sha256::digest(tmpl.as_bytes()).into()
+        };
+
         // Load the template via handlebars
         let mut reg = handlebars::Handlebars::new();
 
@@ -204,8 +201,8 @@ impl Handlebars {
         }
 
         let insts = serde_yaml::from_str::<BuildSyncHelper>(&out);
-        match insts {
-            Ok(helper) => Ok((helper.build, helper.sync)),
+        let (build, sync) = match insts {
+            Ok(helper) => (helper.build, helper.sync),
             Err(e) => panic!(
                 "{}",
                 formatdoc! {r#"
@@ -217,7 +214,41 @@ impl Handlebars {
                 ------
             "#, recipe.recipe_type}
             ),
+        };
+
+        // The template is verified good - now cache it if it was remote
+        if should_cache {
+            match tmpl_source {
+                TemplateSource::LocalPath(_) => unreachable!("local files are never cached"),
+                TemplateSource::RemoteUrl(u) => {
+                    self.pkg_cache
+                        .with_write(async |w| {
+                            cache_uri_recipe(w, &u, &hex::encode(hash), tmpl.as_bytes())
+                                .context(CacheRecipeSnafu)?;
+                            Ok(())
+                        })
+                        .await
+                        .context(LockCacheSnafu)??;
+                }
+                TemplateSource::Registry(registry, recipe_name, version) => {
+                    let package = format!("@{registry}/{recipe_name}");
+                    self.pkg_cache
+                        .with_write(async |w| {
+                            cache_registry_recipe(
+                                w,
+                                &package,
+                                &version,
+                                &hex::encode(hash),
+                                tmpl.as_bytes(),
+                            )
+                            .context(CacheRecipeSnafu)
+                        })
+                        .await
+                        .context(LockCacheSnafu)??;
+                }
+            }
         }
+        Ok((build, sync))
     }
 
     /// Fetch raw bytes from a remote URL.
@@ -241,80 +272,6 @@ impl Handlebars {
 
         Ok(resp.bytes().await.context(HttpRequestSnafu)?.to_vec())
     }
-
-    /// Fetch a remote template, verifying checksum if provided.
-    async fn fetch_remote_template(
-        &self,
-        url: &str,
-        sha256: Option<&str>,
-    ) -> Result<String, HandlebarsError> {
-        let bytes = self.fetch_remote_bytes(url).await?;
-        if let Some(expected) = sha256 {
-            verify_checksum(&bytes, expected)?;
-        }
-        parse_bytes_to_string(bytes)
-    }
-
-    /// Resolve a GitHub release tag to its underlying git commit SHA.
-    async fn resolve_git_tag_sha(
-        &self,
-        owner: &str,
-        repo: &str,
-        tag: &str,
-    ) -> Result<String, HandlebarsError> {
-        let ref_response = self
-            .github_api_get(
-                &format!("https://api.github.com/repos/{owner}/{repo}/git/ref/tags/{tag}"),
-                tag,
-            )
-            .await?;
-
-        let obj_type = ref_response["object"]["type"]
-            .as_str()
-            .ok_or_else(|| ParseGitTagSnafu { tag }.build())?;
-        let obj_sha = ref_response["object"]["sha"]
-            .as_str()
-            .ok_or_else(|| ParseGitTagSnafu { tag }.build())?;
-
-        match obj_type {
-            // Lightweight tag — points directly at the commit
-            "commit" => Ok(obj_sha.to_owned()),
-            // Annotated tag — dereference through the tag object to get the commit
-            "tag" => {
-                let tag_response = self
-                    .github_api_get(
-                        &format!("https://api.github.com/repos/{owner}/{repo}/git/tags/{obj_sha}"),
-                        tag,
-                    )
-                    .await?;
-                tag_response["object"]["sha"]
-                    .as_str()
-                    .map(str::to_owned)
-                    .ok_or_else(|| ParseGitTagSnafu { tag }.build())
-            }
-            _ => ParseGitTagSnafu { tag }.fail(),
-        }
-    }
-
-    /// Make an authenticated GET request to the GitHub API.
-    async fn github_api_get(
-        &self,
-        url: &str,
-        tag: &str,
-    ) -> Result<serde_json::Value, HandlebarsError> {
-        let mut req = self.http_client.get(url).header("User-Agent", "icp-cli");
-        if let Ok(token) = std::env::var("ICP_CLI_GITHUB_TOKEN") {
-            req = req.bearer_auth(token);
-        }
-        req.send()
-            .await
-            .context(ResolveGitTagSnafu { tag })?
-            .error_for_status()
-            .context(ResolveGitTagSnafu { tag })?
-            .json()
-            .await
-            .context(ResolveGitTagSnafu { tag })
-    }
 }
 
 #[async_trait]
@@ -353,22 +310,21 @@ impl HelperDef for ReplaceHelper {
 }
 
 /// Helper function to verify sha256 checksum of recipe template bytes
-fn verify_checksum(bytes: &[u8], expected: &str) -> Result<(), HandlebarsError> {
-    let actual = hex::encode({
+fn verify_checksum(bytes: &[u8], expected: &str) -> Result<[u8; 32], HandlebarsError> {
+    let actual_hash = {
         let mut h = Sha256::new();
         h.update(bytes);
         h.finalize()
-    });
-
+    };
+    let actual = hex::encode(actual_hash);
     if actual != expected {
         return ChecksumMismatchSnafu {
             expected: expected.to_string(),
             actual,
         }
         .fail();
     }
-
-    Ok(())
+    Ok(actual_hash.into())
 }
 
 /// Helper function to parse bytes into a UTF-8 string
diff --git a/crates/icp/src/manifest/recipe.rs b/crates/icp/src/manifest/recipe.rs
@@ -6,7 +6,7 @@ use serde::{Deserialize, de::Error as _};
 /// Represents the accepted values for a recipe type in
 /// the canister manifest
 #[derive(Clone, Debug, PartialEq, JsonSchema)]
-#[serde(rename_all = "lowercase", from = "String")]
+#[schemars(from = "String")]
 pub enum RecipeType {
     /// path to a locally defined recipe
     File(String),
diff --git a/crates/icp/src/package.rs b/crates/icp/src/package.rs
@@ -108,7 +108,7 @@ pub fn cache_prebuilt(
 /// Read a cached recipe template by recipe and version (e.g., `@dfinity/rust`, `v1.0.2`).
 /// Resolves the version to a git SHA via the package manifest, then reads
 /// the cached template from `recipes/{sha}/recipe.hbs`.
-pub fn read_cached_recipe(
+pub fn read_cached_registry_recipe(
     cache: LRead<&PackageCachePaths>,
     recipe: &str,
     version: &str,
@@ -119,7 +119,35 @@ pub fn read_cached_recipe(
     else {
         return Ok(None);
     };
-    let cache_path = cache.recipe_sha(&sha);
+    read_cached_recipe(cache, &sha)
+}
+
+pub fn read_cached_uri_recipe(
+    cache: LRead<&PackageCachePaths>,
+    uri: &str,
+    sha2: Option<&str>,
+) -> Result<Option<Vec<u8>>, RecipeCacheError> {
+    assert!(uri.starts_with("http://") || uri.starts_with("https://"));
+    let Some(latest) =
+        get_tag(cache, &format!("uri+{uri}"), "latest").context(LoadRecipeTagSnafu)?
+    else {
+        return Ok(None);
+    };
+    if let Some(sha2) = sha2
+        && sha2 != latest
+    {
+        // the content at this URL has clearly changed, but we may have a cached recipe for the old version
+        read_cached_recipe(cache, sha2)
+    } else {
+        read_cached_recipe(cache, &latest)
+    }
+}
+
+pub fn read_cached_recipe(
+    cache: LRead<&PackageCachePaths>,
+    cache_key: &str,
+) -> Result<Option<Vec<u8>>, RecipeCacheError> {
+    let cache_path = cache.recipe_sha(cache_key);
     let template_path = cache_path.template();
     if template_path.exists() {
         let template = crate::fs::read(&template_path).context(RecipeCacheIoSnafu)?;
@@ -131,19 +159,40 @@ pub fn read_cached_recipe(
 }
 
 /// Cache a recipe template. `recipe` is the registry-qualified name (e.g., `@dfinity/rust`),
-/// `version` is the recipe version (e.g., `v1.0.2`), and `sha` is the git commit SHA that
-/// the version resolves to. Stores the version→SHA mapping in the package manifest and
-/// writes the template to `recipes/{sha}/recipe.hbs`.
-pub fn cache_recipe(
+/// `version` is the recipe version (e.g., `v1.0.2`), and `sha2` is the hash of the recipe
+/// (not the git commit sha). Stores the version→SHA mapping in the package manifest and
+/// writes the template to `recipes/{sha2}/recipe.hbs`.
+pub fn cache_registry_recipe(
     cache: LWrite<&PackageCachePaths>,
     recipe: &str,
     version: &str,
-    sha: &str,
+    sha2: &str,
     template: &[u8],
 ) -> Result<(), RecipeCacheError> {
     assert!(recipe.starts_with('@'));
-    set_tag(cache, &format!("recipe{recipe}"), sha, version).context(SaveRecipeTagSnafu)?;
-    let cache_path = cache.recipe_sha(sha);
+    set_tag(cache, &format!("recipe{recipe}"), sha2, version).context(SaveRecipeTagSnafu)?;
+    cache_recipe(cache, sha2, template)
+}
+
+/// Cache a recipe template from a URL. Stores the version→SHA mapping in the package manifest
+/// and writes the template to `recipes/{sha2}/recipe.hbs`.
+pub fn cache_uri_recipe(
+    cache: LWrite<&PackageCachePaths>,
+    uri: &str,
+    sha2: &str,
+    template: &[u8],
+) -> Result<(), RecipeCacheError> {
+    assert!(uri.starts_with("http://") || uri.starts_with("https://"));
+    set_tag(cache, &format!("uri+{uri}"), sha2, "latest").context(SaveRecipeTagSnafu)?;
+    cache_recipe(cache, sha2, template)
+}
+
+pub fn cache_recipe(
+    cache: LWrite<&PackageCachePaths>,
+    cache_key: &str,
+    template: &[u8],
+) -> Result<(), RecipeCacheError> {
+    let cache_path = cache.recipe_sha(cache_key);
     let template_path = cache_path.template();
     if !template_path.exists() {
         crate::fs::create_dir_all(cache_path.dir()).context(RecipeCacheIoSnafu)?;
