Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow iframes to be loaded from data URIs. #793

Merged
merged 1 commit into from
Oct 30, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,16 @@ class AmpIframe extends AMP.BaseElement {
var url = parseUrl(src);
assert(
url.protocol == 'https:' ||
url.protocol == 'data:' ||
url.origin.indexOf('http://iframe.localhost:') == 0,
'Invalid <amp-iframe> src. Must start with https://. Found %s',
this.element);
var containerUrl = parseUrl(containerSrc);
assert(
!((' ' + sandbox + ' ').match(/\s+allow-same-origin\s+/)) ||
url.origin != containerUrl.origin,
'Origin of <amp-iframe> must not be equal to container %s.',
'Origin of <amp-iframe> must not be equal to container %s' +
'if allow-same-origin is set.',
this.element);
return src;
}
Expand All @@ -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);
Expand Down
58 changes: 58 additions & 0 deletions extensions/amp-iframe/0.1/test/test-amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<script>try{parent.location.href}catch(e){' +
'parent.parent./*OK*/postMessage(\'loaded-iframe\', \'*\');}' +
'</script>',
}).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(() => {
Expand All @@ -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/);
});
});

Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-iframe/amp-iframe.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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.
13 changes: 11 additions & 2 deletions src/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
18 changes: 17 additions & 1 deletion test/functional/test-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {assertHttpsUrl, parseQueryString, parseUrl, removeFragment}
import {assertHttpsUrl, parseQueryString, parseUrl, removeFragment, getOrigin}
from '../../src/url';

describe('url', () => {
Expand Down Expand Up @@ -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');
});
});