fix(skills): harden skill import against DNS rebinding and SSRF TOCTOU

[bitboody]

Summary

Hardened the skill import fetch path (services/memory/skill_importer.py and outbound URL validation utilities) against multiple vectors of Server-Side Request Forgery (SSRF). This PR fixes a critical DNS rebinding time-of-check to time-of-use (TOCTOU) vulnerability where httpx performed an independent second DNS lookup on redirect hops, allowing attackers to swap validated public IPs for internal targets (e.g., loopback, cloud metadata, or RFC 6598 CGNAT ranges). It introduces atomic IP pinning per hop, and strict domain boundary checking to block substring spoofing.

Target branch

• This PR targets dev, not main. All PRs land in dev; main is curated by the maintainer at each release. If your PR is on main by accident, click "Edit" on this PR and change the base.

Linked Issue

Fixes #5609

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. Type-checks and unit tests are not enough.

How to Test

1. Run the security test to verify SSRF guards, DNS rebinding mitigations, CGNAT blocking, and local exception overrides:

python -m pytest tests/test_skill_importer_security.py

2. Verify that attempting to import a skill bundle from a spoofed domain or an internal private IP range correctly triggers a SkillImportError.

Visual / UI changes — REQUIRED if you touched anything that renders

No visual changes done.

[RaresKeY]

Update: I pushed the reviewed four-commit fix directly onto this PR branch in 9dc2e87cc, 3fb472a07, ce24f5e71, and b1a17adf2. The PR now preserves the logical request identity while pinning sockets, rejects unsupported wrapper hosts before fetch, removes the unused shared exception, and adds deterministic DNS/redirect coverage. The commits were applied as a clean fast-forward on top of the contributor’s e6a374fc8 head and are patch-equivalent to the reviewed candidate series. Fresh GitHub checks are complete: all substantive checks passed, the expected Trivy SARIF upload is skipped, and the PR is MERGEABLE/CLEAN. The isolated runner previously returned no focused pytest result, but GitHub’s full Python test job passed.

---

Original review of head e6a374fc8:

I checked the latest head and found four author-actionable issues in the skill-import pinning and host-dispatch paths.

Findings

issue (runtime): Preserve the hostname URL when pinning the socket destination

• Problem: _get_checked still replaces the request authority with a validated IP and passes that IP URL to HTTPX. The response URL is therefore IP-based, but successful API/raw callers immediately require an exact GitHub hostname; relative redirects are also joined against the IP URL.
• Impact: Successful ordinary GitHub responses are rejected, so direct-file and directory imports cannot complete. Validating all DNS answers does not repair the logical URL/Host/SNI/response identity contract.
• Ask: Keep the original request URL and pin only the network backend's socket destination to the validated per-hop snapshot, following the existing repository transport pattern. Add a successful end-to-end fetch that proves socket destination and logical origin separately.
• Location: services/memory/skill_importer.py:147-175, services/memory/skill_importer.py:305-330

issue (security): Reject unsupported authorities before the skills unwrap request

• Problem: Initial dispatch accepts every *.skills.sh host and every numeric-IP/localhost URL whose path contains skills.sh, then fetches before exact GitHub-host validation. The new schemeless path repeats the wildcard admission.
• Impact: An unsupported public destination can be contacted from an admin-supplied import even though bug(security): skill importer has a DNS-rebinding TOCTOU (validation resolves, connect re-resolves) #5609 requires an exact supported-host gate before network access.
• Ask: Use an explicit supported skills-host set and remove the path/IP compatibility branch. Rewrite the synthetic IP fixture around the real supported host plus injected resolution.
• Location: services/memory/skill_importer.py:185-219

issue (tests): Exercise the validation-to-connect DNS change through the transport

• Problem: The new security test imports _get_checked but never calls it, and existing redirect mocks still use IP literals. No test makes validation return public A, a later answer be blocked B, and proves the socket uses A while URL/Host/SNI remain on the supported hostname.
• Impact: The defining bug(security): skill importer has a DNS-rebinding TOCTOU (validation resolves, connect re-resolves) #5609 acceptance criterion remains untested, allowing the response-URL outage through green CI.
• Ask: Add a deterministic resolver and recording backend that proves one validation snapshot controls the socket, preserves logical request/response identity and redirects, and prevents unsupported hosts from reaching fetch.
• Location: tests/test_skill_importer_security.py:1-111, tests/test_skill_importer_ssrf_redirect.py:32-75

