fix(skills): block private SSRF targets and revalidate redirects in importer

[EmirCobanOfficial]

Summary

The skill importer (services/memory/skill_importer.py) makes outbound fetches for the admin-only POST /api/skills/import-from-url. It validated only the initial URL with the lenient SSRF guard (check_outbound_url → block_private=False) and then fetched with httpx.Client(follow_redirects=True), so a 3xx to an internal/metadata address (169.254.169.254, 127.0.0.1, RFC-1918) was still connected to — inconsistent with the hardened services/search/content.py:_get_public_url path, which pins the resolved IP and re-validates every redirect hop. This PR adds a _get_checked() helper that follows redirects manually and re-runs the SSRF guard with block_private=True on every hop, then routes all three fetch sites (skills.sh unwrap, _fetch_bytes, _list_github_dir) through it. GitHub's own redirects and the final-host _assert_github_url checks are preserved.

Scope / severity: defense-in-depth, not a cross-boundary vulnerability — the endpoint is require_admin-gated and admins are trusted per THREAT_MODEL.md; the response is discarded by the final GitHub-host check, so the effect today is a blind request. Prepared with AI assistance (Claude Code): a single, human-reviewed change with regression tests, not a bulk auto-generated PR.

Target branch

• This PR targets dev, not main.

Linked Issue

Part of #5262

Type of Change

• Bug fix (non-breaking — fixes a confirmed issue)
• New feature (non-breaking — adds new behaviour)
• Breaking change (changes or removes existing behaviour)
• Refactor / cleanup (behaviour unchanged)
• Documentation only
• CI / tooling / configuration

Checklist

• I searched open issues and open PRs — this is not a duplicate.
• This PR targets dev
• My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
• I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. — Verified at the unit level (see How to Test); I did not run the full app end-to-end, so this box is intentionally left unchecked. A maintainer / CI run is recommended.

How to Test

Hermetic unit tests (IP-literal hosts, faked HTTP layer — no network):

1. Run the importer SSRF tests:

   pytest tests/test_skill_importer_ssrf_redirect.py tests/test_skill_importer.py -q
   

2. Expected — the new suite passes, including:
   • test_get_checked_blocks_redirect_to_internal[127.0.0.1] / [169.254.169.254] — a redirect to an internal/metadata target now raises SkillImportError (before this PR the request was still sent).
   • test_fetch_bytes_blocks_redirect_to_internal[...] — same via the public fetch helper.
   • test_skills_sh_entry_blocks_redirect_to_metadata — the skills.sh unwrap path revalidates hops.
   • test_get_checked_follows_public_redirect — a legitimate public→public redirect is still followed (no over-blocking).
3. No UI changes; nothing to screenshot.

What changed

• services/memory/skill_importer.py — add _check_fetch_url() (block_private=True) + _get_checked() (manual, per-hop-revalidated redirects); route the three fetch sites through it.
• tests/test_skill_importer_ssrf_redirect.py — new hermetic regression tests.
• tests/test_skill_importer.py — update the existing check_outbound_url mock signature for the new block_private kwarg.

---

🤖 Prepared with assistance from Claude Code.

[StressTestor]

nice first PR, and a real catch. follow_redirects=True with the SSRF check on the initial URL only was a genuine blind-SSRF gap, and _get_checked closes the redirect vector cleanly. hermetic tests too, good to see.

one note, not a blocker: the summary says this matches services/search/content.py:_get_public_url, and it's very close but not quite the same guarantee. that path doesn't just re-validate each hop. it resolves the hop, checks the IP, then pins the actual TCP connect to that resolved IP via _PinnedTransport, which closes the DNS-rebinding TOCTOU between the check and the connect. _get_checked runs check_outbound_url(...) and then hands the hostname back to a plain httpx.Client that re-resolves at connect time, so a hostname whose DNS flips between check and fetch could still land on an internal IP. same admin-only, response-discarded envelope you already scoped, so practical risk is low. just flagging so the parity note isn't taken at face value later.

if you're up for it, one option is to route these fetches through the existing _resolve_public_ips + _PinnedTransport helpers (or call _get_public_url directly) so there's a single hardened fetch path rather than two similar ones. happy to help wire that up if useful. either way, solid contribution.

[EmirCobanOfficial]

Hi @StressTestor , thank you for the detailed review and the explanation about the DNS-rebinding/TOCTOU gap! I completely understand your point about _PinnedTransport. Since I used AI assistance to prepare this PR and closed my session, I would love to route these fetches through _get_public_url or _PinnedTransport as you suggested. Could you give me a quick pointer on how best to wire that up here? I'd appreciate the help to make it a single hardened fetch path

[StressTestor]

thanks for digging back in on this. honestly i'd land your PR as-is: the per-hop revalidation is the right fix for the redirect vector and it's self-contained.

the remaining bit (the DNS-rebinding window i mentioned) can only really be closed by pinning the TCP connect to the IP you validated, and the only pinning helper in the repo today is private inside services/search/content.py. wiring your importer to reach into that would mean a private cross-module import and basically a second copy of that fetch path, which is the thing i was flagging in the first place. so rather than push that into your PR, i'll take it as a follow-up: promote the pinning helper into a shared spot and route both the web-fetch path and the importer through it. you shouldn't have to carry a refactor of someone else's module to land your fix.

so nothing needed from you here. nice work chasing the redirect gap down, this is a real hardening.

[EmirCobanOfficial]

Sounds great! Thank you for the detailed explanation and for taking care of the follow up refactoring. I'm glad this PR helps harden the system. Looking forward to contributing again in the future

[RaresKeY]

Jumping in with one regression test gap. I rechecked the latest head against the current target branch and the discussion here. The DNS-pinning item is already correctly separated into the shared-fetch follow-up, but I found one independent regression-test gap that should be fixed before merge.

