Skip to content

Commit

Permalink
Fix brave/brave-ios#8635: URLBar updates and CertValidation (brave/br…
Browse files Browse the repository at this point in the history
…ave-ios#8634)

Update URL bar on cancellation instead of URL change because URL change can update the certificate state.
Remove didCommit serverTrust validation as it works fine with URLBar Revamp.
Make the cert validation function async-await
Remove select as it crashes in debug builds
  • Loading branch information
Brandon-T committed Jan 9, 2024
1 parent 7a7f37b commit 4f87310
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import SafariServices
import LocalAuthentication
import BraveShared
import UniformTypeIdentifiers
import CertificateUtilities

extension WKNavigationAction {
/// Allow local requests only if the request is privileged.
Expand Down Expand Up @@ -143,6 +144,7 @@ extension BrowserViewController: WKNavigationDelegate {

@MainActor
public func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, preferences: WKWebpagePreferences) async -> (WKNavigationActionPolicy, WKWebpagePreferences) {

guard var requestURL = navigationAction.request.url else {
return (.cancel, preferences)
}
Expand Down Expand Up @@ -596,9 +598,7 @@ extension BrowserViewController: WKNavigationDelegate {
let host = challenge.protectionSpace.host
let port = challenge.protectionSpace.port

let result = BraveCertificateUtility.verifyTrust(serverTrust,
host: host,
port: port)
let result = await BraveCertificateUtils.verifyTrust(serverTrust, host: host, port: port)
let certificateChain = SecTrustCopyCertificateChain(serverTrust) as? [SecCertificate] ?? []

// Cert is valid and should be pinned
Expand Down Expand Up @@ -675,16 +675,6 @@ extension BrowserViewController: WKNavigationDelegate {
// Set the committed url which will also set tab.url
tab.committedURL = webView.url

DispatchQueue.main.async {
// Server Trust and URL is also updated in didCommit
// However, WebKit does NOT trigger the `serverTrust` observer when the URL changes, but the trust has not.
// So manually trigger it with the current trust.
self.observeValue(forKeyPath: KVOConstants.serverTrust.keyPath,
of: webView,
change: [.newKey: webView.serverTrust, .kindKey: 1],
context: nil)
}

// Need to evaluate Night mode script injection after url is set inside the Tab
tab.nightMode = Preferences.General.nightModeEnabled.value
tab.clearSolanaConnectedAccounts()
Expand Down Expand Up @@ -788,6 +778,10 @@ extension BrowserViewController: WKNavigationDelegate {
// original web page in the tab instead of replacing it with an error page.
var error = error as NSError
if error.domain == "WebKitErrorDomain" && error.code == 102 {
if let tab = tabManager[webView], tab === tabManager.selectedTab {
updateToolbarCurrentURL(tab.url?.displayURL)
updateWebViewPageZoom(tab: tab)
}
return
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1768,16 +1768,6 @@ public class BrowserViewController: UIViewController {
// Catch history pushState navigation, but ONLY for same origin navigation,
// for reasons above about URL spoofing risk.
navigateInTab(tab: tab)
} else {
updateURLBar()
// If navigation will start from NTP, tab display url will be nil until
// didCommit is called and it will cause url bar be empty in that period
// To fix this when tab display url is empty, webview url is used
if tab.url?.displayURL == nil {
if let url = webView.url, !url.isLocal, !InternalURL.isValid(url: url) {
updateToolbarCurrentURL(url.displayURL)
}
}
}

// Rewards reporting
Expand Down Expand Up @@ -1905,11 +1895,10 @@ public class BrowserViewController: UIViewController {
port = 80
}

Task.detached {
Task { @MainActor in
do {
let result = BraveCertificateUtility.verifyTrust(serverTrust,
host: host,
port: port)
let result = await BraveCertificateUtils.verifyTrust(serverTrust, host: host, port: port)

// Cert is valid!
if result == 0 {
tab.secureContentState = .secure
Expand All @@ -1925,9 +1914,7 @@ public class BrowserViewController: UIViewController {
tab.secureContentState = .invalidCert
}

Task { @MainActor in
self.updateURLBar()
}
self.updateURLBar()
}
case ._sampledPageTopColor:
updateStatusBarOverlayColor()
Expand Down Expand Up @@ -2837,15 +2824,6 @@ extension BrowserViewController: ToolbarUrlActionsDelegate {
let tabIsPrivate = TabType.of(tabManager.selectedTab).isPrivate
self.tabManager.addTabsForURLs(urls, zombie: false, isPrivate: tabIsPrivate)
}

#if DEBUG
public override func select(_ sender: Any?) {
if sender is URL {
assertionFailure("Wrong method called, use `select(url:)` or `select(_:action:)`")
}
super.select(sender)
}
#endif

func select(url: URL, isUserDefinedURLNavigation: Bool) {
select(url, action: .openInCurrentTab, isUserDefinedURLNavigation: isUserDefinedURLNavigation)
Expand Down
5 changes: 5 additions & 0 deletions Sources/CertificateUtilities/BraveCertificateUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import Foundation
import Shared
import BraveCore

public struct BraveCertificateUtils {
/// Formats a hex string
Expand Down Expand Up @@ -232,4 +233,8 @@ public extension BraveCertificateUtils {
}
}
}

static func verifyTrust(_ trust: SecTrust, host: String, port: Int) async -> Int {
return Int(BraveCertificateUtility.verifyTrust(trust, host: host, port: port))
}
}

0 comments on commit 4f87310

Please sign in to comment.