Fix issues from code review: shell-escaping gap, exit code, and more
All checks were successful
CI / build (pull_request) Successful in 51s
CI / test (pull_request) Successful in 3m35s
CI / audit (pull_request) Successful in 13s
CI / coverage (pull_request) Successful in 6m57s

A single-agent code review of this branch's diff (builder/pipeline/
publisher/sanity/hash + main/config/fetcher/state/verifier changes)
found six real issues, all fixed here:

- builder.rs: validate_shell_safe only rejected a literal single quote
  and newline, written for the single-quoted PKGBUILD fields. But
  asset_name (via install_source) and binary_name land in the install()
  line, which is necessarily double-quoted so ${srcdir}/${pkgdir} can
  expand — where $, backtick, and backslash are still live. Not
  currently exploitable (the one variable component, version, is already
  independently constrained by validate_pkgver's strict charset), but a
  latent gap relying on that coincidence rather than the validator
  actually covering its real use context. Widened the reject-list to
  cover both quoting styles, added regression tests including one at the
  generate_pkgbuild level. Corrected SPEC.md's "single-quoted" claim to
  match.
- pipeline.rs: a verification failure returned Ok(()) from
  process_package, so run_check never counted it as a failure and the
  process exited 0 even on a failed cryptographic/attestation check —
  exactly the event a monitoring setup (systemd OnFailure=, cron
  mail-on-error) most needs a non-zero exit to catch. Now bails, which
  run_check already treats as a package failure. Added an integration
  test against a mocked GitHub server exercising this exact path.
- builder.rs: find_built_package hardcoded the .pkg.tar.zst suffix, so a
  box with a different PKGEXT in makepkg.conf would report a false
  "makepkg failed" for a build that actually succeeded. Widened to match
  any .pkg.tar.* compression. Added direct unit tests (it had none).
- pipeline.rs: a newer tier 4-6 version silently overwrote a still-
  unreviewed older pending version with no indication anything was
  superseded. Now says so explicitly.
- hash.rs: builder/verifier each read a whole downloaded artifact into
  memory via std::fs::read just to hash it, doubling peak memory for no
  reason since the file's already on disk. Added sha256_hex_file,
  streamed in fixed-size chunks; both callers switched to it.
- Deduplicated two near-identical test-only "write an executable shell
  script" helpers (publisher.rs, sanity.rs) into a shared
  src/test_support.rs.

75 tests (was 63), cargo make ci clean. Re-verified end to end against
the real astral-sh/uv release after all six fixes — build, sanity check,
and publish into a scratch repo all still succeed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Austin Schaefer 2026-09-17 12:06:40 +02:00
parent 3f30e0acc6
commit f55188e36f
9 changed files with 287 additions and 49 deletions

View file

@ -353,9 +353,11 @@ Open questions on the schema:
attestation verify` rather than reimplementing sigstore verification.)*
- **Builder**: for tiers 13 on pass, generates a PKGBUILD (strict
validation on every upstream-controlled string — version, asset name,
download URL — before it touches generated shell content; every
interpolated value is embedded in a single-quoted bash string and a
literal `'` or newline in the input is rejected outright, never
download URL — before it touches generated shell content; most fields
are single-quoted, but the `install()` line necessarily uses double
quotes so `${srcdir}`/`${pkgdir}` expand, so the validation rejects `'`,
newline, `$`, backtick, *and* backslash — safe for either quoting style
rather than assuming a value only ever lands in one of them — never
unescaped interpolation) and runs `makepkg`. *(Implemented —
`src/builder.rs`. One fixed "prebuilt binary" PKGBUILD shape covers both
tracked packages so far: a bare-binary download (scaleway-cli) and a

View file

@ -3,7 +3,7 @@
//! PKGBUILD templating and upstream-string validation behind `build()`.
use crate::config::Package;
use crate::hash::sha256_hex;
use crate::hash;
use anyhow::{Context, Result, bail};
use std::path::{Path, PathBuf};
use std::process::Command;
@ -84,9 +84,7 @@ fn generate_pkgbuild(req: &BuildRequest) -> Result<String> {
let binary_name = req.pkg.binary_name(req.pkg_name);
validate_pkgname(binary_name)?;
let artifact_data = std::fs::read(req.artifact_path)
.with_context(|| format!("reading {}", req.artifact_path.display()))?;
let sha256 = sha256_hex(&artifact_data);
let sha256 = hash::sha256_hex_file(req.artifact_path)?;
let install_source = match archive_stem(req.asset_name) {
Some(stem) => format!("{stem}/{binary_name}"),
@ -118,6 +116,11 @@ fn generate_pkgbuild(req: &BuildRequest) -> Result<String> {
))
}
/// Matches `<prefix><anything>.pkg.tar.<compression>` — not hardcoded to
/// `.zst` specifically, since `PKGEXT` in makepkg.conf can be set to any
/// of pacman's supported compressions (`.xz`, `.gz`, `.bz2`, ...). This
/// box's default happens to be `.zst`, but guessing wrong would otherwise
/// report a false "makepkg failed" for a build that actually succeeded.
fn find_built_package(build_dir: &Path, pkg_name: &str, version: &str) -> Result<PathBuf> {
let prefix = format!("{pkg_name}-{version}-");
for entry in std::fs::read_dir(build_dir)? {
@ -125,12 +128,12 @@ fn find_built_package(build_dir: &Path, pkg_name: &str, version: &str) -> Result
let Some(file_name) = path.file_name().and_then(|n| n.to_str()) else {
continue;
};
if file_name.starts_with(&prefix) && file_name.ends_with(".pkg.tar.zst") {
if file_name.starts_with(&prefix) && file_name.contains(".pkg.tar.") {
return Ok(path);
}
}
bail!(
"makepkg reported success but no {prefix}*.pkg.tar.zst found in {}",
"makepkg reported success but no {prefix}*.pkg.tar.* found in {}",
build_dir.display()
)
}
@ -145,12 +148,17 @@ fn archive_stem(asset_name: &str) -> Option<&str> {
.find_map(|ext| asset_name.strip_suffix(ext))
}
/// Rejects a single quote or newline: both would let upstream-controlled
/// text (asset names, download URLs) break out of the single-quoted bash
/// strings the PKGBUILD template embeds them in. See SPEC.md > Architecture
/// > Builder ("never unescaped interpolation").
/// Rejects characters that are dangerous in *either* quoting style the
/// PKGBUILD template uses: a single quote breaks out of the single-quoted
/// fields (`pkgname`, `sha256sums`, ...); `$`, a backtick, or a backslash
/// are still live inside the double-quoted `install()` line, where
/// `asset_name` (via `install_source`) and `binary_name` end up embedded
/// so `${srcdir}`/`${pkgdir}` can expand. A single check covering both
/// contexts is safer than trying to remember which fields land in which
/// quoting style. See SPEC.md > Architecture > Builder ("never unescaped
/// interpolation").
fn validate_shell_safe(field: &str, value: &str) -> Result<()> {
if value.contains('\'') || value.contains('\n') {
if value.contains(['\'', '\n', '$', '`', '\\']) {
bail!("{field} '{value}' contains an unsafe character for a generated PKGBUILD");
}
Ok(())
@ -201,6 +209,40 @@ mod tests {
assert_eq!(archive_stem("scaleway-cli_2.62.0_linux_amd64"), None);
}
#[test]
fn find_built_package_matches_default_zst_extension() {
let dir = tempfile::tempdir().unwrap();
std::fs::write(dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.zst"), b"").unwrap();
let found = find_built_package(dir.path(), "uv", "0.12.15").unwrap();
assert_eq!(found, dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.zst"));
}
#[test]
fn find_built_package_matches_non_default_pkgext() {
// A box with PKGEXT='.pkg.tar.xz' in makepkg.conf shouldn't report
// a false failure just because this crate's default assumption
// (.zst) doesn't match.
let dir = tempfile::tempdir().unwrap();
std::fs::write(dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.xz"), b"").unwrap();
let found = find_built_package(dir.path(), "uv", "0.12.15").unwrap();
assert_eq!(found, dir.path().join("uv-0.12.15-1-x86_64.pkg.tar.xz"));
}
#[test]
fn find_built_package_ignores_non_matching_prefix() {
let dir = tempfile::tempdir().unwrap();
std::fs::write(dir.path().join("other-0.12.15-1-x86_64.pkg.tar.zst"), b"").unwrap();
assert!(find_built_package(dir.path(), "uv", "0.12.15").is_err());
}
#[test]
fn find_built_package_errors_with_clear_message_when_nothing_matches() {
let dir = tempfile::tempdir().unwrap();
let err = find_built_package(dir.path(), "uv", "0.12.15").unwrap_err();
assert!(err.to_string().contains("uv-0.12.15-"));
assert!(err.to_string().contains(".pkg.tar.*"));
}
#[test]
fn validate_pkgver_accepts_dotted_version() {
assert!(validate_pkgver("2.62.0").is_ok());
@ -241,6 +283,24 @@ mod tests {
assert!(validate_shell_safe("download url", "https://example.com/a\nb").is_err());
}
#[test]
fn validate_shell_safe_rejects_dollar_sign() {
// asset_name lands inside a double-quoted string via
// install_source — $() command substitution is still live there
// even though single-quote breakout isn't.
assert!(validate_shell_safe("asset name", "thing$(touch pwned).tar.gz").is_err());
}
#[test]
fn validate_shell_safe_rejects_backtick() {
assert!(validate_shell_safe("asset name", "thing`touch pwned`.tar.gz").is_err());
}
#[test]
fn validate_shell_safe_rejects_backslash() {
assert!(validate_shell_safe("asset name", "thing\\$(touch pwned).tar.gz").is_err());
}
#[test]
fn validate_shell_safe_accepts_normal_url() {
assert!(validate_shell_safe("download url", "https://example.com/a/b.tar.gz").is_ok());
@ -294,7 +354,7 @@ mod tests {
let dir = tempfile::tempdir().unwrap();
let artifact_path = dir.path().join("scaleway-cli_2.62.0_linux_amd64");
std::fs::write(&artifact_path, b"binary-bytes").unwrap();
let expected_sha = sha256_hex(b"binary-bytes");
let expected_sha = hash::sha256_hex(b"binary-bytes");
let req = BuildRequest {
pkg_name: "scaleway-cli",
@ -360,4 +420,26 @@ mod tests {
};
assert!(generate_pkgbuild(&req).is_err());
}
#[test]
fn generate_pkgbuild_rejects_asset_name_with_command_substitution() {
// Regression test: asset_name feeds install_source, which is
// embedded in the double-quoted install() line, not a
// single-quoted field — a single-quote-only check would miss this.
let pkg = make_package(None);
let dir = tempfile::tempdir().unwrap();
let artifact_path = dir.path().join("thing.tar.gz");
std::fs::write(&artifact_path, b"data").unwrap();
let req = BuildRequest {
pkg_name: "thing",
pkg: &pkg,
version: "1.0.0",
repo: "o/r",
asset_name: "thing$(touch pwned).tar.gz",
download_url: "https://example.com/thing.tar.gz",
artifact_path: &artifact_path,
};
assert!(generate_pkgbuild(&req).is_err());
}
}

View file

@ -1,19 +1,44 @@
//! One function, shared by two real callers (`verifier`, `builder`) —
//! Two functions, shared by two real callers (`verifier`, `builder`) —
//! not a general-purpose utils dump. See ARCHITECTURE.md > "organize by
//! pipeline stage, not by layer" for why that distinction matters.
use anyhow::{Context, Result};
use sha2::{Digest, Sha256};
use std::io::Read;
use std::path::Path;
/// Shared by `verifier` (same-origin-sha256 checks) and `builder` (every
/// generated PKGBUILD needs a `sha256sums` entry for makepkg's own local
/// integrity check, regardless of pkgwatch's own trust tier for that
/// package).
pub fn sha256_hex(data: &[u8]) -> String {
/// In-memory digest — test-only now that both real callers (`verifier`,
/// `builder`) hash a file already on disk via `sha256_hex_file` instead.
/// Kept for building expected hashes from in-memory test fixtures.
#[cfg(test)]
pub(crate) fn sha256_hex(data: &[u8]) -> String {
let mut hasher = Sha256::new();
hasher.update(data);
hex::encode(hasher.finalize())
}
/// Same digest as `sha256_hex(&std::fs::read(path)?)`, but streamed in
/// fixed-size chunks instead of reading the whole file into memory first —
/// downloaded release assets are tens of MB, and the file is already on
/// disk, so there's no reason to hold a second full copy in memory just to
/// hash it.
pub fn sha256_hex_file(path: &Path) -> Result<String> {
let mut file =
std::fs::File::open(path).with_context(|| format!("opening {}", path.display()))?;
let mut hasher = Sha256::new();
let mut buf = [0u8; 64 * 1024];
loop {
let n = file
.read(&mut buf)
.with_context(|| format!("reading {}", path.display()))?;
if n == 0 {
break;
}
hasher.update(&buf[..n]);
}
Ok(hex::encode(hasher.finalize()))
}
#[cfg(test)]
mod tests {
use super::*;
@ -26,4 +51,31 @@ mod tests {
"b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9"
);
}
#[test]
fn sha256_hex_file_matches_in_memory_digest() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("data.bin");
// Bigger than one read chunk, to actually exercise the loop.
let data = vec![0x5au8; 200 * 1024];
std::fs::write(&path, &data).unwrap();
assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(&data));
}
#[test]
fn sha256_hex_file_matches_for_empty_file() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("empty.bin");
std::fs::write(&path, b"").unwrap();
assert_eq!(sha256_hex_file(&path).unwrap(), sha256_hex(b""));
}
#[test]
fn sha256_hex_file_errors_on_missing_file() {
let dir = tempfile::tempdir().unwrap();
let missing = dir.path().join("does-not-exist.bin");
assert!(sha256_hex_file(&missing).is_err());
}
}

View file

@ -12,6 +12,8 @@ mod pipeline;
mod publisher;
mod sanity;
mod state;
#[cfg(test)]
mod test_support;
mod verifier;
use anyhow::{Result, bail};

View file

@ -131,15 +131,19 @@ fn process_package(
fetched.verification.justification
);
let already_pending =
state::load_pending_version(state_dir, name).as_deref() == Some(latest.as_str());
let previously_pending = state::load_pending_version(state_dir, name);
let already_pending = previously_pending.as_deref() == Some(latest.as_str());
match decide_tier_action(
fetched.verification.tier,
fetched.verification.passed,
already_pending,
) {
// Bail rather than just print-and-return: a verification failure
// is exactly the kind of event a monitoring setup (systemd
// OnFailure=, cron mail-on-error) needs a non-zero exit to catch —
// see run_check, which treats an Err here as a failed package.
TierAction::VerificationFailed => {
println!(" verification failed — not publishing, not updating state");
bail!("verification failed — not publishing, not updating state");
}
TierAction::Publish => {
println!(" tier 1-3 pass: building + publishing");
@ -153,7 +157,14 @@ fn process_package(
}
TierAction::NewlyPending => {
state::save_pending_version(state_dir, name, &latest)?;
println!(" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)");
match previously_pending {
Some(superseded) => println!(
" tier 4-6 pass: flagged for human review, superseding still-unreviewed {superseded} (`pkgwatch review` to approve {latest})"
),
None => println!(
" tier 4-6 pass: flagged for human review (`pkgwatch review` to approve)"
),
}
}
}
Ok(())
@ -338,4 +349,88 @@ mod tests {
fn decide_tier_action_tier_4_to_6_still_pending_when_already_flagged() {
assert_eq!(decide_tier_action(4, true, true), TierAction::StillPending);
}
/// Regression test for the exit-code gap this PR fixes: a verification
/// failure previously returned `Ok(())` from `process_package`, so
/// `run_check` never counted it as a failure and the process exited 0
/// even though the single most security-relevant check had failed.
/// Exercises the full check -> fetch -> verify path against a mocked
/// GitHub (no real network), stopping before any build/publish step
/// since verification failure returns before reaching those.
#[test]
fn process_package_returns_err_on_verification_failure() {
let mut server = mockito::Server::new();
let endpoints = GithubEndpoints {
web: server.url(),
api: server.url(),
};
let feed = format!(
r#"<feed><link rel="alternate" href="{}/o/r/releases/tag/v1.0.0"/></feed>"#,
server.url()
);
let _atom = server
.mock("GET", "/o/r/releases.atom")
.with_status(200)
.with_body(feed)
.create();
let asset_url = format!("{}/download/thing.tar.gz", server.url());
let sums_url = format!("{}/download/SHA256SUMS", server.url());
let release_body = format!(
r#"{{"assets": [
{{"name": "thing.tar.gz", "browser_download_url": "{asset_url}"}},
{{"name": "SHA256SUMS", "browser_download_url": "{sums_url}"}}
]}}"#
);
let _release = server
.mock("GET", "/repos/o/r/releases/tags/v1.0.0")
.with_status(200)
.with_body(release_body)
.create();
let _asset = server
.mock("GET", "/download/thing.tar.gz")
.with_status(200)
.with_body(b"artifact-bytes".as_slice())
.create();
// Wrong hash for "artifact-bytes" — forces a verification failure.
let _sums = server
.mock("GET", "/download/SHA256SUMS")
.with_status(200)
.with_body(
"0000000000000000000000000000000000000000000000000000000000000000 thing.tar.gz\n",
)
.create();
let pkg: Package = toml::from_str(
r#"
repo = "o/r"
asset_pattern = "thing.tar.gz"
[verification]
method = "same-origin-sha256"
checksum_asset_pattern = "SHA256SUMS"
"#,
)
.unwrap();
let client = reqwest::blocking::Client::new();
let state_dir = tempfile::tempdir().unwrap();
let work_dir = tempfile::tempdir().unwrap();
let err = process_package(
&client,
&endpoints,
state_dir.path(),
work_dir.path(),
"thing",
&pkg,
)
.unwrap_err();
assert!(err.to_string().contains("verification failed"));
// Neither published nor queued for review — a failed verification
// shouldn't leave any trace in state.
assert_eq!(state::load_last_version(state_dir.path(), "thing"), None);
assert_eq!(state::load_pending_version(state_dir.path(), "thing"), None);
}
}

View file

@ -96,21 +96,13 @@ fn ensure_registered_at(pacman_conf: &Path, repo_name: &str, repo_dir: &Path) ->
#[cfg(test)]
mod tests {
use super::*;
fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf {
let path = dir.join(name);
std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap();
let mut perms = std::fs::metadata(&path).unwrap().permissions();
std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755);
std::fs::set_permissions(&path, perms).unwrap();
path
}
use crate::test_support::write_executable_script;
#[test]
fn publish_copies_package_and_invokes_repo_add() {
let stub_dir = tempfile::tempdir().unwrap();
let log_path = stub_dir.path().join("invoked_with.txt");
let repo_add = write_stub(
let repo_add = write_executable_script(
stub_dir.path(),
"fake-repo-add",
&format!("echo \"$@\" > {}", log_path.display()),
@ -137,7 +129,7 @@ mod tests {
#[test]
fn publish_errors_when_repo_add_fails() {
let stub_dir = tempfile::tempdir().unwrap();
let repo_add = write_stub(stub_dir.path(), "fake-repo-add-fail", "exit 1");
let repo_add = write_executable_script(stub_dir.path(), "fake-repo-add-fail", "exit 1");
let src_dir = tempfile::tempdir().unwrap();
let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst");
@ -151,7 +143,7 @@ mod tests {
#[test]
fn publish_creates_repo_dir_if_missing() {
let stub_dir = tempfile::tempdir().unwrap();
let repo_add = write_stub(stub_dir.path(), "fake-repo-add-ok", "exit 0");
let repo_add = write_executable_script(stub_dir.path(), "fake-repo-add-ok", "exit 0");
let src_dir = tempfile::tempdir().unwrap();
let package_path = src_dir.path().join("thing-1.0.0-1-x86_64.pkg.tar.zst");

View file

@ -67,14 +67,7 @@ pub fn run(check: &SanityCheck, pkg_bin_dir: &Path, expected_version: &str) -> R
#[cfg(test)]
mod tests {
use super::*;
fn write_fake_binary(dir: &Path, name: &str, script: &str) {
let path = dir.join(name);
std::fs::write(&path, format!("#!/bin/sh\n{script}\n")).unwrap();
let mut perms = std::fs::metadata(&path).unwrap().permissions();
std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755);
std::fs::set_permissions(&path, perms).unwrap();
}
use crate::test_support::write_executable_script as write_fake_binary;
#[test]
fn run_passes_when_reported_version_matches() {

21
src/test_support.rs Normal file
View file

@ -0,0 +1,21 @@
//! Test-only fixture helpers shared across modules' `#[cfg(test)]` code
//! (`publisher`, `sanity`) — not production code, and not built outside
//! `cargo test`. See ARCHITECTURE.md > "organize by pipeline stage, not
//! by layer": this exists to remove one specific piece of duplication
//! (two near-identical copies of "write an executable shell script"), not
//! as a general test-utils dump.
use std::path::{Path, PathBuf};
/// Writes an executable `#!/bin/sh` script named `name` into `dir`,
/// running `body` as its contents. Used to stand in for a real binary
/// (`repo-add`, a package's own `--version` command) in tests, without
/// needing the real tool installed or a mutated global `PATH`.
pub(crate) fn write_executable_script(dir: &Path, name: &str, body: &str) -> PathBuf {
let path = dir.join(name);
std::fs::write(&path, format!("#!/bin/sh\n{body}\n")).unwrap();
let mut perms = std::fs::metadata(&path).unwrap().permissions();
std::os::unix::fs::PermissionsExt::set_mode(&mut perms, 0o755);
std::fs::set_permissions(&path, perms).unwrap();
path
}

View file

@ -7,7 +7,7 @@ use crate::checker::version_from_tag;
use crate::config::Verification;
use crate::fetcher;
use crate::github::GithubEndpoints;
use crate::hash::sha256_hex;
use crate::hash;
use anyhow::{Context, Result, bail};
use std::path::Path;
use std::process::Command;
@ -51,8 +51,7 @@ pub fn verify(
.context("artifact path has no filename")?;
let expected = expected_checksum(&checksum_text, artifact_name)?;
let data = std::fs::read(artifact_path)?;
let actual = sha256_hex(&data);
let actual = hash::sha256_hex_file(artifact_path)?;
let passed = actual == expected;
Ok(VerificationResult {
@ -192,7 +191,7 @@ mod tests {
let dest_dir = tempfile::tempdir().unwrap();
let artifact_path = dest_dir.path().join("thing.tar.gz");
std::fs::write(&artifact_path, b"hello world").unwrap();
let expected_hash = sha256_hex(b"hello world");
let expected_hash = hash::sha256_hex(b"hello world");
let release_url = format!("{}/download/SHA256SUMS", server.url());
let release_body = format!(