From 9c991d415d84a12a3f3f6122c85fb31e75f76ff6 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Thu, 31 Mar 2022 13:12:33 +0200
Subject: [PATCH 1/8] Fix handling external schemes in address bar

---
 DuckDuckGo/Browser Tab/Model/Tab.swift | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/DuckDuckGo/Browser Tab/Model/Tab.swift b/DuckDuckGo/Browser Tab/Model/Tab.swift
index b17df3d4ec..b210a90e06 100644
--- a/DuckDuckGo/Browser Tab/Model/Tab.swift	
+++ b/DuckDuckGo/Browser Tab/Model/Tab.swift	
@@ -849,9 +849,8 @@ extension Tab: WKNavigationDelegate {
             return .download(navigationAction, using: webView)
 
         } else if url.isExternalSchemeLink {
-            // ignore <iframe src="custom://url"> but allow via address bar
-            guard navigationAction.sourceFrame.isMainFrame,
-                  // ignore 2nd+ external scheme navigation not initiated by user
+            // allow external links via address bar, but ignore <iframe src="custom://url">
+            guard userEnteredUrl || navigationAction.sourceFrame.isMainFrame,
                   !self.externalSchemeOpenedPerPageLoad || navigationAction.isUserInitiated
             else { return .cancel }
 

From 56f1cb123b2ab00a41e7e374a634d1e55cd5cd4f Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Thu, 31 Mar 2022 14:31:16 +0200
Subject: [PATCH 2/8] Always allow user entered URLs

---
 DuckDuckGo/Browser Tab/Model/Tab.swift | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/DuckDuckGo/Browser Tab/Model/Tab.swift b/DuckDuckGo/Browser Tab/Model/Tab.swift
index b210a90e06..9611842997 100644
--- a/DuckDuckGo/Browser Tab/Model/Tab.swift	
+++ b/DuckDuckGo/Browser Tab/Model/Tab.swift	
@@ -849,14 +849,16 @@ extension Tab: WKNavigationDelegate {
             return .download(navigationAction, using: webView)
 
         } else if url.isExternalSchemeLink {
-            // allow external links via address bar, but ignore <iframe src="custom://url">
-            guard userEnteredUrl || navigationAction.sourceFrame.isMainFrame,
-                  !self.externalSchemeOpenedPerPageLoad || navigationAction.isUserInitiated
-            else { return .cancel }
-
-            self.externalSchemeOpenedPerPageLoad = true
+            // always allow user entered URLs
+            if !userEnteredUrl {
+                // allow external links via address bar, but ignore <iframe src="custom://url">
+                guard navigationAction.sourceFrame.isMainFrame,
+                      !self.externalSchemeOpenedPerPageLoad || navigationAction.isUserInitiated
+                else { return .cancel }
+
+                self.externalSchemeOpenedPerPageLoad = true
+            }
             self.delegate?.tab(self, requestedOpenExternalURL: url, forUserEnteredURL: userEnteredUrl)
-
             return .cancel
         }
 

From f2b65320613b76c7d65ff5faf026c83d77b84238 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Thu, 31 Mar 2022 14:40:10 +0200
Subject: [PATCH 3/8] Fix comments

---
 DuckDuckGo/Browser Tab/Model/Tab.swift | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/DuckDuckGo/Browser Tab/Model/Tab.swift b/DuckDuckGo/Browser Tab/Model/Tab.swift
index 9611842997..1131600a94 100644
--- a/DuckDuckGo/Browser Tab/Model/Tab.swift	
+++ b/DuckDuckGo/Browser Tab/Model/Tab.swift	
@@ -851,7 +851,8 @@ extension Tab: WKNavigationDelegate {
         } else if url.isExternalSchemeLink {
             // always allow user entered URLs
             if !userEnteredUrl {
-                // allow external links via address bar, but ignore <iframe src="custom://url">
+                // ignore <iframe src="custom://url">
+                // ignore 2nd+ external scheme navigation not initiated by user
                 guard navigationAction.sourceFrame.isMainFrame,
                       !self.externalSchemeOpenedPerPageLoad || navigationAction.isUserInitiated
                 else { return .cancel }

From f633951b9d50ba06dfb7a2bfcd7f9ff32b19dd50 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Thu, 31 Mar 2022 15:13:20 +0200
Subject: [PATCH 4/8] Attempt to fix a bug when external URL is searched for

Scenario:
1. Enter `mailto:test@example.com`, submit: confirmation popover appears
2. Without reloading the page, enter a search phrase in the address bar
3. The search is performed for `mailto:test@example.com`

This is caused by PermissionAuthorizationQuery deinitializer
which calls decisionHandler that ends up requesting a search
for the external URL. The query is deinitialized when the new search
phrase is submitted and the sequence of events is following:
1. make search URL for the search phrase
2. make search URL for the external URL from a deinitialized PermissionAuthorizationQuery
The latter overrides the search phrase and that's how the bug is
observed.

I updated PermissionModel.queryAuthorization to not call its decision
handler at all when decision from query is `.deinitialized`.
---
 DuckDuckGo/Permissions/Model/PermissionModel.swift | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/DuckDuckGo/Permissions/Model/PermissionModel.swift b/DuckDuckGo/Permissions/Model/PermissionModel.swift
index 352eb6cfdb..866bfd6932 100644
--- a/DuckDuckGo/Permissions/Model/PermissionModel.swift
+++ b/DuckDuckGo/Permissions/Model/PermissionModel.swift
@@ -151,7 +151,12 @@ final class PermissionModel {
             }
 
             defer {
-                decisionHandler(granted)
+                switch decision {
+                case .deinitialized:
+                    break
+                default:
+                    decisionHandler(granted)
+                }
             }
             guard let self = self,
                   let query = query, // otherwise handling decision on Query deallocation

From 38872baa40e80de71d7fd5c076b343859b0fb590 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Fri, 1 Apr 2022 09:32:35 +0200
Subject: [PATCH 5/8] Skip calling decisionHandler only for external scheme
 permissions

---
 DuckDuckGo/Permissions/Model/PermissionModel.swift | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/DuckDuckGo/Permissions/Model/PermissionModel.swift b/DuckDuckGo/Permissions/Model/PermissionModel.swift
index 866bfd6932..1e85298ba0 100644
--- a/DuckDuckGo/Permissions/Model/PermissionModel.swift
+++ b/DuckDuckGo/Permissions/Model/PermissionModel.swift
@@ -151,8 +151,8 @@ final class PermissionModel {
             }
 
             defer {
-                switch decision {
-                case .deinitialized:
+                switch (permissions.first, decision) {
+                case (.externalScheme, .deinitialized):
                     break
                 default:
                     decisionHandler(granted)

From bfee91becfbb59a97922dac1df456202a330f459 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Fri, 1 Apr 2022 09:50:13 +0200
Subject: [PATCH 6/8] Add a unit test for external scheme permissions query

Verify that when the query is reset (deinitialized)
then decision handler is not called at all.
---
 .../Permissions/PermissionModelTests.swift    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Unit Tests/Permissions/PermissionModelTests.swift b/Unit Tests/Permissions/PermissionModelTests.swift
index 5fb246467f..2ed2cf221b 100644
--- a/Unit Tests/Permissions/PermissionModelTests.swift	
+++ b/Unit Tests/Permissions/PermissionModelTests.swift	
@@ -301,6 +301,25 @@ final class PermissionModelTests: XCTestCase {
         XCTAssertEqual(model.permissions, [:])
     }
 
+    func testWhenExternalSchemePermissionQueryIsResetThenItDoesNotTriggerDecisionHandler() {
+        let c = model.$authorizationQuery.sink {
+            if $0 != nil {
+                self.model!.tabDidStartNavigation()
+            }
+        }
+
+        let e = expectation(description: "Permission granted")
+        e.isInverted = true
+        model.permissions([.externalScheme(scheme: "mailto")], requestedForDomain: "test@example.com") { _ in
+            e.fulfill()
+        }
+
+        withExtendedLifetime(c) {
+            waitForExpectations(timeout: 0.1)
+        }
+        XCTAssertEqual(model.permissions, [:])
+    }
+
     func testWhenAllowPermissionIsPersistedThenPermissionQueryIsGranted() {
         let e = expectation(description: "Permission granted")
         self.webView.urlValue = URL.duckDuckGo

From 01c3e5e8bb84151261765cbf5e780dd84f6369a1 Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Fri, 1 Apr 2022 11:16:18 +0200
Subject: [PATCH 7/8] Refactor
 PermissionModel.queryAuthorization(for:domain:url:decisionHandler:) to make
 SwiftLint happy

---
 .../Permissions/Model/PermissionModel.swift   | 58 +++++++++++--------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/DuckDuckGo/Permissions/Model/PermissionModel.swift b/DuckDuckGo/Permissions/Model/PermissionModel.swift
index 1e85298ba0..b5bc9b1b67 100644
--- a/DuckDuckGo/Permissions/Model/PermissionModel.swift
+++ b/DuckDuckGo/Permissions/Model/PermissionModel.swift
@@ -126,36 +126,14 @@ final class PermissionModel {
                                                  url: url,
                                                  permissions: permissions) { [weak self] decision, remember in
 
-            let query: PermissionAuthorizationQuery?
-            let granted: Bool
-            switch decision {
-            case .deinitialized:
-                query = nil
-                granted = false
-
-            case .denied(let completedQuery):
-                query = completedQuery
-                granted = false
-
-                for permission in permissions {
-                    self?.permissions[permission].denied()
-                }
-
-            case .granted(let completedQuery):
-                query = completedQuery
-                granted = true
-
-                for permission in permissions {
-                    self?.permissions[permission].granted()
-                }
-            }
+            let (query, isGranted) = self?.handleQueryDecision(decision, requestedPermissions: permissions) ?? (nil, false)
 
             defer {
                 switch (permissions.first, decision) {
                 case (.externalScheme, .deinitialized):
                     break
                 default:
-                    decisionHandler(granted)
+                    decisionHandler(isGranted)
                 }
             }
             guard let self = self,
@@ -168,7 +146,7 @@ final class PermissionModel {
 
             if remember == true {
                 for permission in permissions {
-                    self.permissionManager.setPermission(granted ? .allow : .deny, forDomain: domain, permissionType: permission)
+                    self.permissionManager.setPermission(isGranted ? .allow : .deny, forDomain: domain, permissionType: permission)
                 }
             }
         }
@@ -185,6 +163,36 @@ final class PermissionModel {
         authorizationQueries.append(query)
     }
 
+    private func handleQueryDecision(_ decision: PermissionAuthorizationQuery.Decision, requestedPermissions: [PermissionType])
+    -> (completedQuery: PermissionAuthorizationQuery?, isGranted: Bool) {
+
+        let query: PermissionAuthorizationQuery?
+        let isGranted: Bool
+        switch decision {
+        case .deinitialized:
+            query = nil
+            isGranted = false
+
+        case .denied(let completedQuery):
+            query = completedQuery
+            isGranted = false
+
+            for permission in requestedPermissions {
+                permissions[permission].denied()
+            }
+
+        case .granted(let completedQuery):
+            query = completedQuery
+            isGranted = true
+
+            for permission in requestedPermissions {
+                permissions[permission].granted()
+            }
+        }
+
+        return (query, isGranted)
+    }
+
     private func permissionManager(_: PermissionManagerProtocol,
                                    didChangePermanentDecisionFor permissionType: PermissionType,
                                    forDomain domain: String,

From e47ee400b1ab7cd8073f8b68bfdd8a2abdd95f9c Mon Sep 17 00:00:00 2001
From: Dominik Kapusta <dkapusta@duckduckgo.com>
Date: Fri, 1 Apr 2022 17:08:36 +0200
Subject: [PATCH 8/8] Clear Tab permissions on didEndDisplaying

---
 DuckDuckGo/Tab Bar/View/TabBarViewController.swift | 8 ++++++++
 DuckDuckGo/Tab Bar/View/TabBarViewItem.swift       | 5 ++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/DuckDuckGo/Tab Bar/View/TabBarViewController.swift b/DuckDuckGo/Tab Bar/View/TabBarViewController.swift
index a2c23dd207..8f5d6b52fe 100644
--- a/DuckDuckGo/Tab Bar/View/TabBarViewController.swift	
+++ b/DuckDuckGo/Tab Bar/View/TabBarViewController.swift	
@@ -549,6 +549,14 @@ extension TabBarViewController: NSCollectionViewDataSource {
         }
         return view
     }
+    
+    func collectionView(
+        _ collectionView: NSCollectionView,
+        didEndDisplaying item: NSCollectionViewItem,
+        forRepresentedObjectAt indexPath: IndexPath) {
+
+            (item as? TabBarViewItem)?.clear()
+    }
 }
 
 extension TabBarViewController: NSCollectionViewDelegate {
diff --git a/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift b/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift
index 43ef42c9c3..d8d446a0eb 100644
--- a/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift	
+++ b/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift	
@@ -220,6 +220,7 @@ final class TabBarViewItem: NSCollectionViewItem {
 
     func clear() {
         clearSubscriptions()
+        usedPermissions = Permissions()
         faviconImageView.image = nil
         titleTextField.stringValue = ""
     }
@@ -241,9 +242,7 @@ final class TabBarViewItem: NSCollectionViewItem {
     }
 
     private func clearSubscriptions() {
-        cancellables.forEach { (cancellable) in
-            cancellable.cancel()
-        }
+        cancellables.removeAll()
     }
 
     private func updateSubviews() {