Address Fable review: ctx lifetime, race, fail-closed, caps, wire masking

Correctness batch first (Fable found the module was silently broken):

- Blocker #1 (fan-out ctx dies on hook return). The framework cancels
  each hook's own ctx the moment the hook returns
  (hooks/hookexecution/execution.go), so a fan-out rooted in an
  entrypoint hook's ctx was Done before the goroutine spawned.
  Consequence: every provider call started already-cancelled, zero
  segments landed, and the failure was silent (errors stayed inside
  providerResult.Errs and analytics reported Success). Removed the
  entrypoint hook entirely; the async holder now allocates inside
  HandleProcessedAuctionHook with a Background-rooted ctx, cancelled
  via defer async.cancel() in the response hook. That also fixes
  finding prebid#6 (response hook stalling for its full group timeout when
  processed-auction never ran) — no holder ever means the response
  hook short-circuits cleanly.
- Blocker prebid#2 (data race on live BidRequest). The fan-out goroutine
  read Site/Imp/Device.Geo/User.EIDs and unmarshalled User.Ext while
  the auction continued to mutate the wrapper (RebuildRequest,
  privacy scrubbing, other modules). deriveInputs now runs
  synchronously on the caller's stack; the goroutine only sees the
  tmpInputs value snapshot and never touches the BidRequest.
- Finding prebid#10 (no hooks-level test). New hooks_test.go exercises
  HandleProcessedAuctionHook → HandleAuctionResponseHook through real
  hookstage plumbing, including simulating the framework's per-hook
  ctx cancellation. This is the test that would have caught #1 in
  the first place; it fails against the pre-fix code.

Then the small independent correctness fixes:

- prebid#4 registry bare-record parse: fall back to decoding the raw
  payload as PropertyRecord when the {"property": {...}} envelope
  isn't found. The old code negative-cached bare records as
  "not found" for 300 s, silently.
- prebid#7 per-provider request IDs: build ctxReq and idReq inside
  callProvider so two colluding providers don't get the same
  request_id pair for the same auction.
- prebid#8 hostile domain: cap site.domain / app.bundle at 253 chars and
  restrict to `[a-z0-9._-]`. Rejects invalid keys before touching
  the LRU or the registry.

Then the design/product calls user directed on:

- prebid#3 fail-closed on identity error: providerResult tracks
  IdentityAttempted (URL configured AND tokens present). When
  IdentityAttempted is true but the call returned no response,
  mergeSegments drops all offers for that provider. A hostile or
  flaky identity endpoint can no longer convert identity-gated
  packages into unconditionally-served packages.
- prebid#5 cap segments + batch per-bid targeting write. New config knobs
  MaxSegments (default 128) and MaxSegmentValueLen (default 256)
  bound both the total segment count and each segment's length,
  regardless of what a provider returns. Response-hook per-bid
  targeting now builds one map from segments and writes it via a
  single sjson.SetBytes per bid at ext.prebid.targeting instead of
  O(bids × segments) rewrites. Provider names are also validated
  in Config.validated() to prevent them from colliding with Prebid's
  reserved targeting prefixes (hb_*).
- prebid#9 wire the masking config. coarseGeo now takes cfg and honors
  PreserveMetro / PreserveZip / PreserveCity / LatLongPrecision
  (with math.Trunc for lat/lon precision); extractIdentities honors
  PreserveMobileIds (drops maid-typed tokens when masking enabled
  and false) and PreserveEids (allowlist over the default set).
  Deleted the now-empty masking.go — every knob is now wired
  through adapter.go at input-derivation time.

Nits also addressed:

- Analytics no longer reports Success when every provider errored;
  errCount from routerResult drives Status/ResultStatus explicitly.
- site.page's query and fragment components are stripped before
  emission as an artifact ref — gclid / click IDs / occasional
  email leak into the identity-free context path.
- Ordering test reframed: with DecorrelationMaxDelayMs > 0 the
  second-to-spawn call is deterministically delayed, so HTTP arrival
  order actually reflects the shuffle instead of scheduler noise.
- Panic-recovery test replaced with one that injects a panicking
  RoundTripper — actually exercises the recover paths in
  callProvider's inner goroutines.
- Nil-guard on payload.BidResponse in the response-hook mutation.
- Removed unused asyncRequest.err field.

