Skip to content

Commit 61189bc

Browse files
jawwad-aliclaude
andcommitted
fix(extensions): rollback atomically moves the backup dir instead of copytree
The rollback recreated dest_dir then copytree(..., dirs_exist_ok=True)'d the backup into it. copytree uses copy2, which follows a symlink at a config leaf — so a leaf raced into the recreated dest_dir could redirect a preserved secret to an external target (validating only the ancestor directory does not protect leaf paths). Instead, after _reset_dir(dest_dir) removes any occupant and the ancestor chain is verified non-symlink, os.replace() atomically MOVES the whole backup directory (which already holds real files written via _atomic_restore_config) into place. There is no per-leaf copy step and no window where dest_dir exists with a raced leaf, so rollback can no longer follow a leaf symlink. The consumed backup is set to None so cleanup skips it. Existing reinstall/rollback tests (copytree-failure, failed-restore, symlinked dest_dir, file-at-dest) all pass (361). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent df6441b commit 61189bc

1 file changed

Lines changed: 15 additions & 11 deletions

File tree

src/specify_cli/extensions/__init__.py

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1577,32 +1577,36 @@ def install_from_directory(
15771577
self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime)
15781578
restored_ok = True
15791579
except Exception:
1580-
# Roll the configs back into a clean, symlink-safe dest_dir from the
1581-
# durable backup so a failed reinstall leaves them intact, then
1582-
# re-raise the original error. Reset dest_dir (unlink a symlink /
1583-
# rmtree a real dir — surfacing failures, never ignore_errors) and
1584-
# recreate it under a verified non-symlink ancestor chain BEFORE
1585-
# copying, so recovery can never merge through a symlinked or
1586-
# partially-removed directory into an external target.
1580+
# Roll the configs back into place from the durable backup so a failed
1581+
# reinstall leaves them intact, then re-raise the original error.
1582+
# Reset dest_dir (unlink a symlink / rmtree a real dir — surfacing
1583+
# failures, never ignore_errors), verify its ancestor chain is not
1584+
# symlinked, then ATOMICALLY MOVE the whole backup directory into
1585+
# place with os.replace. The backup already holds real files written
1586+
# via _atomic_restore_config, so moving the directory wholesale avoids
1587+
# copytree's copy2 following a leaf symlink raced into the recreated
1588+
# dest_dir and writing a preserved secret to an external target.
15871589
if backup_dir is not None:
15881590
try:
15891591
from ..shared_infra import _ensure_safe_shared_directory
15901592
self._reset_dir(dest_dir)
15911593
_ensure_safe_shared_directory(
15921594
self.project_root,
1593-
dest_dir,
1594-
create=True,
1595+
dest_dir.parent,
1596+
create=False,
15951597
context="extension directory",
15961598
)
1597-
shutil.copytree(backup_dir, dest_dir, dirs_exist_ok=True)
1599+
os.replace(backup_dir, dest_dir)
1600+
backup_dir = None # consumed by the move; nothing to clean up
15981601
restored_ok = True
15991602
except (OSError, ValueError):
16001603
# ValueError covers SymlinkedSharedPathError (a symlinked
16011604
# ancestor): leave the durable backup on disk for recovery.
16021605
restored_ok = False
16031606
raise
16041607
finally:
1605-
# Drop the backup only when the configs are provably in dest_dir.
1608+
# Drop the backup only when the configs are provably in dest_dir
1609+
# (a successful move sets backup_dir to None, so nothing to clean).
16061610
if backup_dir is not None and restored_ok and backup_dir.exists():
16071611
shutil.rmtree(backup_dir, ignore_errors=True)
16081612

0 commit comments

Comments
 (0)