1119021 - Fetch API: Implement CORS support

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: patch 1 - CORS support

Use nsCrossSiteListenerProxy.h helpers to implement CORS support.
Several CORS fixes and lots of CORS tests.

Fixes:
Use empty string stream if response has no stream.
Parse Access-Control-Expose-Headers correctly.
Copy over remaining InternalRequest constructor attributes and set unsafe request flag.
Call FailWithNetworkError() in more cases.
Add non-simple Request headers to unsafeHeaders list for CORS check.
Do not AsyncOpen channel directly when CORS preflight is required.
Fix check for simple request method (was checking the opposite condition).

Comment 2

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

Attachment: patch 2 - CORS credentials tests

Allow request to continue when useCredentials is set.

Comment 3

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

Attachment: patch 3 - Implement fetch() redirects correctly

Comment 4

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

Sorry about the noise, bzexport messed up.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8545519 [details] [diff] [review]
patch 1 - CORS support

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

I had a hard time lining the spec up to the code in this patch, unfortunately.  This is mainly due to some parts being implemented in fetch and others being implemented in NS_StartCORSPreflight.  It would be really nice to have comments indicating what steps in the spec are handled here vs. NS_StartCORSPreflight vs. other bugs.

That being said, I *think* it does what its supposed to. r+'ing, although I would also like Andrea to review it.

::: dom/fetch/FetchDriver.cpp
@@ +260,2 @@
>  {
>    mResponse = nullptr;

This is step 1 of the HTTP Fetch spec.  Where do we implement step 2?

  2. If request's skip service worker flag is unset and request's client's
     global object is not a ServiceWorkerGlobalScope object, run these
     substeps: [HTML] [SW]
    1. Set response to the result of invoking handle fetch for request. [SW]
    2. If either response's type is opaque and request's mode is not no CORS or
       response's type is error, return a network error.

Or is this effectively the fetch event handling?

@@ +298,5 @@
> +  bool useCredentials = false;
> +  if (mRequest->GetCredentialsMode() == RequestCredentials::Include ||
> +      (mRequest->GetCredentialsMode() == RequestCredentials::Same_origin && !aCORSFlag)) {
> +    useCredentials = true;
> +  }

Setting credentials appears to be step 3.3 of HTTP Fetch.  Step 3.2, though, is:

  "Set request's skip service worker flag. "

Is this also part of the fetch event patches?

@@ +366,5 @@
>        }
>      }
>    }
> +
> +  nsCOMPtr<nsIStreamListener> listener = this;

Why is this necessary?  Can't you just pass |this| into nsCORSLIstenerProxy() below?

@@ +376,5 @@
> +  }
> +
> +  if (aCORSPreflightFlag) {
> +    nsCOMPtr<nsIChannel> preflightChannel;
> +    nsTArray<nsCString> unsafeHeaders;

nsAutoTArray

@@ +385,5 @@
> +        const InternalHeaders::Entry& header = headers[i];
> +        if (!InternalHeaders::IsSimpleHeader(header.mName, header.mValue)) {
> +          unsafeHeaders.AppendElement(header.mName);
> +        }
> +      }

Can you instead implement this as an InternalHeaders::GetUnsafeHeaders() call?

