Restore the DNS over HTTPS work-around on Windows #15989
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
3 times, most recently
from
505ccde
to
6336008
Compare
spylogsster
changed the title
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
from
6336008
to
91ad8e1
Compare
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
19 times, most recently
from
f3efb3d
to
fe1db43
Compare
bridiver
reviewed
Dec 12, 2022
| @@ -22,18 +22,16 @@ | |||
| #include "components/prefs/pref_registry_simple.h" | |||
| #include "components/prefs/pref_service.h" | |||
| #include "net/base/network_change_notifier.h" | |||
| #include "third_party/abseil-cpp/absl/types/optional.h" | |||
There was a problem hiding this comment.
why was this header added?
There was a problem hiding this comment.
old code used it, removed in c2b3991
bridiver
reviewed
Dec 12, 2022
| @@ -9,6 +9,7 @@ | |||
| #include <string> | |||
|
|
|||
| #include "build/build_config.h" | |||
| #include "third_party/abseil-cpp/absl/types/optional.h" | |||
There was a problem hiding this comment.
same as above, but also there's not need to duplicate the same header in .h and .cc
There was a problem hiding this comment.
same as above, removed in c2b3991
bridiver
reviewed
Dec 12, 2022
components/brave_vpn/features.h
Outdated
| @@ -12,6 +12,7 @@ namespace brave_vpn { | |||
| namespace features { | |||
|
|
|||
| BASE_DECLARE_FEATURE(kBraveVPN); | |||
| BASE_DECLARE_FEATURE(kBraveVPNDnsProtection); | |||
There was a problem hiding this comment.
added in 08d26ca
bridiver
reviewed
Dec 12, 2022
| @@ -3,7 +3,41 @@ | |||
| * 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/. */ | |||
|
|
|||
| #include "brave/components/brave_vpn/buildflags/buildflags.h" | |||
| #include "build/build_config.h" | |||
| #include "build/buildflag.h" | |||
There was a problem hiding this comment.
obsolete, removed in 08d26ca
bridiver
reviewed
Dec 12, 2022
| bool skip_notification_dialog_for_testing_ = false; | ||
| raw_ptr<PrefService> local_state_; | ||
| raw_ptr<PrefService> profile_prefs_; | ||
| raw_ptr<PrefService> pref_service_for_testing_; |
There was a problem hiding this comment.
this is unnecessary, you can control the pref service by either passing it in directly or with TestingProfile::Builder::SetPrefService, but it also doesn't appear to be used anywhere. It's also ambiguous in terms of profile vs local
There was a problem hiding this comment.
obsolete, removed in 08d26ca
bridiver
reviewed
Dec 12, 2022
| void BraveSecureDnsHandler::OnJavascriptAllowed() { | ||
| SecureDnsHandler::OnJavascriptAllowed(); | ||
| pref_registrar_.Init(g_browser_process->local_state()); | ||
| #if BUILDFLAG(IS_WIN) |
There was a problem hiding this comment.
this isn't needed anymore because the entire file is only included for windows https://github.com/brave/brave-core/pull/15989/files#diff-7ce4cface9f839816f0fc02bad0e04aa80284b048ef0d8f0876547e6e37e38b6R299
There was a problem hiding this comment.
in a follow-up we should rename these files with _win
There was a problem hiding this comment.
created brave/brave-browser#27308
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
from
8086276
to
f460f6b
Compare
bridiver
reviewed
Dec 12, 2022
|
|
||
| #include "base/feature_list.h" | ||
| #include "brave/components/brave_vpn/features.h" | ||
| #include "chrome/browser/browser_process.h" |
There was a problem hiding this comment.
it is used by pref registrar
bridiver
reviewed
Dec 12, 2022
| @@ -24,11 +24,9 @@ void MigrateVPNSettings(PrefService* profile_prefs, PrefService* local_prefs); | |||
| void RegisterLocalStatePrefs(PrefRegistrySimple* registry); | |||
| void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); | |||
| void RegisterAndroidProfilePrefs(PrefRegistrySimple* registry); | |||
|
|
|||
| #if !BUILDFLAG(IS_ANDROID) | |||
There was a problem hiding this comment.
if there are no other changes to this file then please set it back to the original state to reduce the diff
There was a problem hiding this comment.
done in beb1228
bridiver
reviewed
Dec 12, 2022
| "brave.brave_vpn.show_dns_policy_warning_dialog"; | ||
| #endif // BUILDFLAG(IS_WIN) | ||
| constexpr char kBraveVPNShowNotificationDialog[] = |
There was a problem hiding this comment.
this one also belongs in IS_WIN
There was a problem hiding this comment.
done in beb1228
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
5 times, most recently
from
beb1228
to
714ee86
Compare
bridiver
approved these changes
Dec 13, 2022
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
from
714ee86
to
808fd9a
Compare
Re-introduces logic from #13434 This reverts commit b9f88f6. Fixes brave/brave-browser#26787
spylogsster
force-pushed
the
bsc-restore-windows-doh-workaround
branch
from
808fd9a
to
85c07e3
Compare
spylogsster
added a commit
that referenced
this pull request
Dec 13, 2022
Re-introduces logic from #13434 This reverts commit b9f88f6. Fixes brave/brave-browser#26787 Co-authored-by: Brian Clifton <brian@clifton.me>
7 tasks
Verification
|
| Brave | 1.48.52 Chromium: 108.0.5359.99 (Official Build) nightly (64-bit) |
|---|---|
| Revision | 410951fc34bb4b2cbf182231f9f779efaafaf682-refs/branch-heads/5359_71@{#9} |
| OS | Windows 10 Version 22H2 (Build 19045.2364) |
"Happiest of paths" - PASSED
- installed
1.48.52 - launched Brave
- opened
brave://flags - set
brave://flags/#brave-vpntoEnabled - clicked
Relaunch - loaded
account.bravesoftware.com - entered basic-auth credentials
- entered
dec1405@mailinator.comand clickedGet login link - clicked
Log in to Bravefrom the resulting email - clicked on
Browse plans - scrolled down to
Brave VPN Subscriptionand clickedBuy now - filled out Stripe and clicked
Subscribe - connected to
BraveVPN - confirmed I got the
DoHwarning modal - clicked
OK - loaded
https://browserleaks.com/dns - confirmed my local ISP (
AT&T) DNS resolvers aren't shown - confirmed
Cloudflareresolvers are shown
| step 14 | steps 16-18 |
|---|---|
 |
 |
Confirmed only Cloudflare DNS resolvers were shown, not my local A&T (ISP) ones
Ensure Do not warn me about this anymore works - PASSED
Pre-requisite: ran Happiest of paths test
- continued from
Happiest of pathstest - clicked on
Do not warn me about this anymore - disconnected from
BraveVPNvia the macOSNetworkpanel - reconnected to
BraveVPNvia the browser'sVPNbutton toggle - confirm the modal doesn't show again
Confirmed no more DoH warning modal(s)
| step 2 | step 3 | steps 4-5 |
|---|---|---|
 |
 |
 |
Confirmed I was not presented with the Do not warm me about this anymore DoH leak-warning dialog
User enables DoH and connects to VPN - PASSED
- fresh profile
- open
brave://settings/securityand underUse secure DNSpickWith Cloudflare (1.1.1.1) - purchased, enabled, and connected to
BraveVPN(see steps above) - confirmed no
DoHwarning modal - reloaded
brave://settings/security - confirmed my
DoHsettings were not overwritten - changed to
Cloudflare - changed settings for
Use secure DNStoWith your current service provider - connected to
BraveVPN - confirmed I got the
DoHwarning modal
Confirmed whenever I connected to BraveVPN, Use secure DNS was automatically collapsed and enabled, and I got the DoH DNS-leak warning dialog
| step 3 | step 4 | steps 5-6 | step 9-10 |
|---|---|---|---|
 |
 |
 |
 |
Group policy - PASSED
When DnsOverHttpsMode is overriden - PASSED
- opened
regedit.exe - elevated user permissions
- navigated to
HKEY_LOCAL_MACHINE\SOFTWARE\Policies\ - context-clicked the
Bravebranch in the registry tree, and choseNew->string - entered
DnsOverHttpsMode - set its value to
off - opened
brave://settings/securityand confirmed theUse secure DNSsetting should not be editable (shows a "house" icon - managed via policy) - ran through
Happiest of Pathstest - confirmed the
Secure DNS is disabled by your...warning dialog text - clicked on
OKto dismiss
Confirmed while connected to BraveVPN with the above policy, I saw both the DoH warning dialog, as well as the tooltip + home icon, in the Settings panel
| steps 1-6 | step 7 | step 9 | step 10 |
|---|---|---|---|
 |
 |
 |
 |
When DnsOverHttpsMode and DnsOverHttpsTemplates are overriden - PASSED
- set the value of
DnsOverHttpsModetosecure(fromoff) - create a
stringkey ofDnsOverHttpsTemplatesand set its value tohttps://chrome.cloudflare-dns.com/dns-query - open
brave://settings/securityand confirmed theUse secure DNSsetting is disabled (with the house icon) - run the
Happiest of pathstest - confirm no
DoHwarning dialogs when successfully reconnecting toBraveVPN
| step 2 | step 3 | step 5 |
|---|---|---|
 |
 |
 |
Security and Privacy - PASSED
DNS-over-HTTPs (DOH) - PASSED
- connect to
BraveVPN - click
OKon the DoH DNS leak warning dialog - load
browserleaks.com/dns - confirm you see only
CloudflareDNS-server IPs
| DoH warning dialog | browserleaks.com/dns |
|---|---|
 |
 |
DNS leak (ISP) - PASSED
- connect to
BraveVPN - load
https://browserleaks.com/dns - confirm local ISP (AT&T) DNS resolvers aren't shown (they should be from the connected region, Cloudflare)
- disconnect from
BraveVPN - reload
https://browserleaks.com/dns - confirm local ISP (AT&T) DNS resolvers are shown
| step 3 | step 4 | step 5-6 |
|---|---|---|
 |
 |
 |
Tor - PASSED
- connect to
BraveVPN - open a
New Private window with Tor - load
check.torproject.org - ensure you see
Congratulations. This browser is configured to use Tor.
Torrent (via WebTorrents support) - PASSED
- disconnect from a working
BraveVPNsetup - load
https://ipleak.net - click on
ActivateunderTorrent Address detection - click on
this Magnet Link(it'll open in a new window) - reload the URL
- click on
Start Torrent - return to the
https://ipleak.nettab - confirm that you see your public (local ISP) IP address
- connect to
BraveVPN - click
OKon the DoH leak warning dialog shift+reloadthe page (to clear cache)- repeat steps 3-7
- confirm you now see the appropriate VPN IP address (for the connected region)
- switch to another region (for bonus points!)
Actual torrenting looks to be blocked on BraveVPN, with the recent change to DoH mitigation:
beta - works |
nightly - fails |
|---|---|
 |
 |
Will investigate the above further, but it shouldn't block the DoH-mitigation code here from moving forward 👍
| step 4 | step | step | step | step |
|---|---|---|---|---|
 |
 |
 |
 |
 |
WebRTC - PASSED
- connect to
BraveVPN - open
brave://settings/privacy - load
https://browserleaks.com/webrtcin a new tab - iterate through the values for
WebRTC IP handling policy - note the absence/presence and value of both local + public IP addresses, as reflected by their respective setting
VPN connected
Default |
Default public and private interfaces |
Default public interface only |
Disable non-proxied UDP |
|---|---|---|---|
 |
 |
 |
 |
Disconnected from VPN
Default |
Default public and private interfaces |
Default public interface only |
Disable non-proxied UDP |
|---|---|---|---|
 |
 |
 |
 |
kjozwiak
added a commit
that referenced
this pull request
Dec 15, 2022
(Uplift 1.47.x) Restore the DNS over HTTPS work-around on Windows (#15989)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-introduces logic from #13434 This reverts commit b9f88f6.
Fixes brave/brave-browser#26787
Example of notification message for warning about overriding the setting
DnsOverHttpsMode is off by policy we show warning message because user has DNS leak. Pref to skip warning message is saved to the profile prefs
without policy we override user's settings to Cloudflare DoH automatically and block settings form changing:
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
For official test plan, see brave/brave-browser#26787