Skip to content

Use XHR + POST for request if CORS supported #420

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

Merged
merged 2 commits into from
Dec 28, 2015
Merged

Use XHR + POST for request if CORS supported #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7429333
Use XHR + POST for request if CORS supported
benvinegar Nov 11, 2015
4de0c94
Use protocol relative if dsn targets app.getsentry.com
benvinegar Nov 12, 2015
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
48 changes: 46 additions & 2 deletions src/raven.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/*global XDomainRequest:false*/
'use strict';

// First, check for JSON support
Expand Down Expand Up @@ -105,7 +106,9 @@ var Raven = {
(uri.port ? ':' + uri.port : '') +
'/' + path + 'api/' + globalProject + '/store/';

if (uri.protocol) {
// can safely use protocol relative (//) if target host is
// app.getsentry.com; otherwise use protocol from DSN
if (uri.protocol && uri.host !== 'app.getsentry.com') {
globalServer = uri.protocol + ':' + globalServer;
}

Expand Down Expand Up @@ -840,7 +843,7 @@ function send(data) {
});
}

function makeRequest(opts) {
function makeImageRequest(opts) {
// Tack on sentry_data to auth options, which get urlencoded
opts.auth.sentry_data = JSON.stringify(opts.data);

Expand All @@ -856,6 +859,47 @@ function makeRequest(opts) {
img.src = src;
}

function makeXhrRequest(opts) {
var request;

function handler() {
if (request.status === 200) {
if (opts.onSuccess) {
opts.onSuccess();
}
} else if (opts.onError) {
opts.onError();
}
}

request = new XMLHttpRequest();
if ('withCredentials' in request) {
request.onreadystatechange = function () {
if (request.readyState !== 4) {
return;
}
handler();
};
} else {
request = new XDomainRequest();
// onreadystatechange not supported by XDomainRequest
request.onload = handler;
}

// NOTE: auth is intentionally sent as part of query string (NOT as custom
// HTTP header) so as to avoid preflight CORS requests
request.open('POST', opts.url + '?' + urlencode(opts.auth));
request.send(JSON.stringify(opts.data));
}

function makeRequest(opts) {
var hasCORS =
'withCredentials' in new XMLHttpRequest() ||
typeof XDomainRequest !== 'undefined';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert here, so lemme get this straight:

  • You're checking for either XMLHttpRequest that has the proper support. Is XMLHttpRequest guaranteed to exist? I assume if it's undefined, we'll have an issue calling new XMLHttpRequest()
  • Then falling back to checking for XDomainRequest, which is IE specific. This is great, but then inside makeXhrRequest, XDomainRequest isn't used or even attempted to be used. So it's not clear to me what good this check is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is XMLHttpRequest guaranteed to exist?

Realistically – yes, unless someone has removed it (e.g. the host page or a browser extension).

But I'll add the check.

Then falling back to checking for XDomainRequest, which is IE specific. This is great, but then inside makeXhrRequest, XDomainRequest isn't used or even attempted to be used. So it's not clear to me what good this check is doing.

Yep, that was a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Realistically – yes, unless someone has removed it (e.g. the host page or a browser extension)

What about the IE case though since they use XDomainRequest? Or do I misunderstand this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the IE case though since they use XDomainRequest?

The XMLHttpRequest object still exists, and can be safely queried for presence of withCredentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. I see. IE's XMLHttpRequest object just isn't capable of issuing a CORS request.


return (hasCORS ? makeXhrRequest : makeImageRequest)(opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just going to assume that this is going to be problematic to fall back to the image request, because the goal of XHR is to be able to transmit more data. And if we attempt to fall back, it's unlikely things are even going to work.

So I guess we need to make a decision here: do we just 100% remove the image transport, and assume if there's no CORS support, maybe we just ignore entirely?

/cc @dcramer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to have worked well so far? Or is the plan to increase the amount of data going forward?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we want to start sending more.

Or at least, not be so paranoid about it. See: #419 as one thing.

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'd rather we got notified of an issue, even with partial information due to data constraints, rather than have it disappear into the ether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One idea: we could deploy the fallback, see if we get any actual hits from it, to assess its usefulness.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. So we'll have to implement the stuff to make sure we trim our data packet before sending to make an attempt.

Otherwise, we'll just get a 413 on the response and Sentry won't log it anyways.

So not sure the best solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we'll have to implement the stuff to make sure we trim our data packet before sending to make an attempt.

Let's do that when we increase the stack size – not here.

}

// Note: this is shitty, but I can't figure out how to get
// sinon to stub document.createElement without breaking everything
// so this wrapper is just so I can stub it for tests.
Expand Down
90 changes: 84 additions & 6 deletions test/raven.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,84 @@ describe('globals', function() {
});

describe('makeRequest', function() {
beforeEach(function() {
// use fake xml http request so we can muck w/ its prototype
this.xhr = sinon.useFakeXMLHttpRequest();
this.sinon.stub(window, 'makeImageRequest');
this.sinon.stub(window, 'makeXhrRequest');
});

afterEach(function() {
this.xhr.restore();
});

it('should call makeXhrRequest if CORS is supported', function () {
XMLHttpRequest.prototype.withCredentials = true;

makeRequest({
url: 'http://localhost/',
auth: {a: '1', b: '2'},
data: {foo: 'bar'},
options: globalOptions
});

assert.isTrue(makeImageRequest.notCalled);
assert.isTrue(makeXhrRequest.calledOnce);
});

it('should call makeImageRequest if CORS is NOT supported', function () {
delete XMLHttpRequest.prototype.withCredentials;

var oldXDR = window.XDomainRequest;
window.XDomainRequest = undefined;

makeRequest({
url: 'http://localhost/',
auth: {a: '1', b: '2'},
data: {foo: 'bar'},
options: globalOptions
});

assert.isTrue(makeImageRequest.calledOnce);
assert.isTrue(makeXhrRequest.notCalled);

window.XDomainRequest = oldXDR;
});
});

describe('makeXhrRequest', function() {
beforeEach(function() {
// NOTE: can't seem to call useFakeXMLHttpRequest via sandbox; must
// restore manually
this.xhr = sinon.useFakeXMLHttpRequest();
var requests = this.requests = [];

this.xhr.onCreate = function (xhr) {
requests.push(xhr);
};
});

afterEach(function() {
this.xhr.restore();
});

it('should create an XMLHttpRequest object with body as JSON payload', function() {
XMLHttpRequest.prototype.withCredentials = true;

makeXhrRequest({
url: 'http://localhost/',
auth: {a: '1', b: '2'},
data: {foo: 'bar'},
options: globalOptions
});

var lastXhr = this.requests[this.requests.length - 1];
assert.equal(lastXhr.requestBody, '{"foo":"bar"}');
assert.equal(lastXhr.url, 'http://localhost/?a=1&b=2');
});
});

describe('makeImageRequest', function() {
var imageCache;

beforeEach(function () {
Expand All @@ -1346,7 +1424,7 @@ describe('globals', function() {
})

it('should load an Image', function() {
makeRequest({
makeImageRequest({
url: 'http://localhost/',
auth: {a: '1', b: '2'},
data: {foo: 'bar'},
Expand All @@ -1360,7 +1438,7 @@ describe('globals', function() {
globalOptions = {
crossOrigin: 'something',
};
makeRequest({
makeImageRequest({
url: globalServer,
auth: {lol: '1'},
data: {foo: 'bar'},
Expand All @@ -1374,7 +1452,7 @@ describe('globals', function() {
globalOptions = {
crossOrigin: ''
};
makeRequest({
makeImageRequest({
url: globalServer,
auth: {lol: '1'},
data: {foo: 'bar'},
Expand All @@ -1388,7 +1466,7 @@ describe('globals', function() {
globalOptions = {
crossOrigin: false
};
makeRequest({
makeImageRequest({
url: globalServer,
auth: {lol: '1'},
data: {foo: 'bar'},
Expand Down Expand Up @@ -2043,11 +2121,11 @@ describe('Raven (public API)', function() {

it('should work as advertised #integration', function() {
var imageCache = [];
this.sinon.stub(window, 'newImage', function(){ var img = {}; imageCache.push(img); return img; });
this.sinon.stub(window, 'makeRequest');

setupRaven();
Raven.captureMessage('lol', {foo: 'bar'});
assert.equal(imageCache.length, 1);
assert.equal(window.makeRequest.callCount, 1);
// It'd be hard to assert the actual payload being sent
// since it includes the generated url, which is going to
// vary between users running the tests
Expand Down