Files
peltier.io/crates/harnessd/src/allowlist.rs
T
benbrownandClaude Opus 4.8 899d55f4b6 gates: close the audit findings — enforce the gates the docs claimed
An adversarial audit found the enforcement story ahead of the code: gates
that were documented as hard gates were advisory, manual, or absent, and
several human-facing claims outran the machine record. This makes the
claims true.

High findings

- Differential fuzz was documentation, not enforcement: verdict hardcoded
  fuzz_iters=0 and diff-test hardcoded the gate to Skipped, while the case
  studies claimed 10k-iteration fuzz on the same wins. Fuzz is now a real
  pipeline gate (strict FUZZ-RESULT contract, executed count recorded), and
  it is inherently baseline-vs-candidate, so it runs on the accept path and
  reports Skipped in the standalone `just gates` flow. New hard rule: a
  machine `accepted` verdict is impossible unless the fuzz gate actually
  passed — otherwise it caps at needs-human-review.
- The agent's system prompt claimed FP/concurrency/unsafe patches were
  "routed to human review automatically". No such mechanism existed. Added
  a conservative lexical risk classifier (crates/verdict/src/risk.rs) that
  forces needs-human-review on any would-be accept carrying those signals;
  fp-tolerance equivalence always signals.
- The local verdict path ran agent-influenced code unsandboxed. It now runs
  under scripts/no-net.sh (network namespace) and fails closed. Upstream
  test suites are hash-verified against corpora/<t>/TESTSUITE.sha256 when
  present (scripts/pin-testsuite.sh generates them).

Also fixed, found while fixing the above

- The pipeline script's `set -e` was inert (brace group as the left operand
  of `&&`), so a failing candidate build did not abort the run and verdict
  could bench a stale binary. Now a scoped subshell whose status is branched
  on at top level.
- A wrapper that exits before exec'ing (no-net.sh refusing, bad wrapper path)
  stranded RUNNING.lock and left read_verdict polling forever. Failure
  accounting now lives outside the wrapper.
- harnessd's patch allowlist ignored git rename/copy headers and --no-prefix
  diffs; the rename-escape case is now a committed regression test.
- The workspace did not compile on Windows (ungated Unix APIs). cfg-gated;
  71 portable tests now run on both platforms.
- pin-check regenerated the corpus manifest instead of verifying it; it is
  now verify-only, with a deliberate `just pin-corpus` to re-pin.
- Max RSS is captured per run on Unix; PMU counters are opt-in diagnostics
  (compare --perf-stat), never part of the accept decision.

Docs now match the code, including what is NOT yet enforced: no target ships
a TESTSUITE.sha256 pin, no target declares [build].tsan, and matmul is not
wired to the fp-tolerance gate — each is stated as mechanized-but-inert
rather than done. Corrected the stale "33 ledger rows" (34), the playbook's
class-3 cJSON win (rejected), adversarial-review.md's sanitizer-separation
claim (five accepted rows carry sanitizers_clean=false), the unsupported
"40,000 requests" figure, and the blanket "historical rows record
fuzz_iters=0" (8 of 10; phase0-tokei-002/-003 record 4,332).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-14 13:10:14 -04:00

310 lines
12 KiB
Rust

