Disallow some or all bidders from using setcookie / set local storage methods

[patmmccann]

Type of issue

Several bid adapters duplicate the functionality of SharedId in which an id is set in the publishers' first party domain. I believe this behavior should be opt in and in tight control of the publisher. The module rules say this behavior is reserved for ID submodules, but in the RTD module rules section. I believe this rule should extend to bid adapters as well

7279 is an example of a PR to add this functionality.

Here are the adapters that appear to do it already: https://github.com/prebid/Prebid.js/blob/12f461093b15657e11bedacba91319db873201b0/modules/datablocksBidAdapter.js#L99 https://github.com/prebid/Prebid.js/blob/0ca2a83a28ecdcfba37035f01c34aa19a64932bb/modules/adbookpspBidAdapter.js#L625 https://github.com/prebid/Prebid.js/blob/481b94f8c27d535d7b9105a69e76d01bad49a64b/modules/jixieBidAdapter.js#L30 https://github.com/prebid/Prebid.js/blob/412277f72b97cab72644cb4a009f58067a165899/modules/widespaceBidAdapter.js#L235 https://github.com/prebid/Prebid.js/blob/3e71ab46dabced5a17716b548a64fb818af268ef/modules/unicornBidAdapter.js#L138 https://github.com/prebid/Prebid.js/blob/0492821b8ce055cac419ad00ac90eee1b66e5e92/modules/relaidoBidAdapter.js#L245 https://github.com/prebid/Prebid.js/blob/412277f72b97cab72644cb4a009f58067a165899/modules/ucfunnelBidAdapter.js#L288

One adapter does it in their tests https://github.com/prebid/Prebid.js/blob/a633a637db703f90cb7dda9b160ea8b7fc401113/test/spec/modules/gmosspBidAdapter_spec.js#L65

Another adapter writes their own set cookie method in tests, and testing indicates they do this in the wild as well on winning bids https://github.com/prebid/Prebid.js/blob/a633a637db703f90cb7dda9b160ea8b7fc401113/test/spec/modules/kargoBidAdapter_spec.js#L115

This adapter test implies this behavior as well https://github.com/prebid/Prebid.js/blob/a5e7d08950f33301372942a64333522474285895/test/spec/modules/invibesBidAdapter_spec.js#L201

I think it is important we clarify the https://docs.prebid.org/dev-docs/module-rules.html on this behavior and potentially require changes to the above adapters in prebid 6 or prebid 7.

[patmmccann]

@muuki88 were you able to track down the bidder putting the entire consent string in the 1st party cookie?

[muuki88]

Not yet unfortunately :disappointed: Will update here if I find something :+1:

[bretg]

Removing the 6.0 label. @patmmccann - is this an accurate summary of the proposal:

1) Update the Rules to clarify that bid adapters cannot generate or store IDs without publisher control 2) Create a mechanism for publishers to control this behavior per-bidder. Default behavior should be to allow this for now. We might flip the behavior to disallow-by-default in a future major release. 3) Update bid adapters to read this new setting before generating/storing an ID

[patmmccann]

Almost, Changes to storage manager, not adapters, should take care of #3

On Fri, Nov 5, 2021, 11:57 AM bretg @.***> wrote:

Removing the 6.0 label. @patmmccann https://github.com/patmmccann - is this an accurate summary of the proposal:

1. Update the Rules to clarify that bid adapters cannot generate or store IDs without publisher control
2. Create a mechanism for publishers to control this behavior per-bidder. Default behavior should be to allow this for now. We might flip the behavior to disallow-by-default in a future major release.
3. Update bid adapters to read this new setting before generating/storing an ID

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prebid/Prebid.js/issues/7280#issuecomment-962013474, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM25Z7I55Z2NAIP2KCCHODUKQEHNANCNFSM5B2TLQSA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[bretg]

Ok, so it's bigger than IDs -- it's any use of local storage

[bretg]

The global module rules already state:

Modules must not add any pixel, iframe, cookie or local storage directly onto the page. Rather, they must use wrapper-provided mechanisms for usersyncs, cookies, and local storage.

That seems enough for this.

Here's a proposed config adding a new value to the "bidderSettings" object:

pbjs.bidderSettings = {
    standard: {
         ...
         storageAllowed: true
    },
    bidderA: {
        ...
         storageAllowed: false
    },
    bidderB: {
        ...
         storageAllowed: false
    },
}

The storageManager code would be changed to read this and enforce for cookies and localStorage.

If storageManager rejects a storage request, a debug log should be emitted: "BidderA storage request declined due to biddersettings."

I'm assuming that we don't need to break out the config to separate variables for the specific technology being used.

[dgirardi]

From the storage manager layer, as far as I can tell, there's no easy way to get at the right bidder code to look up in bidderSettings. It may have the GVLID and/or module name which in many cases would work, but not always (I counted 33 adapters that just call getStorageManager() without passing in any sort of identifier; even if they do pass in the module name, it's not clear to me that it would be the same as the bidder code, if not by convention).

To have this new setting work as expected, it seems to me that we need to:

• go through every adapter and check / update that they always pass their bidder code as the module name to getStorageManager, and
• enforce this convention for every adapter in the future.

Alternatively, we could refactor the way adapters are registered to make sure that getStorageManager (and maybe getConfig) know who the caller is, but that'd be a bigger task.

[dgirardi]

I learned that getConfig already has something similar in place, e.g. in https://github.com/prebid/Prebid.js/blob/1463ed95401a91defb6b020b5c45b5091b67ea5d/src/adapterManager.js#L429

