Skip to content

Commit d16bb42

Browse files
bbondybbondy
committed
Merge pull request #942 from brave/issue_2095_allow_cookies
Issue 2095: use the proper tab origin.
1 parent 302062f commit d16bb42

17 files changed

+205
-47
lines changed

‎browser/brave_content_browser_client.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ bool BraveContentBrowserClient::AllowAccessCookie(const GURL& url, const GURL& f
9797
content::ResourceContext* context, int render_process_id, int render_frame_id) {
9898
GURL tab_origin =
9999
BraveShieldsWebContentsObserver::GetTabURLFromRenderFrameInfo(
100-
render_process_id, render_frame_id).GetOrigin();
100+
render_process_id, render_frame_id, -1).GetOrigin();
101101
ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
102102
bool allow_brave_shields =
103103
brave_shields::IsAllowContentSettingWithIOData(

‎browser/net/brave_ad_block_tp_network_delegate_helper.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,11 @@ void OnBeforeURLRequestDispatchOnIOThread(
121121
ctx->new_url_spec != ctx->request_url.spec()) {
122122
if (ctx->blocked_by == kAdBlocked) {
123123
brave_shields::DispatchBlockedEventFromIO(ctx->request_url,
124-
ctx->render_process_id, ctx->render_frame_id, ctx->frame_tree_node_id,
124+
ctx->render_frame_id, ctx->render_process_id, ctx->frame_tree_node_id,
125125
brave_shields::kAds);
126126
} else if (ctx->blocked_by == kTrackerBlocked) {
127127
brave_shields::DispatchBlockedEventFromIO(ctx->request_url,
128-
ctx->render_process_id, ctx->render_frame_id, ctx->frame_tree_node_id,
128+
ctx->render_frame_id, ctx->render_process_id, ctx->frame_tree_node_id,
129129
brave_shields::kTrackers);
130130
}
131131
}

‎browser/net/brave_httpse_network_delegate_helper.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ void OnBeforeURLRequest_HttpsePostFileWork(
3333
if (!ctx->new_url_spec.empty() &&
3434
ctx->new_url_spec != ctx->request_url.spec()) {
3535
brave_shields::DispatchBlockedEventFromIO(ctx->request_url,
36-
ctx->render_process_id, ctx->render_frame_id, ctx->frame_tree_node_id,
36+
ctx->render_frame_id, ctx->render_process_id, ctx->frame_tree_node_id,
3737
brave_shields::kHTTPUpgradableResources);
3838
}
3939

@@ -79,7 +79,7 @@ int OnBeforeURLRequest_HttpsePreFileWork(
7979
} else {
8080
if (!ctx->new_url_spec.empty()) {
8181
brave_shields::DispatchBlockedEventFromIO(ctx->request_url,
82-
ctx->render_process_id, ctx->render_frame_id,
82+
ctx->render_frame_id, ctx->render_process_id,
8383
ctx->frame_tree_node_id,
8484
brave_shields::kHTTPUpgradableResources);
8585
}

‎browser/net/brave_network_delegate_base.cc

+10-14
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "base/task/post_task.h"
1010
#include "brave/common/pref_names.h"
1111
#include "brave/components/brave_shields/browser/brave_shields_util.h"
12+
#include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h"
1213
#include "brave/components/brave_shields/common/brave_shield_constants.h"
1314
#include "chrome/browser/browser_process.h"
1415
#include "chrome/browser/content_settings/tab_specific_content_settings.h"
@@ -33,6 +34,8 @@ namespace {
3334
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
3435
return content::WebContents::FromRenderFrameHost(rfh);
3536
}
37+
// TODO(iefremov): Seems like a typo?
38+
// issues/2263
3639
return content::WebContents::FromFrameTreeNodeId(render_frame_id);
3740
}
3841

@@ -160,14 +163,10 @@ bool BraveNetworkDelegateBase::OnCanGetCookies(const URLRequest& request,
160163
return callback.Run(ctx);
161164
});
162165

163-
int frame_id;
164-
int process_id;
165-
int frame_tree_node_id;
166-
brave_shields::GetRenderFrameInfo(&request, &frame_id, &process_id,
167-
&frame_tree_node_id);
168166
base::RepeatingCallback<content::WebContents*(void)> wc_getter =
169-
base::BindRepeating(&GetWebContentsFromProcessAndFrameId, process_id,
170-
frame_id);
167+
base::BindRepeating(&GetWebContentsFromProcessAndFrameId,
168+
ctx->render_process_id,
169+
ctx->render_frame_id);
171170
base::PostTaskWithTraits(
172171
FROM_HERE, {BrowserThread::UI},
173172
base::BindOnce(&TabSpecificContentSettings::CookiesRead, wc_getter,
@@ -185,19 +184,16 @@ bool BraveNetworkDelegateBase::OnCanSetCookie(const URLRequest& request,
185184
new brave::BraveRequestInfo());
186185
brave::BraveRequestInfo::FillCTXFromRequest(&request, ctx);
187186
ctx->event_type = brave::kOnCanSetCookies;
187+
188188
bool allow = std::all_of(can_set_cookies_callbacks_.begin(), can_set_cookies_callbacks_.end(),
189189
[&ctx](brave::OnCanSetCookiesCallback callback){
190190
return callback.Run(ctx);
191191
});
192192

193-
int frame_id;
194-
int process_id;
195-
int frame_tree_node_id;
196-
brave_shields::GetRenderFrameInfo(&request, &frame_id, &process_id,
197-
&frame_tree_node_id);
198193
base::RepeatingCallback<content::WebContents*(void)> wc_getter =
199-
base::BindRepeating(&GetWebContentsFromProcessAndFrameId, process_id,
200-
frame_id);
194+
base::BindRepeating(&GetWebContentsFromProcessAndFrameId,
195+
ctx->render_process_id,
196+
ctx->render_frame_id);
201197
base::PostTaskWithTraits(
202198
FROM_HERE, {BrowserThread::UI},
203199
base::BindOnce(&TabSpecificContentSettings::CookieChanged, wc_getter,

‎browser/net/brave_network_delegate_browsertest.cc

+101
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
3+
* You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
#include "base/path_service.h"
6+
#include "brave/common/brave_paths.h"
7+
#include "brave/components/brave_shields/common/brave_shield_constants.h"
8+
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
9+
#include "chrome/browser/profiles/profile.h"
10+
#include "chrome/browser/ui/browser.h"
11+
#include "chrome/test/base/in_process_browser_test.h"
12+
#include "chrome/test/base/ui_test_utils.h"
13+
#include "components/content_settings/core/browser/host_content_settings_map.h"
14+
#include "content/public/test/browser_test_utils.h"
15+
#include "net/dns/mock_host_resolver.h"
16+
#include "url/gurl.h"
17+
18+
class BraveNetworkDelegateBrowserTest : public InProcessBrowserTest {
19+
public:
20+
void SetUpOnMainThread() override {
21+
InProcessBrowserTest::SetUpOnMainThread();
22+
23+
host_resolver()->AddRule("*", "127.0.0.1");
24+
content::SetupCrossSiteRedirector(embedded_test_server());
25+
26+
brave::RegisterPathProvider();
27+
base::FilePath test_data_dir;
28+
base::PathService::Get(brave::DIR_TEST_DATA, &test_data_dir);
29+
embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
30+
31+
ASSERT_TRUE(embedded_test_server()->Start());
32+
33+
url_ = embedded_test_server()->GetURL("a.com", "/nested_iframe.html");
34+
nested_iframe_script_url_ =
35+
embedded_test_server()->GetURL("a.com", "/nested_iframe_script.html");
36+
37+
top_level_page_pattern_ =
38+
ContentSettingsPattern::FromString("http://a.com/*");
39+
first_party_pattern_ =
40+
ContentSettingsPattern::FromString("https://firstParty/*");
41+
}
42+
43+
HostContentSettingsMap* content_settings() {
44+
return HostContentSettingsMapFactory::GetForProfile(browser()->profile());
45+
}
46+
47+
void AllowCookies() {
48+
content_settings()->SetContentSettingCustomScope(
49+
top_level_page_pattern_, ContentSettingsPattern::Wildcard(),
50+
CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kCookies,
51+
CONTENT_SETTING_ALLOW);
52+
content_settings()->SetContentSettingCustomScope(
53+
top_level_page_pattern_, first_party_pattern_,
54+
CONTENT_SETTINGS_TYPE_PLUGINS, brave_shields::kCookies,
55+
CONTENT_SETTING_ALLOW);
56+
}
57+
58+
protected:
59+
GURL url_;
60+
GURL nested_iframe_script_url_;
61+
62+
private:
63+
ContentSettingsPattern top_level_page_pattern_;
64+
ContentSettingsPattern first_party_pattern_;
65+
ContentSettingsPattern iframe_pattern_;
66+
};
67+
68+
// It is important that cookies in following tests are set by response headers,
69+
// not by javascript. Fetching such cookies is controlled by NetworkDelegate.
70+
IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest, Iframe3PCookieBlocked) {
71+
ui_test_utils::NavigateToURL(browser(), url_);
72+
const std::string cookie =
73+
content::GetCookies(browser()->profile(), GURL("http://c.com/"));
74+
EXPECT_TRUE(cookie.empty()) << "Actual cookie: " << cookie;
75+
}
76+
77+
IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest, Iframe3PCookieAllowed) {
78+
AllowCookies();
79+
ui_test_utils::NavigateToURL(browser(), url_);
80+
const std::string cookie =
81+
content::GetCookies(browser()->profile(), GURL("http://c.com/"));
82+
EXPECT_FALSE(cookie.empty());
83+
}
84+
85+
// Fetching not just a frame, but some other resource.
86+
IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
87+
IframeJs3PCookieBlocked) {
88+
ui_test_utils::NavigateToURL(browser(), nested_iframe_script_url_);
89+
const std::string cookie =
90+
content::GetCookies(browser()->profile(), GURL("http://c.com/"));
91+
EXPECT_TRUE(cookie.empty()) << "Actual cookie: " << cookie;
92+
}
93+
94+
IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBrowserTest,
95+
IframeJs3PCookieAllowed) {
96+
AllowCookies();
97+
ui_test_utils::NavigateToURL(browser(), nested_iframe_script_url_);
98+
const std::string cookie =
99+
content::GetCookies(browser()->profile(), GURL("http://c.com/"));
100+
EXPECT_FALSE(cookie.empty());
101+
}

‎browser/net/url_context.cc

+16-4
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include "brave/common/url_constants.h"
1111
#include "brave/components/brave_shields/browser/brave_shields_util.h"
12+
#include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h"
1213
#include "brave/components/brave_shields/common/brave_shield_constants.h"
1314
#include "content/public/browser/resource_request_info.h"
1415

@@ -24,13 +25,25 @@ void BraveRequestInfo::FillCTXFromRequest(const net::URLRequest* request,
2425
std::shared_ptr<brave::BraveRequestInfo> ctx) {
2526
ctx->request_identifier = request->identifier();
2627
ctx->request_url = request->url();
27-
ctx->tab_origin = request->site_for_cookies().GetOrigin();
2828
auto* request_info = content::ResourceRequestInfo::ForRequest(request);
2929
if (request_info) {
3030
ctx->resource_type = request_info->GetResourceType();
3131
}
32-
brave_shields::GetRenderFrameInfo(request, &ctx->render_process_id, &ctx->render_frame_id,
33-
&ctx->frame_tree_node_id);
32+
brave_shields::GetRenderFrameInfo(request,
33+
&ctx->render_frame_id,
34+
&ctx->render_process_id,
35+
&ctx->frame_tree_node_id);
36+
if (!request->site_for_cookies().is_empty()) {
37+
ctx->tab_url = request->site_for_cookies();
38+
} else {
39+
// We can not always use site_for_cookies since it can be empty in certain
40+
// cases. See the comments in url_request.h
41+
ctx->tab_url = brave_shields::BraveShieldsWebContentsObserver::
42+
GetTabURLFromRenderFrameInfo(ctx->render_process_id,
43+
ctx->render_frame_id,
44+
ctx->frame_tree_node_id).GetOrigin();
45+
}
46+
ctx->tab_origin = ctx->tab_url.GetOrigin();
3447
ctx->allow_brave_shields = brave_shields::IsAllowContentSettingFromIO(
3548
request, ctx->tab_origin, ctx->tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS,
3649
brave_shields::kBraveShields) &&
@@ -50,5 +63,4 @@ void BraveRequestInfo::FillCTXFromRequest(const net::URLRequest* request,
5063
ctx->request = request;
5164
}
5265

53-
5466
} // namespace brave

‎browser/net/url_context.h

+1
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ struct BraveRequestInfo {
5151
~BraveRequestInfo();
5252
GURL request_url;
5353
GURL tab_origin;
54+
GURL tab_url;
5455
std::string new_url_spec;
5556
bool allow_brave_shields = true;
5657
bool allow_ads = false;

‎components/brave_shields/browser/brave_shields_util.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,9 @@ void GetRenderFrameInfo(const URLRequest* request,
107107
if (request_info) {
108108
*frame_tree_node_id = request_info->GetFrameTreeNodeId();
109109
}
110-
extensions::ExtensionApiFrameIdMap::FrameData frame_data;
111110
if (!content::ResourceRequestInfo::GetRenderFrameForRequest(
112111
request, render_process_id, render_frame_id)) {
112+
113113
const content::WebSocketHandshakeRequestInfo* websocket_info =
114114
content::WebSocketHandshakeRequestInfo::ForRequest(request);
115115
if (websocket_info) {

‎components/brave_shields/browser/brave_shields_web_contents_observer.cc

+37-19
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,9 @@ namespace brave_shields {
9494

9595
base::Lock BraveShieldsWebContentsObserver::frame_data_map_lock_;
9696
std::map<BraveShieldsWebContentsObserver::RenderFrameIdKey, GURL>
97-
BraveShieldsWebContentsObserver::render_frame_key_to_tab_url;
97+
BraveShieldsWebContentsObserver::frame_key_to_tab_url_;
98+
std::map<int, GURL>
99+
BraveShieldsWebContentsObserver::frame_tree_node_id_to_tab_url_;
98100

99101
BraveShieldsWebContentsObserver::RenderFrameIdKey::RenderFrameIdKey()
100102
: render_process_id(content::ChildProcessHost::kInvalidUniqueID),
@@ -136,27 +138,21 @@ void BraveShieldsWebContentsObserver::RenderFrameCreated(
136138
WebContents* web_contents = WebContents::FromRenderFrameHost(rfh);
137139
if (web_contents) {
138140
UpdateContentSettingsToRendererFrames(web_contents);
141+
139142
base::AutoLock lock(frame_data_map_lock_);
140143
const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
141-
std::map<RenderFrameIdKey, GURL>::iterator iter =
142-
render_frame_key_to_tab_url.find(key);
143-
if (iter != render_frame_key_to_tab_url.end()) {
144-
render_frame_key_to_tab_url.erase(key);
145-
}
146-
render_frame_key_to_tab_url.insert(
147-
std::pair<RenderFrameIdKey, GURL>(key, web_contents->GetURL()));
144+
frame_key_to_tab_url_[key] = web_contents->GetURL();
145+
frame_tree_node_id_to_tab_url_[rfh->GetFrameTreeNodeId()] =
146+
web_contents->GetURL();
148147
}
149148
}
150149

151150
void BraveShieldsWebContentsObserver::RenderFrameDeleted(
152151
RenderFrameHost* rfh) {
153152
base::AutoLock lock(frame_data_map_lock_);
154153
const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
155-
std::map<RenderFrameIdKey, GURL>::iterator iter =
156-
render_frame_key_to_tab_url.find(key);
157-
if (iter != render_frame_key_to_tab_url.end()) {
158-
render_frame_key_to_tab_url.erase(key);
159-
}
154+
frame_key_to_tab_url_.erase(key);
155+
frame_tree_node_id_to_tab_url_.erase(rfh->GetFrameTreeNodeId());
160156
}
161157

162158
void BraveShieldsWebContentsObserver::RenderFrameHostChanged(
@@ -169,15 +165,37 @@ void BraveShieldsWebContentsObserver::RenderFrameHostChanged(
169165
}
170166
}
171167

168+
void BraveShieldsWebContentsObserver::DidFinishNavigation(
169+
content::NavigationHandle* navigation_handle) {
170+
RenderFrameHost* main_frame = web_contents()->GetMainFrame();
171+
if (!web_contents() || !main_frame) {
172+
return;
173+
}
174+
int process_id = main_frame->GetProcess()->GetID();
175+
int routing_id = main_frame->GetRoutingID();
176+
int tree_node_id = main_frame->GetFrameTreeNodeId();
177+
178+
base::AutoLock lock(frame_data_map_lock_);
179+
frame_key_to_tab_url_[{process_id, routing_id}] = web_contents()->GetURL();
180+
frame_tree_node_id_to_tab_url_[tree_node_id] = web_contents()->GetURL();
181+
}
182+
172183
// static
173184
GURL BraveShieldsWebContentsObserver::GetTabURLFromRenderFrameInfo(
174-
int render_process_id, int render_frame_id) {
185+
int render_process_id, int render_frame_id, int render_frame_tree_node_id) {
175186
base::AutoLock lock(frame_data_map_lock_);
176-
const RenderFrameIdKey key(render_process_id, render_frame_id);
177-
std::map<RenderFrameIdKey, GURL>::iterator iter =
178-
render_frame_key_to_tab_url.find(key);
179-
if (iter != render_frame_key_to_tab_url.end()) {
180-
return iter->second;
187+
if (-1 != render_process_id && -1 != render_frame_id) {
188+
auto iter = frame_key_to_tab_url_.find({render_process_id,
189+
render_frame_id});
190+
if (iter != frame_key_to_tab_url_.end()) {
191+
return iter->second;
192+
}
193+
}
194+
if (-1 != render_frame_tree_node_id) {
195+
auto iter2 = frame_tree_node_id_to_tab_url_.find(render_frame_tree_node_id);
196+
if (iter2 != frame_tree_node_id_to_tab_url_.end()) {
197+
return iter2->second;
198+
}
181199
}
182200
return GURL();
183201
}

‎components/brave_shields/browser/brave_shields_web_contents_observer.h

+10-4
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@ class BraveShieldsWebContentsObserver : public content::WebContentsObserver,
3535
std::string subresource,
3636
int render_process_id,
3737
int render_frame_id, int frame_tree_node_id);
38-
static GURL GetTabURLFromRenderFrameInfo(int render_process_id, int render_frame_id);
38+
static GURL GetTabURLFromRenderFrameInfo(int render_process_id,
39+
int render_frame_id,
40+
int render_frame_tree_node_id);
3941
void AllowScriptsOnce(const std::vector<std::string>& origins,
4042
content::WebContents* web_contents);
4143
bool IsBlockedSubresource(const std::string& subresource);
@@ -64,6 +66,8 @@ class BraveShieldsWebContentsObserver : public content::WebContentsObserver,
6466
content::RenderFrameHost* new_host) override;
6567
void ReadyToCommitNavigation(
6668
content::NavigationHandle* navigation_handle) override;
69+
void DidFinishNavigation(
70+
content::NavigationHandle* navigation_handle) override;
6771

6872
// Invoked if an IPC message is coming from a specific RenderFrameHost.
6973
bool OnMessageReceived(const IPC::Message& message,
@@ -75,10 +79,12 @@ class BraveShieldsWebContentsObserver : public content::WebContentsObserver,
7579
content::RenderFrameHost* render_frame_host,
7680
const base::string16& details);
7781

78-
static std::map<RenderFrameIdKey, GURL> render_frame_key_to_tab_url;
79-
// This lock protects |frame_data_map_| from being concurrently written on the
80-
// UI thread and read on the IO thread.
82+
// TODO(iefremov): Refactor this away or at least put into base::NoDestructor.
83+
// Protects global maps below from being concurrently written on the UI thread
84+
// and read on the IO thread.
8185
static base::Lock frame_data_map_lock_;
86+
static std::map<RenderFrameIdKey, GURL> frame_key_to_tab_url_;
87+
static std::map<int, GURL> frame_tree_node_id_to_tab_url_;
8288

8389
private:
8490
friend class content::WebContentsUserData<BraveShieldsWebContentsObserver>;

‎test/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ test("brave_browser_tests") {
227227
"//brave/browser/extensions/brave_extension_functional_test.cc",
228228
"//brave/browser/extensions/brave_extension_functional_test.h",
229229
"//brave/browser/extensions/api/brave_shields_api_browsertest.cc",
230+
"//brave/browser/net/brave_network_delegate_browsertest.cc",
230231
"//brave/browser/renderer_context_menu/brave_mock_render_view_context_menu.cc",
231232
"//brave/browser/renderer_context_menu/brave_mock_render_view_context_menu.h",
232233
"//brave/browser/renderer_context_menu/brave_spelling_menu_observer_browsertest.cc",

0 commit comments

Comments
 (0)