Prebid.js icon indicating copy to clipboard operation
Prebid.js copied to clipboard

Prebid Server: native ad JS event tracker support

Open jbartek25 opened this issue 2 years ago • 25 comments

Type of issue

Question/Feature

Description

DV360 requires support for native JS trackers as otherwise the bidding might be limited (more details here. Is there any reason why in this code there's only method 1 (img) but not also method 2? Since Prebid itself is a piece of JavaScript and hence it's safe to assume JS trackers are always supported. Or am I missing something?

jbartek25 avatar Jul 08 '22 14:07 jbartek25

I also think that should probably include 2. There was some discussion in https://github.com/prebid/Prebid.js/pull/8086#discussion_r863957819 but I'm afraid I don't recall the full context. @musikele do you remember why we agreed just image trackers was fine?

dgirardi avatar Jul 08 '22 16:07 dgirardi

I don't know why it's only 1 since ages, the "general" opinion is that pixels cause less harm than javascript trackers (that in theory may inject malicious code). When we discussed this topic we thought about adding 2 but then we decided to keep existing behaviour (and keep only 1).

musikele avatar Jul 11 '22 13:07 musikele

@musikele - is the intention of the new PBJS native approach to completely specify imp.native, or just imp.native.assets?

If all of imp.native, then seems like imp.native.eventtrackers would just be specified there.

If just just imp.native.assets, I would propose creating an s2sConfig override for the other fields in ORTB imp.native. e.g.

pbjs.setConfig({
    s2sConfig: {
        ...
        ortbNative: {.     // replaces the default JSON 
              context: 1,
              plcmttype: 1,
              eventtrackers: [
                {event: 1, methods: [1]}  // so you can put whatever you want
              ]
        }
    }
});

bretg avatar Jul 12 '22 20:07 bretg

@bretg , the new native requires users to specify the whole imp.native request, comprising of eventtrackers. That piece of code is just to specify defaults if any value is missing.

So, i don't think we need to change that piece of code right now... Also, not do this change in the native PR

musikele avatar Jul 13 '22 10:07 musikele

Ok cool @musikele. I don't see changes to the pbsBidAdapter in https://github.com/prebid/Prebid.js/pull/8086 -- agree we'd need some then, right?

@jbartek25 - this is the proposed solution to what you're bringing up... the AdUnit would specify whatever event trackers desired. Sound ok?

bretg avatar Jul 13 '22 12:07 bretg

@bretg I believe the current default for 1.2 (JS trackers disabled) is actually breaking the behaviour established with native 1.1 where jstracker was always executed without any further checks. Now with 1.2 we are saying that JS trackers are no longer allowed by default and have to be explicitly enabled. The same threat coming from JS imp trackers existed with 1.1 and nobody complained about any malicious code before, did they? If not, then 1.2 eventtrackers don't pose any bigger risk.

From a different angle, even when an override is possible via a config, we are an SSP and don't have control over the config, this would mean educating all our publishers, regularly check their Prebid setup and getting them update their configs which is a time consuming process. Also, JS trackers are now explicitly required by DV360 and which publisher don't want demand from Google? Maybe a small minority in which case the default behaviour should cover the majority use case, not the minor one.

jbartek25 avatar Jul 13 '22 13:07 jbartek25

I don't understand this part of your statement @jbartek25 :

the current default for 1.2 (JS trackers disabled) is actually breaking the behaviour established with native 1.1 where jstracker was always executed without any further checks

Aren't we talking about this specific piece of code? There's no 1.1 anywhere.

          mediaTypes['native'] = {
            request: JSON.stringify({
              context: 1,
              plcmttype: 1,
              eventtrackers: [
                {event: 1, methods: [1]}   // this one line of code is the focus of this issue, right?
              ],
              assets: nativeAssets
            }),
            ver: '1.2'                                // just 1.2. always.
          }

My understanding is that you're proposing this block of code get changed to add event 2:

          mediaTypes['native'] = {
            request: JSON.stringify({
              context: 1,
              plcmttype: 1,
              eventtrackers: [
                {event: 1, methods: [1]},
                {event: 2, methods: [1]}   // would adding this make everything right?
              ],
              assets: nativeAssets
            }),
            ver: '1.2'
          }

Right? If not, please be very explicit about what you'd like to see.

I'll coordinate a response from the relevant Prebid committees. Prebid doesn't have strong leadership in Native (obviously), so it may take some effort to get on the same page that this is a bug fix and not a breaking change.

bretg avatar Jul 13 '22 14:07 bretg

@bretg no not event 2, but method 2 should be added, i.e. {event: 1, methods: [1]} should be updated to {event: 1, methods: [1,2]}

Please see here and the ortb spec in the attached image. Screenshot 2022-07-13 at 16 23 36

jbartek25 avatar Jul 13 '22 14:07 jbartek25

Also, by mentioning 1.1 I wasn't refering to the exact code in the PBS adapter but rather to the general handling of 1.1 by Prebid: i.e. the adapters with native support put any JS tracker into nativeAd.javascriptTrackers bid variable (example and Prebid just dropped the JS tracker in DOM here - there's no Prebid config param to enable JS trackers for native 1.1, it is always on and any JS trackers coming from bidder adapter get executed.

jbartek25 avatar Jul 13 '22 14:07 jbartek25

Ok, thanks for the correction. So the proposal is to update the pbsBidAdapter code to

          mediaTypes['native'] = {
            request: JSON.stringify({
              context: 1,
              plcmttype: 1,
              eventtrackers: [
                {event: 1, methods: [1,2]}
              ],
              assets: nativeAssets
            }),
            ver: '1.2'
          }

Dug into the history and read the words above many times. Here's my understanding of what happened:

  1. 3 years ago Rich and Jason created this block of JSON in the pbsBidAdapter with little fanfare. Looks to me like it may have been top-of-the-head "this works for AppNexus" without necessarily broad community discussion.
  2. Their decision must not have been terrible, because it took 3 years for someone to point out maybe that decision wasn't quite fully fleshed out. That's this issue.
  3. Michele, knowing how awesome Rich and Jason are, guessed that maybe the reasoning behind the initial JSON was sensitivity to javascript in general.
  4. Jozef points out that javascript sensitivity is probably not the case because other parts of Prebid.js support native javascript trackers.

So basically, the argument here is that this 3-year old JSON is missing the number 2. It was an oversight at the time, and is neither a breaking change nor a security vulnerability.

I agree that it's not a breaking change because it pubs currently using this feature would not notice any problems if they upgrade to this version.

However, not sure I agree with the security vulnerability part. In general, I do not like the idea that the pbsBidAdapter is hardcoded with this value with no way for a pub to control it. This is a special adapter that's owned by Prebid and used by thousands of publishers with no recourse. If a pub doesn't like the security behavior of an individual bid adapter, they can choose not to use that bid adapter. But pbsBidAdapter is the only game in town of its type, so if there are any publishers out there who want to control javascript trackers, they would be unable to.

So I return to the suggestion made above: make this configurable in s2sConfig. As @musikele points out, this would only be useful in the 'legacy' native scenario where adunit.native.ortb is not specified.

pbjs.setConfig({
    s2sConfig: {
        ...
        ortbNative: {.     // replaces the default JSON 
              context: 1,
              plcmttype: 1,
              eventtrackers: [
                {event: 1, methods: [1]} 
              ]
        }
    }
});

I'm ok changing the default to include javascript trackers as long as publishers have (simple) recourse to control the behavior if they need to.

bretg avatar Jul 13 '22 15:07 bretg

I would be careful about labeling this a security parameter; Prebid does not attempt to enforce it, and I bet neither does PBS. If any downstream DSP puts a JS tracker in the response we will run it regardless of what was in this parameter.

dgirardi avatar Jul 13 '22 15:07 dgirardi

Fair. The general point is that hardcoded JSON is asking for trouble. There may well be other native parameters someone might want to tune. An override to a default would allow for this.

bretg avatar Jul 13 '22 16:07 bretg

I would strongly advocate including JS Event tracker support:

eventtrackers: [
                {event: 1, methods: [1,2]}
              ],

DV360 made JSON tracking of native required as of 7/1 so they can run their fraud script on page. From reading the initial request regarding DV360, I do think it is something buyers will expect. An overide to a default sounds like the right option.

jlustig11 avatar Jul 13 '22 18:07 jlustig11

Ok. Leaving it to @patmmccann to prioritize .

bretg avatar Jul 13 '22 20:07 bretg

@jbartek25 your pull request is welcomed

patmmccann avatar Jul 14 '22 18:07 patmmccann

safe to assume JS trackers are always supported

publishers may want to avoid js in native for other reasons, please make this configurable

patmmccann avatar Jul 14 '22 18:07 patmmccann

@patmmccann

publishers may want to avoid js in native for other reasons, please make this configurable

As part of the same PR? As I understand it, it will be configurable once the native PR is done and the PR for this ticket should be just to change the default.

jbartek25 avatar Jul 15 '22 07:07 jbartek25

@jbartek25 once the native pr is merged, we would still want the legacy specification to allow this parameter to be configurable in PBS adapter? We may not want to assume the transition will be quick.

https://github.com/prebid/Prebid.js/blob/3d71e71e0d9c5fe3ecd2fc2a855224eaecea9af2/modules/prebidServerBidAdapter/index.js#L688

The above line is from the branch in the PR.

patmmccann avatar Jul 18 '22 15:07 patmmccann

So to reiterate, what we're proposing is this:

  1. The new Native PR will provide a way to specify the eventtrackers when a pub changes to use adunit.native.ortb.
  2. We expect that most publishers will take some time to switch how native is defined in the adunit. i.e. legacy isn't going away soon
  3. It would be easy to create a way to override the hardcoded JSON in the pbsBidAdapter. This could prove useful for more than just this eventtrackers. The behavior is simple: if s2sConfig.ortbNative is specified, use it instead of what's currently hardcoded in pbsBidAdapter
  4. We can change the hardcoded value in pbsBidAdapter in PBJS 8.0

bretg avatar Jul 18 '22 15:07 bretg

I am tackling this as part of https://github.com/prebid/Prebid.js/pull/8738, with this logic:

  • use what's currently hardcoded as the default request,
    const defaultRequest = {
      context: 1,
      plcmttype: 1,
      eventtrackers: [
        {event: 1, methods: [1]}
      ],
    };
    
  • take in overrides from s2sConfig.ortbNative,
    Object.assign(defaultRequest, s2sConfig.ortbNative)
    

so that to signal support for js trackers (until 8.0), the pub needs to define

setConfig({
  s2sConfig: {
    // ....
    ortbNative: {
      eventtrackers: [
        {event: 1, methods: [1, 2]}
      ],
    }
 }  
})

I think this is slightly different from the proposal because I am not asking them to repeat context, plcmttype, etc if they are OK with the defaults.

dgirardi avatar Jul 27 '22 17:07 dgirardi

Actually - reading the native spec - I am not sure that the default we now set for context and plcmttype make sense, so maybe it's better to discard them if s2sConfig.ortbNative is specified?

  • context: The context in which the ad appears (1 is "Content-centric context such as newsfeed, article, image gallery, video gallery, or similar.")
  • plcmttype: The design/format/layout of the ad unit being offered (1 is "In the feed of content - for example as an item inside the organic feed/grid/listing/carousel.")

Neither is required, so I don't know why we decided to set them, I don't think either is generally correct.

dgirardi avatar Jul 27 '22 17:07 dgirardi

I agree, neither is correct to default to 1

patmmccann avatar Jul 27 '22 17:07 patmmccann

I started adopting native ortb in Improve Digital adapter and unless I missed something, the adapter defaults of any native params can be currently done at ad unit level via bidRequest.mediaTypes.native.ortb. For PBS the proposal is to allow adapter-level overrides via s2sConfig.ortbNative. I'm wondering, shouldn't ortbNative object be moved to the top config level to make the overrides available to all adapters, not just PBS?, i.e.:

setConfig({
    // ....
    ortbNative: {
      eventtrackers: [
        {event: 1, methods: [1, 2]}
      ],
    }
})

jbartek25 avatar Jul 28 '22 07:07 jbartek25

The issues I see with a top-level config like ortbNative are:

  • most properties do not make sense as global config. looking at the spec, the only ones I see that do are eventtrackers and privacy. The rest are specific to the adUnit/impression. This is a problem for s2sConfig.ortbNative as well.
  • all adapters would need to be updated to read the new config. Adapters that do not talk ORTB to their backend wouldn't necessarily even be able to use it.

In my opinion, it would make more sense to have a higher level control for native trackers. Something like

setConfig({
   nativeTrackers: {
       click: false // disable native click trackers 
       impression: {
         js: false,     // disable native js impression trackers
         img: false   // disable native pixel impression trackers
       },
       viewability: {
         // maybe? I am not up to speed on how the viewability module does things
       }
   }
})

(I'm sure someone smarter can think of better names). Those would turn off the logic that runs the trackers, instead of setting a flag on the request (although adapters could still use it to set their request accordingly). This can be done by core without help from the adapters. It also seems more valuable from a publisher POV because it actually means "disable trackers", not "ask downstream DSPs to please not send any trackers".

dgirardi avatar Jul 28 '22 22:07 dgirardi

I'm fine with the nativeTrackers config object.

Those would turn off the logic that runs the trackers, instead of setting a flag on the request

It's not a good idea to not set flag on the request and just turn off the trackers in the client. This would mean discarding all the bids that include JS trackers from the auction in the client, not giving DSPs/SSPs a chance to bid with a creative that doesn't have JS tracker.

jbartek25 avatar Jul 29 '22 12:07 jbartek25