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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: bkelly, Assigned: dimi)

Tracking

(Blocks 1 bug)

32 Branch
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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
(Assignee)

Updated

3 years ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Hi Ben,
I don't know how to verify the result since "no-cor" request return a opaque response, do you have any suggestion ?
Flags: needinfo?(bkelly)
I think you would need to have an sjs confirm if the cookies were sent on the server side.
Flags: needinfo?(bkelly)
(Assignee)

Comment 4

3 years ago
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.
Attachment #8713078 - Attachment is obsolete: true
Attachment #8713519 - Flags: review?(bkelly)
(Assignee)

Comment 5

3 years ago
Posted file SJS diff file (obsolete) —
Attach sjs file diff (original one is file_CrossSiteXHR_server.sjs)
(Assignee)

Updated

3 years ago
Attachment #8713524 - Attachment is patch: false
(Assignee)

Comment 6

3 years ago
Posted patch sjs diff file (obsolete) — Splinter Review
Attachment #8713524 - Attachment is obsolete: true
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.
Attachment #8713519 - Flags: review?(bkelly) → review-
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Use |Response.cloneUnfiltered| to check no-cors response status.
Attachment #8713519 - Attachment is obsolete: true
Attachment #8713528 - Attachment is obsolete: true
Attachment #8715121 - Flags: review?(bkelly)
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.
Attachment #8715121 - Flags: review?(bkelly) → review-
(Assignee)

Comment 11

3 years ago
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.
Attachment #8715121 - Attachment is obsolete: true
Attachment #8715623 - Flags: review?(bkelly)
(Assignee)

Comment 12

3 years ago
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
Attachment #8715623 - Attachment is obsolete: true
Attachment #8715623 - Flags: review?(bkelly)
(Assignee)

Comment 13

3 years ago
I didn't use Promise.all because the origin design tend to execute tests one by one. Handle promise return for |testResponse|.
Attachment #8715693 - Flags: review?(bkelly)
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"
  }
Attachment #8715693 - Flags: review?(bkelly) → review-
(Assignee)

Comment 15

3 years ago
(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.
Flags: needinfo?(bkelly)
Yea, expected status code of 500 works for me.  No problem on the delay.  Have a good holiday!
Flags: needinfo?(bkelly)
(Assignee)

Comment 17

3 years ago
Add testcases with expected status code 500.
Although this testcase doesn't use 'pass=0', i still add it to sync with other testcases.
Attachment #8715693 - Attachment is obsolete: true
Attachment #8719356 - Flags: review?(bkelly)
Comment on attachment 8719356 [details] [diff] [review]
Testcase. No-cors mode fetch with different credentials. v4

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

Thanks!
Attachment #8719356 - Flags: review?(bkelly) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff879c6f1d7b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.