tmproto: URL validation + SSRF contract on URL-bearing fields - #486
Conversation
ArtifactRef.Value (type=url), Artifact.URL, ImageAsset.URL, VideoAsset.URL, VideoAsset.ThumbnailURL, and AudioAsset.URL were raw strings with no canonicalization check and no SSRF contract, even though a buyer agent may fetch several of them during content evaluation (#49). Adds ValidateFetchableURL, a shared, dependency-free (net/url + net only) pre-flight validator: http/https scheme allowlist, userinfo/credential rejection, and a private/reserved-IP-range check (loopback, RFC 1918, link-local, CGNAT 100.64.0.0/10, benchmark 198.18.0.0/15, IPv6 ULA/ deprecated-site-local/6to4/NAT64-tunneled-private) plus known-internal hostname patterns (localhost, *.local, *.internal, cloud metadata hosts). The range list is ported from and cross-checked against the AdCP webhook SSRF rules' canonical implementation, adcontextprotocol/adcp server/src/utils/url-security.ts (isPrivateHostname/validateExternalUrl) — including the CGNAT range that repo's own validator was found missing in adcontextprotocol/adcp#7091, so the same class of gap isn't reintroduced here in Go. Wires the validator into the existing Validate() methods on ArtifactRef, Artifact, ImageAsset, VideoAsset, and AudioAsset (following this package's established per-type Validate() convention rather than introducing new wrapper types), so every content-agent request already running through ValidateContextRequest gets SSRF-safety checked for free. ArtifactRef.Value additionally rejects query strings and fragments when type=url per the spec's identity-leak-vector rule. Adds a MUST-validate-before-fetch contract to the Artifact doc comment and each asset URL field, and documents that this SDK-side check is a synchronous, DNS-free pre-flight only — any actual fetch still needs a dial-time guard (e.g. adcp/signing.NewSafeHTTPClient) to close the DNS-rebind TOCTOU window. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — SSRF URL validation, error sanitization, and MCP test coverage look clean.
Checked:
tmproto/urlsecurity.go(added): SSRF private/reserved range logic verified correct at boundaries, wired live intoValidateContextRequest.targeting/contextagent/handler.go(modified): info-leak fix mirrors identityagent's sanitization exactly; contract-preserving.reference/seller-agent/cmd/seller-agent/main.go:newServerextraction is behavior-preserving.
No Critical/High/Medium findings. The one non-blocking note (pre-existing seller-agent compile break unrelated to this PR preventing new tests from running in CI) is a Low-tier follow-up and does not block.
Decision table: rows 1 (no critical/high) N/A; row 2 (gated_paths false) N/A; rows 3–5 (high_risk true but no medium finding and no deletions) N/A; no prior escalation; no no-auto-approve team; fewer than 3 mediums → falls through to row 9: approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Security-additive PR with no blocking or medium findings. Reviewed: a DNS-free SSRF pre-flight validator (ValidateFetchableURL) wired into tmproto asset/ArtifactRef Validate() paths, validation-error sanitization on the context-agent handler (generic "invalid request" body + server-side log, mirroring identityagent), and a mechanical main()→newServer extraction with CI-validated seller-agent tests. SSRF range logic (v4/v6, CGNAT, benchmark, 6to4/NAT64) is correct and boundary-tested; the dial-time TOCTOU gap is documented rather than hidden.
Decision path: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high-risk reasons are all (added) or (modified) with no (deleted) (row 3 n/a); no medium findings at all, so rows 4–5 n/a; no prior decision (row 6 n/a); no no-auto-approve team match (row 7 n/a); zero medium findings (row 8 n/a). Falls through to row 9 → approve.
The high_risk flag is a look-closer heuristic; modifications preserve contracts and the reviewer surfaced no medium-or-higher concerns. Two non-blocking observations noted (stricter validation may 400 previously-accepted non-canonical URLs; integer-IP hosts unroutable in Go's stack) — neither is actionable.
The merge-base changed after approval.
The .local TLD is on tmproto's SSRF blocklist (see tmproto/urlsecurity.go: reserved for mDNS per RFC 6762 §2), added in tmproto v0.3.0's ValidateFetchableURL contract on artifact_ref.value (#486). Once context-agent bumped to tmproto v0.3.0 in this batch, the container started rejecting every fixture that carried artifact_refs pointing at *.news.e2e.local with: invalid context-match request: artifact_refs[0]: artifact_ref.value: tmproto: url is not safe to fetch: url: host uses a reserved internal-only TLD (.local / .internal) Switch to the .test TLD (RFC 6761 reserved for testing — guaranteed unroutable and not on tmproto's blocklist). Fixture URLs are identifiers, not fetched, so the TLD change is transparent to the stack behavior. Renames: seller.e2e.local → seller.e2e.test, news.e2e.local → news.e2e.test, video.e2e.local → video.e2e.test, shuttered.e2e.local → shuttered.e2e.test (6 sites). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Policy-compliant relay of external contribution #476 so repository-default CodeQL can run. The original contributor remains the commit author; maintainer and Ladon reviews plus CI passed on #476.
Closes #49.
Design choice
The issue lists three options: named
FetchableURL/PublicContentURLwrapper types, validators alongsideValidate()on each wrapper type, or doc-comment-only (explicitly ruled out as insufficient).tmprotoalready has an established per-typeValidate()convention (tmproto/validate_ladder.go:ArtifactRef.Validate(),Artifact.Validate(),ImageAsset.Validate(),VideoAsset.Validate(),AudioAsset.Validate(),AssetAccess.Validate(), all field-level, all called transitively fromValidateContextRequest). I went with option 2 — a shared exported validator function wired into those existingValidate()methods — rather than introducing new named URL types, so every request already running throughtmproto.ValidateContextRequestgets SSRF-safety checked for free, with no call-site changes required anywhere that already callsValidate().What changed
tmproto/urlsecurity.go(new):ValidateFetchableURL(raw string) error— a synchronous, dependency-free (net/url+netonly) pre-flight validator:http/httpsonly (rejectsfile://,javascript:,data:, etc).https://user:pass@host/...).localhost(+ subdomains),.local,.internal, malformed/empty DNS labels.10/8,172.16/12,192.168/16), link-local169.254/16(covers the cloud-metadata endpoint169.254.169.254), CGNAT100.64.0.0/10, benchmarking198.18.0.0/15,0.0.0.0/8, unspecified/multicast, IPv6 ULAfc00::/7, deprecated site-localfec0::/10, and IPv6-tunneled-private forms (6to42002::/16, NAT6464:ff9b::/96) that embed a private IPv4 address.MaxContentURLLength = 2048), matching the existingMaxSellerAgentURLLengthconvention invalidate.go.ErrUnsafeURLsentinel lets callers distinguish "malformed" from "syntactically fine but blocked" viaerrors.Is.validateArtifactRefURLadditionally rejects query strings and fragments forArtifactRefTypeURL, per the spec's identity-leak-vector rule onArtifactRef.value.tmproto/validate_ladder.go: wiresValidateFetchableURLintoArtifactRef.Validate()(type=url only),Artifact.Validate()(URL, optional),ImageAsset.Validate()(URL),VideoAsset.Validate()(URL+ optionalThumbnailURL),AudioAsset.Validate()(URL). Error messages describe the rule violated, never the raw value — consistent with this file's existing convention (validateSafeID,validateSellerAgentURL) and withAGENTS.md's "never interpolate user-supplied values into error messages" rule, sinceValidateContextRequest's error is echoed verbatim in the 400 response body bytargeting/contextagent/handler.go.tmproto/artifact.go: package-level doc contract — a new "URL fields MUST be validated before fetch (SSRF contract)" section on theArtifactdoc comment, plus a pointer from each ofImageAsset/VideoAsset/AudioAsset's doc comments toValidateFetchableURL.tmproto/urlsecurity_test.go(new): table-driven tests (this package's establishedtestifystyle — seeurlcanon_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.255allowed /100.64.0.0–100.127.255.255rejected); 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 fortype=url, and that otherArtifactReftypes are unaffected; and thatArtifact.Validate()recurses into a malicious asset URL nested inAssets.Where the rule set came from
Per the issue, I read the canonical TypeScript SSRF validator in the main
adcontextprotocol/adcprepo —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 fromisPrivateHostname. 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.isDisallowedIPin this PR is also cross-checked against this SDK's own existing dial-time SSRF guard,adcp/signing.NewSafeHTTPClient'sisDisallowedIP(a separate Go module — logic is intentionally duplicated rather than imported, perAGENTS.md's zero-unnecessary-dependencies rule fortmproto), 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.ValidateFetchableURLis deliberately a synchronous, DNS-free pre-flight only (mirrors the TS repo's ownvalidateExternalUrl, which the TS repo's docs describe as "no DNS... for stronger SSRF guarantees... prefersafeFetchat 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 onArtifactsays so explicitly and points callers atadcp/signing.NewSafeHTTPClientas this SDK's reference dial-time guard.Acceptance criteria
ArtifactRef.Validate()rejects obviously user-specific URLs — query string, fragment, and userinfo are all rejected whenType == url, plus full SSRF validation viaValidateFetchableURL. 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 onvalidateArtifactRefURLcalls this out as a publisher obligation this SDK cannot mechanically check.Artifact's doc comment and the per-field notes onImageAsset/VideoAsset/AudioAsset.tmproto.ValidateFetchableURL, exported and reusable outsidetmproto.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 ofArtifactRef/Artifact/asset URLs — the targeting engine only hashes/matchesArtifactRefTypeURLvalues (targeting/engine_signals.go), it never dereferences them, and no reference agent fetchesArtifact.Assets[*].URL. I deliberately did not retrofit this validator intotmproto's existing JWKS/registry HTTP clients (keystore_jwks.go,keystore_authorization.go,keystore_remote.go): those fetch operator-configured or origin-pinned URLs (documented andgosec-justified as such), and several of their own tests intentionally point athttptest.NewServer(loopback) with an explicitAllowInsecureSchemelocal-dev opt-in — a hard SSRF block there would break that documented, tested contract without the issue asking for it.ValidateFetchableURLis 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 intmproto/, the root module, and every other module in the workspace except two pre-existing, unrelated issues I verified also reproduce identically on unmodifiedupstream/main:reference/seller-agenthas a pre-existing compile break unrelated to URLs (adcp.ProductAllowedAction/ budget-type mismatch), ande2e'sTestPerformance_EndToEnd/throughputload test occasionally hits local ephemeral-port exhaustion in this sandbox (dial tcp 127.0.0.1:NNNNN: connect: can't assign requested address) — confirmed bygit stash-ing this change and rerunning both.gofmt -lclean on every file this PR touches.🤖 Generated with Claude Code
https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc