Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update parameter field from consentManagement iframe call #5505

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

jsnellbaker
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

Fixes #5292

The parameter field in the iframe call used by the consentManagement module was causing some issues to the Quantcast CMP (specifically when it was set to null). From reviewing our use of the field, we don't actually use the field in any v1 or v2 CMP calls (as we were just hard-coding the value to null in all use-cases anyway).

From the community's testing, removing the field entirely addresses the issue with the Quantcast CMP. From my testing, removing the field didn't show any issues for other CMPs - so this removal should be safe to implement.

@jbartek25
Copy link
Contributor

The parameter field shouldn't just be removed from window.__cmp and window.__tcfapi signature.
Imagine this iframe scenario: top frame (with CMP) -> unfriendly iframe1 -> unfriendly iframe2. iframe2 is inside iframe1 and they are friendly with each other. Now iframe1 fetches the consent via postMessage from the top frame and defines window.__cmp for v1 or window.__tcfapi for v2. When iframe2 looks for CMP API via findCMP(), it will find the API function in iframe1 and will try to call it directly. In that case the iframe window.__cmp and window.__tcfapi functions must have the same signature as in IAB CMP spec, i.e. as the original API functions in the top frame:

  • v1: window.__cmp = function(cmd, arg, callback)
  • v2: window.__tcfapi = function(cmd, version, callback, arg)

I was going to open a GH issue that for v2 the iframe window.__tcfapi is not per IAB spec but since the fix suggested in this PR is also going to break compatibility with window.__cmp, I'm mentioning it here.

The easy fix for just the issue in this PR would be to replace null with undefined in the CMP call. Then a fix for iframe CMP function signatures would be to either let it be, ignore CMP API functions defined in child frames and make all unfriendly iframes always use postMessage and look for frame which contains __cmpLocator or __tcfapiLocator frame. Or let's fix the signatures and the null issue in one go by something like this: jbartek25@2fb536a

@stale
Copy link

stale bot commented Aug 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 8, 2020
@jsnellbaker jsnellbaker removed the stale label Aug 10, 2020
Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jsnellbaker
Copy link
Collaborator Author

Hi @jbartek25

I'm sorry for the delayed response here. I went through your comments/points and I had a few thoughts and questions in return.

In the iframe scenario described, I don't think that would actually be an issue. From what you described, it should happen when you're doing a subsequent call to the CMP after the go-around within this unique iframe environment. However, Prebid stores the consent data after the first request in memory and uses that value again on the follow-up auction requests within the same page session. If the user happened to update their consent information in that same session - the event listener we registered to the CMP would still be active and we'd receive the updated event trigger and store that new version of the consent data. When the user goes to a new page (ending that original page session), everything gets reset and we have to find the true CMP fresh again.

Further, even we didn't store the consent data and let the module try to find the 'CMP' again, it appears to still work out (at least from my testing). When Prebid first goes through the logic to create a local CMP function due to a 3rd party iframe, it creates that iframe at the same level where Prebid sits. In your example, that would be the 2nd inner iframe (at least that's what I got from the description). So when we call the findCMP during the 2nd round, it would immediately find the local CMP function at the same frame as Prebid.

The module would then use the local function path (not the iframe route) to make the CMP call. This local CMP function would then generate the postMessage request (with the needed data since we assign the version param independently and not using the function params). Because we're still in the same page session and the callCmpWhileInIframe hasn't been invoked, Prebid would remember the correct window frame to properly send the postMessage request and get the correct information from the CMP.

So given the factors above and the fact that we still don't actively populate the parameter field with a positive value in any of the postMessage request payloads - I think we would be fine without the extra set of changes from the commit that was linked (ie where it reformatted the function calls slightly but still generated the same request payload in the end).

We could opt to go with the undefined route, but I'm not sure that would fix the Quantcast issue. @Josh-G did you happen to test this alternative in your setup using Quantcast. If not, would you be able to test it for this PR?

@jbartek25
Copy link
Contributor

Hi @jsnellbaker

To clarify: iframe1 and iframe2 in my example are running separate Prebid.js instances. That should mean that each Prebid instance performs its own CMP lookup. iframe1 Prebid will create a local CMP function which iframe2 Prebid will find and call (since iframe1 and iframe2 are friendly) but the TCFv2 call will fail because iframe1 currently defines a local window.__tcfapi(cmd, arg, callback) but iframe2 will call it with "2" as the second argument because per TCFv2 spec __tcfapi has the version number as the second parameter. CMP throws an error because 2 is invalid parameter value.

@Josh-G
Copy link

Josh-G commented Aug 17, 2020

@jsnellbaker

@Josh-G did you happen to test this alternative in your setup using Quantcast. If not, would you be able to test it for this PR?

I have:

  • replicated the original issue
  • verified that removing parameter resolves the original issue
  • verified that setting parameter to undefined is acceptable to the Quantcast CMP
  • reproduced the issue reported by @jbartek25 and verified that their commit git apply'd as-is works for QC and resolves the issue

harpere
harpere previously approved these changes Aug 19, 2020
@harpere harpere dismissed their stale review August 19, 2020 16:25

want to review some of the comments again.

@@ -220,7 +219,7 @@ function lookupIabConsent(cmpSuccess, cmpError, hookConfig) {
window.addEventListener('message', readPostMessageResponse, false);

// call CMP
window[apiName](commandName, null, moduleCallback);
window[apiName](commandName, moduleCallback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Josh-G do I understand correctly that changing null to undefined fixes this for Quantcast?

window[apiName](commandName, undefined, moduleCallback);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if so, sounds like this is the change we should make to preserve the function signature.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llooks like an official quantcast response below , thanks guys!

@HeinzBaumann
Copy link

There is a discussion within the IAB Typescript slack forum. The IAB spec requires that arguments to be either valid or undefined. But not null. I think the core TCF library is going to be fixed. That said changing this to window[apiName](commandName, undefined, moduleCallback); will fix the issue.

@jsnellbaker
Copy link
Collaborator Author

To all involved, I have updated the changes to just use the undefined in the call as requested.

Please take a look when you get the chance.

@jsnellbaker jsnellbaker changed the title remove parameter field from consentManagement iframe call update parameter field from consentManagement iframe call Aug 27, 2020
@jsnellbaker jsnellbaker merged commit d90642d into master Sep 1, 2020
BrightMountainMediaInc pushed a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
* remove parameter field from consentManagement iframe call

* update to undefined approach
BrightMountainMediaInc added a commit to BrightMountainMediaInc/Prebid.js that referenced this pull request Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants