Skip to content

Commit

Permalink
Populate NetworkIsolationKey for websockets
Browse files Browse the repository at this point in the history
Populate NIK for websocket requests to provide a TopFrameOrigin
for cookie access checks.

Bug: 989067
Change-Id: Ic3bceee832a33abea84d2f0fc5ab0291c5f30336
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1859986
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Ehimare Okoyomon <eokoyomon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708586}
  • Loading branch information
Ehimare Okoyomon authored and Commit Bot committed Oct 23, 2019
1 parent 60f19c8 commit 4446d8c
Show file tree
Hide file tree
Showing 23 changed files with 139 additions and 58 deletions.
3 changes: 2 additions & 1 deletion content/browser/frame_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6557,7 +6557,8 @@ void RenderFrameHostImpl::CreateWebSocketConnector(
mojo::PendingReceiver<blink::mojom::WebSocketConnector> receiver) {
mojo::MakeSelfOwnedReceiver(
std::make_unique<WebSocketConnectorImpl>(
GetProcess()->GetID(), routing_id_, last_committed_origin_),
GetProcess()->GetID(), routing_id_, last_committed_origin_,
network_isolation_key_),
std::move(receiver));
}

Expand Down
8 changes: 7 additions & 1 deletion content/browser/renderer_interface_binders.cc
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,14 @@ void RendererInterfaceBinders::CreateWebSocketConnector(
const url::Origin& origin) {
// TODO(jam): is it ok to not send extraHeaders for sockets created from
// shared and service workers?
//
// Shared Workers and service workers are not directly associated with a
// frame, so the concept of "top-level frame" does not exist. Can use
// (origin, origin) for the NetworkIsolationKey for requests because these
// workers can only be created when the site has cookie access.
mojo::MakeSelfOwnedReceiver(std::make_unique<WebSocketConnectorImpl>(
host->GetID(), MSG_ROUTING_NONE, origin),
host->GetID(), MSG_ROUTING_NONE, origin,
net::NetworkIsolationKey(origin, origin)),
std::move(receiver));
}

Expand Down
32 changes: 20 additions & 12 deletions content/browser/websockets/websocket_connector_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,15 @@

namespace content {

WebSocketConnectorImpl::WebSocketConnectorImpl(int process_id,
int frame_id,
const url::Origin& origin)
: process_id_(process_id), frame_id_(frame_id), origin_(origin) {}
WebSocketConnectorImpl::WebSocketConnectorImpl(
int process_id,
int frame_id,
const url::Origin& origin,
const net::NetworkIsolationKey& network_isolation_key)
: process_id_(process_id),
frame_id_(frame_id),
origin_(origin),
network_isolation_key_(network_isolation_key) {}

WebSocketConnectorImpl::~WebSocketConnectorImpl() = default;

Expand All @@ -33,6 +38,7 @@ void WebSocketConnectorImpl::Connect(
if (!process) {
return;
}

RenderFrameHost* frame = RenderFrameHost::FromID(process_id_, frame_id_);
const uint32_t options =
GetContentClient()->browser()->GetWebSocketOptions(frame);
Expand All @@ -41,8 +47,8 @@ void WebSocketConnectorImpl::Connect(
GetContentClient()->browser()->CreateWebSocket(
frame,
base::BindOnce(ConnectCalledByContentBrowserClient, requested_protocols,
site_for_cookies, process_id_, frame_id_, origin_,
options),
site_for_cookies, network_isolation_key_, process_id_,
frame_id_, origin_, options),
url, site_for_cookies, user_agent, std::move(handshake_client));
return;
}
Expand All @@ -52,14 +58,15 @@ void WebSocketConnectorImpl::Connect(
net::HttpRequestHeaders::kUserAgent, *user_agent));
}
process->GetStoragePartition()->GetNetworkContext()->CreateWebSocket(
url, requested_protocols, site_for_cookies, std::move(headers),
process_id_, frame_id_, origin_, options, std::move(handshake_client),
mojo::NullRemote(), mojo::NullRemote());
url, requested_protocols, site_for_cookies, network_isolation_key_,
std::move(headers), process_id_, frame_id_, origin_, options,
std::move(handshake_client), mojo::NullRemote(), mojo::NullRemote());
}

void WebSocketConnectorImpl::ConnectCalledByContentBrowserClient(
const std::vector<std::string>& requested_protocols,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
int process_id,
int frame_id,
const url::Origin& origin,
Expand All @@ -77,9 +84,10 @@ void WebSocketConnectorImpl::ConnectCalledByContentBrowserClient(
return;
}
process->GetStoragePartition()->GetNetworkContext()->CreateWebSocket(
url, requested_protocols, site_for_cookies, std::move(additional_headers),
process_id, frame_id, origin, options, std::move(handshake_client),
std::move(auth_handler), std::move(trusted_header_client));
url, requested_protocols, site_for_cookies, network_isolation_key,
std::move(additional_headers), process_id, frame_id, origin, options,
std::move(handshake_client), std::move(auth_handler),
std::move(trusted_header_client));
}

} // namespace content
5 changes: 4 additions & 1 deletion content/browser/websockets/websocket_connector_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ class WebSocketConnectorImpl final : public blink::mojom::WebSocketConnector {
// MSG_ROUTING_NONE because they do not have a frame.
WebSocketConnectorImpl(int process_id,
int frame_id,
const url::Origin& origin);
const url::Origin& origin,
const net::NetworkIsolationKey& network_isolation_key);
~WebSocketConnectorImpl() override;

// WebSocketConnector implementation
Expand All @@ -45,6 +46,7 @@ class WebSocketConnectorImpl final : public blink::mojom::WebSocketConnector {
static void ConnectCalledByContentBrowserClient(
const std::vector<std::string>& requested_protocols,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
int process_id,
int frame_id,
const url::Origin& origin,
Expand All @@ -60,6 +62,7 @@ class WebSocketConnectorImpl final : public blink::mojom::WebSocketConnector {
const int process_id_;
const int frame_id_;
const url::Origin origin_;
const net::NetworkIsolationKey network_isolation_key_;
};

} // namespace content
Expand Down
8 changes: 4 additions & 4 deletions content/browser/worker_host/dedicated_worker_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,10 @@ void DedicatedWorkerHost::CreateWebSocketConnector(
"The parent frame has already been gone.");
return;
}
mojo::MakeSelfOwnedReceiver(
std::make_unique<WebSocketConnectorImpl>(
worker_process_id_, ancestor_render_frame_id_, origin_),
std::move(receiver));
mojo::MakeSelfOwnedReceiver(std::make_unique<WebSocketConnectorImpl>(
worker_process_id_, ancestor_render_frame_id_,
origin_, network_isolation_key_),
std::move(receiver));
}

void DedicatedWorkerHost::CreateNestedDedicatedWorker(
Expand Down
20 changes: 12 additions & 8 deletions net/websockets/websocket_channel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -265,10 +265,12 @@ void WebSocketChannel::SendAddChannelRequest(
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers) {
SendAddChannelRequestWithSuppliedCallback(
socket_url, requested_subprotocols, origin, site_for_cookies,
additional_headers, base::Bind(&WebSocketStream::CreateAndConnectStream));
network_isolation_key, additional_headers,
base::Bind(&WebSocketStream::CreateAndConnectStream));
}

