Add 'XHR' to the Resource::Type enum, and use it for XHR requests.

Add 'XHR' to the Resource::Type enum, and use it for XHR requests.

We're currently hacking Mixed Content support for XHR in by adding an
enum to the request options, which is only set for XHR. We do this only
because we can't otherwise distinguish between XHR and Pepper requests.
Adding a resource type makes this a much clearer distinction, and allows
us to simplify the 'checkInsecureContent' implementation. Eventually, we
can move all that logic into MixedContentChecker.

This change should have no web-visible effect; existing tests should
continue to pass or fail, accordingly.

[Mike West, 2014-06-25 14:48:35]

Hey Tom, how do you feel about this change? It splits XHR out from the general
"Raw" in a way that might or might not make sense. I'd like to get that enum off
of the request options object, because I'd like to move most of this logic into
MixedContentChecker. Creating an explicit type means we can simplify the logic.
Ideally, we'd just pass in a Request, and magic would happen...

-mike

[Tom Sepez, 2014-06-25 16:45:40]

I like it, but I don't know enough to determine all the implications of the
change.  Adding Nate, who does understand these things (but note he is a little
distracted at the moment).

[Mike West, 2014-06-26 11:50:58]

Adding Adam and Jochen for context.

[abarth-chromium, 2014-06-26 16:55:37]

What about EventSource?

[Mike West, 2014-06-26 18:00:23]

We should block it in a mixed context along with WebSockets and XHR. We don't
currently do that; it just gets lumped in with the rest of the Resource::Raw
requests. Filed http://crbug.com/389209 to cover that work.

Note, though, that EventSource doesn't have an explicit
ResourceRequest::TargetType. Should it? If so, then I could do the same thing
with EventSource that this patch does with XMLHttpRequest.

[abarth-chromium, 2014-06-26 18:07:47]

On 2014/06/26 at 18:00:23, mkwst wrote:
> We should block it in a mixed context along with WebSockets and XHR. We don't
currently do that; it just gets lumped in with the rest of the Resource::Raw
requests. Filed http://crbug.com/389209 to cover that work.
> 
> Note, though, that EventSource doesn't have an explicit
ResourceRequest::TargetType. Should it? If so, then I could do the same thing
with EventSource that this patch does with XMLHttpRequest.

Sorry for being terse.  The point I was trying to make is that we shouldn't
treat XHR differently from other similar APIs like EventSource.

Some part of the system needs to know about the different kind of resources and
which types of resources have which mixed content policies, but it's not clear
to me what we should have a different ResourceFetcher::fetchFoo function for
each kind of resource.

Maybe the mixed content policy should be part of FetchRequest (similar to
OriginRestriction) and it should be the job of all the clients of
fetchRawResource to set the policy appropriately?  That way we can avoid
teaching core/fetch about the difference between XHR and EventSource.

[Mike West, 2014-06-26 18:27:18]

On 2014/06/26 at 18:07:47, abarth wrote:
> Sorry for being terse.  The point I was trying to make is that we shouldn't
treat XHR differently from other similar APIs like EventSource.
> 
> Some part of the system needs to know about the different kind of resources
and which types of resources have which mixed content policies, but it's not
clear to me what we should have a different ResourceFetcher::fetchFoo function
for each kind of resource.

We have lots of fetchXXX functions now; splitting out 'Raw' into more granular
types seems like a win for clarity, but certainly at a cost, since most of the
time we don't care about the differences.

> Maybe the mixed content policy should be part of FetchRequest (similar to
OriginRestriction) and it should be the job of all the clients of
fetchRawResource to set the policy appropriately?

I think DocumentThreadableLoader is the only client of fetchRawResource,
directly anyway. It constructs the FetchRequest based on the ResourceRequest
that EventSource/XHR/whatever passes into ThreadableLoader::create. We would
need to add a flag to ResourceRequest in order to avoid creating new
Resource::Type values. I guess there's no reason we couldn't do that, but I
don't think it's a good idea for EventSource to directly claim that it's active
content. If the API is responsible, I worry that it's one more thing to forget
to do when writing new code.

I'd like to centralize that logic in the MixedContentChecker, which means some
sort of mapping between an exhaustive list of request types and their respective
mixed content dispositions. If we have an exhaustive enum somewhere, it means
the compiler will help out when a new type is added. "Hey, think about the mixed
content disposition of this new type!", Clang will say. It'll be great. :)

