-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow for redirects after a CORS-preflight #15889
Conversation
Heads up! This PR modifies the following files:
|
@wafflespeanut could you look at the changes for #14787, please? I only took what was done in #14811 and #15547 (e0b2146) and made the required change (46ae9d0) after rebasing. @avadacatavra could you review the changes for #14519, please? The fixed is in 001448d. Thank you! |
The changes on the cors/redirect-preflight.htm WPTs are proposed here. |
@KiChjang ah, good to know. Thanks! |
☔ The latest upstream changes (presumably #15903) made this pull request unmergeable. Please resolve the merge conflicts. |
001448d
to
ec314a7
Compare
@wafflespeanut already landed the fix for #14787 in #15903 |
@bors-servo r+ |
@avadacatavra: 🔑 Insufficient privileges: Not in reviewers |
@bors-servo r=avadacatavra delegate=avadacatavra |
✌️ @avadacatavra can now approve this pull request |
📌 Commit ec314a7 has been approved by |
Allow for redirects after a CORS-preflight ~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix ~#14787 and~ #14519 - [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](#14519 (comment)). <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15889) <!-- Reviewable:end -->
💔 Test failed - mac-rel-wpt1 |
ec314a7
to
469eb19
Compare
@bors-servo r+ |
📌 Commit 469eb19 has been approved by |
Allow for redirects after a CORS-preflight ~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix ~#14787 and~ #14519 - [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](#14519 (comment)). <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15889) <!-- Reviewable:end -->
💔 Test failed - mac-dev-unit |
@bors-servo retry
|
⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only mac-dev-unit... |
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
FWIW, I don't really understand how this changed results in cors/redirect-userinfo.htm or eventsource/eventsource-cross-origin.htm. I did find some issues with the former that I'm cleaning up in web-platform-tests/wpt#5121. |
Apart from applying the spec change to allow redirects after preflights, this patch also fixed the same origin checks for steps 7 and 10 of HTTP-redirect fetch. I believe for both tests, we were previously failing at step 7 of HTTP-redirect fetch because of the incorrect same origin check. |
Add Diane and Canaltinova to reviewers @avadacatavra is a Mozilla employee (she's reviewed servo/servo#15889 and servo/servo#15783, and possibly more in the future). And, @canaltinova has reviewed various PRs, have filed a number of issues for newcomers, and is a frequent contributor to servo. I think we can move both of them to reviewers list. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/616) <!-- Reviewable:end -->
Add Diane and Canaltinova to reviewers @avadacatavra is a Mozilla employee (she's reviewed servo/servo#15889 and servo/servo#15783, and possibly more in the future). And, @canaltinova has reviewed various PRs, have filed a number of issues for newcomers, and is a frequent contributor to servo. I think we can move both of them to reviewers list. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/616) <!-- Reviewable:end -->
Continue the work done in #14811 and #15547. And applies the Fetch spec change about allowing redirects after CORS preflights../mach build -d
does not report any errors./mach test-tidy
does not report any errorsSet origin header in http_network_or_cache_fetch #14787 andAllow for redirects after a CORS-preflight #14519This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)