Clarify extension catalog trust model in docs, help, and messaging - #4177
Conversation
…ithub#4176) Extension catalog management gave no explanation of why the community catalog is discovery-only, and the install-error text nudged users to flip a discovery catalog to install_allowed — exactly the wrong move. - Docs: add a "discovery-only vs. install sources" trust-model section, document `add --from <url>` as the lightweight vetted-install path, and stop implying you should make community installable. - Help: expand the `catalog` app and `--install-allowed` help to state the vetting intent instead of bare mechanics. - Messaging: rewrite the not-installable errors in `add`, `search`, and `info` to point at `--from` and self-curated catalogs, and to say explicitly not to flip a discovery-only catalog to install_allowed. - `catalog list` now prints trust-model guidance when a discovery-only catalog is active. - Tests cover the new list guidance (present/absent). Deliberately does not add a verb to toggle install_allowed on an existing catalog: discovery-only is a security boundary, not an inconvenience. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d
There was a problem hiding this comment.
Pull request overview
Clarifies the extension catalog trust model across documentation and CLI messaging.
Changes:
- Documents discovery-only versus trusted install catalogs.
- Improves CLI help and installation guidance.
- Tests catalog-list trust guidance.
Show a summary per file
| File | Description |
|---|---|
docs/reference/extensions.md |
Documents catalog trust and vetted installation paths. |
src/specify_cli/extensions/_commands.py |
Updates catalog help and user-facing guidance. |
tests/test_extensions.py |
Tests conditional catalog-list guidance. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
…rning (github#4176) - The discovery-only "install directly" hint used the user-typed argument, which can be a display name with spaces (resolved via search) and would break when copied as a shell command. Emit the resolved catalog ID (ext_info['id']) instead. Added a regression test. - The `--from` untrusted-source warning claimed the URL was "not listed in any of your configured extension catalogs", which is false for a URL copied from a discovery-only catalog — the exact flow this PR documents. Reword it to state the install is bypassing trusted (install-allowed) catalogs, which is accurate regardless of discovery-catalog membership. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d
|
Addressed both review comments in a9299f1:
Full suite green: Posted by GitHub Copilot (model: Claude Opus 4.8) on behalf of @mnriem. |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
docs/reference/extensions.md:113
- The documented path still leaves users without a way to obtain
<archive-url>. Community catalog entries containdownload_url, butextension searchonly shows the repository andextension infoomitsdownload_url, so following this instruction currently requires inspecting the raw catalog—the confusion this PR is intended to remove. Document how to locate and vet the release archive, or expose the candidate URL through the CLI before recommending this command.
> 1. **Install a single vetted extension directly** with `--from` (no catalog authoring needed):
> ```bash
> specify extension add <name> --from <archive-url>
> ```
src/specify_cli/extensions/_commands.py:1539
- This newly suggested command interpolates an ID from an explicitly unvetted catalog.
_escape_markuponly escapes Rich syntax, not shell metacharacters, so copying the command can execute catalog-controlled shell text. Validate the ID against the extension manifest's lowercase-alphanumeric-and-hyphen rule before showing it as a command, or render command arguments through a shell-safe helper.
f"Once you've vetted it, install directly: specify extension add {safe_id} --from <archive-url>"
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
…RL (github#4176) Second review round on github#4177. Shell-safety: catalog entry IDs (especially from discovery-only catalogs) are not validated during catalog merge, and rich.markup.escape only neutralizes Rich markup, not shell metacharacters. A malicious ID like `foo; rm -rf ~` was interpolated into the `specify extension add ... --from` command we encourage the user to copy. Add `_command_safe_id`, which only emits an ID matching the manifest rule `^[a-z0-9-]+$` (via VALID_EXTENSION_ARTIFACT_NAME_PATTERN) and otherwise falls back to a literal `<extension-id>` placeholder. Applied to every suggested command in `add`, `search`, and `info`. Discoverability: the documented `--from <archive-url>` flow gave no CLI path to obtain the URL. `extension info` now prints the candidate `download_url` for a discovery-only entry (clearly flagged as needing vetting), and the docs show `extension info <name>` as the way to get the archive URL. Tests cover the resolved-ID hint, the unsafe-ID neutralization, and pass the full extensions + CLI suites (635). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d
|
Addressed the second review round in 8d41177:
New tests: Posted by GitHub Copilot (model: Claude Opus 4.8) on behalf of @mnriem. |
…ub#4176) Third review round on github#4177. _command_safe_id: an ID like `--force` satisfies the manifest character rule `^[a-z0-9-]+$` but Typer parses a leading hyphen as an option rather than the positional extension argument, so an untrusted catalog could still yield a non-copyable or option-altering suggested command. Reject a leading hyphen and fall back to the `<extension-id>` placeholder. Tests: cover the new `extension info` discovery-only branch that surfaces the candidate `download_url` (plus the no-URL fallback), and the leading-hyphen rejection. Full extensions suite green (528). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d
|
Addressed the third review round in 74a7e0a:
Full extensions suite green — 528 passed. Posted by GitHub Copilot (model: Claude Opus 4.8) on behalf of @mnriem. |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/specify_cli/extensions/_commands.py:104
re.Pattern.match()with a$anchor accepts a match immediately before one trailing newline, so a catalog ID such asacme\nis returned verbatim. Every generated hint then becomes a two-line, non-copyable shell command despite this helper's safety contract. Usefullmatch()so the complete catalog key must satisfy the slug rule.
if VALID_EXTENSION_ARTIFACT_NAME_PATTERN.match(text):
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Closes #4176
Problem
The issue author found extension catalog management confusing: the "default"/"community" catalogs were unexplained, and after discovering the community catalog they tried to make it installable — hitting the deliberate
install_allowed: falseboundary with no explanation of why it exists or what the correct path is.Investigating confirmed the root cause is documentation + messaging, not the design:
docs/reference/extensions.mdnor--helpexplained thatcommunityis discovery-only by design (a security/vetting boundary), or thatcommunityis already active forsearchby default.add to an approved catalog with install_allowed: true) reads like an invitation to flip the community catalog toinstall_allowed— exactly the wrong move.Changes
add --from <url>as the lightweight vetted-install path; stop implying community should be made installable.--help: expand thecatalogapp and--install-allowedhelp to state the vetting intent instead of bare mechanics.add,search, andinfoto point at--fromand self-curated catalogs, and to say explicitly not to flip a discovery-only catalog toinstall_allowed.catalog list: prints trust-model guidance when a discovery-only catalog is active.catalog listguidance (present and absent).Deliberately out of scope
No verb to toggle
install_allowedon an existing catalog. Discovery-only is a security boundary, not an inconvenience — the fix is making the correct path (--from, self-curated catalog) obvious, not making it easy to weaken the boundary.Validation
tests/test_extensions.pyandtests/integrations/test_cli.py— 633 passed.This PR was authored by GitHub Copilot (model: Claude Opus 4.8) under the supervision of @mnriem.