//! Diff-header path vetting: the allowlist between agent-authored diffs
//! and `git apply` in the target workspace. Portable (no Unix
//! dependencies) so the escape-regression tests run on every development
//! platform, not just the daemon's POSIX hosts.
use anyhow::{anyhow, ensure, Result};
use std::path::{Component, Path};
/// Relative paths only; no parent components, no .git, no absolutes.
pub fn check_rel_path(p: &str) -> Result<()> {
let path = Path::new(p);
ensure!(!path.is_absolute(), "absolute paths forbidden");
for c in path.components() {
match c {
Component::Normal(seg) => ensure!(seg != ".git", ".git access forbidden"),
Component::CurDir => {}
_ => anyhow::bail!("path escapes workspace: {p}"),
}
}
Ok(())
}
const RENAME_COPY_HEADERS: [&str; 4] = ["rename from ", "rename to ", "copy from ", "copy to "];
/// Appended to every `---`/`+++` rejection. That rule is the one place where a
/// hunk BODY line can be taken for a header — `-` and `+` are the body markers
/// themselves — and the bare error ("lacks the a/ or b/ prefix", "path escapes
/// workspace") is baffling if all you did was delete a comment. Say the cause
/// out loud. No other header rule can collide: a body line always carries a
/// `-`, `+` or space marker, so it can never begin with `diff --git ` or
/// `rename `.
const HEADER_COLLISION_NOTE: &str = "\
note: this line was vetted as a `---`/`+++` file header. The scanner does not track hunk \
boundaries — deliberate, and it stays that way: a real header must never slip past it as body \
text — so a hunk body line reaches this rule too. REMOVING a line whose own text begins with \
`-- ` renders at column 0 as `--- ...` (an unindented SQL/Lua `-- comment`), and ADDING one \
that begins with `++ ` renders as `+++ ...`. If that is what this line is, the patch is being \
rejected on a false positive. Workaround: leave such lines untouched as context and edit \
around them. Rewording or indenting the line does NOT help — the removal itself is what \
renders as `--- `. Mind the space: `--x`/`++x` (C decrement/increment) never collide.";
/// Vet every path-bearing header line of a unified git diff: `diff --git`,
/// `---`, `+++`, and the rename/copy extended headers. The whole diff is
/// rejected if any extracted path is absolute, contains a `..` component,
/// is quoted (git's C-style quoting can encode `..` invisibly), or — for
/// `---`/`+++` lines — lacks the standard `a/`/`b/` prefix (`--no-prefix`
/// output). Every line is scanned without tracking hunk boundaries, so a
/// content line colliding with a header prefix is vetted too: over-rejection
/// is the safe direction, and `git apply` remains the backstop.
pub fn check_diff_paths(diff: &str) -> Result<()> {
for line in diff.lines() {
if let Some(rest) = line.strip_prefix("diff --git ") {
let (a, b) = split_git_header_paths(rest).ok_or_else(|| {
anyhow!(
"cannot parse diff header {line:?}: expected \
`diff --git a/<path> b/<path>` with plain unquoted paths"
)
})?;
vet_extracted(a)?;
vet_extracted(b)?;
} else if line.starts_with("--- ") || line.starts_with("+++ ") {
vet_file_header(&line[4..]).map_err(|e| {
anyhow!("{e}\n offending line: {line:?}\n {HEADER_COLLISION_NOTE}")
})?;
} else if let Some(rest) = RENAME_COPY_HEADERS
.iter()
.find_map(|pre| line.strip_prefix(pre))
{
vet_extracted(header_path(rest))?;
}
}
Ok(())
}
/// Vet the path on a `---`/`+++` file-header line; `rest` is the text after
/// the four-byte marker. Errors are wrapped by the caller with
/// `HEADER_COLLISION_NOTE`, since any line reaching here *might* be a hunk
/// body line rather than a header.
fn vet_file_header(rest: &str) -> Result<()> {
let p = header_path(rest);
if p == "/dev/null" {
return Ok(());
}
ensure!(
!p.starts_with('"'),
"quoted diff paths forbidden: {p} (rewrite the patch with plain ASCII paths)"
);
let stripped = p
.strip_prefix("a/")
.or_else(|| p.strip_prefix("b/"))
.ok_or_else(|| {
anyhow!(
"diff path {p:?} lacks the a/ or b/ prefix — regenerate with \
standard `git diff` output (--no-prefix diffs are rejected)"
)
})?;
vet_extracted(stripped)
}
/// Header paths may carry a traditional-diff `\t<timestamp>` suffix.
fn header_path(rest: &str) -> &str {
rest.split('\t').next().unwrap_or(rest).trim_end()
}
/// `diff --git a/<x> b/<y>`: split at the LAST ` b/` so a ` b/` embedded
/// in <x> cannot truncate <y>; splitting only ever removes the three
/// bytes ` b/`, so a `..` component in either real path always survives
/// intact into one of the two fragments.
fn split_git_header_paths(rest: &str) -> Option<(&str, &str)> {
let a_rest = rest.strip_prefix("a/")?;
let idx = a_rest.rfind(" b/")?;
Some((&a_rest[..idx], a_rest[idx + 3..].trim_end()))
}
fn vet_extracted(p: &str) -> Result<()> {
if p == "/dev/null" {
return Ok(());
}
ensure!(!p.is_empty(), "empty path in diff header");
ensure!(
!p.starts_with('"'),
"quoted diff paths forbidden: {p} (rewrite the patch with plain ASCII paths)"
);
ensure!(
!p.starts_with('/'),
"absolute diff paths forbidden (only /dev/null is allowed, for new/deleted files): {p}"
);
check_rel_path(p)
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn rel_path_rules() {
assert!(check_rel_path("src/lib.rs").is_ok());
assert!(check_rel_path("./src/lib.rs").is_ok());
assert!(check_rel_path("../outside").is_err());
assert!(check_rel_path("a/../b").is_err());
assert!(check_rel_path("/etc/passwd").is_err());
assert!(check_rel_path(".git/config").is_err());
assert!(check_rel_path("a/.git/hooks").is_err());
}
#[test]
fn normal_diff_passes() {
let diff = "diff --git a/src/lib.rs b/src/lib.rs\n\
index 1111111..2222222 100644\n\
--- a/src/lib.rs\n\
+++ b/src/lib.rs\n\
@@ -1,2 +1,2 @@\n\
-old\n\
+new\n";
assert!(check_diff_paths(diff).is_ok());
}
#[test]
fn new_and_deleted_file_dev_null_passes() {
let diff = "diff --git a/newfile.rs b/newfile.rs\n\
new file mode 100644\n\
--- /dev/null\n\
+++ b/newfile.rs\n\
@@ -0,0 +1 @@\n\
+hello\n\
diff --git a/gone.rs b/gone.rs\n\
deleted file mode 100644\n\
--- a/gone.rs\n\
+++ /dev/null\n\
@@ -1 +0,0 @@\n\
-bye\n";
assert!(check_diff_paths(diff).is_ok());
}
#[test]
fn in_workspace_rename_passes() {
let diff = "diff --git a/old.rs b/nested/new.rs\n\
similarity index 100%\n\
rename from old.rs\n\
rename to nested/new.rs\n";
assert!(check_diff_paths(diff).is_ok());
}
/// The exact fixture from the apply-escape audit: a rename whose
/// destination climbs out of the workspace. `---`/`+++` scanning alone
/// never sees it — the paths live only in the `diff --git` and
/// rename headers.
#[test]
fn rename_escape_fixture_rejected() {
let diff = "diff --git a/src.txt b/../outside/escaped.txt\n\
similarity index 100%\n\
rename from src.txt\n\
rename to ../outside/escaped.txt\n";
assert!(check_diff_paths(diff).is_err());
}
#[test]
fn rename_headers_rejected_without_git_header_line() {
let diff = "similarity index 100%\n\
rename from src.txt\n\
rename to ../outside/escaped.txt\n";
assert!(check_diff_paths(diff).is_err());
}
#[test]
fn copy_escape_rejected() {
let diff = "diff --git a/src.txt b/src2.txt\n\
copy from src.txt\n\
copy to ../outside/copied.txt\n";
assert!(check_diff_paths(diff).is_err());
}
#[test]
fn no_prefix_diff_rejected() {
let diff = "diff --git src/lib.rs src/lib.rs\n\
index 1111111..2222222 100644\n\
--- src/lib.rs\n\
+++ src/lib.rs\n\
@@ -1 +1 @@\n\
-old\n\
+new\n";
let err = check_diff_paths(diff).unwrap_err().to_string();
assert!(
err.contains("diff --git") || err.contains("a/ or b/ prefix"),
"{err}"
);
}
#[test]
fn absolute_path_diff_rejected() {
let diff = "diff --git a/x b/x\n\
rename from x\n\
rename to /etc/passwd\n";
assert!(check_diff_paths(diff).is_err());
let plus = "--- a/x\n\
+++ /etc/passwd\n";
assert!(check_diff_paths(plus).is_err());
}
#[test]
fn parent_component_in_file_headers_rejected() {
assert!(check_diff_paths("--- a/../escape.rs\n+++ b/escape.rs\n").is_err());
assert!(check_diff_paths("--- a/ok.rs\n+++ b/nested/../../escape.rs\n").is_err());
assert!(check_diff_paths("diff --git a/ok.rs b/../escape.rs\n").is_err());
}
#[test]
fn dot_git_path_rejected() {
let diff = "diff --git a/.git/hooks/pre-commit b/.git/hooks/pre-commit\n\
--- a/.git/hooks/pre-commit\n\
+++ b/.git/hooks/pre-commit\n";
assert!(check_diff_paths(diff).is_err());
}
#[test]
fn quoted_paths_rejected() {
assert!(check_diff_paths("diff --git \"a/x\" \"b/y\"\n").is_err());
assert!(check_diff_paths("--- \"a/x\"\n+++ b/y\n").is_err());
assert!(check_diff_paths("rename to \"\\056\\056/evil\"\n").is_err());
}
#[test]
fn embedded_b_slash_cannot_hide_escape() {
// Whatever way ` b/` splits this header, the `..` lands in a fragment.
assert!(check_diff_paths("diff --git a/x b/y b/../evil\n").is_err());
assert!(check_diff_paths("diff --git a/../evil b/y b/z\n").is_err());
}
/// A removed body line that renders as `--- ` at column 0 — deleting an
/// unindented SQL/Lua `-- comment` — is still REJECTED (fail-closed is the
/// point; tracking hunk boundaries here would risk desyncing from git's own
/// parser). But the error must name that cause instead of leaving an agent
/// staring at a complaint about a/ and b/ prefixes it never wrote.
#[test]
fn body_line_colliding_with_header_prefix_explains_itself() {
let diff = "diff --git a/q.sql b/q.sql\n\
--- a/q.sql\n\
+++ b/q.sql\n\
@@ -1,2 +1 @@\n\
--- drop this comment\n\
\x20SELECT 1;\n";
let err = check_diff_paths(diff).unwrap_err().to_string();
assert!(err.contains("--- drop this comment"), "{err}");
assert!(err.contains("hunk body line"), "{err}");
assert!(err.contains("context"), "{err}");
}
/// The collision needs the SPACE. A removed `--x;` renders as `---x;` and an
/// added `++x;` as `+++x;` — neither is a `--- `/`+++ ` header, and both
/// must sail through. Guards against anyone "fixing" the note above by
/// widening the prefix match.
#[test]
fn body_line_without_the_space_does_not_collide() {
let diff = "diff --git a/x.c b/x.c\n\
--- a/x.c\n\
+++ b/x.c\n\
@@ -1,2 +1,2 @@\n\
---x;\n\
+++x;\n";
assert!(check_diff_paths(diff).is_ok());
}
#[test]
fn timestamp_suffix_tolerated() {
let diff = "--- a/src/lib.rs\t2026-07-13 00:00:00\n\
+++ b/src/lib.rs\t2026-07-13 00:00:00\n";
assert!(check_diff_paths(diff).is_ok());
}
}