diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index aac1ba6481..e00b9c247a 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2737,6 +2737,36 @@ def _get_local_config(self) -> Dict[str, Any]: config_file = self.extension_dir / "local-config.yml" return self._load_yaml_config(config_file) + def _sibling_extension_ids(self) -> list[str]: + """Return IDs of other extensions installed alongside this one. + + Sourced from ``ExtensionRegistry`` (``.specify/extensions/.registry``) + rather than a directory scan: ``ExtensionManager.remove(..., + keep_config=True)`` deliberately preserves the extension directory + while dropping the registry entry, so a directory scan would treat + that config-only leftover as an installed sibling and keep silently + absorbing its ``SPECKIT__*`` env vars into no one. The + registry is the source of truth for "installed". + + Returns an empty list if the registry is missing or corrupted + (fresh project, ad-hoc test harness) so ``_get_env_config`` degrades + to its pre-fix behaviour rather than crashing. ``UnicodeError`` is + caught alongside ``OSError`` because ``ExtensionRegistry._load()`` + opens the file in text mode and only handles ``JSONDecodeError`` / + ``FileNotFoundError``, so a registry file with non-UTF-8 bytes would + otherwise surface a ``UnicodeDecodeError`` here and break *every* + config read instead of degrading gracefully. + + Used by ``_get_env_config`` to detect env vars whose remainder claims + a longer, sibling-owned prefix (e.g. ``SPECKIT_GIT_HOOKS_URL`` is + owned by ``git-hooks`` when it is co-installed with ``git``). + """ + extensions_dir = self.project_root / ".specify" / "extensions" + try: + return list(ExtensionRegistry(extensions_dir).keys()) + except (OSError, UnicodeError): + return [] + def _get_env_config(self) -> Dict[str, Any]: """Get configuration from environment variables. @@ -2756,15 +2786,49 @@ def _get_env_config(self) -> Dict[str, Any]: ext_id_upper = self.extension_id.replace("-", "_").upper() prefix = f"SPECKIT_{ext_id_upper}_" + # Cross-extension prefix collision: because ``_`` doubles as both the + # separator between the extension ID and the config path *and* the + # substitute for ``-`` inside an extension ID, an env var like + # ``SPECKIT_GIT_HOOKS_URL`` begins with *both* the ``SPECKIT_GIT_`` + # prefix of the ``git`` extension and the ``SPECKIT_GIT_HOOKS_`` prefix + # of a co-installed ``git-hooks`` extension. It logically belongs to + # the extension whose normalized ID is the longer, more specific match + # — otherwise config intended for one extension silently surfaces + # inside another and can drive hooks that only inspect + # ``config. is set``. Build the list of sibling-owned + # remainder-prefixes here so a later env var can be skipped if it + # matches one. + sibling_prefixes: list[str] = [] + for sibling_id in self._sibling_extension_ids(): + if sibling_id == self.extension_id: + continue + sib_upper = sibling_id.replace("-", "_").upper() + # A sibling collides only when its normalized ID *extends* our own + # (i.e. starts with ``_``). ``git`` vs ``not-git`` is not a + # collision; ``git`` vs ``git-hooks`` is. + if sib_upper.startswith(ext_id_upper + "_"): + # The portion of the env-var *remainder* the sibling claims, + # including the trailing ``_`` so a shorter ID that shares a + # non-boundary prefix cannot false-positive (e.g. sibling + # ``hook`` would not eat env vars under key ``hooks``). + sibling_prefixes.append(sib_upper[len(ext_id_upper) + 1 :] + "_") + for key, value in os.environ.items(): if not key.startswith(prefix): continue + remainder = key[len(prefix) :] + # Skip when a longer sibling ID claims this var — see the block + # above. Keeps ``SPECKIT_GIT_HOOKS_URL`` out of the ``git`` + # extension's config when ``git-hooks`` is co-installed. + if any(remainder.startswith(sp) for sp in sibling_prefixes): + continue + # Remove prefix and split into parts. Drop empty components from a # malformed name (e.g. ``SPECKIT__`` with no key, or # consecutive underscores ``SPECKIT_X__Y``) so we never create an # entry under an empty key. - config_path = [p for p in key[len(prefix) :].lower().split("_") if p] + config_path = [p for p in remainder.lower().split("_") if p] if not config_path: continue diff --git a/tests/test_extensions.py b/tests/test_extensions.py index d9ffa5ca6f..d2f4af8264 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7896,3 +7896,135 @@ def test_malformed_env_names_ignored(self, tmp_path, monkeypatch): cfg = cm._get_env_config() assert "" not in cfg assert cfg == {"a": {"b": "z"}} + + +class TestConfigManagerCrossExtensionEnvLeak: + """Cross-extension env-var leak: a longer, co-installed sibling ID must + own its own env vars instead of leaking them into a shorter-prefix sibling. + + Before the fix, ``SPECKIT_GIT_HOOKS_URL`` (intended for a ``git-hooks`` + extension) also surfaced inside the ``git`` extension's config as + ``{'hooks': {'url': ...}}`` because ``SPECKIT_GIT_`` is a strict prefix of + ``SPECKIT_GIT_HOOKS_``. + """ + + def _install(self, project_root, ext_id): + extensions_dir = project_root / ".specify" / "extensions" + (extensions_dir / ext_id).mkdir(parents=True) + # Register in the extension registry — the registry is the source of + # truth for "installed" (a bare directory can be a config-only leftover + # from ``ExtensionManager.remove(..., keep_config=True)``). + ExtensionRegistry(extensions_dir).add(ext_id, {}) + + def test_sibling_owns_longer_prefix_env(self, tmp_path, monkeypatch): + """SPECKIT_GIT_HOOKS_URL belongs to git-hooks when co-installed with git.""" + self._install(tmp_path, "git") + self._install(tmp_path, "git-hooks") + monkeypatch.setenv("SPECKIT_GIT_URL", "for_git") + monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks") + + git_cfg = ConfigManager(tmp_path, "git")._get_env_config() + gh_cfg = ConfigManager(tmp_path, "git-hooks")._get_env_config() + + # 'git' must NOT see the git-hooks var — no cross-extension leak. + assert git_cfg == {"url": "for_git"} + # 'git-hooks' still receives its own var (unchanged behaviour). + assert gh_cfg == {"url": "for_git_hooks"} + + def test_no_sibling_installed_keeps_legacy_absorption(self, tmp_path, monkeypatch): + """Without a longer-prefix sibling installed, the legacy behaviour is + preserved: ``SPECKIT_GIT_HOOKS_URL`` is absorbed as a nested key of + the ``git`` extension. This keeps the fix strictly to the *collision* + case and avoids surprising users who deliberately set a nested key + via env with no sibling to disambiguate against. + """ + self._install(tmp_path, "git") + monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks") + + cfg = ConfigManager(tmp_path, "git")._get_env_config() + assert cfg == {"hooks": {"url": "for_git_hooks"}} + + def test_non_prefix_sibling_ignored(self, tmp_path, monkeypatch): + """A sibling whose ID does not extend our own is not a collision. + + e.g. current='git' and sibling='not-git' — 'not-git' normalized to + 'NOT_GIT' does not start with 'GIT_', so its presence must not + influence git's env-var interpretation. + """ + self._install(tmp_path, "git") + self._install(tmp_path, "not-git") + monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks") + + cfg = ConfigManager(tmp_path, "git")._get_env_config() + assert cfg == {"hooks": {"url": "for_git_hooks"}} + + def test_boundary_prevents_false_positive(self, tmp_path, monkeypatch): + """Sibling ID 'hook' (not 'hooks') must NOT eat env keys starting + with 'hooks'. The trailing-underscore boundary in the sibling prefix + prevents this false positive. + """ + self._install(tmp_path, "git") + self._install(tmp_path, "git-hook") + monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_key_hooks") + + # git-hook's prefix is 'HOOK_', which does not match 'HOOKS_URL', + # so 'git' keeps the env var (single-installed semantics). + cfg = ConfigManager(tmp_path, "git")._get_env_config() + assert cfg == {"hooks": {"url": "for_git_key_hooks"}} + + def test_missing_extensions_dir_does_not_crash(self, tmp_path, monkeypatch): + """A ConfigManager built against a project without ``.specify/extensions`` + (fresh project, ad-hoc test harness) must still evaluate env config + rather than raising from the sibling scan. + """ + # Note: no _install call — extensions dir intentionally absent. + monkeypatch.setenv("SPECKIT_TESTEXT_URL", "v") + + cfg = ConfigManager(tmp_path, "testext")._get_env_config() + assert cfg == {"url": "v"} + + def test_config_only_leftover_not_treated_as_sibling(self, tmp_path, monkeypatch): + """A directory left behind by ``remove(..., keep_config=True)`` must + NOT be treated as an installed sibling. + + ``ExtensionManager.remove(keep_config=True)`` preserves the extension + directory (config files remain, dormant, for a possible reinstall) but + removes the registry entry. The sibling scan is sourced from the + registry, so a leftover ``git-hooks/`` directory without a registry + entry must not silently discard ``SPECKIT_GIT_HOOKS_*`` from ``git``. + """ + self._install(tmp_path, "git") + # Simulate ``remove('git-hooks', keep_config=True)``: dir present, + # config file preserved, but no registry entry. + gh_dir = tmp_path / ".specify" / "extensions" / "git-hooks" + gh_dir.mkdir(parents=True) + (gh_dir / "git-hooks-config.yml").write_text("url: leftover\n") + # Sanity: git-hooks is NOT registered. + registry = ExtensionRegistry(tmp_path / ".specify" / "extensions") + assert "git-hooks" not in registry.keys() + + monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git") + + cfg = ConfigManager(tmp_path, "git")._get_env_config() + # git absorbs the var (no registered sibling owns it). + assert cfg == {"hooks": {"url": "for_git"}} + + def test_non_utf8_registry_does_not_crash(self, tmp_path, monkeypatch): + """A registry file with invalid text encoding must NOT propagate + ``UnicodeDecodeError`` out of the sibling scan and abort every + config read. ``ExtensionRegistry._load()`` catches ``JSONDecodeError`` + / ``FileNotFoundError`` only, so ``_sibling_extension_ids`` must + additionally swallow ``UnicodeError`` and degrade to the documented + pre-fix behaviour. + """ + extensions_dir = tmp_path / ".specify" / "extensions" + extensions_dir.mkdir(parents=True) + # Write bytes that are not valid UTF-8 to the registry file. + (extensions_dir / ExtensionRegistry.REGISTRY_FILE).write_bytes( + b"\xff\xfe invalid utf-8 registry \xc3\x28" + ) + monkeypatch.setenv("SPECKIT_TESTEXT_URL", "v") + + # Must not raise; must fall back to the "no siblings" path. + cfg = ConfigManager(tmp_path, "testext")._get_env_config() + assert cfg == {"url": "v"}