Author: ohalushchak-exadel

modules/adcontextprotocol/tmp/README.md

@@ -51,13 +51,24 @@ hooks:
         decorrelation_max_delay_ms: 0
         targeting_key: adcp
         add_to_targeting: false
+        # Caps on the segment set surfaced onto the response ext. Guards
+        # against a misbehaving or hostile provider bloating the bid
+        # response.
+        max_segments: 128
+        max_segment_value_len: 256
+        # Masking gates optional finer-grained fields into the context
+        # payload (zip / city / lat-long) and controls which EID sources
+        # / mobile IDs flow into the identity payload. Defaults are
+        # strict: only country / region / metro on the context path;
+        # a small hardcoded EID whitelist on the identity path unless
+        # `enabled: true` and `preserve_eids` narrows or widens it.
         masking:
           enabled: true
           geo:
             preserve_metro: true
             preserve_zip: false
             preserve_city: false
-            lat_long_precision: 2
+            lat_long_precision: 0
           user:
             preserve_eids:
               - liveramp.com
@@ -70,12 +81,6 @@ hooks:
     endpoints:
       /openrtb2/auction:
         stages:
-          entrypoint:
-            groups:
-              - timeout: 5
-                hook_sequence:
-                  - module_code: "adcontextprotocol.tmp"
-                    hook_impl_code: "HandleEntrypointHook"
           auction_processed:
             groups:
               - timeout: 500

modules/adcontextprotocol/tmp/adapter.go

