From 124bad27ddadf01834b891604affb6379dbd93f4 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Mon, 1 Feb 2016 08:04:06 -0800 Subject: [PATCH 01/44] Update spec to correctly point out allowed link-rel values. Fixes #1546 --- spec/amp-html-format.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/amp-html-format.md b/spec/amp-html-format.md index 1923761e5c30..e988827c3116 100644 --- a/spec/amp-html-format.md +++ b/spec/amp-html-format.md @@ -126,11 +126,11 @@ HTML tags can be used unchanged in AMP HTML. Certain tags have equivalent custom | param | Prohibited. | | applet | Prohibited. | | embed | Prohibited. | -| form | Prohibited. | +| form | Prohibited. [Support coming in the future.](https://github.com/ampproject/amphtml/issues/1286) | | input elements | Prohibited. Includes input, textarea, select, option. Notably, button element is allowed. | | button | Allowed. | | style | [Required style tags for adjusting opacity](#opacity) One additional style tag is allowed in head tag for the purpose of custom styling. This style tag must have the attribute `amp-custom`. [🔗](#cust) | -| link | Allowed for certain values of rel: `canonical`. `stylesheet` is generally disallowed, but some values may be whitelisted for font providers. | +| link | `rel` values registered on [microformats.org](http://microformats.org/wiki/existing-rel-values) are allowed. If a rel value is missing from our whitelist, [please submit an issue](https://github.com/ampproject/amphtml/issues/new). `stylesheet` and other values like `preconnect`, `prerender` and `prefectch` that has side effects in the browser are disallowed. There is a special case for fetching stylesheets from whitelisted font providers. | | meta | The `http-equiv` attribute is banned. Otherwise allowed. | | a | The `href` attribute value must not begin with `javascript:`. If set, the `target` attribute value must be `_blank`. Otherwise allowed. [🔗](#ancr) | | svg | Most SVG elements are allowed | From 798e5274b329c8508c6ee8fb3c91754e177bcbba Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 28 Jan 2016 17:44:57 -0800 Subject: [PATCH 02/44] Send GA client id to doubleclick and adsense if available. Also renames the fallback cookie name to AMP_ECID_GOOGLE. --- ads/_config.js | 2 ++ ads/adsense.js | 7 +++++++ ads/doubleclick.js | 7 +++++++ extensions/amp-analytics/0.1/vendors.js | 3 ++- 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/ads/_config.js b/ads/_config.js index 1f3c8eef1074..4763f3339e5d 100644 --- a/ads/_config.js +++ b/ads/_config.js @@ -52,4 +52,6 @@ export const adPreconnect = { export const clientIdScope = { // Add a mapping like // adNetworkType: 'cidScope' here. + adsense: 'AMP_ECID_GOOGLE', + doubleclick: 'AMP_ECID_GOOGLE', }; diff --git a/ads/adsense.js b/ads/adsense.js index 1938c429b7dd..5c4e4369ae57 100644 --- a/ads/adsense.js +++ b/ads/adsense.js @@ -22,6 +22,13 @@ import {checkData} from '../src/3p'; */ export function adsense(global, data) { checkData(data, ['adClient', 'adSlot']); + if (global.context.clientId) { + // Read by GPT for GA/GPT integration. + global.gaGlobal = { + vid: global.context.clientId, + hid: global.context.pageViewId, + }; + } /*eslint "google-camelcase/google-camelcase": 0*/ global.google_page_url = global.context.canonicalUrl; const s = document.createElement('script'); diff --git a/ads/doubleclick.js b/ads/doubleclick.js index 2a26b62a4eaa..f78b1e2b502f 100644 --- a/ads/doubleclick.js +++ b/ads/doubleclick.js @@ -26,6 +26,13 @@ export function doubleclick(global, data) { 'tagForChildDirectedTreatment', 'cookieOptions', 'overrideWidth', 'overrideHeight', ]); + if (global.context.clientId) { + // Read by GPT for GA/GPT integration. + global.gaGlobal = { + vid: global.context.clientId, + hid: global.context.pageViewId, + }; + } loadScript(global, 'https://www.googletagservices.com/tag/js/gpt.js', () => { global.googletag.cmd.push(function() { const googletag = global.googletag; diff --git a/extensions/amp-analytics/0.1/vendors.js b/extensions/amp-analytics/0.1/vendors.js index 8a8b26561fbf..ac9024d5eed9 100644 --- a/extensions/amp-analytics/0.1/vendors.js +++ b/extensions/amp-analytics/0.1/vendors.js @@ -66,7 +66,8 @@ export const ANALYTICS_CONFIG = { 'host': 'https://www.google-analytics.com', 'basePrefix': 'v=1&_v=a0&aip=true&_s=${requestCount}' + 'dt=${title}&sr=${screenWidth}x${screenHeight}&_utmht=${timestamp}&' + - 'jid=&cid=${clientId(_ga)}&tid=${account}&dl=${documentLocation}&' + + 'jid=&cid=${clientId(AMP_ECID_GOOGLE)}&tid=${account}&' + + 'dl=${documentLocation}&' + 'dr=${documentReferrer}&sd=${screenColorDepth}&' + 'ul=${browserLanguage}&de=${documentCharset}' , 'baseSuffix': '&a=${pageViewId}&z=${random}', From 9b3bdb8c2d3d640bce19a417a1d326996546faf9 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Tue, 2 Feb 2016 14:27:08 -0800 Subject: [PATCH 03/44] Recreate stored objects as prototype-less objects --- src/service/storage-impl.js | 29 +++++++++++++++++++++---- test/functional/test-storage.js | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/service/storage-impl.js b/src/service/storage-impl.js index b4efae21f8ea..db7d69562f31 100644 --- a/src/service/storage-impl.js +++ b/src/service/storage-impl.js @@ -18,6 +18,7 @@ import {assert} from '../asserts'; import {getService} from '../service'; import {getSourceOrigin} from '../url'; import {isDevChannel, isExperimentOn} from '../experiments'; +import {isObject} from '../types'; import {log} from '../log'; import {timer} from '../timer'; import {viewerFor} from '../viewer'; @@ -202,15 +203,15 @@ export class Store { */ constructor(obj, opt_maxValues) { /** @const {!JSONObject} */ - this.obj = obj; + this.obj = recreateObject(obj); /** @private @const {number} */ this.maxValues_ = opt_maxValues || MAX_VALUES_PER_ORIGIN; /** @private @const {!Object} */ - this.values_ = obj['vv'] || {}; - if (!obj['vv']) { - obj['vv'] = this.values_; + this.values_ = this.obj['vv'] || Object.create(null); + if (!this.obj['vv']) { + this.obj['vv'] = this.values_; } } @@ -231,6 +232,8 @@ export class Store { * @private */ set(name, value) { + assert(name != '__proto__' && name != 'prototype', + 'Name is not allowed: %s', name); // The structure is {key: {v: *, t: time}} if (this.values_[name] !== undefined) { const item = this.values_[name]; @@ -366,6 +369,24 @@ export class ViewerStorageBinding { } +/** + * Recreates objects with prototype-less copies. + * @param {!JSONObject} obj + * @return {!JSONObject} + */ +function recreateObject(obj) { + const copy = Object.create(null); + for (const k in obj) { + if (!obj.hasOwnProperty(k)) { + continue; + } + const v = obj[k]; + copy[k] = isObject(v) ? recreateObject(v) : v; + } + return copy; +} + + /** * @param {!Window} window * @return {!Storage} diff --git a/test/functional/test-storage.js b/test/functional/test-storage.js index f561c2145278..7a5b93165ec6 100644 --- a/test/functional/test-storage.js +++ b/test/functional/test-storage.js @@ -101,6 +101,35 @@ describe('Storage', () => { }); }); + it('should initialize empty store with prototype-less objects', () => { + bindingMock.expects('loadBlob') + .withExactArgs('https://acme.com') + .returns(Promise.resolve(null)) + .once(); + return storage.get('key1').then(() => { + return storage.storePromise_; + }).then(store => { + expect(store.obj.__proto__).to.be.undefined; + expect(store.values_.__proto__).to.be.undefined; + }); + }); + + it('should restore store with prototype-less objects', () => { + const store1 = new Store({}); + store1.set('key1', 'value1'); + store1.set('key2', 'value2'); + bindingMock.expects('loadBlob') + .withExactArgs('https://acme.com') + .returns(Promise.resolve(btoa(JSON.stringify(store1.obj)))) + .once(); + return storage.get('key1').then(() => { + return storage.storePromise_; + }).then(store => { + expect(store.obj.__proto__).to.be.undefined; + expect(store.values_.__proto__).to.be.undefined; + }); + }); + it('should get the value first time and reuse store', () => { const store1 = new Store({}); store1.set('key1', 'value1'); @@ -391,6 +420,15 @@ describe('Store', () => { expect(store.get('k1')).to.be.undefined; expect(store.get('k2')).to.be.undefined; }); + + it('should prohibit unsafe values', () => { + expect(() => { + store.set('__proto__', 'value1'); + }).to.throw(/Name is not allowed/); + expect(() => { + store.set('prototype', 'value1'); + }).to.throw(/Name is not allowed/); + }); }); From 5076ccf69cd644464719c63ff00225c245176413 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Tue, 2 Feb 2016 16:39:30 -0800 Subject: [PATCH 04/44] Remove ambiguity of keys matching by requiring longer keys to match first --- src/url-replacements.js | 3 +++ test/functional/test-url-replacements.js | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/url-replacements.js b/src/url-replacements.js index b8db2527065a..96d8eab87cb5 100644 --- a/src/url-replacements.js +++ b/src/url-replacements.js @@ -351,6 +351,9 @@ class UrlReplacements { * @private */ buildExpr_(keys) { + // The keys must be sorted to ensure that the longest keys are considered + // first. This avoids a problem where a RANDOM conflicts with RANDOM_ONE. + keys.sort((s1, s2) => s2.length - s1.length); const all = keys.join('|'); // Match the given replacement patterns, as well as optionally // arguments to the replacement behind it in parantheses. diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index 9dcb1c3e912d..7a6f13c26e94 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -399,4 +399,21 @@ describe('UrlReplacements', () => { expect(res).to.equal('v=false'); }); }); + + it('should resolve sub-included bindings', () => { + // RANDOM is a standard property and we add RANDOM_OTHER. + return expand('r=RANDOM&ro=RANDOM_OTHER?', false, {'RANDOM_OTHER': 'ABC'}) + .then(res => { + expect(res).to.match(/r=(\d\.\d+)&ro=ABC\?$/); + }); + }); + + it('should expand multiple vars', () => { + return expand('a=VALUEA&b=VALUEB?', false, { + 'VALUEA': 'aaa', + 'VALUEB': 'bbb', + }).then(res => { + expect(res).to.match(/a=aaa&b=bbb\?$/); + }); + }); }); From a9497e599c2f95e88e422135e6f733bdab525fc2 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 1 Feb 2016 13:41:14 -0800 Subject: [PATCH 05/44] Pass referrer from the viewer --- build-system/tasks/presubmit-checks.js | 16 +- .../0.1/amp-dynamic-css-classes.js | 8 +- .../0.1/test/test-runtime-classes.js | 3 +- src/3p-frame.js | 4 +- src/service/viewer-impl.js | 95 ++++++++++- src/url-replacements.js | 3 +- test/functional/test-3p-frame.js | 19 ++- test/functional/test-viewer.js | 158 +++++++++++++++++- 8 files changed, 285 insertions(+), 21 deletions(-) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 6e9c50156e4d..303adbfa3970 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -195,6 +195,12 @@ var forbiddenTerms = { 'src/experiments.js', ] }, + 'isTrusted': { + message: requiresReviewPrivacy, + whitelist: [ + 'src/service/viewer-impl.js', + ] + }, 'eval\\(': '', 'storageFor': { message: requiresReviewPrivacy, @@ -243,7 +249,15 @@ var forbiddenTerms = { whitelist: [ 'extensions/amp-access/0.1/access-expr-impl.js', ], - } + }, + + // Overridden APIs. + '(doc.*)\\.referrer': { + message: 'Use Viewer.getReferrerUrl() instead.', + whitelist: [ + 'src/service/viewer-impl.js', + ], + }, }; var ThreePTermsMessage = 'The 3p bootstrap iframe has no polyfills loaded and' + diff --git a/extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js b/extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js index 2db1e9018c24..5c87c0cfaacc 100644 --- a/extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js +++ b/extensions/amp-dynamic-css-classes/0.1/amp-dynamic-css-classes.js @@ -14,11 +14,11 @@ * limitations under the License. */ +import {getService} from '../../../src/service'; +import {isExperimentOn} from '../../../src/experiments'; +import {log} from '../../../src/log'; import {parseUrl} from '../../../src/url'; import {viewerFor} from '../../../src/viewer'; -import {log} from '../../../src/log'; -import {isExperimentOn} from '../../../src/experiments'; -import {getService} from '../../../src/service'; import {vsyncFor} from '../../../src/vsync'; /** @const */ @@ -33,7 +33,7 @@ const EXPERIMENT = 'dynamic-css-classes'; * @returns {string} */ function referrerDomain(win) { - const referrer = win.document.referrer; + const referrer = viewerFor(win).getReferrerUrl(); if (referrer) { return parseUrl(referrer).hostname; } diff --git a/extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js b/extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js index 6e6b55177be0..67bf27e80876 100644 --- a/extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js +++ b/extensions/amp-dynamic-css-classes/0.1/test/test-runtime-classes.js @@ -16,6 +16,7 @@ import {createServedIframe} from '../../../../testing/iframe'; import {toggleExperiment} from '../../../../src/experiments'; +import {viewerFor} from '../../../../src/viewer'; import {vsyncFor} from '../../../../src/vsync'; function overwrite(object, property, value) { @@ -56,7 +57,7 @@ describe('dynamic classes are inserted at runtime', () => { overwrite(win.navigator, 'userAgent', userAgent); } if (referrer !== undefined) { - overwrite(fixture.doc, 'referrer', referrer); + viewerFor(win).getReferrerUrl = () => referrer; } return win.insertDynamicCssScript(); diff --git a/src/3p-frame.js b/src/3p-frame.js index e4e0512618c0..4adaaf5122cc 100644 --- a/src/3p-frame.js +++ b/src/3p-frame.js @@ -23,6 +23,7 @@ import {getMode} from './mode'; import {preconnectFor} from './preconnect'; import {dashToCamelCase} from './string'; import {parseUrl, assertHttpsUrl} from './url'; +import {viewerFor} from './viewer'; /** @type {!Object} Number of 3p frames on the for that type. */ @@ -55,6 +56,7 @@ function getFrameAttributes(parentWindow, element, opt_type) { attributes.initialWindowHeight = box.height; attributes.type = type; const docInfo = documentInfoFor(parentWindow); + const viewer = viewerFor(parentWindow); let locationHref = parentWindow.location.href; // This is really only needed for tests, but whatever. Children // see us as the logical origin, so telling them we are about:srcdoc @@ -63,7 +65,7 @@ function getFrameAttributes(parentWindow, element, opt_type) { locationHref = parentWindow.parent.location.href; } attributes._context = { - referrer: parentWindow.document.referrer, + referrer: viewer.getReferrerUrl(), canonicalUrl: docInfo.canonicalUrl, pageViewId: docInfo.pageViewId, clientId: element.getAttribute('ampcid'), diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index fac189e24786..087f0f37495c 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -19,7 +19,7 @@ import {assert} from '../asserts'; import {documentStateFor} from '../document-state'; import {getService} from '../service'; import {log} from '../log'; -import {parseQueryString, removeFragment} from '../url'; +import {parseQueryString, parseUrl, removeFragment} from '../url'; import {platform} from '../platform'; @@ -74,6 +74,20 @@ export const VisibilityState = { }; +/** + * These domains are trusted with more sensitive viewer operations such as + * propagating the referrer. If you believe your domain should be here, + * file the issue on GitHub to discuss. The process will be similar + * (but somewhat more stringent) to the one described in the [3p/README.md]( + * https://github.com/ampproject/amphtml/blob/master/3p/README.md) + * + * @export {!Array} + */ +export const TRUSTED_VIEWER_HOSTS = [ + /^(.*\.)?(google)(\.com?)?(\.[a-z]{2})?$/ +]; + + /** * An AMP representation of the Viewer. This class doesn't do any work itself * but instead delegates everything to the actual viewer. This class and the @@ -215,6 +229,9 @@ export class Viewer { // Wait for document to become visible. this.docState_.onVisibilityChanged(this.onVisibilityChange_.bind(this)); + /** @const @private {boolean} */ + this.isTrustedViewer_ = this.calcTrustedViewer_(); + // Remove hash - no reason to keep it around, but only when embedded. if (this.isEmbedded_) { const newUrl = removeFragment(this.win.location.href); @@ -391,6 +408,82 @@ export class Viewer { return this.paddingTop_; } + /** + * Returns the "referrer" URL that can be optionally customized by + * the viewer. + * @return {string} + */ + getReferrerUrl() { + // If not embedded in a IFrame always return the document's referrer. + if (!this.isEmbedded()) { + return this.win.document.referrer; + } + + // Viewer override, but only for whitelisted viewers. + if (this.params_['referrer'] && this.isTrustedViewer()) { + return this.params_['referrer']; + } + + // Document's referrer. + if (this.win.document.referrer) { + return this.win.document.referrer; + } + + // Ancestor in the embedded case. + const ancestorOrigins = this.win.location.ancestorOrigins; + if (ancestorOrigins && ancestorOrigins.length > 0) { + return ancestorOrigins[0]; + } + + // No referrer found. + return ''; + } + + /** + * Whether the viewer has been whitelisted for more sensitive operations + * such as customizing referrer. + * @return {boolean} + */ + isTrustedViewer() { + return this.isTrustedViewer_; + } + + /** + * @return {boolean} + */ + calcTrustedViewer_() { + // This only applies to the documents embedded as iframes. + if (!this.isEmbedded_) { + return false; + } + + // Ancestors when available take precedence. This is the main API used + // for this determination. Fallback is only done when this API is not + // supported by the browser. + const ancestorOrigins = this.win.location.ancestorOrigins; + if (ancestorOrigins) { + return (ancestorOrigins.length > 0 && + this.isTrustedViewerOrigin_(ancestorOrigins[0])); + } + + // Fallback to the referrer if ancestorOrigins is not available. + return this.isTrustedViewerOrigin_(this.win.document.referrer); + } + + /** + * @param {string} urlString + * @return {boolean} + * @private + */ + isTrustedViewerOrigin_(urlString) { + const url = parseUrl(urlString); + if (url.protocol != 'https:') { + // Non-https origins are never trusted. + return false; + } + return TRUSTED_VIEWER_HOSTS.some(th => th.test(url.hostname)); + } + /** * Adds a "visibilitychange" event listener for viewer events. The * callback can check {@link isVisible} and {@link getPrefetchCount} diff --git a/src/url-replacements.js b/src/url-replacements.js index b8db2527065a..4ae1a4058ff2 100644 --- a/src/url-replacements.js +++ b/src/url-replacements.js @@ -21,6 +21,7 @@ import {getService} from './service'; import {loadPromise} from './event-helper'; import {log} from './log'; import {parseUrl, removeFragment} from './url'; +import {viewerFor} from './viewer'; import {viewportFor} from './viewport'; import {vsyncFor} from './vsync'; import {userNotificationManagerFor} from './user-notification'; @@ -68,7 +69,7 @@ class UrlReplacements { // Returns the referrer URL. this.set_('DOCUMENT_REFERRER', () => { - return this.win_.document.referrer; + return viewerFor(this.win_).getReferrerUrl(); }); // Returns the title of this AMP document. diff --git a/test/functional/test-3p-frame.js b/test/functional/test-3p-frame.js index 73849d826939..03b7ade5f028 100644 --- a/test/functional/test-3p-frame.js +++ b/test/functional/test-3p-frame.js @@ -21,9 +21,14 @@ import {documentInfoFor} from '../../src/document-info'; import {loadPromise} from '../../src/event-helper'; import {setModeForTesting} from '../../src/mode'; import {resetServiceForTesting} from '../../src/service'; +import {viewerFor} from '../../src/viewer'; describe('3p-frame', () => { + beforeEach(() => { + sandbox = sinon.sandbox.create(); + }); + afterEach(() => { resetServiceForTesting(window, 'bootstrapBaseUrl'); setModeForTesting(null); @@ -32,6 +37,8 @@ describe('3p-frame', () => { if (m) { m.parentElement.removeChild(m); } + sandbox.restore(); + sandbox = null; }); function addCustomBootstrap(url) { @@ -85,10 +92,14 @@ describe('3p-frame', () => { }; }; + const viewer = viewerFor(window); + const viewerMock = sandbox.mock(viewer); + viewerMock.expects('getReferrerUrl') + .returns('http://acme.org/') + .once(); + const iframe = getIframe(window, div, '_ping_'); const src = iframe.src; - const referrer = window.document.referrer; - expect(referrer).to.not.be.empty; const locationHref = location.href; expect(locationHref).to.not.be.empty; const docInfo = documentInfoFor(window); @@ -96,7 +107,7 @@ describe('3p-frame', () => { const fragment = '#{"testAttr":"value","ping":"pong","width":50,"height":100,' + '"initialWindowWidth":100,"initialWindowHeight":200,"type":"_ping_"' + - ',"_context":{"referrer":"' + referrer + '",' + + ',"_context":{"referrer":"http://acme.org/",' + '"canonicalUrl":"https://foo.bar/baz",' + '"pageViewId":"' + docInfo.pageViewId + '","clientId":"cidValue",' + '"location":{"href":"' + locationHref + '"},"tagName":"MY-ELEMENT",' + @@ -120,7 +131,7 @@ describe('3p-frame', () => { expect(win.context.location.originValidated).to.be.false; } expect(win.context.pageViewId).to.equal(docInfo.pageViewId); - expect(win.context.referrer).to.equal(referrer); + expect(win.context.referrer).to.equal('http://acme.org/'); expect(win.context.data.testAttr).to.equal('value'); expect(win.context.noContentAvailable).to.be.a('function'); expect(win.context.observeIntersection).to.be.a('function'); diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index f59e1c431345..9cce48c30b89 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -30,7 +30,16 @@ describe('Viewer', () => { const WindowApi = function() {}; WindowApi.prototype.setTimeout = function() {}; windowApi = new WindowApi(); - windowApi.location = {hash: '', href: '/test/viewer'}; + windowApi.location = { + hash: '', + href: '/test/viewer', + ancestorOrigins: null, + }; + windowApi.document = { + referrer: '', + body: {style: {}}, + documentElement: {style: {}}, + }; windowMock = sandbox.mock(windowApi); viewer = new Viewer(windowApi); }); @@ -55,7 +64,6 @@ describe('Viewer', () => { windowApi.name = '__AMP__viewportType=virtual&width=222&height=333' + '&scrollTop=15'; windowApi.location.hash = '#width=111&paddingTop=17&other=something'; - windowApi.document = {body: {style: {}}}; const viewer = new Viewer(windowApi); expect(viewer.getViewportType()).to.equal('virtual'); expect(viewer.getViewportWidth()).to.equal(111); @@ -86,9 +94,6 @@ describe('Viewer', () => { it('should configure correctly for iOS embedding', () => { windowApi.name = '__AMP__viewportType=natural'; windowApi.parent = {}; - const body = {style: {}}; - const documentElement = {style: {}}; - windowApi.document = {body: body, documentElement: documentElement}; sandbox.mock(platform).expects('isIos').returns(true).once(); const viewer = new Viewer(windowApi); @@ -98,9 +103,6 @@ describe('Viewer', () => { it('should NOT configure for iOS embedding if not embedded', () => { windowApi.name = '__AMP__viewportType=natural'; windowApi.parent = windowApi; - const body = {style: {}}; - const documentElement = {style: {}}; - windowApi.document = {body: body, documentElement: documentElement}; sandbox.mock(platform).expects('isIos').returns(true).once(); expect(new Viewer(windowApi).getViewportType()).to.equal('natural'); @@ -211,4 +213,144 @@ describe('Viewer', () => { expect(delivered[1].eventType).to.equal('documentResized'); expect(delivered[1].data.width).to.equal(13); }); + + describe('isTrustedViewer', () => { + function test(referrer, toBeTrusted) { + windowApi.parent = {}; + + windowApi.location.ancestorOrigins = referrer ? [referrer] : []; + expect(new Viewer(windowApi).isTrustedViewer()) + .to.equal(toBeTrusted, 'by ancestor'); + windowApi.location.ancestorOrigins = null; + + windowApi.document.referrer = referrer; + expect(new Viewer(windowApi).isTrustedViewer()) + .to.equal(toBeTrusted, 'by referrer'); + windowApi.document.referrer = ''; + } + + it('should verify ancestorOrigins ahead of referrer', () => { + windowApi.parent = {}; + windowApi.document.referrer = 'https://google.com'; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(true); + + windowApi.location.ancestorOrigins = ['https://google.com']; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(true); + + windowApi.location.ancestorOrigins = ['https://google.other.com']; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(false); + + // Even empty ancestorOrigins takes precendence. + windowApi.location.ancestorOrigins = []; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(false); + }); + + it('should only consider viewer trusted when iframed', () => { + windowApi.parent = {}; + windowApi.location.ancestorOrigins = ['https://google.com']; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(true); + + windowApi.parent = windowApi; + expect(new Viewer(windowApi).isTrustedViewer()).to.equal(false); + }); + + it('should flag viewer as not trusted without referrer or ancestor', () => { + test('', false); + }); + + it('should trust host as referrer with https', () => { + test('https://google.com', true); + }); + + it('should not trust host as referrer with http', () => { + test('http://google.com', false); + }); + + it('should trust domain variations', () => { + test('https://google.com', true); + test('https://www.google.com', true); + test('https://news.google.com', true); + test('https://www.google.co.uk', true); + test('https://www.google.co.au', true); + test('https://news.google.co.uk', true); + test('https://news.google.co.au', true); + test('https://google.de', true); + test('https://www.google.de', true); + test('https://news.google.de', true); + }); + + it('should NOT trust wrong or non-whitelisted domain variations', () => { + test('https://google.net', false); + test('https://google.other.com', false); + test('https://www.google.other.com', false); + test('https://withgoogle.com', false); + test('https://acme.com', false); + }); + }); + + describe('referrer', () => { + it('should return overridden trusted viewer referrer by ancestor', () => { + windowApi.parent = {}; + windowApi.location.hash = '#referrer=' + + encodeURIComponent('https://acme.org/viewer'); + windowApi.location.ancestorOrigins = [ + 'https://google.com' + ]; + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/viewer'); + }); + + it('should return overridden trusted viewer referrer by referrer', () => { + windowApi.parent = {}; + windowApi.location.hash = '#referrer=' + + encodeURIComponent('https://acme.org/viewer'); + windowApi.document.referrer = 'https://google.com/'; + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/viewer'); + }); + + it('should return document referrer if no viewer referrer', () => { + windowApi.parent = {}; + windowApi.location.hash = '#'; + windowApi.document.referrer = 'https://acme.org/docref'; + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/docref'); + }); + + it('should return document referrer if not embedded', () => { + windowApi.parent = windowApi; // Top window. + windowApi.document.referrer = 'https://acme.org/docref'; + windowApi.location.hash = '#referrer=' + + encodeURIComponent('https://acme.org/viewer'); + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/docref'); + }); + + it('should return doc referrer if not embedded by trusted viewer', () => { + windowApi.parent = {}; + windowApi.document.referrer = 'https://acme.org/docref'; + windowApi.location.hash = '#referrer=' + + encodeURIComponent('https://acme.org/viewer'); + windowApi.location.ancestorOrigins = ['https://acme.org/viewer']; + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/docref'); + }); + + it('should return ancestor origin no viewer/doc referrer', () => { + windowApi.parent = {}; + windowApi.location.hash = '#'; + windowApi.location.ancestorOrigins = [ + 'https://acme.org/ancestor1', + 'https://acme.org/ancestor2' + ]; + expect(new Viewer(windowApi).getReferrerUrl()) + .to.equal('https://acme.org/ancestor1'); + }); + + it('should return empty string if nothing matches', () => { + windowApi.parent = {}; + windowApi.location.hash = '#'; + expect(new Viewer(windowApi).getReferrerUrl()).to.be.equal(''); + }); + }); }); From bb4faca147a28e26522f181248b3f3b7c7d7abd0 Mon Sep 17 00:00:00 2001 From: dotandads Date: Mon, 1 Feb 2016 17:49:48 +0100 Subject: [PATCH 06/44] added implementation for dotandads type for amp-ad added implementation for dotandads type for amp-ad fix merge merge fix fix merge fix merge --- 3p/integration.js | 2 ++ ads/_config.js | 2 ++ ads/dotandads.js | 26 ++++++++++++++++++++++ ads/dotandads.md | 51 +++++++++++++++++++++++++++++++++++++++++++ examples/ads.amp.html | 19 +++++++++++++++- 5 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 ads/dotandads.js create mode 100644 ads/dotandads.md diff --git a/3p/integration.js b/3p/integration.js index ab39988915d0..70a6dd951861 100644 --- a/3p/integration.js +++ b/3p/integration.js @@ -29,6 +29,7 @@ import {adsense} from '../ads/adsense'; import {adtech} from '../ads/adtech'; import {plista} from '../ads/plista'; import {doubleclick} from '../ads/doubleclick'; +import {dotandads} from '../ads/dotandads'; import {facebook} from './facebook'; import {manageWin} from './environment'; import {nonSensitiveDataPostMessage, listenParent} from './messaging'; @@ -53,6 +54,7 @@ register('adtech', adtech); register('plista', plista); register('doubleclick', doubleclick); register('taboola', taboola); +register('dotandads', dotandads); register('_ping_', function(win, data) { win.document.getElementById('c').textContent = data.ping; }); diff --git a/ads/_config.js b/ads/_config.js index 5fa606c3c567..83305495839d 100644 --- a/ads/_config.js +++ b/ads/_config.js @@ -25,6 +25,7 @@ export const adPrefetch = { doubleclick: 'https://www.googletagservices.com/tag/js/gpt.js', a9: 'https://c.amazon-adsystem.com/aax2/assoc.js', adsense: 'https://pagead2.googlesyndication.com/pagead/js/adsbygoogle.js', + dotandads: 'https://amp.ad.dotandad.com/dotandadsAmp.js', }; /** @@ -43,6 +44,7 @@ export const adPreconnect = { 'https://securepubads.g.doubleclick.net', 'https://tpc.googlesyndication.com', ], + dotandads: 'https://bal.ad.dotandad.com', }; /** diff --git a/ads/dotandads.js b/ads/dotandads.js new file mode 100644 index 000000000000..e54b01a9c682 --- /dev/null +++ b/ads/dotandads.js @@ -0,0 +1,26 @@ +/** + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {writeScript} from '../src/3p'; + +/** + * @param {!Window} global + * @param {!Object} data + */ +export function dotandads(global, data) { + global.data = data; + writeScript(global, 'https://amp.ad.dotandad.com/dotandadsAmp.js'); +} diff --git a/ads/dotandads.md b/ads/dotandads.md new file mode 100644 index 000000000000..43b80ded8240 --- /dev/null +++ b/ads/dotandads.md @@ -0,0 +1,51 @@ + + +# DotAndAds + +## Example + +300x250 box: +```html + + +``` + +980x250 masthead: +```html + + +``` + +## Configuration + +For semantics of configuration, please see ad network documentation. + +Supported parameters: + +- sp: sizepos (the ad size and position code) +- mpo: multipoint (an extraction parameter based on site) +- mpt: mediapoint tag (the box where the ad will be shown) +- cid: customer id \ No newline at end of file diff --git a/examples/ads.amp.html b/examples/ads.amp.html index 618545f6c7e0..ee046886d9d3 100644 --- a/examples/ads.amp.html +++ b/examples/ads.amp.html @@ -142,6 +142,23 @@

Taboola responsive widget

data-placement="Ads Example" data-article="auto"> - + +

DotAndAds masthead

+ + + +

DotAndAds 300x250 box

+ + From 6e45dc56d3f63f8483ee6b37e951428bae8a94fa Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 3 Feb 2016 07:46:22 -0800 Subject: [PATCH 07/44] Update list of supported ad networks --- builtins/amp-ad.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/builtins/amp-ad.md b/builtins/amp-ad.md index 0f387daa9a02..f9237fbce2c2 100644 --- a/builtins/amp-ad.md +++ b/builtins/amp-ad.md @@ -39,6 +39,16 @@ resources in AMP. It requires a `type` argument that select what ad network is d ``` +#### Supported ad networks + +- [A9](../ads/a9.md) +- [AdReactor](../ads/adreactor.md) +- [AdSense](../ads/adsense.md) +- [AdTech](../ads/adtech.md) +- [Dot and Media](../ads/dotandads.md) +- [Doubleclick](../ads/doubleclick.md) +- [plista](../ads/plista.md) + #### Styling `` elements may not themselves have or be placed in containers that have CSS `position: fixed` set (with the exception of `amp-lightbox`). @@ -87,15 +97,6 @@ Optionally `amp-ad` supports a child element with the `placeholder` attribute. I - If there is no fallback element available, the amp-ad tag will be collapsed (set to display: none) if the ad sends a message that the ad slot cannot be filled and AMP determines that this operation can be performed without affecting the user's scroll position. -#### Supported ad networks - -- [A9](../ads/a9.md) -- [AdReactor](../ads/adreactor.md) -- [AdSense](../ads/adsense.md) -- [AdTech](../ads/adtech.md) -- [Doubleclick](../ads/doubleclick.md) - - #### Running ads from a custom domain AMP supports loading the bootstrap iframe that is used to load ads from a custom domain such as your own domain. From ade253b95b7dc998ccbcb71854494bfa968b6292 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 3 Feb 2016 07:47:37 -0800 Subject: [PATCH 08/44] Reference taboola docs from amp-embed page. --- builtins/amp-embed.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/builtins/amp-embed.md b/builtins/amp-embed.md index 3e75f6a3998e..494b03667a52 100644 --- a/builtins/amp-embed.md +++ b/builtins/amp-embed.md @@ -19,7 +19,7 @@ The `amp-embed` element is used to allow embedding elements in to the AMP page. #### Implementation -The `` is actually an alias to the [``](amp-ad.md) tag, deriving all of it's functionality with a different tag name. +The `` is actually an alias to the [``](amp-ad.md) tag, deriving all of it's functionality with a different tag name. Can be used instead of `` when that would be semantically more accurate. ```html @@ -30,4 +30,8 @@ Can be used instead of `` when that would be semantically more accurate. data-article=auto data-placement="Below Article Thumbnails"> -``` \ No newline at end of file +``` + +#### Supported embed types + +- [Taboola](../ads/taboola.md) From fb5469784a331b610725347a40793733399995db Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Wed, 3 Feb 2016 10:00:34 -0800 Subject: [PATCH 09/44] Enable custom events in analytics --- extensions/amp-analytics/0.1/amp-analytics.js | 3 ++- .../amp-analytics/0.1/instrumentation.js | 23 ++++++++++++++++++- .../0.1/test/test-instrumentation.js | 18 +++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 60511eee2126..b2e7b9af816a 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -15,7 +15,7 @@ */ import {ANALYTICS_CONFIG} from './vendors'; -import {addListener} from './instrumentation'; +import {addListener, instrumentationServiceFor} from './instrumentation'; import {assertHttpsUrl} from '../../../src/url'; import {expandTemplate} from '../../../src/string'; import {installCidService} from '../../../src/service/cid-impl'; @@ -30,6 +30,7 @@ import {xhrFor} from '../../../src/xhr'; installCidService(AMP.win); installStorageService(AMP.win); +instrumentationServiceFor(AMP.win); export class AmpAnalytics extends AMP.BaseElement { diff --git a/extensions/amp-analytics/0.1/instrumentation.js b/extensions/amp-analytics/0.1/instrumentation.js index 50130978a872..28c49c549354 100644 --- a/extensions/amp-analytics/0.1/instrumentation.js +++ b/extensions/amp-analytics/0.1/instrumentation.js @@ -80,8 +80,11 @@ class InstrumentationService { /** @private {boolean} */ this.clickHandlerRegistered_ = false; - /** @private {!Observable} */ + /** @private {!Observable} */ this.clickObservable_ = new Observable(); + + /** @private {!Object>} */ + this.observers_ = {}; } /** @@ -111,6 +114,24 @@ class InstrumentationService { this.clickObservable_.add( this.createSelectiveListener_(listener, opt_selector)); } + } else { + let observers = this.observers_[eventType]; + if (!observers) { + observers = new Observable(); + this.observers_[eventType] = observers; + } + observers.add(listener); + } + } + + /** + * Triggers the analytics event with the specified type. + * @param {string} eventType + */ + triggerEvent(eventType) { + const observers = this.observers_[eventType]; + if (observers) { + observers.fire(new AnalyticsEvent(eventType)); } } diff --git a/extensions/amp-analytics/0.1/test/test-instrumentation.js b/extensions/amp-analytics/0.1/test/test-instrumentation.js index 6d9e0d6595ed..0ee15dde6284 100644 --- a/extensions/amp-analytics/0.1/test/test-instrumentation.js +++ b/extensions/amp-analytics/0.1/test/test-instrumentation.js @@ -87,4 +87,22 @@ describe('instrumentation', function() { expect(fnIdY.callCount).to.equal(1); }); + it('should listen on custom events', () => { + const handler1 = sinon.spy(); + const handler2 = sinon.spy(); + ins.addListener('custom-event-1', handler1); + ins.addListener('custom-event-2', handler2); + + ins.triggerEvent('custom-event-1'); + expect(handler1.callCount).to.equal(1); + expect(handler2.callCount).to.equal(0); + + ins.triggerEvent('custom-event-2'); + expect(handler1.callCount).to.equal(1); + expect(handler2.callCount).to.equal(1); + + ins.triggerEvent('custom-event-1'); + expect(handler1.callCount).to.equal(2); + expect(handler2.callCount).to.equal(1); + }); }); From 31331bc98c893325f5d5ed86bc7c25224fe39974 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 2 Feb 2016 12:39:16 -0800 Subject: [PATCH 10/44] Tag error reports coming from the cache over the general internet. Sets both severity and ads "-cdn" to the "version" for being able to filter in the error reporting tool. Also fixes an error where apparently some elements miss the .classList property in Safari found via error reporting. --- src/error.js | 2 +- tools/errortracker/app.yaml | 2 +- tools/errortracker/errortracker.go | 13 ++++++++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/error.js b/src/error.js index ebe933ff3acd..c7622a7e635f 100644 --- a/src/error.js +++ b/src/error.js @@ -41,7 +41,7 @@ export function reportError(error, opt_associatedElement) { } error.reported = true; const element = opt_associatedElement || error.associatedElement; - if (element) { + if (element && element.classList) { element.classList.add('-amp-error'); if (getMode().development) { element.classList.add('-amp-element-error'); diff --git a/tools/errortracker/app.yaml b/tools/errortracker/app.yaml index 894a834ef9ae..3a46342cb057 100644 --- a/tools/errortracker/app.yaml +++ b/tools/errortracker/app.yaml @@ -1,5 +1,5 @@ application: amp-error-reporting -version: 2 +version: 4 runtime: go api_version: go1 diff --git a/tools/errortracker/errortracker.go b/tools/errortracker/errortracker.go index 6c8e70230730..fc5954055adf 100644 --- a/tools/errortracker/errortracker.go +++ b/tools/errortracker/errortracker.go @@ -63,6 +63,7 @@ type ErrorEvent struct { Line int32 `json:"line,omitempty"` Classname string `json:"classname,omitempty"` Function string `json:"function,omitempty"` + Severity string `json:"severity,omitempty"` } func init() { @@ -104,7 +105,14 @@ func handle(w http.ResponseWriter, r *http.Request) { if r.URL.Query().Get("a") == "1" { errorType = "assert" } + // By default we log as "INFO" severity, because reports are very spammy + severity := "INFO" + level := logging.Info + // But if the request comes from the cache (and thus only from valid AMP + // docs) we log as "ERROR". if strings.HasPrefix(r.Referer(), "https://cdn.ampproject.org/") { + severity = "ERROR" + level = logging.Error errorType += "-cdn" } @@ -118,6 +126,7 @@ func handle(w http.ResponseWriter, r *http.Request) { Filename: r.URL.Query().Get("f"), Line: int32(line), Classname: r.URL.Query().Get("el"), + Severity: severity, } if event.Message == "" && event.Exception == "" { @@ -146,6 +155,7 @@ func handle(w http.ResponseWriter, r *http.Request) { err = logc.LogSync(logging.Entry{ Time: time.Now().UTC(), Payload: event, + Level: level, }) if err != nil { @@ -160,7 +170,8 @@ func handle(w http.ResponseWriter, r *http.Request) { if r.URL.Query().Get("debug") == "1" { w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusOK) - fmt.Fprintln(w, "OK") + fmt.Fprintln(w, "OK\n"); + fmt.Fprintln(w, event); } else { w.WriteHeader(http.StatusNoContent) } From 7bfca6af4796dd9a8dec97fcb5ec95741053e6ab Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Wed, 3 Feb 2016 13:49:21 -0800 Subject: [PATCH 11/44] Viewer handshake with origin validation --- examples/viewer-integr-messaging.js | 17 +++++-- examples/viewer-integr.js | 34 +++++++++++--- examples/viewer.html | 72 +++++++++++++++++++++++++---- src/service/viewer-impl.js | 9 +++- test/functional/test-viewer.js | 2 +- 5 files changed, 111 insertions(+), 23 deletions(-) diff --git a/examples/viewer-integr-messaging.js b/examples/viewer-integr-messaging.js index 44505bfd291a..9445e6efefc5 100644 --- a/examples/viewer-integr-messaging.js +++ b/examples/viewer-integr-messaging.js @@ -18,11 +18,12 @@ /** * This is a very simple messaging protocol between viewer and viewer client. * @param {!Window} target + * @param {string} targetOrigin * @param {function(string, *, boolean):(!Promise<*>|undefined)} * requestProcessor * @constructor */ -function ViewerMessaging(target, requestProcessor) { +function ViewerMessaging(target, targetOrigin, requestProcessor) { this.sentinel_ = '__AMP__'; this.requestSentinel_ = this.sentinel_ + 'REQUEST'; this.responseSentinel_ = this.sentinel_ + 'RESPONSE'; @@ -30,9 +31,17 @@ function ViewerMessaging(target, requestProcessor) { this.requestIdCounter_ = 0; this.waitingForResponse_ = {}; + /** @const @private {!Widnow} */ this.target_ = target; + /** @const @private {string} */ + this.targetOrigin_ = targetOrigin; + /** @const @private {function(string, *, boolean):(!Promise<*>|undefined)} */ this.requestProcessor_ = requestProcessor; + if (!this.targetOrigin_) { + throw new Error('Target origin must be specified'); + } + window.addEventListener('message', this.onMessage_.bind(this), false); } @@ -65,10 +74,9 @@ ViewerMessaging.prototype.sendRequest = function(eventType, payload, * @private */ ViewerMessaging.prototype.onMessage_ = function(event) { - if (event.source != this.target_) { + if (event.source != this.target_ && event.origin != this.targetOrigin_) { return; } - // TODO: must check for origin/target. var message = event.data; if (message.sentinel == this.requestSentinel_) { this.onRequest_(message); @@ -128,7 +136,6 @@ ViewerMessaging.prototype.onResponse_ = function(message) { */ ViewerMessaging.prototype.sendMessage_ = function(sentinel, requestId, eventType, payload, awaitResponse) { - // TODO: must check for origin/target. var message = { sentinel: sentinel, requestId: requestId, @@ -136,7 +143,7 @@ ViewerMessaging.prototype.sendMessage_ = function(sentinel, requestId, payload: payload, rsvp: awaitResponse }; - this.target_./*TODO-REVIEW*/postMessage(message, '*'); + this.target_./*OK*/postMessage(message, this.targetOrigin_); }; diff --git a/examples/viewer-integr.js b/examples/viewer-integr.js index 8c4202790710..b40bf16ccaca 100644 --- a/examples/viewer-integr.js +++ b/examples/viewer-integr.js @@ -42,13 +42,35 @@ function whenMessagingLoaded(callback) { var viewer = AMP.viewer; if (window.parent && window.parent != window) { + var handshakePromise = new Promise(function(resolve) { + var unconfirmedViewerOrigin = viewer.getParam('viewerorigin'); + if (!unconfirmedViewerOrigin) { + throw new Error('Expected viewer origin must be specified!'); + } + + var listener = function(event) { + if (event.origin == unconfirmedViewerOrigin && + event.data == 'amp-handshake-response' && + (!event.source || event.source == window.parent)) { + window.removeEventListener('message', listener, false); + resolve(event.origin); + } + }; + window.addEventListener('message', listener, false); + + window.parent./*OK*/postMessage('amp-handshake-request', + unconfirmedViewerOrigin); + }); + whenMessagingLoaded(function(ViewerMessaging) { - var messaging = new ViewerMessaging(window.parent, - function(type, payload, awaitResponse) { - return viewer.receiveMessage(type, payload, awaitResponse); - }); - viewer.setMessageDeliverer(function(type, payload, awaitResponse) { - return messaging.sendRequest(type, payload, awaitResponse); + handshakePromise.then(function(viewerOrigin) { + var messaging = new ViewerMessaging(window.parent, viewerOrigin, + function(type, payload, awaitResponse) { + return viewer.receiveMessage(type, payload, awaitResponse); + }); + viewer.setMessageDeliverer(function(type, payload, awaitResponse) { + return messaging.sendRequest(type, payload, awaitResponse); + }, viewerOrigin); }); }); } diff --git a/examples/viewer.html b/examples/viewer.html index 9d50b1153a2c..07541266ebfc 100644 --- a/examples/viewer.html +++ b/examples/viewer.html @@ -193,14 +193,18 @@ height: this.container./*OK*/offsetHeight, paddingTop: this.header./*OK*/offsetHeight, visibilityState: this.visibilityState_, - prerenderSize: this.prerenderSize_ + prerenderSize: this.prerenderSize_, + viewerorigin: parseUrl(window.location.href).origin }; log('Params:' + JSON.stringify(params)); - var url = './article-access.amp.max.html#' + paramsStr(params); + var inputUrl = './article-access.amp.max.html#' + paramsStr(params); if (window.location.hash && window.location.hash.length > 1) { - url += '&' + window.location.hash.substring(1); + inputUrl += '&' + window.location.hash.substring(1); } + var parsedUrl = parseUrl(inputUrl); + var url = parsedUrl.href; + this.frameOrigin_ = parsedUrl.origin; log('AMP URL = ', url); this.iframe.style.display = 'none'; this.container.appendChild(this.iframe); @@ -208,8 +212,7 @@ // Notice that name can only be set once the IFrame is in the DOM. this.iframe.contentWindow.name = SENTINEL + paramsStr(params); - this.messaging_ = new ViewerMessaging(this.iframe.contentWindow, - this.processRequest_.bind(this)); + this.awaitHandshake_(); this.iframe.onload = this.loaded_.bind(this); setTimeout(function() { @@ -223,10 +226,35 @@ }; + Viewer.prototype.awaitHandshake_ = function() { + var target = this.iframe.contentWindow; + var targetOrigin = this.frameOrigin_; + var listener = function(event) { + if (event.origin == targetOrigin && + event.data == 'amp-handshake-request' && + (!event.source || event.source == target)) { + log('Viewer ' + this.id + ' messaging established with ', + targetOrigin); + window.removeEventListener('message', listener, false); + + target./*OK*/postMessage('amp-handshake-response', targetOrigin); + + this.messaging_ = new ViewerMessaging(target, targetOrigin, + this.processRequest_.bind(this)); + this.sendRequest_('visibilitychange', { + state: this.visibilityState_, + prerenderSize: this.prerenderSize + }, true); + } + }.bind(this); + window.addEventListener('message', listener, false); + }; + + Viewer.prototype.toggleVisibility = function(visible) { log('Viewer ' + this.id + ' visibility -> ', visible); this.visibilityState_ = visible ? 'visible' : 'hidden'; - this.messaging_.sendRequest('visibilitychange', { + this.sendRequest_('visibilitychange', { state: this.visibilityState_, prerenderSize: this.prerenderSize }, true); @@ -271,7 +299,7 @@ Viewer.prototype.onScroll_ = function() { - this.messaging_.sendRequest('viewport', { + this.sendRequest_('viewport', { scrollTop: this.container./*OK*/scrollTop }, false); }; @@ -282,7 +310,7 @@ this.container./*OK*/offsetHeight, this.header./*OK*/offsetHeight, this.container./*OK*/scrollTop); - this.messaging_.sendRequest('viewport', { + this.sendRequest_('viewport', { scrollTop: this.container./*OK*/scrollTop, width: this.container./*OK*/offsetWidth, height: this.container./*OK*/offsetHeight, @@ -331,7 +359,7 @@ // Even more trivial. Always assumes that only one step was popped in // history. this.stackIndex_--; - this.messaging_.sendRequest('historyPopped', { + this.sendRequest_('historyPopped', { newStackIndex: this.stackIndex_ }, false); log('history popped to ', this.stackIndex_); @@ -380,6 +408,14 @@ }; + Viewer.prototype.sendRequest_ = function(type, data, awaitResponse) { + if (!this.messaging_) { + return; + } + return this.messaging_.sendRequest(type, data, awaitResponse); + }; + + function log() { var var_args = Array.prototype.slice.call(arguments, 0); var_args.unshift('[VIEWER]'); @@ -387,6 +423,23 @@ } + function parseUrl(urlString) { + var a = document.createElement('a'); + a.href = urlString; + return { + href: a.href, + protocol: a.protocol, + host: a.host, + hostname: a.hostname, + port: a.port == '0' ? '' : a.port, + pathname: a.pathname, + search: a.search, + hash: a.hash, + origin: a.protocol + '//' + a.host + }; + } + + function paramsStr(params) { var s = ''; for (var k in params) { @@ -413,7 +466,6 @@ new Viewer('container3', '3', false).start(); addShowContainer('3'); - showContainer('1'); } diff --git a/src/service/viewer-impl.js b/src/service/viewer-impl.js index 087f0f37495c..ad4b895c407e 100644 --- a/src/service/viewer-impl.js +++ b/src/service/viewer-impl.js @@ -154,6 +154,9 @@ export class Viewer { /** @private {?function(string, *, boolean):(Promise<*>|undefined)} */ this.messageDeliverer_ = null; + /** @private {?string} */ + this.messagingOrigin_ = null; + /** @private {!Array} */ this.messageQueue_ = []; @@ -647,11 +650,15 @@ export class Viewer { * Provides a message delivery mechanism by which AMP document can send * messages to the viewer. * @param {function(string, *, boolean):(!Promise<*>|undefined)} deliverer + * @param {string} origin * @export */ - setMessageDeliverer(deliverer) { + setMessageDeliverer(deliverer, origin) { assert(!this.messageDeliverer_, 'message deliverer can only be set once'); + log.fine(TAG_, 'message channel established with origin: ', origin); this.messageDeliverer_ = deliverer; + // TODO(dvoytenko, #1764): Make `origin` required when viewers catch up. + this.messagingOrigin_ = origin; if (this.messageQueue_.length > 0) { const queue = this.messageQueue_.slice(0); this.messageQueue_ = []; diff --git a/test/functional/test-viewer.js b/test/functional/test-viewer.js index 9cce48c30b89..dfd7802f779d 100644 --- a/test/functional/test-viewer.js +++ b/test/functional/test-viewer.js @@ -204,7 +204,7 @@ describe('Viewer', () => { const delivered = []; viewer.setMessageDeliverer((eventType, data) => { delivered.push({eventType: eventType, data: data}); - }); + }, 'https://acme.com'); expect(viewer.messageQueue_.length).to.equal(0); expect(delivered.length).to.equal(2); From c8880f45a970c3044a8e6eaf9fae608a6adbb148 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Wed, 3 Feb 2016 17:34:28 -0500 Subject: [PATCH 12/44] Add Code Blocks to AMP Boilerplate Code Docs --- spec/amp-boilerplate.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/amp-boilerplate.md b/spec/amp-boilerplate.md index 8f6660c7d40f..38d3419d853b 100644 --- a/spec/amp-boilerplate.md +++ b/spec/amp-boilerplate.md @@ -15,5 +15,6 @@ limitations under the License. --> # AMP Boilerplate Code - - \ No newline at end of file +``` + +``` From 97ed7f9d6bbef4ac1eb6c483b551acb19865253e Mon Sep 17 00:00:00 2001 From: Sriram Krishnan Date: Wed, 3 Feb 2016 14:47:34 -0800 Subject: [PATCH 13/44] Fix old-boilerplate test file --- test/fixtures/boilerplate-old-opacity.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/boilerplate-old-opacity.html b/test/fixtures/boilerplate-old-opacity.html index f45e104f69ac..3069954dfc79 100644 --- a/test/fixtures/boilerplate-old-opacity.html +++ b/test/fixtures/boilerplate-old-opacity.html @@ -6,7 +6,7 @@ - + From 446b59bd5e50c0946af4cefe453d5078e43ebaa2 Mon Sep 17 00:00:00 2001 From: Johannes Henkel Date: Wed, 3 Feb 2016 18:07:32 -0800 Subject: [PATCH 14/44] Validate CSS recursively. - Adds RuleVisitor to parse_css. - Explores the entire AST for invalid rules (implementing the TODO in the validator code). - Removes some of the blacklist regexes. - Adds a simple test with nesting. --- validator/parse-css.js | 47 ++++++++ .../feature_tests/incorrect_custom_style.html | 3 +- .../feature_tests/incorrect_custom_style.out | 1 + validator/validator.js | 104 +++++++++++------- validator/validator.protoascii | 97 ++++++---------- 5 files changed, 149 insertions(+), 103 deletions(-) diff --git a/validator/parse-css.js b/validator/parse-css.js index 02be67d3dbe2..489f8ac7a646 100644 --- a/validator/parse-css.js +++ b/validator/parse-css.js @@ -25,6 +25,7 @@ goog.provide('parse_css.BlockType'); goog.provide('parse_css.Declaration'); goog.provide('parse_css.QualifiedRule'); goog.provide('parse_css.Rule'); +goog.provide('parse_css.RuleVisitor'); goog.provide('parse_css.Stylesheet'); goog.provide('parse_css.TokenStream'); goog.provide('parse_css.extractAFunction'); @@ -183,6 +184,9 @@ goog.inherits(parse_css.Rule, parse_css.Token); /** @type {parse_css.TokenType} */ parse_css.Rule.tokenType = parse_css.TokenType.UNKNOWN; +/** @param {!parse_css.RuleVisitor} visitor */ +parse_css.Rule.prototype.accept = goog.abstractMethod; + /** * @param {number=} opt_indent * @return {string} @@ -215,6 +219,14 @@ parse_css.Stylesheet.prototype.toJSON = function() { return json; }; +/** @param {!parse_css.RuleVisitor} visitor */ +parse_css.Stylesheet.prototype.accept = function(visitor) { + visitor.visitStylesheet(this); + for (const rule of this.rules) { + rule.accept(visitor); + } +}; + /** * @param {string} name * @constructor @@ -246,6 +258,17 @@ parse_css.AtRule.prototype.toJSON = function() { return json; }; +/** @param {!parse_css.RuleVisitor} visitor */ +parse_css.AtRule.prototype.accept = function(visitor) { + visitor.visitAtRule(this); + for (const rule of this.rules) { + rule.accept(visitor); + } + for (const declaration of this.declarations) { + declaration.accept(visitor); + } +}; + /** * @constructor * @extends {parse_css.Rule} @@ -271,6 +294,14 @@ parse_css.QualifiedRule.prototype.toJSON = function() { return json; }; +/** @param {!parse_css.RuleVisitor} visitor */ +parse_css.QualifiedRule.prototype.accept = function(visitor) { + visitor.visitQualifiedRule(this); + for (const declaration of this.declarations) { + declaration.accept(visitor); + } +}; + /** * @param {string} name * @constructor @@ -299,6 +330,22 @@ parse_css.Declaration.prototype.toJSON = function() { return json; }; +/** @param {!parse_css.RuleVisitor} visitor */ +parse_css.Declaration.prototype.accept = function(visitor) { + visitor.visitDeclaration(this); +}; + +/** @constructor */ +parse_css.RuleVisitor = function() {}; +/** @param {!parse_css.Stylesheet} stylesheet */ +parse_css.RuleVisitor.prototype.visitStylesheet = function(stylesheet) {}; +/** @param {!parse_css.AtRule} atRule */ +parse_css.RuleVisitor.prototype.visitAtRule = function(atRule) {}; +/** @param {!parse_css.QualifiedRule} qualifiedRule */ +parse_css.RuleVisitor.prototype.visitQualifiedRule = function(qualifiedRule) {}; +/** @param {!parse_css.Declaration} declaration */ +parse_css.RuleVisitor.prototype.visitDeclaration = function(declaration) {}; + /** * Enum describing how to parse the rules inside a CSS AT Rule. * @enum {string} diff --git a/validator/testdata/feature_tests/incorrect_custom_style.html b/validator/testdata/feature_tests/incorrect_custom_style.html index 247435fc0433..35f53030cfd4 100644 --- a/validator/testdata/feature_tests/incorrect_custom_style.html +++ b/validator/testdata/feature_tests/incorrect_custom_style.html @@ -30,7 +30,8 @@ foo.i-amp-class {} foo.amp-class {} foo { b: red !important; } - @viewport (mumble mumble) {} + @viewport (mumble mumble) { } + @media (whatever) { @notallowednested } diff --git a/validator/testdata/feature_tests/incorrect_custom_style.out b/validator/testdata/feature_tests/incorrect_custom_style.out index 9048b977e84c..754e91fd50f2 100644 --- a/validator/testdata/feature_tests/incorrect_custom_style.out +++ b/validator/testdata/feature_tests/incorrect_custom_style.out @@ -1,3 +1,4 @@ FAIL feature_tests/incorrect_custom_style.html:29:4 CSS syntax error in tag 'author stylesheet' - saw invalid at rule 'import'. feature_tests/incorrect_custom_style.html:33:4 CSS syntax error in tag 'author stylesheet' - saw invalid at rule 'viewport'. +feature_tests/incorrect_custom_style.html:34:24 CSS syntax error in tag 'author stylesheet' - saw invalid at rule 'notallowednested'. diff --git a/validator/validator.js b/validator/validator.js index eac750a472c4..4e6d72d57b5f 100644 --- a/validator/validator.js +++ b/validator/validator.js @@ -41,6 +41,7 @@ goog.require('goog.structs.Map'); goog.require('goog.structs.Set'); goog.require('goog.uri.utils'); goog.require('parse_css.BlockType'); +goog.require('parse_css.RuleVisitor'); goog.require('parse_css.parseAStylesheet'); goog.require('parse_css.tokenize'); @@ -414,6 +415,64 @@ TagNameStack.prototype.hasAncestor = function(ancestor) { return false; }; + +/** + * Returns true if the given AT rule is considered valid. + * @param {!amp.validator.CssSpec} cssSpec + * @param {string} atRuleName + * @return {boolean} + */ +function isAtRuleValid(cssSpec, atRuleName) { + let defaultType = ''; + + for (const atRuleSpec of cssSpec.atRuleSpec) { + if (atRuleSpec.name === '$DEFAULT') { + defaultType = atRuleSpec.type; + } else if (atRuleSpec.name === atRuleName) { + return atRuleSpec.type !== + amp.validator.AtRuleSpec.BlockType.PARSE_AS_ERROR; + } + } + + goog.asserts.assert(defaultType !== ''); + return defaultType !== amp.validator.AtRuleSpec.BlockType.PARSE_AS_ERROR; +}; + +/** + * @param {!amp.validator.TagSpec} tagSpec + * @param {!amp.validator.CssSpec} cssSpec + * @param {!Context} context + * @param {!amp.validator.ValidationResult} result + * @constructor + * @extends {parse_css.RuleVisitor} + */ +const InvalidAtRuleVisitor = function(tagSpec, cssSpec, context, result) { + goog.base(this); + /** @type {!amp.validator.TagSpec} */ + this.tagSpec = tagSpec; + /** @type {!amp.validator.CssSpec} */ + this.cssSpec = cssSpec; + /** @type {!Context} */ + this.context = context; + /** @type {!amp.validator.ValidationResult} */ + this.result = result; + /** @type {boolean} */ + this.errorsSeen = false; +}; +goog.inherits(InvalidAtRuleVisitor, parse_css.RuleVisitor); + +/** @param {!parse_css.AtRule} atRule */ +InvalidAtRuleVisitor.prototype.visitAtRule = function(atRule) { + if (!isAtRuleValid(this.cssSpec, atRule.name)) { + this.context.addErrorWithLineCol( + new LineCol(atRule.line, atRule.col), + amp.validator.ValidationError.Code.CSS_SYNTAX_INVALID_AT_RULE, + /* params */ [getDetailOrName(this.tagSpec), atRule.name], + /* url */ '', this.result); + this.errorsSeen = true; + } +}; + /** * This matcher maintains a constraint to check which an opening tag * introduces: a tag's cdata matches constraints set by it's cdata @@ -479,28 +538,6 @@ function computeAtRuleDefaultParsingSpec(atRuleParsingSpec) { return ret; } -/** - * Returns true if the given AT rule is considered valid. - * @param {!amp.validator.CssSpec} cssSpec - * @param {string} atRuleName - * @return {boolean} - */ -CdataMatcher.prototype.isAtRuleValid = function(cssSpec, atRuleName) { - let defaultType = ''; - - for (const atRuleSpec of cssSpec.atRuleSpec) { - if (atRuleSpec.name === '$DEFAULT') { - defaultType = atRuleSpec.type; - } else if (atRuleSpec.name === atRuleName) { - return atRuleSpec.type !== - amp.validator.AtRuleSpec.BlockType.PARSE_AS_ERROR; - } - } - - goog.asserts.assert(defaultType !== ''); - return defaultType !== amp.validator.AtRuleSpec.BlockType.PARSE_AS_ERROR; -}; - /** * Matches the provided cdata against what this matcher expects. * @param {string} cdata @@ -571,7 +608,6 @@ CdataMatcher.prototype.match = function(cdata, context, validationResult) { const sheet = parse_css.parseAStylesheet( tokenList, atRuleParsingSpec, computeAtRuleDefaultParsingSpec(atRuleParsingSpec), cssErrors); - let reportCdataRegexpErrors = (cssErrors.length == 0); for (const errorToken of cssErrors) { const lineCol = new LineCol(errorToken.line, errorToken.col); context.addErrorWithLineCol( @@ -579,28 +615,14 @@ CdataMatcher.prototype.match = function(cdata, context, validationResult) { /* params */ [getDetailOrName(this.tagSpec_), errorToken.msg], /* url */ '', validationResult); } - - // TODO(johannes): This needs improvement to validate all fields - // recursively. For now, it's just doing one layer of fields to - // demonstrate the idea and keep tests passing. - for (const cssRule of sheet.rules) { - if (cssRule.tokenType === 'AT_RULE') { - if (!this.isAtRuleValid(cdataSpec.cssSpec, cssRule.name)) { - reportCdataRegexpErrors = false; - const lineCol = new LineCol(cssRule.line, cssRule.col); - context.addErrorWithLineCol( - lineCol, - amp.validator.ValidationError.Code.CSS_SYNTAX_INVALID_AT_RULE, - /* params */ [getDetailOrName(this.tagSpec_), cssRule.name], - /* url */ '', validationResult); - } - } - } + const visitor = new InvalidAtRuleVisitor( + this.tagSpec_, cdataSpec.cssSpec, context, validationResult); + sheet.accept(visitor); // As a hack to not report some errors twice, both via the css parser // and via the regular expressions below, we return early if there // are parser errors and skip the regular expression errors. - if (!reportCdataRegexpErrors) + if (visitor.errorsSeen || cssErrors.length > 0) return; } // } end oneof diff --git a/validator/validator.protoascii b/validator/validator.protoascii index 834fd7e971df..e3c633e96bc4 100644 --- a/validator/validator.protoascii +++ b/validator/validator.protoascii @@ -20,12 +20,12 @@ # in production from crashing. This id is not relevant to validator.js # because thus far, engine (validator.js) and spec file (validator.protoascii) # are always released together. -min_validator_revision_required: 83 +min_validator_revision_required: 85 # The spec file revision allows the validator engine to distinguish # newer versions of the spec file. This is currently a Google internal # mechanism, validator.js does not use this facility. However, any # change to this file requires updating this revision id. -spec_file_revision: 136 +spec_file_revision: 137 # Rules for AMP HTML # (https://github.com/google/amphtml/blob/master/spec/amp-html-format.md). # @@ -458,42 +458,42 @@ tags: { # Special custom 'author' spreadsheet. cdata: { max_bytes: 50000 max_bytes_spec_url: "https://github.com/ampproject/amphtml/blob/master/spec/amp-html-format.md#maximum-size" - css_spec: { - at_rule_spec: { - name: 'media' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: 'keyframes' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: '-webkit-keyframes' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: '-moz-keyframes' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: '-o-keyframes' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: 'supports' - type: PARSE_AS_RULES - } - at_rule_spec: { - name: 'font-face' - type: PARSE_AS_DECLARATIONS - } - at_rule_spec: { - name: '$DEFAULT' # matches if none of the above match - type: PARSE_AS_ERROR - } + css_spec: { + at_rule_spec: { + name: 'media' + type: PARSE_AS_RULES } + at_rule_spec: { + name: 'keyframes' + type: PARSE_AS_RULES + } + at_rule_spec: { + name: '-webkit-keyframes' + type: PARSE_AS_RULES + } + at_rule_spec: { + name: '-moz-keyframes' + type: PARSE_AS_RULES + } + at_rule_spec: { + name: '-o-keyframes' + type: PARSE_AS_RULES + } + at_rule_spec: { + name: 'supports' + type: PARSE_AS_RULES + } + at_rule_spec: { + name: 'font-face' + type: PARSE_AS_DECLARATIONS + } + at_rule_spec: { + name: '$DEFAULT' # matches if none of the above match + type: PARSE_AS_ERROR + } + } # These regex blacklists are temporary hacks to validate essential CSS - # rules. They will be replaced later with more principalled solutions. + # rules. They will be replaced later with more principled solutions. blacklisted_cdata_regex: { regex: "\\.i?-amp-" error_message: "CSS -amp- class name prefix" @@ -502,31 +502,6 @@ tags: { # Special custom 'author' spreadsheet. regex: "!important" error_message: "CSS !important" } - # All known @ rules except for media and font-face are disabled. - blacklisted_cdata_regex: { - regex: "charset" - error_message: "CSS @charset" - } - blacklisted_cdata_regex: { - regex: "@import" - error_message: "CSS @import" - } - blacklisted_cdata_regex: { - regex: "@namespace" - error_message: "CSS @namespace" - } - blacklisted_cdata_regex: { - regex: "@document" - error_message: "CSS @document" - } - blacklisted_cdata_regex: { - regex: "@page" - error_message: "CSS @page" - } - blacklisted_cdata_regex: { - regex: "@viewport" - error_message: "CSS @viewport" - } } spec_url: "https://github.com/ampproject/amphtml/blob/master/spec/amp-html-format.md#stylesheets" } From 0110ebe2d9ed7d93183bb46df8af5a81966c21d3 Mon Sep 17 00:00:00 2001 From: Johannes Henkel Date: Wed, 3 Feb 2016 18:16:46 -0800 Subject: [PATCH 15/44] =?UTF-8?q?Fix=20for=20the=20Turkish=20=C4=B0=20occu?= =?UTF-8?q?rring=20in=20the=20document.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a character which when in a javascript string yields .length == 1, but when toLowerCase()'d, yields .length == 2. htmlparser.js uses .length extensively and gets thrown off, generating incorrect SAX events. The fix detects the case (better than the previous attempt) and only lower cases A-Z instead. @gregable had most of the ideas here, including the final fix. Fixes https://github.com/ampproject/amphtml/issues/1768 --- validator/htmlparser.js | 23 +++++++++--------- validator/htmlparser_test.js | 47 ++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/validator/htmlparser.js b/validator/htmlparser.js index f818220b5373..b1bfc65a15bd 100644 --- a/validator/htmlparser.js +++ b/validator/htmlparser.js @@ -651,25 +651,26 @@ amp.htmlparser.HtmlParser.prototype.normalizeRCData_ = function(rcdata) { /** - * TODO(goto): why isn't this in the string package ? does this solves any - * real problem ? move it to the goog.string package if it does. - * * @param {string} str The string to lower case. * @return {string} The str in lower case format. */ amp.htmlparser.toLowerCase = function(str) { - // The below may not be true on browsers in the Turkish locale. - if ('script' === 'SCRIPT'.toLowerCase()) { - return str.toLowerCase(); - } else { - return str.replace(/[A-Z]/g, function(ch) { - return String.fromCharCode(ch.charCodeAt(0) | 32); - }); + // htmlparser.js heavily relies on the length of the strings, and + // unfortunately some characters change their length when + // lowercased; for instance, the Turkish İ has a length of 1, but + // when lower-cased, it has a length of 2. So, as a workaround we + // check that the length be the same as before lower-casing, and if + // not, we only lower-case the letters A-Z. + const lowerCased = str.toLowerCase(); + if (lowerCased.length == str.length) { + return lowerCased; } + return str.replace(/[A-Z]/g, function(ch) { + return String.fromCharCode(ch.charCodeAt(0) | 32); + }); }; - /** * An interface to the {@code amp.htmlparser.HtmlParser} visitor, that gets * called while the HTML is being parsed. diff --git a/validator/htmlparser_test.js b/validator/htmlparser_test.js index 486def210c98..6cbcdd753c17 100644 --- a/validator/htmlparser_test.js +++ b/validator/htmlparser_test.js @@ -340,4 +340,51 @@ describe('HtmlParser with location', () => { ':17:0: endTag(html)', ':17:6: endDoc()']); }); + + it('Supports Turkish UTF8 İ character in body', () => { + // A Javascript string with this character in it has .length 1, but + // when .toLowerCase()'d it becomes length 2, which would throw off + // the bookkeeping in htmlparser.js. Hence, amp.htmlparser.toLowerCase + // works around the problem. + const handler = new LoggingHandlerWithLocation(); + const parser = new amp.htmlparser.HtmlParser(); + parser.parse( + handler, + '\n' + + '\n' + + '\n' + + '\n' + + '\n' + + '\n' + + '\n' + + 'İ\n' + + ''); + expect(handler.log).toEqual([ + ':1:0: startDoc()', + ':1:0: startTag(!doctype,[html,html])', + ':1:14: pcdata("\n")', + ':2:0: startTag(html,[amp,amp,lang,tr])', + ':2:19: pcdata("\n")', + ':3:0: startTag(head,[])', + ':3:5: pcdata("\n")', + ':4:0: startTag(meta,[charset,utf-8])', + ':4:21: pcdata("\n")', + ':5:0: startTag(title,[])', + ':5:0: rcdata("")', + ':5:7: endTag(title)', + ':5:14: pcdata("\n")', + ':6:0: startTag(script,[async,async,src,'+ + 'https://cdn.ampproject.org/v0.js])', + ':6:0: cdata("")', + ':6:53: endTag(script)', + ':6:61: pcdata("\n")', + ':7:0: endTag(head)', + ':7:6: pcdata("\n")', + ':8:0: startTag(body,[])', + ':8:5: pcdata("İ")', + ':8:7: endTag(body)', + ':8:13: pcdata("\n")', + ':9:0: endTag(html)', + ':9:6: endDoc()' ]); + }); }); From f7c11f18486484db5934525b629923dee761d004 Mon Sep 17 00:00:00 2001 From: Ivan Date: Thu, 4 Feb 2016 13:37:25 +0300 Subject: [PATCH 16/44] Fixed bug with missing & Fixed bug when you get _s:1dt=title instead of _s:1 dt:title in the request. --- extensions/amp-analytics/0.1/vendors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-analytics/0.1/vendors.js b/extensions/amp-analytics/0.1/vendors.js index ac9024d5eed9..39e5fc84a1e6 100644 --- a/extensions/amp-analytics/0.1/vendors.js +++ b/extensions/amp-analytics/0.1/vendors.js @@ -64,7 +64,7 @@ export const ANALYTICS_CONFIG = { }, 'requests': { 'host': 'https://www.google-analytics.com', - 'basePrefix': 'v=1&_v=a0&aip=true&_s=${requestCount}' + + 'basePrefix': 'v=1&_v=a0&aip=true&_s=${requestCount}&' + 'dt=${title}&sr=${screenWidth}x${screenHeight}&_utmht=${timestamp}&' + 'jid=&cid=${clientId(AMP_ECID_GOOGLE)}&tid=${account}&' + 'dl=${documentLocation}&' + From 9561a8f0e2d28b30cf7db0dea5eef39393c2044d Mon Sep 17 00:00:00 2001 From: Simas Toleikis Date: Wed, 3 Feb 2016 16:28:39 +0200 Subject: [PATCH 17/44] Added support. --- 3p/integration.js | 2 + ads/_config.js | 1 + ads/adform.js | 53 ++++++++++++++++++++++++ ads/adform.md | 63 +++++++++++++++++++++++++++++ builtins/amp-ad.md | 1 + docs/include_features.md | 1 + src/3p.js | 18 +++++++-- test/functional/test-3p.js | 30 ++++++++++---- test/functional/test-integration.js | 1 + 9 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 ads/adform.js create mode 100644 ads/adform.md diff --git a/3p/integration.js b/3p/integration.js index 70a6dd951861..74293d9264dc 100644 --- a/3p/integration.js +++ b/3p/integration.js @@ -24,6 +24,7 @@ import './polyfills'; import {a9} from '../ads/a9'; +import {adform} from '../ads/adform'; import {adreactor} from '../ads/adreactor'; import {adsense} from '../ads/adsense'; import {adtech} from '../ads/adtech'; @@ -48,6 +49,7 @@ const AMP_EMBED_ALLOWED = { }; register('a9', a9); +register('adform', adform); register('adreactor', adreactor); register('adsense', adsense); register('adtech', adtech); diff --git a/ads/_config.js b/ads/_config.js index 1fccdace1726..5b523064fe89 100644 --- a/ads/_config.js +++ b/ads/_config.js @@ -36,6 +36,7 @@ export const adPrefetch = { * @const {!Object)>} */ export const adPreconnect = { + adform: 'https://track.adform.net', adreactor: 'https://adserver.adreactor.com', adsense: 'https://googleads.g.doubleclick.net', taboola: 'https://cdn.taboola.com', diff --git a/ads/adform.js b/ads/adform.js new file mode 100644 index 000000000000..cd7794a5537e --- /dev/null +++ b/ads/adform.js @@ -0,0 +1,53 @@ +/** + * Copyright 2015 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {writeScript, validateSrcPrefix, validateExactlyOne} from '../src/3p'; + +// Valid adform ad source hosts +const hosts = { + track: 'https://track.adform.net/', + adx: 'https://adx.adform.net/' +}; + +/** + * @param {!Window} global + * @param {!Object} data + */ +export function adform(global, data) { + + validateExactlyOne(data, ['src', 'bn', 'mid']); + global.Adform = {ampData: data}; + const src = data.src; + const bn = data.bn; + const mid = data.mid; + let url; + + // Custom ad url using "data-src" attribute + if (src) { + validateSrcPrefix(Object.keys(hosts).map(type => hosts[type]), src); + url = src; + } + // Ad tag using "data-bn" attribute + else if (bn) { + url = hosts.track + 'adfscript/?bn=' + encodeURIComponent(bn) + ';msrc=1'; + } + // Ad placement using "data-mid" attribute + else if (mid) { + url = hosts.adx + 'adx/?mid=' + encodeURIComponent(mid); + } + + writeScript(global, url); +} diff --git a/ads/adform.md b/ads/adform.md new file mode 100644 index 000000000000..f348c2e18a6d --- /dev/null +++ b/ads/adform.md @@ -0,0 +1,63 @@ + + +# Adform + +Please refer to [Adform Help Center](http://help.adform.com) for more +information on how to get required ad tag or placement IDs. + +## Examples + +### Simple ad tag with `data-bn` + +```html + + +``` + +### Ad placement with `data-mid` + +```html + + +``` + +### Ad tag or placement with `src` + +```html + + +``` + +### Supported parameters + +- `data-bn` +- `data-mid` +- `src` + +Only one of the mentioned parameters should be used at the same time. + +The `src` parameter must use **https** protocol and must be from one of the +allowed hosts: + +- `https://track.adform.net/...` +- `https://adx.adform.net/...` diff --git a/builtins/amp-ad.md b/builtins/amp-ad.md index f9237fbce2c2..f60f7cd77236 100644 --- a/builtins/amp-ad.md +++ b/builtins/amp-ad.md @@ -42,6 +42,7 @@ resources in AMP. It requires a `type` argument that select what ad network is d #### Supported ad networks - [A9](../ads/a9.md) +- [Adform](../ads/adform.md) - [AdReactor](../ads/adreactor.md) - [AdSense](../ads/adsense.md) - [AdTech](../ads/adtech.md) diff --git a/docs/include_features.md b/docs/include_features.md index bf37b65bccf6..452bd80861e7 100644 --- a/docs/include_features.md +++ b/docs/include_features.md @@ -271,6 +271,7 @@ An example `amp-pixel` from the The following ad networks are supported in AMP HTML pages: - [A9](../ads/a9.md) +- [Adform](../ads/adform.md) - [AdReactor](../ads/adreactor.md) - [AdSense](../ads/adsense.md) - [AdTech](../ads/adtech.md) diff --git a/src/3p.js b/src/3p.js index 01dffd3cd1cd..4eab1b1f6e41 100644 --- a/src/3p.js +++ b/src/3p.js @@ -23,6 +23,7 @@ import {assert} from './asserts'; +import {isArray} from './types'; /** @typedef {function(!Window, !Object)} */ @@ -103,14 +104,23 @@ function executeAfterWriteScript(win, fn) { } /** - * Throws if the given src doesn't start with prefix. - * @param {string} prefix + * Throws if the given src doesn't start with prefix(es). + * @param {!Array|string} prefix * @param {string} src */ export function validateSrcPrefix(prefix, src) { - if (src.indexOf(prefix) !== 0) { - throw new Error('Invalid src ' + src); + + if (!isArray(prefix)) { + prefix = [prefix]; + } + + for (const p of prefix) { + if (src.indexOf(p) === 0) { + return; + } } + + throw new Error('Invalid src ' + src); } /** diff --git a/test/functional/test-3p.js b/test/functional/test-3p.js index eee5de8e91e4..b9faa51a0e5b 100644 --- a/test/functional/test-3p.js +++ b/test/functional/test-3p.js @@ -33,14 +33,28 @@ describe('3p', () => { 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'); + describe('validateSrcPrefix()', () => { + + it('should throw when a string prefix does not match', () => { + expect(() => { + validateSrcPrefix('https:', 'http://example.org'); + }).to.throw(/Invalid src/); + }); + + it('should throw when array prefixes do not match', () => { + expect(() => { + validateSrcPrefix(['https:', 'ftp:'], 'http://example.org'); + }).to.throw(/Invalid src/); + }); + + it('should not throw when a string prefix matches', () => { + validateSrcPrefix('http:', 'http://example.org'); + }); + + it('should not throw when any of the array prefixes match', () => { + validateSrcPrefix(['https:', 'http:'], 'http://example.org'); + validateSrcPrefix(['http:', 'https:'], 'http://example.org'); + }); }); it('should throw an error if src does not contain addyn', () => { diff --git a/test/functional/test-integration.js b/test/functional/test-integration.js index 91662d015eef..af49ce24152e 100644 --- a/test/functional/test-integration.js +++ b/test/functional/test-integration.js @@ -29,6 +29,7 @@ describe('3p integration.js', () => { it('should register integrations', () => { expect(registrations).to.include.key('a9'); + expect(registrations).to.include.key('adform'); expect(registrations).to.include.key('adsense'); expect(registrations).to.include.key('adtech'); expect(registrations).to.include.key('adreactor'); From aaafecc35c97e7e7252679edb4c8b0daefd7e959 Mon Sep 17 00:00:00 2001 From: Avi Mehta Date: Thu, 4 Feb 2016 07:39:16 -0800 Subject: [PATCH 18/44] Fixed config to send document title correctly. Fixes #1778. As an aside, opened #1779 so that this does not happen in future. --- extensions/amp-analytics/0.1/vendors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-analytics/0.1/vendors.js b/extensions/amp-analytics/0.1/vendors.js index ac9024d5eed9..39e5fc84a1e6 100644 --- a/extensions/amp-analytics/0.1/vendors.js +++ b/extensions/amp-analytics/0.1/vendors.js @@ -64,7 +64,7 @@ export const ANALYTICS_CONFIG = { }, 'requests': { 'host': 'https://www.google-analytics.com', - 'basePrefix': 'v=1&_v=a0&aip=true&_s=${requestCount}' + + 'basePrefix': 'v=1&_v=a0&aip=true&_s=${requestCount}&' + 'dt=${title}&sr=${screenWidth}x${screenHeight}&_utmht=${timestamp}&' + 'jid=&cid=${clientId(AMP_ECID_GOOGLE)}&tid=${account}&' + 'dl=${documentLocation}&' + From 500801e6c0ccd58a0a0013b6fe0950570ff878dd Mon Sep 17 00:00:00 2001 From: Jordan M Adler Date: Thu, 4 Feb 2016 08:40:13 -0800 Subject: [PATCH 19/44] Clarify non-support for xml attributes --- spec/amp-html-format.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/amp-html-format.md b/spec/amp-html-format.md index b12dc5683f78..7134af0da784 100644 --- a/spec/amp-html-format.md +++ b/spec/amp-html-format.md @@ -147,6 +147,8 @@ Attribute names starting with `on` (such as `onclick` or `onmouseover`) are disa The `style` attribute must not be used. +XML-related attributes, such as xmlns, xml:lang, xml:base, and xml:space are disallowed in AMP HTML. + ### Links The `javascript:` schema is disallowed. From d935d982028666d96f66049d79d7aeb3ed910007 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 4 Feb 2016 08:28:08 -0800 Subject: [PATCH 20/44] Tunes ad loading heuristic. We now load out of viewport ads before initial scrolling. Given recent performance improvements this seems a better trade off for UX, as ads below initial viewport are more likely to be done. --- 3p/integration.js | 6 +----- builtins/amp-ad.js | 6 ------ src/service/resources-impl.js | 2 +- 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/3p/integration.js b/3p/integration.js index 70a6dd951861..4749b6ff0426 100644 --- a/3p/integration.js +++ b/3p/integration.js @@ -148,11 +148,7 @@ window.draw3p = function(opt_configCallback) { window.context.reportRenderedEntityIdentifier = reportRenderedEntityIdentifier; delete data._context; - // Run this only in canary and local dev for the time being. - if (location.pathname.indexOf('-canary') || - location.pathname.indexOf('current')) { - manageWin(window); - } + manageWin(window); draw3p(window, data, opt_configCallback); nonSensitiveDataPostMessage('render-start'); }; diff --git a/builtins/amp-ad.js b/builtins/amp-ad.js index 5a4c0e03550f..b91de2655f91 100644 --- a/builtins/amp-ad.js +++ b/builtins/amp-ad.js @@ -51,12 +51,6 @@ export function installAd(win) { /** @override */ renderOutsideViewport() { - // Before the user has scrolled we only render ads in view. This prevents - // excessive jank in situations like swiping through a lot of articles. - if (!this.getViewport().hasScrolled()) { - return false; - }; - // If another ad is currently loading we only load ads that are currently // in viewport. if (loadingAdsCount > 0) { diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 9026bc8ace6f..590378d9f39d 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -57,7 +57,7 @@ export function getElementPriority(tagName) { if (tagName == 'amp-ad') { return 2; } - if (tagName == 'amp-pixel') { + if (tagName == 'amp-pixel' || tagName == 'amp-analytics') { return 1; } return 0; From 1c2640fd122f40cbc890d1c5abd2ccdb5e6d6312 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 4 Feb 2016 09:38:50 -0800 Subject: [PATCH 21/44] Add missing files to example validation. --- test/integration/test-example-validation.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/test-example-validation.js b/test/integration/test-example-validation.js index 3566a990e0ac..de3639553c46 100644 --- a/test/integration/test-example-validation.js +++ b/test/integration/test-example-validation.js @@ -46,11 +46,15 @@ describe('example', function() { 'metadata-examples/video-json-ld.amp.html', 'metadata-examples/video-microdata.amp.html', 'article.amp.html', + 'analytics.amp.html', + 'analytics-notification.amp.html', 'everything.amp.html', + 'facebook.amp.html', 'instagram.amp.html', 'released.amp.html', 'twitter.amp.html', 'vine.amp.html', + 'vimeo.amp.html', 'old-boilerplate.amp.html', ]; @@ -64,6 +68,8 @@ describe('example', function() { */ const errorWhitelist = [ /invalid value \'.\/viewer-integr.js\'/, + /vimeo/, + /comscore/, ]; const usedWhitelist = []; From f5fa42543f05909c3dd4f2983d83da30423b7ce5 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Wed, 3 Feb 2016 16:21:56 -0800 Subject: [PATCH 22/44] Requirements of source origin security in access endpoints --- extensions/amp-access/amp-access-spec.md | 42 +++++++++++++++--------- spec/amp-cors-requests.md | 36 ++++++++++++++++++++ src/url-replacements.js | 12 ++++++- test/functional/test-url-replacements.js | 7 ++++ 4 files changed, 80 insertions(+), 17 deletions(-) create mode 100644 spec/amp-cors-requests.md diff --git a/extensions/amp-access/amp-access-spec.md b/extensions/amp-access/amp-access-spec.md index 7d456955daa8..7012a6f995b8 100644 --- a/extensions/amp-access/amp-access-spec.md +++ b/extensions/amp-access/amp-access-spec.md @@ -51,12 +51,18 @@ Supporting AMP Access requires Publisher to implement the components described a To assist access services and use cases, AMP Access introduces the concept of the *Reader ID*. -The Reader ID is an anonymous and unique ID created by the AMP ecosystem. It is unique for each reader/publisher pair. i.e., a Reader is identified differently to two different publishers. It is a non-reversible ID. The Reader ID is included in all AMP/Publisher communications. Publishers can use the Reader ID to identify the Reader and map it to their own identity systems. +The Reader ID is an anonymous and unique ID created by the AMP ecosystem. It is unique for each reader/publisher pair. i.e., a Reader is identified differently to two different publishers. It is a non-reversible ID. The Reader ID is included in all AMP/Publisher communications. Publishers must use the Reader ID to identify the Reader and map it to their own identity systems. The Reader ID is constructed on the user device and intended to be long-lived. However, it follows the normal browser storage rules, including those for incognito windows. The intended lifecycle of a Reader ID is 1 year between uses or until the user clears their cookies. The Reader IDs are not currently shared between devices. The Reader ID is constructed similar to the mechanism used to build ExternalCID described [here](https://docs.google.com/document/d/1f7z3X2GM_ASb3ZCI_7tngglxwS6WoWi1EB3aKzdf6vo/edit#heading=h.hyyaxi8m42a). +##AMP Access and Cookies + +Even though some of the Publisher's own authentication cookies may be available at the time of the Authorization and Pingback requests, the cookies should only be used for internal mapping. There are no guarantees that the Publisher will be able to read or write cookies given all surfaces and platforms where an AMP document can be embedded. The Reader ID is the only identifier that is guaranteed to work. + +This means, in particular, that features such as metering and first-click-free implementation have to rely on the AMP Reader ID and server-side storage. + ##Access Content Markup Access Content Markup determines which sections are visible or hidden based on the authorization response returned from the Authorization endpoint. It is described via special markup attributes. @@ -107,11 +113,11 @@ Here’s an example of the AMP Access configuration: ``` @@ -125,6 +131,7 @@ Var | Description READER_ID | The AMP Reader ID. AUTHDATA(field) | The value of the field in the authorization response. RETURN_URL | The placeholder for the return URL specified by the AMP runtime for a Login Dialog to return to. +SOURCE_URL | The Source URL of this AMP Document. If document is served from CDN, AMPDOC_URL will be a CDN URL, while SOURCE_URL will be the original source URL. AMPDOC_URL | The URL of this AMP Document. CANONICAL_URL | The canonical URL of this AMP Document. DOCUMENT_REFERRER | The Referrer URL. @@ -200,9 +207,9 @@ And here’s an example that shows additional content to the premium subscribers ##Authorization Endpoint -Authorization is configured via ```authorization``` property in the [AMP Access Configuration][8] section. It is a credentialed CORS endpoint. See [CORS Origin Security][9] for a list of origins that should be allowed. +Authorization is configured via ```authorization``` property in the [AMP Access Configuration][8] section. It is a credentialed CORS endpoint. See [CORS Origin Security][9] for how this request should be secured. -Authorization can take any parameters as defined in the [Access URL Variables][7] section. For instance, it could pass AMP Reader ID and document URL. Besides URL parameters, the Publisher may use any information naturally delivered via HTTP protocol, such as Reader’s IP address. +Authorization can take any parameters as defined in the [Access URL Variables][7] section. For instance, it could pass AMP Reader ID and document URL. Besides URL parameters, the Publisher may use any information naturally delivered via HTTP protocol, such as Reader’s IP address. The inclusion of the `READER_ID` is required. This endpoint produces the authorization response that can be used in the content markup expressions to show/hide different parts of content. @@ -210,7 +217,7 @@ The request format is: ``` https://publisher.com/amp-access.json? rid={READER_ID} - &url={AMPDOC_URL} + &url={SOURCE_URL} ``` The response is a free-form JSON object: it can contain any properties and values with few limitations. The limitations are: - The property names have to conform to the restrictions defined by the ```amp-access``` expressions grammar (see [Appendix A][1]. This mostly means that the property names cannot contain characters such as spaces, dashes and other characters that do not conform to the “amp-access” specification. @@ -247,7 +254,7 @@ The authorization response may be used by AMP Runtime and extensions for two dif 1. When evaluating ```amp-access``` expressions. 2. When evaluating ```