:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 10 years ago

As specified by <http://fetch.spec.whatwg.org/#basic-fetch> and tested by <http://w3c-test.org/XMLHttpRequest/data-uri-basic.htm> and <http://w3c-test.org/XMLHttpRequest/data-uri.htm>.

	Thomas Wisniewski Assignee
	Comment 1 • 8 years ago

Here's a basic patch. Needinfo'ing valentin as I did for bug 918735, since I'm not sure whether it's desirable to solve this in such a straightforward manner, but it was the least invasive-seeming way to pass the five relevant web platform tests at the bottom of http://w3c-test.org/XMLHttpRequest/data-uri.htm 

Note that Chrome doesn't seem to pass the WPT for the HEAD method, though it passes the other four for other non-GET methods.

I also tried an alternative approach (in the hopes that it would make the Fetch API fail similarly), by having nsContentSecurityManager::doContentSecurityCheck fail on non-GET data URI requests, then having the XMLHttpRequest code detect and convert the failure into the expected results for the WPT rather than throwing. Unfortunately, I ran into snags that made it seem like it was more trouble than it was worth;

- wanting to skip parsing the data URI entirely, I tried to skip the eventual call to AsyncOpen and just ChangeStateToDone() on the XHR, but no DOM onreadystatechange events were ever fired when I did that. It seems that BeginPumpingData() and AsyncRead()ing the channel is necessary before they're fired, and I'm still too green to understand why.

- when I had doContentSecurityCheck return a failure nsResult, but then converted it to NS_OK so Send() wouldn't throw, the GetStatus/GetStatusText/GetResponseText methods acted as though the security check had passed. I had to store the result and add checks like in the above patch anyway, which seemed like a bit of a waste.

- with doContentSecurityCheck returning a failure, calls to fetch() seemed to crash the tab on trying to recv from the request's channel, so rather than debugging any further I figured I'd better just ask for more info.

I'm also not sure whether the Fetch API behavior should be fixed to match Chrome's behavior (throwing a "TypeError: Failed to fetch" causing the promise to fail, and also logging an informative message to the console). It seems more reasonable to me than Firefox's just letting the promise succeed with a 200/OK and undefined responseText, but I don't see any WPT for it, so I thought I'd check.

	Valentin Gosu [:valentin] (he/him)
	Comment 2 • 8 years ago

I'm not very familiar with that code. I suggest asking one of the DOM peers for review:
https://wiki.mozilla.org/Modules/All#Document_Object_Model

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

Comment on attachment 8756857 [details] [diff] [review]
1063526-disallow-non-get-data-uri-xmlhttprequests.diff

Review of attachment 8756857 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. RyanVM suggested :sicking on IRC. I'll make it a habit to ask there first.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 8 years ago

Comment on attachment 8756857 [details] [diff] [review]
1063526-disallow-non-get-data-uri-xmlhttprequests.diff

Review of attachment 8756857 [details] [diff] [review]:
-----------------------------------------------------------------

Is this data: specific, or should we forbid non-GET to any non-http url. I.e. should we add an else-branch here instead:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#1657

What is the intended behavior when POST-ing to a data: URL? Doesn't really look like the current patch "disallows" as much as it silently doesn't load the URL.

	Thomas Wisniewski Assignee
	Comment 5 • 8 years ago

I don't know if there is any specified behavior for non-GET accesses to URLs, but I see nothing obvious on the whatwg XHR spec (and the W3C XHR level 2 spec is TR). In fact, the web platform test mentioned in this bug is the only reference for data urls that I've seen so far.

Regardless, Firefox and Chrome already both seem to forbid XHR access to non-http/https/data schemes by throwing an exception, at least for the schemes I tried like javascript, about, ws, file, and "nonexistent". Chrome also allows a couple others like chrome and chrome-extension, but general web content can't access them anyhow (exceptions are raised).

The only case where the browsers behave differently that I've seen is this one, and Chrome currently passes the web platform test while throwing will not pass it. I don't have any problem with trying to get the test changed instead, though.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years ago

:hallvors, mind chiming in here? My patch simply passes the related tests in http://w3c-test.org/XMLHttpRequest/data-uri.htm, and I'm not sure if :sicking's comment implies a better fix. Perhaps it is, if both blob and data URLs are to be treated this way, but I'm not sure.

Also note that there seem to be two more instances of this bug report;
https://bugzilla.mozilla.org/show_bug.cgi?id=918714
https://bugzilla.mozilla.org/show_bug.cgi?id=1238007

	Hallvord R. M. Steen [:hallvors]
	Comment 9 • 8 years ago

non-GET data: and "other protocols" end up with the same "network error" condition per https://fetch.spec.whatwg.org/#basic-fetch - I don't see anything in the XHR spec about protocols being allowed or not. Anne, comments?

	Anne (:annevk)
	Comment 10 • 8 years ago

What does "protocol" mean here? "Scheme"?

The only schemes that can fetch resources are those mentioned in Fetch. So technically "about:blank" ought to work, as should "blob:...". "ftp"/"file" can't work since they're not same-origin and "filesystem" isn't really a thing so far.

about:blank seems to work for any request method at the moment.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 11 • 8 years ago

So POST and FOOPY should work for about:blank and ftp:..., but not for data:? Is there a logic to this?

	Hallvord R. M. Steen [:hallvors]
	Comment 12 • 8 years ago

Well, it doesn't make sense to POST to about:blank, no more than POSTing to data:.. Shouldn't that be consistent?

