Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421093003:
Allow redirects for requests that require preflight. (Closed)
|
Created:
4 years, 2 months ago by Jack Bates Modified:
4 years ago CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow redirects for requests that require preflight.
BUG=580796
Committed: https://crrev.com/eaeb7a5f8e9432594d8bcc09956c1f50e8f0ba66
Cr-Commit-Position: refs/heads/master@{#436374}
Patch Set 1 #Patch Set 2 : Layout tests #
Total comments: 4
Patch Set 3 : Layout tests corrections and AUTHORS #Patch Set 4 : REDIRECT_URL #Patch Set 5 : Fix failing test #
Total comments: 2
Patch Set 6 : Rebase on custom headers CL #
Total comments: 6
Patch Set 7 : Custom method comment #
Total comments: 8
Patch Set 8 : Remove space #
Messages
Total messages: 57 (31 generated)
Description was changed from ========== Allow redirects for requests that require preflight. BUG=580796 ========== to ========== Allow redirects for requests that require preflight. BUG=580796 ==========
jack@nottheoilrig.com changed reviewers: + yhirano@chromium.org
Thanks. Please add layout tests to cover the change.
On 2016/10/17 05:23:11, tyoshino wrote: > Thanks. Please add layout tests to cover the change. Done.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js (right): https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:195: [OTHER_BASE_URL + I'm a bit confused: As you are making redirect + CORS w/preflight allowed, I didn't expect the url change from _REDIRECT_ to _BASE_. Can you explain? https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:564: The preflight request (not a request w/preflight) shouldn't follow redirect, right?
Also, can you add your name and address to src/AUTHORS?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2016/10/27 06:24:21, yhirano wrote: > Also, can you add your name and address to src/AUTHORS? Done.
https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js (right): https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:195: [OTHER_BASE_URL + On 2016/10/27 06:19:44, yhirano wrote: > I'm a bit confused: As you are making redirect + CORS w/preflight allowed, I > didn't expect the url change from _REDIRECT_ to _BASE_. Can you explain? My mistake. REDIRECT_URL was incorrectly redirecting the preflight request (before the request w/preflight). I thought I needed BASE_URL to properly handle the preflight, however I now see that REDIRECT_URL does the job. In the latest CL I updated redirect.php to avoid redirecting the preflight and reverted BASE_URL back to REDIRECT_URL. https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:564: On 2016/10/27 06:19:44, yhirano wrote: > The preflight request (not a request w/preflight) shouldn't follow redirect, > right? Right. I confirm that if you try to redirect the preflight itself, Chromium correctly raises a network error, both before and after this CL.
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I fixed the failing test in the latest CL.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the long delay. The code generally looks good. Which CL do you want to land first? We won't need the FIXME in redirect.js with https://codereview.chromium.org/2471533005/, right? https://codereview.chromium.org/2421093003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html (right): https://codereview.chromium.org/2421093003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html:53: access-control-allow-origin=http://127.0.0.1:8000", Hm, the document origin is http://127.0.0.1:8000 and the setting has been wrong for a long time, right? If so, should we replace all localhost:8000 in ACAO headers as well?
On 2016/11/16 08:20:28, yhirano wrote: > Sorry for the long delay. The code generally looks good. > > Which CL do you want to land first? We won't need the FIXME in redirect.js with > https://codereview.chromium.org/2471533005/, right? Thank you for reviewing this. I landed the custom headers CL and dropped the FIXME.
https://codereview.chromium.org/2421093003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html (right): https://codereview.chromium.org/2421093003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html:53: access-control-allow-origin=http://127.0.0.1:8000", On 2016/11/16 08:20:28, yhirano wrote: > Hm, the document origin is http://127.0.0.1:8000 and the setting has been wrong > for a long time, right? If so, should we replace all localhost:8000 in ACAO > headers as well? Done.
https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js (left): https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:195: // Custom method Should this line remain? https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:277: // Custom method ditto https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:2: if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { Sorry, I don't understand the intention of this change. Can you explain?
https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js (left): https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:195: // Custom method On 2016/11/22 04:05:23, yhirano OOO till 11-28 wrote: > Should this line remain? > Done. https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:277: // Custom method On 2016/11/22 04:05:23, yhirano OOO till 11-28 wrote: > ditto Done. https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:2: if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { On 2016/11/22 04:05:23, yhirano OOO till 11-28 wrote: > Sorry, I don't understand the intention of this change. Can you explain? Without this, it sends a redirect response to the preflight request. The tests that use REDIRECT_URL are for a redirect response to the actual request (which is sometimes preceded by a preflight).
yhirano@chromium.org changed reviewers: + horo@chromium.org
+horo@ for LayoutTests/http/tests/serviceworker/resources/redirect.php
https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:5: header("Location: $url"); I think it is better to set HTTP status line first for readability. if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { if (isset($_GET['Status'])) { header("HTTP/1.1 " . $_GET["Status"]); } else { header("HTTP/1.1 302"); } $url = $_GET['Redirect']; if ($url != "noLocation") { header("Location: $url"); } } https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:8: header ("HTTP/1.1 " . $_GET["Status"]); remove space. header("HTTP/1.1 " . $_GET["Status"]); https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:10: header ("HTTP/1.1 302"); remove space. header("HTTP/1.1 302");
https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:5: header("Location: $url"); On 2016/11/29 02:07:46, horo wrote: > I think it is better to set HTTP status line first for readability. > > if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { > if (isset($_GET['Status'])) { > header("HTTP/1.1 " . $_GET["Status"]); > } else { > header("HTTP/1.1 302"); > } > $url = $_GET['Redirect']; > if ($url != "noLocation") { > header("Location: $url"); > } > } That makes sense, but on my machine setting the status line first doesn't work if the status code is 200, for instance. If it is, then header("Location: $url") resets the response status to 302. So REDIRECT_URL + encodeURIComponent(BASE_URL) + '&Status=200' doesn't behave the way I'd expect. https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:8: header ("HTTP/1.1 " . $_GET["Status"]); On 2016/11/29 02:07:46, horo wrote: > remove space. > > header("HTTP/1.1 " . $_GET["Status"]); Done. https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:10: header ("HTTP/1.1 302"); On 2016/11/29 02:07:46, horo wrote: > remove space. > header("HTTP/1.1 302"); Done.
https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:5: header("Location: $url"); On 2016/11/29 16:29:34, Jack Bates wrote: > On 2016/11/29 02:07:46, horo wrote: > > I think it is better to set HTTP status line first for readability. > > > > if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { > > if (isset($_GET['Status'])) { > > header("HTTP/1.1 " . $_GET["Status"]); > > } else { > > header("HTTP/1.1 302"); > > } > > $url = $_GET['Redirect']; > > if ($url != "noLocation") { > > header("Location: $url"); > > } > > } > > That makes sense, but on my machine setting the status line first doesn't work > if the status code is 200, for instance. > If it is, then header("Location: $url") resets the response status to 302. > So REDIRECT_URL + encodeURIComponent(BASE_URL) + '&Status=200' doesn't behave > the way I'd expect. How about this? if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { $status = isset($_GET['Status']) ? intval($_GET["Status"]) : 302; $url = $_GET['Redirect']; if ($url != "noLocation") { header("Location: $url", true, $status); } else { header("HTTP/1.1 $status"); } }
https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php (right): https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:5: header("Location: $url"); On 2016/11/30 02:07:20, horo wrote: > On 2016/11/29 16:29:34, Jack Bates wrote: > > On 2016/11/29 02:07:46, horo wrote: > > > I think it is better to set HTTP status line first for readability. > > > > > > if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { > > > if (isset($_GET['Status'])) { > > > header("HTTP/1.1 " . $_GET["Status"]); > > > } else { > > > header("HTTP/1.1 302"); > > > } > > > $url = $_GET['Redirect']; > > > if ($url != "noLocation") { > > > header("Location: $url"); > > > } > > > } > > > > That makes sense, but on my machine setting the status line first doesn't work > > if the status code is 200, for instance. > > If it is, then header("Location: $url") resets the response status to 302. > > So REDIRECT_URL + encodeURIComponent(BASE_URL) + '&Status=200' doesn't behave > > the way I'd expect. > > How about this? > > if ($_SERVER['REQUEST_METHOD'] !== 'OPTIONS') { > $status = isset($_GET['Status']) ? intval($_GET["Status"]) : 302; > $url = $_GET['Redirect']; > if ($url != "noLocation") { > header("Location: $url", true, $status); > } else { > header("HTTP/1.1 $status"); > } > } That works too. I dunno, I think personally I prefer the solution in the current patch (setting the status and location separately, status last). I think it's more straightforward. But it's just a matter of taste (I think that functionally they are equivalent). I'll upload another patch with what you propose.
lgtm On 2016/11/28 08:29:02, yhirano wrote: > +horo@ for LayoutTests/http/tests/serviceworker/resources/redirect.php
lgtm Thank you for working on this issue!
Thank you for reviewing this.
I thought about it some more and there is a functional difference between patch
#8 and #9, it just makes no difference in this instance:
header("Location: $url", true, $status) doesn't set the response status if $url
is empty.
We don't ever do that, but it means REDIRECT_URL + '&Status=302' doesn't have
the expected status.
Still, in general I think we should prefer setting the status and location
separately, over the three argument form of header(), because it's more
consistent.
It's a small nit but if you approve, I want to go back to patch #8 instead of
#9.
On 2016/12/02 04:27:44, horo wrote:
> lgtm
>
> On 2016/11/28 08:29:02, yhirano wrote:
> > +horo@ for LayoutTests/http/tests/serviceworker/resources/redirect.php
On 2016/12/04 16:54:06, Jack Bates wrote:
> Thank you for reviewing this.
> I thought about it some more and there is a functional difference between
patch
> #8 and #9, it just makes no difference in this instance:
> header("Location: $url", true, $status) doesn't set the response status if
$url
> is empty.
> We don't ever do that, but it means REDIRECT_URL + '&Status=302' doesn't have
> the expected status.
> Still, in general I think we should prefer setting the status and location
> separately, over the three argument form of header(), because it's more
> consistent.
> It's a small nit but if you approve, I want to go back to patch #8 instead of
> #9.
I don't have so strong opinion.
#8 is also LGTM.
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by jack@nottheoilrig.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480961355745260,
"parent_rev": "6f2de0eb600be20909a4a3dcd28f039c58eff0a3", "commit_rev":
"f134539909d552d18f2f380fbab971f1bac54147"}
Message was sent while issue was closed.
Description was changed from ========== Allow redirects for requests that require preflight. BUG=580796 ========== to ========== Allow redirects for requests that require preflight. BUG=580796 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Allow redirects for requests that require preflight. BUG=580796 ========== to ========== Allow redirects for requests that require preflight. BUG=580796 Committed: https://crrev.com/eaeb7a5f8e9432594d8bcc09956c1f50e8f0ba66 Cr-Commit-Position: refs/heads/master@{#436374} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/eaeb7a5f8e9432594d8bcc09956c1f50e8f0ba66 Cr-Commit-Position: refs/heads/master@{#436374} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
