Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #8635: URLBar updates and CertValidation #8634

Merged
merged 1 commit into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
}
Copy link
Member

Choose a reason for hiding this comment

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

@Brandon-T what was the issue with this logic? where it was removed in brave/brave-core#22534

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))
}
}
soner-yuksel marked this conversation as resolved.
Show resolved Hide resolved