[iOS] Update secure content state update flow #22849

kylehickinson · 2024-03-28T22:03:10Z

This change moves all of the logic that used to happen in each KVO observation into one unified location so that it has all the data available each update. It also only updates the tab content state once per update which should fix possible races when updating the URL bar in other locations.

Resolves brave/brave-browser#36951

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Brandon's test list:

Mobile provisioning profiles.
Download Apple Wallet passes.
PDF loading (https and http).
External App Links.
Universal Links.
Default Browser External Links.
Default Browser App Links.
Back-Forward Navigation.
Back-Forward https to http.
Back-Forward http to https.
Back-Forward mixed-content.
Back-Forward Local Pages (NTP).
Back-Forward Internal Pages (Error pages)
Back-Forward all bad-ssl pages
Back-Forward Safe-Browsing pages
Back-Forward NTP to Http
Back-Forward NTP to Https
Back-Forward Reader-Mode icon tests
Back-Forward Youtube tests
Back-Forward Youtube Playlist tests
Session Restore.
Session Restore AppStore URLs.
Session Restore Universal Links.
Session Restore multiple tabs AppStore URLs.
Session Restore multiple tabs Universal Links.
Backgrounding AppStore URLs.
Backgrounding Universal Links.
Backgrounding External Launches.
Shortcuts
Shortcuts Session Restore
Shortcuts multiple tabs
Opening Tab-Switcher and doing ALL of the above
Opening 3-dot menu and loading a page from a shortcut
Opening 3-dot menu and loading a page from external app.
Load cancellation when page fails.
Throwing 503 from a server.
Throwing 400/403 from a server.
Basic-Authentication to secure and insecure server.
Loading local-host pages
Server Redirects from http to https
Server Redirects from https to http
Server Redirects from https to https
Server Redirects from http to http
Server Redirects from current domain to other domain
document.location.href = "https://google.ca"
window.open("about:blank")
window.open("google.com")
window.open("google.com", "_blank")
window.open("https://google.com")
window.open("https://google.com", "_blank")
Visit http://neverssl.com and try to spoof via:

    setInterval(() => {
      document.location.href = "https://google.com";
      console.log("Hello");
    }, 500);

github-actions · 2024-03-28T22:07:13Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "secure" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

fmarier · 2024-03-28T22:50:53Z

Adding @bridiver since he was involved in the discussion.

This change moves all of the logic that used to happen in each KVO observation into one unified location so that it has all the data available each update. It also only updates the tab content state once per update which should fix possible races when updating the URL bar in other locations.

hffvld · 2024-04-08T19:07:03Z

Brandon's test list:

    setInterval(() => {
      document.location.href = "https://google.com";
      console.log("Hello");
    }, 500);

hffvld · 2024-04-08T19:13:32Z

Verified on iPhone 14 using version(s):

Device/OS: iPhone 14 / iOS 17.5
Brave build: 1.66 (67)
BraveCore: 1.66.67 (123.0.6312.105)

ACTUAL RESULTS:
Verified that everything is working fine and no Cert issue is shown on https pages.

Uni-verse · 2024-04-10T23:22:59Z

Verified on iPhone X running iOS 16.7 using version 1.66.73

Ensured the following scenarios were showing the correct state in URL bar
- Backgrounding AppStore URLs.
- Backgrounding Universal Links.
- Backgrounding External Launches.
- Shortcuts
- Shortcuts Session Restore
- Shortcuts multiple tabs
- Opening Tab-Switcher and doing ALL of the above
- Opening 3-dot menu and loading a page from a shortcut
- Opening 3-dot menu and loading a page from external app.
- Load cancellation when page fails.
- Throwing 503 from a server.
- Throwing 400/403 from a server.
- Basic-Authentication to secure and insecure server.
- Loading local-host pages
- Server Redirects from http to https
- Server Redirects from https to http
- Server Redirects from https to https
- Server Redirects from http to http
- Server Redirects from current domain to other domain
- document.location.href = "https://google.ca/"
- window.open("about:blank")
- window.open("google.com")
- window.open("google.com", "_blank")
- window.open("https://google.com/")
- window.open("https://google.com/", "_blank")
- Visit http://neverssl.com/ and try to spoof via:
```
    setInterval(() => {
      document.location.href = "https://google.com";
      console.log("Hello");
    }, 500);
```

) Uplift of #22849 (squashed) to beta

) Uplift of #22849 (squashed) to release

Brandon-T · 2024-04-11T18:16:27Z

Condensed List:

