From 08c7e706726a2e90e8362526eaf02ee19c87362b Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 01:35:16 +0500 Subject: [PATCH 1/6] 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 b84b836545..1b8067dc92 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1397,6 +1397,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) @@ -1404,6 +1424,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 2885180360..81e14f9703 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 e2116587faa620404d207f8d93cdd48a367be17d Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 22:07:55 +0500 Subject: [PATCH 2/6] 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 1b8067dc92..c4a049600e 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1404,7 +1404,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 ( @@ -1415,7 +1419,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(): @@ -1424,11 +1431,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 81e14f9703..ad06b809f9 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 4a885d7abb3cfbe3f9a1fc4033403d76e7d5cdec Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 9 Jul 2026 19:39:18 +0500 Subject: [PATCH 3/6] 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 c4a049600e..6906f5b860 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1404,11 +1404,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 ( @@ -1419,9 +1420,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) @@ -1432,14 +1436,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 ad06b809f9..355d59e514 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 83ef225632ca136bfbd4c4f18f8a0fc0f633b459 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 10 Jul 2026 21:40:16 +0500 Subject: [PATCH 4/6] 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 6906f5b860..3c5de2b092 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1441,9 +1441,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 355d59e514..a9beb10847 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 9b10572a54e2c3e6ed50fa45b2ad0bf3e41197cf Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 13 Jul 2026 22:28:11 +0500 Subject: [PATCH 5/6] 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 3c5de2b092..aa37c3c9e0 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1312,6 +1312,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, @@ -1428,43 +1466,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 a9beb10847..6d6be42312 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 61e4b7be10a85c207b7bf311a93a7056e015657f Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 14 Jul 2026 20:54:59 +0500 Subject: [PATCH 6/6] 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 aa37c3c9e0..292a6fec7e 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1467,42 +1467,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 6d6be42312..916f06f7be 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 =====