tmproto: URL validation + SSRF contract on URL-bearing fields

[sujanchalla0510]

Closes #49.

Design choice

The issue lists three options: named FetchableURL/PublicContentURL wrapper types, validators alongside Validate() on each wrapper type, or doc-comment-only (explicitly ruled out as insufficient). tmproto already has an established per-type Validate() convention (tmproto/validate_ladder.go: ArtifactRef.Validate(), Artifact.Validate(), ImageAsset.Validate(), VideoAsset.Validate(), AudioAsset.Validate(), AssetAccess.Validate(), all field-level, all called transitively from ValidateContextRequest). I went with option 2 — a shared exported validator function wired into those existing Validate() methods — rather than introducing new named URL types, so every request already running through tmproto.ValidateContextRequest gets SSRF-safety checked for free, with no call-site changes required anywhere that already calls Validate().

What changed

• tmproto/urlsecurity.go (new): ValidateFetchableURL(raw string) error — a synchronous, dependency-free (net/url + net only) pre-flight validator:

  • Scheme allowlist: http/https only (rejects file://, javascript:, data:, etc).
  • Rejects embedded userinfo/credentials (https://user:pass@host/...).
  • Rejects known-internal hostname patterns: localhost (+ subdomains), .local, .internal, malformed/empty DNS labels.
  • When the host is an IP literal, rejects private/reserved ranges: loopback, RFC 1918 (10/8, 172.16/12, 192.168/16), link-local 169.254/16 (covers the cloud-metadata endpoint 169.254.169.254), CGNAT 100.64.0.0/10, benchmarking 198.18.0.0/15, 0.0.0.0/8, unspecified/multicast, IPv6 ULA fc00::/7, deprecated site-local fec0::/10, and IPv6-tunneled-private forms (6to4 2002::/16, NAT64 64:ff9b::/96) that embed a private IPv4 address.
  • Length-capped (MaxContentURLLength = 2048), matching the existing MaxSellerAgentURLLength convention in validate.go.
  • ErrUnsafeURL sentinel lets callers distinguish "malformed" from "syntactically fine but blocked" via errors.Is.
  • validateArtifactRefURL additionally rejects query strings and fragments for ArtifactRefTypeURL, per the spec's identity-leak-vector rule on ArtifactRef.value.

• tmproto/validate_ladder.go: wires ValidateFetchableURL into ArtifactRef.Validate() (type=url only), Artifact.Validate() (URL, optional), ImageAsset.Validate() (URL), VideoAsset.Validate() (URL + optional ThumbnailURL), AudioAsset.Validate() (URL). Error messages describe the rule violated, never the raw value — consistent with this file's existing convention (validateSafeID, validateSellerAgentURL) and with AGENTS.md's "never interpolate user-supplied values into error messages" rule, since ValidateContextRequest's error is echoed verbatim in the 400 response body by targeting/contextagent/handler.go.

• tmproto/artifact.go: package-level doc contract — a new "URL fields MUST be validated before fetch (SSRF contract)" section on the Artifact doc comment, plus a pointer from each of ImageAsset/VideoAsset/AudioAsset's doc comments to ValidateFetchableURL.

• tmproto/urlsecurity_test.go (new): table-driven tests (this package's established testify style — see urlcanon_test.go, robustness_test.go) covering valid public URLs; disallowed schemes; credentials-in-URL; the full private/reserved IPv4 and IPv6 range list including explicit CGNAT boundary tests (100.63.255.255 allowed / 100.64.0.0–100.127.255.255 rejected); IPv6 6to4/NAT64 tunneled addresses (rejects when the embedded v4 is private, allows when public); internal hostname patterns; oversized/malformed URLs; that error text never echoes the raw URL; ArtifactRef.Validate() query/fragment/userinfo/SSRF rejection for type=url, and that other ArtifactRef types are unaffected; and that Artifact.Validate() recurses into a malicious asset URL nested in Assets.

Where the rule set came from

Per the issue, I read the canonical TypeScript SSRF validator in the main adcontextprotocol/adcp repo — server/src/utils/url-security.ts (isPrivateHostname, normalizeExternalHostname, validateExternalUrl, as it stands after adcontextprotocol/adcp#7091) — rather than inventing a private-range list from scratch. That PR is the same author's prior fix to that exact file: it closed a gap where the CGNAT range (100.64.0.0/10) was missing from isPrivateHostname. This Go port includes CGNAT from the start (and is tested at both its boundaries) specifically so the same class of gap doesn't reopen in a second language. isDisallowedIP in this PR is also cross-checked against this SDK's own existing dial-time SSRF guard, adcp/signing.NewSafeHTTPClient's isDisallowedIP (a separate Go module — logic is intentionally duplicated rather than imported, per AGENTS.md's zero-unnecessary-dependencies rule for tmproto), and extends it with the IPv6 6to4/NAT64-tunneled-private-address checks the TypeScript validator has that the existing Go dial-time guard does not yet cover.

ValidateFetchableURL is deliberately a synchronous, DNS-free pre-flight only (mirrors the TS repo's own validateExternalUrl, which the TS repo's docs describe as "no DNS... for stronger SSRF guarantees... prefer safeFetch at fetch time"). A hostname that isn't an IP literal and doesn't match a known-internal suffix passes this check even if it later resolves to a private address, or is DNS-rebound between validation and fetch — this function cannot close that TOCTOU window on its own. The doc comment on Artifact says so explicitly and points callers at adcp/signing.NewSafeHTTPClient as this SDK's reference dial-time guard.

Acceptance criteria

• ArtifactRef.Validate() rejects obviously user-specific URLs — query string, fragment, and userinfo are all rejected when Type == url, plus full SSRF validation via ValidateFetchableURL. Note on scope: the spec text also says "no user-specific path segments" — that half of the rule is a content judgment (a path can't be told apart from a legitimate article slug by shape alone) that can't be enforced structurally; the doc comment on validateArtifactRefURL calls this out as a publisher obligation this SDK cannot mechanically check.
• Package-level doc on Artifact asset URLs documents the MUST-validate-before-fetch contract — see the new section on Artifact's doc comment and the per-field notes on ImageAsset/VideoAsset/AudioAsset.
• Shared URL-validation helper (SSRF-safe) that matches the AdCP webhook rules — tmproto.ValidateFetchableURL, exported and reusable outside tmproto.
• / [x] (with a caveat, stated honestly) Used by any code in the SDK that fetches publisher URLs — wired into every Validate() call for the fields this issue names, which is the actual enforcement point today: I confirmed by grep that no code currently exists anywhere in this SDK that performs an HTTP fetch of ArtifactRef/Artifact/asset URLs — the targeting engine only hashes/matches ArtifactRefTypeURL values (targeting/engine_signals.go), it never dereferences them, and no reference agent fetches Artifact.Assets[*].URL. I deliberately did not retrofit this validator into tmproto's existing JWKS/registry HTTP clients (keystore_jwks.go, keystore_authorization.go, keystore_remote.go): those fetch operator-configured or origin-pinned URLs (documented and gosec-justified as such), and several of their own tests intentionally point at httptest.NewServer (loopback) with an explicit AllowInsecureScheme local-dev opt-in — a hard SSRF block there would break that documented, tested contract without the issue asking for it. ValidateFetchableURL is exported specifically so the first piece of SDK code that does add a content-fetch path (buyer-side content evaluation, per the issue's own framing) has the validated, tested helper ready to call.

Testing

• go build ./..., go vet ./..., go test ./... — clean in tmproto/, the root module, and every other module in the workspace except two pre-existing, unrelated issues I verified also reproduce identically on unmodified upstream/main: reference/seller-agent has a pre-existing compile break unrelated to URLs (adcp.ProductAllowedAction / budget-type mismatch), and e2e's TestPerformance_EndToEnd/throughput load test occasionally hits local ephemeral-port exhaustion in this sandbox (dial tcp 127.0.0.1:NNNNN: connect: can't assign requested address) — confirmed by git stash-ing this change and rerunning both.
• gofmt -l clean on every file this PR touches.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc

[garvitkaushik-123]

Could you take a look at IPv6 addresses that include a zone identifier? I tried http://[::1%25lo0]/, http://[fe80::1%25eth0]/, and http://[fd00::1%25eth0]/ locally, and ValidateFetchableURL accepted all three, even though they point to loopback or private addresses.

In this check, Hostname() retains the zone suffix, but net.ParseIP cannot parse it. The address then passes through as a regular hostname.

Would it make sense to reject these addresses, or check the IP separately from its zone, and add a test for this case? I saw the note about checking addresses again when connecting. This finding is limited to the initial URL validation.

[bokelley]

Good catch — this is a real gap. Here's what's happening and the exact fix.

Root cause

url.URL.Hostname() percent-decodes the zone-ID separator, so [::1%25lo0] becomes ::1%lo0. net.ParseIP cannot handle zone suffixes and returns nil for them, so checkHostnameNotInternal falls past the IP-literal branch and reaches the final return nil — passing the address as though it were an ordinary hostname.

All three addresses you tested hit this path: ::1 (loopback), fe80::1 (link-local), and fd00::1 (ULA, caught by the fc00::/7 check) — all disallowed by isDisallowedIP but never seen by it.

Fix — tmproto/urlsecurity.go

In checkHostnameNotInternal, replace the existing IP-literal block:

	// IP literal (v4, v6, or v6 with brackets already stripped by
	// url.URL.Hostname): classify by range.
	if ip := net.ParseIP(lower); ip != nil {
		if isDisallowedIP(ip) {
			return errors.New("host resolves to a disallowed private/reserved address")
		}
		return nil
	}

with:

	// IP literal (v4, v6, or v6 with brackets already stripped by
	// url.URL.Hostname): classify by range.
	//
	// Strip any IPv6 zone identifier before passing to net.ParseIP.
	// url.URL.Hostname() percent-decodes the zone separator, so "[::1%25lo0]"
	// yields "::1%lo0". net.ParseIP cannot parse zone suffixes and returns nil,
	// letting a zoned loopback or link-local address fall through to the
	// hostname path and pass unchecked.
	ipStr := lower
	if pct := strings.IndexByte(ipStr, '%'); pct >= 0 {
		ipStr = ipStr[:pct]
	}
	if ip := net.ParseIP(ipStr); ip != nil {
		if isDisallowedIP(ip) {
			return errors.New("host resolves to a disallowed private/reserved address")
		}
		return nil
	}

No new imports are needed — strings.IndexByte is already covered by the existing "strings" import.

Tests — tmproto/urlsecurity_test.go

Add three entries to the TestValidateFetchableURL_RejectsPrivateAndReservedIPv6 cases map:

"loopback zone-id":   "http://[::1%25lo0]/",
"link-local zone-id": "http://[fe80::1%25eth0]/",
"ula zone-id":        "http://[fd00::1%25eth0]/",

Each will now reach net.ParseIP with the zone stripped (::1, fe80::1, fd00::1 respectively), be parsed as a valid IPv6 literal, and be rejected by isDisallowedIP — loopback via IsLoopback(), link-local via IsLinkLocalUnicast(), and ULA via the ip[0]&0xfe == 0xfc check.

Note on scope: this only affects URLs where the host contains a % after zone-ID decoding. Plain IPv4 literals and zone-free IPv6 literals are unaffected (they have no %), and the zone strip is a no-op for non-IP hostnames.

---

Generated by Claude Code

[bokelley]

@bokelley's analysis and fix are confirmed correct. Applied the patch locally and verified:

• go build ./tmproto/... ✓
• go vet ./tmproto/... ✓
• go test ./tmproto/... ✓ — all existing tests pass, and the three new zone-ID cases (loopback zone-id, link-local zone-id, ula zone-id) now correctly reject instead of passing through

Since the PR head branch lives in the fork (sujanchalla0510/adcp-go), this session can't push to it directly. @sujanchalla0510, please apply the two-part change from @bokelley's comment:

1. tmproto/urlsecurity.go — replace the net.ParseIP(lower) block in checkHostnameNotInternal with the zone-strip + net.ParseIP(ipStr) version (exact replacement shown in the comment above).

2. tmproto/urlsecurity_test.go — add the three entries to TestValidateFetchableURL_RejectsPrivateAndReservedIPv6:

   "loopback zone-id":   "http://[::1%25lo0]/",
   "link-local zone-id": "http://[fe80::1%25eth0]/",
   "ula zone-id":        "http://[fd00::1%25eth0]/",

No new imports are needed; strings.IndexByte is already covered by the existing "strings" import.

---

Generated by Claude Code

[bokelley]

The fix is also available as a verified, tested commit on adcontextprotocol/adcp-go at branch claude/pr-476-ssrf-zone-id-fix (commit e984096). @sujanchalla0510 can cherry-pick it directly:

git remote add upstream https://github.com/adcontextprotocol/adcp-go.git
git fetch upstream claude/pr-476-ssrf-zone-id-fix
git cherry-pick e984096
git push

Or a maintainer can apply it directly to the PR head if they have fork write access.

---

Generated by Claude Code

[aao-secretariat]

Ladon verdict: Approve

Approve — clean PR, no blocking or medium findings.

Checked:

• Adds tmproto.ValidateFetchableURL (SSRF-safe, DNS-free pre-flight) wired into per-type Validate() ladder for publisher-supplied URL fields.
• isDisallowedIP faithfully mirrors the sibling adcp/signing guard and correctly extends it with IPv6 6to4/NAT64 tunneled-private checks (bitmask ranges + boundary tests confirmed).
• feat conventional-commit marker is appropriate; no schema/generated-type or TMP-signing surface touched — no wire-shape/interop/auth impact.
• Error messages do not leak the raw URL.

High-risk flag is true only because files match tmproto/**, but the reasons are (added)/(modified) with no medium-or-higher findings, so no escalation trigger applies. gated_paths is false; no no-auto-approve team match. Falls through to row 9.

[bokelley]

Maintainer review: URL validation is DNS-free, rejects dangerous literal/network forms, and keeps user-controlled URLs out of errors. Approved pending CI.

[bokelley]

Superseded by #486 solely to satisfy the repository CodeQL ruleset, which GitHub default setup cannot evaluate on fork PRs. The original commit and contributor authorship are preserved in #486.

[aao-secretariat]

Ladon verdict: Approve

Approve — no blocking findings.

This is a subsequent pass on PR #476. The head SHA (c63ecd3) differs from the prior clean-approve SHA (27eeba4) only by a merge of main touching .release-please-manifest.json and adcp/v3/CHANGELOG.md — none of the four PR code files changed, so they remain byte-identical to the previously approved state. The reviewer re-scanned the SSRF validator (tmproto/urlsecurity.go) anyway: mask arithmetic on private/reserved ranges verifies against pinned boundary tests, and the 6to4/NAT64 recursion into isDisallowedIP is bounded.

Checks:

• No critical/high/medium findings.
• high_risk is true (all matches are tmproto/**), but the two modified files carry no medium-or-higher concerns and the two new files are additive scaffolding — no escalation trigger.
• gated_paths is false; review_decision is APPROVED.
• No no-auto-approve team match.

Decision table: rows 1–8 do not fire (no findings, no deletions, no gated-path gate, no team gate, fewer than three medium findings). Falls through to row 9 → approve. Re-affirms the prior clean approve.