void WebSocketChannel::SetState(State new_state) {
Expand Down Expand Up @@ -407,11 +409,12 @@ void WebSocketChannel::SendAddChannelRequestForTesting(
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
const WebSocketStreamRequestCreationCallback& callback) {
SendAddChannelRequestWithSuppliedCallback(socket_url, requested_subprotocols,
origin, site_for_cookies,
additional_headers, callback);
SendAddChannelRequestWithSuppliedCallback(
socket_url, requested_subprotocols, origin, site_for_cookies,
network_isolation_key, additional_headers, callback);
}

void WebSocketChannel::SetClosingHandshakeTimeoutForTesting(
Expand All @@ -429,6 +432,7 @@ void WebSocketChannel::SendAddChannelRequestWithSuppliedCallback(
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
const WebSocketStreamRequestCreationCallback& callback) {
DCHECK_EQ(FRESHLY_CONSTRUCTED, state_);
Expand All @@ -441,10 +445,10 @@ void WebSocketChannel::SendAddChannelRequestWithSuppliedCallback(
}
socket_url_ = socket_url;
auto connect_delegate = std::make_unique<ConnectDelegate>(this);
stream_request_ =
callback.Run(socket_url_, requested_subprotocols, origin,
site_for_cookies, additional_headers, url_request_context_,
NetLogWithSource(), std::move(connect_delegate));
stream_request_ = callback.Run(
socket_url_, requested_subprotocols, origin, site_for_cookies,
network_isolation_key, additional_headers, url_request_context_,
NetLogWithSource(), std::move(connect_delegate));
SetState(CONNECTING);
}

Expand Down
4 changes: 4 additions & 0 deletions net/websockets/websocket_channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class NET_EXPORT WebSocketChannel {
const std::vector<std::string>&,
const url::Origin&,
const GURL&,
const net::NetworkIsolationKey&,
const HttpRequestHeaders&,
URLRequestContext*,
const NetLogWithSource&,
Expand All @@ -79,6 +80,7 @@ class NET_EXPORT WebSocketChannel {
const std::vector<std::string>& requested_protocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers);

// Sends a data frame to the remote side. It is the responsibility of the
Expand Down Expand Up @@ -127,6 +129,7 @@ class NET_EXPORT WebSocketChannel {
const std::vector<std::string>& requested_protocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
const WebSocketStreamRequestCreationCallback& callback);

Expand Down Expand Up @@ -193,6 +196,7 @@ class NET_EXPORT WebSocketChannel {
const std::vector<std::string>& requested_protocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
const WebSocketStreamRequestCreationCallback& callback);

Expand Down
18 changes: 16 additions & 2 deletions net/websockets/websocket_channel_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -756,13 +756,15 @@ struct WebSocketStreamCreationCallbackArgumentSaver {
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
URLRequestContext* url_request_context,
const NetLogWithSource& net_log,
std::unique_ptr<WebSocketStream::ConnectDelegate> connect_delegate) {
this->socket_url = socket_url;
this->origin = origin;
this->site_for_cookies = site_for_cookies;
this->network_isolation_key = network_isolation_key;
this->url_request_context = url_request_context;
this->connect_delegate = std::move(connect_delegate);
return std::make_unique<MockWebSocketStreamRequest>();
Expand All @@ -771,6 +773,7 @@ struct WebSocketStreamCreationCallbackArgumentSaver {
GURL socket_url;
url::Origin origin;
GURL site_for_cookies;
net::NetworkIsolationKey network_isolation_key;
URLRequestContext* url_request_context;
std::unique_ptr<WebSocketStream::ConnectDelegate> connect_delegate;
};
Expand Down Expand Up @@ -808,7 +811,7 @@ class WebSocketChannelTest : public TestWithTaskEnvironment {
channel_->SendAddChannelRequestForTesting(
connect_data_.socket_url, connect_data_.requested_subprotocols,
connect_data_.origin, connect_data_.site_for_cookies,
HttpRequestHeaders(),
connect_data_.network_isolation_key, HttpRequestHeaders(),
base::Bind(&WebSocketStreamCreationCallbackArgumentSaver::Create,
base::Unretained(&connect_data_.argument_saver)));
}
Expand Down Expand Up @@ -843,7 +846,11 @@ class WebSocketChannelTest : public TestWithTaskEnvironment {
ConnectData()
: socket_url("ws://ws/"),
origin(url::Origin::Create(GURL("http://ws"))),
site_for_cookies("http://ws/") {}
site_for_cookies("http://ws/") {
url::Origin top_frame_origin = url::Origin::Create(GURL("http://ws-1"));
this->network_isolation_key =
net::NetworkIsolationKey(top_frame_origin, origin);
}

// URLRequestContext object.
URLRequestContext url_request_context;
Expand All @@ -856,6 +863,8 @@ class WebSocketChannelTest : public TestWithTaskEnvironment {
url::Origin origin;
// First party for cookies for the request.
GURL site_for_cookies;
// NetworkIsolationKey created from the origin of the top level frame.
net::NetworkIsolationKey network_isolation_key;

WebSocketStreamCreationCallbackArgumentSaver argument_saver;
};
Expand Down Expand Up @@ -979,6 +988,10 @@ TEST_F(WebSocketChannelTest, EverythingIsPassedToTheCreatorFunction) {
connect_data_.socket_url = GURL("ws://example.com/test");
connect_data_.origin = url::Origin::Create(GURL("http://example.com"));
connect_data_.site_for_cookies = GURL("http://example.com/");
url::Origin top_frame_origin =
url::Origin::Create(GURL("http://example-1.com"));
connect_data_.network_isolation_key =
net::NetworkIsolationKey(top_frame_origin, connect_data_.origin);
connect_data_.requested_subprotocols.push_back("Sinbad");

CreateChannelAndConnect();
Expand All @@ -991,6 +1004,7 @@ TEST_F(WebSocketChannelTest, EverythingIsPassedToTheCreatorFunction) {
EXPECT_EQ(connect_data_.socket_url, actual.socket_url);
EXPECT_EQ(connect_data_.origin.Serialize(), actual.origin.Serialize());
EXPECT_EQ(connect_data_.site_for_cookies, actual.site_for_cookies);
EXPECT_EQ(connect_data_.network_isolation_key, actual.network_isolation_key);
}

TEST_F(WebSocketChannelEventInterfaceTest, ConnectSuccessReported) {
Expand Down
4 changes: 3 additions & 1 deletion net/websockets/websocket_end_to_end_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -293,11 +293,13 @@ class WebSocketEndToEndTest : public TestWithTaskEnvironment {
}
url::Origin origin = url::Origin::Create(GURL("http://localhost"));
GURL site_for_cookies("http://localhost/");
net::NetworkIsolationKey network_isolation_key(origin, origin);
event_interface_ = new ConnectTestingEventInterface();
channel_ = std::make_unique<WebSocketChannel>(
base::WrapUnique(event_interface_), &context_);
channel_->SendAddChannelRequest(GURL(socket_url), sub_protocols_, origin,
site_for_cookies, HttpRequestHeaders());
site_for_cookies, network_isolation_key,
HttpRequestHeaders());
event_interface_->WaitForResponse();
return !event_interface_->failed();
}
Expand Down
12 changes: 8 additions & 4 deletions net/websockets/websocket_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
const URLRequestContext* context,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
std::unique_ptr<WebSocketStream::ConnectDelegate> connect_delegate,
std::unique_ptr<WebSocketStreamRequestAPI> api_delegate)
Expand All @@ -140,6 +141,7 @@ class WebSocketStreamRequestImpl : public WebSocketStreamRequestAPI {
url_request_->SetExtraRequestHeaders(headers);
url_request_->set_initiator(origin);
url_request_->set_site_for_cookies(site_for_cookies);
url_request_->set_network_isolation_key(network_isolation_key);

auto create_helper = std::make_unique<WebSocketHandshakeStreamCreateHelper>(
connect_delegate_.get(), requested_subprotocols, this);
Expand Down Expand Up @@ -470,14 +472,15 @@ std::unique_ptr<WebSocketStreamRequest> WebSocketStream::CreateAndConnectStream(
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
URLRequestContext* url_request_context,
const NetLogWithSource& net_log,
std::unique_ptr<ConnectDelegate> connect_delegate) {
auto request = std::make_unique<WebSocketStreamRequestImpl>(
socket_url, requested_subprotocols, url_request_context, origin,
site_for_cookies, additional_headers, std::move(connect_delegate),
nullptr);
site_for_cookies, network_isolation_key, additional_headers,
std::move(connect_delegate), nullptr);
request->Start(std::make_unique<base::OneShotTimer>());
return std::move(request);
}
Expand All @@ -488,6 +491,7 @@ WebSocketStream::CreateAndConnectStreamForTesting(
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
URLRequestContext* url_request_context,
const NetLogWithSource& net_log,
Expand All @@ -496,8 +500,8 @@ WebSocketStream::CreateAndConnectStreamForTesting(
std::unique_ptr<WebSocketStreamRequestAPI> api_delegate) {
auto request = std::make_unique<WebSocketStreamRequestImpl>(
socket_url, requested_subprotocols, url_request_context, origin,
site_for_cookies, additional_headers, std::move(connect_delegate),
std::move(api_delegate));
site_for_cookies, network_isolation_key, additional_headers,
std::move(connect_delegate), std::move(api_delegate));
request->Start(std::move(timer));
return std::move(request);
}
Expand Down
3 changes: 3 additions & 0 deletions net/websockets/websocket_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/time/time.h"
#include "net/base/completion_once_callback.h"
#include "net/base/net_export.h"
#include "net/base/network_isolation_key.h"
#include "net/websockets/websocket_event_interface.h"
#include "net/websockets/websocket_handshake_request_info.h"
#include "net/websockets/websocket_handshake_response_info.h"
Expand Down Expand Up @@ -154,6 +155,7 @@ class NET_EXPORT_PRIVATE WebSocketStream {
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
URLRequestContext* url_request_context,
const NetLogWithSource& net_log,
Expand All @@ -169,6 +171,7 @@ class NET_EXPORT_PRIVATE WebSocketStream {
const std::vector<std::string>& requested_subprotocols,
const url::Origin& origin,
const GURL& site_for_cookies,
const net::NetworkIsolationKey& network_isolation_key,
const HttpRequestHeaders& additional_headers,
URLRequestContext* url_request_context,
const NetLogWithSource& net_log,
Expand Down
Loading

0 comments on commit 4446d8c

Please sign in to comment.