From 4f3cd0de6dadbd6a978dba249f730c950b3286e3 Mon Sep 17 00:00:00 2001 From: William Chou Date: Wed, 13 Feb 2019 15:28:26 -0500 Subject: [PATCH] Don't send amp-bind state to untrusted viewers (#20822) * Only send state for history updates to trusted viewers. * Add and fix tests. * Fix type check. * Fix presubmit. --- build-system/tasks/presubmit-checks.js | 1 + extensions/amp-bind/0.1/bind-impl.js | 40 +++++-- .../0.1/test/integration/test-bind-impl.js | 102 +++++++++++++----- 3 files changed, 108 insertions(+), 35 deletions(-) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 384a9a0e1fd6..43bf9ab3db2d 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -397,6 +397,7 @@ const forbiddenTerms = { 'isTrustedViewer': { message: requiresReviewPrivacy, whitelist: [ + 'extensions/amp-bind/0.1/bind-impl.js', 'src/error.js', 'src/utils/xhr-utils.js', 'src/service/viewer-impl.js', diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js index b8e6e0d99278..111d0d0f7474 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/extensions/amp-bind/0.1/bind-impl.js @@ -302,11 +302,12 @@ export class Bind { dev().info(TAG, 'setState:', `"${expression}"`); this.setStatePromise_ = this.evaluateExpression_(expression, scope) .then(result => this.setState(result)) - .then(() => { - this.history_.replace({ - 'data': dict({'amp-bind': this.state_}), - 'title': this.localWin_.document.title, - }); + .then(() => this.getDataForHistory_()) + .then(data => { + // Don't bother calling History.replace with empty data. + if (data) { + this.history_.replace(data); + } }); return this.setStatePromise_; } @@ -334,15 +335,34 @@ export class Bind { const onPop = () => this.setState(oldState); return this.setState(result) - .then(() => { - this.history_.push(onPop, { - 'data': dict({'amp-bind': this.state_}), - 'title': this.localWin_.document.title, - }); + .then(() => this.getDataForHistory_()) + .then(data => { + this.history_.push(onPop, data); }); }); } + /** + * Returns data that should be saved in browser history on AMP.setState() or + * AMP.pushState(). This enables features like restoring browser tabs. + * @return {!Promise} + */ + getDataForHistory_() { + const data = dict({ + 'data': dict({'amp-bind': this.state_}), + 'title': this.localWin_.document.title, + }); + if (!this.viewer_.isEmbedded()) { + // CC doesn't recognize !JsonObject as a subtype of (JsonObject|null). + return /** @type {!Promise} */ (Promise.resolve(data)); + } + // Only pass state for history updates to trusted viewers, since they + // may contain user data e.g. form input. + return this.viewer_.isTrustedViewer().then(trusted => { + return (trusted) ? data : null; + }); + } + /** * Removes bindings from `removed` elements and scans `added` for bindings. * Then, evaluates all expressions and applies results to `added` elements. diff --git a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js index f15113cc2c48..36b74f6d3ca0 100644 --- a/extensions/amp-bind/0.1/test/integration/test-bind-impl.js +++ b/extensions/amp-bind/0.1/test/integration/test-bind-impl.js @@ -262,22 +262,29 @@ describe.configure().ifChrome().run('Bind', function() { mockFetch: false, }, env => { let bind; + let clock; let container; + let history; let viewer; - let clock; + let sandbox; beforeEach(() => { const {ampdoc, win} = env; + sandbox = env.sandbox; // Make sure we have a chunk instance for testing. chunkInstanceForTesting(ampdoc); viewer = Services.viewerForDoc(ampdoc); sandbox.stub(viewer, 'sendMessage'); + bind = new Bind(ampdoc); + // Connected
element created by describes.js. container = win.document.getElementById('parent'); + history = bind.historyForTesting(); + clock = lolex.install({target: win}); }); @@ -337,7 +344,7 @@ describe.configure().ifChrome().run('Bind', function() { }); it('should call createTreeWalker() with all params', () => { - const spy = env.sandbox.spy(env.win.document, 'createTreeWalker'); + const spy = sandbox.spy(env.win.document, 'createTreeWalker'); createElement(env, container, '[text]="1+1"'); return onBindReady(env, bind).then(() => { // createTreeWalker() on IE does not support optional arguments. @@ -392,7 +399,7 @@ describe.configure().ifChrome().run('Bind', function() { createElement(env, container, '[class]="\'foo\'" class=" foo "'); createElement(env, container, '[class]="\'\'"'); createElement(env, container, '[class]="\'bar\'" class="qux"'); // Error - const warnSpy = env.sandbox.spy(user(), 'warn'); + const warnSpy = sandbox.spy(user(), 'warn'); return onBindReady(env, bind).then(() => { expect(warnSpy).to.be.calledOnce; expect(warnSpy).calledWithMatch('amp-bind', /\[class\]/); @@ -403,7 +410,7 @@ describe.configure().ifChrome().run('Bind', function() { window.AMP_MODE = {development: true, test: true}; // Only the initial value for [a] binding does not match. createElement(env, container, '[text]="\'a\'" [class]="\'b\'" class="b"'); - const warnSpy = env.sandbox.spy(user(), 'warn'); + const warnSpy = sandbox.spy(user(), 'warn'); return onBindReady(env, bind).then(() => { expect(warnSpy).to.be.calledOnce; expect(warnSpy).calledWithMatch('amp-bind', /\[text\]/); @@ -415,7 +422,7 @@ describe.configure().ifChrome().run('Bind', function() { createElement(env, container, '[disabled]="true" disabled', 'button'); createElement(env, container, '[disabled]="false"', 'button'); createElement(env, container, '[disabled]="true"', 'button'); // Mismatch. - const warnSpy = env.sandbox.spy(user(), 'warn'); + const warnSpy = sandbox.spy(user(), 'warn'); return onBindReady(env, bind).then(() => { expect(warnSpy).to.be.calledOnce; expect(warnSpy).calledWithMatch('amp-bind', /\[disabled\]/); @@ -636,22 +643,68 @@ describe.configure().ifChrome().run('Bind', function() { }); }); - it('should replace history state in setStateWithExpression()', () => { - const replaceHistorySpy = - env.sandbox.spy(bind.historyForTesting(), 'replace'); - const promise = onBindReadyAndSetStateWithExpression( - env, bind, '{"onePlusOne": one + one}', {one: 1}); - return promise.then(() => { - expect(replaceHistorySpy).calledOnce; - expect(replaceHistorySpy.firstCall.args[0].data['amp-bind']) - .to.deep.equal({onePlusOne: 2}); + describe('history', () => { + beforeEach(() => { + sandbox.spy(history, 'replace'); + sandbox.spy(history, 'push'); + sandbox.stub(viewer, 'isEmbedded').returns(true); + }); + + describe('with untrusted viewer', () => { + it('should not replace history on AMP.setState()', () => { + const promise = onBindReadyAndSetStateWithExpression( + env, bind, '{"onePlusOne": one + one}', {one: 1}); + return promise.then(() => { + // Shouldn't call replace() with null `data`. + expect(history.replace).to.not.be.called; + }); + }); + + it('should push history (no data) on AMP.pushState()', () => { + const promise = bind.pushStateWithExpression('{"foo": "bar"}', {}); + return promise.then(() => { + expect(history.push).to.be.called; + // `data` param should be null on untrusted viewers. + expect(history.push).to.be.calledWith(sinon.match.func, null); + }); + }); }); - }); + describe('with trusted viewer', () => { + beforeEach(() => { + sandbox.stub(viewer, 'isTrustedViewer') + .returns(Promise.resolve(true)); + }); + + it('should replace history on AMP.setState()', () => { + const promise = onBindReadyAndSetStateWithExpression( + env, bind, '{"onePlusOne": one + one}', {one: 1}); + return promise.then(() => { + expect(history.replace).calledOnce; + // `data` param should exist on trusted viewers. + expect(history.replace).calledWith({ + data: {'amp-bind': {'onePlusOne': 2}}, + title: '', + }); + }); + }); + + it('should push history on AMP.pushState()', () => { + const promise = bind.pushStateWithExpression('{"foo": "bar"}', {}); + return promise.then(() => { + expect(history.push).calledOnce; + // `data` param should exist on trusted viewers. + expect(history.push).calledWith(sinon.match.func, { + data: {'amp-bind': {foo: 'bar'}}, + title: '', + }); + }); + }); + }); + }); it('should support pushStateWithExpression()', () => { - const pushHistorySpy = - env.sandbox.spy(bind.historyForTesting(), 'push'); + sandbox.spy(history, 'push'); const element = createElement(env, container, '[text]="foo"'); expect(element.textContent).to.equal(''); @@ -660,9 +713,9 @@ describe.configure().ifChrome().run('Bind', function() { env.flushVsync(); expect(element.textContent).to.equal('bar'); - expect(pushHistorySpy).calledOnce; + expect(history.push).calledOnce; // Pop callback should restore `foo` to original value (null). - const onPopCallback = pushHistorySpy.firstCall.args[0]; + const onPopCallback = history.push.firstCall.args[0]; return onPopCallback(); }).then(() => { expect(element.textContent).to.equal('null'); @@ -670,8 +723,7 @@ describe.configure().ifChrome().run('Bind', function() { }); it('pushStateWithExpression() should work with nested objects', () => { - const pushHistorySpy = - env.sandbox.spy(bind.historyForTesting(), 'push'); + sandbox.spy(history, 'push'); const element = createElement(env, container, '[text]="foo.bar"'); expect(element.textContent).to.equal(''); @@ -684,9 +736,9 @@ describe.configure().ifChrome().run('Bind', function() { env.flushVsync(); expect(element.textContent).to.equal('1'); - expect(pushHistorySpy).calledTwice; + expect(history.push).calledTwice; // Pop callback should restore `foo.bar` to second pushed value (0). - const onPopCallback = pushHistorySpy.secondCall.args[0]; + const onPopCallback = history.push.secondCall.args[0]; return onPopCallback(); }).then(() => { expect(element.textContent).to.equal('0'); @@ -720,7 +772,7 @@ describe.configure().ifChrome().run('Bind', function() { + 'checked [checked]="false" [disabled]="true" [multiple]="false"'; const element = createElement(env, container, binding, /* opt_tagName */ 'input', /* opt_amp */ true); - const spy = env.sandbox.spy(element, 'mutatedAttributesCallback'); + const spy = sandbox.spy(element, 'mutatedAttributesCallback'); return onBindReadyAndSetState(env, bind, {}).then(() => { expect(spy).calledWithMatch({ checked: false, @@ -799,7 +851,7 @@ describe.configure().ifChrome().run('Bind', function() { it('should stop scanning once max number of bindings is reached', () => { bind.setMaxNumberOfBindingsForTesting(2); - const errorStub = env.sandbox.stub(dev(), 'expectedError'); + const errorStub = sandbox.stub(dev(), 'expectedError'); const foo = createElement(env, container, '[text]="foo"'); const bar = createElement(env, container, '[text]="bar" [class]="baz"');