Merge pull request #1211 from cramforce/3p-args

Add a feature that checks arguments to 3p embeds

ads/a9.js

@@ -14,13 +14,14 @@
  * limitations under the License.
  */
 
-import {writeScript} from '../src/3p';
+import {writeScript, checkData} from '../src/3p';
 
 /**
  * @param {!Window} global
  * @param {!Object} data
  */
 export function a9(global, data) {
+  checkData(data, ['aax_size', 'aax_pubname', 'aax_src']);
   /*eslint "google-camelcase/google-camelcase": 0*/
   global.aax_size = data.aax_size;
   global.aax_pubname = data.aax_pubname;

ads/adreactor.js

@@ -14,13 +14,14 @@
  * limitations under the License.
  */
 
-import {writeScript} from '../src/3p';
+import {writeScript, checkData} from '../src/3p';
 
 /**
  * @param {!Window} global
  * @param {!Object} data
  */
 export function adreactor(global, data) {
+  checkData(data, ['zid', 'pid', 'custom3']);
   const url = 'https://adserver.adreactor.com' +
       '/servlet/view/banner/javascript/zone?' +
       'zid=' + encodeURIComponent(data.zid) +

ads/adsense.js

@@ -14,13 +14,14 @@
  * limitations under the License.
  */
 
-import {writeScript} from '../src/3p';
+import {writeScript, checkData} from '../src/3p';
 
 /**
  * @param {!Window} global
  * @param {!Object} data
  */
 export function adsense(global, data) {
+  checkData(data, ['adClient', 'adSlot']);
   /*eslint "google-camelcase/google-camelcase": 0*/
   global.google_page_url = global.context.canonicalUrl;
   const s = document.createElement('script');

ads/doubleclick.js

@@ -14,13 +14,17 @@
  * limitations under the License.
  */
 
-import {loadScript} from '../src/3p';
+import {loadScript, checkData} from '../src/3p';
 
 /**
  * @param {!Window} global
  * @param {!Object} data
  */
 export function doubleclick(global, data) {
+  checkData(data, [
+    'slot', 'targeting', 'categoryExclusion',
+    'tagForChildDirectedTreatment', 'cookieOptions'
+  ]);
   loadScript(global, 'https://www.googletagservices.com/tag/js/gpt.js', () => {
     global.googletag.cmd.push(function() {
       const googletag = global.googletag;

extensions/amp-iframe/0.1/test/test-amp-iframe.js

@@ -17,7 +17,8 @@
 import {Timer} from '../../../../src/timer';
 import {AmpIframe, listen} from '../amp-iframe';
 import {adopt} from '../../../../src/runtime';
-import {createIframePromise, pollForLayout} from '../../../../testing/iframe';
+import {createIframePromise, pollForLayout, poll}
+    from '../../../../testing/iframe';
 import {loadPromise} from '../../../../src/event-helper';
 import {resourcesFor} from '../../../../src/resources';
 import * as sinon from 'sinon';
@@ -43,6 +44,12 @@ describe('amp-iframe', () => {
     }
   };
 
+  function waitForJsInIframe() {
+    return poll('waiting for JS to run', () => {
+      return ranJs > 0;
+    }, undefined, 300);
+  }
+
   function getAmpIframe(attributes, opt_top, opt_height, opt_translateY) {
     return createIframePromise().then(function(iframe) {
       const i = iframe.doc.createElement('amp-iframe');
@@ -117,7 +124,7 @@ describe('amp-iframe', () => {
       expect(amp.iframe.src).to.equal(iframeSrc);
       expect(amp.iframe.getAttribute('sandbox')).to.equal('');
       expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper);
-      return timer.promise(0).then(() => {
+      return timer.promise(50).then(() => {
         expect(ranJs).to.equal(0);
       });
     });
@@ -132,7 +139,7 @@ describe('amp-iframe', () => {
       scrolling: 'no'
     }).then(amp => {
       expect(amp.iframe.getAttribute('sandbox')).to.equal('allow-scripts');
-      return timer.promise(100).then(() => {
+      return waitForJsInIframe().then(() => {
         expect(ranJs).to.equal(1);
         expect(amp.scrollWrapper).to.be.null;
       });
@@ -198,7 +205,7 @@ describe('amp-iframe', () => {
       expect(amp.iframe.src).to.equal(dataUri);
       expect(amp.iframe.getAttribute('sandbox')).to.equal('');
       expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper);
-      return timer.promise(0).then(() => {
+      return timer.promise(50).then(() => {
         expect(ranJs).to.equal(0);
       });
     });
@@ -220,7 +227,7 @@ describe('amp-iframe', () => {
       expect(amp.iframe.getAttribute('sandbox')).to.equal(
           'allow-scripts');
       expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper);
-      return timer.promise(0).then(() => {
+      return waitForJsInIframe().then(() => {
         expect(ranJs).to.equal(1);
       });
     });
@@ -367,7 +374,7 @@ describe('amp-iframe', () => {
       poster: 'https://i.ytimg.com/vi/cMcCTVAFBWM/hqdefault.jpg'
     }).then(amp => {
       const impl = amp.container.implementation_;
-      return timer.promise(0).then(() => {
+      return timer.promise(100).then(() => {
         expect(impl.iframe_.style.zIndex).to.equal('');
         expect(impl.placeholder_).to.be.null;
         expect(activateIframeSpy_.callCount).to.equal(2);

src/3p.js

@@ -123,3 +123,51 @@ export function validateSrcContains(string, src) {
     throw new Error('Invalid src ' + src);
   }
 }
+
+/**
+ * Throws a non-interrupting exception if data contains a field not supported
+ * by this embed type.
+ * @param {!Object} data
+ * @param {!Array<string>} allowedFields
+ */
+export function checkData(data, allowedFields) {
+  // Throw in a timeout, because we do not want to interrupt execution,
+  // because that would make each removal an instant backward incompatible
+  // change.
+  try {
+    validateData(data, allowedFields);
+  } catch (e) {
+    setTimeout(() => {
+      throw e;
+    });
+  }
+}
+
+/**
+ * Throws an exception if data contains a field not supported
+ * by this embed type.
+ * @param {!Object} data
+ * @param {!Array<string>} allowedFields
+ */
+export function validateData(data, allowedFields) {
+  const defaultAvailableFields = {
+    width: true,
+    height: true,
+    initialWindowWidth: true,
+    initialWindowHeight: true,
+    type: true,
+    referrer: true,
+    canonicalUrl: true,
+    pageViewId: true,
+    location: true,
+    mode: true,
+  };
+  for (const field in data) {
+    if (!data.hasOwnProperty(field) ||
+        field in defaultAvailableFields) {
+      continue;
+    }
+    assert(allowedFields.indexOf(field) != -1,
+        'Unknown attribute for %s: %s.', data.type, field);
+  }
+}

test/functional/test-3p-frame.js

@@ -16,6 +16,7 @@
 
 import {addDataAndJsonAttributes_, getIframe, getBootstrapBaseUrl,
     prefetchBootstrap} from '../../src/3p-frame';
+import {validateData} from '../../src/3p';
 import {documentInfoFor} from '../../src/document-info';
 import {loadPromise} from '../../src/event-helper';
 import {setModeForTesting, getMode} from '../../src/mode';
@@ -125,6 +126,7 @@ describe('3p-frame', () => {
       const c = win.document.getElementById('c');
       expect(c).to.not.be.null;
       expect(c.textContent).to.contain('pong');
+      validateData(win.context.data, ['ping', 'testAttr']);
     });
   });
 

test/functional/test-3p.js

@@ -14,31 +14,88 @@
  * limitations under the License.
  */
 
- import {validateSrcPrefix, validateSrcContains} from '../../src/3p';
-
- describe('3p', () => {
-   it('should throw an error if prefix is not https:', () => {
-     expect(() => {
-       validateSrcPrefix('https:', 'http://adserver.adtechus.com');
-     }).to.throw(/Invalid src/);
-   });
-
-   it('should not throw if source starts with https', () => {
-     expect(
-      validateSrcPrefix('https:', 'https://adserver.adtechus.com')
-    ).to.not.throw;
-   });
-
-   it('should throw an error if src does not contain addyn', () => {
-     expect(() => {
-       validateSrcContains('/addyn/', 'http://adserver.adtechus.com/');
-     }).to.throw(/Invalid src/);
-   });
-
-   it('should not throw if source contains /addyn/', () => {
-     expect(
-      validateSrcContains('/addyn/', 'http://adserver.adtechus.com/addyn/')
-    ).to.not.throw;
-   });
- });
+import {validateSrcPrefix, validateSrcContains, checkData, validateData}
+    from '../../src/3p';
+import * as sinon from 'sinon';
+
+describe('3p', () => {
+
+  let sandbox;
+  let clock;
+
+  beforeEach(() => {
+    sandbox = sinon.sandbox.create();
+    clock = sandbox.useFakeTimers();
+  });
+
+  afterEach(() => {
+    clock.tick(1000);
+    clock.restore();
+    sandbox.restore();
+  });
+
+  it('should throw an error if prefix is not https:', () => {
+    expect(() => {
+      validateSrcPrefix('https:', 'http://adserver.adtechus.com');
+    }).to.throw(/Invalid src/);
+  });
+
+  it('should not throw if source starts with https', () => {
+    validateSrcPrefix('https:', 'https://adserver.adtechus.com');
+  });
+
+  it('should throw an error if src does not contain addyn', () => {
+    expect(() => {
+      validateSrcContains('/addyn/', 'http://adserver.adtechus.com/');
+    }).to.throw(/Invalid src/);
+  });
+
+  it('should not throw if source contains /addyn/', () => {
+    validateSrcContains('/addyn/', 'http://adserver.adtechus.com/addyn/');
+  });
+
+  it('should accept good host supplied data', () => {
+    checkData({
+      width: '',
+      height: false,
+      initialWindowWidth: 1,
+      initialWindowHeight: 2,
+      type: true,
+      referrer: true,
+      canonicalUrl: true,
+      pageViewId: true,
+      location: true,
+      mode: true,
+    }, []);
+    clock.tick(1);
+
+    checkData({
+      width: "",
+      foo: true,
+      bar: true,
+    }, ['foo', 'bar']);
+    clock.tick(1);
+  });
+
+  it('should complain about unexpected args', () => {
+    checkData({
+      type: 'TEST',
+      foo: true,
+      'not-whitelisted': true,
+    }, ['foo']);
+    expect(() => {
+      clock.tick(1);
+    }).to.throw(/Unknown attribute for TEST: not-whitelisted./);
+
+
+    expect(() => {
+      // Sync throw, not validateData vs. checkData
+      validateData({
+        type: 'TEST',
+        foo: true,
+        'not-whitelisted2': true,
+      }, ['not-whitelisted', 'foo']);
+    }).to.throw(/Unknown attribute for TEST: not-whitelisted2./);
+  });
+});
 

testing/iframe.js

@@ -225,7 +225,7 @@ export function poll(description, condition, opt_onError, opt_timeout) {
         }
       }
     }
-    let interval = setInterval(poll, 50);
+    let interval = setInterval(poll, 8);
     poll();
   });
 }