thunderbird-patch-review
Simple email patch review tool for Thunderbird
git clone git://mccd.space/thunderbird-patch-reviewcommit 9f4c4376617f9e5f5a8ec419b0717995ce1eff7a
parent c4adc9f80fb2a060196b2ffb9e8015521b010dac
Author: Pi Agent <agent@pi.local>
Date: Mon, 20 Jul 2026 10:36:39 +0200
Suppress git hooks during Apply and the check() worktree probe
Patches arrive by email from strangers on a public mailing list and the
only gate is the Apply button; both apply() and stage 3 of check() hand
them to git, which then runs the user's own hooks against that
attacker-authored content — git am fires applypatch-msg,
pre-applypatch, post-applypatch; git worktree add fires post-checkout
(in a linked worktree hooks resolve through the common .git dir). The
user's own code, so not arbitrary remote execution, but a patch editing
a hook-adjacent file or a checkout tripping a configured clean/smudge
filter narrows the gap.
Point every git invocation that touches the patches at a freshly
created empty directory via `-c core.hooksPath=<dir>` for that single
invocation only (the -c value is never written to the user's config).
An empty existing dir has no hook files for git to find on any platform;
verified to suppress post-checkout and all three am hooks. The dir is
created before the worktree add (so post-checkout fires nothing) and
removed in the same finally blocks that already clean the patch
workspace.
Filters (clean/smudge) remain a narrower surface; disabling them needs
per-filter config and is out of scope here.
Diffstat:
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/extension/api/patchHost/implementation.js b/extension/api/patchHost/implementation.js
@@ -41,6 +41,29 @@ async function run(command, args) {
return { exitCode, output };
}
+// Return a path to a freshly created empty directory git can be pointed at
+// via `-c core.hooksPath=<path>` to silence every hook for a single
+// invocation. git am fires applypatch-msg, pre-applypatch and
+// post-applypatch; `git worktree add` fires post-checkout. The patches we
+// hand git arrive by email from strangers on a public mailing list, and
+// the only gate is the Apply button — the user's own hooks then execute
+// against attacker-authored content (a patch touching a hook-adjacent
+// file narrows the gap to real remote execution). We never want that
+// during an auto-apply. core.hooksPath is read per invocation (the -c
+// value is never written to the user's config), and an empty existing
+// dir has no hook files for git to find, on every platform. A purely
+// nonexistent path would also work on POSIX but is less portable
+// (Windows treats `/dev/null`-style paths oddly), so we make a real empty
+// directory instead. Callers remove it in their finally.
+async function emptyHooksDir() {
+ const dir = PathUtils.join(
+ PathUtils.tempDir,
+ `patch-review.hooks.${Date.now()}.${Math.floor(Math.random() * 1e9)}`
+ );
+ await IOUtils.makeDirectory(dir, { permissions: 0o700 });
+ return dir;
+}
+
async function findGit() {
try {
return await Subprocess.pathSearch("git", Subprocess.getEnvironment());
@@ -101,8 +124,12 @@ async function apply(repo, strategy, patches) {
`patch-review.${Date.now()}.${Math.floor(Math.random() * 1e9)}`
);
await IOUtils.makeDirectory(work, { permissions: 0o700 });
+ // Point git at an empty hooks dir so the user's own hooks never run on
+ // attacker-authored patch content during am (and the --abort rollback).
+ const hooks = await emptyHooksDir();
try {
- const args = ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "am"];
+ const hp = ["-c", `core.hooksPath=${hooks}`];
+ const args = ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "am"];
if (strategy !== "am") {
args.push("--3way");
}
@@ -115,7 +142,7 @@ async function apply(repo, strategy, patches) {
if (am.exitCode === 0) {
return { ok: true, output: am.output };
}
- const abort = await run(git, ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "am", "--abort"]);
+ const abort = await run(git, ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "am", "--abort"]);
return {
ok: false,
output:
@@ -125,6 +152,7 @@ async function apply(repo, strategy, patches) {
};
} finally {
IOUtils.remove(work, { recursive: true }).catch(() => {});
+ IOUtils.remove(hooks, { recursive: true }).catch(() => {});
}
}
@@ -357,17 +385,23 @@ async function check(repo, strategy, patches) {
PathUtils.tempDir,
`patch-review.am.${Date.now()}.${Math.floor(Math.random() * 1e9)}`
);
- const make = await run(git, ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "worktree", "add", "--detach", work]);
+ // Empty hooks dir, created before the worktree so post-checkout fires
+ // nothing on the checkout, and reused for the am run and its abort so
+ // applypatch-msg/pre-applypatch/post-applypatch fire nothing either.
+ const hooks = await emptyHooksDir();
+ const hp = ["-c", `core.hooksPath=${hooks}`];
+ const make = await run(git, ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "worktree", "add", "--detach", work]);
if (make.exitCode !== 0) {
// Rare (bare repo, no worktree support, …). Don't trust --check's
// partial answer here — report "conflict" so the user clicks Apply
// and reads am's own error, instead of guessing.
+ IOUtils.remove(hooks, { recursive: true }).catch(() => {});
return { ok: true, status: "conflict", output: fwd.output + make.output };
}
try {
const file = PathUtils.join(work, "series.mbox");
await IOUtils.writeUTF8(file, patches.join("\n"));
- const args = ["-C", work, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "am"];
+ const args = ["-C", work, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "am"];
if (strategy !== "am") {
args.push("--3way");
}
@@ -382,12 +416,13 @@ async function check(repo, strategy, patches) {
// am stopped mid-series (context mismatch, merge conflict, dirty
// tree). Roll the worktree back so its state isn't left dangling, then
// report conflict with am's diagnostic — it names the file and hunk.
- await run(git, ["-C", work, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "am", "--abort"]);
+ await run(git, ["-C", work, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "am", "--abort"]);
return { ok: true, status: "conflict", output: am.output };
} finally {
// --force drops the worktree even if am left its working tree dirty.
await run(git, ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", "worktree", "remove", "--force", work]);
IOUtils.remove(work, { recursive: true }).catch(() => {});
+ IOUtils.remove(hooks, { recursive: true }).catch(() => {});
}
}