prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.33k stars 2.08k forks source link

Price in `AUCTION_PRICE` macro is not converted anymore #8319

Closed acsbendi closed 2 years ago

acsbendi commented 2 years ago

Type of issue

Bug.

Description

https://github.com/prebid/Prebid.js/pull/8171 introduced a significant breaking change which caused a serious bug in our systems (Kobler adapter) and possibly in other bidders as well:

The previous functionality was much better as it avoided currency-conversion-related problems (we could record the exact same price that the publisher will charge us). However, the biggest problem here is that this macro, or more generally, prices in Prebid.js don't have an associated currency (e.g. there is no standard AUCTION_PRICE_CURRENCY macro to contain the currency of AUCTION_PRICE) so it's very easy to end up with bugs related to currency conversion.

Steps to reproduce

To verify it was working correctly before:

To reproduce the current erroneous behavior:

Test page

<html>

<head>
    <link rel="icon" type="image/png" href="/favicon.png">
    <script async src="https://www.googletagservices.com/tag/js/gpt.js"></script>
    <script src="file:///C:/Users/Bendi/Documents/Kobler/Prebid.js/build/dist/prebid.js"></script>
    <script>
        var div_1_sizes = [
            [300, 250],
            [320, 250],
            [300, 600]
        ];
        var PREBID_TIMEOUT = 1000;
        var FAILSAFE_TIMEOUT = 3000;

        var adUnits = [
            {
                code: '/19968336/header-bid-tag-0',
                mediaTypes: {
                    banner: {
                        sizes: div_1_sizes
                    }
                },
                bids: [{
                    bidder: 'kobler',
                    params: {
                        placementId: 'k5H7et3R0'
                    }
                }]
            }
        ];

        // ======== DO NOT EDIT BELOW THIS LINE =========== //
        var googletag = googletag || {};
        googletag.cmd = googletag.cmd || [];
        googletag.cmd.push(function() {
            googletag.pubads().disableInitialLoad();
        });

        var pbjs = pbjs || {};
        pbjs.que = pbjs.que || [];

        pbjs.que.push(function() {
            pbjs.addAdUnits(adUnits);
            pbjs.setConfig({
                "currency": {
                    "adServerCurrency": "NOK"
                }
            });
            pbjs.requestBids({
                bidsBackHandler: initAdserver,
                timeout: PREBID_TIMEOUT
            });
        });

        function initAdserver() {
            if (pbjs.initAdserverSet) return;
            pbjs.initAdserverSet = true;
            googletag.cmd.push(function() {
                pbjs.que.push(function() {
                    pbjs.setTargetingForGPTAsync();
                    googletag.pubads().refresh();
                });
            });
        }
        // in case PBJS doesn't load
        setTimeout(function() {
            initAdserver();
        }, FAILSAFE_TIMEOUT);

        googletag.cmd.push(function() {
            googletag.defineSlot('/19968336/header-bid-tag-0', div_1_sizes, 'div-1').addService(googletag.pubads());
            googletag.pubads().enableSingleRequest();
            googletag.enableServices();
        });

    </script>

</head>

<body>
<h2>Basic Prebid.js Example</h2>
<h5>Div-1</h5>
<div id='div-1'>
    <script type='text/javascript'>
        googletag.cmd.push(function() {
            googletag.display('div-1');
        });

    </script>
</div>

</body>

</html>

Expected results

The price is in the currency used by the publisher (set as adServerCurrency), just like before.

Actual results

The price is in the currency that the bidder used for bidding.

Platform details

All Prebid versions are affected after and including 6.16.0.

Other information

Related PR: https://github.com/prebid/Prebid.js/pull/8171

patmmccann commented 2 years ago

This is very interesting; the concern addressed in 8171 is that publishers should be able to adjust your bids however they want to compete in the auction without it raising or lowering what you pay them. In your previous code, a publisher could simply multiply all your bids by two and get paid double. If they cut them in half, you would pay them half. This matching is undesirable and reduced publisher flexibility.

How do you propose we fix that bug without causing this bug? It seems your win notification url should just accept a second macro?

acsbendi commented 2 years ago

@patmmccann I've been looking around the codebase, and it seems to me that there are three different prices, each for its own purpose:

If my understanding is correct, the publishers are supposed to change adserverTargeting.hb_pb instead of bid.cpm if they want to give higher priority to some bidders while not charging them a higher price. Please correct me if I'm wrong.

