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.

Sign up for GitHub

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

Do not XHR intercept first party (1P) CDN resources #16650

Merged
merged 6 commits into from
Jul 11, 2018
Merged

Do not XHR intercept first party (1P) CDN resources #16650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe819fe
do not intercept 1p cdn resources
alabiaga Jul 10, 2018
eabe973
fix test
alabiaga Jul 10, 2018
3515cfc
address choumx comment
alabiaga Jul 10, 2018
02371d1
address comments
alabiaga Jul 11, 2018
fdaf519
fix unit test
alabiaga Jul 11, 2018
2f84663
revert change
alabiaga Jul 11, 2018
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
Next Next commit
do not intercept 1p cdn resources
  • Loading branch information
@alabiaga
alabiaga committed Jul 11, 2018
commit fe819fe127f5e3c4eba20d1cab3e77279d58037c
5 changes: 4 additions & 1 deletion src/service/xhr-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {getService, registerServiceBuilder} from '../service';
import {isArray, isObject} from '../types';
import {isFormDataWrapper} from '../form-data-wrapper';
import {parseJson} from '../json';
import {urls} from '../config';
import {utf8Encode} from '../utils/bytes';

/**
Expand Down Expand Up @@ -166,6 +167,7 @@ export class Xhr {
*
* XHRs are intercepted if all of the following are true:
* - The AMP doc is in single doc mode
* - The requested resource is not a 1p request.
* - The viewer has the `xhrInterceptor` capability
* - The Viewer is a trusted viewer or AMP is currently in developement mode
* - The AMP doc is opted-in for XHR interception (`<html>` tag has
Expand All @@ -185,7 +187,8 @@ export class Xhr {
}
const viewer = Services.viewerForDoc(this.ampdocSingle_);
const whenFirstVisible = viewer.whenFirstVisible();
if (!viewer.hasCapability('xhrInterceptor')) {
const firstPartyResource = urls.cdnProxyRegex.test(new URL(input).origin);
Copy link

Choose a reason for hiding this comment

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

URL is not supported everywhere. Use our helper functions in url.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. thanks.

if (firstPartyResource || !viewer.hasCapability('xhrInterceptor')) {
return whenFirstVisible;
}
const htmlElement = this.ampdocSingle_.getRootNode().documentElement;
Expand Down
62 changes: 41 additions & 21 deletions test/functional/test-xhr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,13 +890,33 @@ describe.configure().skipSafari().run('XHR', function() {
.then(() => expect(sendMessageStub).to.not.have.been.called);
});

it('should not intercept a 1p cdn from subdomain', () => {
sandbox.stub(viewer, 'isTrustedViewer').returns(Promise.resolve(true));
interceptionEnabledWin.AMP_DEV_MODE = true;

const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://subdomain-model.cdn.ampproject.org/ww.js')
.then(() => expect(sendMessageStub).to.not.have.been.called);
});

it('should not intercept a 1p cdn resource', () => {
sandbox.stub(viewer, 'isTrustedViewer').returns(Promise.resolve(true));
interceptionEnabledWin.AMP_DEV_MODE = true;

const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org/ww.js')
.then(() => expect(sendMessageStub).to.not.have.been.called);
});

it('should intercept if viewer untrusted but dev mode', () => {
sandbox.stub(viewer, 'isTrustedViewer').returns(Promise.resolve(false));
interceptionEnabledWin.AMP_DEV_MODE = true;

const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org')
return xhr.fetch('https://www.some-url.org/some-resource/')
.then(() => expect(sendMessageStub).to.have.been.called);
});

Expand All @@ -906,14 +926,14 @@ describe.configure().skipSafari().run('XHR', function() {

const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org')
return xhr.fetch('https://www.some-url.org/some-resource/')
.then(() => expect(sendMessageStub).to.have.been.called);
});

it('should send viewer message named `xhr`', () => {
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org')
return xhr.fetch('https://www.some-url.org/some-resource/')
.then(() =>
expect(sendMessageStub).to.have.been.calledWithMatch(
'xhr', sinon.match.any));
Expand All @@ -922,12 +942,12 @@ describe.configure().skipSafari().run('XHR', function() {
it('should post correct structurally-cloneable GET request', () => {
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org')
return xhr.fetch('https://www.some-url.org/some-resource/')
.then(() =>
expect(sendMessageStub).to.have.been.calledWithMatch(
sinon.match.any, {
originalRequest: {
input: 'https://cdn.ampproject.org' +
input: 'https://www.some-url.org/some-resource/' +
'?__amp_source_origin=https%3A%2F%2Facme.com',
init: {
headers: {},
Expand All @@ -942,7 +962,7 @@ describe.configure().skipSafari().run('XHR', function() {
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr
.fetch('https://cdn.ampproject.org', {
.fetch('https://www.some-url.org/some-resource/', {
method: 'POST',
headers: {'Content-Type': 'application/json;charset=utf-8'},
body: JSON.stringify({a: 42, b: [24, true]}),
Expand All @@ -951,7 +971,7 @@ describe.configure().skipSafari().run('XHR', function() {
expect(sendMessageStub).to.have.been.calledWithMatch(
sinon.match.any, {
originalRequest: {
input: 'https://cdn.ampproject.org' +
input: 'https://www.some-url.org/some-resource/' +
'?__amp_source_origin=https%3A%2F%2Facme.com',
init: {
headers: {
Expand All @@ -975,15 +995,15 @@ describe.configure().skipSafari().run('XHR', function() {
formData.append('b', true);

return xhr
.fetch('https://cdn.ampproject.org', {
.fetch('https://www.some-url.org/some-resource/', {
method: 'POST',
body: formData,
})
.then(() =>
expect(sendMessageStub).to.have.been.calledWithMatch(
sinon.match.any, {
originalRequest: {
input: 'https://cdn.ampproject.org' +
input: 'https://www.some-url.org/some-resource/' +
'?__amp_source_origin=https%3A%2F%2Facme.com',
init: {
headers: {
Expand All @@ -1002,23 +1022,23 @@ describe.configure().skipSafari().run('XHR', function() {
sendMessageStub.returns(Promise.resolve());
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return expect(xhr.fetch('https://cdn.ampproject.org'))
return expect(xhr.fetch('https://www.some-url.org/some-resource/'))
.to.eventually.be.rejectedWith(Error, 'Object expected: undefined');
});

it('should be rejected when response null', () => {
sendMessageStub.returns(Promise.resolve(null));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return expect(xhr.fetch('https://cdn.ampproject.org'))
return expect(xhr.fetch('https://www.some-url.org/some-resource/'))
.to.eventually.be.rejectedWith(Error, 'Object expected: null');
});

it('should be rejected when response is string', () => {
sendMessageStub.returns(Promise.resolve('response text'));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return expect(xhr.fetch('https://cdn.ampproject.org')).to.eventually
return expect(xhr.fetch('https://www.some-url.org/some-resource/')).to.eventually
.be.rejectedWith(Error, 'Object expected: response text');
});

Expand All @@ -1041,7 +1061,7 @@ describe.configure().skipSafari().run('XHR', function() {
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org').then(response => {
return xhr.fetch('https://www.some-url.org/some-resource/').then(response => {
expect(response.headers.get('a')).to.equal('2');
expect(response.headers.get('b')).to.equal('false');
expect(response.headers.get('Amp-Access-Control-Allow-source-origin'))
Expand All @@ -1067,7 +1087,7 @@ describe.configure().skipSafari().run('XHR', function() {
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetchDocument('https://cdn.ampproject.org').then(doc => {
return xhr.fetchDocument('https://www.some-url.org/some-resource/').then(doc => {
expect(doc).to.have.nested.property('body.textContent')
.that.equals('Foo');
});
Expand Down Expand Up @@ -1116,15 +1136,15 @@ describe.configure().skipSafari().run('XHR', function() {
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetchDocument('https://cdn.ampproject.org')
return xhr.fetchDocument('https://www.some-url.org/some-resource/')
.then(doc => expect(doc.body.textContent).to.equal('Foo'));
});

it('should return default response when body/init missing', () => {
sendMessageStub.returns(Promise.resolve({}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
expect(response.headers.get('a')).to.be.null;
expect(response.headers.has('a')).to.be.false;
Expand All @@ -1138,7 +1158,7 @@ describe.configure().skipSafari().run('XHR', function() {
sendMessageStub.returns(Promise.resolve({body: '', init: {}}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
expect(response.headers.get('a')).to.be.null;
expect(response.headers.has('a')).to.be.false;
Expand All @@ -1150,7 +1170,7 @@ describe.configure().skipSafari().run('XHR', function() {
sendMessageStub.returns(Promise.resolve({body: 32}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
return expect(response.text()).to.eventually.equal('32');
});
Expand All @@ -1160,7 +1180,7 @@ describe.configure().skipSafari().run('XHR', function() {
sendMessageStub.returns(Promise.resolve({init: {status: '209.6'}}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
return expect(response).to.have.property('status')
.that.equals(209);
Expand All @@ -1176,7 +1196,7 @@ describe.configure().skipSafari().run('XHR', function() {
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
expect(response.headers.get(1)).to.equal('true');
expect(response.headers.get('false')).to.equal('NaN');
Expand All @@ -1197,7 +1217,7 @@ describe.configure().skipSafari().run('XHR', function() {
}));
const xhr = xhrServiceForTesting(interceptionEnabledWin);

return xhr.fetch('https://cdn.ampproject.org', {ampCors: false})
return xhr.fetch('https://www.some-url.org/some-resource/', {ampCors: false})
.then(response => {
expect(response.headers.get('content-type'))
.to.equal('text/plain');
Expand Down