Merge branch 'main' into dominik/xcode-16

* main: Validate VPN errors before re-throwing them (#1054) Allowing users to delete suggestions (#1027) Revert "Bump github.com/1024jp/gzipswift from 6.0.1 to 6.1.0" (#1055) Bump github.com/duckduckgo/privacy-dashboard from 5.3.0 to 7.1.1 (#1046) Bump github.com/duckduckgo/sync_crypto from 0.2.0 to 0.3.0 (#1048) Bump github.com/1024jp/gzipswift from 6.0.1 to 6.1.0 (#1050)
duckduckgo · Nov 3, 2024 · b2ed5cc · b2ed5cc
2 parents 8fb01fb + 45261df
commit b2ed5cc
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 14 deletions.
diff --git a/Package.resolved b/Package.resolved
@@ -50,8 +50,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/duckduckgo/privacy-dashboard",
       "state" : {
-        "revision" : "9de2b2aa317a48d3ee31116dc15b0feeb2cc9414",
-        "version" : "5.3.0"
+        "revision" : "53fd1a0f8d91fcf475d9220f810141007300dffd",
+        "version" : "7.1.1"
       }
     },
     {
@@ -77,8 +77,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/duckduckgo/sync_crypto",
       "state" : {
-        "revision" : "2ab6ab6f0f96b259c14c2de3fc948935fc16ac78",
-        "version" : "0.2.0"
+        "revision" : "0c8bf3c0e75591bc366407b9d7a73a9fcfc7736f",
+        "version" : "0.3.0"
       }
     },
     {

diff --git a/Package.swift b/Package.swift
@@ -49,9 +49,9 @@ let package = Package(
         .package(url: "https://github.com/duckduckgo/duckduckgo-autofill.git", exact: "15.1.0"),
         .package(url: "https://github.com/duckduckgo/GRDB.swift.git", exact: "2.4.0"),
         .package(url: "https://github.com/duckduckgo/TrackerRadarKit", exact: "3.0.0"),
-        .package(url: "https://github.com/duckduckgo/sync_crypto", exact: "0.2.0"),
+        .package(url: "https://github.com/duckduckgo/sync_crypto", exact: "0.3.0"),
         .package(url: "https://github.com/gumob/PunycodeSwift.git", exact: "3.0.0"),
-        .package(url: "https://github.com/duckduckgo/privacy-dashboard", exact: "5.3.0"),
+        .package(url: "https://github.com/duckduckgo/privacy-dashboard", exact: "7.1.1"),
         .package(url: "https://github.com/duckduckgo/content-scope-scripts", exact: "6.28.0"),
         .package(url: "https://github.com/httpswift/swifter.git", exact: "1.5.0"),
         .package(url: "https://github.com/duckduckgo/bloom_cpp.git", exact: "3.0.0"),

diff --git a/Sources/History/HistoryCoordinator.swift b/Sources/History/HistoryCoordinator.swift
@@ -44,6 +44,7 @@ public protocol HistoryCoordinating: AnyObject {
     func burnDomains(_ baseDomains: Set<String>, tld: TLD, completion: @escaping (Set<URL>) -> Void)
     func burnVisits(_ visits: [Visit], completion: @escaping () -> Void)
 
+    func removeUrlEntry(_ url: URL, completion: ((Error?) -> Void)?)
 }
 
 /// Coordinates access to History. Uses its own queue with high qos for all operations.
@@ -191,6 +192,20 @@ final public class HistoryCoordinator: HistoryCoordinating {
         }
     }
 
+    public enum EntryRemovalError: Error {
+        case notAvailable
+    }
+
+    public func removeUrlEntry(_ url: URL, completion: ((Error?) -> Void)? = nil) {
+        guard let historyDictionary = historyDictionary else { return }
+        guard let entry = historyDictionary[url] else {
+            completion?(EntryRemovalError.notAvailable)
+            return
+        }
+
+        removeEntries([entry], completionHandler: completion)
+    }
+
     var cleaningDate: Date { .monthAgo }
 
     @objc private func cleanOld() {

diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift
@@ -42,6 +42,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
         case rekeyAttempt(_ step: RekeyAttemptStep)
         case failureRecoveryAttempt(_ step: FailureRecoveryStep)
         case serverMigrationAttempt(_ step: ServerMigrationAttemptStep)
+        case malformedErrorDetected(_ error: Error)
     }
 
     public enum AttemptStep: CustomDebugStringConvertible {
@@ -710,7 +711,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
             Logger.networkProtection.log("🔴 Stopping VPN due to no auth token")
             await attemptShutdownDueToRevokedAccess()
 
-            throw error
+            // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down
+            if let wrappedError = wrapped(error: error) {
+                providerEvents.fire(.malformedErrorDetected(error))
+                throw wrappedError
+            } else {
+                throw error
+            }
         }
 
         do {
@@ -737,7 +744,14 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
             self.knownFailureStore.lastKnownFailure = KnownFailure(error)
 
             providerEvents.fire(.tunnelStartAttempt(.failure(error)))
-            throw error
+
+            // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down
+            if let wrappedError = wrapped(error: error) {
+                providerEvents.fire(.malformedErrorDetected(error))
+                throw wrappedError
+            } else {
+                throw error
+            }
         }
     }
 
@@ -1815,6 +1829,56 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
         snoozeTimingStore.reset()
     }
 
