From 4f8731012a5f2c54e4b2a5cd559b5a0bdf32df5b Mon Sep 17 00:00:00 2001 From: Brandon-T Date: Tue, 9 Jan 2024 16:41:32 -0500 Subject: [PATCH] Fix brave/brave-ios#8635: URLBar updates and CertValidation (brave/brave-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 --- .../BVC+WKNavigationDelegate.swift | 20 +++++-------- .../BrowserViewController.swift | 30 +++---------------- .../BraveCertificateUtils.swift | 5 ++++ 3 files changed, 16 insertions(+), 39 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift index 2c3377727d49..e31ce286f382 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift @@ -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. @@ -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) } @@ -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 @@ -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() @@ -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 } diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift index 375638c6e9ce..89f1f7ee5ec8 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift @@ -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 @@ -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 @@ -1925,9 +1914,7 @@ public class BrowserViewController: UIViewController { tab.secureContentState = .invalidCert } - Task { @MainActor in - self.updateURLBar() - } + self.updateURLBar() } case ._sampledPageTopColor: updateStatusBarOverlayColor() @@ -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) diff --git a/Sources/CertificateUtilities/BraveCertificateUtils.swift b/Sources/CertificateUtilities/BraveCertificateUtils.swift index 0ad64e00c65c..3a21e8103da3 100644 --- a/Sources/CertificateUtilities/BraveCertificateUtils.swift +++ b/Sources/CertificateUtilities/BraveCertificateUtils.swift @@ -5,6 +5,7 @@ import Foundation import Shared +import BraveCore public struct BraveCertificateUtils { /// Formats a hex string @@ -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)) + } }