There are two additional related issues:

The following changes could be made to address these:

I know changes like that are very hard to make since it would affect all the adapters, but I think it would be worth it in the long term to avoid bugs related to money, which are arguably the most serious ones.

patmmccann commented 2 years ago

There's a lot to unpack here.

First is, prebid is open source software, subject to edits by publishers before deployment, and by paying publishers based on what prebid decides you open your company up to many attacks in which your payables might be greatly inflated or otherwise manipulated. For example, a malware provider could buy a ton of ads on your exchange and then not pay for any of them by changing the win price to zero.

Second, re 'bid.cpm: the price the bidder will be charged.' That is not the meaning of this field. It is meant to be the price the publisher adjusts a bid to so it is comparable to other bids in the auction, potentially by unifying the currency or by multiplying each bid by an average discrepency for that bidder. It is a mistake to pay yourself this amount, as the publisher may choose to cut it in half to account for credit risk, or to double it because they wish to prefer a bidder. In this scenario, it seems you would simply double the price you pay the publisher. The more likely scenario is a publisher may wish to adjust all your bids downward by 2-5%, as they do for each ssp, in order to account for billing discrepencies. This adjustment would then deflate your payments, necessitating a further downward adjustment, in a neverending cycle.

Third, the Kobler bid adapter is resolving the macros in the kobler win notification. The other pull request was to address the point just above, where it seemed a bug was implemented in that the publisher adjusted price was considered the win price. There is no reason the kobler bid adapter could not resolve a currency macro eg

const winNotificationUrl = replaceAuctionCurrency(replaceAuctionPrice(bid.nurl, bid.originalCpm || cpm))

where replace auction currency is a function you define in your adapter. There would be no need for a currency macro convention, you could call it Beezlebub and it would work fine as long as your adapter function expected that.

However, publishers do not hold second price auctions, the existence of the price macro seems to be a holdover from days of lore when ssps did. You shouldn't open yourself up to malfeasance so easily; you should just fill this macro in, and likely encrypt it, before you transmit your win notification along, there is no good reason to have it filled in client side.

acsbendi commented 2 years ago

Thank you for the thorough explanation! Based on it, the following is how I understand the whole picture now. The general recommendation is not to rely on Prebid.js for billing in any way. Instead, we just have to base our billing on the fact that Prebid is first-price always, and if we win, we have to pay exactly the price we bid. Please let me know if any of this is not correct or imprecise.

Assuming my understanding is correct, I have 2 remaining questions/suggestions:

patmmccann commented 2 years ago

I agree Auction Price should be deprecated, we're just exposing ourselves to vulnerabilities. I suppose it is to support the remaining ecosystem players that haven't taken action yet. I changed the doc as you suggested https://github.com/prebid/prebid.github.io/pull/3743

Why would you allow the publisher to choose the exchange rate? Aren't publishers going to choose rates extremely favorable to themselves? This seems like another area you would open yourselves to vulnerability. You may bid in euros, and I may decide that euros are on parity with yen and pay you back in yen? This is obviously an extreme scenario, but it seems unwise for you to allow. If you are going to send back the price and currency the publisher is choosing, I certainly recommend preserving the original in another key value pair.

In fact, you could change your adapter back to have this behavior if you so choose, as long as you also pass the original cpm. We didn't intend to throw a wrench in your process, we thought we were benevolently correcting an oversight in your adapter code. You should not pay publishers on the adjusted prices for reasons expressed elsewhere in the thread, but if you want to record them, there isn't a rule against it I am aware of.

The concept of an ssp receiving a bill from a publisher with the publishers chosen currency and adjustments to any bid is quite unusual. Publishers typically receive a check from an ssp based on the ssp's calculations and do discounting so that the estimated payments match the actuals.

acsbendi commented 2 years ago

I think I understand everything now, thanks again.

The changed behavior that caused a problem for us was not actually in our adapter, it was the change in what's being substituted in the actual ad tag before serving (see here). We only use the win notifications for some extra logging, the billing is done based on the ad tag request.

The real solution in our case is just to bid in the publisher's currency so that no conversion will be necessary and of course, to only trust the price received in our own encrypted payload. And if conversion is necessary for any reason, we will also record the original price along with the converted one.