From ecbe606003ad3e331f12082f2012136b23e960eb Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 01:35:16 +0500 Subject: [PATCH 1/9] fix(extensions): preserve keep-config leftover on reinstall (data loss) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove(keep_config=True) intentionally leaves the top-level *-config.yml / *-config.local.yml in place and drops the registry entry. A subsequent install_from_directory is not a --force removal (did_remove is False), so the unconditional 'if dest_dir.exists(): shutil.rmtree(dest_dir)' wiped the preserved config and the backup-restore path never ran — silently destroying user configuration the feature promised to keep. Capture the top-level *-config.yml/*-config.local.yml from an existing dest_dir before the rmtree and write them back after the fresh copytree, mirroring the *-config filter the --force restore path already uses. Co-Authored-By: Claude Fable 5 --- src/specify_cli/extensions/__init__.py | 26 ++++++++++++++++++++++++++ tests/test_extensions.py | 23 +++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 05aa35f7fb..4d081a8f6f 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1428,6 +1428,26 @@ def install_from_directory( backup_config_dir.unlink() did_remove = self.remove(manifest.id) + # A prior `remove(keep_config=True)` leaves the top-level + # *-config.yml / *-config.local.yml in place while dropping the + # registry entry. A later reinstall is not a --force removal + # (did_remove is False), so the rmtree below would wipe those + # preserved configs and the backup-restore path never runs. Capture + # them here and write them back after the fresh copytree, mirroring + # the *-config filter the --force restore path uses. + preserved_configs: dict[str, bytes] = {} + if dest_dir.exists(): + for cfg_file in dest_dir.iterdir(): + if ( + cfg_file.is_file() + and not cfg_file.is_symlink() + and ( + cfg_file.name.endswith("-config.yml") + or cfg_file.name.endswith("-config.local.yml") + ) + ): + preserved_configs[cfg_file.name] = cfg_file.read_bytes() + # Install extension (dest_dir computed above during self-install guard) if dest_dir.exists(): shutil.rmtree(dest_dir) @@ -1435,6 +1455,12 @@ def install_from_directory( ignore_fn = self._load_extensionignore(source_dir) shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) + # Restore configs preserved from a keep-config leftover (see above). + # The --force backup-restore below (did_remove) still takes precedence + # for that path, which uses .backup/ rather than an in-place leftover. + for name, data in preserved_configs.items(): + (dest_dir / name).write_bytes(data) + # Register commands with AI agents registered_commands = {} if register_commands: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 652540d1be..d5c806b2eb 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1557,6 +1557,29 @@ def test_config_backup_on_remove(self, extension_dir, project_dir): assert backup_file.exists() assert backup_file.read_text() == "test: config" + def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_dir): + """A reinstall after `remove(keep_config=True)` must not wipe the + preserved config. remove(keep_config=True) leaves the *-config.yml in + place and drops the registry entry; the subsequent reinstall is not a + --force removal, so the unconditional rmtree used to destroy it.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + + # keep-config removal: registry entry dropped, config left in place. + assert manager.remove("test-ext", keep_config=True) is True + assert config_file.exists() + + # Reinstall (no --force; registry now reports not-installed). + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # The user's config must survive the reinstall (was silently wiped). + assert config_file.exists() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + # ===== CommandRegistrar Tests ===== From e99457fe189fdcc7b6a3b9153007269347aca362 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 22:07:55 +0500 Subject: [PATCH 2/9] fix(extensions): preserve config file mode on keep-config restore Per review: config files can hold secrets, so recreating them with write_bytes() (default perms) could widen access. Capture the original st_mode alongside the bytes and re-apply it (permission bits) after restore, matching the --force path's shutil.copy2 mode preservation. Co-Authored-By: Claude Fable 5 --- src/specify_cli/extensions/__init__.py | 27 +++++++++++++++++++------- tests/test_extensions.py | 6 ++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 4d081a8f6f..8e42d618b4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1435,7 +1435,11 @@ def install_from_directory( # preserved configs and the backup-restore path never runs. Capture # them here and write them back after the fresh copytree, mirroring # the *-config filter the --force restore path uses. - preserved_configs: dict[str, bytes] = {} + # Capture (bytes, mode) so we can restore permissions too — config + # files may hold secrets (API keys), and recreating them with default + # perms could widen access. Mirrors the --force restore's shutil.copy2 + # (which preserves mode/mtime). + preserved_configs: dict[str, tuple[bytes, int]] = {} if dest_dir.exists(): for cfg_file in dest_dir.iterdir(): if ( @@ -1446,7 +1450,10 @@ def install_from_directory( or cfg_file.name.endswith("-config.local.yml") ) ): - preserved_configs[cfg_file.name] = cfg_file.read_bytes() + preserved_configs[cfg_file.name] = ( + cfg_file.read_bytes(), + cfg_file.stat().st_mode, + ) # Install extension (dest_dir computed above during self-install guard) if dest_dir.exists(): @@ -1455,11 +1462,17 @@ def install_from_directory( ignore_fn = self._load_extensionignore(source_dir) shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) - # Restore configs preserved from a keep-config leftover (see above). - # The --force backup-restore below (did_remove) still takes precedence - # for that path, which uses .backup/ rather than an in-place leftover. - for name, data in preserved_configs.items(): - (dest_dir / name).write_bytes(data) + # Restore configs preserved from a keep-config leftover (see above), + # preserving the original file mode. The --force backup-restore below + # (did_remove) still takes precedence for that path, which uses + # .backup/ rather than an in-place leftover. + for name, (data, mode) in preserved_configs.items(): + dest_cfg = dest_dir / name + dest_cfg.write_bytes(data) + try: + os.chmod(dest_cfg, mode & 0o7777) # permission bits only + except OSError: + pass # Register commands with AI agents registered_commands = {} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index d5c806b2eb..cf5fba29ff 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1568,6 +1568,9 @@ def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_d ext_dir = project_dir / ".specify" / "extensions" / "test-ext" config_file = ext_dir / "test-ext-config.yml" config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + # Restrictive perms — a config may hold secrets; the restore must not + # widen them (POSIX only; Windows chmod ignores group/other bits). + config_file.chmod(0o600) # keep-config removal: registry entry dropped, config left in place. assert manager.remove("test-ext", keep_config=True) is True @@ -1579,6 +1582,9 @@ def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_d # The user's config must survive the reinstall (was silently wiped). assert config_file.exists() assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + # ...and keep its original (restrictive) mode, not default perms. + if platform.system() != "Windows": + assert config_file.stat().st_mode & 0o777 == 0o600 # ===== CommandRegistrar Tests ===== From 193c1246d592e106f65a1cc2ea2a02031d3eece2 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 9 Jul 2026 19:39:18 +0500 Subject: [PATCH 3/9] fix(extensions): preserve config mtime on keep-config restore (mirror copy2) Per review: the restore comment claimed it mirrors shutil.copy2 (mode + mtime) but only restored permission bits. Capture and re-apply the original atime/mtime via os.utime alongside the mode, so the keep-config leftover restore truly matches copy2's timestamp preservation. Co-Authored-By: Claude Fable 5 --- src/specify_cli/extensions/__init__.py | 25 +++++++++++++++---------- tests/test_extensions.py | 7 ++++++- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 8e42d618b4..56b17b0f69 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1435,11 +1435,12 @@ def install_from_directory( # preserved configs and the backup-restore path never runs. Capture # them here and write them back after the fresh copytree, mirroring # the *-config filter the --force restore path uses. - # Capture (bytes, mode) so we can restore permissions too — config - # files may hold secrets (API keys), and recreating them with default - # perms could widen access. Mirrors the --force restore's shutil.copy2 - # (which preserves mode/mtime). - preserved_configs: dict[str, tuple[bytes, int]] = {} + # Capture (bytes, mode, atime, mtime) so restore preserves both + # permissions and timestamps — truly mirroring the --force restore's + # shutil.copy2 (which preserves mode/mtime). Config files may hold + # secrets (API keys); recreating them with default perms could widen + # access. + preserved_configs: dict[str, tuple[bytes, int, float, float]] = {} if dest_dir.exists(): for cfg_file in dest_dir.iterdir(): if ( @@ -1450,9 +1451,12 @@ def install_from_directory( or cfg_file.name.endswith("-config.local.yml") ) ): + st = cfg_file.stat() preserved_configs[cfg_file.name] = ( cfg_file.read_bytes(), - cfg_file.stat().st_mode, + st.st_mode, + st.st_atime, + st.st_mtime, ) # Install extension (dest_dir computed above during self-install guard) @@ -1463,14 +1467,15 @@ def install_from_directory( shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) # Restore configs preserved from a keep-config leftover (see above), - # preserving the original file mode. The --force backup-restore below - # (did_remove) still takes precedence for that path, which uses - # .backup/ rather than an in-place leftover. - for name, (data, mode) in preserved_configs.items(): + # preserving the original file mode and timestamps. The --force + # backup-restore below (did_remove) still takes precedence for that + # path, which uses .backup/ rather than an in-place leftover. + for name, (data, mode, atime, mtime) in preserved_configs.items(): dest_cfg = dest_dir / name dest_cfg.write_bytes(data) try: os.chmod(dest_cfg, mode & 0o7777) # permission bits only + os.utime(dest_cfg, (atime, mtime)) # and timestamps except OSError: pass diff --git a/tests/test_extensions.py b/tests/test_extensions.py index cf5fba29ff..2e08bf6280 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1571,6 +1571,9 @@ def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_d # Restrictive perms — a config may hold secrets; the restore must not # widen them (POSIX only; Windows chmod ignores group/other bits). config_file.chmod(0o600) + # Distinctive mtime to prove timestamps survive too (mirrors copy2). + old_mtime = 1_600_000_000 + os.utime(config_file, (old_mtime, old_mtime)) # keep-config removal: registry entry dropped, config left in place. assert manager.remove("test-ext", keep_config=True) is True @@ -1582,7 +1585,9 @@ def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_d # The user's config must survive the reinstall (was silently wiped). assert config_file.exists() assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() - # ...and keep its original (restrictive) mode, not default perms. + # ...and keep its original mtime (copy2-style timestamp preservation). + assert config_file.stat().st_mtime == old_mtime + # ...and its original (restrictive) mode, not default perms. if platform.system() != "Windows": assert config_file.stat().st_mode & 0o777 == 0o600 From de5245108410ed4679d70d566b0da6c620a53b78 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 10 Jul 2026 21:40:16 +0500 Subject: [PATCH 4/9] fix(extensions): keep-config restore must not follow symlinks; split mode/time restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes to the reinstall keep-config restore loop: 1. Never write *through* a non-regular file at the config path. If dest_cfg is a symlink (copied with symlinks=True, or swapped in by a racing process between copytree and the restore), write_bytes() would follow it and clobber an arbitrary target outside the extension dir. Drop any symlink (unlink, never follow) or stray directory (rmtree) first, so write_bytes() always creates a fresh regular file — and skip the entry if the path can't be made safe rather than clobber. 2. Restore mode and mtime independently. The single try/except meant a chmod failure skipped os.utime (and vice versa); either can succeed on its own, so each gets its own guard. Tests: a symlink-at-path case (proves the write doesn't follow the link to an external victim; skipped where symlinks need privilege, runs on CI) and a directory-at-path case (cross-platform: fails/crashes before the guard, passes after). Behavior for the normal regular-file case is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/__init__.py | 18 +++++ tests/test_extensions.py | 92 ++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 56b17b0f69..64ce78187e 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1472,9 +1472,27 @@ def install_from_directory( # path, which uses .backup/ rather than an in-place leftover. for name, (data, mode, atime, mtime) in preserved_configs.items(): dest_cfg = dest_dir / name + # Never write *through* a symlink (or a non-regular-file) at this + # path: if dest_cfg is a symlink — copied with symlinks=True, or + # swapped in by a racing process between the copytree above and + # here — write_bytes() would follow it and clobber an arbitrary + # target outside the extension dir. Drop any non-regular entry + # first so write_bytes() always creates a fresh regular file. + try: + if dest_cfg.is_symlink(): + dest_cfg.unlink() # remove the link itself, never follow it + elif dest_cfg.is_dir(): + shutil.rmtree(dest_cfg) + except OSError: + continue # can't make the path safe — skip rather than clobber dest_cfg.write_bytes(data) + # Restore mode and timestamps independently: a failure to set one + # must not prevent the other, since either can succeed on its own. try: os.chmod(dest_cfg, mode & 0o7777) # permission bits only + except OSError: + pass + try: os.utime(dest_cfg, (atime, mtime)) # and timestamps except OSError: pass diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 2e08bf6280..b422de552e 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1591,6 +1591,98 @@ def test_reinstall_preserves_keep_config_leftover(self, extension_dir, project_d if platform.system() != "Windows": assert config_file.stat().st_mode & 0o777 == 0o600 + def test_reinstall_config_restore_does_not_follow_symlink( + self, extension_dir, project_dir, monkeypatch + ): + """The keep-config restore must never write *through* a symlink. If the + config path is a symlink at restore time (e.g. swapped in between the + copytree and the restore), the write must not follow it and clobber the + external target — it must replace the link with a fresh regular file.""" + # Probe symlink capability (Windows usually needs privilege). + probe_target = project_dir / "_probe_target" + probe_link = project_dir / "_probe_link" + try: + probe_target.write_text("x") + probe_link.symlink_to(probe_target) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported in this environment") + finally: + probe_link.unlink(missing_ok=True) + probe_target.unlink(missing_ok=True) + + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + + # keep-config removal: registry entry dropped, config left in place. + assert manager.remove("test-ext", keep_config=True) is True + + # A victim file outside the extension dir that the symlink points at. + victim = project_dir / "victim.txt" + victim.write_text("DO-NOT-OVERWRITE") + + # Simulate the config path being a symlink to the victim right after the + # fresh copytree (the TOCTOU / symlinks=True case the guard defends). + real_copytree = shutil.copytree + + def copytree_then_symlink(src, dst, *args, **kwargs): + result = real_copytree(src, dst, *args, **kwargs) + link = Path(dst) / "test-ext-config.yml" + if link.exists() or link.is_symlink(): + link.unlink() + link.symlink_to(victim) + return result + + monkeypatch.setattr(shutil, "copytree", copytree_then_symlink) + + # Reinstall: restore must drop the symlink, not follow it. + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # The victim outside the extension dir is untouched. + assert victim.read_text() == "DO-NOT-OVERWRITE" + # The config path is now a regular file (not a symlink) with the + # preserved content written in place. + assert not config_file.is_symlink() + assert config_file.is_file() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + + def test_reinstall_config_restore_replaces_directory_at_path( + self, extension_dir, project_dir, monkeypatch + ): + """Cross-platform guard check: if a directory occupies the config path at + restore time, write_bytes() would raise (uncaught, crashing the whole + reinstall). The guard must clear the directory and write a fresh regular + file. Exercises the same non-regular-file guard as the symlink case, but + needs no symlink privilege so it runs everywhere (incl. Windows).""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + assert manager.remove("test-ext", keep_config=True) is True + + # Simulate a directory occupying the config path right after copytree. + real_copytree = shutil.copytree + + def copytree_then_dir(src, dst, *args, **kwargs): + result = real_copytree(src, dst, *args, **kwargs) + clash = Path(dst) / "test-ext-config.yml" + clash.mkdir() + (clash / "sentinel").write_text("stray") + return result + + monkeypatch.setattr(shutil, "copytree", copytree_then_dir) + + # Must not raise, and must restore a regular file with preserved content. + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + assert config_file.is_file() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + # ===== CommandRegistrar Tests ===== From fd70be4e3ae7e7afcd36540fab3ecc19267ff4b7 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 13 Jul 2026 22:28:11 +0500 Subject: [PATCH 5/9] fix(extensions): atomic config restore (os.replace) + recover config if copytree fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardens the keep-config restore against the two remaining data-loss/race windows: 1. Atomic restore: write each preserved config to a sibling temp file and os.replace() it into place (new _atomic_restore_config, mirroring shared_infra._write_shared_bytes). os.replace swaps the directory entry, so it never follows a symlink that may occupy the path — closing the check-then-use race the previous is_symlink() guard could not — and never leaves a partial write. Replaces the guard-then-write_bytes block. 2. Failure recovery: the only copy of the preserved configs after rmtree is in memory, so if copytree fails (permissions, disk full, bad source entry) the config was permanently lost. Wrap copytree; on failure, write the configs back into a clean dest_dir before re-raising, so a failed reinstall keeps the user's config. Tests: config survives a simulated copytree failure (fails before: file gone), plus the existing symlink-at-path (Linux CI) and directory-at-path (cross-platform) cases still pass under the atomic path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/__init__.py | 90 ++++++++++++++++++-------- tests/test_extensions.py | 32 +++++++++ 2 files changed, 96 insertions(+), 26 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 64ce78187e..2de164e2b6 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1343,6 +1343,44 @@ def check_compatibility( return True + @staticmethod + def _atomic_restore_config( + dest: Path, data: bytes, mode: int, atime: float, mtime: float + ) -> None: + """Restore a preserved config to *dest* atomically (temp file + os.replace). + + Writes to a sibling temp file, then ``os.replace()`` swaps it into place. + ``os.replace`` operates on the directory entry, so it (a) never follows a + symlink that may occupy *dest* — closing the check-then-use race where a + racing process swaps in a symlink to overwrite an external target — and + (b) leaves either the old file or the fully-written new one, never a + partial write. Mirrors ``shared_infra._write_shared_bytes``. Mode and + timestamps are set on the temp file (independently — either can succeed + on its own) so the installed file carries them from the first moment. + """ + # os.replace cannot replace a directory with a file; clear a stray + # (non-symlink) directory at the path first. A symlink is handled by + # os.replace itself (it swaps the link entry without following it). + if dest.is_dir() and not dest.is_symlink(): + shutil.rmtree(dest, ignore_errors=True) + fd, temp_name = tempfile.mkstemp(prefix=f".{dest.name}.", dir=dest.parent) + temp_path = Path(temp_name) + try: + with os.fdopen(fd, "wb") as fh: + fh.write(data) + try: + temp_path.chmod(mode & 0o7777) # permission bits only + except OSError: + pass + try: + os.utime(temp_path, (atime, mtime)) + except OSError: + pass + os.replace(temp_path, dest) + finally: + if temp_path.exists(): + temp_path.unlink() + def install_from_directory( self, source_dir: Path, @@ -1459,43 +1497,43 @@ def install_from_directory( st.st_mtime, ) - # Install extension (dest_dir computed above during self-install guard) + # Install extension (dest_dir computed above during self-install guard). + # rmtree removes the only on-disk copy of the preserved configs; the sole + # remaining copy is in `preserved_configs` (in memory). If copytree then + # fails (permissions, disk full, a bad source entry), write the configs + # back into a clean dest_dir before re-raising so a failed reinstall never + # loses the user's config. if dest_dir.exists(): shutil.rmtree(dest_dir) ignore_fn = self._load_extensionignore(source_dir) - shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) + try: + shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) + except Exception: + if preserved_configs: + if dest_dir.exists(): + shutil.rmtree(dest_dir, ignore_errors=True) + dest_dir.mkdir(parents=True, exist_ok=True) + for name, (data, mode, atime, mtime) in preserved_configs.items(): + try: + self._atomic_restore_config( + dest_dir / name, data, mode, atime, mtime + ) + except OSError: + pass + raise # Restore configs preserved from a keep-config leftover (see above), # preserving the original file mode and timestamps. The --force # backup-restore below (did_remove) still takes precedence for that - # path, which uses .backup/ rather than an in-place leftover. + # path, which uses .backup/ rather than an in-place leftover. Each write + # is atomic (temp file + os.replace) so it never follows a symlink that + # may occupy the path (see _atomic_restore_config). for name, (data, mode, atime, mtime) in preserved_configs.items(): - dest_cfg = dest_dir / name - # Never write *through* a symlink (or a non-regular-file) at this - # path: if dest_cfg is a symlink — copied with symlinks=True, or - # swapped in by a racing process between the copytree above and - # here — write_bytes() would follow it and clobber an arbitrary - # target outside the extension dir. Drop any non-regular entry - # first so write_bytes() always creates a fresh regular file. - try: - if dest_cfg.is_symlink(): - dest_cfg.unlink() # remove the link itself, never follow it - elif dest_cfg.is_dir(): - shutil.rmtree(dest_cfg) - except OSError: - continue # can't make the path safe — skip rather than clobber - dest_cfg.write_bytes(data) - # Restore mode and timestamps independently: a failure to set one - # must not prevent the other, since either can succeed on its own. try: - os.chmod(dest_cfg, mode & 0o7777) # permission bits only + self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime) except OSError: - pass - try: - os.utime(dest_cfg, (atime, mtime)) # and timestamps - except OSError: - pass + continue # can't safely restore this one — skip rather than clobber # Register commands with AI agents registered_commands = {} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index b422de552e..210e41626a 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1683,6 +1683,38 @@ def copytree_then_dir(src, dst, *args, **kwargs): assert config_file.is_file() assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + def test_reinstall_config_survives_copytree_failure( + self, extension_dir, project_dir, monkeypatch + ): + """The preserved config is captured in memory and the old dir is removed + before copytree runs. If copytree fails, the config must be written back + (not lost) so a failed reinstall leaves the user's config intact.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + config_file.chmod(0o600) + assert manager.remove("test-ext", keep_config=True) is True + + # Simulate a mid-reinstall failure after the old dir was removed. + def boom(src, dst, *args, **kwargs): + raise OSError("simulated disk-full during copytree") + + monkeypatch.setattr(shutil, "copytree", boom) + + with pytest.raises(OSError): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Config recovered despite the failure (was permanently lost before). + assert config_file.is_file() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + if platform.system() != "Windows": + assert config_file.stat().st_mode & 0o777 == 0o600 + # ===== CommandRegistrar Tests ===== From 2853b0b4c439ec97710a15e58abc5d5cbf0bc3bd Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 14 Jul 2026 20:54:59 +0500 Subject: [PATCH 6/9] fix(extensions): make keep-config reinstall fully transactional (no silent loss) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two remaining data-loss windows in the keep-config restore: 1. Validate the source ignore file BEFORE deleting anything. _load_extensionignore() can raise (e.g. invalid UTF-8); it ran after rmtree and outside the recovery block, so a malformed ignore file lost the preserved config. It's now computed before any destructive step. 2. Stage a durable on-disk backup of the configs and cover rmtree + copytree + every restore in one guarded block. A restore failure is no longer silently skipped (which reported a successful install with the config missing) — on any failure the configs are rolled back into a clean dest_dir from the backup and the error re-raised. The backup is removed only once the configs are provably in place (normal success OR a completed rollback), so even a rollback failure leaves them recoverable on disk. Tests: ignore-file validation failure leaves the config intact (fails before: it was deleted first); a failed restore raises instead of silently succeeding and the config is rolled back (fails before: DID NOT RAISE). Existing copytree-failure test updated to fail only the source copy so the backup rollback can be observed. Full test_extensions.py (340) passes. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/__init__.py | 77 +++++++++++++++----------- tests/test_extensions.py | 75 ++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 33 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2de164e2b6..f6b508e7b4 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1498,42 +1498,57 @@ def install_from_directory( ) # Install extension (dest_dir computed above during self-install guard). - # rmtree removes the only on-disk copy of the preserved configs; the sole - # remaining copy is in `preserved_configs` (in memory). If copytree then - # fails (permissions, disk full, a bad source entry), write the configs - # back into a clean dest_dir before re-raising so a failed reinstall never - # loses the user's config. - if dest_dir.exists(): - shutil.rmtree(dest_dir) - + # Preserved configs are the ONLY surviving copy once the old extension dir + # is removed, so guard the whole destructive reinstall against loss at any + # step (a malformed source ignore file, the rmtree, copytree, or a + # restore): + # 1. Validate the source ignore file BEFORE deleting anything — it can + # raise on a malformed file, and nothing is destroyed yet if it does. + # 2. Stage a durable on-disk backup of the configs. + # 3. Run the destructive rmtree + copytree + restore inside one block; + # on ANY failure, roll the configs back into a clean dest_dir and + # re-raise. The backup is removed only once the configs are provably + # in place (normal success OR a completed rollback) — otherwise it is + # left on disk so nothing is ever lost, even on a rollback failure. + # Each config write is atomic (temp file + os.replace) so it never follows + # a symlink that may occupy the path (see _atomic_restore_config). ignore_fn = self._load_extensionignore(source_dir) + + backup_dir: Path | None = None + if preserved_configs: + backup_dir = Path( + tempfile.mkdtemp(prefix=f".{dest_dir.name}.cfgbak-", dir=dest_dir.parent) + ) + for name, (data, mode, atime, mtime) in preserved_configs.items(): + self._atomic_restore_config(backup_dir / name, data, mode, atime, mtime) + + restored_ok = not preserved_configs try: + if dest_dir.exists(): + shutil.rmtree(dest_dir) shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) + # Restore every preserved config. A failure here is NOT swallowed: a + # config we promised to preserve but could not restore must fail the + # reinstall, not report success with the config silently missing. + for name, (data, mode, atime, mtime) in preserved_configs.items(): + self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime) + restored_ok = True except Exception: - if preserved_configs: - if dest_dir.exists(): - shutil.rmtree(dest_dir, ignore_errors=True) - dest_dir.mkdir(parents=True, exist_ok=True) - for name, (data, mode, atime, mtime) in preserved_configs.items(): - try: - self._atomic_restore_config( - dest_dir / name, data, mode, atime, mtime - ) - except OSError: - pass + # Roll the configs back into a clean dest_dir from the durable backup + # so a failed reinstall leaves them intact, then re-raise the original. + if backup_dir is not None: + try: + if dest_dir.exists(): + shutil.rmtree(dest_dir, ignore_errors=True) + shutil.copytree(backup_dir, dest_dir, dirs_exist_ok=True) + restored_ok = True + except OSError: + restored_ok = False # keep the backup below for recovery raise - - # Restore configs preserved from a keep-config leftover (see above), - # preserving the original file mode and timestamps. The --force - # backup-restore below (did_remove) still takes precedence for that - # path, which uses .backup/ rather than an in-place leftover. Each write - # is atomic (temp file + os.replace) so it never follows a symlink that - # may occupy the path (see _atomic_restore_config). - for name, (data, mode, atime, mtime) in preserved_configs.items(): - try: - self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime) - except OSError: - continue # can't safely restore this one — skip rather than clobber + finally: + # Drop the backup only when the configs are provably in dest_dir. + if backup_dir is not None and restored_ok and backup_dir.exists(): + shutil.rmtree(backup_dir, ignore_errors=True) # Register commands with AI agents registered_commands = {} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 210e41626a..dfa4510101 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1698,9 +1698,16 @@ def test_reinstall_config_survives_copytree_failure( config_file.chmod(0o600) assert manager.remove("test-ext", keep_config=True) is True - # Simulate a mid-reinstall failure after the old dir was removed. + # Fail the source→dest copytree (after the old dir is removed) but let the + # rollback copytree (backup→dest) succeed, so recovery can be observed. + real_copytree = shutil.copytree + calls = {"n": 0} + def boom(src, dst, *args, **kwargs): - raise OSError("simulated disk-full during copytree") + calls["n"] += 1 + if calls["n"] == 1: + raise OSError("simulated disk-full during copytree") + return real_copytree(src, dst, *args, **kwargs) monkeypatch.setattr(shutil, "copytree", boom) @@ -1715,6 +1722,70 @@ def boom(src, dst, *args, **kwargs): if platform.system() != "Windows": assert config_file.stat().st_mode & 0o777 == 0o600 + def test_reinstall_validates_ignore_before_deleting_config( + self, extension_dir, project_dir, monkeypatch + ): + """A malformed source ignore file must be detected BEFORE the destructive + rmtree, so a reinstall that fails on it leaves the preserved config + untouched (it used to be loaded only after rmtree had run).""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + assert manager.remove("test-ext", keep_config=True) is True + + def bad_ignore(src): + raise ValueError("malformed .extensionignore") + + monkeypatch.setattr(manager, "_load_extensionignore", bad_ignore) + + with pytest.raises(ValueError): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Nothing was deleted — the leftover config is still in place. + assert config_file.is_file() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + + def test_reinstall_failed_config_restore_is_not_silent( + self, extension_dir, project_dir, monkeypatch + ): + """A restore failure after copytree must NOT be silently skipped and + reported as a successful install — it fails loudly, and the config is + rolled back from the durable backup rather than left missing.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: MY-CUSTOMIZED-VALUE") + assert manager.remove("test-ext", keep_config=True) is True + + # Fail the live restore into the extension dir, but let the durable-backup + # staging (a ".cfgbak-" temp dir) succeed so rollback can recover. + real_restore = ExtensionManager._atomic_restore_config + + def flaky_restore(dest, data, mode, atime, mtime): + if "cfgbak" not in dest.parent.name: + raise OSError("simulated restore failure") + return real_restore(dest, data, mode, atime, mtime) + + monkeypatch.setattr( + ExtensionManager, "_atomic_restore_config", staticmethod(flaky_restore) + ) + + with pytest.raises(OSError): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Not a silent success: the config was rolled back, not lost. + assert config_file.is_file() + assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + # ===== CommandRegistrar Tests ===== From 621af2a692be45c2eaba852dd5f319a71b282472 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 15 Jul 2026 10:14:12 +0500 Subject: [PATCH 7/9] fix(extensions): validate ignore before --force removal; reject symlinked ancestors in config restore Three review-driven hardening fixes to the keep-config reinstall, adopting the repo's own safe-write contract (shared_infra) instead of leaf-only guards: 1. Validate the source ignore file BEFORE the --force self.remove(). It was loaded only after removal, so a malformed/invalid-UTF-8 ignore file uninstalled a working extension (files + registry entry) before validation raised. Moved the single _load_extensionignore call above the force-removal block. 2. Reject a symlinked ANCESTOR of the config destination. _atomic_restore_config's mkstemp(dir=dest.parent)+os.replace operated inside dest.parent, so if dest_dir was swapped to a symlink after copytree, config bytes (secrets) were created in and replaced into the external target. It now calls the repo's shared_infra._ensure_safe_shared_directory to refuse any symlinked ancestor under the project root before writing. 3. Rollback no longer merges through a symlinked/partial dir. It reset dest_dir with rmtree(ignore_errors=True) then copytree(dirs_exist_ok=True), which could follow a symlink. It now resets via a new _reset_dir (unlink a symlink / rmtree a real dir, surfacing failures) and recreates dest_dir under a verified non-symlink ancestor chain before copying; a rejected ancestor leaves the durable backup on disk. Tests: --force reinstall with a malformed ignore leaves the extension installed (fails before: it was removed); a symlinked dest_dir is refused and never receives config bytes, rollback recreating a real dir (skipped without symlink privilege, runs on CI). Existing reinstall/keep-config tests unchanged (341 pass). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/__init__.py | 114 ++++++++++++++++++------- tests/test_extensions.py | 90 ++++++++++++++++++- 2 files changed, 169 insertions(+), 35 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index f6b508e7b4..f6b7410058 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1344,23 +1344,56 @@ def check_compatibility( return True @staticmethod + def _reset_dir(path: Path) -> None: + """Remove whatever occupies *path* so a fresh directory can take its place. + + A symlink is unlinked (never followed); a real directory is rmtree'd; any + other entry is unlinked. Unlike ``rmtree(..., ignore_errors=True)`` this + surfaces failures so a caller never merges into a partially-removed or + still-symlinked path. + """ + if path.is_symlink(): + path.unlink() + elif path.is_dir(): + shutil.rmtree(path) + elif path.exists(): + path.unlink() + def _atomic_restore_config( - dest: Path, data: bytes, mode: int, atime: float, mtime: float + self, dest: Path, data: bytes, mode: int, atime: float, mtime: float ) -> None: - """Restore a preserved config to *dest* atomically (temp file + os.replace). + """Restore a preserved config to *dest* atomically and symlink-safely. + + Two guards, both required because *dest*'s parent is a fresh copytree + output that a racing process could swap for a symlink: + + 1. Reject a symlinked ancestor of *dest* (``dest.parent`` and up, under + the project root) via the repo's shared safe-write guard + (``shared_infra._ensure_safe_shared_directory``). Without this, + ``mkstemp(dir=dest.parent)`` + ``os.replace`` would create and install + the config (possibly secrets) *inside a symlinked external target*. + 2. Write to a sibling temp file, then ``os.replace`` swaps it into the + directory entry — never following a symlink that occupies the leaf + *dest*, and never leaving a partial write. + + Mode/timestamps are set on the temp file (independently — either can + succeed alone) so the installed file carries them from the first moment. + """ + from ..shared_infra import _ensure_safe_shared_directory + + # (1) Refuse a symlinked ancestor of the destination. dest.parent must + # already exist (a fresh copytree dir, the backup dir, or a rollback dir + # recreated safely just above the call). + _ensure_safe_shared_directory( + self.project_root, + dest.parent, + create=False, + context="extension config directory", + ) - Writes to a sibling temp file, then ``os.replace()`` swaps it into place. - ``os.replace`` operates on the directory entry, so it (a) never follows a - symlink that may occupy *dest* — closing the check-then-use race where a - racing process swaps in a symlink to overwrite an external target — and - (b) leaves either the old file or the fully-written new one, never a - partial write. Mirrors ``shared_infra._write_shared_bytes``. Mode and - timestamps are set on the temp file (independently — either can succeed - on its own) so the installed file carries them from the first moment. - """ # os.replace cannot replace a directory with a file; clear a stray - # (non-symlink) directory at the path first. A symlink is handled by - # os.replace itself (it swaps the link entry without following it). + # (non-symlink) directory at the leaf first. A symlink at the leaf is + # handled by os.replace itself (it swaps the link entry, never follows). if dest.is_dir() and not dest.is_symlink(): shutil.rmtree(dest, ignore_errors=True) fd, temp_name = tempfile.mkstemp(prefix=f".{dest.name}.", dir=dest.parent) @@ -1448,6 +1481,13 @@ def install_from_directory( f"extension. Install from a copy in a different location instead." ) + # Validate the source ignore file BEFORE any destructive step. It can + # raise on a malformed/invalid-UTF-8 file, and neither the --force + # self.remove() below nor the later rmtree must run if it does — otherwise + # a bad ignore file would uninstall a working extension (and drop its + # registry entry) before validation fails. Reused as-is at the copytree. + ignore_fn = self._load_extensionignore(source_dir) + # Remove existing installation AFTER all validations pass so that a # validation failure doesn't leave the user with a half-uninstalled # extension (configs stranded in .backup/). @@ -1499,21 +1539,18 @@ def install_from_directory( # Install extension (dest_dir computed above during self-install guard). # Preserved configs are the ONLY surviving copy once the old extension dir - # is removed, so guard the whole destructive reinstall against loss at any - # step (a malformed source ignore file, the rmtree, copytree, or a - # restore): - # 1. Validate the source ignore file BEFORE deleting anything — it can - # raise on a malformed file, and nothing is destroyed yet if it does. + # is removed, so guard the whole destructive reinstall against loss: + # 1. The source ignore file is already validated above (before any + # destructive step), so a malformed one never reaches here. # 2. Stage a durable on-disk backup of the configs. # 3. Run the destructive rmtree + copytree + restore inside one block; - # on ANY failure, roll the configs back into a clean dest_dir and - # re-raise. The backup is removed only once the configs are provably - # in place (normal success OR a completed rollback) — otherwise it is - # left on disk so nothing is ever lost, even on a rollback failure. - # Each config write is atomic (temp file + os.replace) so it never follows - # a symlink that may occupy the path (see _atomic_restore_config). - ignore_fn = self._load_extensionignore(source_dir) - + # on ANY failure, reset dest_dir to a clean, symlink-safe directory + # and roll the configs back, then re-raise. The backup is removed + # only once the configs are provably in place (normal success OR a + # completed rollback) — otherwise it is left on disk so nothing is + # ever lost, even on a rollback failure. + # Each config write is atomic (temp file + os.replace) AND refuses a + # symlinked ancestor of the destination (see _atomic_restore_config). backup_dir: Path | None = None if preserved_configs: backup_dir = Path( @@ -1534,16 +1571,29 @@ def install_from_directory( self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime) restored_ok = True except Exception: - # Roll the configs back into a clean dest_dir from the durable backup - # so a failed reinstall leaves them intact, then re-raise the original. + # Roll the configs back into a clean, symlink-safe dest_dir from the + # durable backup so a failed reinstall leaves them intact, then + # re-raise the original error. Reset dest_dir (unlink a symlink / + # rmtree a real dir — surfacing failures, never ignore_errors) and + # recreate it under a verified non-symlink ancestor chain BEFORE + # copying, so recovery can never merge through a symlinked or + # partially-removed directory into an external target. if backup_dir is not None: try: - if dest_dir.exists(): - shutil.rmtree(dest_dir, ignore_errors=True) + from ..shared_infra import _ensure_safe_shared_directory + self._reset_dir(dest_dir) + _ensure_safe_shared_directory( + self.project_root, + dest_dir, + create=True, + context="extension directory", + ) shutil.copytree(backup_dir, dest_dir, dirs_exist_ok=True) restored_ok = True - except OSError: - restored_ok = False # keep the backup below for recovery + except (OSError, ValueError): + # ValueError covers SymlinkedSharedPathError (a symlinked + # ancestor): leave the durable backup on disk for recovery. + restored_ok = False raise finally: # Drop the backup only when the configs are provably in dest_dir. diff --git a/tests/test_extensions.py b/tests/test_extensions.py index dfa4510101..c368fe58e9 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1768,13 +1768,13 @@ def test_reinstall_failed_config_restore_is_not_silent( # staging (a ".cfgbak-" temp dir) succeed so rollback can recover. real_restore = ExtensionManager._atomic_restore_config - def flaky_restore(dest, data, mode, atime, mtime): + def flaky_restore(self, dest, data, mode, atime, mtime): if "cfgbak" not in dest.parent.name: raise OSError("simulated restore failure") - return real_restore(dest, data, mode, atime, mtime) + return real_restore(self, dest, data, mode, atime, mtime) monkeypatch.setattr( - ExtensionManager, "_atomic_restore_config", staticmethod(flaky_restore) + ExtensionManager, "_atomic_restore_config", flaky_restore ) with pytest.raises(OSError): @@ -1786,6 +1786,90 @@ def flaky_restore(dest, data, mode, atime, mtime): assert config_file.is_file() assert "MY-CUSTOMIZED-VALUE" in config_file.read_text() + def test_reinstall_force_validates_ignore_before_removal( + self, extension_dir, project_dir, monkeypatch + ): + """A --force reinstall must validate the source ignore file BEFORE + self.remove() runs. Otherwise a malformed ignore file uninstalls a + working extension (dropping its files and registry entry) before the + validation raises. (The leftover-path test covers the non-force case.)""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + assert manager.registry.is_installed("test-ext") + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + manifest_file = ext_dir / "extension.yml" + assert manifest_file.is_file() + + def bad_ignore(src): + raise ValueError("malformed .extensionignore") + + monkeypatch.setattr(manager, "_load_extensionignore", bad_ignore) + + with pytest.raises(ValueError): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False, force=True + ) + + # The working extension was NOT removed by the failed --force reinstall. + assert manager.registry.is_installed("test-ext") + assert manifest_file.is_file() + + def test_reinstall_refuses_symlinked_dest_dir( + self, extension_dir, project_dir, monkeypatch + ): + """If dest_dir is swapped to a symlink after copytree, the config restore + must NOT create bytes (potential secrets) inside the symlinked external + target — the repo's ancestor guard rejects it, and rollback recreates a + real directory. Guards the symlinked-parent gap (leaf-only guards miss + it). Skipped where symlinks need privilege; runs on Linux CI.""" + probe_t = project_dir / "_pt" + probe_l = project_dir / "_pl" + try: + probe_t.mkdir() + probe_l.symlink_to(probe_t, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported in this environment") + finally: + if probe_l.is_symlink(): + probe_l.unlink() + if probe_t.exists(): + shutil.rmtree(probe_t, ignore_errors=True) + + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + config_file = ext_dir / "test-ext-config.yml" + config_file.write_text("api_key: SECRET-VALUE") + assert manager.remove("test-ext", keep_config=True) is True + + victim = project_dir / "victim_target" + victim.mkdir() + + real_copytree = shutil.copytree + calls = {"n": 0} + + def copytree_symlinks_destdir_first(src, dst, *args, **kwargs): + calls["n"] += 1 + if calls["n"] == 1: + # Simulate a racing swap: dest_dir is a symlink to an external dir. + Path(dst).symlink_to(victim, target_is_directory=True) + return dst + return real_copytree(src, dst, *args, **kwargs) + + monkeypatch.setattr(shutil, "copytree", copytree_symlinks_destdir_first) + + with pytest.raises(Exception): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + # Secrets never landed in the external target. + assert list(victim.iterdir()) == [] + # dest_dir is a real directory again (rollback), with the config recovered. + assert not ext_dir.is_symlink() + assert config_file.is_file() + assert "SECRET-VALUE" in config_file.read_text() + # ===== CommandRegistrar Tests ===== From df6441b8067781e3da46196727699e467c4ae319 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 17 Jul 2026 15:33:04 +0500 Subject: [PATCH 8/9] fix(extensions): preserve-capture only from a real dir; reset non-dir dest_dir in main path Two more symlink-safety fixes to the keep-config reinstall: 1. The preserved-config capture guarded on dest_dir.exists(), which is true for a symlink; iterating a symlinked dest_dir would follow the link and read arbitrary external files into preserved_configs. Require a real directory (is_dir() and not is_symlink()) so only genuine on-disk leftover configs are captured. 2. The main install path cleared dest_dir with shutil.rmtree(dest_dir), which raises on a symlink or file (NotADirectoryError) and didn't benefit from the _reset_dir behavior already used in rollback. Use self._reset_dir(dest_dir) so an unexpected occupant (symlink/file) is handled consistently instead of crashing on path type. Tests: a stray FILE at dest_dir is replaced cleanly (cross-platform; fails before with NotADirectoryError), and the capture ignores a symlinked dest_dir without reading/overwriting external files (skipped without symlink privilege, runs on CI). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/extensions/__init__.py | 12 ++++-- tests/test_extensions.py | 57 ++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index f6b7410058..2e16c23009 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1519,7 +1519,11 @@ def install_from_directory( # secrets (API keys); recreating them with default perms could widen # access. preserved_configs: dict[str, tuple[bytes, int, float, float]] = {} - if dest_dir.exists(): + # Require a REAL directory, not a symlink: iterating a symlinked dest_dir + # would follow the link and read arbitrary external files into + # preserved_configs. This path only ever legitimately captures on-disk + # leftover configs inside the real extension directory. + if dest_dir.is_dir() and not dest_dir.is_symlink(): for cfg_file in dest_dir.iterdir(): if ( cfg_file.is_file() @@ -1561,8 +1565,10 @@ def install_from_directory( restored_ok = not preserved_configs try: - if dest_dir.exists(): - shutil.rmtree(dest_dir) + # Reset via _reset_dir (unlink a symlink / rmtree a real dir) rather + # than a bare rmtree, which raises on a symlink or file at dest_dir. + # Handles an unexpected occupant consistently with the rollback path. + self._reset_dir(dest_dir) shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) # Restore every preserved config. A failure here is NOT swallowed: a # config we promised to preserve but could not restore must fail the diff --git a/tests/test_extensions.py b/tests/test_extensions.py index c368fe58e9..6a2fc50bdd 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1870,6 +1870,63 @@ def copytree_symlinks_destdir_first(src, dst, *args, **kwargs): assert config_file.is_file() assert "SECRET-VALUE" in config_file.read_text() + def test_reinstall_replaces_file_at_dest_dir(self, extension_dir, project_dir): + """If a non-directory (a stray file) occupies dest_dir at reinstall, the + main install path clears it via _reset_dir rather than crashing on + shutil.rmtree (which raises NotADirectoryError on a file).""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + # Drop the registry entry, then replace the extension dir with a file. + assert manager.remove("test-ext", keep_config=True) is True + shutil.rmtree(ext_dir) + ext_dir.write_text("not a directory") + assert ext_dir.is_file() + + # Reinstall must succeed, replacing the file with a real extension dir. + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + assert ext_dir.is_dir() and not ext_dir.is_symlink() + assert (ext_dir / "extension.yml").is_file() + + def test_preserve_capture_ignores_symlinked_dest_dir( + self, extension_dir, project_dir + ): + """The preserve-config capture must not follow a symlinked dest_dir and + read external files into preserved_configs. Skipped without symlink + privilege; runs on Linux CI.""" + probe_t = project_dir / "_pt2" + probe_l = project_dir / "_pl2" + try: + probe_t.mkdir() + probe_l.symlink_to(probe_t, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported in this environment") + finally: + if probe_l.is_symlink(): + probe_l.unlink() + if probe_t.exists(): + shutil.rmtree(probe_t, ignore_errors=True) + + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + assert manager.remove("test-ext", keep_config=True) is True + + # External dir with a config-looking file that must NOT be captured. + external = project_dir / "external_configs" + external.mkdir() + (external / "test-ext-config.yml").write_text("api_key: EXTERNAL-SECRET") + # Swap dest_dir for a symlink to the external dir. + shutil.rmtree(ext_dir) + ext_dir.symlink_to(external, target_is_directory=True) + + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # The external secret was never captured, restored, or overwritten. + assert (external / "test-ext-config.yml").read_text() == "api_key: EXTERNAL-SECRET" + assert ext_dir.is_dir() and not ext_dir.is_symlink() + assert not (ext_dir / "test-ext-config.yml").exists() + # ===== CommandRegistrar Tests ===== From 61189bcce90954e516971551284ca5c17612c3a2 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 19 Jul 2026 02:09:25 +0500 Subject: [PATCH 9/9] fix(extensions): rollback atomically moves the backup dir instead of copytree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/specify_cli/extensions/__init__.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2e16c23009..37d88b134b 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1577,24 +1577,27 @@ def install_from_directory( self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime) restored_ok = True except Exception: - # Roll the configs back into a clean, symlink-safe dest_dir from the - # durable backup so a failed reinstall leaves them intact, then - # re-raise the original error. Reset dest_dir (unlink a symlink / - # rmtree a real dir — surfacing failures, never ignore_errors) and - # recreate it under a verified non-symlink ancestor chain BEFORE - # copying, so recovery can never merge through a symlinked or - # partially-removed directory into an external target. + # Roll the configs back into place from the durable backup so a failed + # reinstall leaves them intact, then re-raise the original error. + # Reset dest_dir (unlink a symlink / rmtree a real dir — surfacing + # failures, never ignore_errors), verify its ancestor chain is not + # symlinked, then ATOMICALLY MOVE the whole backup directory into + # place with os.replace. The backup already holds real files written + # via _atomic_restore_config, so moving the directory wholesale avoids + # copytree's copy2 following a leaf symlink raced into the recreated + # dest_dir and writing a preserved secret to an external target. if backup_dir is not None: try: from ..shared_infra import _ensure_safe_shared_directory self._reset_dir(dest_dir) _ensure_safe_shared_directory( self.project_root, - dest_dir, - create=True, + dest_dir.parent, + create=False, context="extension directory", ) - shutil.copytree(backup_dir, dest_dir, dirs_exist_ok=True) + os.replace(backup_dir, dest_dir) + backup_dir = None # consumed by the move; nothing to clean up restored_ok = True except (OSError, ValueError): # ValueError covers SymlinkedSharedPathError (a symlinked @@ -1602,7 +1605,8 @@ def install_from_directory( restored_ok = False raise finally: - # Drop the backup only when the configs are provably in dest_dir. + # Drop the backup only when the configs are provably in dest_dir + # (a successful move sets backup_dir to None, so nothing to clean). if backup_dir is not None and restored_ok and backup_dir.exists(): shutil.rmtree(backup_dir, ignore_errors=True)