Skip to content

fix(security): guard browse.sh skillMdUrl fetches against SSRF - #70330

Closed
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:fix/skills-hub-browse-sh-ssrf
Closed

fix(security): guard browse.sh skillMdUrl fetches against SSRF#70330
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:fix/skills-hub-browse-sh-ssrf

Conversation

@zapabob

@zapabob zapabob commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • BrowseShSource fetched catalog-supplied skillMdUrl (and the detail endpoint) with raw httpx.get(..., follow_redirects=True), so a malicious CDN URL or redirect hop could reach private/link-local addresses.
  • Route both the detail lookup and content download through the existing _guarded_http_get helper (SSRF + hop re-validation), matching the rest of Skills Hub.
  • Add a regression test that a private skillMdUrl is refused.

Test plan

  • pytest tests/tools/test_skills_hub_browse_sh.py (8 passed)

Route BrowseShSource detail and CDN content fetches through
_guarded_http_get so catalog-supplied skillMdUrl values cannot
redirect to private or link-local targets.

Co-authored-by: Cursor <cursoragent@cursor.com>
@alt-glitch alt-glitch added type/security Security vulnerability or hardening tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have needs-repro Bug needs reproduction steps sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 23, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the unguarded browse.sh path. The premise is confirmed on current main: BrowseShSource.fetch() uses raw redirect-following httpx.get for skillMdUrl at tools/skills_hub.py:3004, and _resolve_skill_md_url() does the same for the detail endpoint at tools/skills_hub.py:3035. Routing both through _guarded_http_get matches its per-hop validation at tools/skills_hub.py:302-337.

Problems

  • The test hunk needs current-HEAD adaptation. Commit 39975613b13b418e0eceda178434d7be90ad4f91 removed the BrowseShSource fetch tests that this PR changes, so tests/tools/test_skills_hub_browse_sh.py no longer contains those anchors.

Suggested changes

  • During salvage, add the private-skillMdUrl regression as a standalone test in the current BrowseShSource test file, while retaining the production guarded-fetch conversion.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 30, 2026
@Adolanium

Copy link
Copy Markdown
Contributor

This is a slice of #63920, not a leftover.

#63920 already routes browse.sh skillMdUrl / detail through _guarded_http_get (and the other hub GETs). Open since Jul 13. This PR is the browse.sh-only cut from Jul 23, with #70334 / #70336 / #70343 doing the same for ClawHub, skills.sh, and GitHub.

"Not merged yet" is not the same as "browse.sh is unguarded." Please don't land this as the browse.sh fix over #63920.

@zapabob

zapabob commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Closing this as a duplicate of #63920. The latter covers the same BrowseSh skillMdUrl and detail-fetch SSRF paths, uses the current test layout, and also consolidates the shared redirect guard across the related Skills Hub sources. This branch's tests target anchors that no longer exist on current main, so maintaining a second implementation would create unnecessary overlap.

@zapabob zapabob closed this Aug 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have 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