And yes, I meant scheme. Interestingly, we don't seem to have any tests in the test suite that try to load about:blank, javascript: or foobar: URLs - we should fix that. Once I've fixed that, I'll figure out if there are implementation differences that would warrant a closer look at the spec, or just "network error" all the way..

	Anne (:annevk)
	Comment 13 • 8 years ago

To be clear, how ftp works is not defined. data I think was decided at some point to keep it restricted, in case we want to do more with it later (seems unlikely at this point).

We could change how they work probably.

Hallvord, see https://github.com/w3c/web-platform-tests/commit/d3451d24e3f7a9c2c9d3e39c9efeea6e768a70c5 for some scheme tests I created recently. You might be able to adapt those further.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 14 • 8 years ago

Comment on attachment 8756857 [details] [diff] [review]
1063526-disallow-non-get-data-uri-xmlhttprequests.diff

Review of attachment 8756857 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I see no reason to special-case data: here, nor to special-case XHR.

POST/FOOPY to about:blank or to blob: makes as much and as little sense as POST/FOOPY to data:. Same goes for other protocols which doesn't have a concept of a request method.

So I think the correct solution here would be to look if the created nsIChannel implements nsIHttpChannel or not. And we should do this in the fetch() API as well.

	Thomas Wisniewski Assignee
	Comment 15 • 8 years ago

I really like that suggestion at first glance, as it would hypothetically reduce this to just adding a new CORS violation check. However, unless I'm mistaken it could be a pretty deep rabbit hole.

Not only have I seen internal code making some system XHRs that can try to POST to data URIs (from about:config options even), but I suspect that addons are also likely going to break as well for similar reasons.

On top of that, special-casing it so that System XHRs are allowed to bypass this will require being able to tell which type of CORS failure it was, and right now there's only one CORS-violation error as I understand things, NS_ERROR_DOM_BAD_URI. Adding a second code so XHRs can tell them apart would require making sure that other places also check for both, so the patch will likely require careful review.

So if I'm right, even if we're willing to just break addons (and potentially Thunderbird for all I know), this might take a while to get right. Thoughts?

	Anne (:annevk)
	Comment 16 • 8 years ago

Jonas, so your proposal is to change Fetch to return a network error when the request's method is non-GET and the request's url's scheme is not an HTTP(S) scheme? Seems reasonable.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 8 years ago

My proposal is to treat non-GET methods for non-HTTP(S) protocols consistently.

Whether to, for such requests, return a network error, return an empty response, treat the request as GET, or do something else should be decided based on what is web-compatible and based on what browsers are willing to implement.

I would personally be very worried about web-compat when making changes. So I would not recommend any changes to Firefox until we at the very least know that other browsers plan to make the same changes. And ideally wait until other browsers actually have such patches landed.


Thomas: I would not recommend implementing this as part of CORS since the CORS code isn't always run. And conceptually this is not a part of CORS, so it seems like the check could easily get lost if it lives there when we make future changes.

Instead the implementation strategy I'd use is that when the current XHR code calls SetMethod, rather than doing nothing if the channel is not an nsIHttpChannel, take the appropriate action.

But see above about web-compat concerns.

	Anne (:annevk)
	Comment 18 • 8 years ago

I filed https://github.com/whatwg/fetch/issues/339 on sorting this out standards-wise. Not really high priority for me, but if someone is interested in doing the testing, I'm happy to help.

	Anne (:annevk)
	Comment 19 • 8 years ago

Olli, I think we should reconsider this patch. I did some testing and it seems most browsers agree with Fetch already. For about:blank anything goes, and blob and data URLs are restricted to GET-only. I'm not sure it's worth it to try to change everyone anymore.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 20 • 8 years ago

See comment 17. I think consistency and simplicity trumps forbidding non-sensical edge cases.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 21 • 8 years ago

I.e. my proposal is to WONTFIX this bug and make the spec allow all methods to all protocols.

	Anne (:annevk)
	Comment 22 • 8 years ago

The conclusion of https://github.com/whatwg/fetch/issues/339 is to keep the restriction for blob URLs since nobody cares for the churn. The data URL restriction will be lifted from Fetch.

	Anne (:annevk)
	Comment 23 • 8 years ago

Note that currently we fail tests in http://w3c-test.org/fetch/api/basic/scheme-data.html so I'm not sure WONTFIX is the right approach here.

	Anne (:annevk)
	Comment 24 • 8 years ago

Doh, that's the test that needs changing.

	Thomas Wisniewski Assignee
	Comment 25 • 8 years ago

So then will this patch do for fixing that WPT, Anne?

If so, would it be best to close this bug as INVALID (once it lands)?

	Anne (:annevk)
	Comment 26 • 8 years ago

Comment on attachment 8787887 [details] [diff] [review]
1063526-let-scheme-data-fetch-WPT-pass-for-head-and-post-requests.diff

Review of attachment 8787887 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that looks fine.

	Anne (:annevk)
	Comment 27 • 8 years ago

Not sure what the policy around resolving bugs is after a patch lands. INVALID does seem appropriate.

	Thomas Wisniewski Assignee
	Comment 28 • 8 years ago

Thanks. Here's a version of the patch with a proper commit message. Carrying over r+ and requesting check-in.

	Thomas Wisniewski Assignee
	Comment 29 • 8 years ago

Sorry, this is the patch we want. Still requesting check-in for this one.

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9c8d5aaeb00e

	Kohei Yoshino
	Comment 31 • 8 years ago

Changing the confusing resolution as per comment 27.