Closed Bug 1119021 Opened 5 years ago Closed 5 years ago

Fetch API: Implement CORS support

Categories

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

33 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(5 files)

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).
Attachment #8545519 - Flags: review?(bkelly)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Allow request to continue when useCredentials is set.
Attachment #8545522 - Flags: review?(bkelly)
Attachment #8545519 - Attachment is obsolete: true
Attachment #8545519 - Flags: review?(bkelly)
Attachment #8545522 - Attachment is obsolete: true
Attachment #8545522 - Flags: review?(bkelly)
Attachment #8545519 - Attachment description: CORS support → patch 1 - CORS support
Attachment #8545519 - Attachment is obsolete: false
Attachment #8545519 - Flags: review?(bkelly)
Attachment #8545522 - Attachment description: CORS credentials tests → patch 2 - CORS credentials tests
Attachment #8545522 - Attachment is obsolete: false
Attachment #8545522 - Flags: review?(bkelly)
Attachment #8545524 - Attachment description: Implement fetch() redirects correctly → patch 3 - Implement fetch() redirects correctly
Sorry about the noise, bzexport messed up.
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?
Attachment #8545519 - Flags: review?(bkelly)
Attachment #8545519 - Flags: review?(amarchesini)
Attachment #8545519 - Flags: review+
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?
Attachment #8545522 - Flags: review?(bkelly) → review+
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?
Attachment #8545524 - Flags: review?(bkelly) → review-
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.
Attachment #8547851 - Flags: review?(bkelly)
Attachment #8547851 - Flags: review?(amarchesini)
(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.
comments and unset same origin data URL flag.
Attachment #8547865 - Flags: review?(bkelly)
(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 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?
Attachment #8547865 - Flags: review?(bkelly) → review+
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.
Attachment #8547851 - Flags: review?(bkelly) → review+
Attachment #8547851 - Flags: review?(amarchesini) → review+
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
Attachment #8545519 - Flags: review?(amarchesini) → review+
Depends on: 1124206
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.