Skip to content

Commit

Permalink
[MIX-DL] Fix blob: URL handling and clarify console messages
Browse files Browse the repository at this point in the history
This CL fixes a bug wherein the final URL in a redirect chain was
considered as part of the middle of the redirect chain. This resulted in
two seemingly-unrelated bugs:
 * The console message always said that blocked downloads were
   "redirected through" an insecure URL, even if there were no
   redirects/the redirects were all secure.
 * file:// and blob:// URLs were considered insecure, even when you got
   there through secure means, and even though we had tried to
   special-case them.

The only non-test change in this CL is to exclude the last URL in the
redirect chain when considering redirects, since the last URL in the
chain is always the final URL.

This CL also adds a test for the blob case, and fixes the test harness
so that the redirect chain behavior is consistent with Chrome (it
previously was not including the final URL in the chain).

(cherry picked from commit 426ce50)

Bug: 1197573
Change-Id: I1e624a31b47fd3f90aec5bb3032f74d19d2b47e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2818720
Commit-Queue: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#871128}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824021
Auto-Submit: Joe DeBlasio <jdeblasio@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/branch-heads/4472@{#48}
Cr-Branched-From: 3d60439-refs/heads/master@{#870763}
  • Loading branch information
Joe DeBlasio authored and Chromium LUCI CQ committed Apr 14, 2021
1 parent b44b5bb commit f5d8a68
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,8 @@ ChromeDownloadManagerDelegateTest::PrepareDownloadItemForMixedContent(
std::vector<GURL> url_chain;
if (redirect_url.has_value())
url_chain.push_back(redirect_url.value());
// The redirect chain always contains the final destination at the end.
url_chain.push_back(download_url);
std::unique_ptr<download::MockDownloadItem> download_item =
CreateActiveDownloadItem(0);
ON_CALL(*download_item, GetURL())
Expand Down Expand Up @@ -507,7 +509,7 @@ void ExpectExtensionOnlyIn(const InsecureDownloadExtensions& ext,
}

// Determine download target for |download_item| after enabling active content
// download blocking with the |parameers| enabled. Verify |extension|,
// download blocking with the |parameters| enabled. Verify |extension|,
// |interrupt_reason| and |mixed_content_status|. Used by
// BlockedAsActiveContent_ tests.
void ChromeDownloadManagerDelegateTest::VerifyMixedContentExtensionOverride(
Expand Down Expand Up @@ -931,6 +933,42 @@ TEST_F(ChromeDownloadManagerDelegateTest,
download::DownloadItem::MixedContentStatus::SAFE);
}

// Verify that downloads ending in a blob URL are considered secure.
TEST_F(ChromeDownloadManagerDelegateTest,
BlockedAsActiveContent_BlobConsideredSecure) {
// Verifies blob URLs are not blocked for active content blocking.
const GURL kRedirectUrl("https://example.org/");
const GURL kFinalUrl("blob:null/xyz.foo");
const auto kSecureOrigin = Origin::Create(GURL("https://example.org"));

DetermineDownloadTargetResult result;
base::test::ScopedFeatureList feature_list;
base::HistogramTester histograms;

std::unique_ptr<download::MockDownloadItem> download_item =
PrepareDownloadItemForMixedContent(kFinalUrl, kSecureOrigin,
kRedirectUrl);

feature_list.InitAndEnableFeature(features::kTreatUnsafeDownloadsAsActive);

#if BUILDFLAG(ENABLE_PLUGINS)
// DownloadTargetDeterminer looks for plugin handlers if there's an
// extension.
content::PluginService::GetInstance()->Init();
#endif

DetermineDownloadTarget(download_item.get(), &result);
EXPECT_EQ(download::DOWNLOAD_INTERRUPT_REASON_NONE, result.interrupt_reason);
EXPECT_EQ(download::DownloadItem::MixedContentStatus::SAFE,
result.mixed_content_status);
histograms.ExpectUniqueSample(
kInsecureDownloadHistogramName,
InsecureDownloadSecurityStatus::kInitiatorSecureFileSecure, 1);
ExpectExtensionOnlyIn(InsecureDownloadExtensions::kUnknown,
kInsecureDownloadExtensionInitiatorSecure,
kInsecureDownloadHistogramTargetSecure, histograms);
}

TEST_F(ChromeDownloadManagerDelegateTest, BlockedAsActiveContent_SilentBlock) {
// Verifies that active mixed content download silent blocking works by
// default, and that extensions can be overridden by feature parameter.
Expand Down
15 changes: 11 additions & 4 deletions chrome/browser/download/mixed_content_download_blocking.cc
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,17 @@ struct MixedContentDownloadData {

// Evaluate download security.
is_redirect_chain_secure_ = true;
for (const auto& url : item->GetUrlChain()) {
if (!network::IsUrlPotentiallyTrustworthy(url)) {
is_redirect_chain_secure_ = false;
break;
// Skip over the final URL so that we can investigate it separately below.
// The redirect chain always contains the final URL, so this is always safe
// in Chrome, but some tests don't plan for it, so we check here.
const auto& chain = item->GetUrlChain();
if (chain.size() > 1) {
for (unsigned i = 0; i < chain.size() - 1; ++i) {
const GURL& url = chain[i];
if (!network::IsUrlPotentiallyTrustworthy(url)) {
is_redirect_chain_secure_ = false;
break;
}
}
}
const GURL& dl_url = item->GetURL();
Expand Down

0 comments on commit f5d8a68

Please sign in to comment.