Alex Ivasyuv Reporter
	Description • 13 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

1. open http://arunranga.com/examples/access-control/simpleXSInvocation.html
2. open FB (FireBug)
3. reload page
4. click button
5. find an OK response

1. now, add credentials. type in Javascript console: url = "admin:123@" + url;
2. click button


Actual results:

Component returned failure code: 0x805e0006 [nsIXMLHttpRequest.open]
[Break On This Error] Reload the page to get source for: htt...access-control/simpleXSInvocation.html


Expected results:

Should send request, ignore credentials

	Tim (fmdeveloper)
	Comment 1 • 13 years ago

Looking at the screen shot the URL generated is admin:123@http://arunranga.com/examples/access-control/simpleXSInvocation.html - is that the correct format? Isn't that an invalid URL that causes a failure?

	Alex Ivasyuv Reporter
	Comment 2 • 13 years ago

Sorry, should be

url = "http://admin:123@aruner.net/resources/access-control-with-get/";

in this case we have 

Access to restricted URI denied
[Break On This Error] Reload the page to get source for: htt...access-control/simpleXSInvocation.html

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

Hmm.  The spec used to say that this should throw, as I recall.  Now it seems to say to just ignore the username and password here, if I read it right.  Jonas, is that correct?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 12 years ago

I haven't been following this in enough detail to remember.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 12 years ago

But flat out ignoring data always seems bad. If anyone has the bandwidth to raise this on the webapps mailing list that would be great. Throwing or producing an error seems like better solutions to me.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

Links to the changelogs for CORS and XHR2 would be good if you want me to do that; I can't find them, and I'd like to see whether there's any useful info about why the spec changed, if it did.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 12 years ago

OK, I found said links, and I think the impl here has been "wrong" all along: we treat the userpass the same no matter whether it comes from the URI or from the API call, whereas the current spec draft says they should be treated differently.

I have no idea why the spec does that, and Anne says there's a good chance the spec is bogus here in general and that he'd love some feedback on what it _should_ say. Jonas?  Mounir?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 12 years ago

I would personally prefer to treat URLs with username/password in them as invalid URLs such that we can eventually phase them out of gecko.

Alternatively we parse them but simply ignore them. Again with the goal of phasing username/password URLs out of gecko.

I believe IE has dropped support for username/password URLs, though I don't know which of the above two behaviors they have.

	Hallvord R. M. Steen [:hallvors]
	Comment 10 • 11 years ago

Test case that seems relevant:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-authentication-basic-cors.htm

Now, while I have no opinion on the user@pass: URL parts, I think we should figure out the handling of user name and password in XHR, both CORS and non-CORS requests. My preferred approach to CORS is that servers should be able to opt-in with 

Access-Control-Allow-Header: Authorization

and if script sets withCredentials to true and the server allows Authorization header, then it should just work. However, no implementations did this last time I tested, and I've not been able to convince the spec authors (myself included) that we can sanely get away with it.. The multiple "preflights" (one for CORS, another one to discover the 401 and authentication method, then the real one?!?) seemed too hacky or something..

Regarding this bug, it seems indeed that the spec currently requires a sort of silent failure: make a preflight request, pretend it failed, return empty responseText. No exceptions thrown. At least that was my reading of the spec when writing tests. If you're not comfortable with the maturity of this discussion, you may hold off working on this bug..

	Thomas Wisniewski Assignee
	Comment 11 • 8 years ago

Here's a patch which fixes this by letting async requests silently fail (use onerror) on CORS failures, rather than throwing. It triggers the non-throwing error where the spec implies it ought to be thrown, which matches Chrome's behavior. When bug 918703 lands, it will also match the spec's expected order for ProgressEvents.

A try run shows only unrelated failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef218f5abdf

On a related note, I wonder if there is a reason to not rename NS_ERROR_DOM_BAD_URI to something like NS_ERROR_DOM_CORS_FAILURE, to reflect what it actually seems to be being used for. To me, "Bad URI" doesn't really imply a security issue, so I didn't even realize it was related to CORS until recently.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 8 years ago

