Skip to content

Commit

Permalink
[Topics] For fetch and <iframe> request, still send empty topics when…
Browse files Browse the repository at this point in the history 
… disallowed by user settings

This aligns with the handling for document.browsingTopics(), and will
reveal less information.

PR: patcg-individual-drafts/topics#276

Bug: 1502719
Change-Id: I2fba92cbf7af21613d2d0756c8318c4ea1ee4887
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5034182
Reviewed-by: Arthur Sonzogni <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Reviewed-by: Josh Karlin <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1238153}
  • Loading branch information
@yaoxiachromium
yaoxiachromium authored and Chromium LUCI CQ committed Dec 15, 2023
1 parent 213eb0c commit 42e4a22
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 42 deletions.
55 changes: 48 additions & 7 deletions chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1329,9 +1329,9 @@ IN_PROC_BROWSER_TEST_F(
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

// Expect no topics header as the request was not eligible for topics due to
// user settings.
EXPECT_FALSE(topics_header_value);
// When the request is ineligible for topics due to user settings, an empty
// list of topics will be sent in the header.
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

// No observation should have been recorded in addition to the pre-existing
// one even though the response had the `Observe-Browsing-Topics: ?1` header,
Expand Down Expand Up @@ -1887,6 +1887,47 @@ IN_PROC_BROWSER_TEST_F(BrowsingTopicsBrowserTest,
EXPECT_EQ(api_usage_contexts.size(), 1u);
}

IN_PROC_BROWSER_TEST_F(
BrowsingTopicsBrowserTest,
CrossOriginDynamicIframe_TopicsNotEligibleDueToUserSettings_HasObserveResponse) {
GURL main_frame_url =
https_server_.GetURL("b.test", "/browsing_topics/empty_page.html");

ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), main_frame_url));

base::StringPairs replacement;
replacement.emplace_back(std::make_pair("{{STATUS}}", "200 OK"));
replacement.emplace_back(std::make_pair("{{OBSERVE_BROWSING_TOPICS_HEADER}}",
"Observe-Browsing-Topics: ?1"));
replacement.emplace_back(std::make_pair("{{REDIRECT_HEADER}}", ""));

GURL subframe_url = https_server_.GetURL(
"a.test", net::test_server::GetFilePathWithReplacements(
"/browsing_topics/"
"page_with_custom_topics_header.html",
replacement));

CookieSettingsFactory::GetForProfile(browser()->profile())
->SetCookieSetting(subframe_url, CONTENT_SETTING_BLOCK);

CreateIframe(subframe_url, /*browsing_topics_attribute=*/true);

absl::optional<std::string> topics_header_value =
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

// When the request is ineligible for topics due to user settings, an empty
// list of topics will be sent in the header.
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

// No observation should have been recorded in addition to the pre-existing
// one even though the response had the `Observe-Browsing-Topics: ?1` header,
// as the request was not eligible for topics.
std::vector<ApiUsageContext> api_usage_contexts =
content::GetBrowsingTopicsApiUsage(browsing_topics_site_data_manager());
EXPECT_EQ(api_usage_contexts.size(), 1u);
}

