Repository navigation
fix(aqua): read checksum files that carry a byte-order mark - #11552
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds BOM-aware decoding for UTF-8, UTF-16LE, and UTF-16BE text. Aqua checksum verification and fetching use this decoder. Tests cover valid encodings, invalid input, and mismatched digests. ChangesChecksum decoding
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds BOM-aware checksum decoding for Aqua.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(aqua): read checksum files that carr..." | Re-trigger Greptile |
`std::fs::read_to_string` rejects any file that is not UTF-8, so a UTF-16 checksum file aborted the install (jdx#5399): 1: failed read_to_string: .../powershell-7.4.10-linux-x64.tar.gz.checksum 2: stream did not contain valid UTF-8 PowerShell's release tooling regressed and published `hashes.sha256` as UTF-16LE (PowerShell/PowerShell#25693). The workaround was aquaproj/aqua-registry#38202, which disabled checksum verification for the affected range — so the standing cost of not reading UTF-16 is that verification is still switched off for those versions, not a crash users hit today. Fetching the raw bytes shows the registry boundary is exactly an encoding boundary: v7.4.7 starts `61 32 39 37`, v7.4.10 starts `FF FE 36 00`, v7.5.0 is UTF-8 again. It is not PowerShell-specific either — Windows PowerShell 5.1's `Out-File` writes UTF-16LE by default. Adds `file::decode_text` and `file::read_to_string_bom`, and uses them at the three aqua sites that feed `parse_checksum_from_content`: the install-path verify, the lockfile checksum binding, and `fetch_checksum_from_file` (which switches from `get_text` to `get_bytes`, since `get_text` decodes with the response charset and replaces what it cannot map rather than failing). Only a BOM selects the encoding. Detecting UTF-16 without one means guessing from the density of NUL bytes, which can misfire on binary input, and `Out-File` always writes a BOM. Input with no BOM is decoded as UTF-8 exactly as before, so `file::read_to_string` itself is left alone — it has over a hundred call sites, config files and plugin scripts among them. Tested at both levels: the decoder against UTF-16LE/BE, a UTF-8 BOM, plain UTF-8, an odd trailing byte and an unpaired surrogate; and end to end against the real line for the artifact in jdx#5399, taken from the v7.4.10 file that broke it, with a deliberately wrong expectation to prove the file is really parsed.
2d56687 to
c2286e6
Compare
jdx#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 `<!DOCTYPE html>` 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 jdx#11552 are unaffected and stay: `std::fs` really does reject non-UTF-8, which is the failure in jdx#5399's stack trace. `file::read_to_string_bom` now documents that asymmetry so the HTTP path does not get "fixed" again.
On Windows a shebang is what makes an extensionless file a task, and `has_shebang` read exactly two bytes -- so a script carrying a UTF-8 BOM showed `EF BB` and was ruled not executable. The task vanished from `mise tasks` with no diagnostic at any verbosity, while the same file on unix was a task because there the execute bit decides. Windows PowerShell 5.1 writes that BOM from `Out-File -Encoding utf8` and `Set-Content -Encoding UTF8`, so a task written at the prompt acquires one without its author choosing it. mise already reads a marked `mise.toml` fine, and jdx#11552 taught `decode_text` about the same writer. Also skip the mark when reading the shebang for shell dispatch (a marked script otherwise fell back to `cmd /c`) and before scanning `#MISE` and `#USAGE` headers, which anchor at the start of a line and so lost a directive written on line 1.
On Windows a shebang is what makes an extensionless file a task, and `has_shebang` read exactly two bytes -- so a script carrying a UTF-8 BOM showed `EF BB` and was ruled not executable. The task vanished from `mise tasks` with no diagnostic at any verbosity, while the same file on unix was a task because there the execute bit decides. Windows PowerShell 5.1 writes that BOM from `Out-File -Encoding utf8` and `Set-Content -Encoding UTF8`, so a task written at the prompt acquires one without its author choosing it. mise already reads a marked `mise.toml` fine, and jdx#11552 taught `decode_text` about the same writer. Also skip the mark when reading the shebang for shell dispatch (a marked script otherwise fell back to `cmd /c`) and before scanning `#MISE` and `#USAGE` headers, which anchor at the start of a line and so lost a directive written on line 1.
From #5399.
std::fs::read_to_stringrejects any file that is not UTF-8, so a UTF-16 checksum file aborts the install:What this actually costs today
I want to be precise rather than oversell it: nobody is hitting that crash right now, and the reason is the point.
PowerShell's release tooling regressed and published
hashes.sha256as UTF-16LE (PowerShell/PowerShell#25693). The fix was aquaproj/aqua-registry#38202, which disabled checksum verification for the affected range. So the standing cost of mise not reading UTF-16 is that verification stays switched off for a whole band of releases.Fetching the raw bytes from GitHub shows the registry's version boundary is exactly an encoding boundary:
61 32 39 37 33 65 …FF FE 36 00 64 00 …35 38 31 33 34 31 …which lines up with the vendored registry:
semver("<= 7.4.7")keepsasset: hashes.sha256, and everything above it has nochecksumblock at all.It is not PowerShell-specific either — Windows PowerShell 5.1's
Out-Filewrites UTF-16LE by default, so this recurs for anything generating checksums that way.The change
file::decode_text+file::read_to_string_bom, used at the three aqua sites that feedparse_checksum_from_content:verify_checksum_file_matches_expected— the install path, and the frame in powershell-core install in Fedora (WSL) fails #5399's stackfetch_checksum_from_file— switchesget_text→get_bytes+decode_text.get_textdecodes with the response charset and replaces what it cannot map instead of failing, so that path could not fail the way the file path does; it would record whatever the lossy decode produced. I have not reproduced that — converting it just makes all three agree.Only a BOM selects the encoding. Detecting UTF-16 without one means guessing from the density of NUL bytes, which can misfire on binary input, and
Out-Filealways writes a BOM, so the guess buys nothing. Input with no BOM is decoded as UTF-8 exactly as before — it just gets an error message naming the problem instead of std's bare "stream did not contain valid UTF-8".encoding_rsis only a transitive dep (via reqwest'scharset), and BOM'd UTF-16 is ~25 lines ofstd, so no new direct dependency.Deliberately not done
file::read_to_stringitself is untouched. Over a hundred call sites, config files and plugin scripts among them; that is a much bigger behaviour change than this bug warrants. Happy to widen it if you'd rather have it everywhere.http.rs,pkgx.rs,static_helpers.rs, all viaget_text). Same defect class, left out to keep this reviewable —decode_textis public so they can adopt it.Tests
Decoder: UTF-16LE, UTF-16BE, a UTF-8 BOM, plain UTF-8 and empty input unchanged, an odd trailing byte, and an unpaired surrogate.
End to end: the real line for the artifact in #5399, taken from the v7.4.10
hashes.sha256that broke it —— written as UTF-16LE with BOM, verified against the correct expectation and then against a wrong one, so the passing case cannot be a file that was silently never parsed.
Verification
No local
cargorun;rustfmt --edition 2024only.lint(clippy-D warnings), linuxcargo test,unit-macosandwindows-unitall cover these files.Summary by CodeRabbit