Repository navigation
bug(deploy): the husk predicate follows symlinks, and #17339 makes it run as root on every host #17362
Description
Problem
venv_provenance.is_provenance_husk classifies by Path.is_dir() and Path.iterdir(), both of which follow symlinks:
if not dist_info.is_dir():
return False
...
contents = [entry.name for entry in dist_info.iterdir()]
return contents == [PROVENANCE_MARKER_FILENAME]So a symlink named *.dist-info pointing at a directory whose sole entry is a file called AUTOBOT_PROVENANCE is classified as a husk, and clear_provenance_husks then runs:
(dist_info / PROVENANCE_MARKER_FILENAME).unlink() # unlinks THROUGH the link, in the target dir
dist_info.rmdir() # fails on a symlink, leaving the link behindThe rmdir fails (a symlink is not an empty directory), so the link survives — but the file inside the target directory is already gone. The predicate's whole promise is that it "cannot reach a real installation, an operator's files, or another tool's"; through a link, it can.
Why this is worth recording now rather than when it was written
It is inherited from #17332 and it is theoretical — nothing in this codebase creates such a symlink, and an attacker who can write into site-packages has easier paths. What changed is the blast radius:
- Before deploy(update): the venv husk repair is unreachable from ansible provisioning, so a husked host stays broken #17339 the predicate ran only on the builtin code-sync path, on the manager, when an update was requested.
- After deploy(update): the venv husk repair is unreachable from ansible provisioning, so a husked host stays broken #17339 it runs from ansible provisioning, as root, unconditionally, on every provisioned host, in three venvs (
autobot-backend,autobot-slm-backend,autobot-ai-stack).
Same defect, three orders of magnitude more occasions to meet one.
Acceptance criteria
-
is_provenance_huskrefuses adist-infothat is a symlink —Path.is_dir(follow_symlinks=False)on 3.13+, oros.path.islinkfirst - The marker unlink refuses to follow a link out of the directory it is clearing
- A test plants a
*.dist-infosymlink whose target holds a loneAUTOBOT_PROVENANCEfile and asserts the target file survives - The narrow-deletion docstring says so, since it currently promises a property the code does not have
Notes
Found during #17339's review (comment 5807391532). Not fixed there because widening a PR that makes a deletion reachable into one that also changes what the deletion matches is two different risks in one review — and the second wants its own test, not a line in someone else's.
Activity
fix(venv): make the RECORD rewrite atomic and stop the husk scan foll…
fix(ansible): a held package is installed -- the new gate said it was…
mrveiss commentedon Sep 24, 2026
Delivered in PR #17398, squash-merged as
5dcdb18cec. Verified against mergedmainviagit show origin/main:<path>, criterion by criterion.main@5dcdb18cecis_provenance_huskrefuses adist-infothat is a symlinkvenv_provenance.py:167—if dist_info.is_symlink() or not dist_info.is_dir(): return False. The symlink test comes first, so it is decided beforeis_dir()can follow the link and answerTrue.dist-infois never classified as a husk and the unlink is never reached for one.*.dist-infosymlink whose target holds a loneAUTOBOT_PROVENANCEfile and asserts the target file survivestest_a_symlinked_dist_info_is_not_a_husk(test_venv_provenance_husk_17332.py:326) —link.symlink_to(outside, target_is_directory=True), then asserts the predicate is False, that the target file still exists ("the husk clearer deleted a file through a symlink, outside the directory it was clearing"), and that the link itself is left alone rather than half-removed.The detail worth keeping
The third assertion —
assert link.is_symlink()— is the one that would be easy to leave out, and it tests a different failure from the other two. Without it, an implementation that refused the husk and removed the link would still pass: the target file survives, the predicate returns False, and a half-removed link is left behind. Three assertions, three distinct ways to get this wrong.And the docstring criterion is not cosmetic here. The old text promised narrow deletion, the code did not deliver it, and the gap only became dangerous when #17339 changed who runs it and how often — root, every host, three venvs. A promise that was merely optimistic became a promise that could delete files outside
site-packages.