Anne, what was this bug about? I'm not entirely sure what the context is. Can it be closed?

What is unclear about the title plus summary? It cannot be closed.

XHR spec mentions:

force preflight flag
    Set if the upload events flag is set. 

Force preflight flag links to Fetch: 
http://fetch.spec.whatwg.org/#force-preflight-flag
where it is defined. I assume that you want to add more text about it in the XHR spec since you want this bug to remain open? (Of course using non-trivial HTTP verbs or custom headers also should trigger the preflight - but that logic is in Fetch already..) Anyway - up to you, since you know what you want to get done here :-)

This bug is mostly about figuring out whether the "upload events flag" makes sense and by extension the "force preflight flag" which was introduced for XMLHttpRequest.

People kept asking why it was needed and I don't remember.

Thanks for explaining this a bit more..

So yeah, why do we need a preflight if XHR sends some data and has registered upload event listeners? Well, I presume there's a concern about cross-origin information leakage if some upload event listeners fire. Stuff like intranet port/host sniffing might be possible?

So we should  add something like:

<p class="note">Firing upload events for cross-origin requests requires a preflight because otherwise, information about the existence of sites and services might leak across origins.</p>

..although it also seems feasible to just kill the "upload events flag" and do it this way:
https://github.com/whatwg/xhr/pull/15

It seems there is a bug in the specification. We should not be dispatching upload progress events unless that flag is set. Otherwise event listeners registered after send() is invoked will still get events.

It's not entirely clear to me how you cannot figure out the server exists with just <img>, but I guess it makes timing attacks even easier maybe? Still feels somewhat sketchy.

No, whether to fire events is controlled by the "upload complete" flag. That works fine as far as I can tell, so there should be no reason to keep the "upload events flag".

(Though I note that I agree with your comment about server/port detection via <IMG> and I'm still unsure why you need to enforce a preflight at all)

(Anoter note - to self - is that if we keep the "force preflight flag" it needs a test or two written.)

Anne's last comment had some details I didn't really respond to above.

> Otherwise event listeners registered after send() is invoked will
> still get events.

Hm.. If xhr is an async request object, and a script does his:

xhr.send(data)
xhr.upload.onloadend = function(){...}

it sounds like you're saying the loadend listener should not fire - ? Why not?

> It's not entirely clear to me how you cannot figure out the 
> server exists with just <img>, but I guess it makes timing
> attacks even easier maybe? Still feels somewhat sketchy.

http://xhr.spec.whatwg.org/#security-considerations has a useful statement about reading information like size of resources with progress events. That's certainly sufficient reason to require a preflight here.

It should not fire if we want to require a preflight for that case and no preflight was made.

And those security considerations are for a response, not a request (of which you already know the size).

x.open("POST", "...")
x.send("test")

should not send a preflight and should not allow for late registration of upload event listeners.

x.open("POST", "...")
x.setRequestHeader("test", "test")
x.send("test")

should send a preflight and should probably allow for late registration of upload event listeners therefore.

So I guess what we want is keep track in Fetch of whether a preflight happened and if a preflight happened we dispatch events and otherwise we do not.

(It's also somewhat icky we look for event listeners for a certain event name here but there's no API to do that in the platform. We should maybe define some kind of hook...)

> x.open("POST", "...")
> x.send("test")
> should not send a preflight and should not allow
> for late registration of upload event listeners.

> x.open("POST", "...")
> x.setRequestHeader("test", "test")
> x.send("test")
> should send a preflight and should probably allow
> for late registration of upload event listeners therefore.

Anne, I can see the logic - sort of - but I think the usability of this is going to suck. What you're suggesting is to introduce some secret state in the API which the JS author can't inspect or know about ('force_preflight') that will depend on side effects of whether specific methods like setRequestHeader() were called with specific arguments - and this secret state will determine whether my upload event listeners fire?

Are you really, really sure?

Because it sure seems like a much simpler option to just say "OK, you want to listen to upload events? Righty, doing that will take a preflight request but the events you expect will fire". That to some extent goes against the norm that adding event listeners should not have side effects - but the API seems much more usable that way.

So.. tried to write a test case for this to see how current browsers behave. This should be a valid test:
https://github.com/w3c/web-platform-tests/commit/5fd7fd9b759d12006d2ecba5e26d0026521343d6

Story right now is that:

* Firefox implements what Anne and (apparently) the spec says is correct: don't fire upload events for "simple" requests, fire for "not simple" requests (preflighted).

* Opera (Presto) fires all the events, whether the request was "preflighted" or not.

* Chromium fires none of those events, again regardless of preflight status.

(Can't test in a modern IE right now - it would be a pretty sweet mess if they did the opposite of Firefox - so we had all possible behaviours covered among the main four engines..)

Note: the data being sent in the above test case is only 1000 characters - well, 999 to be precise - maybe sending a more complex payload might change the way some implementations behave? Thinking of Chrome in particular. I tried to send a larger payload but at some point it seemed to hit some configured limit for the python serve.py script..

IMO this mess indicates that this specific corner of the spec isn't relied upon by authors yet, and we could do a bit of cleanup. I'm not sure how exactly it should work though - given that it's too late to make sure a preflight request happens after send() is called, maybe Chromium's "ignore events registered after send()" model is best?

So Chrome only dispatches events if the "upload events flag" is set? That would be okay with me.

Seems correct. I agree this is reasonable.

(Also I'd like browsers to issue warnings when authors register listeners that will never fire, but I guess that's an implementation detail)

Okay. So it sounds like what we want to do here is require events listeners to be added to XMLHttpRequest's associated XMLHttpRequestUpload object before send() is invoked.

Do we do this even for same-origin fetches? Since due to redirects they might be hard to distinguish.


Hallvord, I did not get this by the way from comment 13 upon rereading this bug:

> given that it's too late to make sure a preflight request happens 
> after send() is called

Gecko does not force a preflight for sameorigin requests if there's a progress listener. Doing so would likely break a lot of existing content that was written before CORS existed.

However gecko does not support redirecting to cross-origin in that case since we generally don't support redirects when a preflight is required. Both because this used to be what the spec required, and because doing anything else used to be impossible due to the design of Gecko's network library.

And yes, I believe the force-preflight flag was added to avoid making it possible to detect servers. I agree it's doubtful if that's really useful. Feel free to reach out to the mozilla security team to see if they feel comfortable removing this restriction. Obviously other browser vendors might have the same concern.

I did not mean forcing a preflight for same-origin content. I meant requiring that listeners are added in advance. I believe Chrome is doing that.

(Note that I don't believe you're right about the specification having changed. I think same-origin -> cross-origin has always been there.)

What's the advantage to requiring that listeners are added before the .send() call? Why not instead treat listeners added before .send() as setting the "force preflight". And if we're not preflighting a cross-origin request, then no progress events are fired.

That requires an awful lot of coordination between the API and the Fetch layer due to e.g. same-origin requests redirecting to cross-origin requests. Whether a preflight was made for a cross-origin request. I'm not sure we want to have that kind of back-and-forth between those layers. Seems cumbersome.

Hmm.. good point. Though seems really silly to require event listeners to be added at any particular time for same-origin requests that have never been done cross-origin.

I just went with what implementations do for now: https://github.com/whatwg/xhr/commit/667d4f3604aaee4e24bde33ed487cf8a98274e70

Basically always require listeners upfront...

I also submitted a correction for Hallvord's test based on what implementations do today: https://github.com/w3c/web-platform-tests/pull/3451