From d6a84df7d9aa68e72ecdc6b10ba2ebd32d3e540e Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 10:38:27 -0700 Subject: [PATCH 1/3] fix(extensions): stop env-var config leaking across prefix-colliding IDs (#3494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because ``_`` doubles as both the separator between an extension ID and its config path AND the substitute for ``-`` inside an extension ID, an env var like ``SPECKIT_GIT_HOOKS_URL`` starts with *both* the ``SPECKIT_GIT_`` prefix of the ``git`` extension and the ``SPECKIT_GIT_HOOKS_`` prefix of a co-installed ``git-hooks`` extension. ``ConfigManager._get_env_config`` matched only on the shorter prefix, so the same env var silently surfaced inside both extensions' configs (as ``{'hooks': {'url': ...}}`` for ``git`` and ``{'url': ...}`` for ``git-hooks``). Impact: config intended for one extension leaked into another and, worse, could flip ``config. is set`` hook conditions on the wrong extension. Route the env var to the extension whose normalized ID is the longest match — the more specific one. When another installed sibling's normalized ID + ``_`` claims the remainder, skip the var here. The sibling scan reads ``.specify/extensions/`` directly and degrades to a no-op if the dir is missing (fresh project / ad-hoc harness), so the pre-fix single-extension behaviour is unchanged when there is no collision. Distinct from #3350 (intra-extension prefix collision between two keys of the same extension) — this fixes the cross-extension case. Fixes #3494 --- src/specify_cli/extensions/__init__.py | 62 +++++++++++++++++++- tests/test_extensions.py | 81 ++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index aac1ba6481..61ee1ab337 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2737,6 +2737,32 @@ 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. + + Scans ``.specify/extensions/`` for sibling directories and returns + their names. Dot-prefixed entries (``.cache``, ``.backup``, …) and + non-directories are skipped, so a misconfigured extensions dir never + raises. Returns an empty list if the extensions dir does not exist + (fresh project, ad-hoc test harness) so ``_get_env_config`` degrades + to its pre-fix behaviour rather than crashing. + + 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" + if not extensions_dir.is_dir(): + return [] + try: + return [ + entry.name + for entry in extensions_dir.iterdir() + if entry.is_dir() and not entry.name.startswith(".") + ] + except OSError: + return [] + def _get_env_config(self) -> Dict[str, Any]: """Get configuration from environment variables. @@ -2756,15 +2782,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..30eddb184b 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7896,3 +7896,84 @@ 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): + (project_root / ".specify" / "extensions" / ext_id).mkdir(parents=True) + + 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"} From ca75851d09aa8577e8b6c51cc33e56deaffda50a Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 13:49:30 -0700 Subject: [PATCH 2/3] fix(extensions): source sibling scan from registry, not directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #3497: ``ExtensionManager.remove(..., keep_config=True)`` preserves the extension directory but drops the registry entry, so the previous directory-scan approach would treat a config-only leftover as an installed sibling and silently discard ``SPECKIT__*`` env vars into no owner. Sourced the sibling list from ``ExtensionRegistry.keys()`` — the registry is the source of truth for "installed" — and kept the same graceful ``[]`` fallback so the fresh-project / ad-hoc harness path is unaffected. Updated the ``TestConfigManagerCrossExtensionEnvLeak`` ``_install`` helper to register its fake installations and added ``test_config_only_leftover_not_treated_as_sibling`` to lock in the new behaviour for the ``keep_config=True`` scenario. Full suite: 3978 passed, 110 skipped. --- src/specify_cli/extensions/__init__.py | 21 ++++++++-------- tests/test_extensions.py | 33 +++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 61ee1ab337..f3e627c620 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2740,10 +2740,15 @@ def _get_local_config(self) -> Dict[str, Any]: def _sibling_extension_ids(self) -> list[str]: """Return IDs of other extensions installed alongside this one. - Scans ``.specify/extensions/`` for sibling directories and returns - their names. Dot-prefixed entries (``.cache``, ``.backup``, …) and - non-directories are skipped, so a misconfigured extensions dir never - raises. Returns an empty list if the extensions dir does not exist + 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. @@ -2752,14 +2757,8 @@ def _sibling_extension_ids(self) -> list[str]: owned by ``git-hooks`` when it is co-installed with ``git``). """ extensions_dir = self.project_root / ".specify" / "extensions" - if not extensions_dir.is_dir(): - return [] try: - return [ - entry.name - for entry in extensions_dir.iterdir() - if entry.is_dir() and not entry.name.startswith(".") - ] + return list(ExtensionRegistry(extensions_dir).keys()) except OSError: return [] diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 30eddb184b..b7e095c890 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7909,7 +7909,12 @@ class TestConfigManagerCrossExtensionEnvLeak: """ def _install(self, project_root, ext_id): - (project_root / ".specify" / "extensions" / ext_id).mkdir(parents=True) + 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.""" @@ -7977,3 +7982,29 @@ def test_missing_extensions_dir_does_not_crash(self, tmp_path, monkeypatch): 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"}} From ee434f389450386595bfcf484505471fe2089a4c Mon Sep 17 00:00:00 2001 From: thejesh23 Date: Mon, 13 Jul 2026 13:55:59 -0700 Subject: [PATCH 3/3] fix(extensions): swallow non-UTF-8 registry in sibling scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot follow-up on #3497: ``ExtensionRegistry._load()`` catches ``JSONDecodeError`` / ``FileNotFoundError`` but not decode failures — a registry file with invalid text encoding would surface a ``UnicodeDecodeError`` out of ``_sibling_extension_ids`` and break every config read instead of degrading to the documented pre-fix behaviour. Extend the fallback in ``_sibling_extension_ids`` to also catch ``UnicodeError`` and add ``test_non_utf8_registry_does_not_crash`` as a regression pin (kept ``_load()`` itself out of scope — that broader hardening belongs in a separate PR since it affects all readers). Full suite: 3979 passed, 110 skipped. --- src/specify_cli/extensions/__init__.py | 9 +++++++-- tests/test_extensions.py | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index f3e627c620..e00b9c247a 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2750,7 +2750,12 @@ def _sibling_extension_ids(self) -> list[str]: 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. + 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 @@ -2759,7 +2764,7 @@ def _sibling_extension_ids(self) -> list[str]: extensions_dir = self.project_root / ".specify" / "extensions" try: return list(ExtensionRegistry(extensions_dir).keys()) - except OSError: + except (OSError, UnicodeError): return [] def _get_env_config(self) -> Dict[str, Any]: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index b7e095c890..d2f4af8264 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -8008,3 +8008,23 @@ def test_config_only_leftover_not_treated_as_sibling(self, tmp_path, monkeypatch 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"}