thunderbird-patch-review
Simple email patch review tool for Thunderbird
git clone git://mccd.space/thunderbird-patch-reviewcommit 15dadef0836087ea6982e7bf63c03ae47b4a97d9
parent a90a0d90dcc5dd872db92b6256a07ddf286f456b
Author: Pi Agent <agent@pi.local>
Date: Wed, 22 Jul 2026 15:35:41 +0200
Run hooks on Apply by default; add 'Apply without hooks' option
The Apply button is a deliberate human act, so the user's own git hooks
(applypatch-msg, pre-applypatch, post-applypatch) now run on it as part
of a normal apply — their workflow expects them. The earlier blanket
suppression was for the auto-apply-without-a-deliberate-act risk, but
the click is that deliberate act.
For untrusted content where even a deliberate apply shouldn't run user
code on the patch text, add "Apply without hooks" to the apply
dropdown: it sets the new noHooks arg, which points git at an empty
hooks dir for both am and its --abort rollback (the same suppression
the automated check() probe always uses, since it has no human act at
all). Modify and apply keeps running hooks (also deliberate).
patchHost.apply() gains an optional noHooks boolean (schema, default
false); background.js forwards request.noHooks; review.js maps the
apply-no-hooks mode to it. The -c core.hooksPath value is per-invocation
only, never written to the user's config.
Diffstat:
5 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/extension/api/patchHost/implementation.js b/extension/api/patchHost/implementation.js
@@ -44,17 +44,21 @@ async function run(command, args) {
// 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.
+// post-applypatch; `git worktree add` fires post-checkout.
+//
+// The Apply button is a deliberate human act, so the user's own hooks run
+// there as part of a normal apply — their workflow expects them. The
+// "Apply without hooks" dropdown item (apply()'s noHooks arg) instead
+// points git at an empty hooks dir, for applying untrusted content where
+// even a deliberate apply shouldn't run user code on the patch text. The
+// automated check() probe always suppresses (no human act at all).
+//
+// 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,
@@ -137,7 +141,7 @@ async function describe(repo) {
return describeRepo(git, repo);
}
-async function apply(repo, strategy, patches) {
+async function apply(repo, strategy, patches, noHooks) {
const git = await findGit();
await describeRepo(git, repo);
if (!patches.length) {
@@ -170,11 +174,17 @@ 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();
+ // Hooks: the Apply button is a deliberate human act, so the user's own
+ // hooks run as part of a normal apply (their workflow expects them).
+ // "Apply without hooks" (noHooks) instead points git at an empty hooks
+ // dir for both am and its rollback, for applying untrusted content
+ // where even a deliberate apply shouldn't run user code on the patch.
+ // The automated check() probe always suppresses; apply() defaults to
+ // running hooks. The -c value is per-invocation only, never written to
+ // the user's config, so their hooks are intact for normal git use.
+ const hooks = noHooks ? await emptyHooksDir() : null;
try {
- const hp = ["-c", `core.hooksPath=${hooks}`];
+ const hp = hooks ? ["-c", `core.hooksPath=${hooks}`] : [];
const args = ["-C", repo, "-c", "advice.mergeConflict=false", "-c", "advice.amWorkDir=false", ...hp, "am"];
if (strategy !== "am") {
args.push("--3way");
@@ -198,7 +208,7 @@ async function apply(repo, strategy, patches) {
};
} finally {
IOUtils.remove(work, { recursive: true }).catch(() => {});
- IOUtils.remove(hooks, { recursive: true }).catch(() => {});
+ if (hooks) IOUtils.remove(hooks, { recursive: true }).catch(() => {});
}
}
@@ -551,7 +561,7 @@ var patchHost = class extends ExtensionAPI {
patchHost: {
ping: () => guarded(ping),
describe: (repo) => guarded(() => describe(repo)),
- apply: (repo, strategy, patches) => guarded(() => apply(repo, strategy, patches)),
+ apply: (repo, strategy, patches, noHooks) => guarded(() => apply(repo, strategy, patches, noHooks)),
check: (repo, strategy, patches) => guarded(() => check(repo, strategy, patches)),
pickDir: (start) => guarded(() => pickDir(start)),
openEditor: (repo, editor, files) => guarded(() => openEditor(repo, editor, files)),
diff --git a/extension/api/patchHost/schema.json b/extension/api/patchHost/schema.json
@@ -36,6 +36,13 @@
"type": "array",
"items": { "type": "string" },
"description": "Raw RFC822 patch messages, in series order."
+ },
+ {
+ "name": "noHooks",
+ "type": "boolean",
+ "optional": true,
+ "default": false,
+ "description": "True to suppress the user's git hooks for this apply (applypatch-msg, pre-applypatch, post-applypatch) by pointing git at an empty hooks dir; false (the default) runs hooks as a normal git am would. Use true for untrusted content."
}
]
},
diff --git a/extension/background.js b/extension/background.js
@@ -211,7 +211,7 @@ async function handle(request) {
for (const messageId of request.messageIds) {
patches.push(await rawMessage(messageId));
}
- return host().apply(request.repo, config.strategy || "am3", patches);
+ return host().apply(request.repo, config.strategy || "am3", patches, Boolean(request.noHooks));
}
// Forwarded to patchHost.check(): classifies the series as already
diff --git a/extension/review/review.html b/extension/review/review.html
@@ -16,6 +16,7 @@
<button class="menu-toggle" title="Apply options">▾</button>
<div class="menu" hidden>
<button data-mode="modify">Modify and apply</button>
+ <button data-mode="apply-no-hooks">Apply without hooks</button>
<button data-mode="download">Download patchset…</button>
<button data-bypass="1" id="btn-refresh">Refresh…</button>
</div>
diff --git a/extension/review/review.js b/extension/review/review.js
@@ -780,7 +780,11 @@ async function applySeries(mode) {
return;
}
- const response = await bg({ type: "apply-series", messageIds, repo });
+ // "apply" and "modify" run the user's git hooks (a deliberate apply).
+ // "apply-no-hooks" (Apply without hooks) suppresses them for untrusted
+ // content. download bypasses apply entirely and never reaches here.
+ const noHooks = mode === "apply-no-hooks";
+ const response = await bg({ type: "apply-series", messageIds, repo, noHooks });
const text = [response.ok ? "Series applied." : "Apply failed.", response.output || response.error]
.filter(Boolean)
.join("\n\n");