From 65a363b0a680b35b2e45c51cf619324420b9fce0 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 29 Oct 2015 17:16:30 -0700 Subject: [PATCH] Allow iframes to be loaded from data URIs and using srcdoc. Fixes #404 Fixes #405 --- extensions/amp-iframe/0.1/amp-iframe.js | 28 ++++++++- .../amp-iframe/0.1/test/test-amp-iframe.js | 58 +++++++++++++++++++ extensions/amp-iframe/amp-iframe.md | 4 +- src/url.js | 13 ++++- test/functional/test-url.js | 18 +++++- 5 files changed, 114 insertions(+), 7 deletions(-) diff --git a/extensions/amp-iframe/0.1/amp-iframe.js b/extensions/amp-iframe/0.1/amp-iframe.js index f0d9b9b562f9..45414a1e4213 100644 --- a/extensions/amp-iframe/0.1/amp-iframe.js +++ b/extensions/amp-iframe/0.1/amp-iframe.js @@ -35,6 +35,7 @@ class AmpIframe extends AMP.BaseElement { var url = parseUrl(src); assert( url.protocol == 'https:' || + url.protocol == 'data:' || url.origin.indexOf('http://iframe.localhost:') == 0, 'Invalid src. Must start with https://. Found %s', this.element); @@ -42,7 +43,8 @@ class AmpIframe extends AMP.BaseElement { assert( !((' ' + sandbox + ' ').match(/\s+allow-same-origin\s+/)) || url.origin != containerUrl.origin, - 'Origin of must not be equal to container %s.', + 'Origin of must not be equal to container %s' + + 'if allow-same-origin is set.', this.element); return src; } @@ -61,9 +63,31 @@ class AmpIframe extends AMP.BaseElement { minTop); } + /** + * Transforms the srcdoc attribute if present to an equivalent data URI. + * + * It may be OK to change this later to leave the `srcdoc` in place and + * instead ensure that `allow-same-origin` is not present, but this + * implementation has the right security behavior which is that the document + * may under no circumstances be able to run JS on the parent. + * @return {string} Data URI for the srcdoc + */ + transformSrcDoc() { + var srcdoc = this.element.getAttribute('srcdoc'); + if (!srcdoc) { + return; + } + var sandbox = this.element.getAttribute('sandbox'); + assert( + !((' ' + sandbox + ' ').match(/\s+allow-same-origin\s+/)), + 'allow-same-origin is not allowed with the srcdoc attribute %s.', + this.element); + return 'data:text/html;charset=utf-8;base64,' + btoa(srcdoc); + } + /** @override */ firstAttachedCallback() { - var iframeSrc = this.element.getAttribute('src'); + var iframeSrc = this.element.getAttribute('src') || this.transformSrcDoc(); this.iframeSrc = this.assertSource(iframeSrc, window.location.href, this.element.getAttribute('sandbox')); this.preconnect.url(this.iframeSrc); diff --git a/extensions/amp-iframe/0.1/test/test-amp-iframe.js b/extensions/amp-iframe/0.1/test/test-amp-iframe.js index 986072af5b39..d3dfadcbf93c 100644 --- a/extensions/amp-iframe/0.1/test/test-amp-iframe.js +++ b/extensions/amp-iframe/0.1/test/test-amp-iframe.js @@ -160,6 +160,58 @@ describe('amp-iframe', () => { }); }); + it('should allow data-uri', () => { + var dataUri = 'data:text/html;charset=utf-8;base64,' + + 'PHNjcmlwdD5kb2N1bWVudC53cml0ZSgnUiAnICsgZG9jdW1lbnQucmVmZXJyZXIgK' + + 'yAnLCAnICsgbG9jYXRpb24uaHJlZik8L3NjcmlwdD4='; + return getAmpIframe({ + src: dataUri, + width: 100, + height: 100 + }).then(amp => { + expect(amp.iframe).to.be.instanceof(Element); + expect(amp.iframe.src).to.equal(dataUri); + expect(amp.iframe.getAttribute('sandbox')).to.equal(''); + expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper); + return timer.promise(0).then(() => { + expect(ranJs).to.equal(0); + }); + }); + }); + + it('should support srcdoc', () => { + return getAmpIframe({ + width: 100, + height: 100, + sandbox: 'allow-scripts', + srcdoc: '', + }).then(amp => { + expect(amp.iframe).to.be.instanceof(Element); + expect(amp.iframe.src).to.match( + /^data\:text\/html;charset=utf-8;base64,/); + expect(amp.iframe.getAttribute('srcdoc')).to.be.null; + expect(amp.iframe.getAttribute('sandbox')).to.equal( + 'allow-scripts'); + expect(amp.iframe.parentNode).to.equal(amp.scrollWrapper); + return timer.promise(0).then(() => { + expect(ranJs).to.equal(1); + }); + }); + }); + + it('should deny srcdoc with allow-same-origin', () => { + return getAmpIframe({ + width: 100, + height: 100, + sandbox: 'allow-same-origin', + srcdoc: '', + }).then(amp => { + expect(amp.iframe).to.be.null; + }); + }); + it('should deny same origin', () => { return getAmpIframeObject().then(amp => { expect(() => { @@ -182,6 +234,12 @@ describe('amp-iframe', () => { amp.assertSource('http://iframe.localhost:123/foo', 'https://foo.com', ''); amp.assertSource('https://container.com', 'https://foo.com', ''); + + amp.element.setAttribute('srcdoc', 'abc'); + amp.element.setAttribute('sandbox', 'allow-same-origin'); + expect(() => { + amp.transformSrcDoc(); + }).to.throw(/allow-same-origin is not allowed with the srcdoc attribute/); }); }); diff --git a/extensions/amp-iframe/amp-iframe.md b/extensions/amp-iframe/amp-iframe.md index a5b96a416050..4d3b59621950 100644 --- a/extensions/amp-iframe/amp-iframe.md +++ b/extensions/amp-iframe/amp-iframe.md @@ -22,7 +22,7 @@ Displays an iframe. - `amp-iframe` may not appear close to the top of the document. They must be either 600px away from the top or not within the first 75% of the viewport when scrolled to the top – whichever is smaller. NOTE: We are currently looking for feedback as to how well this restriction works in practice. - They are sandboxed by default. That means that authors needs to be explicit about what should be allowed in the iframe. See the [the docs on MDN](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe) for details on the sandbox attribute. -- They must only request resources via HTTPS. +- They must only request resources via HTTPS or from a data-URI or via the srcdoc attribute. - They must not be in the same origin as the container unless they do not allow `allow-same-origin` in the sandbox attribute. Example: @@ -37,6 +37,6 @@ Example: #### Attributes -**src, sandbox, frameborder, allowfullscreen, allowtransparency** +**src, srcdoc, sandbox, frameborder, allowfullscreen, allowtransparency** The attributes above should all behave like they do on standard iframes. diff --git a/src/url.js b/src/url.js index d467b8eafd1f..a856ac62c332 100644 --- a/src/url.js +++ b/src/url.js @@ -36,7 +36,9 @@ export function parseUrl(url) { search: a.search, hash: a.hash }; - info.origin = a.origin || getOrigin(info); + // For data URI a.origin is equal to the string 'null' which is not useful. + // We instead return the actual origin which is the full URL. + info.origin = (a.origin && a.origin != 'null') ? a.origin : getOrigin(info); assert(info.origin, 'Origin must exist'); return info; } @@ -101,10 +103,17 @@ export function parseQueryString(queryString) { /** + * Don't use this directly, only exported for testing. The value + * is available via the origin property of the object returned by + * parseUrl. * @param {!Location} info * @return {string} + * @visibleForTesting */ -function getOrigin(info) { +export function getOrigin(info) { + if (info.protocol == 'data:' || !info.host) { + return info.href; + } return info.protocol + '//' + info.host; } diff --git a/test/functional/test-url.js b/test/functional/test-url.js index 93a17a2232db..d9da54fed237 100644 --- a/test/functional/test-url.js +++ b/test/functional/test-url.js @@ -14,7 +14,7 @@ * limitations under the License. */ -import {assertHttpsUrl, parseQueryString, parseUrl, removeFragment} +import {assertHttpsUrl, parseQueryString, parseUrl, removeFragment, getOrigin} from '../../src/url'; describe('url', () => { @@ -204,3 +204,19 @@ describe('removeFragment', () => { 'https://twitter.com/path'); }); }); + +describe('getOrigin', () => { + it('should parse https://twitter.com/path#abc', () => { + expect(getOrigin(parseUrl('https://twitter.com/path#abc'))) + .to.equal('https://twitter.com'); + expect(parseUrl('https://twitter.com/path#abc').origin) + .to.equal('https://twitter.com'); + }); + + it('should parse data:12345', () => { + expect(getOrigin(parseUrl('data:12345'))) + .to.equal('data:12345'); + expect(parseUrl('data:12345').origin) + .to.equal('data:12345'); + }); +});