From 27eeba432a03f2265d9870b4cab56a92e3c9fc4b Mon Sep 17 00:00:00 2001 From: sujan reddy Date: Mon, 31 Aug 2026 16:39:49 -0500 Subject: [PATCH] feat(tmproto): add SSRF-safe URL validation for TMP content URL fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (adcontextprotocol/adcp-go#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 Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc --- tmproto/artifact.go | 38 +++++ tmproto/urlsecurity.go | 240 ++++++++++++++++++++++++++ tmproto/urlsecurity_test.go | 332 ++++++++++++++++++++++++++++++++++++ tmproto/validate_ladder.go | 45 ++++- 4 files changed, 650 insertions(+), 5 deletions(-) create mode 100644 tmproto/urlsecurity.go create mode 100644 tmproto/urlsecurity_test.go diff --git a/tmproto/artifact.go b/tmproto/artifact.go index f77eb34e..85e053f1 100644 --- a/tmproto/artifact.go +++ b/tmproto/artifact.go @@ -54,6 +54,10 @@ func (a *TextAsset) MarshalJSON() ([]byte, error) { } // ImageAsset is an image asset with its URL and optional display metadata. +// +// URL is publisher-supplied and MUST be validated with ValidateFetchableURL +// before a buyer agent fetches it — see the MUST-validate-before-fetch +// contract on the Artifact doc comment. type ImageAsset struct { URL string `json:"url"` Access *AssetAccess `json:"access,omitempty"` @@ -78,6 +82,10 @@ func (a *ImageAsset) MarshalJSON() ([]byte, error) { // VideoAsset is a video asset with its URL and optional transcript/metadata. // Transcript is publisher-supplied and MUST be treated as untrusted input. +// +// URL and ThumbnailURL are publisher-supplied and MUST be validated with +// ValidateFetchableURL before a buyer agent fetches either — see the +// MUST-validate-before-fetch contract on the Artifact doc comment. type VideoAsset struct { URL string `json:"url"` Access *AssetAccess `json:"access,omitempty"` @@ -103,6 +111,10 @@ func (a *VideoAsset) MarshalJSON() ([]byte, error) { // AudioAsset is an audio asset with its URL and optional transcript/metadata. // Transcript is publisher-supplied and MUST be treated as untrusted input. +// +// URL is publisher-supplied and MUST be validated with ValidateFetchableURL +// before a buyer agent fetches it — see the MUST-validate-before-fetch +// contract on the Artifact doc comment. type AudioAsset struct { URL string `json:"url"` Access *AssetAccess `json:"access,omitempty"` @@ -331,6 +343,32 @@ func (a AssetAccess) redacted() string { // Publishers MUST NOT include asset access credentials the buyer could use // outside this request flow — for secured assets, use signed URLs with short // expiry. Routers MUST call StripAccess before forwarding to multiple buyers. +// +// # URL fields MUST be validated before fetch (SSRF contract) +// +// URL (below) and every asset URL reachable through Assets — ImageAsset.URL, +// VideoAsset.URL, VideoAsset.ThumbnailURL, AudioAsset.URL — are +// publisher-supplied strings with no built-in transport-layer protection. +// A buyer agent (or any code embedding this SDK) that fetches one of these +// URLs during content evaluation is making a server-side request to an +// address chosen entirely by the request sender: without validation, a +// publisher (or an attacker impersonating one) can direct that fetch at +// cloud-metadata endpoints (`http://169.254.169.254/...`), loopback or +// RFC 1918 addresses, `file://`, or any other internal host — a classic SSRF +// primitive. +// +// Calling (Artifact).Validate() — and transitively each asset's Validate() +// — runs ValidateFetchableURL against every one of these fields and rejects +// disallowed schemes, embedded credentials, and known-private/reserved +// hosts. Callers MUST call Validate() (directly, or via +// ValidateContextRequest on the containing request) before trusting any URL +// field here, and MUST still layer a dial-time SSRF guard (DNS-rebind safe; +// see adcp/signing.NewSafeHTTPClient for this SDK's reference +// implementation) at the point where the fetch actually happens — +// ValidateFetchableURL is a synchronous, DNS-free pre-flight check, not a +// substitute for validating the address that gets dialed. See +// ValidateFetchableURL's doc comment for the full rule set and its +// provenance (ported from the AdCP webhook SSRF rules). type Artifact struct { PropertyRID string `json:"property_rid"` ArtifactID string `json:"artifact_id"` diff --git a/tmproto/urlsecurity.go b/tmproto/urlsecurity.go new file mode 100644 index 00000000..60778d87 --- /dev/null +++ b/tmproto/urlsecurity.go @@ -0,0 +1,240 @@ +package tmproto + +import ( + "errors" + "fmt" + "net" + "net/url" + "strings" +) + +// MaxContentURLLength caps every URL field this file validates (Artifact.URL, +// ImageAsset.URL, VideoAsset.URL, VideoAsset.ThumbnailURL, AudioAsset.URL, +// ArtifactRef.Value when Type == url). The value is copied into structured +// logs and (for ArtifactRef.Value) into content-hash inputs; an unbounded +// string is a log-size and allocation DoS vector the same way +// MaxSellerAgentURLLength bounds seller_agent_url. +const MaxContentURLLength = 2048 + +// ErrUnsafeURL is wrapped by ValidateFetchableURL when raw parses as a +// well-formed URL but is refused on SSRF grounds — as opposed to a plain +// parse/format failure. Callers that want to distinguish "malformed" from +// "syntactically fine but points somewhere it must not" can match on this +// with errors.Is. +var ErrUnsafeURL = errors.New("tmproto: url is not safe to fetch") + +// ValidateFetchableURL is the SDK's shared, SSRF-safe validator for +// publisher-supplied URLs that a buyer agent (or any embedding application) +// may dereference over HTTP(S): Artifact.URL, ImageAsset.URL, VideoAsset.URL, +// VideoAsset.ThumbnailURL, AudioAsset.URL, and ArtifactRef.Value when +// Type == ArtifactRefTypeURL. See the doc comment on Artifact for the full +// MUST-validate-before-fetch contract these fields carry. +// +// Checks performed (all synchronous, no DNS lookups or network I/O): +// - raw parses as an absolute URL and is within MaxContentURLLength. +// - Scheme is http or https (rejects file://, javascript://, data:, etc). +// - No userinfo / embedded credentials (rejects "https://user:pass@host/..."). +// - Host is present and is not a known-internal hostname pattern +// (localhost, *.localhost, *.local, *.internal, cloud metadata hostnames). +// - When the host is an IP literal, the address is not in a private, +// loopback, link-local, CGNAT, benchmark, unique-local, or otherwise +// reserved range — see isDisallowedIP. +// +// Rule source: the AdCP webhook SSRF validation rules +// (https://adcontextprotocol.org/docs/building/implementation/security#webhook-url-validation-ssrf), +// cross-checked against the canonical implementation in the main AdCP +// repository, adcontextprotocol/adcp server/src/utils/url-security.ts +// (isPrivateHostname / normalizeExternalHostname / validateExternalUrl as of +// adcontextprotocol/adcp#7091, which closed a CGNAT 100.64.0.0/10 gap in that +// same validator — the range list here was built to include it from the +// start, along with the IPv6 6to4/NAT64-tunneled-address checks the TS +// implementation carries that this SDK's other SSRF guard, +// adcp/signing.NewSafeHTTPClient's isDisallowedIP, does not yet have). +// +// What this function deliberately does NOT do: resolve DNS. A hostname that +// is not an IP literal and does not match a known-internal suffix passes +// here even if it will later resolve to a private address (or is rebound to +// one between validation and fetch). tmproto has no HTTP client of its own — +// any embedding application that actually performs the fetch MUST layer a +// dial-time guard that re-checks the resolved address at TCP-connect time +// (as adcp/signing.NewSafeHTTPClient does), not rely on this pre-flight +// check alone. Treat ValidateFetchableURL as the fast, always-applicable +// first gate; the dial-time guard closes the DNS-rebind TOCTOU window this +// function cannot. +func ValidateFetchableURL(raw string) error { + if raw == "" { + return errors.New("url: empty") + } + if len(raw) > MaxContentURLLength { + return fmt.Errorf("url: exceeds maximum length of %d", MaxContentURLLength) + } + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("url: does not parse: %w", err) + } + if !u.IsAbs() { + return errors.New("url: must be absolute") + } + scheme := strings.ToLower(u.Scheme) + if scheme != "http" && scheme != "https" { + return fmt.Errorf("%w: url: scheme must be http or https, got %q", ErrUnsafeURL, scheme) + } + if u.User != nil { + return fmt.Errorf("%w: url: must not contain userinfo (credentials)", ErrUnsafeURL) + } + host := u.Hostname() + if host == "" { + return fmt.Errorf("%w: url: missing host", ErrUnsafeURL) + } + if err := checkHostnameNotInternal(host); err != nil { + return fmt.Errorf("%w: url: %s", ErrUnsafeURL, err.Error()) + } + return nil +} + +// checkHostnameNotInternal rejects host strings that identify a +// known-internal target without needing DNS resolution: literal loopback / +// private / reserved IP addresses, "localhost" and its subdomains, and the +// .local / .internal reserved TLDs (mDNS and RFC 6762 §2 / cloud-provider +// internal-DNS conventions respectively — e.g. AWS/GCP internal service +// discovery and metadata.google.internal both live under .internal or a +// link-local IP literal, both covered here). +func checkHostnameNotInternal(host string) error { + lower := strings.ToLower(host) + // A single trailing dot is the canonical DNS root label; strip it before + // classifying so "localhost." isn't treated as some other TLD. + lower = strings.TrimSuffix(lower, ".") + if lower == "" { + return errors.New("empty host") + } + for _, label := range strings.Split(lower, ".") { + if label == "" { + // Empty label anywhere else (e.g. "example..com") is malformed; + // fail closed rather than let a resolver or proxy interpret it + // differently than this check did. + return errors.New("host contains an empty label") + } + } + if lower == "localhost" || strings.HasSuffix(lower, ".localhost") { + return errors.New("host is localhost") + } + if strings.HasSuffix(lower, ".local") || strings.HasSuffix(lower, ".internal") { + return errors.New("host uses a reserved internal-only TLD (.local / .internal)") + } + + // 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 + } + + // Not an IP literal and not a known-internal suffix: this is where + // syntactic validation stops. See the "deliberately does NOT" paragraph + // on ValidateFetchableURL — a dial-time guard must re-check the + // resolved address before connecting. + return nil +} + +// isDisallowedIP reports whether ip must not be dialed per the AdCP webhook +// SSRF rules. Mirrors adcp/signing.isDisallowedIP (a separate Go module, so +// duplicated rather than imported — tmproto has zero external deps by +// design) and additionally covers the IPv6 tunneling forms +// (6to4 2002::/16, NAT64 64:ff9b::/96) that the canonical TypeScript +// validator (adcontextprotocol/adcp server/src/utils/url-security.ts, +// isPrivateHostname) checks and adcp/signing's port does not yet have. +func isDisallowedIP(ip net.IP) bool { + if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || + ip.IsInterfaceLocalMulticast() || ip.IsMulticast() || ip.IsUnspecified() { + return true + } + if ip4 := ip.To4(); ip4 != nil { + switch { + case ip4[0] == 0: // 0.0.0.0/8 — "this network"; routes to loopback on Linux. + return true + case ip4[0] == 10: // 10.0.0.0/8 RFC 1918 private. + return true + case ip4[0] == 172 && ip4[1]&0xf0 == 16: // 172.16.0.0/12 RFC 1918 private. + return true + case ip4[0] == 192 && ip4[1] == 168: // 192.168.0.0/16 RFC 1918 private. + return true + case ip4[0] == 169 && ip4[1] == 254: // 169.254.0.0/16 link-local — covers the + // 169.254.169.254 cloud-metadata endpoint (AWS/GCP/Azure IMDS). + return true + case ip4[0] == 100 && ip4[1]&0xc0 == 64: // 100.64.0.0/10 CGNAT (RFC 6598). + return true + case ip4[0] == 198 && ip4[1]&0xfe == 18: // 198.18.0.0/15 benchmarking (RFC 2544). + return true + } + return false + } + if len(ip) != net.IPv6len { + return false + } + if ip[0]&0xfe == 0xfc { // fc00::/7 unique local address (RFC 4193). + return true + } + if ip[0] == 0xfe && ip[1]&0xc0 == 0xc0 { // fec0::/10 deprecated site-local (RFC 3879). + return true + } + // 6to4 2002::/16 (RFC 3056): bytes[2:6] carry the embedded IPv4 address. + if ip[0] == 0x20 && ip[1] == 0x02 { + if isDisallowedIP(net.IPv4(ip[2], ip[3], ip[4], ip[5])) { + return true + } + } + // NAT64 well-known prefix 64:ff9b::/96 (RFC 6052): bytes[12:16] carry the + // embedded IPv4 address; bytes[4:12] must be zero for the /96 to match. + if ip[0] == 0x00 && ip[1] == 0x64 && ip[2] == 0xff && ip[3] == 0x9b { + allZero := true + for _, b := range ip[4:12] { + if b != 0 { + allZero = false + break + } + } + if allZero && isDisallowedIP(net.IPv4(ip[12], ip[13], ip[14], ip[15])) { + return true + } + } + return false +} + +// validateArtifactRefURL enforces the ArtifactRefTypeURL contract from the +// TMP spec's ArtifactRef.value docstring for type=url: the value MUST be the +// bare canonical content URL, with no user-specific path segments, query +// parameters, or fragments — carrying any of those turns a shareable content +// identifier into an identity-leak vector (e.g. a per-user tracking query +// param would let every buyer who resolves the ref correlate the same user +// across requests). +// +// This enforces the two components that are structurally identifiable as +// user/session-specific carriers — query string and fragment — plus the +// userinfo and SSRF checks from ValidateFetchableURL, since a type=url +// ArtifactRef is by definition "a public handle the buyer can resolve +// independently" (see the ArtifactRefType doc comment) and so is fetched the +// same way Artifact/asset URLs are. "No user-specific path segments" is a +// content judgment this function cannot make structurally — a path can't be +// told apart from a legitimate article slug by shape alone — so that half of +// the spec rule is a publisher obligation this SDK cannot enforce mechanically. +func validateArtifactRefURL(raw string) error { + if len(raw) > MaxContentURLLength { + return fmt.Errorf("exceeds maximum length of %d", MaxContentURLLength) + } + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("does not parse: %w", err) + } + if u.RawQuery != "" { + return errors.New("must not contain a query string (type=url forbids query parameters)") + } + if u.Fragment != "" || u.EscapedFragment() != "" { + return errors.New("must not contain a fragment (type=url forbids fragments)") + } + if err := ValidateFetchableURL(raw); err != nil { + return err + } + return nil +} diff --git a/tmproto/urlsecurity_test.go b/tmproto/urlsecurity_test.go new file mode 100644 index 00000000..f2857221 --- /dev/null +++ b/tmproto/urlsecurity_test.go @@ -0,0 +1,332 @@ +package tmproto + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateFetchableURL_Valid(t *testing.T) { + valid := []string{ + "https://example.com/article/42", + "http://example.com/article/42", // http allowed — matches the AdCP webhook rules (scheme allowlist is http+https) + "https://cdn.example.com/pasta.jpg?w=800", + "https://example.com:8443/path", + "https://93.184.216.34/path", // public IPv4 literal + "https://[2606:2800:220:1:248:1893:25c8:1946]/", // public IPv6 literal + "https://100.63.255.255/", // just below CGNAT range + "https://100.128.0.0/", // just above CGNAT range + "https://198.17.255.255/", // just below benchmark range + "https://198.20.0.0/", // just above benchmark range + } + for _, raw := range valid { + t.Run(raw, func(t *testing.T) { + assert.NoError(t, ValidateFetchableURL(raw), "expected %q to be accepted", raw) + }) + } +} + +func TestValidateFetchableURL_RejectsBadSchemes(t *testing.T) { + cases := []string{ + "file:///etc/passwd", + "ftp://example.com/file", + "javascript:alert(1)", + "data:text/plain;base64,aGVsbG8=", + "gopher://example.com/", + "", + } + for _, raw := range cases { + t.Run(raw, func(t *testing.T) { + err := ValidateFetchableURL(raw) + require.Error(t, err, "expected %q to be rejected", raw) + }) + } +} + +func TestValidateFetchableURL_RejectsCredentials(t *testing.T) { + cases := []string{ + "https://user:pass@example.com/", + "https://token@example.com/", + } + for _, raw := range cases { + t.Run(raw, func(t *testing.T) { + err := ValidateFetchableURL(raw) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrUnsafeURL)) + assert.Contains(t, err.Error(), "userinfo") + }) + } +} + +// TestValidateFetchableURL_RejectsPrivateAndReservedIPv4 exercises the IPv4 +// range list, with CGNAT (100.64.0.0/10) called out explicitly: the sibling +// TypeScript SSRF validator (adcontextprotocol/adcp server/src/utils/url-security.ts) +// was found missing this exact range in adcontextprotocol/adcp#7091. This +// suite pins it here so the same class of gap can't reopen in this port. +func TestValidateFetchableURL_RejectsPrivateAndReservedIPv4(t *testing.T) { + cases := map[string]string{ + "loopback": "https://127.0.0.1/", + "loopback high": "https://127.255.255.255/", + "this-network": "https://0.0.0.0/", + "rfc1918 10/8": "https://10.1.2.3/", + "rfc1918 172.16/12 low": "https://172.16.0.1/", + "rfc1918 172.16/12 high": "https://172.31.255.255/", + "rfc1918 192.168/16": "https://192.168.1.1/", + "link-local": "https://169.254.1.1/", + "cloud metadata": "https://169.254.169.254/latest/meta-data/", + "cgnat low": "https://100.64.0.0/", + "cgnat mid": "https://100.100.100.100/", + "cgnat high": "https://100.127.255.255/", + "benchmark": "https://198.18.0.1/", + "unspecified": "https://0.0.0.0/", + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + err := ValidateFetchableURL(raw) + require.Error(t, err, "expected %q to be rejected", raw) + assert.True(t, errors.Is(err, ErrUnsafeURL), "expected ErrUnsafeURL for %q, got %v", raw, err) + }) + } +} + +func TestValidateFetchableURL_RejectsPrivateAndReservedIPv6(t *testing.T) { + cases := map[string]string{ + "loopback": "https://[::1]/", + "unspecified": "https://[::]/", + "link-local": "https://[fe80::1]/", + "unique-local fc00": "https://[fc00::1]/", + "unique-local fd": "https://[fd12:3456:789a::1]/", + "deprecated site-local": "https://[fec0::1]/", + "6to4 embeds private": "https://[2002:0a00:0001::]/", // embeds 10.0.0.1 + "nat64 embeds private": "https://[64:ff9b::a00:1]/", // embeds 10.0.0.1 + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + err := ValidateFetchableURL(raw) + require.Error(t, err, "expected %q to be rejected", raw) + assert.True(t, errors.Is(err, ErrUnsafeURL), "expected ErrUnsafeURL for %q, got %v", raw, err) + }) + } +} + +func TestValidateFetchableURL_AllowsPublicTunneledIPv6(t *testing.T) { + // 6to4/NAT64 forms that embed a PUBLIC v4 address must not be rejected — + // only the embedded-private case is disallowed. + cases := map[string]string{ + "6to4 embeds public": "https://[2002:0808:0808::]/", // embeds 8.8.8.8 + "nat64 embeds public": "https://[64:ff9b::808:808]/", // embeds 8.8.8.8 + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + assert.NoError(t, ValidateFetchableURL(raw), "expected %q to be accepted", raw) + }) + } +} + +func TestValidateFetchableURL_RejectsInternalHostnames(t *testing.T) { + cases := []string{ + "https://localhost/", + "https://localhost:8080/", + "https://foo.localhost/", + "https://metadata.google.internal/", + "https://service.internal/", + "https://printer.local/", + "https://example..com/", // empty label + } + for _, raw := range cases { + t.Run(raw, func(t *testing.T) { + err := ValidateFetchableURL(raw) + require.Error(t, err, "expected %q to be rejected", raw) + }) + } +} + +func TestValidateFetchableURL_RejectsOversizedURL(t *testing.T) { + raw := "https://example.com/" + strings.Repeat("a", MaxContentURLLength) + err := ValidateFetchableURL(raw) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum length") +} + +func TestValidateFetchableURL_RejectsMalformed(t *testing.T) { + err := ValidateFetchableURL("ht!tp://[::1") + require.Error(t, err) +} + +func TestValidateFetchableURL_ErrorsAreGeneric(t *testing.T) { + // Error text must not echo the raw URL back — consistent with the rest + // of this package's Validate() error messages (see validateSafeID, + // validateSellerAgentURL): describe the rule violated, not the value. + sentinelHost := "169.254.169.254" + raw := "https://" + sentinelHost + "/latest/meta-data/iam/security-credentials/" + err := ValidateFetchableURL(raw) + require.Error(t, err) + assert.NotContains(t, err.Error(), sentinelHost) + assert.NotContains(t, err.Error(), "security-credentials") +} + +func TestArtifactRef_Validate_URLType(t *testing.T) { + cases := []struct { + name string + value string + wantErr string + }{ + {"valid canonical url", "https://example.com/article/42", ""}, + {"query rejected", "https://example.com/article/42?user=alice", "query string"}, + {"fragment rejected", "https://example.com/article/42#section-2", "fragment"}, + {"userinfo rejected", "https://user:pass@example.com/article/42", "userinfo"}, + {"private ip rejected", "https://169.254.169.254/article/42", "disallowed"}, + {"non-http scheme rejected", "file:///etc/passwd", "scheme"}, + {"localhost rejected", "https://localhost/article/42", "localhost"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ref := ArtifactRef{Type: ArtifactRefTypeURL, Value: tc.value} + err := ref.Validate() + if tc.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} + +// TestArtifactRef_Validate_NonURLTypesUnaffected pins that the new URL +// checks only apply when Type == ArtifactRefTypeURL — every other ref type +// (including url_hash, whose value is a base64 digest, not a URL) must be +// unaffected. +func TestArtifactRef_Validate_NonURLTypesUnaffected(t *testing.T) { + cases := []ArtifactRef{ + {Type: ArtifactRefTypeURLHash, Value: "bXlfaGFzaA=="}, + {Type: ArtifactRefTypeEIDR, Value: "10.5240/7791-8534-2C23-9030-8107-4"}, + {Type: ArtifactRefTypeCustom, Value: "not a url at all: has? a #fragment-lookalike"}, + } + for _, ref := range cases { + t.Run(string(ref.Type), func(t *testing.T) { + assert.NoError(t, ref.Validate()) + }) + } +} + +func TestArtifact_Validate_URLField(t *testing.T) { + base := func() *Artifact { + return &Artifact{ + PropertyRID: "p", + ArtifactID: "a", + Assets: Assets{&TextAsset{Content: "hi"}}, + } + } + + t.Run("empty URL is fine (optional field)", func(t *testing.T) { + art := base() + assert.NoError(t, art.Validate()) + }) + + t.Run("valid URL passes", func(t *testing.T) { + art := base() + art.URL = "https://example.com/article/42" + assert.NoError(t, art.Validate()) + }) + + t.Run("SSRF-unsafe URL rejected", func(t *testing.T) { + art := base() + art.URL = "http://169.254.169.254/latest/meta-data/" + err := art.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "artifact.url") + }) + + t.Run("file scheme rejected", func(t *testing.T) { + art := base() + art.URL = "file:///etc/passwd" + require.Error(t, art.Validate()) + }) +} + +func TestImageAsset_Validate_SSRF(t *testing.T) { + cases := []struct { + name string + url string + wantErr string + }{ + {"valid", "https://cdn.example.com/pic.jpg", ""}, + {"loopback", "http://127.0.0.1/pic.jpg", "disallowed"}, + {"cgnat", "http://100.64.1.1/pic.jpg", "disallowed"}, + {"file scheme", "file:///etc/passwd", "scheme"}, + {"credentials", "https://user:pass@cdn.example.com/pic.jpg", "userinfo"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + a := &ImageAsset{URL: tc.url} + err := a.Validate() + if tc.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), "image_asset.url") + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} + +func TestVideoAsset_Validate_SSRF(t *testing.T) { + t.Run("valid url and thumbnail", func(t *testing.T) { + a := &VideoAsset{URL: "https://cdn.example.com/v.mp4", ThumbnailURL: "https://cdn.example.com/thumb.jpg"} + assert.NoError(t, a.Validate()) + }) + t.Run("bad primary url", func(t *testing.T) { + a := &VideoAsset{URL: "http://169.254.169.254/v.mp4"} + err := a.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "video_asset.url") + }) + t.Run("bad thumbnail url", func(t *testing.T) { + a := &VideoAsset{URL: "https://cdn.example.com/v.mp4", ThumbnailURL: "http://10.0.0.1/thumb.jpg"} + err := a.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "video_asset.thumbnail_url") + }) + t.Run("empty thumbnail is fine (optional field)", func(t *testing.T) { + a := &VideoAsset{URL: "https://cdn.example.com/v.mp4"} + assert.NoError(t, a.Validate()) + }) +} + +func TestAudioAsset_Validate_SSRF(t *testing.T) { + t.Run("valid", func(t *testing.T) { + a := &AudioAsset{URL: "https://cdn.example.com/a.mp3"} + assert.NoError(t, a.Validate()) + }) + t.Run("private ip rejected", func(t *testing.T) { + a := &AudioAsset{URL: "http://192.168.1.1/a.mp3"} + err := a.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "audio_asset.url") + }) +} + +// TestArtifact_Validate_RecursesIntoAssetURLs ensures a malicious asset URL +// buried inside Artifact.Assets is caught by the top-level Validate() call a +// handler would actually make (ValidateContextRequest -> Artifact.Validate() +// -> per-asset Validate()), not just by calling the asset's Validate() +// directly. +func TestArtifact_Validate_RecursesIntoAssetURLs(t *testing.T) { + art := &Artifact{ + PropertyRID: "p", + ArtifactID: "a", + Assets: Assets{ + &TextAsset{Content: "hi"}, + &ImageAsset{URL: "http://169.254.169.254/steal-creds.jpg"}, + }, + } + err := art.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "artifact.assets[1]") + assert.Contains(t, err.Error(), "image_asset.url") +} diff --git a/tmproto/validate_ladder.go b/tmproto/validate_ladder.go index a1d0fdc2..e1b280f1 100644 --- a/tmproto/validate_ladder.go +++ b/tmproto/validate_ladder.go @@ -89,7 +89,10 @@ func (c *ContextSignals) Validate() error { return nil } -// Validate checks field-level constraints on an ArtifactRef. +// Validate checks field-level constraints on an ArtifactRef. When +// Type == ArtifactRefTypeURL, Value is additionally checked against the +// spec's type=url contract (no query, no fragment) and against +// ValidateFetchableURL's SSRF-safety rules — see validateArtifactRefURL. func (r *ArtifactRef) Validate() error { if r == nil { return nil @@ -100,6 +103,11 @@ func (r *ArtifactRef) Validate() error { if r.Value == "" { return fmt.Errorf("artifact_ref.value: required") } + if r.Type == ArtifactRefTypeURL { + if err := validateArtifactRefURL(r.Value); err != nil { + return fmt.Errorf("artifact_ref.value: %w", err) + } + } return nil } @@ -149,7 +157,9 @@ func (a *TextAsset) Validate() error { return nil } -// Validate checks ImageAsset field-level constraints. +// Validate checks ImageAsset field-level constraints, including that URL +// passes ValidateFetchableURL — see the MUST-validate-before-fetch contract +// on the Artifact doc comment. func (a *ImageAsset) Validate() error { if a == nil { return nil @@ -157,10 +167,15 @@ func (a *ImageAsset) Validate() error { if a.URL == "" { return fmt.Errorf("image_asset.url: required") } + if err := ValidateFetchableURL(a.URL); err != nil { + return fmt.Errorf("image_asset.url: %w", err) + } return a.Access.Validate() } -// Validate checks VideoAsset field-level constraints. +// Validate checks VideoAsset field-level constraints, including that URL and +// (when present) ThumbnailURL pass ValidateFetchableURL — see the +// MUST-validate-before-fetch contract on the Artifact doc comment. func (a *VideoAsset) Validate() error { if a == nil { return nil @@ -168,13 +183,23 @@ func (a *VideoAsset) Validate() error { if a.URL == "" { return fmt.Errorf("video_asset.url: required") } + if err := ValidateFetchableURL(a.URL); err != nil { + return fmt.Errorf("video_asset.url: %w", err) + } + if a.ThumbnailURL != "" { + if err := ValidateFetchableURL(a.ThumbnailURL); err != nil { + return fmt.Errorf("video_asset.thumbnail_url: %w", err) + } + } if len(a.Transcript) > MaxTranscriptLength { return fmt.Errorf("video_asset.transcript: %d chars exceeds max %d", len(a.Transcript), MaxTranscriptLength) } return a.Access.Validate() } -// Validate checks AudioAsset field-level constraints. +// Validate checks AudioAsset field-level constraints, including that URL +// passes ValidateFetchableURL — see the MUST-validate-before-fetch contract +// on the Artifact doc comment. func (a *AudioAsset) Validate() error { if a == nil { return nil @@ -182,6 +207,9 @@ func (a *AudioAsset) Validate() error { if a.URL == "" { return fmt.Errorf("audio_asset.url: required") } + if err := ValidateFetchableURL(a.URL); err != nil { + return fmt.Errorf("audio_asset.url: %w", err) + } if len(a.Transcript) > MaxTranscriptLength { return fmt.Errorf("audio_asset.transcript: %d chars exceeds max %d", len(a.Transcript), MaxTranscriptLength) } @@ -189,7 +217,9 @@ func (a *AudioAsset) Validate() error { } // Validate checks Artifact-level constraints and recurses into each asset. -// Unknown assets (from forward-compat passthrough) are skipped. +// Unknown assets (from forward-compat passthrough) are skipped. When URL is +// present it is checked against ValidateFetchableURL — see the +// MUST-validate-before-fetch contract on the Artifact doc comment. func (a *Artifact) Validate() error { if a == nil { return nil @@ -200,6 +230,11 @@ func (a *Artifact) Validate() error { if a.ArtifactID == "" { return fmt.Errorf("artifact.artifact_id: required") } + if a.URL != "" { + if err := ValidateFetchableURL(a.URL); err != nil { + return fmt.Errorf("artifact.url: %w", err) + } + } if len(a.Assets) > MaxAssets { return fmt.Errorf("artifact.assets: %d items exceeds max %d", len(a.Assets), MaxAssets) }