From 50d2a1d7a1fd48b8fed27794c4fb091a2155cdab Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Sat, 1 Aug 2026 05:21:54 +0900 Subject: [PATCH] fix(aqua): fetch the checksum manifest with get_text again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #11552 converted `fetch_checksum_from_file` from `get_text` to `get_bytes` + `file::decode_text`, on the reasoning that `get_text` "decodes with the response charset and silently replaces anything it cannot map — a UTF-16 checksum file would be recorded as mojibake instead of failing". That reasoning is wrong. reqwest's `text()` delegates to `text_with_charset("utf-8")` when the `charset` feature is enabled, which mise enables, and that calls `encoding_rs::Encoding::decode`: let (encoding, without_bom) = match Encoding::for_bom(bytes) { Some((encoding, bom_length)) => (encoding, &bytes[bom_length..]), None => (self, bytes), }; A BOM overrides the declared encoding and is stripped, so this path already handled UTF-16 correctly and the conversion fixed nothing. It also cost something. `get_text` goes through `TextRequest::send`, which detects a `` body and re-sends over https when the URL was plain http; `get_bytes` has no such branch, so an http checksum URL answered with an HTML error page would be parsed as a checksum instead of retried. Registry checksum URLs are https, so the exposure was small, but it was behaviour given up for no gain. (Retries were never lost — both paths run through `send_with_https_fallback_with_retries` with `http_retries()`.) The two file-reading sites from #11552 are unaffected and stay: `std::fs` really does reject non-UTF-8, which is the failure in #5399's stack trace. `file::read_to_string_bom` now documents that asymmetry so the HTTP path does not get "fixed" again. --- src/backend/aqua.rs | 70 +++++++++++++++++++++++++++++++++++---------- src/file.rs | 6 ++++ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/backend/aqua.rs b/src/backend/aqua.rs index 05b28b1482d..de3764a25b1 100644 --- a/src/backend/aqua.rs +++ b/src/backend/aqua.rs @@ -2161,8 +2161,8 @@ impl AquaBackend { let url = match checksum_config._type() { AquaChecksumType::GithubRelease => { let asset_strs = checksum_config.asset_strs(pkg, v, target_os, target_arch)?; - // Only the bytes are read here (no filename derivation), so the transport - // URL is all that is needed. + // No filename derivation is needed here, so the transport URL is all that + // is needed. match self.github_release_asset_urls(pkg, v, asset_strs).await { Ok((_, download_url)) => download_url, Err(e) => { @@ -2174,21 +2174,14 @@ impl AquaBackend { AquaChecksumType::Http => checksum_config.url(pkg, v, target_os, target_arch)?, }; - // Download checksum file content. Read the bytes rather than `get_text`, which decodes with - // the response charset and silently replaces anything it cannot map — a UTF-16 checksum - // file would be recorded as mojibake instead of failing. `decode_text` honours a BOM, the - // same as the install path above. - let checksum_bytes = match HTTP.get_bytes(&url).await { - Ok(bytes) => bytes, - Err(e) => { - debug!("Failed to download checksum file {}: {}", url, e); - return Ok(None); - } - }; - let checksum_content = match file::decode_text(checksum_bytes.as_ref()) { + // Download checksum file content. `get_text` decodes with BOM sniffing (reqwest's + // `text_with_charset` -> `encoding_rs::Encoding::decode`), so a UTF-16 checksum file is + // handled here without help — unlike the install path above, which reads from disk. It + // also detects an HTML body and retries over https, which reading bytes would skip. + let checksum_content = match HTTP.get_text(&url).await { Ok(content) => content, Err(e) => { - debug!("Failed to decode checksum file {}: {}", url, e); + debug!("Failed to download checksum file {}: {}", url, e); return Ok(None); } }; @@ -4785,6 +4778,53 @@ mod lock_candidate_tests { assert!(err.contains("does not match expected checksum"), "{err}"); } + /// The counterpart over HTTP, which needs no decoding of its own: reqwest's `text()` goes + /// through `text_with_charset` -> `encoding_rs::Encoding::decode`, which sniffs the BOM and + /// strips it. Pinned here because that is a property of the *transport*, easy to lose by + /// swapping `get_text` for a byte-level read on the assumption it needs the same help the + /// file path does. + #[tokio::test] + async fn test_fetch_checksum_from_file_decodes_a_utf16_body() { + const ARTIFACT: &str = "tool-1.0.0-linux-amd64.tar.gz"; + const DIGEST: &str = "d91b9172668f4b6aef4abce8c780cd298872c7a0f4487cc47444d26877ba49f6"; + + let mut body = vec![0xff, 0xfe]; + body.extend( + format!("{DIGEST} *{ARTIFACT}\n") + .encode_utf16() + .flat_map(u16::to_le_bytes), + ); + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("GET", "/hashes.sha256") + .with_status(200) + .with_body(body) + .expect(1) + .create_async() + .await; + + let mut pkg = AquaPackage::default(); + pkg.checksum = Some( + serde_json::from_str(&format!( + r#"{{"type":"http","algorithm":"sha256","url":"{}/hashes.sha256"}}"#, + server.url() + )) + .unwrap(), + ); + let backend = AquaBackend::from_arg(BackendArg::new( + "tool".to_string(), + Some("aqua:owner/repo".to_string()), + )); + + let checksum = backend + .fetch_checksum_from_file(&pkg, "1.0.0", "linux", "amd64", Some(ARTIFACT)) + .await + .unwrap(); + + mock.assert_async().await; + assert_eq!(checksum, Some(format!("sha256:{DIGEST}"))); + } + #[test] fn test_lock_candidates_no_tag_with_version_prefix() { let (v, candidates) = build_lock_candidates("1.7.1", None, Some("jq-")); diff --git a/src/file.rs b/src/file.rs index 206dbd92437..37db0abc2b7 100644 --- a/src/file.rs +++ b/src/file.rs @@ -374,6 +374,12 @@ pub fn decode_text(bytes: &[u8]) -> Result { } /// [`read_to_string`], but tolerant of a byte-order mark. See [`decode_text`]. +/// +/// Only reads *from disk* need this. Bodies fetched over HTTP already arrive decoded: reqwest's +/// `text()` goes through `text_with_charset` -> `encoding_rs::Encoding::decode`, which sniffs a BOM +/// and lets it override the declared charset. `std::fs::read_to_string` has no such step, and that +/// asymmetry is the only reason this function exists — reaching for it on an `HTTP.get_text` result +/// would be redundant. pub fn read_to_string_bom>(path: P) -> Result { let path = path.as_ref(); trace!("cat {}", path.display_user());