Skip to content

Commit

Permalink
Reland "Extensions: Make most of WebRequestInfo immutable."
Browse files Browse the repository at this point in the history 
This is a reland of ac4285c. We now ensure that
all primitives in WebRequestInfoInitParams are correctly initialized.

Original change's description:
> Extensions: Make most of WebRequestInfo immutable.
>
> This CL makes most of the WebRequestInfo data members const. To achieve this, a
> new helper struct called WebRequestInfoInitParams is introduced. This will help
> with guarantees around WebRequestInfo's immutability.
>
> BUG=696822
> TBR=mmenke@chromium.org
>
> Change-Id: I36bd8284aca9e68e115fba94b579456902f968ec
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594066
> Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#656970}

TBR=mmenke@chromium.org

Bug: 696822
Change-Id: Ib86bdf1ad6e66f80d86adb038fafcd0d341c937a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600593
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657566}
  • Loading branch information
Karan Bhatia authored and Commit Bot committed May 8, 2019
1 parent 72e9f60 commit b711c9d
Show file tree
Hide file tree
Showing 20 changed files with 374 additions and 242 deletions.
30 changes: 19 additions & 11 deletions chrome/browser/extensions/api/chrome_extensions_api_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,31 @@ TEST_F(ChromeExtensionsAPIClientTest, ShouldHideResponseHeader) {
TEST_F(ChromeExtensionsAPIClientTest, ShouldHideBrowserNetworkRequest) {
ChromeExtensionsAPIClient client;

auto create_params = [](content::ResourceType type) {
WebRequestInfoInitParams request_params;
request_params.url = GURL("https://example.com/script.js");
request_params.initiator =
url::Origin::Create(GURL(chrome::kChromeUINewTabURL));
request_params.render_process_id = -1;
request_params.type = type;
return request_params;
};

// Requests made by the browser with chrome://newtab as its initiator should
// not be visible to extensions.
WebRequestInfo request;
request.url = GURL("https://example.com/script.js");
request.initiator = url::Origin::Create(GURL(chrome::kChromeUINewTabURL));
request.render_process_id = -1;
request.type = content::ResourceType::kScript;
EXPECT_TRUE(client.ShouldHideBrowserNetworkRequest(request));
EXPECT_TRUE(client.ShouldHideBrowserNetworkRequest(
WebRequestInfo(create_params(content::ResourceType::kScript))));

// Main frame requests should always be visible to extensions.
request.type = content::ResourceType::kMainFrame;
EXPECT_FALSE(client.ShouldHideBrowserNetworkRequest(request));
EXPECT_FALSE(client.ShouldHideBrowserNetworkRequest(
WebRequestInfo(create_params(content::ResourceType::kMainFrame))));

// Similar requests made by the renderer should be visible to extensions.
request.type = content::ResourceType::kScript;
request.render_process_id = 2;
EXPECT_FALSE(client.ShouldHideBrowserNetworkRequest(request));
WebRequestInfoInitParams params =
create_params(content::ResourceType::kScript);
params.render_process_id = 2;
EXPECT_FALSE(client.ShouldHideBrowserNetworkRequest(
WebRequestInfo(std::move(params))));
}

} // namespace extensions
107 changes: 60 additions & 47 deletions chrome/browser/extensions/api/declarative_net_request/ruleset_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,15 @@ class RulesetManagerTest : public DNRTestBase {
return last_loaded_extension_.get();
}

