Closed Bug 1238950 Opened 5 years ago Closed 5 years ago

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

Categories

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

32 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bkelly, Assigned: dimi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

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: nobody → dlee
Status: NEW → ASSIGNED
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)
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)
Attached file SJS diff file (obsolete) —
Attach sjs file diff (original one is file_CrossSiteXHR_server.sjs)
Attachment #8713524 - Attachment is patch: false
Attached 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-
(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.
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-
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)
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)
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-
(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)
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff879c6f1d7b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.