Open Bug 1254382 Opened 8 years ago Updated 2 years ago

Investigate CORS test failures in testing/web-platform/tests/cors/preflight-cache.htm

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

People

(Reporter: khuey, Unassigned)

Details

(Whiteboard: [tw-dom] btpp-active) 

There's a bunch of tests here that we don't pass, so we should make sure the tests are correct per spec, and then figure out why we don't pass them.
Whiteboard: [tw-dom] → [tw-dom] btpp-fixlater
remote-origin.htm

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Fail	Disallow origin: http://www1.web-platform.test:8000\0	assert_equals: expected "error" but got "load"

reverseOrigin/t.callback<@http://web-platform.test:8000/cors/remote-origin.htm:52:13
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1422:20
@http://web-platform.test:8000/cors/remote-origin.htm:115:5
EventListener.handleEvent*@http://web-platform.test:8000/cors/remote-origin.htm:114:1


++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++


Fail	Disallow origin: *\0	assert_equals: expected "error" but got "load"

reverseOrigin/t.callback<@http://web-platform.test:8000/cors/remote-origin.htm:52:13
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1422:20
@http://web-platform.test:8000/cors/remote-origin.htm:115:5
EventListener.handleEvent*@http://web-platform.test:8000/cors/remote-origin.htm:114:1
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Unexpected results: 8 (FAIL: 8)

Unexpected Results
==================

/cors/allow-headers.htm
-----------------------
FAIL Disallow origin: http://web-platform.test:8000\0
FAIL Disallow origin: *\0
/cors/late-upload-events.htm
----------------------------
FAIL Late listeners: Preflight
/cors/origin.htm
----------------
FAIL Disallow origin: http://web-platform.test:8000\0
FAIL Disallow origin: *\0
/cors/preflight-cache.htm
-------------------------
FAIL preflight for x-print should be cached
/cors/remote-origin.htm
-----------------------
FAIL Disallow origin: http://www1.web-platform.test:8000\0
FAIL Disallow origin: *\0
(In reply to Shawn Huang [:shawnjohnjr] from comment #1)
> remote-origin.htm
> 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++
> Fail	Disallow origin: http://www1.web-platform.test:8000\0	assert_equals:
> expected "error" but got "load"

It fails at "shouldFail("<origin>" + "\0")"


> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++ 
> Fail	Disallow origin: *\0	assert_equals: expected "error" but got "load"
> 

It fails at "shouldFail("*\0")"


I guess it might be related to url parser.
(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> Fail	Disallow origin: http://web-platform.test:8000\0	assert_throws: send
> function "function () { client.send() }" did not throw

> Fail	Disallow origin: *\0	assert_throws: send function "function () {
> client.send() }" did not throw
This looks like the same reason since XMLHttpRequest client has onload.
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom] btpp-active
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> testing/web-platform/tests/cors/late-upload-events.htm
> 
> 
> Fail	Late listeners: Preflight	assert_true: Events did fire expected true
> got false

