makepkg-with-sleuthing audits twice per package; deduplicate rather than skip

[mgalgs]

The cost

Both helpers now audit twice per package, and after #6 that is a change for yay users in particular:

helper	phase	argv	before #6	after #6
yay	fetch	--nobuild -f -C	skip	audit
yay	build	-f --noconfirm --noextract --noprepare --holdver -c	audit	audit
paru	fetch	-ofA -C	audit	audit
paru	build	-feA --noconfirm --noprepare --holdver -c	audit	audit

So yay goes from one audit per package to two, and paru stays at two. #6 closed a real hole to get there — the fetch phase runs prepare() and pkgver(), and skipping it meant the audit landed after maintainer code had already executed — but the second audit is pure waste and should go.

Why the second audit is provably redundant

maintainer_files() is git ls-files on the AUR clone. Extraction into src/ does not change the review set, so the fetch-phase audit and the build-phase audit read a byte-identical file set, send an identical prompt, and bill twice for the same answer. Auditing the earliest code-executing invocation is sufficient: later phases introduce no maintainer file the first pass did not already see.

Shape

Key on the package directory plus a hash of the maintainer file contents, with a short TTL. The hash is what makes it safe for the package under audit: changing any reviewed file changes the key and forces a re-audit, so a maintainer cannot edit past a cached pass.

The objection raised on #6 is worth recording: a cache the user can write is a cache a malicious package() can write, since package() runs under fakeroot, which fakes uid without confining the filesystem — so a previously-installed hostile package could plant a "safe" entry for a package it anticipates. True, but that attacker already has arbitrary code execution as the user and can equally overwrite aur-sleuth, the wrapper, or PATH. The cache is not surface they lack. Worth stating explicitly in whatever lands, because it will be asked again.

Not this

Do not solve it by widening the skip list again. Which makepkg invocations may skip is settled in bench/test-makepkg-wrapper.sh against the real argv both helpers pass, and the answer is "only what exits before extraction". Deduplication has to sit beside that decision, not inside it.

Separately

bench/synthetics/ has no fixture for the wrapper's end-to-end path, so nothing catches a regression in which invocation gets audited on a real helper run. The new suite covers the decision; it does not cover the plumbing.

[mgalgs]

Closing this. The premise is wrong: the second audit is not redundant, and no local deduplication can be made safe.

The second audit is a re-check, not a repeat. prepare() runs as the invoking user, unsandboxed, with $startdir writable, and makepkg copies the .install hook out of $startdir at package time (create_package()), long after run_prepare. So a package whose prepare() reads clean can rewrite its own .install hook after the fetch-phase audit has cleared it, and makepkg will package the rewritten one. The two audits answer different questions:

1. fetch — is it safe to run prepare() and pkgver()?
2. build — did prepare() change anything before build(), package() and the .install hook run?

The "byte-identical input" claim in the original description holds only when nothing tampered, which is exactly the case where the check is not needed.

And the dedup cannot be rescued by hashing. The idea was that a content hash would force a re-audit on tampering. It does not, because of who the attacker is: prepare() of the package under audit, for whom this is the first code execution. No prior foothold is required. Running as the user, it can tamper with the hook, hash what it just wrote, and write a matching entry into whatever cache the wrapper consults. The build-phase audit then reads a hit and skips.

That defeats every local shape: a global cache, a marker inside the package directory, an HMAC (the key has to be readable by the same user whose code is forging), a TTL. Any state the wrapper can read, prepare() can write first.

So two audits per package is correct, and the cost is the price of the check. #6 is what makes the first one land ahead of prepare(); this issue was arguing to undo half of that. bench/test-makepkg-wrapper.sh pins the decision table, and the rule it encodes stands: skip only what exits before extraction.

For the record, @UntilMarsAgain reached this conclusion on #6 and I argued against it. The specific scenario given there — a previously installed package planting an entry for a future one — does require a prior foothold, and that was the part I pushed back on. The self-contained version above does not, and it is the one that settles it. The instinct was right.

If the double cost becomes a real problem, the tractable direction is making an audit cheaper, not making it conditional.