ref(build): Add client-side validation for SHA fields (#2945)

### Description
Add client-side validation for SHA fields. Also, don't send these fields' values if the user overrides them with an empty string like `--head-sha ""`.

### Issues
- Ref #2941
- Ref [CLI-224](https://linear.app/getsentry/issue/CLI-224/skip-serializing-certain-fields-for-build-upload)

Author: szokeasaurusrex

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Improvements
+
+- Added validation for the `sentry-cli build upload` command's `--head-sha` and `--base-sha` arguments ([#2945](https://github.com/getsentry/sentry-cli/pull/2945)). The CLI now validates that these are valid SHA1 sums. Passing an empty string is also allowed; this prevents the default values from being used, causing the values to instead be unset.
+
 ## 2.58.1
 
 ### Deprecations

Cargo.toml

@@ -62,7 +62,7 @@ sentry = { version = "0.34.0", default-features = false, features = [
 ] }
 serde = { version = "1.0.152", features = ["derive"] }
 serde_json = "1.0.93"
-sha1_smol = { version = "1.0.0", features = ["serde"] }
+sha1_smol = { version = "1.0.0", features = ["serde", "std"] }
 sourcemap = { version = "9.2.0", features = ["ram_bundle"] }
 symbolic = { version = "12.13.3", features = ["debuginfo-serde", "il2cpp"] }
 thiserror = "1.0.38"

src/api/data_types/chunking/build.rs

@@ -28,9 +28,9 @@ pub struct AssembleBuildResponse {
 #[derive(Debug, Serialize)]
 pub struct VcsInfo<'a> {
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub head_sha: Option<&'a str>,
+    pub head_sha: Option<Digest>,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub base_sha: Option<&'a str>,
+    pub base_sha: Option<Digest>,
     #[serde(skip_serializing_if = "Option::is_none", rename = "provider")]
     pub vcs_provider: Option<&'a str>,
     #[serde(skip_serializing_if = "Option::is_none")]

src/commands/build/upload.rs

@@ -6,6 +6,7 @@ use anyhow::{anyhow, bail, Context as _, Result};
 use clap::{Arg, ArgAction, ArgMatches, Command};
 use indicatif::ProgressStyle;
 use log::{debug, info, warn};
+use sha1_smol::Digest;
 use symbolic::common::ByteView;
 use zip::write::SimpleFileOptions;
 use zip::{DateTime, ZipWriter};
@@ -53,11 +54,13 @@ pub fn make_command(command: Command) -> Command {
         .arg(
             Arg::new("head_sha")
                 .long("head-sha")
+                .value_parser(parse_sha_allow_empty)
                 .help("The VCS commit sha to use for the upload. If not provided, the current commit sha will be used.")
         )
         .arg(
             Arg::new("base_sha")
                 .long("base-sha")
+                .value_parser(parse_sha_allow_empty)
                 .help("The VCS commit's base sha to use for the upload. If not provided, the merge-base of the current and remote branch will be used.")
         )
         .arg(
@@ -112,10 +115,10 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         .expect("paths argument is required");
 
     let head_sha = matches
-        .get_one("head_sha")
-        .map(String::as_str)
-        .map(Cow::Borrowed)
-        .or_else(|| vcs::find_head_sha().ok().map(Cow::Owned));
+        .get_one::<Option<Digest>>("head_sha")
+        .map(|d| d.as_ref().cloned())
+        .or_else(|| Some(vcs::find_head_sha().ok()))
+        .flatten();
 
     let cached_remote = config.get_cached_vcs_remote();
     // Try to open the git repository and find the remote, but handle errors gracefully.
@@ -232,20 +235,21 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
     };
 
     // Track whether base_sha and base_ref were explicitly provided by the user
-    let base_sha_from_user = matches.get_one::<String>("base_sha").is_some();
+    let base_sha_from_user = matches.get_one::<Option<Digest>>("base_sha").is_some();
     let base_ref_from_user = matches.get_one::<String>("base_ref").is_some();
 
     let mut base_sha = matches
-        .get_one("base_sha")
-        .map(String::as_str)
-        .map(Cow::Borrowed)
+        .get_one::<Option<Digest>>("base_sha")
+        .map(|d| d.as_ref().cloned())
         .or_else(|| {
-            vcs::find_base_sha(&cached_remote)
-                .inspect_err(|e| debug!("Error finding base SHA: {e}"))
-                .ok()
-                .flatten()
-                .map(Cow::Owned)
-        });
+            Some(
+                vcs::find_base_sha(&cached_remote)
+                    .inspect_err(|e| debug!("Error finding base SHA: {e}"))
+                    .ok()
+                    .flatten(),
+            )
+        })
+        .flatten();
 
     let mut base_ref = base_ref;
 
@@ -255,11 +259,11 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         && !base_ref_from_user
         && base_sha.is_some()
         && head_sha.is_some()
-        && base_sha.as_deref() == head_sha.as_deref()
+        && base_sha == head_sha
     {
         debug!(
             "Base SHA equals head SHA ({}), and both were auto-inferred. Skipping base_sha and base_ref, but keeping head_sha.",
-            base_sha.as_deref().expect("base_sha is Some at this point")
+            base_sha.expect("base_sha is Some at this point")
         );
         base_sha = None;
         base_ref = None;
@@ -325,8 +329,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
         info!("Uploading file: {}", path.display());
         let bytes = ByteView::open(zip.path())?;
         let vcs_info = VcsInfo {
-            head_sha: head_sha.as_deref(),
-            base_sha: base_sha.as_deref(),
+            head_sha,
+            base_sha,
             vcs_provider: vcs_provider.as_deref(),
             head_repo_name: head_repo_name.as_deref(),
             base_repo_name: base_repo_name.as_deref(),
@@ -598,6 +602,22 @@ fn upload_file(
     result
 }
 
+/// Utility function to parse a SHA1 digest, allowing empty strings.
+///
+/// Empty strings result in Ok(None), otherwise we return the parsed digest
+/// or an error if the SHA is invalid.
+fn parse_sha_allow_empty(sha: &str) -> Result<Option<Digest>> {
+    if sha.is_empty() {
+        return Ok(None);
+    }
+
+    let digest = sha
+        .parse()
+        .with_context(|| format!("{sha} is not a valid SHA1 digest"))?;
+
+    Ok(Some(digest))
+}
+
 #[cfg(not(windows))]
 #[cfg(test)]
 mod tests {

src/utils/releases.rs

@@ -160,7 +160,7 @@ pub fn detect_release_name() -> Result<String> {
 
     // finally try the git sha
     match vcs::find_head_sha() {
-        Ok(head) => Ok(head),
+        Ok(head) => Ok(head.to_string()),
         Err(e) => Err(anyhow!(
             "Could not automatically determine release name:\n\t {e} \n\n\
             Please ensure your version control system is configured correctly, \

src/utils/vcs.rs

@@ -11,6 +11,7 @@ use lazy_static::lazy_static;
 use log::{debug, info, warn};
 use regex::Regex;
 use serde_json::Value;
+use sha1_smol::Digest;
 
 use crate::api::{GitCommit, PatchSet, Ref, Repo};
 
@@ -550,7 +551,7 @@ fn find_matching_revs(
     Ok((prev_rev, rev))
 }
 
-pub fn find_head_sha() -> Result<String> {
+pub fn find_head_sha() -> Result<Digest> {
     if let Some(pr_head_sha) = std::env::var("GITHUB_EVENT_PATH")
         .ok()
         .and_then(|event_path| std::fs::read_to_string(event_path).ok())
@@ -562,10 +563,14 @@ pub fn find_head_sha() -> Result<String> {
 
     let repo = git2::Repository::open_from_env()?;
     let head = repo.revparse_single("HEAD")?;
-    Ok(head.id().to_string())
+    Ok(head
+        .id()
+        .to_string()
+        .parse()
+        .expect("Repo SHA should be a valid SHA1 digest"))
 }
 
-pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {
+pub fn find_base_sha(remote_name: &str) -> Result<Option<Digest>> {
     if let Some(pr_base_sha) = std::env::var("GITHUB_EVENT_PATH")
         .ok()
         .and_then(|event_path| std::fs::read_to_string(event_path).ok())
@@ -587,15 +592,19 @@ pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {
     Ok(remote_ref
         .peel_to_commit()
         .and_then(|remote_commit| repo.merge_base(head_commit.id(), remote_commit.id()))
-        .map(|oid| oid.to_string())
+        .map(|oid| {
+            oid.to_string()
+                .parse()
+                .expect("Repo SHA should be a valid SHA1 digest")
+        })
         .ok()
         .inspect(|sha| debug!("Found merge-base commit as base reference: {sha}")))
 }
 
 /// Extracts the PR head SHA from GitHub Actions event payload JSON.
 /// Returns None if not a PR event or if SHA cannot be extracted.
 /// Panics if json is malformed.
-fn extract_pr_head_sha_from_event(json_content: &str) -> Option<String> {
+fn extract_pr_head_sha_from_event(json_content: &str) -> Option<Digest> {
     let v: Value = match serde_json::from_str(json_content) {
         Ok(v) => v,
         Err(_) => {
@@ -605,13 +614,13 @@ fn extract_pr_head_sha_from_event(json_content: &str) -> Option<String> {
 
     v.pointer("/pull_request/head/sha")
         .and_then(|s| s.as_str())
-        .map(|s| s.to_owned())
+        .map(|s| s.parse().expect("GitHub Actions provided an invalid SHA"))
 }
 
 /// Extracts the PR base SHA from GitHub Actions event payload JSON.
 /// Returns None if not a PR event or if SHA cannot be extracted.
 /// Panics if json is malformed.
-fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {
+fn extract_pr_base_sha_from_event(json_content: &str) -> Option<Digest> {
     let v: Value = match serde_json::from_str(json_content) {
         Ok(v) => v,
         Err(_) => {
@@ -621,7 +630,7 @@ fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {
 
     v.pointer("/pull_request/base/sha")
         .and_then(|s| s.as_str())
-        .map(|s| s.to_owned())
+        .map(|s| s.parse().expect("GitHub Actions provided an invalid SHA"))
 }
 
 /// Given commit specs, repos and remote_name this returns a list of head
@@ -810,10 +819,7 @@ mod tests {
         crate::api::RepoProvider,
         insta::{assert_debug_snapshot, assert_yaml_snapshot},
         serial_test::serial,
-        std::fs::File,
-        std::io::Write as _,
-        std::path::Path,
-        std::process::Command,
+        std::{fs::File, io::Write as _, path::Path, process::Command},
         tempfile::{tempdir, TempDir},
     };
 
@@ -1600,7 +1606,7 @@ mod tests {
 
         assert_eq!(
             extract_pr_head_sha_from_event(&pr_json),
-            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba5".to_owned())
+            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba5".parse().unwrap())
         );
 
         let push_json = r#"{
@@ -1642,7 +1648,7 @@ mod tests {
 
         assert_eq!(
             extract_pr_head_sha_from_event(real_gh_json),
-            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba4".to_owned())
+            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba4".parse().unwrap())
         );
         let malicious_json = r#"{
   "action": "opened",
@@ -1658,20 +1664,23 @@ mod tests {
 
         assert_eq!(
             extract_pr_head_sha_from_event(malicious_json),
-            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba5".to_owned())
+            Some("19ef6adc4dbddf733db6e833e1f96fb056b6dba5".parse().unwrap())
         );
-        let any_sha_json = r#"{
-  "pull_request": {
-    "head": {
-      "sha": "invalid-sha-123"
     }
-  }
-}"#;
 
-        assert_eq!(
-            extract_pr_head_sha_from_event(any_sha_json),
-            Some("invalid-sha-123".to_owned())
-        );
+    #[test]
+    #[should_panic]
+    fn test_extract_pr_head_sha_from_event_invalid_sha() {
+        let any_sha_json = serde_json::json!({
+            "pull_request": {
+                "head": {
+                    "sha": "invalid-sha-123"
+                }
+            }
+        })
+        .to_string();
+
+        extract_pr_head_sha_from_event(&any_sha_json);
     }
 
     #[test]
@@ -1710,7 +1719,10 @@ mod tests {
         std::env::remove_var("GITHUB_EVENT_PATH");
 
         assert!(result.is_ok());
-        assert_eq!(result.unwrap(), "19ef6adc4dbddf733db6e833e1f96fb056b6dba5");
+        assert_eq!(
+            result.unwrap(),
+            "19ef6adc4dbddf733db6e833e1f96fb056b6dba5".parse().unwrap()
+        );
     }
 
     #[test]
@@ -1734,7 +1746,7 @@ mod tests {
 
         assert_eq!(
             extract_pr_base_sha_from_event(&pr_json),
-            Some("55e6bc8c264ce95164314275d805f477650c440d".to_owned())
+            Some("55e6bc8c264ce95164314275d805f477650c440d".parse().unwrap())
         );
 
         // Test with push event (should return None)
@@ -1790,7 +1802,7 @@ mod tests {
         let result = find_base_sha("origin");
         assert_eq!(
             result.unwrap().unwrap(),
-            "55e6bc8c264ce95164314275d805f477650c440d"
+            "55e6bc8c264ce95164314275d805f477650c440d".parse().unwrap()
         );
 
         // Test without GITHUB_EVENT_PATH

tests/integration/_cases/build/build-invalid-base-sha.trycmd

@@ -0,0 +1,8 @@
+```
+$ sentry-cli build upload tests/integration/_fixtures/build/apk.apk --base-sha test_base_sha
+? failed
+error: invalid value 'test_base_sha' for '--base-sha <base_sha>': test_base_sha is not a valid SHA1 digest
+
+For more information, try '--help'.
+
+```

tests/integration/_cases/build/build-invalid-head-sha.trycmd

@@ -0,0 +1,8 @@
+```
+$ sentry-cli build upload tests/integration/_fixtures/build/apk.apk --head-sha test_head_sha
+? failed
+error: invalid value 'test_head_sha' for '--head-sha <head_sha>': test_head_sha is not a valid SHA1 digest
+
+For more information, try '--help'.
+
+```

tests/integration/_cases/build/build-upload-apk.trycmd

@@ -1,5 +1,5 @@
 ```
-$ sentry-cli build upload tests/integration/_fixtures/build/apk.apk --head-sha test_head_sha
+$ sentry-cli build upload tests/integration/_fixtures/build/apk.apk --head-sha 12345678deadbeef78900987feebdaed87654321
 ? success
 > Preparing for upload completed in [..]
 > Uploading completed in [..]

tests/integration/_cases/build/build-upload-empty-shas.trycmd

@@ -0,0 +1,8 @@
+```
+$ sentry-cli build upload tests/integration/_fixtures/build/apk.apk --head-sha "" --base-sha ""
+? success
+> Preparing for upload completed in [..]
+Successfully uploaded 1 file to Sentry
+  - tests/integration/_fixtures/build/apk.apk (http://sentry.io/wat-org/preprod/wat-project/42)
+
+```