From 16586e1e301a920ef3d82a4cc74153f09b00c37e Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Fri, 22 Mar 2019 22:29:35 +0000 Subject: [PATCH] Use inner origin of blob: URLs to determine their trustworthiness. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL is removing special-casing of blob URLs from content::IsOriginSecure. After the CL, the inner origin of blob URLs will be extracted by network::IsUrlPotentiallyTrustworthy and passed to network::IsOriginPotentiallyTrustworthy (both of these network-layer functions follow https://www.w3.org/TR/powerful-features). The change above is desirable (blob URLs which were previously incorrectly always considered untrustworthy will be correctly judged based on their inner origin). OTOH, the change caused a test failure in PaymentRequestBlobUrlTest.ConnectionTerminated. As suggested by rouslan@ the CL just tweaks the test expectations of this test. I note that before this CL, PaymentRequestBlobUrlTest.ConnectionTerminated was exercising a scenario where the URL is insecure (and after this CL some blob URLs are considered secure based on their inner origin). Similar scenario is already tested by PaymentRequestDataUrlTest.SecurityError (pointed out by rouslan@ - thanks!) and therefore no test coverage is lost by tweaking the expectations of the PaymentRequestBlobUrlTest. Bug: 937451 Change-Id: I9cec22a13fe821eb39ea789e1128bc592267d986 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506941 Reviewed-by: Rouslan Solomakhin Reviewed-by: Alex Moshchuk Auto-Submit: Łukasz Anforowicz Commit-Queue: Łukasz Anforowicz Cr-Commit-Position: refs/heads/master@{#643579} --- .../payments/payment_request_blob_url_browsertest.cc | 4 ++-- content/common/origin_util.cc | 7 ------- content/common/origin_util_unittest.cc | 9 +++++---- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/chrome/browser/ui/views/payments/payment_request_blob_url_browsertest.cc b/chrome/browser/ui/views/payments/payment_request_blob_url_browsertest.cc index 2d23b9a87cff22..0fd8cd53a750b5 100644 --- a/chrome/browser/ui/views/payments/payment_request_blob_url_browsertest.cc +++ b/chrome/browser/ui/views/payments/payment_request_blob_url_browsertest.cc @@ -16,12 +16,12 @@ class PaymentRequestBlobUrlTest : public PaymentRequestBrowserTestBase { IN_PROC_BROWSER_TEST_F(PaymentRequestBlobUrlTest, ConnectionTerminated) { NavigateTo("/payment_request_blob_url_test.html"); - ResetEventWaiter(DialogEvent::DIALOG_CLOSED); + ResetEventWaiter(DialogEvent::NOT_SUPPORTED_ERROR); ASSERT_TRUE(content::ExecuteScript( GetActiveWebContents(), "(function() { document.getElementById('buy').click(); })();")); WaitForObservedEvent(); - ExpectBodyContains({"Rejected: UnknownError: Request failed"}); + ExpectBodyContains({"Rejected: NotSupportedError"}); } } // namespace payments diff --git a/content/common/origin_util.cc b/content/common/origin_util.cc index 5869274d6f2cbc..1261b8e701f4aa 100644 --- a/content/common/origin_util.cc +++ b/content/common/origin_util.cc @@ -26,13 +26,6 @@ bool IsOriginSecure(const GURL& url) { if (url.SchemeIs(url::kDataScheme)) return true; - // TODO(lukasza): trustworthiness of blob: URLs should depend on their inner - // origin (just as it does for filesystem: URLs). Changing this behavior of - // content::IsOriginSecure breaks some tests, so fixing this is postponed to a - // follow-up CL. WIP CL @ https://crrev.com/c/1506941. - if (url.SchemeIs(url::kBlobScheme)) - return false; - return network::IsUrlPotentiallyTrustworthy(url); } diff --git a/content/common/origin_util_unittest.cc b/content/common/origin_util_unittest.cc index 365b937b7435b6..69c522e203602b 100644 --- a/content/common/origin_util_unittest.cc +++ b/content/common/origin_util_unittest.cc @@ -60,11 +60,12 @@ TEST(OriginUtilTest, IsOriginSecure) { // that https + data = mixed content). EXPECT_TRUE(IsOriginSecure(GURL("data:test/plain;blah"))); - // TODO(lukasza): trustworthiness of blob: URLs should depend on their inner - // origin (just as it does for filesystem: URLs). Changing this behavior of - // content::IsOriginSecure breaks some tests, so fixing this is postponed to a - // follow-up CL. EXPECT_FALSE( + IsOriginSecure(GURL("blob:http://www.example.com/guid-goes-here"))); + EXPECT_FALSE( + IsOriginSecure(GURL("blob:ftp://www.example.com/guid-goes-here"))); + EXPECT_TRUE(IsOriginSecure(GURL("blob:ftp://127.0.0.1/guid-goes-here"))); + EXPECT_TRUE( IsOriginSecure(GURL("blob:https://www.example.com/guid-goes-here"))); }