Allow for redirects after a CORS-preflight #5107

ferjm · 2017-03-09T18:08:55Z

This PR tries to address the changes on the Fetch spec introduced to allow redirects after CORS-preflitghts.

I took the opportunity to do some code clean up as well.

wpt-pr-bot · 2017-03-09T18:08:57Z

Notifying @annevk, @hillbrad, @jdm, @odinho, and @zqzhang. (Learn how reviewing works.)

w3c-bots · 2017-03-09T18:15:08Z

Firefox (nightly channel)

Testing web-platform-tests at revision c531fe0
Using browser at version BuildID 20170306110339; SourceStamp 966464a68a2cb3ca1125808e34abb5c1d34e3797
Starting 10 test iterations
All results were stable

All results

2 tests ran

/cors/preflight-failure.htm

Subtest	Results	Messages
	TIMEOUT
`Should throw error if preflight respond with 100`	TIMEOUT	`Test timed out`
`Should throw error if preflight respond with 101`	PASS
`Should throw error if preflight respond with 300`	PASS
`Should throw error if preflight respond with 301`	PASS
`Should throw error if preflight respond with 302`	PASS
`Should throw error if preflight respond with 303`	PASS
`Should throw error if preflight respond with 304`	PASS
`Should throw error if preflight respond with 305`	PASS
`Should throw error if preflight respond with 306`	PASS
`Should throw error if preflight respond with 307`	PASS
`Should throw error if preflight respond with 308`	PASS
`Should throw error if preflight respond with 400`	PASS
`Should throw error if preflight respond with 401`	PASS
`Should throw error if preflight respond with 402`	PASS
`Should throw error if preflight respond with 403`	PASS
`Should throw error if preflight respond with 404`	PASS
`Should throw error if preflight respond with 405`	PASS
`Should throw error if preflight respond with 406`	PASS
`Should throw error if preflight respond with 407`	PASS
`Should throw error if preflight respond with 408`	PASS
`Should throw error if preflight respond with 409`	PASS
`Should throw error if preflight respond with 410`	PASS
`Should throw error if preflight respond with 411`	PASS
`Should throw error if preflight respond with 412`	PASS
`Should throw error if preflight respond with 413`	PASS
`Should throw error if preflight respond with 414`	PASS
`Should throw error if preflight respond with 415`	PASS
`Should throw error if preflight respond with 416`	PASS
`Should throw error if preflight respond with 417`	PASS
`Should throw error if preflight respond with 500`	PASS
`Should throw error if preflight respond with 501`	PASS
`Should throw error if preflight respond with 502`	PASS
`Should throw error if preflight respond with 503`	PASS
`Should throw error if preflight respond with 504`	PASS
`Should throw error if preflight respond with 505`	PASS

/cors/redirect-preflight.htm

Subtest	Results	Messages
	OK
`Should allow redirect 301 after succesful (200) preflight`	FAIL	`assert_equals: Successfull redirect expected 200 but got 0`
`Should allow redirect 302 after succesful (200) preflight`	FAIL	`assert_equals: Successfull redirect expected 200 but got 0`
`Should allow redirect 303 after succesful (200) preflight`	FAIL	`assert_equals: Successfull redirect expected 200 but got 0`
`Should allow redirect 307 after succesful (200) preflight`	FAIL	`assert_equals: Successfull redirect expected 200 but got 0`
`Should allow redirect 308 after succesful (200) preflight`	FAIL	`assert_equals: Successfull redirect expected 200 but got 0`

w3c-bots · 2017-03-09T18:15:09Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision c531fe0
Using browser at version 58.0.3029.14 dev
Starting 10 test iterations
All results were stable

All results

2 tests ran

/cors/preflight-failure.htm

Subtest	Results	Messages
	OK
`Should throw error if preflight respond with 100`	PASS
`Should throw error if preflight respond with 101`	PASS
`Should throw error if preflight respond with 300`	PASS
`Should throw error if preflight respond with 301`	PASS
`Should throw error if preflight respond with 302`	PASS
`Should throw error if preflight respond with 303`	PASS
`Should throw error if preflight respond with 304`	PASS
`Should throw error if preflight respond with 305`	PASS
`Should throw error if preflight respond with 306`	PASS
`Should throw error if preflight respond with 307`	PASS
`Should throw error if preflight respond with 308`	PASS
`Should throw error if preflight respond with 400`	PASS
`Should throw error if preflight respond with 401`	PASS
`Should throw error if preflight respond with 402`	PASS
`Should throw error if preflight respond with 403`	PASS
`Should throw error if preflight respond with 404`	PASS
`Should throw error if preflight respond with 405`	PASS
`Should throw error if preflight respond with 406`	PASS
`Should throw error if preflight respond with 407`	PASS
`Should throw error if preflight respond with 408`	PASS
`Should throw error if preflight respond with 409`	PASS
`Should throw error if preflight respond with 410`	PASS
`Should throw error if preflight respond with 411`	PASS
`Should throw error if preflight respond with 412`	PASS
`Should throw error if preflight respond with 413`	PASS
`Should throw error if preflight respond with 414`	PASS
`Should throw error if preflight respond with 415`	PASS
`Should throw error if preflight respond with 416`	PASS
`Should throw error if preflight respond with 417`	PASS
`Should throw error if preflight respond with 500`	PASS
`Should throw error if preflight respond with 501`	PASS
`Should throw error if preflight respond with 502`	PASS
`Should throw error if preflight respond with 503`	PASS
`Should throw error if preflight respond with 504`	PASS
`Should throw error if preflight respond with 505`	PASS