@@ +389,5 @@
> +      }
> +    }
> +    rv = NS_StartCORSPreflight(chan, corsListener, mPrincipal,
> +                               useCredentials,
> +                               unsafeHeaders, /* FIXME(nsm): Are internal headers already setup correctly due to guards or do we need to do something here? */

Can you lose this comment?  It seems the code now explicitly pulls out unsafe headers.

@@ +460,5 @@
>  {
> +  if (mObserver) {
> +    mObserver->OnResponseEnd();
> +    mObserver = nullptr;
> +  }

Can you assert this only occurs on main thread just to make it clear there is no thread race condition?

@@ +472,5 @@
> +  if (mObserver) {
> +    mObserver->OnResponseAvailable(error);
> +    mObserver->OnResponseEnd();
> +    mObserver = nullptr;
> +  }

Assert main thread for clarity, please.

@@ +600,5 @@
>                             nsresult aStatusCode)
>  {
> +  if (mPipeOutputStream) {
> +    mPipeOutputStream->Close();
> +  }

Assert main thread for clarity?

::: dom/fetch/InternalHeaders.cpp
@@ +302,5 @@
>  {
>    nsRefPtr<InternalHeaders> cors = new InternalHeaders(aHeaders->mGuard);
>    ErrorResult result;
>  
> +  nsCString acExposedNames;

nsAutoCString

@@ +308,3 @@
>    MOZ_ASSERT(!result.Failed());
>  
> +  nsTArray<nsCString> exposeNamesArray;

nsAutoTArray

@@ +314,5 @@
> +    if (token.IsEmpty()) {
> +      continue;
> +    }
> +
> +    if (!NS_IsValidHTTPToken(token)) {

Warn if we see an invalid token?

::: dom/fetch/InternalHeaders.h
@@ +90,5 @@
>    void
>    GetEntries(nsTArray<InternalHeaders::Entry>& aEntries);
> +
> +  static bool IsSimpleHeader(const nsACString& aName,
> +                             const nsACString& aValue);

Rather than exposing this, can you implement a GetUnsafeHeaders() call?  This keeps all the header looping and checking inside InternalHeaders.

::: dom/fetch/InternalRequest.cpp
@@ +29,3 @@
>  
>    copy->mBodyStream = mBodyStream;
> +  copy->mForceOriginHeader = true;

Does this ensure this part of the spec text:

  "client is entry settings object"

Or is that a future bug?  Can we comment to that effect, if so?  Just makes it easier to compare the spec to the code.

@@ +33,2 @@
>    copy->mPreserveContentCodings = true;
>  

Can you add a comment that the default referrer is already "about:client".  Just to make it easier to compare to the spec.

@@ +34,5 @@
>  
>    copy->mContext = nsIContentPolicy::TYPE_FETCH;
>    copy->mMode = mMode;
>    copy->mCredentialsMode = mCredentialsMode;
>    return copy.forget();

Do we have a bug yet to implement cache mode?  Maybe add a comment while we're here to set the cache mode when thats implemented?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8545522 [details] [diff] [review]
patch 2 - CORS credentials tests

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

So this is all handled by NS_StartCORSPreflight?

::: dom/tests/mochitest/fetch/worker_test_fetch_cors.js
@@ +795,5 @@
> +             withCred: 1,
> +             allowCred: 1,
> +           },
> +           ];
> +           // FIXME(nsm): Add "same-origin" credentials test

Can we add a follow-up bug for this?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8545524 [details] [diff] [review]
patch 3 - Implement fetch() redirects correctly

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

Looks good to me, although it seems there are a few steps from the spec missing.  It seems we could implement manual fetch and the redirect count limitation in follow-up bugs, but shouldn't we unset the same-origin data URL flag now?  r- for this issue.

::: dom/fetch/FetchDriver.cpp
@@ +611,5 @@
> +NS_IMETHODIMP
> +FetchDriver::AsyncOnChannelRedirect(nsIChannel* aOldChannel,
> +                                    nsIChannel* aNewChannel,
> +                                    uint32_t aFlags,
> +                                    nsIAsyncVerifyRedirectCallback *aCallback)

Where do you implement these steps?

  6. If request's redirect count is twenty, return a network error.
  7. Increase request's redirect count by one.
  8. Unset request's same-origin data URL flag.

And:

  10. If request's manual redirect flag is unset, run these substeps:
    1. If the CORS flag is set and locationURL's origin is not request's url's origin, set request's origin to an opaque identifier.
    2. If the CORS flag is set and either locationURL's username is not the empty string or locationURL's password is non-null, return a network error.
    3. Set request's url to locationURL.
    4. Return the result of performing a fetch using request, with the CORS flag set if set.

@@ +683,5 @@
> +FetchDriver::GetInterface(const nsIID& aIID, void **aResult)
> +{
> +  if (aIID.Equals(NS_GET_IID(nsIChannelEventSink))) {
> +    // FIXME(nsm): Set internal event sink so redirector can notify it.
> +    // mChannelEventSink = do_GetInterface(mNotificationCallbacks);

What is the impact of not doing this?  Can we write a follow-up bug now?

@@ +722,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      aResult = rv;
> +    } else {
> +      // We need to update our request's URL.
> +      nsCString newUrl;

nsAutoCString

::: dom/fetch/InternalRequest.h
@@ -65,3 @@
>      , mAuthenticationFlag(false)
>      , mForceOriginHeader(false)
> -    , mManualRedirect(false)

It seems these are still in the spec.  Why are you removing them?

Comment 8

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

Attachment: Patch 1 interdiff for comment 5

I've added some extracts from the spec which will ideally make sense.
Added GetUnsafeHeaders() and other recommendations.

In case you are wondering, mResponseAvailableCalled is a DebugOnly<> assertion I've added in dom-fetch-api Patch 5 before I land it to enforce that FetchDriver always calls OnResponseAvailable() before destructing.

Comment 9

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

(In reply to Ben Kelly [:bkelly] from comment #6)
> Comment on attachment 8545522 [details] [diff] [review]
> patch 2 - CORS credentials tests
> 
> Review of attachment 8545522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is all handled by NS_StartCORSPreflight?

That and nsCORSListenerProxy.

Comment 10

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

Attachment: Patch 3 interdiff

comments and unset same origin data URL flag.

Comment 11

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

(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8545524 [details] [diff] [review]
> patch 3 - Implement fetch() redirects correctly
> 
> Review of attachment 8545524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, although it seems there are a few steps from the spec
> missing.  It seems we could implement manual fetch and the redirect count
> limitation in follow-up bugs, but shouldn't we unset the same-origin data
> URL flag now?  r- for this issue.
> 
> ::: dom/fetch/FetchDriver.cpp
> @@ +611,5 @@
> > +NS_IMETHODIMP
> > +FetchDriver::AsyncOnChannelRedirect(nsIChannel* aOldChannel,
> > +                                    nsIChannel* aNewChannel,
> > +                                    uint32_t aFlags,
> > +                                    nsIAsyncVerifyRedirectCallback *aCallback)
> 
> Where do you implement these steps?
> 
>   6. If request's redirect count is twenty, return a network error.
>   7. Increase request's redirect count by one.

This is handled by Necko.

>   8. Unset request's same-origin data URL flag.

Done.

> 
> And:
> 
>   10. If request's manual redirect flag is unset, run these substeps:
>     1. If the CORS flag is set and locationURL's origin is not request's
> url's origin, set request's origin to an opaque identifier.
>     2. If the CORS flag is set and either locationURL's username is not the
> empty string or locationURL's password is non-null, return a network error.
>     3. Set request's url to locationURL.
>     4. Return the result of performing a fetch using request, with the CORS
> flag set if set.

Everything except step 3 handled by Necko. 3 is handled in FetchDriver::OnRedirectVerifyCallback.

> 
> @@ +683,5 @@
> > +FetchDriver::GetInterface(const nsIID& aIID, void **aResult)
> > +{
> > +  if (aIID.Equals(NS_GET_IID(nsIChannelEventSink))) {
> > +    // FIXME(nsm): Set internal event sink so redirector can notify it.
> > +    // mChannelEventSink = do_GetInterface(mNotificationCallbacks);
> 
> What is the impact of not doing this?  Can we write a follow-up bug now?
This isn't really required anymore since I grab the channel's notification callbacks in HttpFetch(). I've removed the comment.

> 
> @@ +722,5 @@
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      aResult = rv;
> > +    } else {
> > +      // We need to update our request's URL.
> > +      nsCString newUrl;
> 
> nsAutoCString
> 
> ::: dom/fetch/InternalRequest.h
> @@ -65,3 @@
> >      , mAuthenticationFlag(false)
> >      , mForceOriginHeader(false)
> > -    , mManualRedirect(false)
> 
> It seems these are still in the spec.  Why are you removing them?

For now the manual redirect flag is only used in the redirect case. It could be approximated by a local flag. If there are any uses we can add it back.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8547865 [details] [diff] [review]
Patch 3 interdiff

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

LGTM.  Thanks!

Can you post the combined patch and flag Andrea to sign off as DOM peer?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8547851 [details] [diff] [review]
Patch 1 interdiff for comment 5

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

Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +336,5 @@
> +  // true..." is handled by the CORS proxy.
> +  //
> +  // Step 3.2 "Set request's skip service worker flag." This isn't required
> +  // since Necko will fall back to the network if the ServiceWorker does not
> +  // responsd with a valid Response.

nit: respond

::: dom/fetch/InternalHeaders.cpp
@@ +307,4 @@
>    aHeaders->Get(NS_LITERAL_CSTRING("Access-Control-Expose-Headers"), acExposedNames, result);
>    MOZ_ASSERT(!result.Failed());
>  
> +  nsAutoTArray<nsCString, 0> exposeNamesArray;

I believe using 0 for nsAutoTArray size is the same as just using nsTArray.  Can you pick a reasonable value for the size here? You use 5 in FetchDriver.cpp.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8545519 [details] [diff] [review]
patch 1 - CORS support

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

::: dom/fetch/FetchDriver.cpp
@@ +117,5 @@
>    }
>  
>    bool corsPreflight = false;
>    if (mRequest->Mode() == RequestMode::Cors_with_forced_preflight ||
> +      (mRequest->UnsafeRequest() && (!mRequest->HasSimpleMethod() || !mRequest->Headers()->HasOnlySimpleHeaders()))) {

80chars

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f549cbc05cde
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f4bf6bd636
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea20fbf138a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f549cbc05cde
https://hg.mozilla.org/mozilla-central/rev/18f4bf6bd636
https://hg.mozilla.org/mozilla-central/rev/9ea20fbf138a