Skip to content

fix(skills): block private ClawHub zip redirects - #24828

Draft
binhnt92 wants to merge 1 commit into
NousResearch:mainfrom
binhnt92:codex/skills-hub-block-clawhub-zip-redirects
Draft

binhnt92 wants to merge 1 commit into
NousResearch:mainfrom
binhnt92:codex/skills-hub-block-clawhub-zip-redirects

Conversation

@binhnt92

@binhnt92 binhnt92 commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

ClawHub ZIP downloads now use the existing guarded URL fetch path instead of automatic redirect following, so redirect targets are validated before the client fetches them.

Raw skill files already blocked private redirect targets through _guarded_http_get. This applies the same protection to the primary ZIP bundle path.

Tests

  • Red: pytest tests/tools/test_skills_hub_clawhub.py -q -k private_zip_redirect failed before the fix because the redirected ZIP was accepted.
  • Green: pytest tests/tools/test_skills_hub_clawhub.py tests/tools/test_skills_hub.py -q, 119 passed.
  • ruff check tools/skills_hub.py tests/tools/test_skills_hub_clawhub.py
  • python3 -m py_compile tools/skills_hub.py tests/tools/test_skills_hub_clawhub.py
  • git diff --check

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists tool/skills Skills system (list, view, manage) labels May 13, 2026
@binhnt92

Copy link
Copy Markdown
Contributor Author

CI note:

  • All focused checks for this PR are green: e2e, build, nix, supply-chain, attribution, Windows footguns, ruff enforcement, and ruff + ty diff all passed.
  • The full test job fails in unrelated baseline areas: missing optional deps (botocore, faster_whisper, numpy), DingTalk card/webhook tests, Feishu bot admission, lsp builtin registration, and switch-model context-length preservation.
  • No skills hub / ClawHub tests fail in CI; local focused validation passed as listed in the PR body.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused SSRF hardening. The premise remains valid on current origin/main: ClawHubSource._download_zip() still calls httpx.get(..., follow_redirects=True) at tools/skills_hub.py:2655-2660, while the existing _guarded_http_get() validates every redirect target at tools/skills_hub.py:294-328.

The proposed change routes the ZIP bundle path through that existing guard, preserving the ZIP extraction and fallback flow. The regression test in commit 5983e26ddb4b specifically rejects a redirect to 127.0.0.1 before it can be fetched. I found no correctness or design-fit issue in the proposed scope.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Three PRs address the ClawHub ZIP download path: #24828 validates redirect targets before fetching them, #57571 adds the same redirect protection while streaming and capping archive bytes before extraction, and #57714 retries transient HTTPX failures instead of abandoning the ZIP path after one exception. The first two overlap on the SSRF cause, while #57714 addresses a distinct retry gap.

Related pull requests

  • #24828 related — (+53/-6) — superseded security fix: Routes ZIP downloads through the existing guarded fetch and proves that a redirect to 127.0.0.1 is rejected before retrieval. Despite the keep_open review on #24828, #57571's diff covers the same redirect-validation cause through _guarded_http_stream and additionally bounds pre-extraction buffering.
  • #57571 related — (+297/-24) — preferred comprehensive fix: Replaces eager resp.content buffering with a policy-checked redirect stream, rejects oversized Content-Length values, and stops reading after ZIP_DOWNLOAD_MAX_BYTES. The blocking 429 concern in the COMMENTED keep_open review is addressed in the shown diff by bypassing extraction for every 429, sleeping only before remaining attempts, and testing three-429 exhaustion.
  • #57714 related — (+35/-1) — retain as a distinct retry fix: Retries transient httpx.HTTPError failures within the existing three-attempt loop and tests a timeout followed by a valid ZIP. It does not duplicate the size or redirect protections, but it must be rebased or adapted because #57571 moves request exceptions into _guarded_http_stream.

Duplicates

#24828 and #57571 substantially duplicate the private-redirect protection; #57571 is the broader implementation. #57714 is not a duplicate.

Suggested consolidation

Merge #57571 as the comprehensive redirect-safe, size-bounded ZIP download implementation; close #24828 as superseded by #57571 despite its keep_open review because the latter's diff contains the same guarded-redirect behavior and regression coverage. Keep #57714 open and rebase/adapt its transient-error retry semantics onto #57571 rather than closing it as a duplicate.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 14 kB of issue/PR text, 3 kB of discussion (5 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/skills Skills system (list, view, manage) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants