Commit 969984d9 authored by arthursonzogni's avatar arthursonzogni Committed by Commit Bot

[COOP] reporting: factorize the "sanitized URL" logic.

Sensitive information are removed from URLs sent with COOP. This was
duplicated several time and way more are coming to support
access-reporting.

This patch:
1. Factorize the logic inside SanitizedURL(url)
2. Clear the #fragment part of the URL. Developers need to know the
   document's URL, not which part of it the user scrolled into.
3. Clear non HTTP/HTTPS urls.
4. As in the ReportingServiceImpl, use GetAsReferrer() instead. This is
   slightly more optimized.
5. Remove the useless sanitization in QueueNavigationReport.

Bug: 1090273
Change-Id: I4b2a23dcb3ed218c58d8a940bed1a34fe42a7fc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401025Reviewed-by: default avatarCamille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806703}
parent 87a03ba4
...@@ -113,6 +113,16 @@ FrameTreeNode* TopLevelOpener(FrameTreeNode* frame) { ...@@ -113,6 +113,16 @@ FrameTreeNode* TopLevelOpener(FrameTreeNode* frame) {
return opener ? opener->frame_tree()->root() : nullptr; return opener ? opener->frame_tree()->root() : nullptr;
} }
// Remove sensitive data from URL used in reports.
std::string SanitizedURL(const GURL& url) {
// Strip username, password and ref fragment from the URL.
// Keep only the valid http/https ones.
//
// Note: This is the exact same operation used in
// ReportingServiceImpl::QueueReport() for the |url|.
return url.GetAsReferrer().spec();
}
} // namespace } // namespace
CrossOriginOpenerPolicyReporter::CrossOriginOpenerPolicyReporter( CrossOriginOpenerPolicyReporter::CrossOriginOpenerPolicyReporter(
...@@ -137,21 +147,12 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationToCOOPReport( ...@@ -137,21 +147,12 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationToCOOPReport(
if (!endpoint) if (!endpoint)
return; return;
url::Replacements<char> replacements;
replacements.ClearUsername();
replacements.ClearPassword();
std::string sanitized_previous_url;
if (same_origin_with_previous) {
sanitized_previous_url =
previous_url.ReplaceComponents(replacements).spec();
}
std::string sanitized_referrer_url =
referrer_url.ReplaceComponents(replacements).spec();
base::DictionaryValue body; base::DictionaryValue body;
body.SetString(kDisposition, body.SetString(kDisposition,
is_report_only ? kDispositionReporting : kDispositionEnforce); is_report_only ? kDispositionReporting : kDispositionEnforce);
body.SetString(kPreviousURL, sanitized_previous_url); body.SetString(kPreviousURL,
body.SetString(kReferrer, sanitized_referrer_url); same_origin_with_previous ? SanitizedURL(previous_url) : "");
body.SetString(kReferrer, SanitizedURL(referrer_url));
body.SetString(kViolationType, kTypeToResponse); body.SetString(kViolationType, kTypeToResponse);
QueueNavigationReport(std::move(body), *endpoint, is_report_only); QueueNavigationReport(std::move(body), *endpoint, is_report_only);
} }
...@@ -167,13 +168,9 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationAwayFromCOOPReport( ...@@ -167,13 +168,9 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationAwayFromCOOPReport(
if (!endpoint) if (!endpoint)
return; return;
url::Replacements<char> replacements;
replacements.ClearUsername();
replacements.ClearPassword();
std::string sanitized_next_url; std::string sanitized_next_url;
if (is_current_source || same_origin_with_next) { if (is_current_source || same_origin_with_next)
sanitized_next_url = next_url.ReplaceComponents(replacements).spec(); sanitized_next_url = SanitizedURL(next_url);
}
base::DictionaryValue body; base::DictionaryValue body;
body.SetString(kNextURL, sanitized_next_url); body.SetString(kNextURL, sanitized_next_url);
body.SetString(kViolationType, kTypeFromResponse); body.SetString(kViolationType, kTypeFromResponse);
...@@ -331,12 +328,8 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationReport( ...@@ -331,12 +328,8 @@ void CrossOriginOpenerPolicyReporter::QueueNavigationReport(
body.SetString( body.SetString(
kEffectivePolicy, kEffectivePolicy,
ToString(is_report_only ? coop_.report_only_value : coop_.value)); ToString(is_report_only ? coop_.report_only_value : coop_.value));
url::Replacements<char> replacements;
replacements.ClearUsername();
replacements.ClearPassword();
GURL sanitized_context_url = context_url_.ReplaceComponents(replacements);
storage_partition_->GetNetworkContext()->QueueReport( storage_partition_->GetNetworkContext()->QueueReport(
"coop", endpoint, sanitized_context_url, /*user_agent=*/base::nullopt, "coop", endpoint, context_url_, /*user_agent=*/base::nullopt,
std::move(body)); std::move(body));
} }
......
...@@ -80,7 +80,8 @@ class CrossOriginOpenerPolicyReporterTest : public testing::Test { ...@@ -80,7 +80,8 @@ class CrossOriginOpenerPolicyReporterTest : public testing::Test {
TEST_F(CrossOriginOpenerPolicyReporterTest, Basic) { TEST_F(CrossOriginOpenerPolicyReporterTest, Basic) {
auto reporter = GetReporter(); auto reporter = GetReporter();
std::string url1 = "https://www1.example.com/y#foo?bar=baz"; std::string url1 = "https://www1.example.com/y?bar=baz#foo";
std::string url1_report = "https://www1.example.com/y?bar=baz";
std::string url2 = "https://www1.example.com/"; std::string url2 = "https://www1.example.com/";
std::string url3 = "http://www2.example.com:41/z"; std::string url3 = "http://www2.example.com:41/z";
...@@ -94,7 +95,7 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, Basic) { ...@@ -94,7 +95,7 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, Basic) {
EXPECT_EQ(r1.type, "coop"); EXPECT_EQ(r1.type, "coop");
EXPECT_EQ(r1.url, context_url()); EXPECT_EQ(r1.url, context_url());
EXPECT_EQ(r1.body.FindKey("disposition")->GetString(), "enforce"); EXPECT_EQ(r1.body.FindKey("disposition")->GetString(), "enforce");
EXPECT_EQ(r1.body.FindKey("previousResponseURL")->GetString(), url1); EXPECT_EQ(r1.body.FindKey("previousResponseURL")->GetString(), url1_report);
EXPECT_EQ(r1.body.FindKey("referrer")->GetString(), url2); EXPECT_EQ(r1.body.FindKey("referrer")->GetString(), url2);
EXPECT_EQ(r1.body.FindKey("type")->GetString(), "navigation-to-response"); EXPECT_EQ(r1.body.FindKey("type")->GetString(), "navigation-to-response");
EXPECT_EQ(r1.body.FindKey("effectivePolicy")->GetString(), EXPECT_EQ(r1.body.FindKey("effectivePolicy")->GetString(),
...@@ -135,7 +136,8 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, UserAndPassSanitization) { ...@@ -135,7 +136,8 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, UserAndPassSanitization) {
TEST_F(CrossOriginOpenerPolicyReporterTest, Clone) { TEST_F(CrossOriginOpenerPolicyReporterTest, Clone) {
auto reporter = GetReporter(); auto reporter = GetReporter();
std::string url1 = "https://www1.example.com/y#foo?bar=baz"; std::string url1 = "https://www1.example.com/y?bar=baz#foo";
std::string url1_sanitized = "https://www1.example.com/y?bar=baz";
mojo::Remote<network::mojom::CrossOriginOpenerPolicyReporter> remote; mojo::Remote<network::mojom::CrossOriginOpenerPolicyReporter> remote;
reporter->Clone(remote.BindNewPipeAndPassReceiver()); reporter->Clone(remote.BindNewPipeAndPassReceiver());
...@@ -150,8 +152,9 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, Clone) { ...@@ -150,8 +152,9 @@ TEST_F(CrossOriginOpenerPolicyReporterTest, Clone) {
EXPECT_EQ(r1.type, "coop"); EXPECT_EQ(r1.type, "coop");
EXPECT_EQ(r1.url, context_url()); EXPECT_EQ(r1.url, context_url());
EXPECT_EQ(r1.body.FindKey("disposition")->GetString(), "enforce"); EXPECT_EQ(r1.body.FindKey("disposition")->GetString(), "enforce");
EXPECT_EQ(r1.body.FindKey("previousResponseURL")->GetString(), url1); EXPECT_EQ(r1.body.FindKey("previousResponseURL")->GetString(),
EXPECT_EQ(r1.body.FindKey("referrer")->GetString(), url1); url1_sanitized);
EXPECT_EQ(r1.body.FindKey("referrer")->GetString(), url1_sanitized);
EXPECT_EQ(r1.body.FindKey("type")->GetString(), "navigation-to-response"); EXPECT_EQ(r1.body.FindKey("type")->GetString(), "navigation-to-response");
EXPECT_EQ(r1.body.FindKey("effectivePolicy")->GetString(), EXPECT_EQ(r1.body.FindKey("effectivePolicy")->GetString(),
"same-origin-plus-coep"); "same-origin-plus-coep");
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment