Reject symlinked Linux update staging folders #1343

Merged

monsterlag merged 1 commit from fix/linux-staging-leaf into main

2026-11-09 23:12:25 +00:00

monsterlag commented

2026-11-09 22:14:52 +00:00

(Migrated from github.com)

Reject a pre-existing symbolic link at versions/<tag>.part before stale-folder cleanup, part placement or download. Leave the link untouched and report the exact staging path through the existing update-error flow. Symlinked parent installation roots remain supported.

Current main reaches cleanup or part placement before failing on these links. Pending #1327 canonicalizes the staging root in _place, so a dangling staging leaf can instead select its resolved target before the new ZIP checks. This is a separate leaf check; it does not duplicate or change #1327's archive containment rules.

Thanks to @Demonicmurderer for #1295 and @bronze-ff for the related #1327.

Validation:

Extended the existing Linux updater fixture. Identical assertions fail both link cases on main and pass after the fix; fresh staging, stale real staging, and a symlinked parent root pass throughout.
The strict selected Linux runner passes (one check, all 12 test methods). All 10 pre-existing methods are unchanged.
A separate authored temp-only probe of exact #1327 source reaches an injected unpack barrier with the resolved target; no outside-staging write is performed. The two-line guard overlaid on that source passes the same five new scenarios.

The new regression uses temporary paths and doubles, with no network or real archive/installer execution. The existing Linux-only test scope is retained; no Windows symlink privilege is required. This does not establish archive integration coverage or protection against concurrent replacement of the staging leaf.

All eight hosted validation checks pass on b65d80e, including Linux/Windows source checks, the Windows Builder/setup and renderer builds, and the Linux AppImage. Release publishing is skipped for the PR.

Reject a pre-existing symbolic link at `versions/<tag>.part` before stale-folder cleanup, part placement or download. Leave the link untouched and report the exact staging path through the existing update-error flow. Symlinked parent installation roots remain supported. Current main reaches cleanup or part placement before failing on these links. Pending #1327 canonicalizes the staging root in `_place`, so a dangling staging leaf can instead select its resolved target before the new ZIP checks. This is a separate leaf check; it does not duplicate or change #1327's archive containment rules. Thanks to @Demonicmurderer for [#1295](https://git.magicbean.dev/MagicBeanDev/Hookshot/issues/1295) and @bronze-ff for the related [#1327](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/1327). Validation: - Extended the existing Linux updater fixture. Identical assertions fail both link cases on main and pass after the fix; fresh staging, stale real staging, and a symlinked parent root pass throughout. - The strict selected Linux runner passes (one check, all 12 test methods). All 10 pre-existing methods are unchanged. - A separate authored temp-only probe of exact #1327 source reaches an injected unpack barrier with the resolved target; no outside-staging write is performed. The two-line guard overlaid on that source passes the same five new scenarios. The new regression uses temporary paths and doubles, with no network or real archive/installer execution. The existing Linux-only test scope is retained; no Windows symlink privilege is required. This does not establish archive integration coverage or protection against concurrent replacement of the staging leaf. All eight [hosted validation checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/1343/checks) pass on `b65d80e`, including Linux/Windows source checks, the Windows Builder/setup and renderer builds, and the Linux AppImage. Release publishing is skipped for the PR.

hckrman101 (Migrated from github.com) approved these changes

2026-11-09 23:08:03 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. The guard sits right before the isdir/rmtree, so a symlinked v2.part is rejected before cleanup, placement or download, and the ValueError goes out through the existing update-error path in hookshot_builder.py. Using islink (not isdir) also covers the dangling-link case, which main would otherwise walk straight into via _place's makedirs. CI is green, including the Linux and Windows source checks and the AppImage.

Overlap with #1327: complementary, not a duplicate. #1327 changes _unpack and _place (realpath check on the part directory, xb open); this only touches install() before any of that runs. The two diffs are in different hunks and the test additions are in different places in test_linux_update.py, so I don't expect a textual conflict. Behaviour-wise, #1327's _place check would also reject a symlinked staging leaf, just later and with a different message; with this guard in place it never gets that far, so the two don't fight. Worth landing both.

Nit, no action needed: the leaf check is still racy against something swapping the link in after the check, which the description already says it doesn't cover.

Looks good. The guard sits right before the isdir/rmtree, so a symlinked `v2.part` is rejected before cleanup, placement or download, and the ValueError goes out through the existing `update-error` path in hookshot_builder.py. Using `islink` (not `isdir`) also covers the dangling-link case, which main would otherwise walk straight into via `_place`'s makedirs. CI is green, including the Linux and Windows source checks and the AppImage. Overlap with #1327: complementary, not a duplicate. #1327 changes `_unpack` and `_place` (realpath check on the part directory, `xb` open); this only touches `install()` before any of that runs. The two diffs are in different hunks and the test additions are in different places in test_linux_update.py, so I don't expect a textual conflict. Behaviour-wise, #1327's `_place` check would also reject a symlinked staging leaf, just later and with a different message; with this guard in place it never gets that far, so the two don't fight. Worth landing both. Nit, no action needed: the leaf check is still racy against something swapping the link in after the check, which the description already says it doesn't cover.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101