However this approach would still not work without reviewing every adapter (it wouldn't work in general for getConfig either, but I'm guessing that is known and something that adapter reviewers keep in mind). The "context" is only there when running synchronous code within runWithBidder, which is not the case if for example an adapter calls getStorageManager on initialization, like:

https://github.com/prebid/Prebid.js/blob/1463ed95401a91defb6b020b5c45b5091b67ea5d/modules/adagioBidAdapter.js#L24

https://github.com/prebid/Prebid.js/blob/1463ed95401a91defb6b020b5c45b5091b67ea5d/modules/adbookpspBidAdapter.js#L563

https://github.com/prebid/Prebid.js/blob/1463ed95401a91defb6b020b5c45b5091b67ea5d/modules/adqueryBidAdapter.js#L14

... and many others.

Bottom line is we would still need to check every adapter and update a bunch of them, the change would be bigger (and harder to maintain) than changing their calls to getStorageManager.

IMO a better approach would be to change adapter registration so that instead of registerBidder(spec) we'd have something like:

registerBidder('bidderA', ['storage', function(storage) {...}]);

registerBidder('bidderB', ['config', 'storage', function(config, storage) { ... }])

That (or some other style of dependency injection) would allow us to tailor behavior of storage, config and so on without allowing the adapter code to break it (even in asynchronous callbacks). But it's of course a non-trivial change.

For now, should we go ahead with this by checking and/or updating every call to getStorageManager to pass in the bidder code as module name?

[bretg]

Thanks for the analysis dgirardi. Since this would be a pretty fundamental change... adding @jsnellbaker, @robertrmartinez, @mkendall07, @snapwich for comment.

[dgirardi]

A better example of an adapter that wouldn't work with config's getCurrentBidder is:

https://github.com/prebid/Prebid.js/blob/1463ed95401a91defb6b020b5c45b5091b67ea5d/modules/adagioBidAdapter.js#L198-L215

this is run on initialization, outside of any auction, so there's no magic way for storage to know who's calling. I don't know of any way to find these problems without manually reviewing every adapter.

[dgirardi]

I should also note that it should be feasible to automatically transpile adapters to have their dependencies injected, which could make a transition easier. We could for example write a webpack loader that looks for certain imports and wraps the source in a closure like above.

[mkendall07]

@dgirardi that's an interesting idea. I'd be curious for a proof of concept PR of that. I'm also interested in what @snapwich thinks, he IIRC was the original author behind the getConfig access control, so can speak better design decisions behind that implementation and limitations.

[snapwich]

IMO a better approach would be to change adapter registration so that instead of registerBidder(spec) we'd have something like:

registerBidder('bidderA', ['storage', function(storage) {...}]);

registerBidder('bidderB', ['config', 'storage', function(config, storage) { ... }])

That (or some other style of dependency injection) would allow us to tailor behavior of storage, config and so on without allowing the adapter code to break it (even in asynchronous callbacks). But it's of course a non-trivial change.

hi @dgirardi ,

i'm not opposed to what you're suggesting but i think it is very similar to early proposals i made about how our module system should work (very require.js-esque) when i was originally architecting how prebid.js modules would work. it was decided early in that process among the larger dev group that we would support es6 modules and that's how modules should request dependencies. i think this decision was primarily made to support "modern syntax" and attract devs to contribute modules.

as you can see it's mostly worked but due to the static nature of es6 modules it does cause issues around code that runs in the body of the module (i.e. not in spec methods). the config.runWithBidder code was added much later as an attempt to add some dynamic functionality to certain dependencies but it's not without it's problems due to how are modules are structured. it wraps all the spec functions and allows them access to a bidder context as well as wraps callbacks (like for ajax responses). i'm not sure if that same system could be made to work with what we want here?

so as i said i'm not opposed, however it would be a pretty large refactor from where we are now and does kinda go against our initial design decision (which is maybe okay). also, it would kind of be a mixture of two dependency systems (es6 and then the require.js esque dependency injection you're suggesting) which could be a little confusing.

if we did go with it i would make the following proposal

registerBidder('bidderB', function({ config, storage }) {
    return {
      buildRequests( ... ) { ... },
      interpretResponse( ... ) { ... },
      // etc
    } // the spec object
})

basically i don't recommend a dynamic string based dependency injection system as it seems prone to runtime errors and impossible to type check (if we were to add typescript support to the module api in the future). i think passing a keyed object works perfectly fine and can still be extended with new keys in the future which allows for type checking.

once again though, in my mind this would be a pretty large refactor and if i remember correctly config.runWithBidder was specifically designed the way it was to avoid a large refactor. so basically i think we should think about this and involve more people. i agree with @mkendall07 that a proof of concept would be pretty crucial in determining if this would actually work in the ways we want it to and be feasible.

[dgirardi]

@snapwich the issue is not so much with the module system but rather the interface exposed by things like getStorageManager; if the caller had to identify itself it'd be fine. I'll admit that it took me a while to realize that we don't need to wrap everything in a closure to work around that; it should be enough to wrap factory functions like getStorageManager in their own factory function; POC here: https://github.com/prebid/Prebid.js/pull/7705

We could use the existing runWithBidder solution for this issue, if we are comfortable with it for GDPR then it should be OK here as well (and we can maybe revisit later if that POC goes well). To me it seems a maintenance nightmare but I now realize that we are already relying heavily on it.