client requests should allow CORS response interceptions

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

2.94 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
9.81 KB, patch
nsm
: review+
Details | Diff | Splinter Review
3.77 KB, patch
nsm
: review+
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → bkelly
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-postv1
Status: NEW → ASSIGNED
Summary: client requests that fail interception due to CORS response log that an opaque response was used → client requests should allow CORS response interceptions
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.
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.
Attachment #8642637 - Flags: review?(ehsan)
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.
Attachment #8642638 - Flags: review?(james)
Finally, I need to investigate a mochitest timeout in test_cross_origin_url_after_redirect.html.
Attachment #8642638 - Flags: review?(james) → review+
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.
Attachment #8642637 - Flags: review?(ehsan)

Comment 7

4 years ago
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

4 years ago
(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.
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.
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.
Depends on: 1184607
No longer depends on: 1184607
Depends on: 1184607
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.
Attachment #8642637 - Attachment is obsolete: true
With the latest changes to the patch in bug 1184607 this code now passes the existing tests.
Attachment #8642638 - Attachment is obsolete: true
Attachment #8642638 - Attachment is obsolete: false
Attachment #8645011 - Attachment is obsolete: true
Oops.  I forgot to include Ehsan's previous review feedback.  Updating to include that.
Attachment #8655013 - Attachment is obsolete: true
Attachment #8655013 - Flags: review?(nsm.nikhil)
Attachment #8655019 - Flags: review?(nsm.nikhil)
This reverts the changes to dom/workers/test/serviceworkers/fetch/origin from bug 1184607 since we can now legally return a CORS Response.
Attachment #8655021 - Flags: review?(nsm.nikhil)
Another try after adding a DebugOnly<> for mIsClientRequest to fix opt builds:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ed8ac15687
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.
Attachment #8655019 - Flags: review?(nsm.nikhil) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/00d29043eee2
https://hg.mozilla.org/mozilla-central/rev/4c7607669727
https://hg.mozilla.org/mozilla-central/rev/8a82bf9c643e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.