issue (scope): Remove the unused shared private-destination bypass

• Problem: check_outbound_url still has an allowed_dist argument that disables private-address blocking on a caller-supplied netloc match, but no production caller uses it and bug(security): skill importer has a DNS-rebinding TOCTOU (validation resolves, connect re-resolves) #5609 does not need it.
• Impact: This focused importer fix unnecessarily expands a shared security API without a runtime owner or configuration contract.
• Ask: Remove it from this PR and introduce any operator-private-destination exception separately with a real caller and threat-model tests.
• Location: src/url_safety.py:60-80, tests/test_skill_importer_security.py:83-100

Related work

This is related to, but not duplicated or superseded by, #5261, #5474, #5727, #5893, or draft #5953. PR #5986 remains the only candidate implementing the skill-importer part of #5609.

Validation

• Current-head Python CI passed 4,911 tests with 4 skipped and 8 warnings; all other substantive security, syntax, dependency, container, and PR checks completed successfully, while the Trivy SARIF upload was expected-skipped.
• The three-commit follow-up delta was reviewed against the completed deep snapshot; all four findings remain in current source.
• The isolated secretless runner was unavailable during the deep pass, so live application, controlled-DNS, and network/TLS behavior were not run locally.

[bitboody]

Looks good to me.

[o3LL]

Reviewed head b1a17adf284210ebbe5a42b9330fe5b8d4d3bc65, including the four-commit delta on top of e6a374fc8.

The pinning approach is right and I confirmed it end-to-end: a real import of https://github.com/anthropics/skills/tree/main/skills/pdf through POST /api/skills/import-from-url on a local instance returned 12 files, and a local socket test shows the transport dialling the pinned IP while Host, SNI, and response.url stay on the original hostname. The P1 from the previous round is fixed.

Findings

issue (test): The only positive skills.sh test asserts a flow the live service cannot produce

• Problem: test_parse_skill_source_allows_exact_skills_host patches _get_checked to return url = "https://github.com/test-owner/test-repo". The real service does not do that. https://skills.sh/<path> returns 308 to https://www.skills.sh/<path>, and www.skills.sh serves 200 text/html — it never redirects on to github.com. So final is a www.skills.sh URL and _assert_github_url(final, context="redirect target") rejects it:

  $ curl -sIL https://skills.sh/anthropics/skills | grep -iE '^HTTP/|^location:|^content-type:'
  HTTP/2 308
  content-type: text/plain
  location: https://www.skills.sh/anthropics/skills
  HTTP/2 200
  content-type: text/html; charset=utf-8
  
  >>> parse_skill_source("https://skills.sh/anthropics/skills")
  SkillImportError: redirect target must stay on GitHub (got www.skills.sh)
  

  To be clear about blame: this is not a regression. dev produces the identical error, because _assert_github_url(final, ...) already ran before the old re.search branch, making that regex fallback unreachable. Deleting it was a correct dead-code removal.

• Impact: The skills.sh → GitHub flow named in the module docstring is broken, and this PR adds a green test that says it works. That test is what will stop the next person from finding the bug.

• Ask: Either drop the positive assertion (or xfail it with a follow-up issue and keep the negative host-rejection cases, which are the ones this PR actually earns), or fix the path properly — which means adding www.skills.sh to _SKILLS_SH_HOSTS and unwrapping the GitHub link from the page body. Related: narrowing to frozenset({"skills.sh"}) now rejects www.skills.sh at parse time, which is the host the service canonicalises to and the one a user copies out of their browser. _GITHUB_HOSTS already carries www.github.com; the asymmetry looks unintentional.

• Location: tests/test_skill_importer_security.py:33-43, services/memory/skill_importer.py:30, services/memory/skill_importer.py:294-301

issue (test): No test exercises the real pool or a real socket for the new transport

• Problem: All three transport tests stub out the layer under test. test_validation_snapshot_is_the_only_connect_destination overwrites backend._real; test_transport_preserves_request_authority_and_response_url closes the real pool and replaces it with _RecordingPool; test_get_checked_uses_fresh_transport_per_redirect_hop fakes httpx.Client wholesale. Nothing ever runs the httpcore.ConnectionPool(ssl_context=..., network_backend=_PinnedBackend(...)) wiring, the _HTTPCORE_TO_HTTPX_EXC mapping, or the httpcore.Response → httpx.Response re-wrap against a live connection.

