HTTPS by default for Desktop #16521

arthuredelstein · 2023-01-04T08:04:49Z

For Desktop and Android:

Disabled, Standard, Strict modes
Controlled by global and per-site shields settings
Merge UI with HTTPS-Only Mode
Behind HttpsByDefault feature flag
Browser tests for each setting

Resolves brave/brave-browser#27141

Submitter Checklist:

Security/privacy review is here: https://github.com/brave/security/issues/1134
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

github-actions · 2023-01-04T08:05:01Z

⚠️ PR head is an unsigned commit
commit: b652d24
reason: unsigned
Please follow the handbook to configure commit signing
cc: @arthuredelstein

brave-builds · 2023-01-04T09:19:08Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-01-05T00:50:12Z

A Storybook has been deployed to preview UI for the latest push

browser/BUILD.gn

components/brave_component_updater/browser/https_upgrade_exceptions_service.h

components/brave_shields/browser/DEPS

components/brave_shields/browser/BUILD.gn

components/brave_shields/browser/brave_shields_util.cc

components/brave_component_updater/browser/https_upgrade_exceptions_service.cc

components/brave_component_updater/browser/https_upgrade_exceptions_service.h

chromium_src/components/prefs/pref_service.h

goodov

looks like most of comments are resolved, but I don't actually see the changes in PR. did you forget to push? 🤔

browser/BUILD.gn

browser/brave_shields/https_upgrade_browsertest.cc

arthuredelstein · 2023-01-31T01:47:54Z

@goodov wrote:

looks like most of comments are resolved, but I don't actually see the changes in PR. did you forget to push? 🤔

Sorry for the confusion -- I haven't pushed yet because I'm working on the remaining comments. Will push when they are ready.

brave-builds · 2023-02-03T23:32:08Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-04T19:54:34Z

A Storybook has been deployed to preview UI for the latest push

arthuredelstein · 2023-02-04T21:19:50Z

The story so far:

I pushed fixes for @goodov's comments in d03b6b3.

But the PR had bitrotted so much that I then rebased it, and squashed those changes. In addition, I decided to remove to the Android UI code for now, to simplify this PR (e7da857).

The browsertest is working in Android, except that there is a problem for all such web-page tests that needs to be fixed in brave/brave-browser#27837, so I have disabled the test in Android for now (4f98503).

As of now all checks are green and ready to merge pending approval from reviewers.

So the plan is to merge this Desktop-only PR first, and then get code/tests for Android working in a future PR.

SergeyZhukovsky

++ android tests side. Please create a separate issue for Android as you mentioned and post it in that PR, so it's linked.

arthuredelstein · 2023-02-05T07:20:40Z

++ android tests side. Please create a separate issue for Android as you mentioned and post it in that PR, so it's linked.

Thanks -- here's the Android issue: brave/brave-browser#28295.

browser/brave_shields/https_upgrade_browsertest.cc

components/https_upgrade_exceptions/browser/BUILD.gn

components/https_upgrade_exceptions/browser/https_upgrade_exceptions_service.cc

goodov · 2023-02-06T09:56:24Z

browser/brave_shields/https_upgrade_browsertest.cc

