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

(#4189) - better CORS error message #4190

Closed
wants to merge 1 commit into from
Closed

(#4189) - better CORS error message #4190

wants to merge 1 commit into from

Conversation

nolanlawson
Copy link
Member

This adds a console error for browser users when we detect CORS is unavailable, and it also uses a custom 405 error instead of "UnknownError".

Here's what it looks like in Chrome:

screenshot from 2015-08-23 15 15 03

And in Firefox:

screenshot from 2015-08-23 15 19 22

@NickColley
Copy link
Contributor

This is such a good change, thanks a lot @nolanlawson . It's going to help so many people. 💃

@nolanlawson
Copy link
Member Author

Cripes, it would have helped me. :) I was scratching my head over this bug for a good 20 minutes recently, before I realized I forgot to enable CORS. 😛

@nolanlawson
Copy link
Member Author

This PR breaks Firefox and I cannot reproduce.

@nolanlawson
Copy link
Member Author

Looks like Travis is using an older version of Firefox that still has that blob+ajax bug. I think this is a good time to just simplify blobSupport.js with a user agent sniff. Only Firefox has the blob+ajax bug, and only Chrome has the blob+idb bug, so we might as well do the shameful, shameful, user agent sniff and not have to futz around with this stuff.

@nolanlawson
Copy link
Member Author

Actually, it just occurred to me that that can't possibly be the issue.

I think I'm just going to scale back the ambition of this PR and make it only log a warning, not try to refactor the error code.

@nolanlawson
Copy link
Member Author

Green! :)

@daleharvey
Copy link
Member

Nice, ac56d71

@daleharvey daleharvey closed this Aug 30, 2015
@darobin
Copy link

darobin commented Sep 2, 2015

There's a problem with this change: statusCode can be set to 0 in several ways. Most notably, aborting the request (for instance because of a timeout) will also cause it to be set to 0. In fact, any network error will look like this.

CORS issues are indistinguishable from other network problems by design. You simply can't really check for this. I'm afraid this change needs to be backed out.

For context, I just spent 20min looking at the code and trying to figure out why I was getting a CORS error for a request that wasn't even cross-domain :)

@NickColley NickColley reopened this Sep 2, 2015
@daleharvey
Copy link
Member

The vast majority of our users come up against statusCode 0 due to cors, the wording could be a little bit clearer with some bikeshedding, but this doesnt need backed out

@darobin
Copy link

darobin commented Sep 2, 2015

Sure, that works too. Note that in the specific instance that I've bumped into the abort() came from https://github.com/pouchdb/pouchdb/blob/master/lib/deps/ajax/request-browser.js#L113 which as far as I can tell can only happen in a timeout. A specific error there might be useful (it seems to be the case if fetch is used).

@nolanlawson
Copy link
Member Author

Agree we should change the error message to say something like

do you see something about Access-Control-Allow-Origin in your console? If so, you have a CORS error

@NickColley
Copy link
Contributor

Is it possible to detect that header and only show the message if it isn't there?

@nolanlawson
Copy link
Member Author

Apparently no. I think it makes more sense to soften the warning message. Or just remove it.

For the record, to everybody saying they wasted time because of this message: I apologize, but keep in mind we've been hearing for years that new users forget to enable CORS, and spend a lot of time hunting down the error.

@daleharvey
Copy link
Member

Being that this is a PR with code against it, fairly confusing to have it open, lets close this and reopen the issue filed - #4256

@daleharvey daleharvey closed this Sep 3, 2015
@darobin
Copy link

darobin commented Sep 3, 2015

@nolanlawson FWIW no need to apologise, you tried to solve a real problem, nothing wrong with that. It's unfortunate that the Web's security model relies on hiding this sort of information.

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.

None yet

4 participants