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

Commit

Permalink
When there is no internet or we're on an internal page, we need to sh…
Browse files Browse the repository at this point in the history
…ow our internal page as secure ONLY IF the page is not an invalid cert page.

When on an internal page, no serverTrust exists, and hasSecureContent is triggered, causing the page to show as insecure since SSL is missing. We now double check that it is actually insecure. SSL security takes priority over mixed-content.
  • Loading branch information
Brandon-T committed Jan 12, 2024
1 parent e4328b8 commit a5d3fdf
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,14 @@ extension BrowserViewController: WKNavigationDelegate {
// check if web view is loading a different origin than the one currently loaded
if let selectedTab = tabManager.selectedTab,
selectedTab.url?.origin != webView.url?.origin {
// reset secure content state to unknown until page can be evaluated
if let url = webView.url, !InternalURL.isValid(url: url) {
selectedTab.secureContentState = .unknown
updateToolbarSecureContentState(.unknown)
if let url = webView.url {
if !InternalURL.isValid(url: url) {
// reset secure content state to unknown until page can be evaluated
selectedTab.secureContentState = .unknown
updateToolbarSecureContentState(.unknown)
}
}

// new site has a different origin, hide wallet icon.
tabManager.selectedTab?.isWalletIconVisible = false
// new site, reset connected addresses
Expand Down Expand Up @@ -672,9 +675,19 @@ extension BrowserViewController: WKNavigationDelegate {

public func webView(_ webView: WKWebView, didCommit navigation: WKNavigation!) {
guard let tab = tab(for: webView) else { return }

// Set the committed url which will also set tab.url
tab.committedURL = webView.url

// 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.
// WebKit also does NOT trigger the `serverTrust` observer when the page is actually insecure (non-https).
// So manually trigger it with the current trust.
observeValue(forKeyPath: KVOConstants.serverTrust.keyPath,
of: webView,
change: [.newKey: webView.serverTrust as Any, .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
Original file line number Diff line number Diff line change
Expand Up @@ -1817,9 +1817,33 @@ public class BrowserViewController: UIViewController {
if let url = tab.webView?.url, url.isReaderModeURL {
break
}

tab.secureContentState = .mixedContent
}


if let url = tab.webView?.url,
InternalURL.isValid(url: url),
let internalUrl = InternalURL(url) {

if internalUrl.isErrorPage {
if ErrorPageHelper.certificateError(for: url) != 0 {
// Cert validation takes precedence over all other errors
tab.secureContentState = .invalidCert
} else if NetworkErrorPageHandler.isNetworkError(errorCode: ErrorPageHelper.errorCode(for: url)) {
// Network error takes precedence over missing cert
// Because we cannot determine if a cert is missing yet, if we cannot connect to the server
// Our network interstitial page shows
tab.secureContentState = .localhost
} else {
// Since it's not a cert error explicitly, and it's not a network error, and the cert is missing (no serverTrust),
// then we display .missingSSL
tab.secureContentState = .missingSSL
}
} else if url.isReaderModeURL || InternalURL.isValid(url: url) {
tab.secureContentState = .localhost
}
}

if tabManager.selectedTab === tab {
updateToolbarSecureContentState(tab.secureContentState)
}
Expand Down Expand Up @@ -1848,10 +1872,19 @@ public class BrowserViewController: UIViewController {
internalUrl.isErrorPage {

if ErrorPageHelper.certificateError(for: url) != 0 {
// Cert validation takes precedence over all other errors
tab.secureContentState = .invalidCert
} else if NetworkErrorPageHandler.isNetworkError(errorCode: ErrorPageHelper.errorCode(for: url)) {
// Network error takes precedence over missing cert
// Because we cannot determine if a cert is missing yet, if we cannot connect to the server
// Our network interstitial page shows
tab.secureContentState = .localhost
} else {
// Since it's not a cert error explicitly, and it's not a network error, and the cert is missing (no serverTrust),
// then we display .missingSSL
tab.secureContentState = .missingSSL
}

if tabManager.selectedTab === tab {
updateToolbarSecureContentState(tab.secureContentState)
}
Expand Down
18 changes: 13 additions & 5 deletions Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -135,18 +135,26 @@ class ErrorPageHelper {
}

extension ErrorPageHelper {
static func certificateError(for url: URL) -> Int {
static func errorCode(for url: URL) -> Int {
if InternalURL.isValid(url: url),
let internalUrl = InternalURL(url),
internalUrl.isErrorPage {

internalUrl.isErrorPage {
let query = url.getQuery()
guard let code = query["code"],
let errCode = Int(code)
let errCode = Int(code)
else {
return 0
}


return errCode
}
return 0
}

static func certificateError(for url: URL) -> Int {
let errCode = errorCode(for: url)
if errCode != 0 {
if let code = CFNetworkErrors(rawValue: Int32(errCode)),
CertificateErrorPageHandler.CFNetworkErrorsCertErrors.contains(code) {
return errCode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ import Shared
import BraveShared

class NetworkErrorPageHandler: InterstitialPageHandler {
static func isNetworkError(errorCode: Int) -> Bool {
if let code = CFNetworkErrors(rawValue: Int32(errorCode)) {
return code == .cfurlErrorNotConnectedToInternet
}

return errorCode == NSURLErrorNotConnectedToInternet
}

func canHandle(error: NSError) -> Bool {
// Handle CFNetwork Error
if error.domain == kCFErrorDomainCFNetwork as String,
Expand Down

0 comments on commit a5d3fdf

Please sign in to comment.