Findings

issue (test): The redirect regression fake ignores the safety-critical client setting

• Problem: The new fake client accepts but ignores follow_redirects, so changing the production helper back to follow_redirects=True still leaves all 21 focused importer tests green. With real httpx behavior, that setting follows a redirect to a loopback URL before the manual next-hop check runs.

• Impact: A one-line regression can restore the blind internal request while the security tests continue reporting success, so the suite does not protect the central invariant introduced here.

• Ask: Make the fake assert that follow_redirects is exactly False, or use a transport-backed fake that honors httpx redirect policy. The same production mutation should fail the regression test.

• Location: tests/test_skill_importer_ssrf_redirect.py:50

Validation

• Ran:
  • Focused importer and URL-safety tests: 32 passed.
  • The submitted tests still passed after the isolated follow_redirects=True mutation.
  • A one-line candidate assertion kept the normal suite green and made that mutation fail six redirect tests.
• Not run:
  • Live network import or controlled DNS-rebinding reproduction.

[EmirCobanOfficial]

Hi @RaresKeY, I've updated the mock client to explicitly assert follow_redirects=False as you requested (commit 7f862ea). The regression gap is now closed, and the tests will fail if the invariant is broken. Thank you for catching this and for your guidance

[EmirCobanOfficial]

Verified: flipping the production helper back to follow_redirects=True now fails 6 redirect tests (the _get_checked / _fetch_bytes / parse_skill_source cases), whereas previously all 21 focused importer tests stayed green. This matches your "the same production mutation should fail the regression test" criterion — the fake now enforces the invariant instead of silently accepting the kwarg.

[RaresKeY]

Thanks for the update. I checked the latest head and found one security issue that should be addressed before merge.

Findings

issue (security): Strict URL validation still permits shared-address redirects

• Problem: The new redirect helper relies on check_outbound_url(..., block_private=True), but strict mode still accepts RFC 6598 shared/CGNAT addresses such as 100.64.0.1. A public skills.sh redirect can therefore reach a shared-address service before the final GitHub-host check.

• Impact: The private-target protection remains bypassable for a blind request to a potentially internal service. The route is admin-gated and the response is discarded, but the exact security boundary this change adds is incomplete.

• Ask: Make strict mode reject non-global addresses (or explicitly reject RFC 6598 shared space), and add a public-to-100.64.0.1 redirect regression test that proves no second GET is issued.

• Location: src/url_safety.py:43 (used by services/memory/skill_importer.py:84 and :103-107)

Validation

• Ran: the focused importer and URL-safety suite passes, and the new mock assertion correctly fails when automatic redirects are restored.
• Not run: live network import or controlled DNS-rebinding validation.

[StressTestor]

good catch @RaresKeY, this one's real. confirmed the classification locally:

100.64.0.1 comes back is_private=False, is_global=False, is_loopback=False. the strict-mode branch keys on ip.is_private or ip.is_loopback, so CGNAT space falls straight through, and since _get_checked re-runs the same guard per hop, a public-to-100.64.0.1 redirect would pass the check and still issue the GET. link-local metadata (169.254.x) is already covered, so this is the residual. P2 is right.

one note on the fix so it doesn't bite later: not ip.is_global is the clean version, but it's python-version-dependent for this range. on 3.13+, CGNAT reads is_global=False and gets blocked; on older pythons is_global was just not is_private, so 100.64/10 read as global and would slip through. so either pin a 3.13+ floor or add an explicit 100.64.0.0/10 reject alongside it.

this lives in url_safety.py (the shared validator), not the importer, so i'll fold it into the shared-fetch-path follow-up i already picked up for the DNS-pinning item. that way the web-fetch path, the importer, and the admin embedding endpoint all get it from one place. @EmirCobanOfficial nothing needed from you, your redirect fix and the test tightening are solid, this is validator-level and i'll carry it.

[EmirCobanOfficial]

Thanks @RaresKeY for catching that CGNAT edge case, and thank you @StressTestor for stepping in to handle it at the validator level I learned a lot from this PR process.

[StressTestor]

@RaresKeY before i merge this, mind a final pass? i ran a deeper audit on the current head and it's clean from my side.

what i checked:

• the per-hop revalidation + block_private=True path is solid. every content-returning path is double-gated (per-hop IP guard + exact github host allowlist), and the host check held against the usual tricks (userinfo, trailing dot, %00, IDNA, obfuscated loopback). the one intentional exception is the skills.sh unwrap hop, which fetches an arbitrary public URL by design, guarded by block_private.
• no regression: the refactor only strengthens the guard (the old code was lenient on hop 0), drops no check, and the new tests genuinely go RED on revert.
• write-path traversal, symlink/submodule handling, and the frontmatter parse (hand-rolled, not yaml.load) are all clean.

residuals, none blocking:

• the DNS-rebinding TOCTOU is still open (the guard resolves, then httpx re-resolves at connect, not pinned). that's the shared connect-pinning follow-up i already picked up.
• the CGNAT gap you found closes on this path once fix(url-safety): reject RFC 6598 shared address space in strict mode #5474 lands (the importer inherits url_safety), so merging fix(url-safety): reject RFC 6598 shared address space in strict mode #5474 first is the cleaner order.

one thing the audit turned up: the same CGNAT class hits services/search/content.py too. its _is_private_address omits 100.64.0.0/10 and doesn't check is_global, and that's the always-on, connect-pinned fetch path. url_security.py already blocks it and url_safety.py is #5474, so content.py is the last one. one-line fix, folding it into the connect-pinning follow-up. flagging it so it's tracked, not a blocker here.

[StressTestor]

approving. audit came back clean on my side, and rareskey's out for a few days so no reason to keep this open longer. nice work @EmirCobanOfficial, solid hardening.