diff --git a/rust/domains-client/Cargo.toml b/rust/domains-client/Cargo.toml index 98c1e89..269ca4c 100644 --- a/rust/domains-client/Cargo.toml +++ b/rust/domains-client/Cargo.toml @@ -34,4 +34,4 @@ syn = "2" [dev-dependencies] httpmock = "0.7" -tokio = { version = "1", features = ["macros", "rt-multi-thread"] } +tokio = { version = "1", features = ["macros", "rt-multi-thread", "sync"] } diff --git a/rust/domains-client/src/lib.rs b/rust/domains-client/src/lib.rs index 29ea78e..15ca0e8 100644 --- a/rust/domains-client/src/lib.rs +++ b/rust/domains-client/src/lib.rs @@ -662,9 +662,11 @@ mod tests { } } - // Serializes tests that mutate the process-wide default transport logger, - // mirroring cli-engine's own (crate-private) test lock. - static TRANSPORT_LOGGER_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + // Serializes tests that mutate the process-wide default transport logger. + // An async-aware lock, not a `std::sync::Mutex` — the guard is held across + // this test's `.await` points (clippy::await_holding_lock), which is only + // sound with a lock that yields the executor instead of blocking a thread. + static TRANSPORT_LOGGER_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); // Restores the noop logger on drop so a panicking assertion below can't // leak a test's logger into later tests in this binary. Declared after @@ -688,9 +690,7 @@ mod tests { // that the request/response round-trips. #[tokio::test] async fn client_hooks_feed_the_debug_transport_bridge() { - let _test_lock = TRANSPORT_LOGGER_TEST_LOCK - .lock() - .expect("lock is never held across a panic"); + let _test_lock = TRANSPORT_LOGGER_TEST_LOCK.lock().await; let _restore = RestoreDefaultTransportLogger; let logger = std::sync::Arc::new(RecordingLogger::default()); diff --git a/rust/src/update/mod.rs b/rust/src/update/mod.rs index c1df40d..4d6315c 100644 --- a/rust/src/update/mod.rs +++ b/rust/src/update/mod.rs @@ -62,11 +62,6 @@ struct UpdateCache { tag_name: String, } -#[derive(Deserialize)] -struct GithubRelease { - tag_name: String, -} - pub fn module() -> Module { Module::new("Admin", |_ctx| { RuntimeGroupSpec::new( @@ -134,12 +129,27 @@ fn apply_command() -> RuntimeCommandSpec { .mutates(true) .no_auth(true) .with_output_schema::() - .with_default_fields("previousVersion,newVersion,status"), - |_cred, _args| async move { Ok(CommandResult::new(run_apply().await?)) }, + .with_default_fields("previousVersion,newVersion,status") + .with_arg( + clap::Arg::new("force") + .long("force") + .action(clap::ArgAction::SetTrue) + .help( + "Reinstall the latest release even if it matches the running \ + version (useful for validating the update mechanism itself)", + ), + ), + |_cred, args| async move { + let force = args + .get("force") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + Ok(CommandResult::new(run_apply(force).await?)) + }, ) } -async fn run_apply() -> Result { +async fn run_apply(force: bool) -> Result { let current = current_version(); let client = http_client()?; let cache = refresh_cache(&client, FOREGROUND_TIMEOUT).await?; @@ -149,7 +159,11 @@ async fn run_apply() -> Result { cache.latest_version )) })?; - if latest <= current { + // `--force` only bypasses the "already up to date" case (`latest == + // current`) — it must never let a stale/dev build "update" to an older + // published release, so a genuine downgrade (`latest < current`) is + // always blocked regardless of `force`. + if latest < current || (latest == current && !force) { return Ok(json!({ "previousVersion": current.to_string(), "newVersion": current.to_string(), @@ -285,43 +299,72 @@ fn http_client() -> Result { .map_err(|e| CliCoreError::message(format!("failed to build HTTP client: {e}"))) } -async fn fetch_latest_release( +/// Resolves the latest release tag via `github.com/{REPO}/releases/latest`'s +/// redirect to `.../releases/tag/`, rather than the `api.github.com` +/// REST endpoint — the REST API enforces a 60-requests/hour *unauthenticated* +/// limit per IP (shared, e.g. behind a corporate NAT, exhausted trivially), +/// while this plain web redirect isn't API-rate-limited at all. `install.sh` +/// already relies on the same redirect (via `releases/latest/download/...`) +/// for the same reason. +async fn fetch_latest_tag( + client: &reqwest::Client, + timeout: Duration, +) -> Result { + fetch_latest_tag_from( + client, + &format!("https://github.com/{REPO}/releases/latest"), + timeout, + ) + .await +} + +/// Testable core of [`fetch_latest_tag`] — takes the `/releases/latest` URL +/// as a parameter so tests can point it at a mock server. +async fn fetch_latest_tag_from( client: &reqwest::Client, + url: &str, timeout: Duration, -) -> Result { - let url = format!("https://api.github.com/repos/{REPO}/releases/latest"); +) -> Result { let resp = client - .get(&url) + .head(url) .timeout(timeout) .send() .await .map_err(|e| CliCoreError::message(format!("failed to check for updates: {e}")))?; if !resp.status().is_success() { return Err(CliCoreError::message(format!( - "GitHub API returned {} while checking for updates", + "GitHub returned {} while checking for updates", resp.status() ))); } - resp.json::() - .await - .map_err(|e| CliCoreError::message(format!("failed to parse GitHub release response: {e}"))) + // Use the resolved (post-redirect) URL in the error below, not the + // constant `.../releases/latest` request URL — that's what actually + // failed to parse and is what's needed to diagnose the failure. + let resolved_url = resp.url().clone(); + resolved_url + .path_segments() + .and_then(Iterator::last) + .filter(|s| !s.is_empty()) + .map(str::to_owned) + .ok_or_else(|| { + CliCoreError::message(format!( + "could not determine latest release tag from resolved URL {resolved_url}" + )) + }) } async fn refresh_cache( client: &reqwest::Client, timeout: Duration, ) -> Result { - let release = fetch_latest_release(client, timeout).await?; - let version = parse_version(&release.tag_name).ok_or_else(|| { - CliCoreError::message(format!( - "unexpected tag format from GitHub: {}", - release.tag_name - )) + let tag_name = fetch_latest_tag(client, timeout).await?; + let version = parse_version(&tag_name).ok_or_else(|| { + CliCoreError::message(format!("unexpected tag format from GitHub: {tag_name}")) })?; let cache = UpdateCache { checked_at: chrono::Utc::now().to_rfc3339(), latest_version: version.to_string(), - tag_name: release.tag_name, + tag_name, }; // Caching is advisory (only the passive notice depends on it) — a write // failure (e.g. read-only config dir) shouldn't fail the actual check. @@ -479,6 +522,9 @@ fn write_temp_binary(bytes: &[u8]) -> Result { #[cfg(test)] mod tests { + use httpmock::Method::HEAD; + use httpmock::prelude::*; + use super::*; #[test] @@ -613,6 +659,67 @@ def456 gddy-aarch64-unknown-linux-gnu.tar.gz assert!(extract_tar_gz(&gz_bytes, "not-gddy").is_err()); } + #[tokio::test] + async fn fetch_latest_tag_from_follows_redirect_to_release_tag() { + let server = MockServer::start_async().await; + let redirect_mock = server + .mock_async(|when, then| { + when.method(HEAD).path("/godaddy/cli/releases/latest"); + then.status(302) + .header("location", "/godaddy/cli/releases/tag/v1.2.3"); + }) + .await; + // reqwest follows the redirect above, so the tag page itself also + // needs to answer the (re-issued) HEAD request with success. + let tag_page_mock = server + .mock_async(|when, then| { + when.method(HEAD).path("/godaddy/cli/releases/tag/v1.2.3"); + then.status(200); + }) + .await; + + let tag = fetch_latest_tag_from( + &reqwest::Client::new(), + &format!("{}/godaddy/cli/releases/latest", server.base_url()), + Duration::from_secs(5), + ) + .await + .expect("resolves tag from redirect"); + + redirect_mock.assert_async().await; + tag_page_mock.assert_async().await; + assert_eq!(tag, "v1.2.3"); + } + + #[tokio::test] + async fn fetch_latest_tag_from_errors_with_resolved_url_when_redirect_has_no_tag_segment() { + let server = MockServer::start_async().await; + server + .mock_async(|when, then| { + when.method(HEAD).path("/empty"); + then.status(302).header("location", "/"); + }) + .await; + server + .mock_async(|when, then| { + when.method(HEAD).path("/"); + then.status(200); + }) + .await; + + let err = fetch_latest_tag_from( + &reqwest::Client::new(), + &format!("{}/empty", server.base_url()), + Duration::from_secs(5), + ) + .await + .expect_err("no path segment to parse a tag from"); + + let message = err.to_string(); + assert!(message.contains("resolved URL")); + assert!(message.contains(&server.base_url())); + } + #[test] fn extract_zip_finds_binary_by_exact_name() { let mut zip_bytes = Vec::new();