1238950 - test that fetch() no-cors requests properly support same-origin/include/omit credentials

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

I believe we already support the various credentials values for no-cors requests, but we don't have tests.  We should write some.  The spec was recently updated to support all credentials values for no-cors.  See:

  https://github.com/whatwg/fetch/commit/4147978673c15047d1a5d4a76b1a403a7d75a956

Comment 1

[Dimi Lee [:dimi]]

Attachment: Testcase for fetch no-cors mode with different credentials

Comment 2

[Dimi Lee [:dimi]]

Hi Ben,
I don't know how to verify the result since "no-cor" request return a opaque response, do you have any suggestion ?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think you would need to have an sjs confirm if the cookies were sent on the server side.

Comment 4

[Dimi Lee [:dimi]]

Attachment: Testcase. No-cors mode fetch with different credentials.

I am not sure if I understand correctly, the approach here is record the result of "no-cors" request in sjs and send another cors request to retrieve it.

Create another sjs file because this is quite specific for this test.

Comment 5

[Dimi Lee [:dimi]]

Attachment: SJS diff file

Attach sjs file diff (original one is file_CrossSiteXHR_server.sjs)

Comment 6

[Dimi Lee [:dimi]]

Attachment: sjs diff file

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8713519 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials.

I see what you mean.  I had not considered that the resulting opaque response hides the result from you.

We do have Response.cloneUnfiltered() as a chrome-only method, though.  Perhaps you could use SpecialPowers with that to extract the result?  See:

https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_response.js#70

Then you could avoid some of the sjs complexity.

Comment 8

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #7)
> We do have Response.cloneUnfiltered() as a chrome-only method, though. 
> Perhaps you could use SpecialPowers with that to extract the result?  See:

Thanks! I will update in next patch.

Comment 9

[Dimi Lee [:dimi]]

Attachment: Testcase. No-cors mode fetch with different credentials. v2

Use |Response.cloneUnfiltered| to check no-cors response status.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8715121 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials. v2

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

Thanks!  I just would like a bit of restructuring to wait for the test results, though.  I'm afraid the test is completing before all of the assertions are run right now.

::: dom/tests/mochitest/fetch/test_fetch_cors.js
@@ +1119,5 @@
> +              {
> +                pass: 1,
> +                setCookie: cookieStr,
> +                withCred: "include",
> +              },

Please add a comment that the first entry here is setting up the cookie that will be used in the following test cases.

@@ +1133,5 @@
> +              },
> +              {
> +                pass: 1,
> +                cookie: cookieStr,
> +                withCred: "include",

Can you add a test case with "pass: 0" to verify that checking is being performed as expected?

@@ +1166,5 @@
> +    ok(test.pass, "Expected test to pass " + test.toSource());
> +    is(res.type, 'opaque', 'wrong response type for ' + test.toSource());
> +
> +    // Get unfiltered response
> +    if (typeof SpecialPowers !== 'object') {

Please move the specialpowers check to the top of the test.  I'm not sure it makes sense to run this test at all if we can't inspect the response values.

@@ +1184,5 @@
> +  function runATest(tests, i) {
> +    var test = tests[i];
> +    var request = makeRequest(test);
> +    fetch(request).then(function(res) {
> +      testResponse(res, test);

The testResponse() function returns a promise, but you don't actually wait for it to complete here.

I suggest wrapping the contents of testResponse() in an explicit promise so that it always completes async instead of sometimes sync.

Then you can collect all the promises and use Promise.all() to know when everything is complete.

Comment 11

[Dimi Lee [:dimi]]

Attachment: Testcase. No-cors mode fetch with different credentials. v3

Hi Ben,
I remove the |pass| from this testcase because i could not find a good way to trigger that in no-cors mode. Incorrect parameters for no-cors mode are already handled by Request ctor and there are already testcases to test no-cors Request ctor.

Comment 12

[Dimi Lee [:dimi]]

Comment on attachment 8715623 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials. v3

Miss something in this patch, will upload again later

Comment 13

[Dimi Lee [:dimi]]

Attachment: Testcase. No-cors mode fetch with different credentials. v3

I didn't use Promise.all because the origin design tend to execute tests one by one. Handle promise return for |testResponse|.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8715693 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials. v3

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

Sorry, I really would like to see a failing case in the test to make sure this is all working right.  I think you can do it by changing the params that set expectations in the sjs.

::: dom/tests/mochitest/fetch/test_fetch_cors.js
@@ +1132,5 @@
> +                withCred: "same-origin",
> +              },
> +              {
> +                cookie: cookieStr,
> +                withCred: "include",

I think you could make a pass:0 case by doing something like:

  {
    pass: 0,
    noCookie: 1,
    withCred: "include"
  },
  {
    pass: 0,
    cookie: cookieStr,
    withCred: "same-origin"
  }

Comment 15

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #14)
> Comment on attachment 8715693 [details] [diff] [review]
> Testcase. No-cors mode fetch with different credentials. v3
> 
> Review of attachment 8715693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I really would like to see a failing case in the test to make sure
> this is all working right.  I think you can do it by changing the params
> that set expectations in the sjs.
> 
> ::: dom/tests/mochitest/fetch/test_fetch_cors.js
> @@ +1132,5 @@
> > +                withCred: "same-origin",
> > +              },
> > +              {
> > +                cookie: cookieStr,
> > +                withCred: "include",
> 
> I think you could make a pass:0 case by doing something like:
> 
>   {
>     pass: 0,
>     noCookie: 1,
>     withCred: "include"
>   },
>   {
>     pass: 0,
>     cookie: cookieStr,
>     withCred: "same-origin"
>   }

The pass value in this testcase is to get fetch exception, but change parameters cause exception in sjs only make response(filtered) with fail status code.
I could add fail testcase with expected status code, 500 for example, but the pass value will still be true here.
Is this what you expected ?

BTW, I'll have CHINESE NEW YEAR next week so my response may be late, sorry in advance.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Yea, expected status code of 500 works for me.  No problem on the delay.  Have a good holiday!

Comment 17

[Dimi Lee [:dimi]]

Attachment: Testcase. No-cors mode fetch with different credentials. v4

Add testcases with expected status code 500.
Although this testcase doesn't use 'pass=0', i still add it to sync with other testcases.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8719356 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials. v4

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

Thanks!

Comment 19

[Dimi Lee [:dimi]]

try:https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a6f85b93a71&selectedJob=16753163

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff879c6f1d7b

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff879c6f1d7b