Allow redirects for requests that require preflight. (issue 2421093003 by jack@nottheoilrig.com)

[ja...@nottheoilrig.com]

Reviewers: yhirano
CL: https://codereview.chromium.org/2421093003/

Description:
Allow redirects for requests that require preflight.

BUG=580796

Affected files (+2, -14 lines):
M third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
M third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp


Index: third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
index b98a47d5d7b4b8a63946e9f1ee0907c199c44183..848fb5d92eb790f7aacecc0d128142e97567673a 100644
--- a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
@@ -161,7 +161,6 @@ DocumentThreadableLoader::DocumentThreadableLoader(
m_forceDoNotAllowStoredCredentials(false),
m_securityOrigin(m_resourceLoaderOptions.securityOrigin),
m_sameOriginRequest(false),
- m_crossOriginNonSimpleRequest(false),
m_isUsingDataConsumerHandle(false),
m_async(blockingBehavior == LoadAsynchronously),
m_requestContext(WebURLRequest::RequestContextUnspecified),
@@ -374,7 +373,6 @@ void DocumentThreadableLoader::makeCrossOriginAccessRequest(
}
loadRequest(crossOriginRequest, crossOriginOptions);
} else {
- m_crossOriginNonSimpleRequest = true;
// Do not set the Origin header for preflight requests.
updateRequestForAccessControl(crossOriginRequest, 0,
effectiveAllowCredentials());
@@ -564,16 +562,8 @@ bool DocumentThreadableLoader::redirectReceived(
bool allowRedirect = false;
String accessControlErrorDescription;

- if (m_crossOriginNonSimpleRequest) {
- // Non-simple cross origin requests (both preflight and actual one) are not
- // allowed to follow redirect.
- accessControlErrorDescription =
- "Redirect from '" + redirectResponse.url().getString() + "' to '" +
- request.url().getString() +
- "' has been blocked by CORS policy: Request requires preflight, which "
- "is disallowed to follow cross-origin redirect.";
- } else if (!CrossOriginAccessControl::isLegalRedirectLocation(
- request.url(), accessControlErrorDescription)) {
+ if (!CrossOriginAccessControl::isLegalRedirectLocation(
+ request.url(), accessControlErrorDescription)) {
accessControlErrorDescription =
"Redirect from '" + redirectResponse.url().getString() +
"' has been blocked by CORS policy: " + accessControlErrorDescription;
Index: third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
diff --git a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
index 06b19a11e48677a11f5e8b4f51323479101b944c..91193f737b4e294a5b069ce91a20dd8077052dd1 100644
--- a/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
+++ b/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
@@ -214,8 +214,6 @@ class CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader,
// True while the initial URL and all the URLs of the redirects this object
// has followed, if any, are same-origin to getSecurityOrigin().
bool m_sameOriginRequest;
- // Set to true if the current request is cross-origin and not simple.
- bool m_crossOriginNonSimpleRequest;

// Set to true when the response data is given to a data consumer handle.
bool m_isUsingDataConsumerHandle;

[tyos...@chromium.org]

Thanks. Please add layout tests to cover the change.

https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

On 2016/10/17 05:23:11, tyoshino wrote:
> Thanks. Please add layout tests to cover the change.

Done.

https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/

[yhi...@chromium.org]

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(right):

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#newcode195
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/Source/core/loader/DocumentThreadableLoader.cpp
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
(right):

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp#newcode564
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:564:
The preflight request (not a request w/preflight) shouldn't follow
redirect, right?

https://codereview.chromium.org/2421093003/

[yhi...@chromium.org]

Also, can you add your name and address to src/AUTHORS?


https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

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/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(right):

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#newcode195
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/Source/core/loader/DocumentThreadableLoader.cpp
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
(right):

https://codereview.chromium.org/2421093003/diff/20001/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp#newcode564
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.

https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

I fixed the failing test in the latest CL.

https://codereview.chromium.org/2421093003/

[yhi...@chromium.org]

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/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html
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/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html#newcode53
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?

https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

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/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/diff/140001/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html
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/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html#newcode53
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/

[yhi...@chromium.org]

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(left):

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#oldcode195
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/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#oldcode277
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/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode2
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/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(left):

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#oldcode195
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/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js#oldcode277
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/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/160001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode2
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).

https://codereview.chromium.org/2421093003/

[yhi...@chromium.org]

+horo@ for LayoutTests/http/tests/serviceworker/resources/redirect.php

https://codereview.chromium.org/2421093003/

[ho...@chromium.org]

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode5
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/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode8
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/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode10
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/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode5
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/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode8
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/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode10
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/

[ho...@chromium.org]

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode5
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/

[ja...@nottheoilrig.com]

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php
(right):

https://codereview.chromium.org/2421093003/diff/180001/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php#newcode5
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/redirect.php:5:
header("Location: $url");

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.

https://codereview.chromium.org/2421093003/

[ho...@chromium.org]

lgtm

[yhi...@chromium.org]

lgtm

Thank you for working on this issue!

https://codereview.chromium.org/2421093003/

[ja...@nottheoilrig.com]

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.

[ho...@chromium.org]

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.

https://codereview.chromium.org/2421093003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2421093003/200001


https://codereview.chromium.org/2421093003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #8 (id:200001)

https://codereview.chromium.org/2421093003/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 8 (id:??) landed as
https://crrev.com/eaeb7a5f8e9432594d8bcc09956c1f50e8f0ba66
Cr-Commit-Position: refs/heads/master@{#436374}

https://codereview.chromium.org/2421093003/