Make service-workers/service-worker/fetch-request-resources.https.html pass

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dimi, Assigned: dimi)

Tracking

(Blocks 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox44 wontfix, firefox45 fixed, firefox46 fixed, firefox47 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Turns out there are some tests for this in the blink wpt tests:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html#171

But it seems we currently fail the test:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/meta/service-workers/service-worker/fetch-request-resources.https.html.ini

I'm working on getting these working over in bug 1189023, but starting with a different part of the tests.  If you want to help get the fetch-request-resources.https.html test running that would be great.
(Assignee)

Comment 2

4 years ago
(In reply to Ben Kelly [:bkelly] from comment #1)
> I'm working on getting these working over in bug 1189023, but starting with
> a different part of the tests.  If you want to help get the
> fetch-request-resources.https.html test running that would be great.

I will check if i can fix fetch-request-resources.https.html test fail
Summary: Test loading stylesheets scenarios with fetch interception → Make service-workers/service-worker/fetch-request-resources.https.html pass
Duplicate of this bug: 1181041
Blocks: 1180731
No longer blocks: ServiceWorkers-v1
In case it helps, you'll probably want to change the uses of HTTP_ORIGIN and HTTP_REMOTE_ORIGIN to HTTPS_ORIGIN to avoid mixed-content blocking.
HTTPS_ORIGIN AND HTTPS_REMOTE_ORIGIN, that is.
(Assignee)

Comment 6

4 years ago
(In reply to Josh Matthews [:jdm] from comment #4)
> In case it helps, you'll probably want to change the uses of HTTP_ORIGIN and
> HTTP_REMOTE_ORIGIN to HTTPS_ORIGIN to avoid mixed-content blocking.

It helps, thanks!
(Assignee)

Comment 7

4 years ago
Hi Ben,
I upload this patch to describe which part cause error, it did fix current testing but the solution is just temporarily.

I might need help to check if my assumptions for each part is correct and also if it is correct, what is the right way to fix it.

Thanks for help!
Attachment #8644293 - Flags: feedback?(bkelly)
(Assignee)

Updated

4 years ago
Attachment #8644293 - Attachment is patch: true
Comment on attachment 8644293 [details] [diff] [review]
Patch describing why the error occurs

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

I see the general issue with the LOAD_ANONYMOUS flag, but generally that is handled by nsCORSListenerProxy.

Ultimately, I think we should be checking the new securityFlags on the nsILoadInfo:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#90

Basically, instead of LOAD_ANONYMOUS, we should be looking to see if securityFlags has SEQ_REQUIRE_CORS_DATA_INHERITS and SEC_REQUIRE_CORS_WITH_CREDENTIALS.

Unfortunately securityFlags is not set everywhere yet, so we probably need to check securityFlags and if its SEC_NORMAL (0), then do our current LOAD_ANONYMOUS thing.  Ultimately we will want to stop using LOAD_ANONYMOUS altogether in favor of the securityFlags.

Switching to securityFlags instead of LOAD_ANONYMOUS is similar to bug 1189945, but we probably want a new bug to describe that.

::: image/imgLoader.cpp
@@ +2095,5 @@
> +     *
> +     * When loading <img>, We did not set channel flags to |nsIRequest::LOAD_ANONYMOUS| 
> +     * when aLoadFlags contains imgILoader::LOAD_CORS_ANONYMOUS
> +     */
> +    requestFlags |= nsIRequest::LOAD_ANONYMOUS;

Hmm.  It seems this should be getting set by the nsCORSListenerProxy:

  https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#998

The imgLoader does set the nsCORSListenerProxy as a listener.  So do nsScriptLoader.cpp and Loader.cpp.

Do we just intercept before the nsCORSListenerProxy can set the value?

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +214,5 @@
> +         * and then return
> +         *
> +         * So fetch event will not be called
> +         */
> +/*

If these are placed in a separate test case then they could be marked "expected fail".

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +39,5 @@
> +   * Test timeout because of fetch event is not triggered, when we use
> +   * url as FontFamily, it will show error in :
> +   * https://dxr.mozilla.org/mozilla-central/source/layout/style/FontFace.cpp?from=FontFace.cpp#208
> +   */  
> +  var fontFace = new FontFace("One", 'url(' + url + ')');

Is this behavior defined by a spec somewhere?
Attachment #8644293 - Flags: feedback?(bkelly)
(Assignee)

Comment 9

4 years ago
(In reply to Ben Kelly [:bkelly] from comment #8)
> Comment on attachment 8644293 [details] [diff] [review]
> Patch describing why the error occurs
> 
> Review of attachment 8644293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgLoader.cpp
> @@ +2095,5 @@
> > +     *
> > +     * When loading <img>, We did not set channel flags to |nsIRequest::LOAD_ANONYMOUS| 
> > +     * when aLoadFlags contains imgILoader::LOAD_CORS_ANONYMOUS
> > +     */
> > +    requestFlags |= nsIRequest::LOAD_ANONYMOUS;
> 
> Hmm.  It seems this should be getting set by the nsCORSListenerProxy:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/security/
> nsCORSListenerProxy.cpp#998
> 
> The imgLoader does set the nsCORSListenerProxy as a listener.  So do
> nsScriptLoader.cpp and Loader.cpp.
> 
> Do we just intercept before the nsCORSListenerProxy can set the value?

For imgLoader we did intercept before creating nsCORSListenerProxy.
For scrip loader and loader.cpp, we call |nsCORSListenerProxy::UpdateChannel|
before intercepting, but we will return at
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#933
(Assignee)

Comment 10

4 years ago
I am wrong about saying sw intercept imgLoader before creating nsCORSListenerProxy in Comment 9.

I just move 
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#998
before
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#933
and all image, script, css tests pass

Hi Ben,
Do you think it is ok to move the code block mentioned above ? or there is a reason we put it in the end of |nsCORSListenerProxy::UpdateChannel|.

O I should solve this by checking securityFlags as you mentioned in Comment 8
Flags: needinfo?(bkelly)
Hmm, this would change how we handle cookies for same origin requests, right?  I don't think we want to make that change.

Jonas, can you confirm that the change suggested in comment 10 is not ok?

The real problem here is that we're setting RequestCredentials based on whether we actually anonymize cookies or not.  Instead RequestCredentials needs to reflect the CORS policy selected (regardless of what would have been done on the wire).

So I recommend writing the code to use securityFlags, although it may not make these tests pass until all the network call sites have been updated to set securityFlags properly.
Flags: needinfo?(bkelly) → needinfo?(jonas)
Yes, I think that would mean, for example, that XHR would never send cookies. Which would break a lot of websites.
Flags: needinfo?(jonas)
(Assignee)

Comment 13

4 years ago
Hi Ben,
For img, script and css, we have 3 different tests with crossOrigin attribute set to:
1.''
2.'anonymous'
3.'use-credentials'

so the expected security flag will be 
1. SEC_NORMAL
2. SEC_REQUIRE_CORS_DATA_INHERITS
3. SEC_REQUIRE_CORS_DATA_INHERITS and SEC_REQUIRE_CORS_WITH_CREDENTIALS

Is that correct ?
Flags: needinfo?(bkelly)
I need to look closer at this later, but SEC_NORMAL is going to go away.  So we need another value there.  SEC_NORMAL basically means that the securityMode is not set in the securityFlags.
A normal <img>, with no crossOrigin attribute set, will use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
It looks like <script> and <style> should also get some variant of SEC_ALLOW_CROSS_ORIGIN_* when .crossOrigin attribute is not set, right?
Flags: needinfo?(bkelly)
Correct. They should both get SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
(Assignee)

Comment 18

4 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Hi Ben,
This patch update the security flag for img, script and style, but I did not change SEC_NORMAL to SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS because I am not sure if I should do it in this patch or it belongs to other bugs.

And also about font_face_test, I don't know what is correct expect result, following is current testcase
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html?offset=0#180

From my test, two tests will both create nsCORSListenerProxy hence request mode will both be 'cors', and what should be the expected credentials ?
Attachment #8644293 - Attachment is obsolete: true
Attachment #8657789 - Flags: feedback?(bkelly)
Comment on attachment 8657789 [details] [diff] [review]
Patch v1

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

Overall this looks good.  I made some notes about changes that are needed, though.

::: dom/base/nsScriptLoader.cpp
@@ +283,5 @@
>    if (mDocument->GetSandboxFlags() & SANDBOXED_SCRIPTS) {
>      return NS_OK;
>    }
>  
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

I believe this should default to SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3569,5 @@
>            MOZ_CRASH("Unexpected CORS mode");
>        }
>  
> +      nsSecurityFlags securityFlags = loadInfo->GetSecurityMode();
> +      if (securityFlags & nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {

Please rebase as this method has changed.  In particular, this code now looks at securityFlags to set the mode:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp?offset=600#268

@@ +3580,3 @@
>            mRequestCredentials = RequestCredentials::Include;
> +        } else {
> +          mRequestCredentials = RequestCredentials::Omit;

You're correct, though, we should also use securityFlags to set RequestCredentials if its not SEC_NORMAL.  We probably need another helper method like I made for the RequestMode (linked above).

::: image/imgLoader.cpp
@@ +1592,5 @@
>  
>      return NS_SUCCEEDED(rv);
>  
>    } else {
> +    nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

This should SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS.

@@ +2092,5 @@
>      // Propagate background loading...
>      requestFlags |= nsIRequest::LOAD_BACKGROUND;
>    }
>  
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS

::: layout/style/Loader.cpp
@@ +1546,1 @@
>    nsLoadFlags securityFlags = nsILoadInfo::SEC_NORMAL;

SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
Attachment #8657789 - Flags: feedback?(bkelly)
Regarding font face, I believe they should always be loaded via cors with anonymous credentials:

 https://drafts.csswg.org/css-fonts/#font-fetching-requirements

So the RequestCredentials should be set to "omit".

Anne, do you agree?

Dimi, so I think you should change the test to expect the 'cors' RequestMode and 'omit' RequestCredentials.
Flags: needinfo?(annevk)

Comment 21

4 years ago
HTML's crossorigin=anonymous maps to credentials mode "same-origin" as far as I know. I would have liked for it to be "omit", but I don't think it's implemented as such.
Flags: needinfo?(annevk)

Comment 22

4 years ago
Dimi, are you still working on this?
(Assignee)

Comment 23

4 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #22)
> Dimi, are you still working on this?

Sorry because of PTO and other issues so this is pending for a while.
I am not working on it right now but I plan to work on this next week.

Please feel free to take it.

Updated

4 years ago
Depends on: 1216687

Updated

4 years ago
Assignee: dlee → ehsan

Updated

4 years ago
Depends on: 1216695

Updated

4 years ago

Comment 24

4 years ago
Given that bug 1216687 hasn't been fixed yet, there is a good chance that I won't have time to work on this.
Assignee: ehsan → nobody
Status: ASSIGNED → NEW

Updated

4 years ago
Blocks: 1189023
No longer blocks: 1180731
(Assignee)

Comment 25

3 years ago
I would like to take this bug
Assignee: nobody → dlee
Status: NEW → ASSIGNED
(Assignee)

Comment 26

3 years ago
Posted patch WIP patch (obsolete) — Splinter Review
For backup
Attachment #8657789 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Hi Ben,
I have update the patch based on the fix in Bug 1216687, but I still have some questions,
1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security flag for image, and stylesheet, It will fail at "contentPolicyType not supported yet"[1] in AsyncOpen2
So should i just add cookie policy to security flag for now ?

2. After applying this patch, the test case fetch-request-xhr.https.html will fail when xhr send to a remote origin and |withCredentials| is not set (The expected credentials is 'omit' but will get 'same-origin'). Originally, 'omit' is set when there is |LOAD_ANONYMOUS|, and in this case, |LOAD_ANONYMOUS| is set in |nsContentSecurityManager::CheckChannel|[2].
But after replacing |LOAD_ANONYMOUS| with security flags, the cookie policy in this case is still 'same-origin', do you have any suggestion ?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#150
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#452
Flags: needinfo?(bkelly)
(In reply to Dimi Lee[:dimi][:dlee] from comment #27)
> 1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security
> flag for image, and stylesheet, It will fail at "contentPolicyType not
> supported yet"[1] in AsyncOpen2
> So should i just add cookie policy to security flag for now ?

I'm not sure.  Jonas, is it safe to use the new SEC_COOKIES_* flags without AsyncOpen2?  Or is there a better fix for the issue Dimi is running into here?

> 2. After applying this patch, the test case fetch-request-xhr.https.html
> will fail when xhr send to a remote origin and |withCredentials| is not set
> (The expected credentials is 'omit' but will get 'same-origin').

That test is wrong, then.  XHR should be using 'same-origin'.  We had no way to do 'omit' before the fetch() API was added.
Flags: needinfo?(bkelly) → needinfo?(jonas)
(In reply to Ben Kelly [:bkelly] from comment #28)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #27)
> > 1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security
> > flag for image, and stylesheet, It will fail at "contentPolicyType not
> > supported yet"[1] in AsyncOpen2
> > So should i just add cookie policy to security flag for now ?
> 
> I'm not sure.  Jonas, is it safe to use the new SEC_COOKIES_* flags without
> AsyncOpen2?

The SEC_COOKIES_* flags only work if you use AsyncOpen2. Otherwise they have no effect.

> Or is there a better fix for the issue Dimi is running into here?

If you are getting to that line, then the call was changed to use AsyncOpen2. Dimi, is it correct that you changed to use AsyncOpen2?

Note though that you should not change to use AsyncOpen2 unless you make sure to pass a correct loadingPrincipal and triggeringPrincipal. However especially for image loads that is a lot of work. It's work that would be great to get help with though, but it's definitely work that should be done in a separate bug focused only on the image code.


All that said, I'm not sure what problem we are trying to solve in this bug. I.e. I don't know why fetch-request-resources.https.html is failing. So I can't really help with advice for how to fix it.
Flags: needinfo?(jonas)
(Assignee)

Comment 30

3 years ago
(In reply to Jonas Sicking (:sicking) from comment #29)
> If you are getting to that line, then the call was changed to use
> AsyncOpen2. Dimi, is it correct that you changed to use AsyncOpen2?

Yes, in the WIP patch I use AyncOpen2 to pass SEC_COOKIES_* flags.

> Note though that you should not change to use AsyncOpen2 unless you make
> sure to pass a correct loadingPrincipal and triggeringPrincipal. However
> especially for image loads that is a lot of work. It's work that would be
> great to get help with though, but it's definitely work that should be done
> in a separate bug focused only on the image code.

Hi Ben,
For now only ScriptLoader.cpp use AsyncOpen2, stylesheet, font, and image are all using AsyncOpen.
According to jonas's comment, it could be lots effort to change to AsyncOpen2.

I think we could use security flags to map to request crendential first and enable only script test.
As for stylesheet, font, and image, these could be enabled after AsynOpen2 is supported accordingly.
How do you think ?
Flags: needinfo?(bkelly)
(In reply to Dimi Lee[:dimi][:dlee] from comment #30)
> I think we could use security flags to map to request crendential first and
> enable only script test.
> As for stylesheet, font, and image, these could be enabled after AsynOpen2
> is supported accordingly.
> How do you think ?

I think that works.  What credentials would we supply for stylesheet, font, and image requests?  Are they more restrictive than the correct values?

I just want to make sure we don't create a security issue.
Flags: needinfo?(bkelly)
(Assignee)

Comment 32

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #31)
> I think that works.  What credentials would we supply for stylesheet, font,
> and image requests?  Are they more restrictive than the correct values?
> 
> I just want to make sure we don't create a security issue.

[1]Current implementation:
"no-cors" request         , correct value will be 'include' , got 'same origin'
"anonymouse" request      , correct value will be 'omit'    , got 'same origin'
"use-credentials" request , correct value will be 'include' , got 'include'

[2]Remove current implementation and map security flag to crendential mode
"no-cors" request         , correct value will be 'include' , got 'include'
"anonymouse" request      , correct value will be 'omit'    , got 'include'
"use-credentials" request , correct value will be 'include' , got 'include'

[3]Check SEC_NORMAL flag first, if exists, apply original implemetation,otherwise
map security flag to crendential mode.

In this approach, stylesheet, font and image will get original result[1], for script, it
will get correct crendential mode.

Do you think we can use approach 3 ? because all credential mode become 'include' may have security issue.
Flags: needinfo?(bkelly)

Comment 33

3 years ago
For crossorigin=anonymous the expected credentials mode is "same-origin" due to legacy. ("no-cors" should be "include" though.)
I don't think we can do (2), so I guess (3) is the best bet.  Also, double check the expectations against Anne's info in comment 33.  Thanks!  Sorry for the delay in responding.
Flags: needinfo?(bkelly)
(Assignee)

Comment 35

3 years ago
(In reply to Anne (:annevk) from comment #33)
> For crossorigin=anonymous the expected credentials mode is "same-origin" due
> to legacy. ("no-cors" should be "include" though.)

Thanks for the information!
(Assignee)

Comment 36

3 years ago
Attachment #8702175 - Attachment is obsolete: true
Attachment #8710326 - Flags: review?(bkelly)
Comment on attachment 8710326 [details] [diff] [review]
Fix fetch-request-resources.https.html v1

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

This is a good improvement.  I think we may end up still failing the tests until AsyncOpen2() is complete, but this is still worth doing.  r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +297,5 @@
>      aRequest->mCORSMode == CORS_NONE
>      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
>      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;

This should be SEC_COOKIES_SAME_ORIGIN, no?

::: dom/fetch/InternalRequest.cpp
@@ +342,5 @@
> +    uint32_t loadFlags;
> +    aChannel->GetLoadFlags(&loadFlags);
> +
> +    if (loadFlags & nsIRequest::LOAD_ANONYMOUS) {
> +      return RequestCredentials::Same_origin;

I think in this case its safer to just use "omit" here.  Once we set LOAD_ANONYMOUS we should ensure its never removed from the request.

We could accidentally map a cross-origin request with same-origin credentials to omit here, but thats ok because omit and same-origin are the same effect for cross-origin urls; don't send credentials.

@@ +360,5 @@
> +
> +  if (cookiePolicy == nsILoadInfo::SEC_COOKIES_INCLUDE) {
> +    return RequestCredentials::Include;
> +  } else if (cookiePolicy == nsILoadInfo::SEC_COOKIES_OMIT) {
> +    return RequestCredentials::Same_origin;

This should be RequestCredentials::Omit.

@@ +367,5 @@
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("Unexpected cookie policy!");
> +  }
> +
> +  return RequestCredentials::Same_origin;

Just put the MOZ_ASSERT_UNREACHABLE() here instead of returning.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-css-images.https.html
@@ +86,5 @@
> +        css_image_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'include');
> +        css_image_test(f, REMOTE_URL, 'backgroundImage', 'no-cors', 'include');
> +
> +        css_image_test(f, LOCAL_URL, 'shapeOutside', 'cors', 'omit');
> +        css_image_test(f, REMOTE_URL, 'shapeOutside', 'cors', 'omit');

These should probably be 'same-origin'.  Only fetch() can produce an 'omit' legitimately.  All pre-existing APIs should result in either 'include' or 'same-origin'.

@@ +89,5 @@
> +        css_image_test(f, LOCAL_URL, 'shapeOutside', 'cors', 'omit');
> +        css_image_test(f, REMOTE_URL, 'shapeOutside', 'cors', 'omit');
> +
> +        css_image_set_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'same-origin');
> +        css_image_set_test(f, REMOTE_URL, 'backgroundImage', 'no-cors', 'same-origin');

Similarly, these should probably be 'include' for no-cors requests.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +30,5 @@
>    document.body.appendChild(link);
>  }
>  
>  function load_font(url) {
> +  var fontFace = new FontFace('test', 'url(' + url + ')');

Why this change?

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-xhr-iframe.https.html
@@ +136,5 @@
>          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
>        })
>      .then(function(response){
>          assert_equals(response.mode, 'cors');
> +        assert_equals(response.credentials, 'same-origin');

This should indeed be 'same-origin', but until we convert everything to AsyncOpen2() we may need to leave this returning 'omit'.
Attachment #8710326 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [:bkelly] from comment #37)
> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-xhr-iframe.https.html
> @@ +136,5 @@
> >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> >        })
> >      .then(function(response){
> >          assert_equals(response.mode, 'cors');
> > +        assert_equals(response.credentials, 'same-origin');
> 
> This should indeed be 'same-origin', but until we convert everything to
> AsyncOpen2() we may need to leave this returning 'omit'.

To clarify, I think we should change the test to 'same-origin' to be correct, but we will need to keep failing the test until AsyncOpen2() is done.
(Assignee)

Comment 39

3 years ago
Thanks for your review!
ni because I have a question about xhr.

(In reply to Ben Kelly [:bkelly] from comment #37)
> ::: dom/base/nsScriptLoader.cpp
> @@ +297,5 @@
> >      aRequest->mCORSMode == CORS_NONE
> >      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> >      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> > +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> > +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;
> 
> This should be SEC_COOKIES_SAME_ORIGIN, no?
>

Ah...I misunderstand Anne's info in comment 33. I thought CORS_ANONYMOUS maps to SEC_COOKIES_OMIT, and SEC_COOKIES_OMIT maps RequestCredentials::Same_origin.
I will fix this in next patch

> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-resources-iframe.https.html
> @@ +30,5 @@
> >    document.body.appendChild(link);
> >  }
> >  
> >  function load_font(url) {
> > +  var fontFace = new FontFace('test', 'url(' + url + ')');
> 
> Why this change?
> 

Pass first argument with url cause error return at[1]
So fetch event will not be triggered.
I just grep testcases that use FontFace and found many of them use 'test'
I guess maybe pass url make parsing error somewhere.

> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-xhr-iframe.https.html
> @@ +136,5 @@
> >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> >        })
> >      .then(function(response){
> >          assert_equals(response.mode, 'cors');
> > +        assert_equals(response.credentials, 'same-origin');
> 
> This should indeed be 'same-origin', but until we convert everything to
> AsyncOpen2() we may need to leave this returning 'omit'.

Sorry, maybe a dumb question here, xhr request already use AsyncOpen2, why do we need everything change to use AsyncOpen2 ?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/FontFace.cpp?from=FontFace.cpp#208
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#2909
Flags: needinfo?(bkelly)
(In reply to Dimi Lee[:dimi][:dlee] from comment #39)
> > ::: dom/base/nsScriptLoader.cpp
> > @@ +297,5 @@
> > >      aRequest->mCORSMode == CORS_NONE
> > >      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> > >      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> > > +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> > > +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;
> > 
> > This should be SEC_COOKIES_SAME_ORIGIN, no?
> >
> 
> Ah...I misunderstand Anne's info in comment 33. I thought CORS_ANONYMOUS
> maps to SEC_COOKIES_OMIT, and SEC_COOKIES_OMIT maps
> RequestCredentials::Same_origin.
> I will fix this in next patch

Yea, SEC_COOKIES_* and RequestCredentials::* values should map one-to-one I believe:

  SEC_COOKIES_INCLUDE = RequestCredentials::Include
  SEC_COOKIES_SAME_ORIGIN = RequestCredentials::SameOrigin
  SEC_COOKIES_OMIT = RequestCredentials::Omit

The spec concept of "cors anonymous" maps to RequestCredentials::SameOrigin (and SEC_COOKIES_SAME_ORIGIN).  All other legacy APIs use RequestCredentials::Include (and SEC_COOKIES_INCLUDE).

The fetch() API is the one API that can now do a request with RequestCredentials::Omit.  This is only possible because fetch uses the AsyncOpen2() and SEC_COOKIES_OMIT.

Thats the desired long term situation with AsyncOpen2().  For now obviously we have to infer RequestCredentials values from LOAD_ANONYMOUS.  This is ambiguous since both same-origin and omit can result in LOAD_ANONYMOUS being set.

> > :::
> > testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> > fetch-request-xhr-iframe.https.html
> > @@ +136,5 @@
> > >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> > >        })
> > >      .then(function(response){
> > >          assert_equals(response.mode, 'cors');
> > > +        assert_equals(response.credentials, 'same-origin');
> > 
> > This should indeed be 'same-origin', but until we convert everything to
> > AsyncOpen2() we may need to leave this returning 'omit'.
> 
> Sorry, maybe a dumb question here, xhr request already use AsyncOpen2, why
> do we need everything change to use AsyncOpen2 ?

Oh, ok.  For some reason I thought this was still using the old path.  If its using AsyncOpen2(), then yes it should pass.

Thanks again!
Flags: needinfo?(bkelly)
(Assignee)

Comment 41

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #40)
> 
> Yea, SEC_COOKIES_* and RequestCredentials::* values should map one-to-one I
> believe:
> 
>   SEC_COOKIES_INCLUDE = RequestCredentials::Include
>   SEC_COOKIES_SAME_ORIGIN = RequestCredentials::SameOrigin
>   SEC_COOKIES_OMIT = RequestCredentials::Omit
> 
> The spec concept of "cors anonymous" maps to RequestCredentials::SameOrigin
> (and SEC_COOKIES_SAME_ORIGIN).  All other legacy APIs use
> RequestCredentials::Include (and SEC_COOKIES_INCLUDE).
> 
> The fetch() API is the one API that can now do a request with
> RequestCredentials::Omit.  This is only possible because fetch uses the
> AsyncOpen2() and SEC_COOKIES_OMIT.
> 
> Thats the desired long term situation with AsyncOpen2().  For now obviously
> we have to infer RequestCredentials values from LOAD_ANONYMOUS.  This is
> ambiguous since both same-origin and omit can result in LOAD_ANONYMOUS being
> set.

Thanks for the clear information :D
(Assignee)

Comment 42

3 years ago
Attachment #8710326 - Attachment is obsolete: true
Attachment #8711598 - Flags: review+

Comment 45

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74ab992b6952
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8711598 [details] [diff] [review]
Fix fetch-request-resources.https.html v2

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: A large website has reported their users are seeing failures due cookies not be sent via service worker pass through requests.
[Describe test coverage new/current, TreeHerder]: WPT tests included.  I tested locally on a beta 45 and verified it fixes the issue.
[Risks and why]: Minimal.  Should only effect service workers.
[String/UUID change made/needed]: None.
Attachment #8711598 - Flags: approval-mozilla-beta?
Attachment #8711598 - Flags: approval-mozilla-aurora?
I had to rebase slightly for beta branch.
Comment on attachment 8711598 [details] [diff] [review]
Fix fetch-request-resources.https.html v2

Improve the feature, have plenty of tests, taking it.
Should be in 45 beta 7
Attachment #8711598 - Flags: approval-mozilla-beta?
Attachment #8711598 - Flags: approval-mozilla-beta+
Attachment #8711598 - Flags: approval-mozilla-aurora?
Attachment #8711598 - Flags: approval-mozilla-aurora+
service-workers/service-worker/fetch-request-css-images.https.html didn't get added to the test manifest, so afaict it's not actually running in automation.  Is that intended?
Flags: needinfo?(dlee)
I recommend filing a separate bug to update the manifest for this test.  This bug was uplifted, but we probably don't need to uplift the manifest change.
Note that as khuey discovered when he accidentally added the test to the manifest today, the test totally doesn't pass.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I repeat, this should be fixed in a new bug.  I opened bug 1251792.

The failure of the test is not a crisis.  It was broken out from a larger test file with known expected failures.  It probably just needs an appropriate .ini file.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: needinfo?(dlee)
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1251878
You need to log in before you can comment on or make changes to this bug.