1184967 - client requests should allow CORS response interceptions

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

When I added the console logging for fetch event interception failures I made a small mistake.  This code:

     if (mIsClientRequest && response->Type() != ResponseType::Basic &&
         response->Type() != ResponseType::Default) {
       autoCancel.SetCancelStatus(NS_ERROR_CLIENT_REQUEST_OPAQUE_INTERCEPTION);
       return;
     }

Will fail for response types other than opaque.  For example, a CORS response will also fail this check.

The error code and resulting console log message, however, are specific to opaque responses.  We should correct this either by adding a new error code for CORS or by making this error code generic enough to cover all the relevant types.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Actually, the spec changed this morning so that we shouldn't fail CORS responses here:

  https://github.com/whatwg/fetch/commit/1612905aae06fdb912779b308d71bfc13422833f

Change this bug to be about fixing the logic to match.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Long term RequestMode will be set more reliably via bug 1189945.  In the short term I think we should just override the RequestMode based on nsIContentPolicy.

We will always need an override for documents because we don't enforce a same-origin policy on navigations normally.  They can redirect cross-origin, etc.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=ehsan

Ehsan, I'm trying to align our FetchEvent.respondWith() to the current spec.  This patch does this, although its a bit more complicated than I like.

The long term solution is to use the nsILoadInfo.securityFlags mode value.  This is in the tree, but not all the network request call sites set it yet.  If its not set we get SEC_NORMAL for now.  In the next few weeks SEC_NORMAL should be removed.

As a fallback, we use the existing nsIHttpChannelInternal.corsMode flag to set the RequestMode.  This flag should be removed along with SEC_NORMAL.

Finally, we must override all this logic for navigations.  Both the securityFlags and corsMode set navigations to no-cors.  We need these to be forced to same-origin for RequestMode.  I've discussed this at length with Anne and Jonas and this appears to be the best course.

I also force same-origin for workers since securityFlags is not being set for workers yet and corsMode does not set the right value either.  In the long run securityFlags will cover worker scripts, however, and we can remove this override.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update fetch-frame-resource.https.html CORS checks to expect pass. r=jgraham

With the code in P1 we can now mark the CORS requests in the wpt test to expected PASS.

Note, there was another case of using the old HTTP_ORIGIN in the CORS tests.  I fixed that in this patch as well.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Finally, I need to investigate a mochitest timeout in test_cross_origin_url_after_redirect.html.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8642637 [details] [diff] [review]
P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=ehsan

I'm no longer sure if this is a correct spec change.  Clarifying with Anne.

Comment 7

[(no longer active)]

Comment on attachment 8642637 [details] [diff] [review]
P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=ehsan

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

::: dom/fetch/InternalRequest.cpp
@@ +293,5 @@
> +  }
> +
> +  // TODO: remove following code once securityMode is fully implemented (bug 1189945)
> +
> +  nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(aChannel);

We have disable jar channel interception for release builds, so I would like to see this restructured in this way:

#ifdef RELEASE_BUILD
  MOZ_ASSERT(!jarChannel);
#else
  if (jarChannel) ...
#endif

Comment 8

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #6)
> Comment on attachment 8642637 [details] [diff] [review]
> P1 Set RequestMode based on LoadInfo securityMode and client request content
> policy. r=ehsan
> 
> I'm no longer sure if this is a correct spec change.  Clarifying with Anne.

Oh, OK.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Here is the spec issue:

  https://github.com/whatwg/fetch/issues/101

Basically this change broke the ability of a navigation to do a cross origin redirect without CORS.  This was revealed by test_cross_origin_url_after_redirect.html.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

It turns out that this should all work for navigations with same-origin mode.  It just needs to set the manual redirect flag.  This is something we haven't implemented yet.  We need bug 1184607 first.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=ehsan

Rebase on top of bug 1184607 patch.  Unfortunately the test still fails for some reason.  I need to dig into it to see if there is bug in the spec or our implementation.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=ehsan

With the latest changes to the patch in bug 1184607 this code now passes the existing tests.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=nsm

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update fetch-frame-resource.https.html CORS checks to expect pass. r=jgraham

Rebase.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=nsm

Oops.  I forgot to include Ehsan's previous review feedback.  Updating to include that.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Switch mochitest redirect tests back to returning a CORS Response. r=nsm

This reverts the changes to dom/workers/test/serviceworkers/fetch/origin from bug 1184607 since we can now legally return a CORS Response.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf650c06e9d

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Another try after adding a DebugOnly<> for mIsClientRequest to fix opt builds:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ed8ac15687

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8655019 [details] [diff] [review]
P1 Set RequestMode based on LoadInfo securityMode and client request content policy. r=nsm

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

::: dom/fetch/InternalRequest.cpp
@@ +215,5 @@
>    // A navigation request context is one of "form", "frame", "hyperlink",
>    // "iframe", "internal" (as long as context frame type is not "none"),
>    // "location", "metarefresh", and "prerender".
>    //
>    // TODO: include equivalent check for "form" context

Nit: Please file and add bug numbers. the form context probably should block v1, prerender doesn't need to IMO.

@@ +301,5 @@
> +  // TODO: remove following code once securityMode is fully implemented (bug 1189945)
> +
> +  nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(aChannel);
> +#ifdef RELEASE_BUILD
> +  MOZ_ASSERT(!jarChannel)

Nit: Semicolon.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #19)
> ::: dom/fetch/InternalRequest.cpp
> @@ +215,5 @@
> >    // A navigation request context is one of "form", "frame", "hyperlink",
> >    // "iframe", "internal" (as long as context frame type is not "none"),
> >    // "location", "metarefresh", and "prerender".
> >    //
> >    // TODO: include equivalent check for "form" context
> 
> Nit: Please file and add bug numbers. the form context probably should block
> v1, prerender doesn't need to IMO.

Actually, I just looked at form/prerender and they both go through docshell.  So I think they are covered as well.  I will fix the comment.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #19)
> @@ +301,5 @@
> > +  // TODO: remove following code once securityMode is fully implemented (bug 1189945)
> > +
> > +  nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(aChannel);
> > +#ifdef RELEASE_BUILD
> > +  MOZ_ASSERT(!jarChannel)
> 
> Nit: Semicolon.

Thanks.  I actually changed this to:

+  // We only support app:// protocol interception in non-release builds.
+#ifndef RELEASE_BUILD
+  nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(aChannel);
+  if (jarChannel) {
+    return RequestMode::No_cors;
+  }
+#endif

Since I realized I would get an unused variable error for jarChannel when this went to release.

We get the same effect as the assertion for jarChannel here since the following QI to http channel will fail and the nsCOMPtr will assert on deref.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00d29043eee2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7607669727
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a82bf9c643e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00d29043eee2
https://hg.mozilla.org/mozilla-central/rev/4c7607669727
https://hg.mozilla.org/mozilla-central/rev/8a82bf9c643e