Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion src/specify_cli/extensions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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_<sibling>_*`` env vars into no one. The
registry is the source of truth for "installed".
Comment on lines +2743 to +2749

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — updated the Fix and Tests sections of the PR description to reflect the registry-sourced sibling scan (ExtensionRegistry.keys() instead of the directory scan) and the current seven-test list, including test_config_only_leftover_not_treated_as_sibling (keep_config=True leftover) and test_non_utf8_registry_does_not_crash (corrupted-encoding fallback). The Fix section also now names the (OSError, UnicodeError) fallback in _sibling_extension_ids and its rationale. No code changes in this update — description-only.


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.

Expand All @@ -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.<field> 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 ``<US>_``). ``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_<EXT>_`` 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

Expand Down
132 changes: 132 additions & 0 deletions tests/test_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Loading