• Impact: The re-wrap is where the last round's P1 lived, and the untested part of it is subtle — the transport hands httpx raw wire bytes plus the upstream Content-Encoding, so correct behaviour depends on httpx applying the decoder during Response.read(). It does today; nothing pins it. Both sibling copies of this transport have a real-socket test (tests/test_security_regressions.py:1309, tests/test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second); this one does not.

• Ask: Add the equivalent of test_dns_rebinding_pinned_transport_dials_pinned_ip — stand up a loopback server, point the transport at a hostname pinned to 127.0.0.1, and assert the socket destination, the Host header, response.url, and a gzip-encoded body decoding correctly. I ran exactly that manually and it passes, so this is a pinning-down exercise, not a bug hunt.

• Location: tests/test_skill_importer_dns_pinning.py:15-113

issue (scope, docs): The PR description advertises code the diff no longer contains

• Problem: The Summary still claims "a secure operator-configured local service override", and How to Test step 3 still says "configuring a local endpoint via allowed_dist". ce24f5e71 removed that — src/url_safety.py is not in the diff, and grep -rn allowed_dist over this head returns nothing. Separately, Type of Change still ticks "Refactor / cleanup (behaviour unchanged)" while the host dispatch behaviour genuinely changed from substring to exact match.

• Impact: Since merges here squash, the description is the durable record of the change. A reviewer working the How to Test list cannot execute step 3 at all.

• Ask: Drop the allowed_dist sentence from the Summary and step 3 from How to Test, and untick "Refactor / cleanup".

• Location: PR description

issue: Dead branch makes every non-HTTP scheme report "URL is required"

• Problem: The schemeless branch does urlparse("//" + url), and prefixing // guarantees scheme is always ''. So if parsed_rough.scheme: never fires and the unsupported URL scheme: {scheme} message below it is unreachable. parsed.scheme not in ("http", "https") two lines further down is unreachable for the same reason — by then url always starts with http:// or https://. Observed:

  'ftp://github.com/o/r'      -> SkillImportError: URL is required
  'file:///etc/passwd'        -> SkillImportError: URL is required
  'gopher://github.com/o/r'   -> SkillImportError: URL is required
  'javascript:alert(1)'       -> SkillImportError: URL is required
  'HTTPS://github.com/o/r'    -> SkillImportError: URL is required
  

  Rejecting these is an improvement — dev accepted ftp://github.com/o/r outright, because _github_host only looked at the hostname. The verdict is right; only the message is wrong. The last case is a separate slip: startswith is case-sensitive, so an uppercase scheme falls into the schemeless branch.

• Impact: "URL is required" for a non-empty URL sends the admin looking for the wrong problem, and the two unreachable branches read as live scheme validation to anyone maintaining this later.

• Ask: Validate the scheme once before the schemeless branch, on url.split("://", 1) or a plain urlparse(url), and delete the two unreachable branches. Lowercase the prefix test.

• Location: services/memory/skill_importer.py:271-286

Open Questions

• question (scope, non-blocking): should this be a third private copy of the pinned transport? _validated_ips, _PinnedBackend, _HTTPCORE_TO_HTTPX_EXC, _PinnedTransport, and the _recording_resolver + check_outbound_url + _validated_ips sequence are near-verbatim ports of src/integrations.py:363-472,561-583 and services/search/content.py:118-236. I know src/integrations.py:419-427 documents per-module copies as deliberate, so this follows precedent rather than breaking it. But refactor(search): extract outbound fetch transport #5953 is open as the mechanical extraction into src/outbound_fetch.py — the step 1 you laid out when closing fix(security): pin + cap outbound image fetches and consolidate the SSRF fetcher #5893 — and its compatibility wrappers cover services/search/content.py only, so a third copy is extra migration work landing while that stack is in flight. Two knock-ons if it stays as-is: the static hygiene guard test_dns_rebinding_transport_uses_public_apis (tests/test_security_regressions.py:1471) is hardcoded to content._PinnedTransport, so this copy sits outside it; and _validated_ips at services/memory/skill_importer.py:78-93 drops the docstring from src/integrations.py:475-489 explaining why de-duplication matters — that getaddrinfo with no socktype filter reports each address three times on glibc, so without it the connect fallback burns the shared deadline retrying one dead address. That reasoning is the non-obvious part and it is worth carrying over even if the duplication stays.

