fix(evaluator): git refused the checkout it was asked to verify
Found by the P0.1 verification run, which is the point of it. Mission
019fc02e's judge executed `git status` for real — and got exit 128:
fatal: detected dubious ownership in repository at
'/var/lib/clawmates-missions/019fc02e-.../repo'
The server clones as uid 65532; the runtime container the judge execs into
runs as root; git's ownership check refuses the repository. So the judge's
most direct verification tool was failing on every mission. It recovered here
by inferring a clean tree from `ls -la` and `find`, and reasoned correctly —
but that is inference from a directory listing standing in for the command
that answers the question directly.
`git` invocations now carry `-c safe.directory=<workdir>`, scoped to that one
checkout. Not `--global`: the protection exists for multi-user machines where
another user could plant a hostile `.git/config`, and disabling it container-
wide to fix one path would trade a real guarantee for convenience. Applied
per-invocation rather than baked into the image so it travels with the workdir
and cannot drift out of sync with it.
Tests cover the rewrite, that non-git commands are untouched, and that a
rewritten `git push` still fails the allow-list — the injected `-c` flags must
not become a way past validation.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
2c7d619cf0
commit
491449f3ce
@@ -249,11 +249,12 @@ impl Sandbox {
|
|||||||
Err(e) => return CheckOutcome::could_not_run(argv, format!("COULD NOT RUN: {e}")),
|
Err(e) => return CheckOutcome::could_not_run(argv, format!("COULD NOT RUN: {e}")),
|
||||||
};
|
};
|
||||||
let workdir = self.workdir.display().to_string();
|
let workdir = self.workdir.display().to_string();
|
||||||
|
let effective = with_safe_directory(argv, &workdir);
|
||||||
let out = crate::container_exec::exec(
|
let out = crate::container_exec::exec(
|
||||||
&docker,
|
&docker,
|
||||||
&self.container,
|
&self.container,
|
||||||
Some(&workdir),
|
Some(&workdir),
|
||||||
argv,
|
&effective,
|
||||||
COMMAND_TIMEOUT,
|
COMMAND_TIMEOUT,
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
@@ -289,6 +290,38 @@ impl Sandbox {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Let git read a checkout it does not own.
|
||||||
|
///
|
||||||
|
/// The server clones the mission repo as uid 65532; the runtime container the
|
||||||
|
/// judge execs into runs as root. Git's ownership check then refuses the
|
||||||
|
/// repository outright:
|
||||||
|
///
|
||||||
|
/// ```text
|
||||||
|
/// fatal: detected dubious ownership in repository at '/var/lib/clawmates-missions/<id>/repo'
|
||||||
|
/// ```
|
||||||
|
///
|
||||||
|
/// Observed in production on mission 019fc02e: `git status` returned 128 and
|
||||||
|
/// the judge had to infer a clean tree from `ls` instead — weaker evidence for
|
||||||
|
/// the exact question `git status` answers directly. The protection is aimed
|
||||||
|
/// at multi-user machines where another user could plant a hostile
|
||||||
|
/// `.git/config`; here the two uids are both ours and the path is one we
|
||||||
|
/// constructed, so scoping the exception to this single directory is the
|
||||||
|
/// narrow fix. `--global` would disable the check everywhere in the container.
|
||||||
|
///
|
||||||
|
/// Applied per-invocation rather than by configuring the image, so it travels
|
||||||
|
/// with the workdir and cannot drift out of sync with it.
|
||||||
|
fn with_safe_directory(argv: &[String], workdir: &str) -> Vec<String> {
|
||||||
|
if argv.first().map(String::as_str) != Some("git") {
|
||||||
|
return argv.to_vec();
|
||||||
|
}
|
||||||
|
let mut out = Vec::with_capacity(argv.len() + 2);
|
||||||
|
out.push("git".to_string());
|
||||||
|
out.push("-c".to_string());
|
||||||
|
out.push(format!("safe.directory={workdir}"));
|
||||||
|
out.extend(argv[1..].iter().cloned());
|
||||||
|
out
|
||||||
|
}
|
||||||
|
|
||||||
/// One verification command and what became of it.
|
/// One verification command and what became of it.
|
||||||
///
|
///
|
||||||
/// This exists because the first version recorded *attempted* commands. The
|
/// This exists because the first version recorded *attempted* commands. The
|
||||||
@@ -440,6 +473,42 @@ mod tests {
|
|||||||
assert!(check_argv(&argv(&["bash", "-c", "ls"])).is_err());
|
assert!(check_argv(&argv(&["bash", "-c", "ls"])).is_err());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn git_gets_a_scoped_safe_directory_exception() {
|
||||||
|
let out = with_safe_directory(&argv(&["git", "status"]), "/missions/abc/repo");
|
||||||
|
assert_eq!(
|
||||||
|
out,
|
||||||
|
argv(&["git", "-c", "safe.directory=/missions/abc/repo", "status"]),
|
||||||
|
"the exception is scoped to this checkout, never --global"
|
||||||
|
);
|
||||||
|
// Flags and subcommand order survive.
|
||||||
|
let diff = with_safe_directory(&argv(&["git", "diff", "--stat"]), "/w");
|
||||||
|
assert_eq!(
|
||||||
|
diff,
|
||||||
|
argv(&["git", "-c", "safe.directory=/w", "diff", "--stat"])
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn non_git_commands_are_untouched() {
|
||||||
|
let cmd = argv(&["cargo", "test"]);
|
||||||
|
assert_eq!(with_safe_directory(&cmd, "/w"), cmd);
|
||||||
|
assert_eq!(with_safe_directory(&[], "/w"), Vec::<String>::new());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The injected `-c` must not become a way past the allow-list: validation
|
||||||
|
/// runs on the original argv, so the subcommand check still sees `status`
|
||||||
|
/// rather than a flag.
|
||||||
|
#[test]
|
||||||
|
fn injection_happens_after_validation() {
|
||||||
|
assert!(check_argv(&argv(&["git", "push"])).is_err());
|
||||||
|
let rewritten = with_safe_directory(&argv(&["git", "push"]), "/w");
|
||||||
|
assert!(
|
||||||
|
check_argv(&rewritten).is_err(),
|
||||||
|
"a rewritten command must still fail the allow-list"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// ── What a check may claim about itself ────────────────────────────
|
// ── What a check may claim about itself ────────────────────────────
|
||||||
|
|
||||||
/// The property the whole struct exists for. A refused command and an
|
/// The property the whole struct exists for. A refused command and an
|
||||||
|
|||||||
Reference in New Issue
Block a user