+    // MARK: - Error Validation
+
+    enum InvalidDiagnosticError: Error, CustomNSError {
+        case errorWithInvalidUnderlyingError(Error)
+
+        var errorCode: Int {
+            switch self {
+            case .errorWithInvalidUnderlyingError(let error):
+                return (error as NSError).code
+            }
+        }
+
+        var localizedDescription: String {
+            switch self {
+            case .errorWithInvalidUnderlyingError(let error):
+                return "Error '\(type(of: error))', message: \(error.localizedDescription)"
+            }
+        }
+
+        var errorUserInfo: [String: Any] {
+            switch self {
+            case .errorWithInvalidUnderlyingError(let error):
+                let newError = NSError(domain: (error as NSError).domain, code: (error as NSError).code)
+                return [NSUnderlyingErrorKey: newError]
+            }
+        }
+    }
+
+    /// Wraps an error instance in a new error type in cases where it is malformed; i.e., doesn't use an `NSError` instance for its underlying error, etc.
+    private func wrapped(error: Error) -> Error? {
+        if containsValidUnderlyingError(error) {
+            return nil
+        } else {
+            return InvalidDiagnosticError.errorWithInvalidUnderlyingError(error)
+        }
+    }
+
+    private func containsValidUnderlyingError(_ error: Error) -> Bool {
+        let nsError = error as NSError
+
+        if let underlyingError = nsError.userInfo[NSUnderlyingErrorKey] as? Error {
+            return containsValidUnderlyingError(underlyingError)
+        } else if nsError.userInfo[NSUnderlyingErrorKey] != nil {
+            // If `NSUnderlyingErrorKey` exists but is not an `Error`, return false
+            return false
+        }
+
+        return true
+    }
+
 }
 
 extension WireGuardAdapterError: LocalizedError, CustomDebugStringConvertible {

diff --git a/Sources/Suggestions/Suggestion.swift b/Sources/Suggestions/Suggestion.swift
@@ -28,7 +28,7 @@ public enum Suggestion: Equatable {
     case openTab(title: String, url: URL)
     case unknown(value: String)
 
-    var url: URL? {
+    public var url: URL? {
         switch self {
         case .website(url: let url),
              .historyEntry(title: _, url: let url, allowedInTopHits: _),
@@ -67,20 +67,26 @@ public enum Suggestion: Equatable {
         }
     }
 
-    var isOpenTab: Bool {
+    public var isOpenTab: Bool {
         if case .openTab = self {
             return true
         }
         return false
     }
 
-    var isBookmark: Bool {
+    public var isBookmark: Bool {
         if case .bookmark = self {
             return true
         }
         return false
     }
 
+    public var isHistoryEntry: Bool {
+        if case .historyEntry = self {
+            return true
+        }
+        return false
+    }
 }
 
 extension Suggestion {

diff --git a/Sources/Suggestions/SuggestionResult.swift b/Sources/Suggestions/SuggestionResult.swift
@@ -24,9 +24,9 @@ public struct SuggestionResult: Equatable {
         SuggestionResult(topHits: [], duckduckgoSuggestions: [], localSuggestions: [])
     }
 
-    private(set) public var topHits: [Suggestion]
-    private(set) public var duckduckgoSuggestions: [Suggestion]
-    private(set) public var localSuggestions: [Suggestion]
+    public let topHits: [Suggestion]
+    public let duckduckgoSuggestions: [Suggestion]
+    public let localSuggestions: [Suggestion]
 
     public init(topHits: [Suggestion],
                 duckduckgoSuggestions: [Suggestion],

diff --git a/Tests/HistoryTests/HistoryCoordinatorTests.swift b/Tests/HistoryTests/HistoryCoordinatorTests.swift
@@ -316,6 +316,43 @@ class HistoryCoordinatorTests: XCTestCase {
         return bookmarksDatabase
     }
 
+    func testWhenRemoveUrlEntryCalledWithExistingUrl_ThenEntryIsRemovedAndNoError() {
+        let (historyStoringMock, historyCoordinator) = HistoryCoordinator.aHistoryCoordinator
+
+        let url = URL(string: "https://duckduckgo.com")!
+        historyCoordinator.addVisit(of: url)
+
+        XCTAssertTrue(historyCoordinator.history!.contains(where: { $0.url == url }))
+
+        let removalExpectation = expectation(description: "Entry removed without error")
+        historyCoordinator.removeUrlEntry(url) { error in
+            XCTAssertNil(error, "Expected no error when removing an existing URL entry")
+            removalExpectation.fulfill()
+        }
+
+        waitForExpectations(timeout: 1.0)
+
+        XCTAssertFalse(historyCoordinator.history!.contains(where: { $0.url == url }))
+        XCTAssertTrue(historyStoringMock.removeEntriesCalled, "Expected removeEntries to be called")
+        XCTAssertEqual(historyStoringMock.removeEntriesArray.count, 1)
+        XCTAssertEqual(historyStoringMock.removeEntriesArray.first?.url, url)
+    }
+
+    func testWhenRemoveUrlEntryCalledWithNonExistingUrl_ThenEntryRemovalFailsWithNotAvailableError() {
+        let (_, historyCoordinator) = HistoryCoordinator.aHistoryCoordinator
+
+        let nonExistentUrl = URL(string: "https://nonexistent.com")!
+
+        let removalExpectation = expectation(description: "Entry removal fails with notAvailable error")
+        historyCoordinator.removeUrlEntry(nonExistentUrl) { error in
+            XCTAssertNotNil(error, "Expected an error when removing a non-existent URL entry")
+            XCTAssertEqual(error as? HistoryCoordinator.EntryRemovalError, .notAvailable, "Expected notAvailable error")
+            removalExpectation.fulfill()
+        }
+
+        waitForExpectations(timeout: 1.0)
+    }
+
 }
 
 fileprivate extension HistoryCoordinator {