Closed Bug 1063526 Opened 7 years ago Closed 5 years ago
Don't load data URLs when method is not "GET"'
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>.
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.
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
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.
Attachment #8756857 - Flags: review?(jonas)
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.
: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
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?
Flags: needinfo?(hsteen) → needinfo?(annevk)
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.
So POST and FOOPY should work for about:blank and ftp:..., but not for data:? Is there a logic to this?
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.
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.
Attachment #8756857 - Flags: review?(jonas) → review-
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?
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.
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.
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.
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.
See comment 17. I think consistency and simplicity trumps forbidding non-sensical edge cases.
I.e. my proposal is to WONTFIX this bug and make the spec allow all methods to all protocols.
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.
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.
Doh, that's the test that needs changing.
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)?
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.
Attachment #8787887 - Flags: review?(annevk) → review+
Not sure what the policy around resolving bugs is after a patch lands. INVALID does seem appropriate.
Thanks. Here's a version of the patch with a proper commit message. Carrying over r+ and requesting check-in.
Assignee: nobody → wisniewskit
Attachment #8787887 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Sorry, this is the patch we want. Still requesting check-in for this one.
Attachment #8787897 - Attachment is obsolete: true
Changing the confusing resolution as per comment 27.
You need to log in before you can comment on or make changes to this bug.