Skip to content

Conversation

benvinegar
Copy link
Contributor

@benvinegar
Copy link
Contributor Author

benvinegar commented Apr 21, 2016

One downside of this integration test – it will attempt to request from http://example.com/api/1/store, but it fails because XHR from file:/// (where tests are served) cannot be initiated to http://. So no XHR actually takes place, but there is a warning message when running tests:

XMLHttpRequest cannot load https://example.com/api/1/store/. Origin file:// is not allowed by Access-Control-Allow-Origin.

I'd stub this but ... the purpose of these integration tests is to be as true-to-life as possible, and should attempt actual XHRs. I wonder if I can just cancel/abort it in such a way that no attempt is made.

src/raven.js Outdated
};

// intentionally checking against 0 here (start of string)
if (url.indexOf(self._globalEndpoint) !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't ever match since we make _globalEndpoint protocol relative inside of _getGlobalServer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or rather, I'm operating under the assumption here that the url for the http request got normalized and added the protocol. Maybe that's a bad assumption?

Copy link
Contributor Author

@benvinegar benvinegar Apr 21, 2016

Choose a reason for hiding this comment

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

What if we just did a substring match then? e.g. url.indexOf(self._globalEndpoint !== -1)

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 could also use parseUrl and verify that host name and path match, omitting protocol.

Copy link
Contributor

Choose a reason for hiding this comment

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

What abotu checking for the public key anywhere int he url? That'd only exist in a url if it were bound for the sentry server since it should effectively be a UUID anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a decent idea, but there's the possibility that somebody might somehow construct a URL to their own servers with the key ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, the same argument can be said about the checking for the presence of _globalEndpoint in a request bound for their servers.

If we want to be precise, we should validate the actual hostname, etc. It'd be marginally more expensive since doing this validation on every XHR request. So tradeoffs.

imo it's unlikely for someone to be kicking off requests to their own servers with the public key in the url, but it's your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm. Let's go with looking for the token. I'd rather keep the impact super minimal and stick with indexOf if we can.

@benvinegar benvinegar merged commit 9a374f4 into master Apr 22, 2016
@benvinegar benvinegar deleted the no-sentry-xhrs branch April 22, 2016 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants