optable-targeting: Hid plus id5 Mobile In-app resolver params - #3
Conversation
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Review
Blocking issues below. Two of them (un-encoded query params, cache key) are also security-relevant, so flagging as request-changes rather than nits.
Most findings are left as inline comments. Two anchor on lines outside the diff, so they are here:
Blocking: new user-identifying params are excluded from the cache key
CachedAPIClient.java:55-63 keys the cache on tenant:origin:ips.getFirst():query.getIds(). This PR adds hid (including i6, which is explicitly filtered out of the ids string in buildIdsString), id5_signature, bundle and ver to the request, and none of them enter the key.
In exactly the post-ATT mobile in-app case this PR targets there is no IFA, so ipv6 and the signature are the only discriminators. Two devices behind the same first IP with the same (possibly empty) id= set will collide: the second gets the first user's cached TargetingResult, including ID5 EIDs and the refs signature. That signature is then round-tripped into the second user's client via ext.prebid.passthrough.optable.id5_signature and replayed on their subsequent auctions, so the contamination persists past the cache TTL.
CacheTest was only touched for constructor arity; nothing covers the key contents. Please add the new params to createCachingKey and a test that asserts two requests differing only in hid / id5_signature / bundle / ver do not share a cache entry.
Should fix: the response hook now blocks on the network future under a 10 ms budget
The sample plan gives the auction-response group timeout: 10 (sample/configs/sample-app-settings-optable.yaml:73). The hook now does moduleContext.getOptableTargetingCall().compose(...), so if the targeting call is still pending at response time (for example the bidder-request hook already recovered on its own timeout) the response hook waits on it and the group times out. Before this PR the response hook enriched synchronously from moduleContext.getTargeting() and could not be delayed by the network call.
Suggest either awaiting the future only when the id5 signature is actually needed, or short-circuiting when the future is not yet complete and falling back to the already-resolved targeting.
Minor
Id5Resolver.java:16-20:STR_OPTABLE_CO/STR_ID_5_SYNC_COMis not the naming convention used elsewhere in PBS;OPTABLE_INSERTER/ID5_SOURCEwould read better.QueryBuilder.java:61usesString.formatwhere the surrounding code uses.formatted().
| private Future<InvocationResult<AuctionResponsePayload>> enrichedById5SignaturePayload( | ||
| ModuleContext moduleContext) { | ||
|
|
||
| return moduleContext.getOptableTargetingCall() |
There was a problem hiding this comment.
Blocking: NPE when optableTargetingCall was never set.
moduleContext.getOptableTargetingCall() is dereferenced here (and at line 74) with no null guard, but the future is set in exactly one place, OptableRawAuctionRequestHook:98. Paths where it is null at response time:
- Deprecated plan (processed-auction-request hook instead of the raw hook): nothing ever stores the future, so even a fully successful targeting call NPEs here. That silently drops adserver-targeting keyword enrichment for every request, which is a regression - before this PR the hook enriched synchronously from
moduleContext.getTargeting(). - Raw-hook plan, early returns:
OptableRawAuctionRequestHookreturns without setting the future whenbiddersToEnrichis empty (lines 83-85) or the properties are invalid (lines 66-74), andshouldSkipEnrichmentstaysfalse. The sample config shipspubmatic: 0, so a pubmatic-only request hits this on every auction.OptableBidderRequestHook:41-44guards this case; this hook does not. - A fresh
ModuleContext(ModuleContext.ofreturnsnew ModuleContext()when the invocation context has none).
GroupExecutor catches the Throwable, so nothing crashes - the module just fails and all response enrichment is dropped, which is why this does not show up as a test failure. The test churn is the tell: pre-existing tests had to be handed futures (OptableTargetingAuctionResponseHookTest.java:66-68) to keep passing.
Please null-guard and fall back to the previous synchronous path, and add a test that runs the response hook with a ModuleContext that has no optableTargetingCall.
| final String hidParameters = Arrays.stream(hidPrefixesString.split(",")) | ||
| .map(prefixToIdValue::get) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> String.format("hid=%s:%s", it.getName(), it.getValue())) |
There was a problem hiding this comment.
Blocking: hid values are appended un-encoded. Compare with buildIdsString below, which percent-encodes id= values, and with id5_signature at line 140 which is also encoded.
These values are client-controlled (user.ext.optable.*, eids, device.ipv6). Two consequences:
- Corruption. An E.164 phone
p:+15551234reaches the edge asp: 15551234because gin decodes+as a space.idtype.Parsethen fails and the resolver silentlycontinues past the hint, so the hid is dropped with no error anywhere. - Query-param injection. A value containing
&injects arbitrary params into the targeting call, and the v2 targeting handler exposes live control params (skip_resolvers,skip_matchers,environment,id5_signature). So a crafted id lets a caller disable or redirect resolver behaviour server-side.
URLEncoder.encode the name:value pair the same way buildIdsString does. Note the new tests assert the un-encoded output (shouldBuildHidWhenHidPrefixesMatchIds), so they need updating too - right now they lock the bug in.
| .ifPresent(app -> { | ||
| final String bundle = app.getBundle(); | ||
| if (StringUtils.isNotEmpty(bundle)) { | ||
| sb.append("&bundle=").append(bundle); |
There was a problem hiding this comment.
Blocking (same class of bug as hid): bundle and ver are appended un-encoded.
app.bundle and app.ver come straight from the bid request. A bundle such as com.example&skip_resolvers=id5 injects a control param into the targeting call. Percent-encode both.
| return StringUtils.EMPTY; | ||
| } | ||
|
|
||
| final String hidParameters = Arrays.stream(hidPrefixesString.split(",")) |
There was a problem hiding this comment.
Should fix: hid-prefixes tokens are not trimmed.
"e, p, i6".split(",") yields " p" and " i6", which never match an id name, so those prefixes are silently dropped and the operator gets no hid params and no error. The config is documented as "a comma-separated set", and a space after a comma is the natural way to write that.
Add .map(String::trim) and drop empty tokens. Worth logging (or failing config validation on) a prefix that matches no known id name, otherwise a typo is invisible. Tests for whitespace and empty tokens are missing.
| } | ||
| final Map<String, Id> prefixToIdValue = ids.stream() | ||
| .collect(Collectors.toMap(Id::getName, it -> it, (a, b) -> b)); | ||
| if (MapUtils.isEmpty(prefixToIdValue)) { |
There was a problem hiding this comment.
Dead code: ids is already known non-empty at this point, so the collected map cannot be empty.
| final String ver = app.getVer(); | ||
| if (StringUtils.isNotEmpty(ver)) { | ||
| sb.append("&ver=").append(ver); | ||
| } |
There was a problem hiding this comment.
ver is only emitted when bundle is non-empty, but the server treats them as independent ResolverParams fields. An app that sets app.ver without app.bundle loses the version for no reason.
| return null; | ||
| } | ||
|
|
||
| final String ref = Optional.of(targetingResult) | ||
| .map(TargetingResult::getOrtb2) | ||
| .map(Ortb2::getUser) | ||
| .map(User::getEids) | ||
| .orElseGet(List::of) | ||
| .stream() | ||
| .filter(it -> STR_OPTABLE_CO.equals(it.getInserter()) && STR_ID_5_SYNC_COM.equals(it.getSource())) | ||
| .map(Eid::getUids) | ||
| .flatMap(Collection::stream) | ||
| .map(Uid::getExt) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> it.get(STR_OPTABLE)) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> it.get(STR_REF)) | ||
| .filter(Objects::nonNull) | ||
| .map(JsonNode::asText) | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| if (ref == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return Optional.ofNullable(targetingResult.getRefs()) | ||
| .map(refs -> refs.get(ref)) |
There was a problem hiding this comment.
Only the first matching eid's ref is considered. If refs has no entry under that particular ref, a valid signature reachable through a second matching eid is ignored and the request goes out without a signature.
shouldReturnSignatureWhenMultipleMatchingEidsExist asserts exactly this first-only behaviour, so the test documents the limitation rather than catching it. Suggest iterating the matching eids until one resolves against refs.
| return builder.build(); | ||
| } | ||
|
|
||
| private static ExtUserOptable parseExtUserOptable(JsonNode node) { |
There was a problem hiding this comment.
Three things here:
- This re-parses
user.ext.optable, whichIdsMapperhas already parsed for the same request. - It uses the static
ObjectMapperProvider.mapper()rather than the mapper injected into this class, which diverges from how the rest of the module gets its mapper. - The parse failure is swallowed silently, whereas
IdsMapperlogs it. A malformeduser.ext.optablewill drop the id5 signature with no trace.
Also, the if (extUserOptable.isPresent()) { extUserOptable.map(...).ifPresent(...) } wrapper just above is redundant - the ifPresent already covers it.
| id-prefix-order: "e,v,c" | ||
| ppid-mapping: { "pubcid.org": "c" } | ||
| hid-prefixes: "c,i6" | ||
| ppid-mapping: { "pubmatic.com": "c" } |
There was a problem hiding this comment.
Unrelated to this PR and looks like a debugging leftover: ppid-mapping changed from {"pubcid.org": "c"} to {"pubmatic.com": "c"}. pubmatic.com is a bidder, not an eid source, so this mapping cannot match anything. Please revert.
… resolving id5Signature
🔧 Type of changes
What's the context?
This update introduces hid (resolver hint) support together with the ID5 Mobile In-App resolver additions.
New features
hid-prefixes— a comma-separated set of id prefixes to emit ashid=, in the given order (e.g. "e,p,i6,a,g,c,c1"). Selection semantics: only the listed prefixes are sent as hid, unlike id-prefix-order which only orders.Bundleandverparams of Targeting API call - emit app bundle and version parameters to the Targeting API when they are present as part of the request (bidRequest.app).id5_signaturefrom client and propagate it as Targeting API query attribute (alongside email/phone/zip/vid).ext.prebid.passthrough.optable.id5_signatureon the next auction (step 3)🧪 Test plan
🏎 Quality check