// Returns a renderer-initiated request to the given |url|.
WebRequestInfo GetRequestForURL(base::StringPiece url) {
// Returns renderer-initiated request params for the given |url|.
WebRequestInfoInitParams GetRequestParamsForURL(
base::StringPiece url,
base::Optional<url::Origin> initiator = base::nullopt) {
const int kRendererId = 1;
WebRequestInfo info;
WebRequestInfoInitParams info;
info.url = GURL(url);
info.render_process_id = kRendererId;
info.initiator = std::move(initiator);
return info;
}

Expand Down Expand Up @@ -160,9 +163,9 @@ TEST_P(RulesetManagerTest, MultipleRulesets) {
RulesetManager* manager = info_map()->GetRulesetManager();
ASSERT_TRUE(manager);

WebRequestInfo request_one_info = GetRequestForURL("http://one.com");
WebRequestInfo request_two_info = GetRequestForURL("http://two.com");
WebRequestInfo request_three_info = GetRequestForURL("http://three.com");
WebRequestInfo request_one_info(GetRequestParamsForURL("http://one.com"));
WebRequestInfo request_two_info(GetRequestParamsForURL("http://two.com"));
WebRequestInfo request_three_info(GetRequestParamsForURL("http://three.com"));

auto should_block_request = [manager](const WebRequestInfo& request) {
return manager->EvaluateRequest(request, false /*is_incognito_context*/) ==
Expand Down Expand Up @@ -228,7 +231,7 @@ TEST_P(RulesetManagerTest, IncognitoRequests) {
manager->AddRuleset(last_loaded_extension()->id(), std::move(matcher),
URLPatternSet());

WebRequestInfo request_info = GetRequestForURL("http://example.com");
WebRequestInfo request_info(GetRequestParamsForURL("http://example.com"));

// By default, the extension is disabled in incognito mode. So requests from
// incognito contexts should not be evaluated.
Expand Down Expand Up @@ -261,8 +264,10 @@ TEST_P(RulesetManagerTest, TotalEvaluationTimeHistogram) {
RulesetManager* manager = info_map()->GetRulesetManager();
ASSERT_TRUE(manager);

WebRequestInfo example_com_request = GetRequestForURL("http://example.com");
WebRequestInfo google_com_request = GetRequestForURL("http://google.com");
WebRequestInfo example_com_request(
GetRequestParamsForURL("http://example.com"));
WebRequestInfo google_com_request(
GetRequestParamsForURL("http://google.com"));
bool is_incognito_context = false;
const char* kHistogramName =
"Extensions.DeclarativeNetRequest.EvaluateRequestTime.AllExtensions2";
Expand Down Expand Up @@ -320,29 +325,31 @@ TEST_P(RulesetManagerTest, Redirect) {
// Create a request to "example.com" with an empty initiator. It should be
// redirected to "google.com".
const bool is_incognito_context = false;
WebRequestInfo request = GetRequestForURL("http://example.com");
request.initiator = base::nullopt;
Action action = manager->EvaluateRequest(request, is_incognito_context);
const char* kExampleURL = "http://example.com";
WebRequestInfo request_1(GetRequestParamsForURL(kExampleURL, base::nullopt));
Action action = manager->EvaluateRequest(request_1, is_incognito_context);
EXPECT_EQ(ActionType::REDIRECT, action.type);
EXPECT_EQ(GURL("http://google.com"), action.redirect_url);

// Change the initiator to "xyz.com". It should not be redirected since we
// don't have host permissions to the request initiator.
request.initiator = url::Origin::Create(GURL("http://xyz.com"));
action = manager->EvaluateRequest(request, is_incognito_context);
WebRequestInfo request_2(GetRequestParamsForURL(
kExampleURL, url::Origin::Create(GURL("http://xyz.com"))));
action = manager->EvaluateRequest(request_2, is_incognito_context);
EXPECT_EQ(Action(ActionType::NONE), action);

// Change the initiator to "abc.com". It should be redirected since we have
// the required host permissions.
request.initiator = url::Origin::Create(GURL("http://abc.com"));
action = manager->EvaluateRequest(request, is_incognito_context);
WebRequestInfo request_3(GetRequestParamsForURL(
kExampleURL, url::Origin::Create(GURL("http://abc.com"))));
action = manager->EvaluateRequest(request_3, is_incognito_context);
EXPECT_EQ(ActionType::REDIRECT, action.type);
EXPECT_EQ(GURL("http://google.com"), action.redirect_url);

// Ensure web-socket requests are not redirected.
request = GetRequestForURL("ws://example.com");
request.initiator = base::nullopt;
action = manager->EvaluateRequest(request, is_incognito_context);
WebRequestInfo request_4(
GetRequestParamsForURL("ws://example.com", base::nullopt));
action = manager->EvaluateRequest(request_4, is_incognito_context);
EXPECT_EQ(Action(ActionType::NONE), action);
}

Expand Down Expand Up @@ -388,32 +395,38 @@ TEST_P(RulesetManagerTest, ExtensionScheme) {

// Ensure that "http://example.com" will be blocked (with blocking taking
// priority over redirection).
WebRequestInfo request = GetRequestForURL("http://example.com");
EXPECT_EQ(Action(ActionType::BLOCK),
manager->EvaluateRequest(request, false /*is_incognito_context*/));
WebRequestInfo request_1(GetRequestParamsForURL("http://example.com"));
EXPECT_EQ(
Action(ActionType::BLOCK),
manager->EvaluateRequest(request_1, false /*is_incognito_context*/));

// Ensure that the background page for |extension_1| won't be blocked or
// redirected.
GURL background_page_url_1 = BackgroundInfo::GetBackgroundURL(extension_1);
EXPECT_TRUE(!background_page_url_1.is_empty());
request = GetRequestForURL(background_page_url_1.spec());
EXPECT_EQ(Action(ActionType::NONE),
manager->EvaluateRequest(request, false /*is_incognito_context*/));
WebRequestInfo request_2(
GetRequestParamsForURL(background_page_url_1.spec()));
EXPECT_EQ(
Action(ActionType::NONE),
manager->EvaluateRequest(request_2, false /*is_incognito_context*/));

// Ensure that the background page for |extension_2| won't be blocked or
// redirected.
GURL background_page_url_2 = BackgroundInfo::GetBackgroundURL(extension_2);
EXPECT_TRUE(!background_page_url_2.is_empty());
request = GetRequestForURL(background_page_url_2.spec());
EXPECT_EQ(Action(ActionType::NONE),
manager->EvaluateRequest(request, false /*is_incognito_context*/));
WebRequestInfo request_3(
GetRequestParamsForURL(background_page_url_2.spec()));
EXPECT_EQ(
Action(ActionType::NONE),
manager->EvaluateRequest(request_3, false /*is_incognito_context*/));

// Also ensure that an arbitrary url on the chrome extension scheme is also
// not blocked or redirected.
request = GetRequestForURL(base::StringPrintf("%s://%s/%s", kExtensionScheme,
"extension_id", "path"));
EXPECT_EQ(Action(ActionType::NONE),
manager->EvaluateRequest(request, false /*is_incognito_context*/));
WebRequestInfo request_4(GetRequestParamsForURL(base::StringPrintf(
"%s://%s/%s", kExtensionScheme, "extension_id", "path")));
EXPECT_EQ(
Action(ActionType::NONE),
manager->EvaluateRequest(request_4, false /*is_incognito_context*/));
}

TEST_P(RulesetManagerTest, PageAllowingAPI) {
Expand Down Expand Up @@ -549,30 +562,31 @@ TEST_P(RulesetManagerTest, PageAllowingAPI) {
SCOPED_TRACE(base::StringPrintf("Testing case number %zu with url %s",
i + 1, test_case.url.c_str()));

WebRequestInfo info = GetRequestForURL(test_case.url);
ASSERT_TRUE(info.url.is_valid());
info.type = test_case.type;
WebRequestInfoInitParams params = GetRequestParamsForURL(test_case.url);
ASSERT_TRUE(params.url.is_valid());
params.type = test_case.type;

if (test_case.initiator)
info.initiator = url::Origin::Create(GURL(*test_case.initiator));
params.initiator = url::Origin::Create(GURL(*test_case.initiator));

info.frame_id = test_case.frame_routing_id;
params.frame_id = test_case.frame_routing_id;

if (test_case.frame_data_params) {
const FrameDataParams& params = *test_case.frame_data_params;
info.frame_data = ExtensionApiFrameIdMap::FrameData(
params.frame_id, params.parent_frame_id, kDummyTabId, kDummyWindowId,
GURL(params.last_committed_main_frame_url));
if (params.pending_main_frame_url)
info.frame_data->pending_main_frame_url =
GURL(*params.pending_main_frame_url);
const FrameDataParams& frame_params = *test_case.frame_data_params;
params.frame_data = ExtensionApiFrameIdMap::FrameData(
frame_params.frame_id, frame_params.parent_frame_id, kDummyTabId,
kDummyWindowId, GURL(frame_params.last_committed_main_frame_url));
if (frame_params.pending_main_frame_url)
params.frame_data->pending_main_frame_url =
GURL(*frame_params.pending_main_frame_url);
}

Action expected_action = test_case.expect_blocked_with_allowed_pages
? Action(ActionType::BLOCK)
: Action(ActionType::NONE);
EXPECT_EQ(expected_action,
manager->EvaluateRequest(info, false /*is_incognito_context*/));
manager->EvaluateRequest(WebRequestInfo(std::move(params)),
false /*is_incognito_context*/));
}
}

Expand Down Expand Up @@ -645,8 +659,7 @@ TEST_P(RulesetManagerTest, HostPermissionForInitiator) {
"Url-%s initiator-%s", url.c_str(),
initiator ? initiator->Serialize().c_str() : "empty"));

WebRequestInfo request = GetRequestForURL(url);
request.initiator = initiator;
WebRequestInfo request(GetRequestParamsForURL(url, initiator));

bool is_incognito_context = false;
EXPECT_EQ(expected_action,
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/extensions/api/declarative_webrequest/webrequest_action_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,9 @@ bool WebRequestActionWithThreadsTest::ActionWorksOnRequest(
EventResponseDeltas deltas;
scoped_refptr<net::HttpResponseHeaders> headers(
new net::HttpResponseHeaders(""));
WebRequestInfo request_info(regular_request.get());
request_info.render_process_id = kRendererId;
WebRequestInfoInitParams request_params(regular_request.get());
request_params.render_process_id = kRendererId;
WebRequestInfo request_info(std::move(request_params));
WebRequestData request_data(&request_info, stage, headers.get());
std::set<std::string> ignored_tags;
WebRequestAction::ApplyInfo apply_info = { extension_info_map_.get(),
Expand Down
25 changes: 13 additions & 12 deletions chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ const char kRuleId3[] = "rule3";
const char kRuleId4[] = "rule4";

// Creates a main-frame navigation request to |url|.
WebRequestInfo CreateRequest(const GURL& url) {
WebRequestInfo info;
WebRequestInfoInitParams CreateRequestParams(const GURL& url) {
WebRequestInfoInitParams info;
info.url = url;
info.is_browser_side_navigation = true;
info.type = content::ResourceType::kMainFrame;
Expand Down Expand Up @@ -286,7 +286,7 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
std::set<const WebRequestRule*> matches;

GURL http_url("http://www.example.com");
WebRequestInfo http_request_info = CreateRequest(http_url);
WebRequestInfo http_request_info(CreateRequestParams(http_url));
WebRequestData request_data(&http_request_info, ON_BEFORE_REQUEST);
matches = registry->GetMatches(request_data);
EXPECT_EQ(2u, matches.size());
Expand All @@ -300,7 +300,7 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
base::ContainsKey(matches_ids, std::make_pair(kExtensionId, kRuleId2)));

GURL foobar_url("http://www.foobar.com");
WebRequestInfo foobar_request_info = CreateRequest(foobar_url);
WebRequestInfo foobar_request_info(CreateRequestParams(foobar_url));
request_data.request = &foobar_request_info;
matches = registry->GetMatches(request_data);
EXPECT_EQ(1u, matches.size());
Expand Down Expand Up @@ -436,7 +436,7 @@ TEST_F(WebRequestRulesRegistryTest, Precedences) {
}

GURL url("http://www.google.com");
WebRequestInfo request_info = CreateRequest(url);
WebRequestInfo request_info(CreateRequestParams(url));
WebRequestData request_data(&request_info, ON_BEFORE_REQUEST);
EventResponseDeltas deltas =
registry->CreateDeltas(NULL, request_data, false);
Expand Down Expand Up @@ -488,7 +488,7 @@ TEST_F(WebRequestRulesRegistryTest, Priorities) {
}

GURL url("http://www.google.com/index.html");
WebRequestInfo request_info = CreateRequest(url);
WebRequestInfo request_info(CreateRequestParams(url));
WebRequestData request_data(&request_info, ON_BEFORE_REQUEST);
EventResponseDeltas deltas =
registry->CreateDeltas(nullptr, request_data, false);
Expand Down Expand Up @@ -564,7 +564,7 @@ TEST_F(WebRequestRulesRegistryTest, IgnoreRulesByTag) {
EXPECT_FALSE(registry->IsEmpty());

GURL url("http://www.foo.com/test");
WebRequestInfo request_info = CreateRequest(url);
WebRequestInfo request_info(CreateRequestParams(url));
WebRequestData request_data(&request_info, ON_BEFORE_REQUEST);
EventResponseDeltas deltas =
registry->CreateDeltas(NULL, request_data, false);
Expand Down Expand Up @@ -614,7 +614,7 @@ TEST_F(WebRequestRulesRegistryTest, GetMatchesCheckFulfilled) {
std::set<const WebRequestRule*> matches;

GURL http_url("http://www.example.com");
WebRequestInfo http_request_info = CreateRequest(http_url);
WebRequestInfo http_request_info(CreateRequestParams(http_url));
WebRequestData request_data(&http_request_info, ON_BEFORE_REQUEST);
matches = registry->GetMatches(request_data);
EXPECT_EQ(1u, matches.size());
Expand Down Expand Up @@ -674,8 +674,9 @@ TEST_F(WebRequestRulesRegistryTest, GetMatchesDifferentUrls) {

for (size_t i = 0; i < base::size(matchingRuleIds); ++i) {
// Construct the inputs.
WebRequestInfo http_request_info = CreateRequest(urls[i]);
http_request_info.site_for_cookies = firstPartyUrls[i];
WebRequestInfoInitParams params = CreateRequestParams(urls[i]);
params.site_for_cookies = firstPartyUrls[i];
WebRequestInfo http_request_info(std::move(params));
WebRequestData request_data(&http_request_info, ON_BEFORE_REQUEST);
// Now run both rules on the input.
matches = registry->GetMatches(request_data);
Expand Down Expand Up @@ -823,14 +824,14 @@ TEST_F(WebRequestRulesRegistryTest, CheckOriginAndPathRegEx) {

// No match because match is in the query parameter.
GURL url1("http://bar.com/index.html?foo.com");
WebRequestInfo request1_info = CreateRequest(url1);
WebRequestInfo request1_info(CreateRequestParams(url1));
WebRequestData request_data1(&request1_info, ON_BEFORE_REQUEST);
deltas = registry->CreateDeltas(NULL, request_data1, false);
EXPECT_EQ(0u, deltas.size());

// This is a correct match.
GURL url2("http://foo.com/index.html");
WebRequestInfo request2_info = CreateRequest(url2);
WebRequestInfo request2_info(CreateRequestParams(url2));
WebRequestData request_data2(&request2_info, ON_BEFORE_REQUEST);
deltas = registry->CreateDeltas(NULL, request_data2, false);
EXPECT_EQ(1u, deltas.size());
Expand Down
Loading

0 comments on commit b711c9d

Please sign in to comment.