It does not fire all expected events for XHR upload. I checked Bug 908375, maybe that's the starting point. Maybe it's related to AllowUploadProgress.
(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> > testing/web-platform/tests/cors/late-upload-events.htm
> > 
> > 
> > Fail	Late listeners: Preflight	assert_true: Events did fire expected true
> > got false
> 
> It does not fire all expected events for XHR upload. I checked Bug 908375,
> maybe that's the starting point. Maybe it's related to AllowUploadProgress.

Ok, so this test case on purpose tries to test late upload event registration.I'm wondering how we're going to fix it.
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> > > testing/web-platform/tests/cors/late-upload-events.htm
> > > 
> > > 
> > > Fail	Late listeners: Preflight	assert_true: Events did fire expected true
> > > got false
Per spec:
In XMLHttpRequest progress events are dispatched only after the cross-origin request status is set to success. Upload progress events are only dispatched once the cross-origin request status is preflight complete.
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> testing/web-platform/tests/cors/late-upload-events.htm
> 
> 
> Fail	Late listeners: Preflight	assert_true: Events did fire expected true
> got false

I think I still have doubts about this test case. Is this a valid test case? I tested on chrome and opera, both of them fail.
The following sequence of events and return: loadstart, progress, load, readyState: 2, readyState: 4, then call send() but no upload event. For async send, it's possible that the event fired before event listener registered.
I'm wondering what's the preferred behavior of the following conditions:
https://github.com/w3c/web-platform-tests/blob/master/cors/late-upload-events.htm#L34-L40

        client.send((new Array(3000)).join('xo'));
        client.upload.onprogress = client.upload.onloadend = client.upload.onloadstart = client.upload.onload = this.step_func(function(e) {
            eventCounter++;
            if (!expectEvents) {
                assert_unreached("Upload events should not fire, but did: " + e.type);
            }
        });

The test case seems expect all the upload progress events will be received. Checking XMLHttpRequestMainThread [1], it looks we dispatch progress event in Send(), so events are missing. Do we expect events will be received in this case? It doesn't seem to be very clear in the XHR specification. Could you provide some suggestion for this test case?


http://searchfox.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2841
Flags: needinfo?(bzbarsky)
Per spec, there should be a progress event on the upload object here that fires after upload.onprogress is set.  We start in https://xhr.spec.whatwg.org/#the-send()-method and get down to step 9 which does the fetch.  The "synchronous flag" is unset, so we start running the substeps.  Substep 9.3 does a fetch, and the "process request end-of-body" for that fetch has a step 4 that says "Fire a progress event named progress on the XMLHttpRequestUpload object with transmitted and length."

OK, so when does "process request end-of-body" run?  It runs off a task queued by fetch; see <https://fetch.spec.whatwg.org/#dfnReturnLink-0>.  So it should happen async, after the send() call has returned and JS has run to completion.

The link you give, to <http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/dom/xhr/XMLHttpRequestMainThread.cpp#2841>, is firing an event named "loadstart", not an event named "progress".  That corresponds to https://xhr.spec.whatwg.org/#the-send()-method substep 9.1, which is in fact supposed to happen before send() returns.

Anyway, back to our implementation, we should end up in XMLHttpRequestMainThread::OnStartRequest which should fire the relevant "progress" event and doesn't seem to.  That is, it fires a "progress" event on mUpload if mUploadTransferred < mUploadTotal, but otherwise it sets mUploadComplete and fires "load" on mUpload.  Arguably, it should do MaybeDispatchProgressEvents(true) even if mUploadTransferred >= mUploadTotal...

Looks like you're running into bug 918703.  Hopefully that can just land soon... but the patches in that bug don't remove the annotation from this testcase, which is a bit surprising to me.
Flags: needinfo?(bzbarsky)
Yes, it turns out that my patches in bug 918703 can probably fix this test as well, if I defer some checks and events until after send() (like I recently did with a runnable method in bug 709991).

I was actually hoping to land that patch next, after I confirm whether a couple of patches I wrote for bug 1285036 stick their landing, so I'll work out the details in 709991 and report back here ASAP.
Actually, upon closer investigation I think that the second test in that file is nonsensical (or maybe the spec is off).

Consider that in the spec, during the send() method in step 5*, we must perform a CORS preflight if there are any event listeners on the upload object AT THAT TIME.

But there can be no listeners on the object at that point, because they're supposed to be set "late" (that is, after send() is called).

So how can the test expect a preflight to happen, if it hasn't attached appropriate event listeners before calling send?


* https://xhr.spec.whatwg.org/#the-send()-method
Hmm...

You're right that the test is weird.  It does this:

  doTest("No preflight", {}, false);
  doTest("Preflight", {"custom-header":"test"}, true);

where the third arg indicates whether there should be upload events.  But the server responds positively to the preflight, so I'd expect both cases to fire upload events, no?  The test doesn't actually test whether there is a preflight or not, afaict....

Note that the "Preflight" case expects a preflight to happen because it set custom headers, not because it added listeners.

I agree that it's weird that if the progress listeners are added after send() we will skip the preflight but still fire progress events, per current spec.  Anne, what's up with that?
Flags: needinfo?(annevk)
bz, https://www.w3.org/Bugs/Public/show_bug.cgi?id=20322 never got resolved. Input appreciated.
Flags: needinfo?(annevk)
I don't have any particular input.  I just think the current setup in the spec is clearly broken from a security perspective, because it doesn't prevent the thing it's trying to prevent.  Whether the answer is to stop preventing it or to fix the prevention is a question for rbarnes, probably.  You may want to ping him via direct email.

It's not clear to me whether our impl is also broken from a security perspective, but cursory examination of the code doesn't show me how it could possibly _not_ be broken.  Maybe I'm missing something?
Flags: needinfo?(rlb)
I discussed a bit with rbarnes on IRC (#security, but it's not logged I think). Neither of us is entirely sure, but upload events give time-to-connection-establishment to the page. Coupled with time-to-end-of-response-body that could leak the size of a resource. That means upload events should be guarded with a CORS-preflight.

The next step is deciding what logic to use, which is where we got stuck in the standards discussion.

I still think that the simplest thing to do here would be to require listeners to be added before send() is invoked. And only if that is the case will we dispatch progress events (and perform a preflight as needed).

Making that logic dependent on whether or not the request is same-origin would require careful coordination with the Fetch layer, which seems rather spurious, especially as we encourage folks to use fetch() anyway.
Flags: needinfo?(rlb)
> I still think that the simplest thing to do here would be to require
> listeners to be added before send() is invoked. And only if that is 
> the case will we dispatch progress events (and perform a preflight 
> as needed).

This seems sensible to me.  

(BTW, I *think* I've gotten bugmail fixed, so n-i? should work OK)
We could be a little smarter perhaps and only have Fetch queue tasks (that lead to the events) for same-origin, and cross-origin when there's been a preflight. We'd still use the signal of event listeners being registered before send() as to whether or not to force a preflight.
Having looked closer I now recall why we made the test in question.

> So how can the test expect a preflight to happen, if it hasn't attached appropriate event listeners before calling send?

It expects a preflight to happen because it sets a custom request header (guarantees a preflight). It then tests if you get upload progress events (since if there's been a preflight it would be safe to dispatch them for the actual request). However, implementations seem to require early listener registration.
Ah, I see. So if we wanted to allow late registration of listeners, we'd have to change the XHR spec in step 5 to something like this:

  use-CORS-preflight flag
    Set if one or more event listeners are registered on the associated XMLHttpRequestUpload object, or if any custom headers have been set.


(And adding any other such criteria we may have missed).
It seems even for same-origin fetches all browsers (minus Edge, didn't test) require the upload listeners to be added early. Maybe that's better as it makes the programming model consistent. It's a little weird though and given the number of timing attacks that exist against servers I'm no longer really sure why we require a preflight to begin with.

Anyway, it seems like I should update the specification to require listeners to be added early for upload, and only if that's done dispatch progress events.

Thomas, that flag is solely intended for forcing a preflight, which headers already do without that flag. But I guess what you mean is that we could add headers to the flag that controls whether upload events get dispatched, which currently is missing in the specification.
I created https://github.com/w3c/web-platform-tests/pull/3451 to fix the upload test. Standard will be fixed per URL in comment 19.
Yeah, I was just about to mention that it's really a "force preflight" flag, upon closer examination.

I don't think this is a problem with the spec now, aside from maybe renaming the variable or making it clear in the XHR send() method that it's a "force" flag, not a final decision on whether to preflight.

I was mislead because the Firefox XHR implementation doesn't use the fetch driver to do the fetch, so it ends up skipping the other preflight-check requirements. As such the code must be changed do the additional checks required in the fetch spec step 5.1.11:

  request's unsafe-request flag is set and either request's method is not a CORS-safelisted method or a header in request's header list is not a CORS-safelisted request-header
The problem with the XMLHttpRequest standard, which I'm about to land a fix for, is that it always dispatches events on XMLHttpRequestUpload objects, despite that only being done in implementations when listeners are registered before send() is invoked.
Actually I was incorrect, and the layers below do the preflight logic correctly. However, I don't see any mention of preflight in the web platform test XMLHttpRequest subfolder, nor any mention of XHRs in the /fetch/cors subfolder. Anne, do you think it's worth doing the same preflight tests on XHRs that are being done on Fetch requests in the fetch/api/cors WPT subfolder? If so I could factor out the tests and make sure they run for both.
Flags: needinfo?(annevk)
http://w3c-test.org/cors/ has some preflight tests that use XMLHttpRequest. E.g., the one we discussed, but also status-preflight.htm. Having said that, if you manage to generalize the fetch() tests to also work for XMLHttpRequest, that would be huge and I think everyone would appreciate that. (There might also be some scenarios we are testing in XMLHttpRequest at the moment that are not covered for fetch().)
Flags: needinfo?(annevk)
Sure, I don't mind working on that. Would it be preferable to move all of the cors-related tests into the /cors folder (perhaps into /cors/xhr and /cors/fetch)? Or would they be better off moved into /xhr/cors and /fetch/cors, and dropping the /cors subfolder entirely?
Flags: needinfo?(annevk)
Thomas, I'd argue all in fetch/cors since CORS (the protocol) is part of Fetch (the standard/subsystem) and those are the APIs that most of CORS is exposed to. Though fetch/api/cors and XMLHttpRequest/cors could work too I suppose if they reference each other in some way (e.g., through a comment or README resource).
Flags: needinfo?(annevk)
Summary: Investigate CORS test failures in testing/web-platform/meta/cors/ → Investigate CORS test failures in testing/web-platform/tests/cors/preflight-cache.htm
Follow up: "preflight for x-print should be cached"
Priority: -- → P3
Component: DOM → DOM: Core & HTML
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.