Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[iOS] Wrap HTTPS-by-default logic around feature flags #25263

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

kylehickinson
Copy link
Collaborator

@kylehickinson kylehickinson commented Aug 21, 2024

Resolves brave/brave-browser#40634

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 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:

@kylehickinson kylehickinson self-assigned this Aug 21, 2024
@kylehickinson kylehickinson requested review from a team as code owners August 21, 2024 19:16
@github-actions github-actions bot added CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) labels Aug 21, 2024
Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src ++

@ShivanKaul
Copy link
Collaborator

ShivanKaul commented Aug 22, 2024

Just an update here that I'm chatting with @diracdeltas and @fmarier about whether we want this, let's not merge for now.
Update: fine to merge, brave/brave-browser#40634 captures the changes we need.

@kylehickinson kylehickinson force-pushed the ios-disable-httpsbydefault branch from e988ae8 to aba394c Compare August 23, 2024 14:56
@kylehickinson kylehickinson changed the title [iOS] Disable HTTPS-by-Default by default and wrap feature by the flag [iOS] Wrap HTTPS-by-default logic around feature flags Aug 23, 2024
@kylehickinson kylehickinson force-pushed the ios-disable-httpsbydefault branch from aba394c to c98f586 Compare August 23, 2024 17:04
@kylehickinson kylehickinson merged commit 642d23a into master Aug 23, 2024
22 checks passed
@kylehickinson kylehickinson deleted the ios-disable-httpsbydefault branch August 23, 2024 18:59
@github-actions github-actions bot added this to the 1.71.x - Nightly milestone Aug 23, 2024
brave-builds added a commit that referenced this pull request Aug 23, 2024
brave-builds added a commit that referenced this pull request Aug 23, 2024
@kjozwiak
Copy link
Member

kjozwiak commented Aug 26, 2024

Verification PASSED on iPhone 11 running iOS 17.5.1 via the following build(s):

Brave | 1.71.29 Chromium: 128.0.6613.85 (Official Build) nightly (64-bit) 
--- | ----
Revision | c532a6657223...
OS | iOS

Test Case #1 - brave://flags#https-only-mode enabled (default mode)

  • ensured that brave://flags#https-only-mode is set as enabled by default on a clean/new install
  • ensured Upgrade Connections to HTTPS is set as Standard as the default
Example Example
IMG_0468 IMG_0471

While both brave://flags#https-by-default & brave://flags#https-only-mode are enabled, went through the STR/Cases outlined via brave/brave-browser#36408 (comment) as per the following:

Test interstitial

Example Example Example Example Example
IMG_0472 IMG_0473 IMG_0474 IMG_0475 IMG_0476

Test upgrading

Example Example Example Example Example Example
IMG_0479 IMG_0480 IMG_0481 IMG_0482 IMG_0483 IMG_0484

Test Case #2 - brave://flags#https-only-mode disabled

  • disabled brave://flags#https-only-mode via brave://flags after installing 1.71.29 Chromium: 128.0.6613.85
  • restart the browser once the brave://flag has been changed
  • once disabled, ensured that Upgrade Connections to HTTPS is set as a boolean toggle (enabled/disabled)
  • visited http://http.badssl.com and ensured that a interstitial page re: HTTP upgrade is NOT being displayed
Example Example Example
IMG_0486 IMG_0487 IMG_0488

Test Case #3 - brave://flags#https-by-default disabled

  • disabled brave://flags#https-by-default via brave://flags after installing 1.71.29 Chromium: 128.0.6613.85
  • restart the browser once the brave://flag has been changed
  • ensured that Upgrade Connections to HTTPS is set as a boolean toggle (enabled/disabled)
    • brave://flags#https-only-mode is also being disabled when brave://flags#https-by-default has been disabled
  • visited http://http.badssl.com and ensured that a interstitial page re: HTTP upgrade is NOT being displayed
  • ensured that http://brave.com --> https://brave.com
  • ensured that http://google.com --> https://google.com
  • ensured that http://facebook.com --> https://m.facebook.com
  • ensured that http://reddit.com --> https://reddit.com
Example Example Example
IMG_0489 IMG_0490 IMG_0491

Test Case #4 - brave://flags#https-only-mode being disabled but user has Strict selected

  • installed 1.71.29 Chromium: 128.0.6613.85
  • set Upgrade Connections to HTTPS as Strict via Settings
  • disabled brave://flags#https-only-mode via brave://flags and restarted the browser
  • ensured that the Upgrade Connections to HTTPS setting is now set as a boolean without Strict mode
  • visited http://http.badssl.com and ensured that the interstitial page re: HTTP upgrade is NOT being displayed
  • enabled brave://flags#https-only-mode via brave://flags and restarted the browser
  • ensured that Upgrade Connections to HTTPS is now a drop down and Strict is being selected (users previous choice)
  • visited http://http.badssl.com and ensured that the interstitial page is being used/displayed
RPReplay_Final1724640350.MP4

kjozwiak pushed a commit that referenced this pull request Aug 26, 2024
kjozwiak pushed a commit that referenced this pull request Aug 26, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPS By Default feature doesn't have functional feature flags
7 participants