Bug 20322 - Upload progress events vs CORS
Summary: Upload progress events vs CORS
Status: NEW
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: XHR (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-10 10:21 UTC by Anne
Modified: 2014-10-21 13:35 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anne 2012-12-10 10:21:53 UTC
It would be good to add some details here. The main thing it prevents detecting I think is the existence of another server, but whether that's useful given that you can already discover it via <img> I'm not sure, and we might be able to address it in a better way?
Comment 1 Hallvord R. M. Steen 2013-11-07 14:33:39 UTC
Anne, what was this bug about? I'm not entirely sure what the context is. Can it be closed?
Comment 2 Anne 2013-11-08 22:31:16 UTC
What is unclear about the title plus summary? It cannot be closed.
Comment 3 Hallvord R. M. Steen 2013-11-08 22:45:32 UTC
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 :-)
Comment 4 Anne 2013-11-08 22:47:14 UTC
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.
Comment 5 Hallvord R. M. Steen 2013-11-11 21:14:21 UTC
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
Comment 6 Anne 2013-11-12 06:30:15 UTC
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.
Comment 7 Hallvord R. M. Steen 2013-11-12 12:06:34 UTC
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".
Comment 8 Hallvord R. M. Steen 2013-11-12 12:10:26 UTC
(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.)
Comment 9 Hallvord R. M. Steen 2013-11-12 20:13:34 UTC
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.
Comment 10 Anne 2013-11-18 13:56:45 UTC
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).
Comment 11 Anne 2013-11-18 14:14:07 UTC
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...)
Comment 12 Hallvord R. M. Steen 2013-11-19 11:11:24 UTC
> 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.
Comment 13 Hallvord R. M. Steen 2013-11-25 16:12:40 UTC
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?
Comment 14 Anne 2013-11-26 13:10:44 UTC
So Chrome only dispatches events if the "upload events flag" is set? That would be okay with me.
Comment 15 Hallvord R. M. Steen 2013-11-26 14:30:37 UTC
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)
Comment 16 Anne 2014-10-20 16:01:25 UTC
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
Comment 17 Jonas Sicking 2014-10-20 23:13:41 UTC
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.
Comment 18 Anne 2014-10-21 07:05:44 UTC
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.)
Comment 19 Jonas Sicking 2014-10-21 07:23:34 UTC
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.
Comment 20 Anne 2014-10-21 07:30:28 UTC
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.
Comment 21 Jonas Sicking 2014-10-21 09:15:28 UTC
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.