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

Safe frame issue fix #64

Merged
merged 1 commit into from
Jun 28, 2019
Merged

Safe frame issue fix #64

merged 1 commit into from
Jun 28, 2019

Conversation

robertrmartinez
Copy link
Contributor

@robertrmartinez robertrmartinez commented Jun 27, 2019

Seems like recently GAM has started always returning Safe Frames as HTTPS.

So our code which assumes our current executing iframe had the same protocol as the publishers domain needs to be updated.

We will get the protocol from the window.location now.

Here are examples of this in different browsers when my test page is loaded via HTTP but the iframe delivered is in HTTPS:

CHROME:

Before:
image

After:
image

Notice the Mixed-Content warning will happen now because prebid and bidders expected HTTP, so may have responded with HTTP content.

FireFox:

Before:
image

Notice that Firefox actually logged the error that caused ads not to render in this scenario!

After:
image

Safari:

Before:
image

After:
image

IE 11

Before:
image

After:

image

@kizzard
Copy link
Contributor

kizzard commented Jun 27, 2019

The legacy safe frame creative at https://github.com/prebid/Prebid.js/blob/master/integrationExamples/gpt/x-domain/creative.html is also affected and could use a PR too

@robertrmartinez
Copy link
Contributor Author

robertrmartinez commented Jun 27, 2019

Here are the results of a Selenium Test which:

Runs across the latest stable versions of: Chrome, Safari, Firefox, IE , Microsoft Edge

loads a test page via HTTP

Gets a rubicon bid via prebid

Calls DFP

Returns the dfp creative which uses the universal creative via Safe frame.

Then asserts that:

  1. the returned iframe is a safeframe
  2. The universal creative is loaded
  3. The rubicon project ad is rendered successfully.

image

IT is the same test page an assertions, but using the NEW creative from this PR, or the current production creative.

@robertrmartinez
Copy link
Contributor Author

robertrmartinez commented Jun 27, 2019

The legacy safe frame creative at https://github.com/prebid/Prebid.js/blob/master/integrationExamples/gpt/x-domain/creative.html is also affected and could use a PR too

Hi @kizzard, thanks for pointing this out, we will update once this PR gets hashed out and approved.

@robertrmartinez
Copy link
Contributor Author

@jaiminpanchal27 I think something is wrong with Browserstack?

Looks like some of the browsers are failing to initialize or something, going to try and re-run.

@idettman idettman self-requested a review June 27, 2019 22:38
idettman
idettman previously approved these changes Jun 27, 2019
@idettman idettman self-requested a review June 27, 2019 22:48
@idettman
Copy link
Contributor

Code LGTM, waiting on circleci for final approval

@idettman idettman dismissed their stale review June 27, 2019 22:59

Removing until circleci issue is resolved

@robertrmartinez
Copy link
Contributor Author

@idettman @jaiminpanchal27

Circle CI is just super flakey right now for some reason.

Not sure what the problem is but they finally passed.

I even created a test branch of master and ran them and they still fail!

https://circleci.com/gh/prebid/prebid-universal-creative/123?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Copy link
Contributor

@harpere harpere left a comment

Choose a reason for hiding this comment

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

Code change looks good, but are there tests that need to be updated?

@jaiminpanchal27
Copy link
Collaborator

@robertrmartinez I have seen this happening in this repo with one of my PR's as well. Nothing related to your fix.

@patmmccann
Copy link

does the cross domain example creative on prebid.js integration examples need fixing too?

@patmmccann
Copy link

specifically, is this render function now broken also?

https://github.com/prebid/Prebid.js/blob/master/integrationExamples/gpt/x-domain/creative.html

@robertrmartinez
Copy link
Contributor Author

@patmmccann Yep!

If you look a couple comments up @kizzard brought this up already :)

The legacy safe frame creative at https://github.com/prebid/Prebid.js/blob/master/integrationExamples/gpt/x-domain/creative.html is also affected and could use a PR too

Hi @kizzard, thanks for pointing this out, we will update once this PR gets hashed out and approved.

Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

LGTM

robertrmartinez added a commit to prebid/Prebid.js that referenced this pull request Jun 28, 2019
Updating for the new safe frame issue discovered, see prebid/prebid-universal-creative#64 for more details!
@robertrmartinez
Copy link
Contributor Author

prebid/Prebid.js#3955

Here is the fix for Prebid.js's legacy x-domain creative.

@patmmccann
Copy link

@patmmccann Yep!

If you look a couple comments up @kizzard brought this up already :)

The legacy safe frame creative at https://github.com/prebid/Prebid.js/blob/master/integrationExamples/gpt/x-domain/creative.html is also affected and could use a PR too

Hi @kizzard, thanks for pointing this out, we will update once this PR gets hashed out and approved.

Thanks!!

idettman pushed a commit to prebid/Prebid.js that referenced this pull request Jun 28, 2019
Updating for the new safe frame issue discovered, see prebid/prebid-universal-creative#64 for more details!
@idettman idettman merged commit 8f47f4a into master Jun 28, 2019
@patmmccann
Copy link

I'm a little confused on the fix merged to prebid.js. The adapters respond with http ads which still won't render in a secure tpc.googlesyndication.com frame; we've been looking at changing the http: to https: in the markup and that seems to get us over the edge. Have other publishers found this fixed their issues?

@patmmccann
Copy link

From Reddit adops:

warning that this doesn't fully fix the issue. It fixes the Prebid rendering issue, but some (maybe most) Prebid adapters mimic the page protocol when requesting bids, so will be requesting insecure ads which then won't necessarily render correctly in the now secure safe frame context. Fixes are to either monkey patch prebid to use all secure bidder URLs, or force your traffic onto a secure version of the site.

@robertrmartinez
Copy link
Contributor Author

Hi @patmmccann

I brought this up a few days ago before the merge in reddit adops.

https://redditadops.slack.com/archives/C0HVALS8P/p1561680898099500?thread_ts=1561595865.094600&cid=C0HVALS8P

I am not very keen on many real world setups, but I have a feeling simply having custom code to change the scripts and replace http with https is not a great idea.

Google did this for a reason, whatever that reason is, it may be an indicator that we need to push for https ads only in prebid now maybe?

As of now a large portion of the adapters detect the pages protocol and use that for their ad requests.

I am guessing that simply altering adapters markups to be https may cause some issues depending on how each adapters ad system works.

We probably need to have a larger discussion about making all ad requests https perhaps?

I do not know the impact of this.

@robertrmartinez
Copy link
Contributor Author

@patmmccann This "fix" is a band aid and we need to come up with the right solution for prebid overall.

It looks like DFP will now send over safeframes as https always, so we need to come up with the prebid solution for this.

Wether that be:

  • only allow https connections
  • a new flag for publishers to set which only allows https adapter connections
  • some other idea we can all come up with!

@patmmccann
Copy link

patmmccann commented Jul 2, 2019 via email

VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
Updating for the new safe frame issue discovered, see prebid/prebid-universal-creative#64 for more details!
sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
Updating for the new safe frame issue discovered, see prebid/prebid-universal-creative#64 for more details!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants