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

Implement replaceUrl protocol for safe URL replacements #9175

Merged
merged 3 commits into from
May 9, 2017
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
25 changes: 24 additions & 1 deletion src/service/viewer-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {registerServiceBuilderForDoc} from '../service';
import {dev, duplicateErrorIfNecessary} from '../log';
import {isIframed} from '../dom';
import {
getSourceOrigin,
parseQueryString,
parseUrl,
removeFragment,
Expand Down Expand Up @@ -362,6 +363,27 @@ export class Viewer {
}
});

// Replace URL if requested.
const replaceUrlParam = this.params_['replaceUrl'];
if (ampdoc.isSingleDoc() &&
replaceUrlParam &&
this.win.history.replaceState) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#replaceState check should be unnecessary, supported in everything but IE 9.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is still an issue. Last time we tried to remove this check, we saw lots of errors. Feel free to file a bug for a deeper cleanup. But it will have to first start in the history-impl.js with error sampling for when it's not available.

try {
// The origin and source origin must match.
const url = parseUrl(this.win.location.href);
const replaceUrl = parseUrl(
removeFragment(replaceUrlParam) + this.win.location.hash);
if (url.origin == replaceUrl.origin &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compare case-insensitive ( I believe origin when coming from navigator will be lowercase but not when coming from this.params_['replaceUrl'] (don't see parseUrl or getSourceOrigin lowercase anything)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe by spec, HTMLAnchorElement must lowercase origins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh didn't know our parseUrl uses <a>. very cool!

getSourceOrigin(url) == getSourceOrigin(replaceUrl)) {
this.win.history.replaceState({}, '', replaceUrl.href);
this.win.location.originalHref = url.href;
dev().fine(TAG_, 'replace url:' + replaceUrl.href);
}
} catch (e) {
dev().error(TAG_, 'replaceUrl failed', e);
}
}

// Remove hash when we have an incoming click tracking string
// (see impression.js).
if (this.params_['click']) {
Expand All @@ -373,7 +395,7 @@ export class Viewer {
this.win.location.originalHash = this.win.location.hash;
}
this.win.history.replaceState({}, '', newUrl);
dev().fine(TAG_, 'replace url:' + this.win.location.href);
dev().fine(TAG_, 'replace fragment:' + this.win.location.href);
}
}

Expand Down Expand Up @@ -941,6 +963,7 @@ function getChannelError(opt_reason) {
return new Error('No messaging channel: ' + opt_reason);
}


/**
* Sets the viewer visibility state. This calls is restricted to runtime only.
* @param {!VisibilityState} state
Expand Down
100 changes: 99 additions & 1 deletion test/functional/test-viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {dev} from '../../src/log';
import {installDocService} from '../../src/service/ampdoc-impl';
import {installPlatformService} from '../../src/service/platform-impl';
import {installTimerService} from '../../src/service/timer-impl';
import {parseUrl, removeFragment} from '../../src/url';
import * as sinon from 'sinon';


Expand Down Expand Up @@ -76,8 +77,11 @@ describe('Viewer', () => {
};
windowApi.navigator = window.navigator;
windowApi.history = {
replaceState: sandbox.spy(),
replaceState: () => {},
};
sandbox.stub(windowApi.history, 'replaceState', (state, title, url) => {
windowApi.location.href = url;
});
installDocService(windowApi, /* isSingleDoc */ true);
ampdoc = ampdocServiceFor(windowApi).getAmpDoc();
installPlatformService(windowApi);
Expand Down Expand Up @@ -226,6 +230,100 @@ describe('Viewer', () => {
expect(viewportEvent).to.not.equal(null);
});

describe('replaceUrl', () => {
function setUrl(href) {
const url = parseUrl(href);
windowApi.location.href = url.href;
windowApi.location.hash = url.hash;
}

it('should replace URL for the same non-proxy origin', () => {
const fragment = '#replaceUrl=http://www.example.com/two%3Fa%3D1&b=1';
setUrl('http://www.example.com/one' + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.be.calledOnce;
expect(windowApi.history.replaceState).to.be.calledWith({}, '',
'http://www.example.com/two?a=1' + fragment);
expect(ampdoc.getUrl())
.to.equal('http://www.example.com/two?a=1' + fragment);
expect(windowApi.location.originalHref)
.to.equal('http://www.example.com/one' + fragment);
});

it('should ignore replacement fragment', () => {
const fragment = '#replaceUrl=http://www.example.com/two%23b=2&b=1';
setUrl('http://www.example.com/one' + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.be.calledOnce;
expect(windowApi.history.replaceState).to.be.calledWith({}, '',
'http://www.example.com/two' + fragment);
expect(windowApi.location.originalHref)
.to.equal('http://www.example.com/one' + fragment);
});

it('should replace relative URL for the same non-proxy origin', () => {
const fragment = '#replaceUrl=/two&b=1';
setUrl(removeFragment(window.location.href) + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.be.calledOnce;
expect(windowApi.history.replaceState).to.be.calledWith({}, '',
window.location.origin + '/two' + fragment);
expect(windowApi.location.originalHref)
.to.equal(removeFragment(window.location.href) + fragment);
});

it('should fail to replace URL for a wrong non-proxy origin', () => {
const fragment = '#replaceUrl=http://other.example.com/two&b=1';
setUrl('http://www.example.com/one' + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.not.be.called;
expect(windowApi.location.originalHref).to.be.undefined;
});

it('should tolerate errors when trying to replace URL', () => {
const fragment = '#replaceUrl=http://www.example.com/two&b=1';
setUrl('http://www.example.com/one' + fragment);
windowApi.history.replaceState.restore();
sandbox.stub(windowApi.history, 'replaceState', () => {
throw new Error('intentional');
});
expect(() => {
new Viewer(ampdoc);
}).to.not.throw();
expect(windowApi.location.originalHref).to.be.undefined;
});

it('should replace URL for the same source origin on proxy', () => {
const fragment =
'#replaceUrl=https://cdn.ampproject.org/c/www.example.com/two&b=1';
setUrl('https://cdn.ampproject.org/c/www.example.com/one' + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.be.calledOnce;
expect(windowApi.history.replaceState).to.be.calledWith({}, '',
'https://cdn.ampproject.org/c/www.example.com/two' + fragment);
expect(windowApi.location.originalHref)
.to.equal('https://cdn.ampproject.org/c/www.example.com/one' +
fragment);
});

it('should fail replace URL for wrong source origin on proxy', () => {
const fragment =
'#replaceUrl=https://cdn.ampproject.org/c/other.example.com/two&b=1';
setUrl('https://cdn.ampproject.org/c/www.example.com/one' + fragment);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.not.be.called;
expect(windowApi.location.originalHref).to.be.undefined;
});

it('should NOT replace URL in shadow doc', () => {
const fragment = '#replaceUrl=http://www.example.com/two&b=1';
setUrl('http://www.example.com/one' + fragment);
sandbox.stub(ampdoc, 'isSingleDoc', () => false);
new Viewer(ampdoc);
expect(windowApi.history.replaceState).to.not.be.called;
});
});

describe('should receive the visibilitychange event', () => {
it('should change prerenderSize', () => {
viewer.receiveMessage('visibilitychange', {
Expand Down