• question (non-blocking): is losing proxy support here intended? dev used a bare httpx.Client(), which honours HTTP_PROXY / HTTPS_PROXY via trust_env. _PinnedTransport ignores proxy configuration entirely, so a deployment behind an egress proxy loses skill import. Pinning and a proxy are fundamentally incompatible and both merged copies behave the same way, so this looks like an accepted project-level tradeoff rather than an oversight — flagging only because it is undocumented.

Validation

• Ran (head b1a17adf2, macOS 15.6 / Python 3.11.15, httpx 0.28.1, httpcore 1.0.9, CHROMADB_PORT pointed at a dead port):

  • Full suite: 2 failed, 4917 passed, 4 skipped, 8 warnings in 139.92s. Both failures (tests/test_workspace_confine.py::test_glob_confined_e2e, tests/test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second) reproduce identically on unmodified dev — macOS /private/tmp resolution and real-socket connect-refused timing.
  • python -m compileall on all five touched files — clean.
  • App booted on 127.0.0.1:7099; /api/health 200; POST /api/skills/import-from-url with https://github.com/anthropics/skills/tree/main/skills/pdf returned 200. Direct fetch_skill_bundle on the same URL plus the /blob/ and raw.githubusercontent.com forms each returned 12 files / 58,654 chars over real DNS and TLS.
  • Real-socket transport check: _PinnedTransport([127.0.0.1]) against a local gzip-serving server via http://pinned.example:<port>/skill — 200, response.url preserved as http://pinned.example:<port>/skill, body decoded correctly.
  • Live skills.sh probe by curl -sIL, and parse_skill_source against real skills.sh on both this head and dev.
  • scripts/pr_blocker_audit.py --repo odysseus-dev/odysseus --limit 130, plus changed-file comparison against fix(skills): block private SSRF targets and revalidate redirects in importer #5261, fix(url-safety): reject RFC 6598 shared address space in strict mode #5474, fix(integrations): pin api_call to the SSRF-validated IP #5727, fix(security): pin + cap outbound image fetches and consolidate the SSRF fetcher #5893, refactor(search): extract outbound fetch transport #5953.

• Not run: a controlled live-DNS rebinding reproduction (validation-time and connect-time answers genuinely differing at the resolver) — I verified the pinning contract at the socket and transport layers instead. No Docker path, no Linux, no IPv6-only target, and no proxied-egress environment.

• Residual risk: low for the GitHub path, which is the one that actually carries traffic and which I exercised end-to-end. The skills.sh path is unverifiable as written because it does not work against the live service — that is pre-existing, but it means the entry host this PR keeps in the allowlist has no working flow behind it.

PR Hygiene

• Target/template/checks: base dev, correct. Title matches the Conventional Commits pattern. All 19 GitHub checks pass (Trivy SARIF upload expected-skipped); ready for review label present; MERGEABLE / CLEAN. Description passes the bot but is stale in two places — see the P3 above. "No visual changes" is accurate; the diff touches no rendering code.
• Related, duplicate, or superseding context: not a duplicate. fix(skills): block private SSRF targets and revalidate redirects in importer #5261, fix(url-safety): reject RFC 6598 shared address space in strict mode #5474, and fix(integrations): pin api_call to the SSRF-validated IP #5727 are merged predecessors in the same family; fix(security): pin + cap outbound image fetches and consolidate the SSRF fetcher #5893 is closed; refactor(search): extract outbound fetch transport #5953 is the open draft extraction that the duplication question above bears on. fix(skills): harden skill import against DNS rebinding and SSRF TOCTOU #5986 remains the only candidate implementing the skill-importer half of bug(security): skill importer has a DNS-rebinding TOCTOU (validation resolves, connect re-resolves) #5609. Fixes is the right verb — bug(security): skill importer has a DNS-rebinding TOCTOU (validation resolves, connect re-resolves) #5609 is scoped to this importer and should close on merge.
• Review provenance: the four-commit delta from e6a374fc8 to b1a17adf2 was authored by the reviewer who raised the findings it addresses, and the author's response was a comment-only "Looks good to me." Worth an independent pair of eyes on that delta before merge, which is what this review is.