/cors/redirect-preflight.htm

Subtest	Results	Messages
	OK
`Should allow redirect 301 after succesful (200) preflight`	PASS
`Should allow redirect 302 after succesful (200) preflight`	PASS
`Should allow redirect 303 after succesful (200) preflight`	PASS
`Should allow redirect 307 after succesful (200) preflight`	PASS
`Should allow redirect 308 after succesful (200) preflight`	PASS

annevk

Another thing we might want to do is to turn these into asynchronous XMLHttpRequest so they don't end up blocking each other. Given the cache busting that should be fine...

annevk · 2017-03-10T07:37:52Z

cors/redirect-preflight.htm

        assert_throws(null, function() { client.send(null) });

-    },
-    'Redirect ' + code + ' on preflight')
+    }, 'Redirect ' + code + ' on preflight');


So these tests are not actually checking redirects as much as they are checking that a non-2xx status code on preflights causes failure?

Yes.

The spec says in steps 6 and 7 of CORS preflight fetch:

(6) If a CORS check for request and response returns success and response’s status is an ok status, run these substeps [...]. (7) Otherwise, return a network error.

There is no explicit mention about 3XX redirection status codes. So maybe this is not the best place for this test? Is this what you are suggesting with your comment?

I was mostly curious. At some point we should probably test other status codes too... I don't really know how much effort you want to put into cleaning up the test suite though. Seems fine to me if you just do it as you come across stuff.

(I've been slowly making my way through the web-platform-tests backlog for Fetch and friends, but still lots that can be done.)

I'm happy to help with the clean up :). It should be easy to test failures for all non-2XX status codes on preflights, so I'll do it in this PR, along with the change to async XHR. Thanks for the feedback!

ferjm · 2017-03-10T15:15:33Z

Sure. I'll turn the tests into async XHRs.

w3c-bots · 2017-03-11T12:10:05Z

These tests are now available on w3c-test.org

annevk · 2017-03-13T07:32:44Z

Sigh. I think to fix the build error you need to rebase on latest master and force push. Not sure what else can be it.

annevk · 2017-03-13T07:42:05Z

Never mind, the problem is #4540.

annevk · 2017-03-13T12:03:21Z

See whatwg/fetch#204 (comment) for bugs I ended up filing and overall status of this feature.

wpt-pr-bot added the fetch label Mar 9, 2017

ferjm mentioned this pull request Mar 9, 2017

Allow for redirects after a CORS-preflight servo/servo#15889

Merged

4 tasks

annevk reviewed Mar 10, 2017

View reviewed changes

ferjm added 2 commits March 11, 2017 13:18

Allow for redirects after a CORS-preflight

e7075a9

Use async XHR and test that non-2XX preflight responses raise errors

292ebb8

ferjm force-pushed the cors-redirect-preflight branch from 7bb0bd3 to 292ebb8 Compare March 11, 2017 12:18

jgraham merged commit 146779a into web-platform-tests:master Mar 13, 2017

annevk mentioned this pull request Mar 13, 2017

Redirect on preflighted CORS requests generally impossible whatwg/fetch#204

Closed

ferjm deleted the cors-redirect-preflight branch March 13, 2017 12:35

Zirro mentioned this pull request May 14, 2017

Update web platform tests jsdom/jsdom#1845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for redirects after a CORS-preflight #5107

Allow for redirects after a CORS-preflight #5107

ferjm commented Mar 9, 2017

wpt-pr-bot commented Mar 9, 2017

w3c-bots commented Mar 9, 2017 •

edited

Loading

w3c-bots commented Mar 9, 2017 •

edited

Loading

annevk left a comment

annevk Mar 10, 2017

ferjm Mar 10, 2017

annevk Mar 10, 2017

annevk Mar 10, 2017

ferjm Mar 10, 2017

ferjm commented Mar 10, 2017

w3c-bots commented Mar 11, 2017 •

edited

Loading

annevk commented Mar 13, 2017

annevk commented Mar 13, 2017

annevk commented Mar 13, 2017

Allow for redirects after a CORS-preflight #5107

Allow for redirects after a CORS-preflight #5107

Conversation

ferjm commented Mar 9, 2017

wpt-pr-bot commented Mar 9, 2017

w3c-bots commented Mar 9, 2017 • edited Loading

Firefox (nightly channel)

All results

w3c-bots commented Mar 9, 2017 • edited Loading

Chrome (unstable channel)

All results

annevk left a comment

Choose a reason for hiding this comment

annevk Mar 10, 2017

Choose a reason for hiding this comment

ferjm Mar 10, 2017

Choose a reason for hiding this comment

annevk Mar 10, 2017

Choose a reason for hiding this comment

annevk Mar 10, 2017

Choose a reason for hiding this comment

ferjm Mar 10, 2017

Choose a reason for hiding this comment

ferjm commented Mar 10, 2017

w3c-bots commented Mar 11, 2017 • edited Loading

annevk commented Mar 13, 2017

annevk commented Mar 13, 2017

annevk commented Mar 13, 2017

w3c-bots commented Mar 9, 2017 •

edited

Loading

w3c-bots commented Mar 9, 2017 •

edited

Loading

w3c-bots commented Mar 11, 2017 •

edited

Loading