mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(extensions): stop env-var config leaking across prefix-colliding extension IDs (#3497)
* 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 * fix(extensions): source sibling scan from registry, not directory 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_<sibling>_*`` 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. * fix(extensions): swallow non-UTF-8 registry in sibling scan 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.
This commit is contained in:
@@ -2737,6 +2737,36 @@ class ConfigManager:
|
||||
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".
|
||||
|
||||
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 @@ class ConfigManager:
|
||||
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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user