-
Notifications
You must be signed in to change notification settings - Fork 868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 2351: Adding WebSocket proxy for Brave shields. #3245
Conversation
// User data key for ResourceContextData. | ||
const void* const kResourceContextUserDataKey = &kResourceContextUserDataKey; | ||
|
||
class ResourceContextData : public base::SupportsUserData::Data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two classes are common for BraveProxyingURLLoaderFactory
and BraveProxyingWebSocket
, so they are extracted to a separate brave_proxying_utils.h/cc
browser/net/brave_proxying_utils.h
Outdated
network::mojom::URLLoaderFactoryRequest request, | ||
network::mojom::URLLoaderFactoryPtrInfo target_factory); | ||
|
||
static void StartProxyingWebSocket( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new methods and a set for storing WebSocket proxies
request_.url = url; | ||
// TODO(iefremov): site_for_cookies is not enough, we should find a way | ||
// to initialize NetworkIsolationKey. | ||
request_.site_for_cookies = site_for_cookies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad news is that we don't have a good way to get a NetworkIsolationKey
here
forwarding_client_ = std::move(client); | ||
additional_headers_ = std::move(additional_headers); | ||
|
||
// If the header client will be used, we start the request immediately, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
garbage comment, need to delete it
// WebRequestProxyingURLLoaderFactory). | ||
bool should_collapse_initiator = false; | ||
ctx_ = std::make_shared<brave::BraveRequestInfo>(); | ||
brave::BraveRequestInfo::FillCTX(request_, process_id_, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the handcrafted request_
here, because it is not provided by WebSocket machinery. Referer is never used with WebSockets, and other non-initialized fields are basically not needed to create a context (except the request type - I've added a TODO to url_context.h
)
I didn't manage to complete tests yet, and only manually tested that the proxy actually calls |
browser/net/brave_proxying_utils.h
Outdated
* License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
#ifndef BRAVE_BROWSER_NET_BRAVE_PROXYING_UTILS_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should be named resource_context_data.h. I'll change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually in the next merge it will transform from ResourceContextData
to BrowserContextData
:D
if (reason == "HTTP Authentication failed; no valid credentials available" || | ||
reason == "Proxy authentication failed") { | ||
// This is needed to make some tests pass. | ||
// TODO(yhirano): Remove this hack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to track this upstream stuff somehow
// TODO(iefremov): We still need this for WebSockets, currently | ||
// |AddChannelRequest| provides only old-fashioned |site_for_cookies|. | ||
// (See |BraveProxyingWebSocket|). | ||
if (ctx->tab_origin.is_empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it makes more sense to set ctx->tab_origin
in BraveProxyingWebSocket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these are still used in other places from the ctx anyway
CI failure is intermittent unrelated test failures on macos only |
Issue 2351: Adding WebSocket proxy for Brave shields.
Issue 2351: Adding WebSocket proxy for Brave shields.
Verified this is in 0.71.x (milestone already set properly) - commit visible here https://github.com/brave/brave-core/commits/0.71.x?before=886f8520767f78e257397cfa47ef585b35bf1aca+140 Uplifting to 0.70.x 👍 |
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.