> That way we can avoid teaching core/fetch about the difference between XHR and
EventSource.

I would like to remove knowledge of mixed content logic from core/fetch
entirely, to be honest. It makes sense to me to encapsulate that knowledge in
MixedContentChecker.  Basically, everything in ::checkInsecureContent should
really live there, perhaps as something like
MixedContentChecker::shouldBlockRequestAsMixedContent(Request*).

[abarth-chromium, 2014-06-26 19:48:33]

On 2014/06/26 at 18:27:18, mkwst wrote:
> On 2014/06/26 at 18:07:47, abarth wrote:
> > Sorry for being terse.  The point I was trying to make is that we shouldn't
treat XHR differently from other similar APIs like EventSource.
> > 
> > Some part of the system needs to know about the different kind of resources
and which types of resources have which mixed content policies, but it's not
clear to me what we should have a different ResourceFetcher::fetchFoo function
for each kind of resource.
> 
> We have lots of fetchXXX functions now; splitting out 'Raw' into more granular
types seems like a win for clarity, but certainly at a cost, since most of the
time we don't care about the differences.

Those different functions produce different subclasses of Resource (or at least
that was the original intention).

> > Maybe the mixed content policy should be part of FetchRequest (similar to
OriginRestriction) and it should be the job of all the clients of
fetchRawResource to set the policy appropriately?
> 
> I think DocumentThreadableLoader is the only client of fetchRawResource,
directly anyway. It constructs the FetchRequest based on the ResourceRequest
that EventSource/XHR/whatever passes into ThreadableLoader::create.

Perhaps they should pass in a FetchRequest rather than a ResourceRequest?

> We would need to add a flag to ResourceRequest in order to avoid creating new
Resource::Type values. I guess there's no reason we couldn't do that, but I
don't think it's a good idea for EventSource to directly claim that it's active
content. If the API is responsible, I worry that it's one more thing to forget
to do when writing new code.

We can make the default be to block mixed content and to force the clients that
want a less secure behavior to be explicit.

> I'd like to centralize that logic in the MixedContentChecker, which means some
sort of mapping between an exhaustive list of request types and their respective
mixed content dispositions. If we have an exhaustive enum somewhere, it means
the compiler will help out when a new type is added. "Hey, think about the mixed
content disposition of this new type!", Clang will say. It'll be great. :)

Why is it better to centralize the logic in MixedContentChecker?

> > That way we can avoid teaching core/fetch about the difference between XHR
and EventSource.
> 
> I would like to remove knowledge of mixed content logic from core/fetch
entirely, to be honest.

Why?

> It makes sense to me to encapsulate that knowledge in MixedContentChecker. 
Basically, everything in ::checkInsecureContent should really live there,
perhaps as something like
MixedContentChecker::shouldBlockRequestAsMixedContent(Request*).

That approach means that ResourceFetcher and MixedContentChecker both need to
know about all the types of requests that exist in the system.  That's not how
the specs are organized, for example.  The fetch spec has a bunch of flags you
can set when triggering a fetch because the fetch spec doesn't want to know
about all the types of resources that might be invented in the future.

Consider, for example, what would happen if we added a new fetch type in a Blink
module.  If the caller configures the policy, then all the knowledge about the
existence of that module is contained inside the module.  If we plumb the type
information through fetch to MixedContentChecker, then both fetch and
MixedContentChecker need to be aware that the module exists, which is counter to
the design principle that core should be ignorant of modules.

[Mike West, 2014-06-27 13:34:57]

On 2014/06/26 19:48:33, abarth wrote:
> Maybe the mixed content policy should be part of FetchRequest (similar to
OriginRestriction)

Right now, we have a mixed content flag hanging off of ResourceLoaderOptions.
That is fed into FetchRequest, and available as
fetchRequest.options.mixedContentBlockingTreatment. Would ResourceLoaderOptions
be an acceptable place to keep this sort of information, or would you prefer it
moved to FetchRequest directly?

> and it should be the job of all the clients of fetchRawResource to set the
policy appropriately?

The default behavior is to examine the resource type in order to determine how
to treat it, which means that ResourceFetcher is currently responsible for
binding together the resource types it knows about with their respective mixed
content policies.

