Skip to content

fix: bind allowlisted extensions to discovered sources - #362

Open
codeforester wants to merge 1 commit into
mainfrom
security/340-20260918-bind-entrypoint-source
Open

codeforester wants to merge 1 commit into
mainfrom
security/340-20260918-bind-entrypoint-source

Conversation

@codeforester

Copy link
Copy Markdown
Contributor

Summary

  • Bind each accepted extension descriptor to the exact entry point and distribution discovered by the allowlist.
  • Fail closed if the source identity changes before load; clear the binding cache on refresh.
  • Cover denied/trusted collisions in both orders, cached loads, load_all, refresh, and identity changes.

Closes #340

Validation

  • uv run --extra dev --extra typer --extra quality python -m pytest tests/test_extensions.py -q
  • Ruff check and format check for changed Python files
  • git diff --check

entry_point = self._entry_point_cache.get(descriptor)
if entry_point is None:
raise ImportError("approved entry point disappeared before it could be loaded")
try:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Robustness (minor): _load_descriptor() only catches (AttributeError, TypeError, ValueError) around _descriptor_from_entry_point, but re-reading a real Distribution's metadata can raise other exceptions (e.g. FileNotFoundError, zipfile.BadZipFile) if the package's dist-info changes between discovery and load. Still fails closed via the broader except Exception in load(), but loses the specific 'identity changed' diagnostic this PR was written to provide.

@codeforester

Copy link
Copy Markdown
Contributor Author

Additional finding (concern spans code not fully covered by this diff's hunks, so not postable as an inline comment):

lib/python/base_cli/extensions.pyload() calls _load_descriptor() outside self._lock, and the new _entry_point_cache dict this PR adds has no self-healing fallback the way the old raw-entry-point rescan did. If a concurrent refresh() clears the cache between a thread's list() call and its subsequent load() call, the load raises ImportError("approved entry point disappeared before it could be loaded") even though nothing about the extension actually changed — the pre-diff code rescanned metadata.entry_points() directly on every call and self-healed from exactly this race. This is a realistic scenario given the existing test_extension_discovery_metadata_cache_is_thread_safe_and_single_snapshot test, which documents concurrent access as supported. Worth considering whether load() should re-validate/refresh under the lock rather than trusting a possibly-stale cache snapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: load the exact distribution approved by extension discovery

1 participant