Closed Bug 1063526 Opened 7 years ago Closed 5 years ago

Don't load data URLs when method is not "GET"'


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

Not set





(Reporter: Ms2ger, Assigned: wisniewskit)




(1 file, 3 obsolete files)

Blocks: xhr2pass
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 

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.
Flags: needinfo?(valentin.gosu)
I'm not very familiar with that code. I suggest asking one of the DOM peers for review:
Flags: needinfo?(valentin.gosu)
Comment on attachment 8756857 [details] [diff] [review]

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]

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:

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.
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.
:hallvors, mind chiming in here? My patch simply passes the related tests in, 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;
Flags: needinfo?(hsteen)
Duplicate of this bug: 918714
Duplicate of this bug: 1238007
non-GET data: and "other protocols" end up with the same "network error" condition per - 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.
Flags: needinfo?(annevk)
So POST and FOOPY should work for about:blank and ftp:..., but not for data:? Is there a logic to this?
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..
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 for some scheme tests I created recently. You might be able to adapt those further.
Comment on attachment 8756857 [details] [diff] [review]

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?
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
I filed 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.
Flags: needinfo?(bugs)
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 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 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)?
Attachment #8756857 - Attachment is obsolete: true
Attachment #8787887 - Flags: review?(annevk)
Comment on attachment 8787887 [details] [diff] [review]

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
Keywords: checkin-needed
Sorry, this is the patch we want. Still requesting check-in for this one.
Attachment #8787897 - Attachment is obsolete: true
Closed: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Changing the confusing resolution as per comment 27.
Keywords: dev-doc-needed
Resolution: FIXED → INVALID
Flags: needinfo?(bugs)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.