> That approach means that ResourceFetcher and MixedContentChecker both need to
> know about all the types of requests that exist in the system.  That's not how
> the specs are organized, for example.  The fetch spec has a bunch of flags you
> can set when triggering a fetch because the fetch spec doesn't want to know
> about all the types of resources that might be invented in the future.

Fetch added a list of "context"s to support both Content Security Policy and
Mixed Content specs: http://fetch.spec.whatwg.org/#concept-request-context. The
caller specifies the context, which is passed off to one of those two specs for
processing. We haven't rewritten CSP to use them yet, but that was my plan for
the next iteration.

Whether or not this is a _good_ model is probably worth discussing, but it's how
things are specified at the moment.

> If we plumb the type information through fetch to MixedContentChecker, then
> both fetch and MixedContentChecker need to be aware that the module exists,
> which is counter to the design principle that core should be ignorant of
> modules.

Hrm. I agree with the principle.

For mixed content, I think your suggestion works pretty well: each caller is
responsible for defining itself as active or passive content (and, as time goes
on, we'll narrow down the list of what's allowed to consider itself passive).

For CSP, I'm a bit more skeptical. If a new fetch type defined in a module also
entails a new CSP directive, we need to teach CSP (and therefore core) about it.

[abarth-chromium, 2014-06-27 15:45:13]

On 2014/06/27 at 13:34:57, mkwst wrote:
> On 2014/06/26 19:48:33, abarth wrote:
> > Maybe the mixed content policy should be part of FetchRequest (similar to
OriginRestriction)
> 
> Right now, we have a mixed content flag hanging off of ResourceLoaderOptions.
That is fed into FetchRequest, and available as
fetchRequest.options.mixedContentBlockingTreatment. Would ResourceLoaderOptions
be an acceptable place to keep this sort of information, or would you prefer it
moved to FetchRequest directly?

ResourceLoaderOptions seems fine.

> > and it should be the job of all the clients of fetchRawResource to set the
policy appropriately?
> 
> The default behavior is to examine the resource type in order to determine how
to treat it, which means that ResourceFetcher is currently responsible for
binding together the resource types it knows about with their respective mixed
content policies.

Maybe we should change the default for Raw resources to be to block mixed
content and clients that want to do something else will need to whitelist their
insecurity using ResourceLoaderOptions?

> > That approach means that ResourceFetcher and MixedContentChecker both need
to
> > know about all the types of requests that exist in the system.  That's not
how
> > the specs are organized, for example.  The fetch spec has a bunch of flags
you
> > can set when triggering a fetch because the fetch spec doesn't want to know
> > about all the types of resources that might be invented in the future.
> 
> Fetch added a list of "context"s to support both Content Security Policy and
Mixed Content specs: http://fetch.spec.whatwg.org/#concept-request-context. The
caller specifies the context, which is passed off to one of those two specs for
processing. We haven't rewritten CSP to use them yet, but that was my plan for
the next iteration.
> 
> Whether or not this is a _good_ model is probably worth discussing, but it's
how things are specified at the moment.

Ok.  Can we change our type enum to match the spec?  If that's not easy, can we
at least document how the implementation types relate to the spec types?

> > If we plumb the type information through fetch to MixedContentChecker, then
> > both fetch and MixedContentChecker need to be aware that the module exists,
> > which is counter to the design principle that core should be ignorant of
> > modules.
> 
> Hrm. I agree with the principle.
> 
> For mixed content, I think your suggestion works pretty well: each caller is
responsible for defining itself as active or passive content (and, as time goes
on, we'll narrow down the list of what's allowed to consider itself passive).
> 
> For CSP, I'm a bit more skeptical. If a new fetch type defined in a module
also entails a new CSP directive, we need to teach CSP (and therefore core)
about it.

That's true...  I guess we can cross that bridge when we come to it.

You've convinced me that it's reasonable to teach core/fetch about the various
types that are defined in the fetch spec.  From reading that list, it sounds
like that should you solve problem you're having.  We need to audit all callers
of ThreadableDocumentLoader to make sure all the folks who should be mark as
|connect| actually are.

[Mike West, 2014-07-01 09:00:14]

Opened https://www.w3.org/Bugs/Public/show_bug.cgi?id=26247 to figure out what
contexts make sense for the resource types that Blink knows about but Fetch
currently doesn't.