+
+  void SetUpOnMainThread() override {
+    g_brave_browser_process->https_upgrade_exceptions_service()
+        ->SetIsReadyForTesting();


I would also like to see a test that uses data from this service.

That is a good idea. @goodov, if it's all right with you, I'd like to write that test in a follow-up PR (soon). In the meantime @pes10k and I are hoping to merge this PR into Nightly as soon as possible. (Hoping this is OK as the feature is disabled by default and we will roll out gradually.)

I opened an issue: brave/brave-browser#28330

brave-builds · 2023-02-07T03:16:37Z

A Storybook has been deployed to preview UI for the latest push

fmarier

This looks really good to me. Well done @arthuredelstein !

The only big thing that needs to be checked before merging (maybe something we could easily add a test for?) is to ensure that HSTS sites never get downgraded since that would a major security issue.

browser/profiles/profile_util.cc

components/brave_shields/browser/brave_shields_util.cc

chromium_src/chrome/browser/ssl/https_only_mode_navigation_throttle.cc

browser/brave_shields/https_upgrade_browsertest.cc

goodov

lgtm with few comments

browser/tor/brave_tor_browsertest.cc

chromium_src/chrome/browser/ssl/https_only_mode_navigation_throttle.cc

chromium_src/net/base/features.h

brave-builds · 2023-02-07T10:18:56Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-02-07T17:22:02Z

A Storybook has been deployed to preview UI for the latest push

browser/brave_shields/https_upgrade_browsertest.cc

brave-builds · 2023-02-07T22:37:33Z

A Storybook has been deployed to preview UI for the latest push

- Disabled, Standard, Strict modes - Controlled by global and per-site shields settings - Merge UI with HTTPS-Only Mode - Behind HttpsByDefault feature flag - Browser tests for each setting

brave-builds · 2023-02-08T02:03:54Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2024-08-27T18:11:44Z

browser/brave_browser_process_impl.cc

@@ -200,6 +202,10 @@ void BraveBrowserProcessImpl::StartBraveServices() {
  https_everywhere_service()->Start();
  resource_component();

+  if (base::FeatureList::IsEnabled(net::features::kBraveHttpsByDefault)) {


this is definitely not the right place for this check. All this is doing is delaying the initialization if the feature is disabled. The check should go inside HttpsUpgradeExceptionsService

arthuredelstein requested review from AlexeyBarabash, samartnik, SergeyZhukovsky, a team and iefremov as code owners January 4, 2023 08:04

github-actions bot added CI/run-network-audit Run network-audit CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Jan 4, 2023

github-actions bot assigned arthuredelstein Jan 4, 2023

diracdeltas requested a review from fmarier January 4, 2023 23:21

goodov reviewed Jan 24, 2023

View reviewed changes

goodov reviewed Jan 30, 2023

View reviewed changes

browser/BUILD.gn Outdated Show resolved Hide resolved

browser/brave_shields/https_upgrade_browsertest.cc Outdated Show resolved Hide resolved

arthuredelstein marked this pull request as draft January 31, 2023 02:56

arthuredelstein marked this pull request as ready for review February 2, 2023 09:13

arthuredelstein force-pushed the issues/27141_c branch from d03b6b3 to e7da857 Compare February 3, 2023 21:54

SergeyZhukovsky approved these changes Feb 5, 2023

View reviewed changes

arthuredelstein changed the title ~~HTTPS by default~~ HTTPS by default for Desktop Feb 5, 2023

goodov reviewed Feb 6, 2023

View reviewed changes

fmarier reviewed Feb 7, 2023

View reviewed changes

goodov approved these changes Feb 7, 2023

View reviewed changes

fmarier approved these changes Feb 7, 2023

View reviewed changes

browser/brave_shields/https_upgrade_browsertest.cc Show resolved Hide resolved

HTTPS by default on Desktop

47e3c88

- Disabled, Standard, Strict modes - Controlled by global and per-site shields settings - Merge UI with HTTPS-Only Mode - Behind HttpsByDefault feature flag - Browser tests for each setting

arthuredelstein force-pushed the issues/27141_c branch from 18a4ccf to 47e3c88 Compare February 8, 2023 01:05

arthuredelstein enabled auto-merge (squash) February 8, 2023 01:06

mihaiplesa disabled auto-merge February 8, 2023 12:00

mihaiplesa merged commit 98b811e into master Feb 8, 2023

mihaiplesa deleted the issues/27141_c branch February 8, 2023 12:00

github-actions bot added this to the 1.50.x - Nightly milestone Feb 8, 2023

mkarolin mentioned this pull request Mar 7, 2023

Update HTTPS by Default to work with HttpsFirstModeV2 brave/brave-browser#28935

Closed

bridiver reviewed Aug 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTPS by default for Desktop #16521

HTTPS by default for Desktop #16521

arthuredelstein commented Jan 4, 2023 •

edited

Loading

github-actions bot commented Jan 4, 2023

brave-builds commented Jan 4, 2023

brave-builds commented Jan 5, 2023

goodov left a comment

arthuredelstein commented Jan 31, 2023

brave-builds commented Feb 3, 2023

brave-builds commented Feb 4, 2023

arthuredelstein commented Feb 4, 2023

SergeyZhukovsky left a comment

arthuredelstein commented Feb 5, 2023

goodov Feb 6, 2023

arthuredelstein Feb 7, 2023 •

edited

Loading

brave-builds commented Feb 7, 2023

fmarier left a comment •

edited

Loading

goodov left a comment

brave-builds commented Feb 7, 2023

brave-builds commented Feb 7, 2023

brave-builds commented Feb 7, 2023

brave-builds commented Feb 8, 2023

bridiver Aug 27, 2024

HTTPS by default for Desktop #16521

HTTPS by default for Desktop #16521

Conversation

arthuredelstein commented Jan 4, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Jan 4, 2023

brave-builds commented Jan 4, 2023

brave-builds commented Jan 5, 2023

goodov left a comment

Choose a reason for hiding this comment

arthuredelstein commented Jan 31, 2023

brave-builds commented Feb 3, 2023

brave-builds commented Feb 4, 2023

arthuredelstein commented Feb 4, 2023

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

arthuredelstein commented Feb 5, 2023

goodov Feb 6, 2023

Choose a reason for hiding this comment

arthuredelstein Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

brave-builds commented Feb 7, 2023

fmarier left a comment • edited Loading

Choose a reason for hiding this comment

goodov left a comment

Choose a reason for hiding this comment

brave-builds commented Feb 7, 2023

brave-builds commented Feb 7, 2023

brave-builds commented Feb 7, 2023

brave-builds commented Feb 8, 2023

bridiver Aug 27, 2024

Choose a reason for hiding this comment

arthuredelstein commented Jan 4, 2023 •

edited

Loading

arthuredelstein Feb 7, 2023 •

edited

Loading

fmarier left a comment •

edited

Loading