Comment on attachment 8771993 [details] [diff] [review]
709991-dont_throw_for_async_XHRs_with_CORS_violations.diff

As discussed in bug 918751 it's not clear to me whether it's _ever_ OK to throw if AsyncOpen2 fails for an async request.  Would it make sense to only return rv if mFlagSynchronous no matter what the value of rv is, and then do the CloseRequestWithError() whenever NS_FAILED(rv)?  In particular, what is the nsresult for failing CSP or CheckLoadURI checks?  Pretty sure it's NS_ERROR_CONTENT_BLOCKED for the former...

We should also add some web platform tests for CSP and async XHR.

Again, I can live with that as a followup if we need to, but it seems like at this point we are changing _precisely_ the code involved.

> I wonder if there is a reason to not rename NS_ERROR_DOM_BAD_URI to something like NS_ERROR_DOM_CORS_FAILURE

Well, it's it's used for a lot more than CORS failures...  As a simple example, it will get used for a blob: with a bogus UUID.

Back to the actual code change... There's a comment there talking about how we need to make sure to not dispatch these events if AsyncOpen failed.  Either the comment is wrong, in which case it would be good to understand why it's wrong now and adjust it to be correct, or it's correct and there's a problem here, right?

	Thomas Wisniewski Assignee
	Comment 13 • 8 years ago

Yeah, you're right, we might as well confirm this now. I was basing this off the spec, but was likely experiencing some tunnel-vision. The spec reads to me as though sync will always throw exceptions on network errors (which would include CORS violations), but async will only ever throw InvalidStateErrors (and those would only happen well before AsyncOpen is called, or if an HTML/XML document cannot be serialized).

Anne, am I reading that correctly? If so, my patch should be extended. If AsyncOpen fails, sync will continue to throw. Async, however, will instead effectively fire the following sequence of events and return: loadstart, readystate 4, progress, error, and loadend.

This seems to be what Chrome is doing as well.

	Anne (:annevk)
	Comment 14 • 8 years ago

