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

Implement stricter sandboxing for amp-ad #14240

Closed
jasti opened this Issue Mar 26, 2018 · 14 comments

Comments

@jasti
Copy link
Collaborator

jasti commented Mar 26, 2018

M66+ now has feature detection of the 'allow-top-navigation-by-user-activation' sandbox feature too.

@jasti jasti added the Category: Ads label Mar 26, 2018

@jasti jasti added this to the Prioritized FRs milestone Mar 26, 2018

@jasti jasti added this to Sprint Candidate in AMP Advertising Mar 26, 2018

@lannka

This comment has been minimized.

Copy link
Collaborator

lannka commented Aug 3, 2018

@jasti was the idea to add this constraint to all 3P ad frames? Or do we let pub/ad network opt-in?

@keithwrightbos is it something used by doubleclick in non-AMP world? I'm bit worried if this could cause false alarm if the creative implementation somehow delayed the navigation after a click.

@jasti

This comment has been minimized.

Copy link
Collaborator

jasti commented Aug 3, 2018

@lannka my idea was to make it the default across all 3P ad frames.

@lannka

This comment has been minimized.

Copy link
Collaborator

lannka commented Aug 3, 2018

in #13146 @coreybyrnes was also suggesting to set allow-scripts allow-popups to limit the top window navigation.

@keithwrightbos I want to learn what doubleclick does, and what's the risks of enabling it now.

@lubin2010

This comment has been minimized.

Copy link

lubin2010 commented Sep 7, 2018

I want to learn what doubleclick does, and what's the risks of enabling it now.

@lusilva knows how doubleclick works, and has been doing sandboxing for a while. I can start a thread about it.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Sep 11, 2018

Do we expect this to get done in Q3?

cramforce added a commit to cramforce/amphtml that referenced this issue Sep 12, 2018

Apply a sandbox to all ad iframes
…if required sandbox flags are supported (which they are widely).

Implements ampproject#14240
@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Sep 12, 2018

I decided to implement this in #18003 on my morning commute :)

But I will need help launching it by the core team.

@lubin2010

This comment has been minimized.

Copy link

lubin2010 commented Sep 12, 2018

Super fast! Thanks @cramforce. Looking forward to the launch.

cramforce added a commit that referenced this issue Sep 12, 2018

Apply a sandbox to all ad iframes (#18003)
…if required sandbox flags are supported (which they are widely).

Implements #14240
@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Sep 13, 2018

@keithwrightbos I saw that fast-fetch-3p-frame already has a sandbox implementation. What is the state on that experiment? Do we even ever use the case I changed and could run an experiment there?

My implementation feature detects the new sandbox APIs. You may want to consider porting that code over since the current code would prevent all navigation in e.g. iOS<11 or, I believe, Microsoft Edge, and possibly also Firefox (not sure about that one).

@keithwrightbos

This comment has been minimized.

Copy link
Collaborator

keithwrightbos commented Sep 13, 2018

In the current implementation, we have other feature detection methodologies to determine where sandboxing is available and apply it as such. I will create a PR that removes the FF 3p frame sandboxing code, uses yours, and augment getIframe to take a disable sandboxing parameter so that we can keep parity for now and start an experiment where we allow sandboxing for all AdSense/Doubleclick FF traffic. Thanks

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Sep 13, 2018

Thanks!

I did some research and at least for the navigation-on-load there should be an intervention in Chrome since 69 that prevents it by default. Safari, however, is still vulnerable to the attack.

@lannka

This comment has been minimized.

Copy link
Collaborator

lannka commented Oct 30, 2018

@keithwrightbos any updates from your experiment? If things look good, we will go forward for all 3P ad networks.

@lannka

This comment has been minimized.

Copy link
Collaborator

lannka commented Nov 9, 2018

Google Ad Manager experiment looks neutral. We should go ahead and turn it on.

@lannka

This comment has been minimized.

Copy link
Collaborator

lannka commented Nov 9, 2018

@ampproject/a4a

to protect user experience, we plan to leverage the stricter sandboxing features available on Chrome 66+ for amp-ad. According to Google Ad Manager's experiment, no noticeable revenue impact was seen.

We plan to turn on the feature on the week after Thanksgiving (12/4). Please let us know if you have any concerns.

/cc @jasti do you want to add anything?

@jasti

This comment has been minimized.

Copy link
Collaborator

jasti commented Nov 16, 2018

All good from my end.

Enriqe added a commit to Enriqe/amphtml that referenced this issue Nov 28, 2018

Apply a sandbox to all ad iframes (ampproject#18003)
…if required sandbox flags are supported (which they are widely).

Implements ampproject#14240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment