Skip to content

Commit d6a84df

Browse files
committed
fix(extensions): stop env-var config leaking across prefix-colliding IDs (#3494)
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.<field> 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
1 parent 6664cf8 commit d6a84df

2 files changed

Lines changed: 142 additions & 1 deletion

File tree

src/specify_cli/extensions/__init__.py

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2737,6 +2737,32 @@ def _get_local_config(self) -> Dict[str, Any]:
27372737
config_file = self.extension_dir / "local-config.yml"
27382738
return self._load_yaml_config(config_file)
27392739

2740+
def _sibling_extension_ids(self) -> list[str]:
2741+
"""Return IDs of other extensions installed alongside this one.
2742+
2743+
Scans ``.specify/extensions/`` for sibling directories and returns
2744+
their names. Dot-prefixed entries (``.cache``, ``.backup``, …) and
2745+
non-directories are skipped, so a misconfigured extensions dir never
2746+
raises. Returns an empty list if the extensions dir does not exist
2747+
(fresh project, ad-hoc test harness) so ``_get_env_config`` degrades
2748+
to its pre-fix behaviour rather than crashing.
2749+
2750+
Used by ``_get_env_config`` to detect env vars whose remainder claims
2751+
a longer, sibling-owned prefix (e.g. ``SPECKIT_GIT_HOOKS_URL`` is
2752+
owned by ``git-hooks`` when it is co-installed with ``git``).
2753+
"""
2754+
extensions_dir = self.project_root / ".specify" / "extensions"
2755+
if not extensions_dir.is_dir():
2756+
return []
2757+
try:
2758+
return [
2759+
entry.name
2760+
for entry in extensions_dir.iterdir()
2761+
if entry.is_dir() and not entry.name.startswith(".")
2762+
]
2763+
except OSError:
2764+
return []
2765+
27402766
def _get_env_config(self) -> Dict[str, Any]:
27412767
"""Get configuration from environment variables.
27422768
@@ -2756,15 +2782,49 @@ def _get_env_config(self) -> Dict[str, Any]:
27562782
ext_id_upper = self.extension_id.replace("-", "_").upper()
27572783
prefix = f"SPECKIT_{ext_id_upper}_"
27582784

2785+
# Cross-extension prefix collision: because ``_`` doubles as both the
2786+
# separator between the extension ID and the config path *and* the
2787+
# substitute for ``-`` inside an extension ID, an env var like
2788+
# ``SPECKIT_GIT_HOOKS_URL`` begins with *both* the ``SPECKIT_GIT_``
2789+
# prefix of the ``git`` extension and the ``SPECKIT_GIT_HOOKS_`` prefix
2790+
# of a co-installed ``git-hooks`` extension. It logically belongs to
2791+
# the extension whose normalized ID is the longer, more specific match
2792+
# — otherwise config intended for one extension silently surfaces
2793+
# inside another and can drive hooks that only inspect
2794+
# ``config.<field> is set``. Build the list of sibling-owned
2795+
# remainder-prefixes here so a later env var can be skipped if it
2796+
# matches one.
2797+
sibling_prefixes: list[str] = []
2798+
for sibling_id in self._sibling_extension_ids():
2799+
if sibling_id == self.extension_id:
2800+
continue
2801+
sib_upper = sibling_id.replace("-", "_").upper()
2802+
# A sibling collides only when its normalized ID *extends* our own
2803+
# (i.e. starts with ``<US>_``). ``git`` vs ``not-git`` is not a
2804+
# collision; ``git`` vs ``git-hooks`` is.
2805+
if sib_upper.startswith(ext_id_upper + "_"):
2806+
# The portion of the env-var *remainder* the sibling claims,
2807+
# including the trailing ``_`` so a shorter ID that shares a
2808+
# non-boundary prefix cannot false-positive (e.g. sibling
2809+
# ``hook`` would not eat env vars under key ``hooks``).
2810+
sibling_prefixes.append(sib_upper[len(ext_id_upper) + 1 :] + "_")
2811+
27592812
for key, value in os.environ.items():
27602813
if not key.startswith(prefix):
27612814
continue
27622815

2816+
remainder = key[len(prefix) :]
2817+
# Skip when a longer sibling ID claims this var — see the block
2818+
# above. Keeps ``SPECKIT_GIT_HOOKS_URL`` out of the ``git``
2819+
# extension's config when ``git-hooks`` is co-installed.
2820+
if any(remainder.startswith(sp) for sp in sibling_prefixes):
2821+
continue
2822+
27632823
# Remove prefix and split into parts. Drop empty components from a
27642824
# malformed name (e.g. ``SPECKIT_<EXT>_`` with no key, or
27652825
# consecutive underscores ``SPECKIT_X__Y``) so we never create an
27662826
# entry under an empty key.
2767-
config_path = [p for p in key[len(prefix) :].lower().split("_") if p]
2827+
config_path = [p for p in remainder.lower().split("_") if p]
27682828
if not config_path:
27692829
continue
27702830

tests/test_extensions.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7896,3 +7896,84 @@ def test_malformed_env_names_ignored(self, tmp_path, monkeypatch):
78967896
cfg = cm._get_env_config()
78977897
assert "" not in cfg
78987898
assert cfg == {"a": {"b": "z"}}
7899+
7900+
7901+
class TestConfigManagerCrossExtensionEnvLeak:
7902+
"""Cross-extension env-var leak: a longer, co-installed sibling ID must
7903+
own its own env vars instead of leaking them into a shorter-prefix sibling.
7904+
7905+
Before the fix, ``SPECKIT_GIT_HOOKS_URL`` (intended for a ``git-hooks``
7906+
extension) also surfaced inside the ``git`` extension's config as
7907+
``{'hooks': {'url': ...}}`` because ``SPECKIT_GIT_`` is a strict prefix of
7908+
``SPECKIT_GIT_HOOKS_``.
7909+
"""
7910+
7911+
def _install(self, project_root, ext_id):
7912+
(project_root / ".specify" / "extensions" / ext_id).mkdir(parents=True)
7913+
7914+
def test_sibling_owns_longer_prefix_env(self, tmp_path, monkeypatch):
7915+
"""SPECKIT_GIT_HOOKS_URL belongs to git-hooks when co-installed with git."""
7916+
self._install(tmp_path, "git")
7917+
self._install(tmp_path, "git-hooks")
7918+
monkeypatch.setenv("SPECKIT_GIT_URL", "for_git")
7919+
monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks")
7920+
7921+
git_cfg = ConfigManager(tmp_path, "git")._get_env_config()
7922+
gh_cfg = ConfigManager(tmp_path, "git-hooks")._get_env_config()
7923+
7924+
# 'git' must NOT see the git-hooks var — no cross-extension leak.
7925+
assert git_cfg == {"url": "for_git"}
7926+
# 'git-hooks' still receives its own var (unchanged behaviour).
7927+
assert gh_cfg == {"url": "for_git_hooks"}
7928+
7929+
def test_no_sibling_installed_keeps_legacy_absorption(self, tmp_path, monkeypatch):
7930+
"""Without a longer-prefix sibling installed, the legacy behaviour is
7931+
preserved: ``SPECKIT_GIT_HOOKS_URL`` is absorbed as a nested key of
7932+
the ``git`` extension. This keeps the fix strictly to the *collision*
7933+
case and avoids surprising users who deliberately set a nested key
7934+
via env with no sibling to disambiguate against.
7935+
"""
7936+
self._install(tmp_path, "git")
7937+
monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks")
7938+
7939+
cfg = ConfigManager(tmp_path, "git")._get_env_config()
7940+
assert cfg == {"hooks": {"url": "for_git_hooks"}}
7941+
7942+
def test_non_prefix_sibling_ignored(self, tmp_path, monkeypatch):
7943+
"""A sibling whose ID does not extend our own is not a collision.
7944+
7945+
e.g. current='git' and sibling='not-git' — 'not-git' normalized to
7946+
'NOT_GIT' does not start with 'GIT_', so its presence must not
7947+
influence git's env-var interpretation.
7948+
"""
7949+
self._install(tmp_path, "git")
7950+
self._install(tmp_path, "not-git")
7951+
monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_hooks")
7952+
7953+
cfg = ConfigManager(tmp_path, "git")._get_env_config()
7954+
assert cfg == {"hooks": {"url": "for_git_hooks"}}
7955+
7956+
def test_boundary_prevents_false_positive(self, tmp_path, monkeypatch):
7957+
"""Sibling ID 'hook' (not 'hooks') must NOT eat env keys starting
7958+
with 'hooks'. The trailing-underscore boundary in the sibling prefix
7959+
prevents this false positive.
7960+
"""
7961+
self._install(tmp_path, "git")
7962+
self._install(tmp_path, "git-hook")
7963+
monkeypatch.setenv("SPECKIT_GIT_HOOKS_URL", "for_git_key_hooks")
7964+
7965+
# git-hook's prefix is 'HOOK_', which does not match 'HOOKS_URL',
7966+
# so 'git' keeps the env var (single-installed semantics).
7967+
cfg = ConfigManager(tmp_path, "git")._get_env_config()
7968+
assert cfg == {"hooks": {"url": "for_git_key_hooks"}}
7969+
7970+
def test_missing_extensions_dir_does_not_crash(self, tmp_path, monkeypatch):
7971+
"""A ConfigManager built against a project without ``.specify/extensions``
7972+
(fresh project, ad-hoc test harness) must still evaluate env config
7973+
rather than raising from the sibling scan.
7974+
"""
7975+
# Note: no _install call — extensions dir intentionally absent.
7976+
monkeypatch.setenv("SPECKIT_TESTEXT_URL", "v")
7977+
7978+
cfg = ConfigManager(tmp_path, "testext")._get_env_config()
7979+
assert cfg == {"url": "v"}

0 commit comments

Comments
 (0)