Stop treating file: as untainted

[pshaughn]

servo/components/net/fetch/methods.rs

Lines 282 to 289 in 5f55cd5

	if (same_origin && !cors_flag ) ||
	current_url.scheme() == "data" ||
	current_url.scheme() == "file" || // FIXME: Fetch spec has already dropped filtering against file:
	// and about: schemes, but CSS tests will break on loading Ahem
	// since we load them through a file: URL.
	current_url.scheme() == "about" ||
	request.mode == RequestMode::Navigate
	{

This allows resources that come from a file: url to be peeked at in various ways, which is definitely not something we should be letting page authors do! I don't know which CSS tests this comment refers to. I know that Chrome is very tight about letting file urls see other file urls, so we should see what they do to pass whatever tests these are. Alternately, if the comment's out of date and the tests are now served over http with WPT tests, we should just remove the comment and the "file" case.

[pshaughn]

This test is in the spec as the first big block in step 5 of https://fetch.spec.whatwg.org/#main-fetch, with just data and not file/about.

[pshaughn]

It occurs to me now that maybe we need to leave about: taint-free because of the way we're using an about:blank load operation as part of iframe setup. Even if so we should taint file: which is a much bigger security risk.

[pshaughn]

I ran it through CI and I'm not finding the case of loading Ahem through a file: url.

The unit tests for fetching about: and fetching file: break; I don't know yet whether this is because removing this line breaks about: and file: fetches, or if it's because the unit tests were expecting something off-spec about the fetch responses.

One thing breaks in WPT tests: an EventSource constructor for about:blank is in readyState 0 when the test expects 2. This actually brings about: handling into alignment with how Servo is treating other cases here; the fact that there was a pass seems to have been potentially "accidental".

[pshaughn]

Looking further into tests: the about:blank one was expecting a 0-length Done body, but what we're seeing after response tainting is an Empty body; interesting that the type changed, but they mean the same thing and changing the test makes sense.

The file: one is going to be more interesting: it was expecting a Content-Type header, which it no longer gets after my change.

[pshaughn]

The unit tests are doing the wrong thing: they are calling the same response methods that the DOM calls to look inside the response, and consequently are seeing what the DOM should see. In the case of a file: url, what the DOM should see is an opaque filtered response giving no indication of whether the file even exists. The test has been expecting an unfiltered response and is failing now that the correct filtering is applied.

Removing these unit tests would be bad; what they really need to do is peek behind the filter. We're able to lay out an image that came from an opaque response, so the code for peeking behind the filter exists and just needs to be used in the unit tests. This might necessitate calling a lower-level method instead of fetch().

[pshaughn]

All the existing net unit tests are sharing one new_fetch_context method, which does a straightforward fetch with no chunk-processing. To really unit-test reading something that's inside an opaque filtered response, we need a special FetchResponseListener that can sniff metadata before filtering happens, like the one HTMLImageElement has.