Mobile provisioning profiles.
Download Apple Wallet passes.
Default Browser External Links (load a link from somewhere while the browser is both open already and closed).
Default Browser App Links (load an appstore link from somewhere while the browser is both open already and closed).
Back-Forward Navigation (switch between two different domains on the same tab via back/forward buttons).
Back-Forward https to http (switch from wikipedia.org to http://neverssl.com).
Back-Forward http to https (switch from http://neverssl.com to wikipedia.org).
Back-Forward all bad-ssl pages
Back-Forward NTP to Https (switch from ntp to wikipedia and back)
Basic-Authentication to secure and insecure server (account.bravesoftware.com and log-in).
Test no internet connection refreshing google.com both offline and online.

hffvld · 2024-04-11T22:55:26Z

Verified on iPhone 14 using version(s):

Device/OS: iPhone 14 / iOS 17.5
Brave build: 1.67 (2)
BraveCore: 1.67.2 (124.0.6367.29)

Filed follow-up issue #37533 and #37538

ACTUAL RESULTS:
Verified that everything is working fine and no Cert issue is shown on https pages.

kylehickinson added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels Mar 28, 2024

kylehickinson self-assigned this Mar 28, 2024

kylehickinson requested a review from a team as a code owner March 28, 2024 22:03

github-actions bot added the needs-security-review label Mar 28, 2024

github-actions bot assigned fmarier and thypon Mar 28, 2024

kylehickinson force-pushed the ios-secure-state-update branch from a5fd3d3 to 7ef9a4d Compare March 28, 2024 22:15

fmarier requested a review from bridiver March 28, 2024 22:50

iccub approved these changes Mar 29, 2024

View reviewed changes

kylehickinson force-pushed the ios-secure-state-update branch from 7ef9a4d to f798d49 Compare April 2, 2024 16:56

github-actions bot assigned bcaller Apr 2, 2024

Brandon-T approved these changes Apr 2, 2024

View reviewed changes

kylehickinson force-pushed the ios-secure-state-update branch from f798d49 to 1281d7e Compare April 2, 2024 20:56

fmarier removed the needs-security-review label Apr 2, 2024

fmarier unassigned fmarier and thypon Apr 2, 2024

soner-yuksel approved these changes Apr 3, 2024

View reviewed changes

kylehickinson added 3 commits April 3, 2024 14:38

Use committed URL to handle secure state evaluation

facf3b0

Guard displayed URL update from NTP behind selected tab check

8fd5a45

kylehickinson force-pushed the ios-secure-state-update branch from 1281d7e to 8fd5a45 Compare April 3, 2024 18:38

github-actions bot added the needs-security-review label Apr 3, 2024

github-actions bot assigned thypon Apr 3, 2024

kylehickinson merged commit c1c9146 into master Apr 3, 2024
19 checks passed

kylehickinson deleted the ios-secure-state-update branch April 3, 2024 19:53

github-actions bot added this to the 1.66.x - Nightly milestone Apr 3, 2024

brave-builds added a commit that referenced this pull request Apr 3, 2024

Uplift of #22849 (squashed) to beta

2de0270

brave-builds mentioned this pull request Apr 3, 2024

[iOS] Update secure content state update flow (uplift to 1.65.x) #22915

Merged

7 tasks

kylehickinson mentioned this pull request Apr 3, 2024

[iOS] Update secure content state update flow (uplift to 1.64.x) #22916

Merged

7 tasks

kylehickinson added a commit that referenced this pull request Apr 3, 2024

Uplift of #22849 (squashed) to release

6c76fe4

kjozwiak pushed a commit that referenced this pull request Apr 11, 2024

[iOS] Update secure content state update flow (uplift to 1.65.x) (#22915

385f812

) Uplift of #22849 (squashed) to beta

kjozwiak pushed a commit that referenced this pull request Apr 11, 2024

[iOS] Update secure content state update flow (uplift to 1.64.x) (#22916

121998a

) Uplift of #22849 (squashed) to release

kjozwiak mentioned this pull request Apr 11, 2024

[Follow-up] URL bar falsely showing insecure state in 1.62+ brave/brave-browser#36951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Update secure content state update flow #22849

[iOS] Update secure content state update flow #22849

kylehickinson commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024

fmarier commented Mar 28, 2024

hffvld commented Apr 8, 2024 •

edited by Uni-verse

Loading

hffvld commented Apr 8, 2024 •

edited

Loading

Uni-verse commented Apr 10, 2024

Brandon-T commented Apr 11, 2024 •

edited

Loading

hffvld commented Apr 11, 2024 •

edited

Loading

[iOS] Update secure content state update flow #22849

[iOS] Update secure content state update flow #22849

Conversation

kylehickinson commented Mar 28, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Mar 28, 2024

fmarier commented Mar 28, 2024

hffvld commented Apr 8, 2024 • edited by Uni-verse Loading

hffvld commented Apr 8, 2024 • edited Loading

Uni-verse commented Apr 10, 2024

Brandon-T commented Apr 11, 2024 • edited Loading

hffvld commented Apr 11, 2024 • edited Loading

Filed follow-up issue #37533 and #37538

kylehickinson commented Mar 28, 2024 •

edited

Loading

hffvld commented Apr 8, 2024 •

edited by Uni-verse

Loading

hffvld commented Apr 8, 2024 •

edited

Loading

Brandon-T commented Apr 11, 2024 •

edited

Loading

hffvld commented Apr 11, 2024 •

edited

Loading