Yeah, that sounds correct. (Just in case, https://xhr.spec.whatwg.org/ is what we should attempt to implement.)

	Thomas Wisniewski Assignee
	Comment 15 • 8 years ago

Alright, here's a patch that has sync XHRs continue to throw, while async ones trigger the XHR's onerror instead. A try seems okay to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b25fc5851d&selectedJob=24181671

The patch is mostly changing tests so they don't rely on exceptions being thrown, but rather listen for onerror.

Also, a couple of tests (including one web platform test) had to be changed so that they added their listeners before calling xhr.send (otherwise onerror never triggered, presumably because they were hooked up too late). I'm not 100% sure if that's their fault or a Gecko bug, but since they're not testing for that sort of thing, I'd expect these changes to be acceptable.

	Thomas Wisniewski Assignee
	Comment 16 • 8 years ago

Sorry for the spam, I just noticed that there was an unnecessary code-move in that version of the patch (the StartProgressEventTimer() lines in XMLHttpRequestMainThread.cpp). This version omits that needless change.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 8 years ago

> I'm not 100% sure if that's their fault or a Gecko bug, but since they're not testing for that sort of thing, I'd expect these changes to be acceptable.

It depends on whether web sites are depending on this behavior or not.  What do other UAs do in this situation?  It would be best to match them (and the spec; if the spec doesn't match UAs we should get that addressed too).

Why all the changes from loadend events to load events in the patch?

	Thomas Wisniewski Assignee
	Comment 18 • 8 years ago

At least with Chrome it's tough to tell, as they're currently throwing on that test like we do. They might run into the same issue once they change over. I think I'll ni? :annevk as this is a corner of the spec I'm not sure about.


>Why all the changes from loadend events to load events in the patch?

It was either checking the status in the loadend event to know if there was an error or not, or checking for load or error events instead (since loadend fires regardless of which it was). Checking onload and onerror seems a touch clearer to me, but I don't mind changing it.

	Anne (:annevk)
	Comment 19 • 8 years ago

Adding the listeners earlier should only affect uploads I believe and I don't think we ever got that interoperable. The idea was that if you added listeners early a preflight would be forced, so that upload progress would not reveal the existence of a server without that server being okay being exposed.

Does open() throw in both Chrome and Firefox today if the URL includes credentials and is cross-origin? What about Edge and Safari? If they all throw there, we could just add a check to the specification for that. You still need checks in the network layer too due to redirects (which is why the specification has all the checks there), but if everyone also has an upfront check it might not be worth the churn changing that.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 8 years ago

> The idea was that if you added listeners early a preflight would be forced

For load/error listeners?

The basic question is whether the error listener should fire in this situation:

  var xhr = new XMLHttpRequest();
  xhr.open("GET", some-blocked-uri, true);
  xhr.send();
  xhr.onerror = function() {
    // Should I get called?
  }

where some-blocked URI is blocked for whatever reasons fetch can block stuff (CORS, CSP, etc, etc).  Seems to me like in this situation the error listener should fire...

	Anne (:annevk)
	Comment 21 • 8 years ago

Yes, that should be called (it's any upload listeners and progress event listeners that are potentially problematic).

Note that we might want to invoke error events with a delay if we don't do so already: https://github.com/whatwg/fetch/issues/338.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 8 years ago

Comment on attachment 8772686 [details] [diff] [review]
709991-dont_throw_for_async_XHRs_on_network_errors.diff

Sorry for the lag here...

>+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_blobUrl.js

>+      xhr.onerror = function() { resolve("OpenErrored"); }
>+      xhr.onload = function() { resolve("OpenLoaded"); }

Preexisting confusing naming in the test, but should probably be SendErrored/SendLoaded respectively.

>+++ b/caps/tests/mochitest/test_extensionURL.html
>+        xhr.addEventListener("error", () => { ok(shouldError, "Unexpected XHR error: " + url); resolve(); });

It's not unexpected.  It's very much expected.

I would wriite that second arg of ok() as:

  `XHR to ${url} should fail`

and probably something similar for the "load" case, actually:

  `XHR to ${url} should succeed`

>+++ b/dom/security/test/mixedcontentblocker/file_main.html

The change to this file should not be needed.  More on this below.

>+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
>+      return NS_ERROR_FAILURE;

Maybe file a followup to figure out what sort of exception this should actually be throwing?  Seems like this should be throwing different exceptions depending on whether the sync XHR failed somehow or was aborted or something else, per spec.  Also per spec it's not supposed to fire error events, afaict, but I think our impl does (e.g. if you do a sync XHR and the server closes the connection on you).

Definitely followup bug fodder, though.

>+      CloseRequestWithError(ProgressEventType::error);

I think we should do this async (e.g. using a runnable method).  That would allow listeners added after send() returns to work as they should.  

>+++ b/security/manager/tools/getHSTSPreloadList.js
>+    if (!inResultList && req.status && req.readyState == 4) {

This could use a comment explaining why req.status is being tested.

>+    dump("ERROR: exception making request to " + host.name + ": " + e + "\n");

s/exception/error/ or "network error" or something?

>+++ b/testing/web-platform/tests/content-security-policy/blink-contrib/connect-src-xmlhttprequest-blocked.sub.html

Change here should not be needed.

The rest looks great.  I'd like to see the updated version, so not marking r+ yet, but this is looking good!

	Thomas Wisniewski Assignee
	Comment 23 • 8 years ago

Here's a new version of the patch with comments addressed. A fresh try run seems fine (just unrelated stuff): https://treeherder.mozilla.org/#/jobs?repo=try&revision=01716fb3c1ba

I'm not sure if a full re-review is needed, but I'll err on the side of caution and request it anyhow, since I added a runnable to defer the async events as you suggested.


>This could use a comment explaining why req.status is being tested.

I just changed that to an onload handler instead, to keep things concise.


>Maybe file a followup to figure out what sort of exception this should actually be throwing?

Yes, I'll file a followup. Based on what I'm reading, in the sync case the spec requires throwing only for aborts and timeouts, and otherwise just silently setting the response to a network error.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 8 years ago

> Based on what I'm reading, in the sync case the spec requires throwing only
> for aborts and timeouts, and otherwise just silently setting the response
> to a network error.

Not quite.  In the sync case, we start in https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send and get to step 10.  During the fetch, the response does get set to a network error.  Per https://fetch.spec.whatwg.org/#concept-network-error this always has a null body.  So in send()'s step 10 substep 2 we jump to https://xhr.spec.whatwg.org/#handle-response-end-of-body

Now in https://xhr.spec.whatwg.org/#handle-response-end-of-body step 2 we jump to https://xhr.spec.whatwg.org/#handle-errors and in step 2 see response is a network error, so jump to https://xhr.spec.whatwg.org/#request-error-steps with "exception" set to NetworkError.

In https://xhr.spec.whatwg.org/#request-error-steps step 4, the synchronous flag is set, so we throw a NetworkError.

Yeah, this is not exactly the most obvious control flow.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 8 years ago

Comment on attachment 8775377 [details] [diff] [review]
709991-dont_throw_for_async_XHRs_on_network_errors.diff

Why did you stop assigning to xhr.onabort and xhr.ontimeout in browser/modules/ContentSearch.jsm?  Doing that seems like the right thing to do to preserve the old semantics...

>+class nsDeferCloseRequestWithError : public Runnable

No, I was pretty serious about using a runnable method.  Like so, at the callsite:

  NS_DispatchToCurrentThread(
    NewRunnableMethod(this, &XMLHttpRequestMainThread::CloseRequestWithError,
                      ProgressEventType::error));

Does that not work for some reason?

r=me on the rest of it, but would like to understand what's going on with these two issues.

	Thomas Wisniewski Assignee
	Comment 26 • 8 years ago

>Why did you stop assigning to xhr.onabort and xhr.ontimeout in browser/modules/ContentSearch.jsm?

You're right, I must have been thinking unclearly at the time. I'll adjust the tests using onload/onerror so they also account for onabort/ontimeout too.

>No, I was pretty serious about using a runnable method.  Like so, at the callsite:

I just didn't realize that was an option. I'll give it a shot, thanks!

>Yeah, this is not exactly the most obvious control flow.  :(

It also wouldn't help that I wasn't quite thinking clearly at the time :)

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 8 years ago

onabort/ontimeout is not a big deal in the tests because they don't set a timeout and don't abort their xhrs, and run in a controlled environment.  That's why I didn't comment on using onabort/ontimeout in the tests.

ContentSearch.jsm, on the other hand, can run in a browser with extensions that do stuff like add a timeout to every XHR or some such thing, and should be robust to that.

	Thomas Wisniewski Assignee
	Comment 28 • 8 years ago

I very much agree.

Here's an updated patch addressing those two outstanding issues (NewRunnableMethod and the onerror/ontimeout/onabort issue with ContentSearch.jsm).

Carrying over r+ and requesting check-in.

	Pulsebot
	Comment 29 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ddf661a7ee
Fire onerror instead of throwing on network errors for async XHRs. r=bz

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/59ddf661a7ee

	Kohei Yoshino [:kohei]
	Comment 31 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/network-error-for-async-xhr-now-fires-error-event-instead-of-throwing/

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 32 • 8 years ago

The changes in https://hg.mozilla.org/mozilla-central/rev/59ddf661a7ee caused getHSTSPreloadList.js to stop working properly (see bug 1298056). (Unfortunately no one noticed at the time because it was already broken due to bug 1295268.) For these kinds of changes, I realize it's a bit of a judgement call to know whether or not to loop in an extra reviewer for each area of code affected, but in the case of code under security/, it would probably be best to err on the side of getting that extra sign-off.

	Jean-Yves Perrier [:teoli]
	Comment 33 • 8 years ago

I've not found a mention of the old behaviour, and to use error/onerror is the logical way, so I merely indicated an info in 
https://developer.mozilla.org/en-US/Firefox/Releases/50#DOM_and_HTML_DOM