// Only allow topics from origin c.test, and test <iframe browsingtopics>
// requests to b.test and c.test to verify that only c.test gets the header.
IN_PROC_BROWSER_TEST_F(
Expand Down Expand Up @@ -2173,7 +2214,7 @@ IN_PROC_BROWSER_TEST_F(AttestationBrowsingTopicsBrowserTest,
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

EXPECT_FALSE(topics_header_value);
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

ASSERT_TRUE(console_observer.Wait());
EXPECT_FALSE(console_observer.messages().empty());
Expand Down Expand Up @@ -2208,7 +2249,7 @@ IN_PROC_BROWSER_TEST_F(
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

EXPECT_FALSE(topics_header_value);
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

ASSERT_TRUE(console_observer.Wait());
EXPECT_FALSE(console_observer.messages().empty());
Expand Down Expand Up @@ -2314,7 +2355,7 @@ IN_PROC_BROWSER_TEST_F(AttestationBrowsingTopicsBrowserTest,
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

EXPECT_FALSE(topics_header_value);
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

// Because a.test is not attested for Topics, we should not have any new
// observations of API usage.
Expand Down Expand Up @@ -2366,7 +2407,7 @@ IN_PROC_BROWSER_TEST_F(
GetTopicsHeaderForRequestPath(
"/browsing_topics/page_with_custom_topics_header.html");

EXPECT_FALSE(topics_header_value);
EXPECT_EQ(topics_header_value, kExpectedHeaderValueForEmptyTopics);

// Because a.test is not attested for Topics, we should not have any new
// observations of API usage.
Expand Down
13 changes: 8 additions & 5 deletions content/browser/browsing_topics/browsing_topics_url_loader_interceptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,15 @@ void BrowsingTopicsURLLoaderInterceptor::PopulateRequestOrRedirectHeaders(
/*get_topics=*/true,
/*observe=*/false, topics);

const int num_versions_in_epochs =
topics_eligible_
? GetContentClient()->browser()->NumVersionsInTopicsEpochs(
request_initiator_frame->GetMainFrame())
: 0;
headers.SetHeader(kBrowsingTopicsRequestHeaderKey,
DeriveTopicsHeaderValue(topics, num_versions_in_epochs));

if (topics_eligible_) {
int num_versions_in_epochs =
GetContentClient()->browser()->NumVersionsInTopicsEpochs(
request_initiator_frame->GetMainFrame());
headers.SetHeader(kBrowsingTopicsRequestHeaderKey,
DeriveTopicsHeaderValue(topics, num_versions_in_epochs));
RecordFetchRequestResultUma(
BrowsingTopicsFetchRequestOrRedirectResult::kSuccess, is_redirect);
} else {
Expand Down
6 changes: 4 additions & 2 deletions content/browser/browsing_topics/browsing_topics_url_loader_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -712,11 +712,13 @@ TEST_F(BrowsingTopicsURLLoaderTest,
network::TestURLLoaderFactory::PendingRequest* pending_request =
&proxied_url_loader_factory.pending_requests()->back();

// The request won't be eligible for topics due to content client settings.
// When the request is ineligible for topics due to user settings, an empty
// list of topics will be sent in the header.
std::string topics_header_value;
bool has_topics_header = pending_request->request.headers.GetHeader(
"Sec-Browsing-Topics", &topics_header_value);
EXPECT_FALSE(has_topics_header);
EXPECT_TRUE(has_topics_header);
EXPECT_EQ(topics_header_value, kExpectedHeaderForEmptyTopics);

EXPECT_EQ(browser_client().handle_topics_web_api_count(), 1u);

Expand Down
62 changes: 34 additions & 28 deletions content/browser/renderer_host/navigation_request.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -864,15 +864,20 @@ void PersistOriginTrialsFromHeaders(
tokens, base::Time::Now());
}

struct TopicsHeaderValueResult {
bool topics_eligible = false;
std::optional<std::string> header_value;
};

// Returns the topics header for a navigation request. Returns absl::nullopt if
// the request isn't eligible for topics. This should align with the handling in
// `GetTopicsHeaderValueForSubresourceRequest()`.
absl::optional<std::string> GetTopicsHeaderValueForNavigationRequest(
TopicsHeaderValueResult GetTopicsHeaderValueForNavigationRequest(
FrameTreeNode* frame_tree_node,
const GURL& url) {
// Skip if the <iframe> does not have the "browsingtopics" opt-in attribute.
if (!frame_tree_node->browsing_topics()) {
return absl::nullopt;
return TopicsHeaderValueResult{};
}

RenderFrameHostImpl* rfh = frame_tree_node->current_frame_host();
Expand All @@ -882,33 +887,33 @@ absl::optional<std::string> GetTopicsHeaderValueForNavigationRequest(
// RenderFrameHostImpl::DidChangeIframeAttributes, and should be a DCHECK
// here.
if (rfh->is_main_frame()) {
return absl::nullopt;
return {};
}

// Skip fenced frames.
if (rfh->IsNestedWithinFencedFrame()) {
return absl::nullopt;
return {};
}

// Skip inactive pages (e.g. prerendered pages).
if (!rfh->GetPage().IsPrimary()) {
return absl::nullopt;
return {};
}

// TODO(crbug.com/1244137): IsPrimary() doesn't actually detect portals yet.
// Remove this when it does.
if (!static_cast<RenderFrameHostImpl*>(rfh->GetMainFrame())
->IsOutermostMainFrame()) {
return absl::nullopt;
return {};
}

url::Origin origin = url::Origin::Create(url);
if (origin.opaque()) {
return absl::nullopt;
return {};
}

if (!network::IsOriginPotentiallyTrustworthy(origin)) {
return absl::nullopt;
return {};
}

const blink::PermissionsPolicy* parent_policy =
Expand All @@ -922,7 +927,7 @@ absl::optional<std::string> GetTopicsHeaderValueForNavigationRequest(
blink::mojom::PermissionsPolicyFeature::
kBrowsingTopicsBackwardCompatible,
origin)) {
return absl::nullopt;
return {};
}

std::vector<blink::mojom::EpochTopicPtr> topics;
Expand All @@ -932,15 +937,15 @@ absl::optional<std::string> GetTopicsHeaderValueForNavigationRequest(
/*get_topics=*/true,
/*observe=*/false, topics);

if (!topics_eligible) {
return absl::nullopt;
}

int num_versions_in_epochs =
GetContentClient()->browser()->NumVersionsInTopicsEpochs(
rfh->GetMainFrame());
topics_eligible
? GetContentClient()->browser()->NumVersionsInTopicsEpochs(
rfh->GetMainFrame())
: 0;

return DeriveTopicsHeaderValue(topics, num_versions_in_epochs);
return {
.topics_eligible = topics_eligible,
.header_value = DeriveTopicsHeaderValue(topics, num_versions_in_epochs)};
}

ukm::SourceId GetPageUkmSourceId(FrameTreeNode* frame_tree_node) {
Expand Down Expand Up @@ -1872,14 +1877,15 @@ NavigationRequest::NavigationRequest(
&commit_params_->post_content_type);
}

absl::optional<std::string> topics_header_value =
TopicsHeaderValueResult topics_header_value_result =
GetTopicsHeaderValueForNavigationRequest(frame_tree_node,
common_params_->url);

topics_eligible_ = topics_header_value.has_value();
topics_eligible_ = topics_header_value_result.topics_eligible;

if (topics_eligible_) {
headers.SetHeader(kBrowsingTopicsRequestHeaderKey, *topics_header_value);
if (topics_header_value_result.header_value) {
headers.SetHeader(kBrowsingTopicsRequestHeaderKey,
*topics_header_value_result.header_value);
}

if (has_ad_auction_headers_attribute_ &&
Expand Down Expand Up @@ -5202,10 +5208,6 @@ void NavigationRequest::OnRedirectChecksComplete(
if (topics_eligible_) {
topics_eligible_ = false;

// Removes the topics header from the request that was passed on from the
// previous one.
removed_headers.push_back(kBrowsingTopicsRequestHeaderKey);

// At this point we may not have a valid `GetRenderFrameHost()` if the
// navigation is during a cross-site redirect. Thus, pass in the current/old
// RenderFrameHost here. This is fine, because it should still give us the
Expand All @@ -5218,15 +5220,19 @@ void NavigationRequest::OnRedirectChecksComplete(
browsing_topics::ApiCallerSource::kIframeAttribute);
}

absl::optional<std::string> topics_header_value =
// Removes the topics header. This will effectively be a no-op if the topics
// header wasn't sent for the previous request.
removed_headers.push_back(kBrowsingTopicsRequestHeaderKey);

TopicsHeaderValueResult topics_header_value_result =
GetTopicsHeaderValueForNavigationRequest(frame_tree_node_,
common_params_->url);

topics_eligible_ = topics_header_value.has_value();
topics_eligible_ = topics_header_value_result.topics_eligible;

if (topics_eligible_) {
if (topics_header_value_result.header_value) {
modified_headers.SetHeader(kBrowsingTopicsRequestHeaderKey,
*topics_header_value);
*topics_header_value_result.header_value);
}

if (ad_auction_headers_eligible_) {
Expand Down

0 comments on commit 42e4a22

Please sign in to comment.