diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index abba66686812..d5c597915f04 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -68,11 +68,11 @@ function encodeValue(val) { * Class to provide variables that pertain to top level AMP window. */ export class GlobalVariableSource extends VariableSource { - + /** + * @param {!./ampdoc-impl.AmpDoc} ampdoc + */ constructor(ampdoc) { - super(); - /** @const {!./ampdoc-impl.AmpDoc} */ - this.ampdoc = ampdoc; + super(ampdoc); /** * @private diff --git a/src/service/variable-source.js b/src/service/variable-source.js index a0519686920d..4efc31f90d25 100644 --- a/src/service/variable-source.js +++ b/src/service/variable-source.js @@ -109,7 +109,13 @@ export function getNavigationData(win, attribute) { * and override initialize() to add more supported variables. */ export class VariableSource { - constructor() { + /** + * @param {?./ampdoc-impl.AmpDoc} ampdoc + */ + constructor(ampdoc) { + /** @protected @const {?./ampdoc-impl.AmpDoc} */ + this.ampdoc = ampdoc; + /** @private {!RegExp|undefined} */ this.replacementExpr_ = undefined; @@ -121,6 +127,47 @@ export class VariableSource { /** @private {boolean} */ this.initialized_ = false; + + /** + * The whitelist of variables allowed for variable substitution. + * @private @const {?Array} + */ + this.variableWhitelist_ = this.getVariableWhitelist_(); + } + + /** + * @return {?Array} The whitelist of allowed AMP variables. (if provided in + * a meta tag). + * @private + */ + getVariableWhitelist_() { + if (this.variableWhitelist_) { + return this.variableWhitelist_; + } + + // TODO(hamousavi): Remove this conditional and change the type annotation + // for this.ampdoc to non-nullable. This is a temporary measures because + // tests currently do not respect the non-nullability measure. + if (!this.ampdoc) { + return null; + } + + const head = this.ampdoc.getRootNode().head; + if (!head) { + return null; + } + + // A meta[name="amp-variable-substitution-whitelist"] tag, if present, + // contains, in its content attribute, a whitelist of variable substitution. + const meta = + head.querySelector('meta[name="amp-variable-substitution-whitelist"]'); + if (!meta) { + return null; + } + + this.variableWhitelist_ = meta.getAttribute('content').split(',') + .map(variable => variable.trim()); + return this.variableWhitelist_; } /** @@ -164,6 +211,10 @@ export class VariableSource { */ set(varName, syncResolver) { dev().assert(varName.indexOf('RETURN') == -1); + if (!this.isWhitelisted_(varName)) { + return this; + } + this.replacements_[varName] = this.replacements_[varName] || {sync: undefined, async: undefined}; this.replacements_[varName].sync = syncResolver; @@ -184,6 +235,9 @@ export class VariableSource { */ setAsync(varName, asyncResolver) { dev().assert(varName.indexOf('RETURN') == -1); + if (!this.isWhitelisted_(varName)) { + return this; + } this.replacements_[varName] = this.replacements_[varName] || {sync: undefined, async: undefined}; this.replacements_[varName].async = asyncResolver; @@ -246,6 +300,12 @@ export class VariableSource { * @private */ buildExpr_(keys, isV2) { + // If a whitelist is present, the keys must belong to the whitelist. + // We filter the keys one last time to ensure no unwhitelisted key is + // allowed. + if (this.getVariableWhitelist_()) { + keys = keys.filter(key => this.getVariableWhitelist_().includes(key)); + } // 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); @@ -264,4 +324,16 @@ export class VariableSource { } return new RegExp(regexStr, 'g'); } + + /** + * Returns `true` if a variable whitelist is *not* present or the present + * whitelist contains the given variable name. + * @param {string} varName + * @return {boolean} + * @private + */ + isWhitelisted_(varName) { + return !this.getVariableWhitelist_() || + this.getVariableWhitelist_().includes(varName); + } } diff --git a/test/functional/test-variable-source.js b/test/functional/test-variable-source.js index 0ee3ab5364dd..97ee9c56aae4 100644 --- a/test/functional/test-variable-source.js +++ b/test/functional/test-variable-source.js @@ -19,11 +19,17 @@ import { getTimingDataAsync, } from '../../src/service/variable-source'; +import {createElementWithAttributes} from '../../src/dom'; -describe('VariableSource', () => { + +describes.fakeWin('VariableSource', { + amp: { + ampdoc: 'single', + }, +}, env => { let varSource; beforeEach(() => { - varSource = new VariableSource(); + varSource = new VariableSource(env.ampdoc); }); it('Works without any variables', () => { @@ -84,6 +90,39 @@ describe('VariableSource', () => { }); }); + describes.realWin('Whitelist of variable substitutions', { + amp: { + ampdoc: 'single', + }, + }, env => { + let variableSource; + beforeEach(() => { + env.win.document.head.appendChild( + createElementWithAttributes(env.win.document, 'meta', { + name: 'amp-variable-substitution-whitelist', + content: 'ABC,ABCD,CANONICAL', + })); + variableSource = new VariableSource(env.ampdoc); + }); + + it('Works with whitelisted variables', () => { + variableSource.setAsync('ABCD', () => Promise.resolve('abcd')); + expect(variableSource.getExpr()).to.be.ok; + + return variableSource.get('ABCD')['async']().then(value => { + expect(value).to.equal('abcd'); + }); + }); + + it('Should not work with unwhitelisted variables', () => { + variableSource.setAsync('RANDOM', () => Promise.resolve('0.1234')); + expect(variableSource.getExpr()).to.be.ok; + + expect(variableSource.get('RANDOM')).to.be.undefined; + }); + + }); + describes.fakeWin('getTimingData', {}, env => { let win; @@ -116,4 +155,4 @@ describe('VariableSource', () => { }); }); }); -}); +}); \ No newline at end of file diff --git a/test/functional/url-expander/test-expander.js b/test/functional/url-expander/test-expander.js index 3b699f3db91c..a265a5b84247 100644 --- a/test/functional/url-expander/test-expander.js +++ b/test/functional/url-expander/test-expander.js @@ -14,8 +14,10 @@ * limitations under the License. */ +import {AmpDocSingle} from '../../../src/service/ampdoc-impl'; import {Expander} from '../../../src/service/url-expander/expander'; import {GlobalVariableSource} from '../../../src/service/url-replacements-impl'; +import {createElementWithAttributes} from '../../../src/dom'; describes.realWin('Expander', { amp: { @@ -86,6 +88,41 @@ describes.realWin('Expander', { }); }); + describe('Whitelist of variables', () => { + let variableSource; + let expander; + + const mockBindings = { + RANDOM: () => 0.1234, + ABC: () => 'three', + ABCD: () => 'four', + }; + beforeEach(() => { + env.win.document.head.appendChild( + createElementWithAttributes(env.win.document, 'meta', { + name: 'amp-variable-substitution-whitelist', + content: 'ABC,ABCD,CANONICAL', + })); + + variableSource = new GlobalVariableSource(env.ampdoc); + expander = new Expander(variableSource); + }); + + it('should not replace unwhitelisted RANDOM', () => { + const url = 'http://www.google.com/?test=RANDOM'; + const expected = 'http://www.google.com/?test=RANDOM'; + return expect(expander.expand(url, mockBindings)) + .to.eventually.equal(expected); + }); + + it('should replace whitelisted ABCD', () => { + const url = 'http://www.google.com/?test=ABCD'; + const expected = 'http://www.google.com/?test=four'; + return expect(expander.expand(url, mockBindings)) + .to.eventually.equal(expected); + }); + }); + describe('#expand', () => { function mockClientIdFn(str) { if (str === '__ga') {