Skip to content

Harden CLI-Hub preview compatibility and installs#299

Open
dragonnite1221-lgtm wants to merge 3 commits into
HKUDS:mainfrom
dragonnite1221-lgtm:harden-cli-hub-shell-installs
Open

Harden CLI-Hub preview compatibility and installs#299
dragonnite1221-lgtm wants to merge 3 commits into
HKUDS:mainfrom
dragonnite1221-lgtm:harden-cli-hub-shell-installs

Conversation

@dragonnite1221-lgtm

@dragonnite1221-lgtm dragonnite1221-lgtm commented May 20, 2026

Copy link
Copy Markdown

Summary

  • make CLI-Hub preview generation compatible with Python 3.10/3.11 by avoiding backslashes inside f-string expressions
  • validate preview artifact URLs/paths before embedding them in generated HTML
  • escape JSON embedded in <script> blocks to avoid HTML/script breakouts
  • harden shell install command generation and mirror command docs
  • replace bare except: handlers in the touched unimol harness code
  • document fork maintenance expectations

Root cause

cli-hub/cli_hub/preview.py used a Python 3.12-only f-string expression form even though the project supports Python 3.10+. The same preview surface also embedded artifact URLs and JSON data with too little validation/escaping.

Validation

  • PYTHONDONTWRITEBYTECODE=1 python3.11 -m py_compile cli-hub/cli_hub/preview.py cli-hub/tests/test_cli_hub.py unimol_tools/agent-harness/cli_anything/unimol_tools/utils/weights.py unimol_tools/agent-harness/cli_anything/unimol_tools/tests/test_full_e2e.py
  • PYTHONDONTWRITEBYTECODE=1 python3 -m pytest -q cli-hub/tests/test_cli_hub.py (106 passed)
  • git diff --check upstream/main...HEAD
  • gitleaks protect --staged --redact --no-banner

Full gitleaks detect still reports existing findings in files outside this PR's changed set (cc-switch, generated docs, cli-hub/analytics.py, n8n, zotero). None overlap this diff.

@github-actions github-actions Bot added the cli-anything-hub Changes CLI-Hub, registries, or hub docs label May 20, 2026
@omerarslan0

Copy link
Copy Markdown
Collaborator

The trust-boundary framing is correct, but a couple of items before this lands:

  1. Existing entries that break by default: the live registry already ships sketch (cd … && npm install && npm link) and jimeng (curl -s … | bash). After this PR, cli-hub install sketch and cli-hub install jimeng fail until the user sets CLI_HUB_ALLOW_SHELL_COMMANDS=1. Either flip the breaking entries to safer shapes in the registry in the same PR, or call this out explicitly in cli-hub/README.md so users hitting the new error know what to do.

  2. Per-entry trust beats a global flag. A global env var re-enables shell execution for every registry entry for the duration of the process, which is exactly the catalog-wide trust the PR is trying to remove. Consider gating on a per-entry field (e.g. "requires_shell": true set by the registry editor at review time) so the opt-in lives with the reviewed entry instead of the user environment. The env var can stay as a hard override for power users.

  3. _SHELL_METACHARACTERS is a substring match on the raw command. A registry entry like curl "https://foo.example/?q=a|b" -o out would now be blocked even though shell=False would have run it safely. Tokenize first (e.g. via shlex.split) and check for unquoted operators, or scope the check to operator-only tokens.

  4. Redirection operators (>, <, >>) are not in the list. They are not executed under shell=False, so they are not a bypass today, but if anyone adds shell=True somewhere they become one. Either add them for defense-in-depth or add a comment explaining why they were intentionally omitted.

  5. The error string surfaced to the user goes through _generic_install as Install failed:\n<stderr>. The returncode=2 path is fine, but worth a short note in the README pointing at the env var so users do not have to grep the source.

@github-actions github-actions Bot added existing-cli-fix Fixes or improves an existing CLI harness cli-anything-skill Changes CLI-Anything plugin or skill files labels Jun 12, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86c19c129f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +68 to +74
if quote:
if ch == quote:
quote = None
elif quote == '"' and (ch == "`" or (ch == "$" and i + 1 < len(cmd) and cmd[i + 1] == "(")):
return True
i += 1
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Block shell syntax hidden behind sh -c

For install commands that invoke a shell explicitly, e.g. sh -c 'curl -s https://example/cli | bash' or bash -c 'echo ok; rm -rf /tmp/x', this quote branch treats the operator-containing payload as a literal argument, so _run_command runs it with shell=False instead of returning the new block. shlex.split then strips the quotes and the invoked shell executes the pipe/semicolon anyway, allowing registry entries without requires_shell to bypass the hardening this commit adds.

Useful? React with 👍 / 👎.

@dragonnite1221-lgtm dragonnite1221-lgtm force-pushed the harden-cli-hub-shell-installs branch from 86c19c1 to edadb6c Compare June 12, 2026 16:26
@dragonnite1221-lgtm dragonnite1221-lgtm changed the title Harden CLI-Hub shell install commands Harden CLI-Hub preview compatibility and installs Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli-anything-hub Changes CLI-Hub, registries, or hub docs cli-anything-skill Changes CLI-Anything plugin or skill files existing-cli-fix Fixes or improves an existing CLI harness

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants