From a2cef209eadc9807a398aff0c411b8fb1d407d96 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Tue, 11 Nov 2025 22:51:08 +0100 Subject: [PATCH] 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) --- CHANGELOG.md | 6 ++ Cargo.toml | 2 +- src/api/data_types/chunking/build.rs | 4 +- src/commands/build/upload.rs | 56 ++++++++++----- src/utils/releases.rs | 2 +- src/utils/vcs.rs | 68 +++++++++++-------- .../build/build-invalid-base-sha.trycmd | 8 +++ .../build/build-invalid-head-sha.trycmd | 8 +++ .../_cases/build/build-upload-apk.trycmd | 2 +- .../build/build-upload-empty-shas.trycmd | 8 +++ .../_cases/build/build-upload-ipa.trycmd | 2 +- tests/integration/build/upload.rs | 43 ++++++++++++ 12 files changed, 157 insertions(+), 52 deletions(-) create mode 100644 tests/integration/_cases/build/build-invalid-base-sha.trycmd create mode 100644 tests/integration/_cases/build/build-invalid-head-sha.trycmd create mode 100644 tests/integration/_cases/build/build-upload-empty-shas.trycmd diff --git a/CHANGELOG.md b/CHANGELOG.md index b3103b631c..1afdc592ff 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Cargo.toml b/Cargo.toml index dcfdd0170f..c6d5f2d82c 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/api/data_types/chunking/build.rs b/src/api/data_types/chunking/build.rs index e46091515e..aad4521737 100644 --- a/src/api/data_types/chunking/build.rs +++ b/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, #[serde(skip_serializing_if = "Option::is_none")] - pub base_sha: Option<&'a str>, + pub base_sha: Option, #[serde(skip_serializing_if = "Option::is_none", rename = "provider")] pub vcs_provider: Option<&'a str>, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/src/commands/build/upload.rs b/src/commands/build/upload.rs index 5e47a99242..c1131bc434 100644 --- a/src/commands/build/upload.rs +++ b/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::>("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::("base_sha").is_some(); + let base_sha_from_user = matches.get_one::>("base_sha").is_some(); let base_ref_from_user = matches.get_one::("base_ref").is_some(); let mut base_sha = matches - .get_one("base_sha") - .map(String::as_str) - .map(Cow::Borrowed) + .get_one::>("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> { + 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 { diff --git a/src/utils/releases.rs b/src/utils/releases.rs index 6d062ab9d2..42c5c95367 100644 --- a/src/utils/releases.rs +++ b/src/utils/releases.rs @@ -160,7 +160,7 @@ pub fn detect_release_name() -> Result { // 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, \ diff --git a/src/utils/vcs.rs b/src/utils/vcs.rs index f745e02b24..923165249b 100644 --- a/src/utils/vcs.rs +++ b/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 { +pub fn find_head_sha() -> Result { 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 { 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> { +pub fn find_base_sha(remote_name: &str) -> Result> { 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,7 +592,11 @@ pub fn find_base_sha(remote_name: &str) -> Result> { 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}"))) } @@ -595,7 +604,7 @@ pub fn find_base_sha(remote_name: &str) -> Result> { /// 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 { +fn extract_pr_head_sha_from_event(json_content: &str) -> Option { 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 { 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 { +fn extract_pr_base_sha_from_event(json_content: &str) -> Option { 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 { 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 diff --git a/tests/integration/_cases/build/build-invalid-base-sha.trycmd b/tests/integration/_cases/build/build-invalid-base-sha.trycmd new file mode 100644 index 0000000000..061110fbed --- /dev/null +++ b/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 ': test_base_sha is not a valid SHA1 digest + +For more information, try '--help'. + +``` diff --git a/tests/integration/_cases/build/build-invalid-head-sha.trycmd b/tests/integration/_cases/build/build-invalid-head-sha.trycmd new file mode 100644 index 0000000000..7014468784 --- /dev/null +++ b/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 ': test_head_sha is not a valid SHA1 digest + +For more information, try '--help'. + +``` diff --git a/tests/integration/_cases/build/build-upload-apk.trycmd b/tests/integration/_cases/build/build-upload-apk.trycmd index a31bfc1bd1..71ca23fbb3 100644 --- a/tests/integration/_cases/build/build-upload-apk.trycmd +++ b/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 [..] diff --git a/tests/integration/_cases/build/build-upload-empty-shas.trycmd b/tests/integration/_cases/build/build-upload-empty-shas.trycmd new file mode 100644 index 0000000000..bfff33a04f --- /dev/null +++ b/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) + +``` diff --git a/tests/integration/_cases/build/build-upload-ipa.trycmd b/tests/integration/_cases/build/build-upload-ipa.trycmd index 73b6656e52..f47481bf67 100644 --- a/tests/integration/_cases/build/build-upload-ipa.trycmd +++ b/tests/integration/_cases/build/build-upload-ipa.trycmd @@ -1,5 +1,5 @@ ``` -$ sentry-cli build upload tests/integration/_fixtures/build/ipa.ipa --head-sha test_head_sha +$ sentry-cli build upload tests/integration/_fixtures/build/ipa.ipa --head-sha deadbeef12345678deadbeef12345678deadbeef ? success > Preparing for upload completed in [..] Successfully uploaded 1 file to Sentry diff --git a/tests/integration/build/upload.rs b/tests/integration/build/upload.rs index 17f3642974..8dad5a29c6 100644 --- a/tests/integration/build/upload.rs +++ b/tests/integration/build/upload.rs @@ -92,6 +92,11 @@ fn command_build_upload_apk_all_uploaded() { .with_default_token(); } +#[test] +fn command_build_upload_apk_invlid_sha() { + TestManager::new().register_trycmd_test("build/build-invalid-*-sha.trycmd"); +} + /// This regex is used to extract the boundary from the content-type header. /// We need to match the boundary, since it changes with each request. /// The regex matches the format as specified in @@ -235,3 +240,41 @@ fn command_build_upload_ipa_chunked() { .register_trycmd_test("build/build-upload-ipa.trycmd") .with_default_token(); } + +#[test] +fn command_build_upload_empty_shas() { + TestManager::new() + .mock_endpoint( + MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/") + .with_response_file("build/get-chunk-upload.json"), + ) + .mock_endpoint( + MockEndpointBuilder::new( + "POST", + "/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/", + ) + .with_response_fn(move |req| { + let body = req.body().expect("body should be readable"); + let json: serde_json::Value = + serde_json::from_slice(body).expect("body should be valid JSON"); + assert!( + json.get("head_sha").is_none(), + "head_sha should not be present" + ); + assert!( + json.get("base_sha").is_none(), + "base_sha should not be present" + ); + + serde_json::json!({ + "state": "created", + "missingChunks": [], + "artifactUrl": "http://sentry.io/wat-org/preprod/wat-project/42" + }) + .to_string() + .into() + }), + ) + .register_trycmd_test("build/build-upload-empty-shas.trycmd") + .with_default_token(); +}