Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2635023003:
Fix a bug in origin header generation for CORS preflight in extensions (Closed)
|
Created:
3 years, 11 months ago by tyoshino (SeeGerritForStatus) Modified:
3 years, 11 months ago Reviewers:
yhirano CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix a bug in origin header generation for CORS preflight in extensions
In the refactoring CL https://codereview.chromium.org/2420603003 by me,
I forgot to set the origin header to the preflight request in
DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the
loading happens in an isolated world, the origin header will have an
incorrect value due to this bug.
We should add the referrer header to a preflight to conform to the
spec, but to limit the effect of this patch, it's not included. See the
added TODO.
This fix also cleans up the code around the main fix:
- createAccessControlPreflightRequests()'s SecurityOrigin argument is
unused. Remove it.
- makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin()
and setHTTPReferrer() (included in prepareCrossoriginRequest() call
next to them). Remove them.
- prepareCrossOriginRequest(crossOriginRequest) in the code path for
issuing a preflight is meaningless. It should have made on
m_actualRequest. That said, since loadActualRequest() has
prepareCrossOriginRequest() call, we don't even have to call
prepareCrossoriginRequest(m_actualRequest). The only difference
https://codereview.chromium.org/2420603003 introduced is that
m_actualRequest doesn't have the origin and referrer header until
loadActualRequest() is called. The only concern with this change is
the preflightResult->allowsCrossOriginHeaders() call in
DocumentThreadableLoader::handlePreflightResponse(). However, since
these headers are listed in the forbidden header names, presence of
these headers doesn't affect the return value of
allowsCrossOriginHeaders(). So, just remove the meaningless code.
BUG=680320
R=yhirano@chromium.org
Review-Url: https://codereview.chromium.org/2635023003
Cr-Commit-Position: refs/heads/master@{#444328}
Committed: https://chromium.googlesource.com/chromium/src/+/78d4c0d304e60af63f0a087b8b3de73c27d489b4
Patch Set 1 #Patch Set 2 : Fixed #Patch Set 3 : Add tests #Patch Set 4 : a #Patch Set 5 : a #
Total comments: 4
Patch Set 6 : Addressed #16 #Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Messages
Total messages: 29 (22 generated)
Description was changed from ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. BUG=680320 ========== to ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. BUG=680320 ==========
The CQ bit was checked by tyoshino@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...
Description was changed from ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. BUG=680320 ========== to ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. BUG=680320 ==========
tyoshino@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. BUG=680320 ========== to ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). BUG=680320 ==========
Description was changed from ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. As a bonus, this fix also fixes a spec violation that a referrer header is not attached to CORS preflight requests. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). BUG=680320 ========== to ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 ==========
The CQ bit was checked by tyoshino@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...
Description was changed from ========== Fix a bug in CORS preflight request creation In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 ========== to ========== Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html (right): https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html:13: xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/access-control-origin-header-in-isolated-world.php"); Please use double-quotation and quotation consistently. https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:396: // TODO: Call prepareCrossOriginRequest(preflightRequest) to also set the TODO(tyoshino)
https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html (right): https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html:13: xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/access-control-origin-header-in-isolated-world.php"); On 2017/01/18 05:28:03, yhirano wrote: > Please use double-quotation and quotation consistently. Done. https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/2635023003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:396: // TODO: Call prepareCrossOriginRequest(preflightRequest) to also set the On 2017/01/18 05:28:03, yhirano wrote: > TODO(tyoshino) Done.
Description was changed from ========== Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 ========== to ========== Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 R=yhirano@chromium.org ==========
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2635023003/#ps110001 (title: "Rebase")
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
Failed to apply patch for
third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp:31
error: third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp:
patch does not apply
Patch:
third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
Index: third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
diff --git
a/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
b/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
index
744bd0f4332ba71861e82b554ab1e27f6577c0d1..89376422e0665e2ad112ea8431a001b5402f0422
100644
--- a/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
+++ b/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
@@ -14,16 +14,7 @@ namespace blink {
namespace {
-class CreateAccessControlPreflightRequestTest : public ::testing::Test {
- protected:
- virtual void SetUp() {
- m_securityOrigin = SecurityOrigin::createFromString("http://example.com");
- }
-
- RefPtr<SecurityOrigin> m_securityOrigin;
-};
-
-TEST_F(CreateAccessControlPreflightRequestTest, LexicographicalOrder) {
+TEST(CreateAccessControlPreflightRequestTest, LexicographicalOrder) {
ResourceRequest request;
request.addHTTPHeaderField("Orange", "Orange");
request.addHTTPHeaderField("Apple", "Red");
@@ -31,55 +22,49 @@ TEST_F(CreateAccessControlPreflightRequestTest,
LexicographicalOrder) {
request.addHTTPHeaderField("Content-Type", "application/octet-stream");
request.addHTTPHeaderField("Strawberry", "Red");
- ResourceRequest preflight =
- createAccessControlPreflightRequest(request, m_securityOrigin.get());
+ ResourceRequest preflight = createAccessControlPreflightRequest(request);
EXPECT_EQ("apple,content-type,kiwifruit,orange,strawberry",
preflight.httpHeaderField("Access-Control-Request-Headers"));
}
-TEST_F(CreateAccessControlPreflightRequestTest, ExcludeSimpleHeaders) {
+TEST(CreateAccessControlPreflightRequestTest, ExcludeSimpleHeaders) {
ResourceRequest request;
request.addHTTPHeaderField("Accept", "everything");
request.addHTTPHeaderField("Accept-Language", "everything");
request.addHTTPHeaderField("Content-Language", "everything");
request.addHTTPHeaderField("Save-Data", "on");
- ResourceRequest preflight =
- createAccessControlPreflightRequest(request, m_securityOrigin.get());
+ ResourceRequest preflight = createAccessControlPreflightRequest(request);
EXPECT_EQ("", preflight.httpHeaderField("Access-Control-Request-Headers"));
}
-TEST_F(CreateAccessControlPreflightRequestTest,
- ExcludeSimpleContentTypeHeader) {
+TEST(CreateAccessControlPreflightRequestTest, ExcludeSimpleContentTypeHeader) {
ResourceRequest request;
request.addHTTPHeaderField("Content-Type", "text/plain");
- ResourceRequest preflight =
- createAccessControlPreflightRequest(request, m_securityOrigin.get());
+ ResourceRequest preflight = createAccessControlPreflightRequest(request);
EXPECT_EQ("", preflight.httpHeaderField("Access-Control-Request-Headers"));
}
-TEST_F(CreateAccessControlPreflightRequestTest, IncludeNonSimpleHeader) {
+TEST(CreateAccessControlPreflightRequestTest, IncludeNonSimpleHeader) {
ResourceRequest request;
request.addHTTPHeaderField("X-Custom-Header", "foobar");
- ResourceRequest preflight =
- createAccessControlPreflightRequest(request, m_securityOrigin.get());
+ ResourceRequest preflight = createAccessControlPreflightRequest(request);
EXPECT_EQ("x-custom-header",
preflight.httpHeaderField("Access-Control-Request-Headers"));
}
-TEST_F(CreateAccessControlPreflightRequestTest,
- IncludeNonSimpleContentTypeHeader) {
+TEST(CreateAccessControlPreflightRequestTest,
+ IncludeNonSimpleContentTypeHeader) {
ResourceRequest request;
request.addHTTPHeaderField("Content-Type", "application/octet-stream");
- ResourceRequest preflight =
- createAccessControlPreflightRequest(request, m_securityOrigin.get());
+ ResourceRequest preflight = createAccessControlPreflightRequest(request);
EXPECT_EQ("content-type",
preflight.httpHeaderField("Access-Control-Request-Headers"));
The CQ bit was checked by tyoshino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2635023003/#ps130001 (title: "Rebase")
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": 130001, "attempt_start_ts": 1484730458539640,
"parent_rev": "4f45c5de5f5444ffb697790c76387e66502d079f", "commit_rev":
"78d4c0d304e60af63f0a087b8b3de73c27d489b4"}
Message was sent while issue was closed.
Description was changed from ========== Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 R=yhirano@chromium.org ========== to ========== Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG=680320 R=yhirano@chromium.org Review-Url: https://codereview.chromium.org/2635023003 Cr-Commit-Position: refs/heads/master@{#444328} Committed: https://chromium.googlesource.com/chromium/src/+/78d4c0d304e60af63f0a087b8b3d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/78d4c0d304e60af63f0a087b8b3d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
