Don't load responsive ad if tag height is not AdSense whitelisted height #12225

noranazmy · 2017-11-28T12:14:00Z

Prevent tampering with height attribute in AdSense responsive ad tags

googlebot · 2017-12-14T13:48:08Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2017-12-14T17:20:09Z

CLAs look good, thanks!

lannka · 2017-12-14T18:06:31Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

@@ -63,6 +63,7 @@ import {insertAnalyticsElement} from '../../../src/extension-analytics';
 import {
  getAdSenseAmpAutoAdsExpBranch,
 } from '../../../ads/google/adsense-amp-auto-ads';
+import {validateAdSenseData} from '../../../ads/google/utils';


this introduces a dependency to 3p/ directory, which we want to avoid.

there is a TODO in doubleclick impl related to this as well. It might make sense to move the validateData function out of 3p/ for sharing.

@keithwrightbos any thoughts where we should move this?

Hey @lannka, @keithwrightbos, any updates on this?

keithwrightbos · 2018-01-04T14:46:34Z

ads/google/utils.js

+  // TODO: check mandatory fields
+  validateData(data, [], [
+    'adClient', 'adSlot', 'adHost', 'adtest', 'tagOrigin', 'experimentId',
+    'ampSlotIndex', 'adChannel', 'autoFormat', 'a4aUpgradeType', 'fullWidth',


Why did you add a4aUpgradeType? That element is actually generated as part of the fast fetch flow for debugging purposes and so you can remove.

keithwrightbos · 2018-01-04T14:54:46Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+    const height = Number(this.element.getAttribute('height'));
+
+    // TODO: check mandatory fields
+    validateAdSenseData(this.element.dataset, height);


I don't think we should call 3p/validateData as part of Fast Fetch as its mostly related to delayed fetch implementation as adds no real value while introducing a dependency that we'd prefer it not. Instead, can you only check the responsive height requirement and do it within #isValidElement

keithwrightbos · 2018-01-05T14:20:28Z

ads/google/adsense.js

@@ -29,6 +30,15 @@ export function adsense(global, data) {
      ['adClient', 'adSlot', 'adHost', 'adtest', 'tagOrigin', 'experimentId',
        'ampSlotIndex', 'adChannel', 'autoFormat', 'fullWidth']);

+  if (data['autoFormat'] === 'rspv') {
+    assert(


user().assert?

keithwrightbos · 2018-01-05T14:21:08Z

ads/google/utils.js

@@ -16,6 +16,11 @@

 import {user} from '../../src/log';

+/**
+ * @const {number} ADSENSE_RSPV_WHITELISTED_HEIGHT


Nit: put comment above the type declaration

keithwrightbos · 2018-01-05T14:23:09Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

+   * @private
+   */
+  hasValidAdSenseHeight_() {
+    return Number(this.element.getAttribute('height')) ==


Can you user.error when hasValidAdSenseHeight_ is false to ensure there is an actionable error in the console? Also, there is no need to convert to Number as == is a type-less comparison (as opposed to === which is typed). So, '320' == 320 will return true.

keithwrightbos · 2018-01-05T14:23:51Z

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

@@ -132,6 +132,13 @@ describes.realWin('amp-ad-network-adsense-impl', {
      impl = new AmpAdNetworkAdsenseImpl(element);
      expect(impl.isValidElement()).to.be.true;
    });
+    it('should be NOT valid (wrong responsive ad height)', () => {


Can you add a test that verifies that isValidElement will return true when data-auto-format=rsvp and height=320?

keithwrightbos · 2018-01-05T14:25:18Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

@@ -159,6 +160,15 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
    return this.autoFormat_ == 'rspv';
  }

+  /**
+   * @return {boolean}


Add a nice comment here and rename function to hasValidResponsiveHeight (AdSense is not necessary given this is within amp-ad-network-adsense-impl)?

keithwrightbos · 2018-01-05T15:16:12Z

ads/google/test/test-adsense.js

+
+  it('throws on invalid responsive ad unit height', () => {
+    const data = {'autoFormat': 'rspv', 'height': '666'};
+    expect(() => {


No need for brackets, this can be: expect(() => adsense(env.win, data)).to.throw(...)

keithwrightbos · 2018-01-05T15:17:52Z

extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js

@@ -168,6 +180,15 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
     * to an amp-ad-adsense element. Thus, if we are an amp-ad, we can be sure
     * that it has been verified.
     */
+    if (this.isResponsive_() && !this.hasValidResponsiveHeight_()) {


This looks good, just one minor change which is to remove the hasValidResponsiveHeight_ function entirely as splitting it off doesn't add much IMO:

if (this.isResponsive_() && this.element.getAttribute('height') != ADSENSE_RSPV_WHITELISTED_HEIGHT) { ... }

keithwrightbos · 2018-01-05T15:18:38Z

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

+      expect(impl.isValidElement()).to.be.true;
+    });
+    it('should be NOT valid (wrong responsive ad height)', () => {
+      element = createAdsenseImplElement(


Curious as to why in one test you create a new element/impl and in another you just set attribute? Seems setAttribute should be sufficient, no?

keithwrightbos · 2018-01-05T15:20:15Z

ads/google/adsense.js

@@ -29,6 +31,15 @@ export function adsense(global, data) {
      ['adClient', 'adSlot', 'adHost', 'adtest', 'tagOrigin', 'experimentId',
        'ampSlotIndex', 'adChannel', 'autoFormat', 'fullWidth']);

+  if (data['autoFormat'] === 'rspv') {


You can remove the if statement entirely by combining rspv check in assert:

user().assert(data['autoFormat'] != 'rspv' || data['height] == ADSENSE_RSPV_WHITELISTED_HEIGHT, ...)

Also do you want == or === when doing height comparison? Do you know it will always be a number as opposed to a string?

…ght (ampproject#12225)

googlebot added the cla: yes label Nov 28, 2017

noranazmy force-pushed the block-ads-with-bad-height branch 3 times, most recently from 24bc5c4 to 1b5c373 Compare November 29, 2017 13:59

ampprojectbot added this to the Pending Triage milestone Dec 4, 2017

noranazmy force-pushed the block-ads-with-bad-height branch 2 times, most recently from 0a8639c to 79f758f Compare December 14, 2017 13:30

googlebot added cla: no and removed cla: yes labels Dec 14, 2017

noranazmy force-pushed the block-ads-with-bad-height branch 6 times, most recently from 5bd7481 to 5b75353 Compare December 14, 2017 17:20

googlebot added cla: yes and removed cla: no labels Dec 14, 2017

noranazmy force-pushed the block-ads-with-bad-height branch 2 times, most recently from 67c3617 to 0339447 Compare December 14, 2017 17:23

lannka self-requested a review December 14, 2017 17:59

lannka reviewed Dec 14, 2017

View reviewed changes

noranazmy force-pushed the block-ads-with-bad-height branch 2 times, most recently from c28ce9a to 24a7abb Compare December 15, 2017 09:24

lannka requested a review from keithwrightbos January 2, 2018 18:02

keithwrightbos requested changes Jan 4, 2018

View reviewed changes

noranazmy force-pushed the block-ads-with-bad-height branch 3 times, most recently from f88c180 to 2bbad8e Compare January 5, 2018 12:05

noranazmy force-pushed the block-ads-with-bad-height branch from 924f0ba to acecba4 Compare January 5, 2018 13:59

keithwrightbos requested changes Jan 5, 2018

View reviewed changes

noranazmy force-pushed the block-ads-with-bad-height branch 4 times, most recently from 0c3f210 to bffaa1a Compare January 5, 2018 15:05

keithwrightbos requested changes Jan 5, 2018

View reviewed changes

Don't load responsive ad if tag height is not AdSense whitelisted height

1dfeef8

noranazmy force-pushed the block-ads-with-bad-height branch from bffaa1a to 1dfeef8 Compare January 5, 2018 17:25

keithwrightbos approved these changes Jan 5, 2018

View reviewed changes

keithwrightbos merged commit 812f244 into ampproject:master Jan 5, 2018

gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018

Don't load responsive ad if tag height is not AdSense whitelisted hei…

b309047

…ght (ampproject#12225)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't load responsive ad if tag height is not AdSense whitelisted height #12225

Don't load responsive ad if tag height is not AdSense whitelisted height #12225

noranazmy commented Nov 28, 2017

googlebot commented Dec 14, 2017

googlebot commented Dec 14, 2017

lannka Dec 14, 2017

noranazmy Jan 2, 2018

keithwrightbos Jan 4, 2018

keithwrightbos Jan 4, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

keithwrightbos Jan 5, 2018

Don't load responsive ad if tag height is not AdSense whitelisted height #12225

Don't load responsive ad if tag height is not AdSense whitelisted height #12225

Conversation

noranazmy commented Nov 28, 2017

googlebot commented Dec 14, 2017

googlebot commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment