WICG / turtledove

TURTLEDOVE
https://wicg.github.io/turtledove/
Other
538 stars 237 forks source link

runAdAuction Input Validation for Multi-Seller #1168

Open JacobGo opened 6 months ago

JacobGo commented 6 months ago

In multi-seller auctions, the top-level seller is responsible for assembling all sellers' component auction configs into a single runAdAuction call. These component auction configs often come directly from different sellers and may accidentally include malformed fields.

Upon receiving a malformed field anywhere inside of the auction config (including component auctions), runAdAuction currently rejects the entire auction. This is illustrated in the example below[1], which causes Chrome to throw an exception and prevents any valid, well-formed auction configs from executing which diminishes PAAPI utility.

Top-level sellers may instead choose to implement their own input validation on component auction configs and surface errors to third-parties directly over their own interfaces. This is problematic due to the top-level seller burden/risk of porting and maintaining internal browser logic to catch invalid inputs early.

One option would be for Chrome to only warn if an invalid component auction config is provided and still continue executing all valid component-level auctions and the top-level auction. This is problematic due to a lack of feedback loop for broken component sellers and reduced JS error observability, thus concealing potential errors.

Another option would be for Chrome to provide an explicit auction config validation API that a top-level seller may use to filter out each invalid component auction config prior to invoking the auction. To avoid incurring additional latency for the valid majority of auction configs, this API could return synchronously or instead be invoked lazily iff runAdAuction rejects with a TypeError.


Example of an Invalid Auction Config

navigator.runAdAuction({
    "seller": "https://seller.example",
    "decisionLogicUrl": "https://seller.example/js",
    "componentAuctions": [
        {
            "seller": "https://valid.seller.example",
            "decisionLogicUrl": "https://valid.seller.example/js",
            "interestGroupBuyers": ["https://buyer.example"],
        },
        {
            // Invalid, missing decisionLogicURL.
            "seller": "https://invalid.seller.example",
            "interestGroupBuyers": ["https://buyer.example"],
        },
    ]
});

> TypeError: Failed to execute 'runAdAuction' on 'Navigator': Missing required field ad auction config decisionLogicURL or serverResponse
MattMenke2 commented 6 months ago

You can call runAdAuction on a component with an empty set of bidders and see if it throws, or even with an already aborted abort signals. Admittedly, the latter option probably won't prevent loading all interest groups from disk, so is not entirely free - so ideally would only resort to that if the main auction throws.

JacobGo commented 6 months ago

Iteratively modifying each component seller auction configs to (temporarily) remove the interest group buyers sounds like a potentially undesirable top-level seller behavior; we generally view these configs as opaque JSON blobs passed verbatim from third-parties. Awaiting multiple async runAdAuction calls for a single auction also sounds problematic for latency.

However, this approach would be better than abandoning the auction entirely or maintaining a series of user-space input validations. We'll experiment with an integration, but it would still be useful if Chrome explored more explicit validation checks long-term, especially considering the level of complexity and room for error in this config object.

Tangentially, this problem reminds me of #759. Microsoft has some great code generation that converts IDLs into TypeScript types. I would love to see PAAPI reach that level of support and improved web platform ergonomics as well.

michaelkleber commented 6 months ago

I agree that this might be a relatively slow affair, since each individual component would be tested in its own async operation. So this is surely something to only do once the real auction has failed and you're trying to diagnose an error.

And I certainly understand why you don't want to run a real auction with modified configs. As Matt said, emptying out the interestGroupBuyers array would prevent the auction from being "real". I would suggest only doing this kind of surgery in throw-away clones of an auction config.

To make this explicit, here's the kind of debugging function that I think we're suggesting could help for now:

// Call this function with a config that made runAdAuction throw an error.
// You will get back a Promise for an object with two properties:
//     'bad' : A list of all the component configs that caused errors
//     'good' : A new auction config that does not include the bad components
// If 'good' is null, then the original auction config has problems of its own,
// that are not caused by component auctions.
async function debugConfig(config) {
    if (!config['componentAuctions'] || !config['componentAuctions'].length) {
        return {'good' : null, 'bad' : null};
    }
    goodComponentConfigs = [];
    badComponentConfigs = [];
    miniConfig = structuredClone(config);
    await Array.fromAsync(config['componentAuctions'], async (component) => {
        // Make an auction config containing only one component seller...
        miniConfig['componentAuctions'] = [ structuredClone(component) ];
        // ...and zero buyers, so that we just error-check the component config.
        miniConfig['componentAuctions'][0]['interestGroupBuyers'] = [];
        return navigator.runAdAuction(miniConfig).then(
            ()=>goodComponentConfigs.push(component),
            ()=>badComponentConfigs.push(component))
    });
    miniConfig['componentAuctions'] = goodComponentConfigs;
    return {'good' : miniConfig,
            'bad' : badComponentConfigs};
}
MattMenke2 commented 6 months ago

Worth noting that "structuredClone()" would need to be smart enough to handle input arguments that are promises (probably by passing them in unmodified). I think [{ ... , component }] would be sufficient here - no need to clone the entire thing, since we don't modify the input auction, just need to copy the component auction itself so can clear buyers.

Could even stash buyers in a temporary, start the promise, restore it, and then await the promise.

MattMenke2 commented 6 months ago

One interesting oddity: If a promise resolves to an invalid value, only the component with the bad-valued promise will fail. The rest of the auction will run and complete without issue, I believe, as long as it's not the top-level auction with the bad value.