Closed
Bug 1119021
Opened 10 years ago
Closed 10 years ago
Fetch API: Implement CORS support
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(5 files)
50.13 KB,
patch
|
bkelly
:
review+
baku
:
review+
|
Details | Diff | Splinter Review |
6.02 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
28.96 KB,
patch
|
bkelly
:
review-
|
Details | Diff | Splinter Review |
16.52 KB,
patch
|
bkelly
:
review+
baku
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Allow request to continue when useCredentials is set.
Attachment #8545522 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8545519 -
Attachment is obsolete: true
Attachment #8545519 -
Flags: review?(bkelly)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8545524 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8545522 -
Attachment is obsolete: true
Attachment #8545522 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8545519 -
Attachment description: CORS support → patch 1 - CORS support
Attachment #8545519 -
Attachment is obsolete: false
Attachment #8545519 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8545522 -
Attachment description: CORS credentials tests → patch 2 - CORS credentials tests
Attachment #8545522 -
Attachment is obsolete: false
Attachment #8545522 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8545524 -
Attachment description: Implement fetch() redirects correctly → patch 3 - Implement fetch() redirects correctly
Assignee | ||
Comment 4•10 years ago
|
||
Sorry about the noise, bzexport messed up.
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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 7•10 years ago
|
||
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-
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
comments and unset same origin data URL flag.
Attachment #8547865 -
Flags: review?(bkelly)
Assignee | ||
Comment 11•10 years ago
|
||
(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•10 years ago
|
||
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 13•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8547851 -
Flags: review?(amarchesini) → review+
Comment 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f549cbc05cde
https://hg.mozilla.org/mozilla-central/rev/18f4bf6bd636
https://hg.mozilla.org/mozilla-central/rev/9ea20fbf138a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•