diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b84b836545..3c5de2b092 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1397,6 +1397,37 @@ 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. + # 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 ( + 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") + ) + ): + st = cfg_file.stat() + preserved_configs[cfg_file.name] = ( + cfg_file.read_bytes(), + st.st_mode, + st.st_atime, + st.st_mtime, + ) + # Install extension (dest_dir computed above during self-install guard) if dest_dir.exists(): shutil.rmtree(dest_dir) @@ -1404,6 +1435,37 @@ 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), + # 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 + # 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 + # Register commands with AI agents registered_commands = {} if register_commands: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 2885180360..a9beb10847 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1557,6 +1557,132 @@ 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") + # 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 + 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() + # ...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 + + 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 =====