@@ -1,6 +1,8 @@
 package tmp
 
 import (
+	"math"
+	"net/url"
 	"strings"
 
 	"github.com/adcontextprotocol/adcp-go/tmproto"
@@ -36,9 +38,13 @@ func deriveInputs(cfg *Config, req *openrtb2.BidRequest) tmpInputs {
 	if req.Site != nil {
 		out.Domain = req.Site.Domain
 		if req.Site.Page != "" {
+			// Strip the query component before emitting as an artifact
+			// ref: gclid, click IDs and sometimes emails ride the query
+			// string, and the context path is supposed to be
+			// identity-free. Fragment is dropped too — same reasoning.
 			out.ArtifactRefs = append(out.ArtifactRefs, tmproto.ArtifactRef{
 				Type:  tmproto.ArtifactRefTypeURL,
-				Value: req.Site.Page,
+				Value: stripURLQueryAndFragment(req.Site.Page),
 			})
 		}
 	}
@@ -70,48 +76,101 @@ func deriveInputs(cfg *Config, req *openrtb2.BidRequest) tmpInputs {
 	}
 
 	if req.Device != nil && req.Device.Geo != nil {
-		out.Geo = coarseGeo(req.Device.Geo)
+		out.Geo = coarseGeo(cfg, req.Device.Geo)
 		out.Country = req.Device.Geo.Country
 	} else if req.User != nil && req.User.Geo != nil {
-		out.Geo = coarseGeo(req.User.Geo)
+		out.Geo = coarseGeo(cfg, req.User.Geo)
 		out.Country = req.User.Geo.Country
 	}
 
 	if req.User != nil {
-		out.Identities = extractIdentities(req.User)
+		out.Identities = extractIdentities(cfg, req.User)
 	}
 	out.Consent = extractConsent(req)
 
 	return out
 }
 
-// coarseGeo drops fields the TMP context schema forbids (postcode, lat/long,
-// accuracy) even after masking. Country / region / metro are the wire allowlist.
-func coarseGeo(geo *openrtb2.Geo) map[string]any {
+// stripURLQueryAndFragment returns the URL with the query and fragment
+// components removed, keeping scheme + host + path. If the input is not
+// parseable as a URL, it is returned unchanged (the wire schema
+// accepts opaque strings on artifact refs).
+func stripURLQueryAndFragment(raw string) string {
+	u, err := url.Parse(raw)
+	if err != nil {
+		return raw
+	}
+	u.RawQuery = ""
+	u.Fragment = ""
+	return u.String()
+}
+
+// coarseGeo emits the geo fields the TMP context payload carries. When
+// masking is enabled, per-field flags gate the finer-grained categories
+// (city / zip / lat-lon); with masking disabled the default is the same
+// strict-mode fields (country / region / metro) that the TMP wire spec
+// treats as coarse enough to not identify a user. Operators who
+// explicitly opt into zip / city / lat-lon through the masking config
+// take responsibility for that being acceptable at their own provider.
+func coarseGeo(cfg *Config, geo *openrtb2.Geo) map[string]any {
 	if geo == nil {
 		return nil
 	}
+	m := cfg.Masking
+	preserveMetro := true
+	preserveZip := false
+	preserveCity := false
+	latLongPrecision := 0
+	if m.Enabled {
+		preserveMetro = m.Geo.PreserveMetro
+		preserveZip = m.Geo.PreserveZip
+		preserveCity = m.Geo.PreserveCity
+		latLongPrecision = m.Geo.LatLongPrecision
+	}
+
 	out := map[string]any{}
 	if geo.Country != "" {
 		out["country"] = geo.Country
 	}
 	if geo.Region != "" {
 		out["region"] = geo.Region
 	}
-	if geo.Metro != "" {
+	if preserveMetro && geo.Metro != "" {
 		out["metro"] = geo.Metro
 	}
+	if preserveZip && geo.ZIP != "" {
+		out["zip"] = geo.ZIP
+	}
+	if preserveCity && geo.City != "" {
+		out["city"] = geo.City
+	}
+	if latLongPrecision > 0 && geo.Lat != nil && geo.Lon != nil {
+		out["lat"] = truncateCoord(*geo.Lat, latLongPrecision)
+		out["lon"] = truncateCoord(*geo.Lon, latLongPrecision)
+	}
 	if len(out) == 0 {
 		return nil
 	}
 	return out
 }
 
+// truncateCoord truncates a coordinate to n decimal places using
+// math.Trunc so negative coordinates truncate toward zero (matching
+// what most operators expect for a "reduce precision" knob).
+func truncateCoord(v float64, precision int) float64 {
+	mult := math.Pow(10, float64(precision))
+	return math.Trunc(v*mult) / mult
+}
+
 // extractIdentities maps openrtb2 user.eids → tmproto.IdentityToken, honoring
 // the TMP cap of three tokens. Priority order: rampid, uid2, id5, then whatever
 // remains. Publishers that need a different priority should tell us — right now
 // this is the most common set.
-func extractIdentities(user *openrtb2.User) []tmproto.IdentityToken {
+//
+// When Masking is enabled and PreserveMobileIds is false, maid-typed
+// tokens (mobile advertising IDs) are dropped from the identity set so
+// they never reach a TMP provider.
+func extractIdentities(cfg *Config, user *openrtb2.User) []tmproto.IdentityToken {
 	if user == nil {
 		return nil
 	}
@@ -121,6 +180,21 @@ func extractIdentities(user *openrtb2.User) []tmproto.IdentityToken {
 		"id5-sync.com": 2,
 		"euid.eu":      3,
 		"adserver.org": 4,
+		"adid.google":  5,
+		"idfa.apple":   5,
+	}
+	dropMaid := cfg.Masking.Enabled && !cfg.Masking.Device.PreserveMobileIds
+	// When PreserveEids is set (masking enabled + operator populated it,
+	// or the default filled in by validated()) it is authoritative: only
+	// EID sources on the allowlist survive. Publishers who want the
+	// default hardcoded whitelist just leave masking off; publishers who
+	// want a narrower allowlist enable masking + populate the field.
+	var eidAllowlist map[string]bool
+	if cfg.Masking.Enabled && len(cfg.Masking.User.PreserveEids) > 0 {
+		eidAllowlist = make(map[string]bool, len(cfg.Masking.User.PreserveEids))
+		for _, s := range cfg.Masking.User.PreserveEids {
+			eidAllowlist[strings.ToLower(s)] = true
+		}
 	}
 	type scored struct {
 		tok   tmproto.IdentityToken
@@ -132,10 +206,16 @@ func extractIdentities(user *openrtb2.User) []tmproto.IdentityToken {
 		if len(eid.UIDs) == 0 || eid.UIDs[0].ID == "" {
 			continue
 		}
+		if eidAllowlist != nil && !eidAllowlist[strings.ToLower(eid.Source)] {
+			continue
+		}
 		uidType := mapEIDToUIDType(eid.Source)
 		if uidType == "" {
 			continue
 		}
+		if dropMaid && uidType == tmproto.UIDTypeMAID {
+			continue
+		}
 		// Every source that survives mapEIDToUIDType has a priority entry;
 		// the map fallback is unreachable.
 		candidates = append(candidates, scored{
@@ -180,6 +260,8 @@ func mapEIDToUIDType(source string) tmproto.UIDType {
 		return tmproto.UIDTypeEUID
 	case "adserver.org":
 		return tmproto.UIDTypePairID
+	case "adid.google", "idfa.apple":
+		return tmproto.UIDTypeMAID
 	}
 	return ""
 }

modules/adcontextprotocol/tmp/config.go

@@ -4,6 +4,7 @@ import (
 	"crypto/ed25519"
 	"errors"
 	"fmt"
+	"regexp"
 
 	"github.com/adcontextprotocol/adcp-go/tmproto"
 )
@@ -58,6 +59,17 @@ type Config struct {
 	// AddToTargeting mirrors the response signals into prebid.targeting so
 	// downstream ad servers (e.g. GAM) can consume them.
 	AddToTargeting bool `json:"add_to_targeting"`
+
+	// MaxSegments caps the total number of segments emitted onto the
+	// response ext, regardless of how many providers respond or how many
+	// offers/signals they include. Default 128. A hostile-or-buggy
+	// provider cannot bloat the bid response past this bound.
+	MaxSegments int `json:"max_segments"`
+
+	// MaxSegmentValueLen bounds each emitted segment string (name +
+	// separator + value). Default 256. Excess is truncated. A cap of 0
+	// disables truncation.
+	MaxSegmentValueLen int `json:"max_segment_value_len"`
 }
 
 // SigningConfig carries the private-key material used to sign outbound TMP
@@ -127,6 +139,14 @@ type DeviceMaskingConfig struct {
 	PreserveMobileIds bool `json:"preserve_mobile_ids"`
 }
 
+// providerNameRE constrains provider names so an operator cannot
+// accidentally name a provider "hb" (or similar) and have its emitted
+// segment keys collide with Prebid's own reserved targeting keys (e.g.
+// hb_pb, hb_adid). The prefix in emitted segments is provider name +
+// underscore; restricting to a lower-case identifier keeps the prefix
+// unambiguous.
+var providerNameRE = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]{0,31}$`)
+
 // validated returns a Config with defaults filled in, along with the parsed
 // Ed25519 private key. Invalid configuration is rejected here rather than at
 // call sites.
@@ -147,11 +167,19 @@ func (c *Config) validated() (ed25519.PrivateKey, error) {
 	if len(c.Providers) == 0 {
 		return nil, errors.New("at least one provider is required")
 	}
+	seenNames := make(map[string]bool, len(c.Providers))
 	for i := range c.Providers {
 		p := &c.Providers[i]
 		if p.Name == "" {
 			return nil, fmt.Errorf("providers[%d].name is required", i)
 		}
+		if !providerNameRE.MatchString(p.Name) {
+			return nil, fmt.Errorf("providers[%d].name %q must match %s (lowercase letters, digits, underscore, hyphen; up to 32 chars)", i, p.Name, providerNameRE)
+		}
+		if seenNames[p.Name] {
+			return nil, fmt.Errorf("providers[%d].name %q is duplicated", i, p.Name)
+		}
+		seenNames[p.Name] = true
 		if p.IdentityURL == "" && p.ContextURL == "" {
 			return nil, fmt.Errorf("providers[%d] (%s): at least one of identity_url or context_url is required", i, p.Name)
 		}
@@ -181,6 +209,15 @@ func (c *Config) validated() (ed25519.PrivateKey, error) {
 	if c.TargetingKey == "" {
 		c.TargetingKey = "adcp"
 	}
+	if c.MaxSegments <= 0 {
+		c.MaxSegments = 128
+	}
+	if c.MaxSegmentValueLen < 0 {
+		return nil, errors.New("max_segment_value_len cannot be negative")
+	}
+	if c.MaxSegmentValueLen == 0 {
+		c.MaxSegmentValueLen = 256
+	}
 	if c.Masking.Enabled {
 		if c.Masking.Geo.LatLongPrecision > 4 {
 			return nil, errors.New("masking.geo.lat_long_precision cannot exceed 4")