From 884a5eac964eeeb6d38780a6b90feaf5a5b3cfcf Mon Sep 17 00:00:00 2001 From: Dax Mobile <44842493+daxmobile@users.noreply.github.com> Date: Thu, 31 Oct 2024 21:24:18 +1100 Subject: [PATCH 01/14] Update autofill to 15.1.0 (#1044) Task/Issue URL: https://app.asana.com/0/1208660715919854/1208660715919854 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/15.1.0 ## Description Updates Autofill to version [15.1.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/15.1.0). --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 62c0cc986..32abe9c26 100644 --- a/Package.resolved +++ b/Package.resolved @@ -23,8 +23,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/duckduckgo-autofill.git", "state" : { - "revision" : "945ac09a0189dc6736db617867fde193ea984b20", - "version" : "15.0.0" + "revision" : "c992041d16ec10d790e6204dce9abf9966d1363c", + "version" : "15.1.0" } }, { diff --git a/Package.swift b/Package.swift index 532d22657..eee36a46b 100644 --- a/Package.swift +++ b/Package.swift @@ -46,7 +46,7 @@ let package = Package( .library(name: "Onboarding", targets: ["Onboarding"]) ], dependencies: [ - .package(url: "https://github.com/duckduckgo/duckduckgo-autofill.git", exact: "15.0.0"), + .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"), From 5159f2515a839e1fe51bb5675df6c66419f11233 Mon Sep 17 00:00:00 2001 From: Fernando Bunn Date: Thu, 31 Oct 2024 11:01:30 +0000 Subject: [PATCH 02/14] Dependabot for BSK (#1045) Task/Issue URL: https://app.asana.com/0/1204167627774280/1208651137074643/f iOS PR: N/A macOS PR: N/A What kind of version bump will this require?: N/A **Description**: Add dependebot for Swift packages in BSK --- .github/dependabot.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 39efafc05..3cd030c3d 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -7,3 +7,7 @@ updates: directory: "/" # Location of package manifests schedule: interval: "daily" + - package-ecosystem: "swift" # For Swift package updates + directory: "/" # Location of Package.swift file + schedule: + interval: "daily" \ No newline at end of file From de77673bd4fa7b8012c8d1f16cbc73b064539a57 Mon Sep 17 00:00:00 2001 From: Alessandro Boron Date: Thu, 31 Oct 2024 17:11:13 +0100 Subject: [PATCH 03/14] Add to Dock - Add extra custom view to Contextual Dialog Content (#1043) Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1206329551987282/1208577512136709 iOS PR: https://github.com/duckduckgo/iOS/pull/3505 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3479 What kind of version bump will this require?: Major **Description**: Add the Add to Dock Promo view to the final dialog of the onboarding flow. **Steps to test this PR**: 1. Ensure BSK "Intro Dialog - title, text, image and button" Preview works as expected 2. For macOS see macOS PR. 3. For iOS see iOS PR. **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) --- .../ContextualDaxDialogContent.swift | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/Sources/Onboarding/ContextualDaxDialogs/ContextualDaxDialogContent.swift b/Sources/Onboarding/ContextualDaxDialogs/ContextualDaxDialogContent.swift index e4bd1d80a..a0a93af35 100644 --- a/Sources/Onboarding/ContextualDaxDialogs/ContextualDaxDialogContent.swift +++ b/Sources/Onboarding/ContextualDaxDialogs/ContextualDaxDialogContent.swift @@ -40,7 +40,7 @@ public struct ContextualDaxDialogContent: View { public let message: NSAttributedString let list: [ContextualOnboardingListItem] let listAction: ((_ item: ContextualOnboardingListItem) -> Void)? - let imageName: String? + let customView: AnyView? let customActionView: AnyView? let orientation: Orientation @@ -54,7 +54,7 @@ public struct ContextualDaxDialogContent: View { messageFont: Font? = nil, list: [ContextualOnboardingListItem] = [], listAction: ((_: ContextualOnboardingListItem) -> Void)? = nil, - imageName: String? = nil, + customView: AnyView? = nil, customActionView: AnyView? = nil ) { self.title = title @@ -63,7 +63,7 @@ public struct ContextualDaxDialogContent: View { self.messageFont = messageFont self.list = list self.listAction = listAction - self.imageName = imageName + self.customView = customView self.customActionView = customActionView self.orientation = orientation @@ -75,8 +75,8 @@ public struct ContextualDaxDialogContent: View { if !list.isEmpty { itemsToAnimate.append(.list) } - if imageName != nil { - itemsToAnimate.append(.image) + if customView != nil { + itemsToAnimate.append(.customView) } if customActionView != nil { itemsToAnimate.append(.button) @@ -124,8 +124,8 @@ public struct ContextualDaxDialogContent: View { VStack(alignment: .leading, spacing: 16) { listView .visibility(nonTypingAnimatableItems.contains(.list) ? .visible : .invisible) - imageView - .visibility(nonTypingAnimatableItems.contains(.image) ? .visible : .invisible) + extraView + .visibility(nonTypingAnimatableItems.contains(.customView) ? .visible : .invisible) actionView .visibility(nonTypingAnimatableItems.contains(.button) ? .visible : .invisible) } @@ -166,13 +166,9 @@ public struct ContextualDaxDialogContent: View { } @ViewBuilder - private var imageView: some View { - if let imageName { - HStack { - Spacer() - Image(imageName) - Spacer() - } + private var extraView: some View { + if let customView { + customView } } @@ -187,7 +183,7 @@ public struct ContextualDaxDialogContent: View { case title case message case list - case image + case customView case button } } @@ -196,7 +192,7 @@ struct NonTypingAnimatableItems: OptionSet { let rawValue: Int static let list = NonTypingAnimatableItems(rawValue: 1 << 0) - static let image = NonTypingAnimatableItems(rawValue: 1 << 1) + static let customView = NonTypingAnimatableItems(rawValue: 1 << 1) static let button = NonTypingAnimatableItems(rawValue: 1 << 2) } @@ -225,8 +221,8 @@ extension ContextualDaxDialogContent { break case .list: nonTypingAnimatableItems.insert(.list) - case .image: - nonTypingAnimatableItems.insert(.image) + case .customView: + nonTypingAnimatableItems.insert(.customView) case .button: nonTypingAnimatableItems.insert(.button) } @@ -274,10 +270,18 @@ enum Metrics { #Preview("Intro Dialog - title, text, image and button") { let contextualText = NSMutableAttributedString(string: "Sabrina is the best\n\nBelieve me! ☝️") + let extraView = { + HStack { + Spacer() + Image("Sync-Desktop-New-128") + Spacer() + } + }() + return ContextualDaxDialogContent( title: "Who is the best?", message: contextualText, - imageName: "Sync-Desktop-New-128", + customView: AnyView(extraView), customActionView: AnyView(Button("Got it!", action: {}))) .padding() .preferredColorScheme(.light) From 8a1bc5526e14c589ca2cc74e6e7d125952b79bc1 Mon Sep 17 00:00:00 2001 From: Anh Do <18567+quanganhdo@users.noreply.github.com> Date: Thu, 31 Oct 2024 12:35:24 -0400 Subject: [PATCH 04/14] Connect refactored update flow to the new release notes page (#1025) Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1199230911884351/1208183525704010/f iOS PR: macOS PR: https://github.com/duckduckgo/macos-browser/pull/3411 What kind of version bump will this require?: Major/Minor/Patch **Optional**: Tech Design URL: CC: **Description**: **Steps to test this PR**: 1. 1. **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 32abe9c26..65d2e66d8 100644 --- a/Package.resolved +++ b/Package.resolved @@ -14,8 +14,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/content-scope-scripts", "state" : { - "revision" : "b74549bd869fdecc16fad851f2f608b1724764df", - "version" : "6.25.0" + "revision" : "48fee2508995d4ac02d18b3d55424adedcb4ce4f", + "version" : "6.28.0" } }, { diff --git a/Package.swift b/Package.swift index eee36a46b..64e215b67 100644 --- a/Package.swift +++ b/Package.swift @@ -52,7 +52,7 @@ let package = Package( .package(url: "https://github.com/duckduckgo/sync_crypto", exact: "0.2.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/content-scope-scripts", exact: "6.25.0"), + .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"), .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.0.1") From ddb358a751f4a8d41b7510e22d192b517138bf9f Mon Sep 17 00:00:00 2001 From: Fernando Bunn Date: Thu, 31 Oct 2024 17:19:57 +0000 Subject: [PATCH 05/14] Ignore GRDB (#1052) Task/Issue URL: https://app.asana.com/0/1204167627774280/1208651137074643/f iOS PR: N/A macOS PR: N/A What kind of version bump will this require?: N/A **Description**: Ignore GRDB --- .github/dependabot.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 3cd030c3d..e9e15439b 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,6 +1,3 @@ -# Please see the documentation for all configuration options: -# https://docs.github.com/github/administering-a-repository/configuration-options-for-dependency-updates - version: 2 updates: - package-ecosystem: "gitsubmodule" # See documentation for possible values @@ -10,4 +7,6 @@ updates: - package-ecosystem: "swift" # For Swift package updates directory: "/" # Location of Package.swift file schedule: - interval: "daily" \ No newline at end of file + interval: "daily" + ignore: + - dependency-name: "GRDB" From 8602894bc92c6fc2c080773208f13d99bc7a03c2 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 1 Nov 2024 11:48:32 +0100 Subject: [PATCH 06/14] Include www.youtube.com as Youtube host (#1028) Task/Issue URL: https://app.asana.com/0/1204099484721401/1208593005609709/f iOS PR: https://github.com/duckduckgo/iOS/pull/3455 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3434 What kind of version bump will this require?: Minor **Description**: We were missing www.youtube.com as a valid host for Youtube. --- Sources/DuckPlayer/DuckPlayerURLExtension.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/DuckPlayer/DuckPlayerURLExtension.swift b/Sources/DuckPlayer/DuckPlayerURLExtension.swift index 81ea0b4c9..698d15bb4 100644 --- a/Sources/DuckPlayer/DuckPlayerURLExtension.swift +++ b/Sources/DuckPlayer/DuckPlayerURLExtension.swift @@ -155,7 +155,7 @@ extension URL { public var isYoutube: Bool { guard let host else { return false } - return host == "m.youtube.com" || host == "youtube.com" + return host == "m.youtube.com" || host == "youtube.com" || host == "www.youtube.com" } public func addingWatchInYoutubeQueryParameter() -> URL? { From d39d04cf36b8522f894eebc3e11ee5fe65d880fa Mon Sep 17 00:00:00 2001 From: Thom Espach Date: Fri, 1 Nov 2024 11:02:30 +0000 Subject: [PATCH 07/14] Bug Fix: Phishing Detection Dataset Discrepancies (#1032) Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1204023833050360/1208567121137949/f iOS PR: https://github.com/duckduckgo/iOS/pull/3469 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3440 What kind of version bump will this require?: Patch **Optional**: Tech Design URL: CC: **Description**: In [Implement desktop integration efficacy tests - 5-7 days](https://app.asana.com/0/1207943168535188/1207205745934704/f) it was discovered that Swift's client-side caching results in out-of-date datasets and significant dataset discrepancies between different clients. For example, it's very common for the same request to return different results from the backend, resulting in a client believing they are updating to a newer revision than they are. Over time, this compounds and results in disparate versions of the same dataset across different clients, putting users at risk of landing on newer phishing pages. Fix: - Remove Client Side Caching in PhishingDetectionClient.swift - Ensure embedded dataset is used to replace the on-disk dataset when the revision of the embedded dataset > on disk dataset **Steps to test this PR**: 1. Check unit tests 3. Change on-disk revision: 4. `echo "1650000" > "/System/Volumes/Data/Users//Library/Application Support/com.duckduckgo.macos.browser.debug/revision.txt"` 5. Build the browser 6. Visit https://privacy-test-pages.site/security/badware/phishing.html 7. Ensure blocked 8. Check on-disk revision: 9. `cat "/System/Volumes/Data/Users//Library/Application Support/com.duckduckgo.macos.browser.debug/revision.txt"` 10. Should be > 1650000 **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) --- .../PhishingDetectionDataStore.swift | 49 +++++++-- .../PhishingDetectionDataStoreTests.swift | 101 ++++++++++++++++++ 2 files changed, 141 insertions(+), 9 deletions(-) diff --git a/Sources/PhishingDetection/PhishingDetectionDataStore.swift b/Sources/PhishingDetection/PhishingDetectionDataStore.swift index ab761f272..f247f90b8 100644 --- a/Sources/PhishingDetection/PhishingDetectionDataStore.swift +++ b/Sources/PhishingDetection/PhishingDetectionDataStore.swift @@ -136,10 +136,16 @@ public class PhishingDetectionDataStore: PhishingDetectionDataSaving { } private func loadHashPrefix() -> Set { - guard let data = fileStorageManager.read(from: hashPrefixFilename) else { return dataProvider.loadEmbeddedHashPrefixes() } + guard let data = fileStorageManager.read(from: hashPrefixFilename) else { + return dataProvider.loadEmbeddedHashPrefixes() + } let decoder = JSONDecoder() do { - return Set(try decoder.decode(Set.self, from: data)) + if loadRevisionFromDisk() < dataProvider.embeddedRevision { + return dataProvider.loadEmbeddedHashPrefixes() + } + let onDiskHashPrefixes = Set(try decoder.decode(Set.self, from: data)) + return onDiskHashPrefixes } catch { Logger.phishingDetectionDataStore.error("Error decoding \(self.hashPrefixFilename): \(error.localizedDescription)") return dataProvider.loadEmbeddedHashPrefixes() @@ -147,18 +153,26 @@ public class PhishingDetectionDataStore: PhishingDetectionDataSaving { } private func loadFilterSet() -> Set { - guard let data = fileStorageManager.read(from: filterSetFilename) else { return dataProvider.loadEmbeddedFilterSet() } + guard let data = fileStorageManager.read(from: filterSetFilename) else { + return dataProvider.loadEmbeddedFilterSet() + } let decoder = JSONDecoder() do { - return Set(try decoder.decode(Set.self, from: data)) + if loadRevisionFromDisk() < dataProvider.embeddedRevision { + return dataProvider.loadEmbeddedFilterSet() + } + let onDiskFilterSet = Set(try decoder.decode(Set.self, from: data)) + return onDiskFilterSet } catch { Logger.phishingDetectionDataStore.error("Error decoding \(self.filterSetFilename): \(error.localizedDescription)") return dataProvider.loadEmbeddedFilterSet() } } - private func loadRevision() -> Int { - guard let data = fileStorageManager.read(from: revisionFilename) else { return dataProvider.embeddedRevision } + private func loadRevisionFromDisk() -> Int { + guard let data = fileStorageManager.read(from: revisionFilename) else { + return dataProvider.embeddedRevision + } let decoder = JSONDecoder() do { return try decoder.decode(Int.self, from: data) @@ -167,22 +181,39 @@ public class PhishingDetectionDataStore: PhishingDetectionDataSaving { return dataProvider.embeddedRevision } } + + private func loadRevision() -> Int { + guard let data = fileStorageManager.read(from: revisionFilename) else { + return dataProvider.embeddedRevision + } + let decoder = JSONDecoder() + do { + let loadedRevision = try decoder.decode(Int.self, from: data) + if loadedRevision < dataProvider.embeddedRevision { + return dataProvider.embeddedRevision + } + return loadedRevision + } catch { + Logger.phishingDetectionDataStore.error("Error decoding \(self.revisionFilename): \(error.localizedDescription)") + return dataProvider.embeddedRevision + } + } } extension PhishingDetectionDataStore { public func saveFilterSet(set: Set) { - writeFilterSet() self.filterSet = set + writeFilterSet() } public func saveHashPrefixes(set: Set) { - writeHashPrefixes() self.hashPrefixes = set + writeHashPrefixes() } public func saveRevision(_ revision: Int) { - writeRevision() self.currentRevision = revision + writeRevision() } } diff --git a/Tests/PhishingDetectionTests/PhishingDetectionDataStoreTests.swift b/Tests/PhishingDetectionTests/PhishingDetectionDataStoreTests.swift index 95a51eb05..79e9fb500 100644 --- a/Tests/PhishingDetectionTests/PhishingDetectionDataStoreTests.swift +++ b/Tests/PhishingDetectionTests/PhishingDetectionDataStoreTests.swift @@ -61,6 +61,60 @@ class PhishingDetectionDataStoreTests: XCTestCase { XCTAssertEqual(actualHashPrefix, expectedHashPrefix) } + func testWhenEmbeddedRevisionNewerThanOnDisk_ThenLoadEmbedded() async { + let encoder = JSONEncoder() + // On Disk Data Setup + fileStorageManager.write(data: "1".utf8data, to: "revision.txt") + let onDiskFilterSet = Set([Filter(hashValue: "other", regex: "other")]) + let filterSetData = try! encoder.encode(Array(onDiskFilterSet)) + let onDiskHashPrefix = Set(["faffa"]) + let hashPrefixData = try! encoder.encode(Array(onDiskHashPrefix)) + fileStorageManager.write(data: filterSetData, to: "filterSet.json") + fileStorageManager.write(data: hashPrefixData, to: "hashPrefixes.json") + + // Embedded Data Setup + mockDataProvider.embeddedRevision = 5 + let embeddedFilterSet = Set([Filter(hashValue: "some", regex: "some")]) + let embeddedHashPrefix = Set(["sassa"]) + mockDataProvider.shouldReturnFilterSet(set: embeddedFilterSet) + mockDataProvider.shouldReturnHashPrefixes(set: embeddedHashPrefix) + + let actualRevision = dataStore.currentRevision + let actualFilterSet = dataStore.filterSet + let actualHashPrefix = dataStore.hashPrefixes + + XCTAssertEqual(actualFilterSet, embeddedFilterSet) + XCTAssertEqual(actualHashPrefix, embeddedHashPrefix) + XCTAssertEqual(actualRevision, 5) + } + + func testWhenEmbeddedRevisionOlderThanOnDisk_ThenDontLoadEmbedded() async { + let encoder = JSONEncoder() + // On Disk Data Setup + fileStorageManager.write(data: "6".utf8data, to: "revision.txt") + let onDiskFilterSet = Set([Filter(hashValue: "other", regex: "other")]) + let filterSetData = try! encoder.encode(Array(onDiskFilterSet)) + let onDiskHashPrefix = Set(["faffa"]) + let hashPrefixData = try! encoder.encode(Array(onDiskHashPrefix)) + fileStorageManager.write(data: filterSetData, to: "filterSet.json") + fileStorageManager.write(data: hashPrefixData, to: "hashPrefixes.json") + + // Embedded Data Setup + mockDataProvider.embeddedRevision = 1 + let embeddedFilterSet = Set([Filter(hashValue: "some", regex: "some")]) + let embeddedHashPrefix = Set(["sassa"]) + mockDataProvider.shouldReturnFilterSet(set: embeddedFilterSet) + mockDataProvider.shouldReturnHashPrefixes(set: embeddedHashPrefix) + + let actualRevision = dataStore.currentRevision + let actualFilterSet = dataStore.filterSet + let actualHashPrefix = dataStore.hashPrefixes + + XCTAssertEqual(actualFilterSet, onDiskFilterSet) + XCTAssertEqual(actualHashPrefix, onDiskHashPrefix) + XCTAssertEqual(actualRevision, 6) + } + func testWriteAndLoadData() async { // Get and write data let expectedHashPrefixes = Set(["aabb"]) @@ -93,4 +147,51 @@ class PhishingDetectionDataStoreTests: XCTestCase { XCTFail("Failed to decode stored PhishingDetection data") } } + + func testLazyLoadingDoesNotReturnStaleData() async { + clearDatasets() + + // Set up initial data + let initialFilterSet = Set([Filter(hashValue: "initial", regex: "initial")]) + let initialHashPrefixes = Set(["initialPrefix"]) + mockDataProvider.shouldReturnFilterSet(set: initialFilterSet) + mockDataProvider.shouldReturnHashPrefixes(set: initialHashPrefixes) + + // Access the lazy-loaded properties to trigger loading + let loadedFilterSet = dataStore.filterSet + let loadedHashPrefixes = dataStore.hashPrefixes + + // Validate loaded data matches initial data + XCTAssertEqual(loadedFilterSet, initialFilterSet) + XCTAssertEqual(loadedHashPrefixes, initialHashPrefixes) + + // Update in-memory data + let updatedFilterSet = Set([Filter(hashValue: "updated", regex: "updated")]) + let updatedHashPrefixes = Set(["updatedPrefix"]) + dataStore.saveFilterSet(set: updatedFilterSet) + dataStore.saveHashPrefixes(set: updatedHashPrefixes) + + // Access lazy-loaded properties again + let reloadedFilterSet = dataStore.filterSet + let reloadedHashPrefixes = dataStore.hashPrefixes + + // Validate reloaded data matches updated data + XCTAssertEqual(reloadedFilterSet, updatedFilterSet) + XCTAssertEqual(reloadedHashPrefixes, updatedHashPrefixes) + + // Validate on-disk data is also updated + let storedFilterSetData = fileStorageManager.read(from: "filterSet.json") + let storedHashPrefixesData = fileStorageManager.read(from: "hashPrefixes.json") + + let decoder = JSONDecoder() + if let storedFilterSet = try? decoder.decode(Set.self, from: storedFilterSetData!), + let storedHashPrefixes = try? decoder.decode(Set.self, from: storedHashPrefixesData!) { + + XCTAssertEqual(storedFilterSet, updatedFilterSet) + XCTAssertEqual(storedHashPrefixes, updatedHashPrefixes) + } else { + XCTFail("Failed to decode stored PhishingDetection data after update") + } + } + } From 7b78b46340c9981b9774352be08c3c28b3a19011 Mon Sep 17 00:00:00 2001 From: Michal Smaga Date: Fri, 1 Nov 2024 13:02:07 +0100 Subject: [PATCH 08/14] Update to subscription cookie (#1053) Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1108686900785972/1208264562025859/f iOS PR: https://github.com/duckduckgo/iOS/pull/3512 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3489 What kind of version bump will this require?: Patch **Description**: Update to how the subscription cookie operates: - constraint the cookie to `subscriptions.duckduckgo.com` - on sign out do not fully remove the cookie - just clear the value - gate the feature behind the `setAccessTokenCookieForSubscriptionDomains` privacy config feature flag **Steps to test this PR**: See client PRs **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) --- .../Features/PrivacyFeature.swift | 1 + .../SubscriptionCookieManager.swift | 83 ++++++++++++------- .../SubscriptionCookieManagerEvent.swift | 5 +- .../SubscriptionCookieManagerMock.swift | 2 + .../SubscriptionCookieManagerTests.swift | 20 +++-- 5 files changed, 71 insertions(+), 40 deletions(-) diff --git a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift index 478aa8c4a..487819d36 100644 --- a/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift +++ b/Sources/BrowserServicesKit/PrivacyConfig/Features/PrivacyFeature.swift @@ -152,6 +152,7 @@ public enum PrivacyProSubfeature: String, Equatable, PrivacySubfeature { case isLaunchedOverride case isLaunchedOverrideStripe case useUnifiedFeedback + case setAccessTokenCookieForSubscriptionDomains } public enum SslCertificatesSubfeature: String, PrivacySubfeature { diff --git a/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManager.swift b/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManager.swift index 7d7099d75..eff9f2e69 100644 --- a/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManager.swift +++ b/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManager.swift @@ -22,6 +22,9 @@ import os.log public protocol SubscriptionCookieManaging { init(subscriptionManager: SubscriptionManager, currentCookieStore: @MainActor @escaping () -> HTTPCookieStore?, eventMapping: EventMapping) + func enableSettingSubscriptionCookie() + func disableSettingSubscriptionCookie() async + func refreshSubscriptionCookie() async func resetLastRefreshDate() @@ -30,7 +33,7 @@ public protocol SubscriptionCookieManaging { public final class SubscriptionCookieManager: SubscriptionCookieManaging { - public static let cookieDomain = ".duckduckgo.com" + public static let cookieDomain = "subscriptions.duckduckgo.com" public static let cookieName = "privacy_pro_access_token" private static let defaultRefreshTimeInterval: TimeInterval = .hours(4) @@ -41,6 +44,7 @@ public final class SubscriptionCookieManager: SubscriptionCookieManaging { public private(set) var lastRefreshDate: Date? private let refreshTimeInterval: TimeInterval + private var isSettingSubscriptionCookieEnabled: Bool = false convenience nonisolated public required init(subscriptionManager: SubscriptionManager, currentCookieStore: @MainActor @escaping () -> HTTPCookieStore?, @@ -60,17 +64,28 @@ public final class SubscriptionCookieManager: SubscriptionCookieManaging { self.eventMapping = eventMapping self.refreshTimeInterval = refreshTimeInterval - registerForSubscriptionAccountManagerEvents() - } - - private func registerForSubscriptionAccountManagerEvents() { NotificationCenter.default.addObserver(self, selector: #selector(handleAccountDidSignIn), name: .accountDidSignIn, object: nil) NotificationCenter.default.addObserver(self, selector: #selector(handleAccountDidSignOut), name: .accountDidSignOut, object: nil) } + public func enableSettingSubscriptionCookie() { + isSettingSubscriptionCookieEnabled = true + } + + public func disableSettingSubscriptionCookie() async { + isSettingSubscriptionCookieEnabled = false + if let cookieStore = await currentCookieStore(), + let cookie = await cookieStore.fetchCurrentSubscriptionCookie() { + await cookieStore.deleteCookie(cookie) + } + } + @objc private func handleAccountDidSignIn() { Task { - guard let cookieStore = await currentCookieStore() else { return } + guard isSettingSubscriptionCookieEnabled, + let cookieStore = await currentCookieStore() + else { return } + guard let accessToken = subscriptionManager.accountManager.accessToken else { Logger.subscription.error("[SubscriptionCookieManager] Handle .accountDidSignIn - can't set the cookie, token is missing") eventMapping.fire(.errorHandlingAccountDidSignInTokenIsMissing) @@ -89,21 +104,24 @@ public final class SubscriptionCookieManager: SubscriptionCookieManaging { @objc private func handleAccountDidSignOut() { Task { - guard let cookieStore = await currentCookieStore() else { return } - guard let subscriptionCookie = await cookieStore.fetchCurrentSubscriptionCookie() else { - Logger.subscription.error("[SubscriptionCookieManager] Handle .accountDidSignOut - can't delete the cookie, cookie is missing") - eventMapping.fire(.errorHandlingAccountDidSignOutCookieIsMissing) - return - } + guard isSettingSubscriptionCookieEnabled, + let cookieStore = await currentCookieStore() + else { return } Logger.subscription.info("[SubscriptionCookieManager] Handle .accountDidSignOut - deleting cookie") - await cookieStore.deleteCookie(subscriptionCookie) - updateLastRefreshDateToNow() + + do { + try await cookieStore.setEmptySubscriptionCookie() + updateLastRefreshDateToNow() + } catch { + eventMapping.fire(.failedToSetSubscriptionCookie) + } } } public func refreshSubscriptionCookie() async { - guard shouldRefreshSubscriptionCookie() else { return } - guard let cookieStore = await currentCookieStore() else { return } + guard isSettingSubscriptionCookieEnabled, + shouldRefreshSubscriptionCookie(), + let cookieStore = await currentCookieStore() else { return } Logger.subscription.info("[SubscriptionCookieManager] Refresh subscription cookie") updateLastRefreshDateToNow() @@ -111,25 +129,22 @@ public final class SubscriptionCookieManager: SubscriptionCookieManaging { let accessToken: String? = subscriptionManager.accountManager.accessToken let subscriptionCookie = await cookieStore.fetchCurrentSubscriptionCookie() - if let accessToken { - if subscriptionCookie == nil || subscriptionCookie?.value != accessToken { - Logger.subscription.info("[SubscriptionCookieManager] Refresh: No cookie or one with different value") - do { + let noCookieOrWithUnexpectedValue = (accessToken ?? "") != subscriptionCookie?.value + + do { + if noCookieOrWithUnexpectedValue { + Logger.subscription.info("[SubscriptionCookieManager] Refresh: No cookie or one with unexpected value") + + if let accessToken { try await cookieStore.setSubscriptionCookie(for: accessToken) - eventMapping.fire(.subscriptionCookieRefreshedWithUpdate) - } catch { - eventMapping.fire(.failedToSetSubscriptionCookie) + eventMapping.fire(.subscriptionCookieRefreshedWithAccessToken) + } else { + try await cookieStore.setEmptySubscriptionCookie() + eventMapping.fire(.subscriptionCookieRefreshedWithEmptyValue) } - } else { - Logger.subscription.info("[SubscriptionCookieManager] Refresh: Cookie exists and is up to date") - return - } - } else { - if let subscriptionCookie { - Logger.subscription.info("[SubscriptionCookieManager] Refresh: No access token but old cookie exists, deleting it") - await cookieStore.deleteCookie(subscriptionCookie) - eventMapping.fire(.subscriptionCookieRefreshedWithDelete) } + } catch { + eventMapping.fire(.failedToSetSubscriptionCookie) } } @@ -161,6 +176,10 @@ private extension HTTPCookieStore { await allCookies().first { $0.domain == SubscriptionCookieManager.cookieDomain && $0.name == SubscriptionCookieManager.cookieName } } + func setEmptySubscriptionCookie() async throws { + try await setSubscriptionCookie(for: "") + } + func setSubscriptionCookie(for token: String) async throws { guard let cookie = HTTPCookie(properties: [ .domain: SubscriptionCookieManager.cookieDomain, diff --git a/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManagerEvent.swift b/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManagerEvent.swift index 86f604d3d..ffedd13c2 100644 --- a/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManagerEvent.swift +++ b/Sources/Subscription/SubscriptionCookie/SubscriptionCookieManagerEvent.swift @@ -20,10 +20,9 @@ import Foundation public enum SubscriptionCookieManagerEvent { case errorHandlingAccountDidSignInTokenIsMissing - case errorHandlingAccountDidSignOutCookieIsMissing - case subscriptionCookieRefreshedWithUpdate - case subscriptionCookieRefreshedWithDelete + case subscriptionCookieRefreshedWithAccessToken + case subscriptionCookieRefreshedWithEmptyValue case failedToSetSubscriptionCookie } diff --git a/Sources/SubscriptionTestingUtilities/SubscriptionCookie/SubscriptionCookieManagerMock.swift b/Sources/SubscriptionTestingUtilities/SubscriptionCookie/SubscriptionCookieManagerMock.swift index 56e970ebb..a9166689a 100644 --- a/Sources/SubscriptionTestingUtilities/SubscriptionCookie/SubscriptionCookieManagerMock.swift +++ b/Sources/SubscriptionTestingUtilities/SubscriptionCookie/SubscriptionCookieManagerMock.swift @@ -48,6 +48,8 @@ public final class SubscriptionCookieManagerMock: SubscriptionCookieManaging { } + public func enableSettingSubscriptionCookie() { } + public func disableSettingSubscriptionCookie() async { } public func refreshSubscriptionCookie() async { } public func resetLastRefreshDate() { } } diff --git a/Tests/SubscriptionTests/SubscriptionCookie/SubscriptionCookieManagerTests.swift b/Tests/SubscriptionTests/SubscriptionCookie/SubscriptionCookieManagerTests.swift index 04dfe7b8b..07fb12bd4 100644 --- a/Tests/SubscriptionTests/SubscriptionCookie/SubscriptionCookieManagerTests.swift +++ b/Tests/SubscriptionTests/SubscriptionCookie/SubscriptionCookieManagerTests.swift @@ -76,6 +76,7 @@ final class SubscriptionCookieManagerTests: XCTestCase { accountManager.accessToken = Constants.accessToken // When + subscriptionCookieManager.enableSettingSubscriptionCookie() NotificationCenter.default.post(name: .accountDidSignIn, object: self, userInfo: nil) try await Task.sleep(seconds: 0.1) @@ -88,11 +89,12 @@ final class SubscriptionCookieManagerTests: XCTestCase { await ensureSubscriptionCookieIsInTheCookieStore() // When + subscriptionCookieManager.enableSettingSubscriptionCookie() NotificationCenter.default.post(name: .accountDidSignOut, object: self, userInfo: nil) try await Task.sleep(seconds: 0.1) // Then - await checkSubscriptionCookieIsNotPresent() + await checkSubscriptionCookieIsHasEmptyValue() } func testRefreshWhenSignedInButCookieIsMissing() async throws { @@ -101,6 +103,7 @@ final class SubscriptionCookieManagerTests: XCTestCase { await ensureNoSubscriptionCookieInTheCookieStore() // When + subscriptionCookieManager.enableSettingSubscriptionCookie() await subscriptionCookieManager.refreshSubscriptionCookie() try await Task.sleep(seconds: 0.1) @@ -114,11 +117,12 @@ final class SubscriptionCookieManagerTests: XCTestCase { await ensureSubscriptionCookieIsInTheCookieStore() // When + subscriptionCookieManager.enableSettingSubscriptionCookie() await subscriptionCookieManager.refreshSubscriptionCookie() try await Task.sleep(seconds: 0.1) // Then - await checkSubscriptionCookieIsNotPresent() + await checkSubscriptionCookieIsHasEmptyValue() } func testRefreshNotTriggeredTwiceWithinSetRefreshInterval() async throws { @@ -127,6 +131,7 @@ final class SubscriptionCookieManagerTests: XCTestCase { let secondRefreshDate: Date? // When + subscriptionCookieManager.enableSettingSubscriptionCookie() await subscriptionCookieManager.refreshSubscriptionCookie() firstRefreshDate = subscriptionCookieManager.lastRefreshDate @@ -145,6 +150,7 @@ final class SubscriptionCookieManagerTests: XCTestCase { let secondRefreshDate: Date? // When + subscriptionCookieManager.enableSettingSubscriptionCookie() await subscriptionCookieManager.refreshSubscriptionCookie() firstRefreshDate = subscriptionCookieManager.lastRefreshDate @@ -186,9 +192,12 @@ final class SubscriptionCookieManagerTests: XCTestCase { XCTAssertEqual(subscriptionCookie.value, Constants.accessToken) } - private func checkSubscriptionCookieIsNotPresent() async { - let cookie = await cookieStore.fetchSubscriptionCookie() - XCTAssertNil(cookie) + private func checkSubscriptionCookieIsHasEmptyValue() async { + guard let subscriptionCookie = await cookieStore.fetchSubscriptionCookie() else { + XCTFail("No subscription cookie in the store") + return + } + XCTAssertEqual(subscriptionCookie.value, "") } } @@ -213,6 +222,7 @@ class MockHTTPCookieStore: HTTPCookieStore { } func setCookie(_ cookie: HTTPCookie) async { + cookies.removeAll { $0.domain == cookie.domain } cookies.append(cookie) } From 8ca19fa8a1a157960e3eae8cff64ef0b988701bb Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Nov 2024 18:42:40 +0100 Subject: [PATCH 09/14] Bump github.com/1024jp/gzipswift from 6.0.1 to 6.1.0 (#1050) Bumps [github.com/1024jp/gzipswift](https://github.com/1024jp/GzipSwift) from 6.0.1 to 6.1.0. --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 65d2e66d8..78a4f7818 100644 --- a/Package.resolved +++ b/Package.resolved @@ -41,8 +41,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/1024jp/GzipSwift.git", "state" : { - "revision" : "731037f6cc2be2ec01562f6597c1d0aa3fe6fd05", - "version" : "6.0.1" + "revision" : "56bf51fdd2fe4b2cf254b2cf34aede3d7caccc6c", + "version" : "6.1.0" } }, { diff --git a/Package.swift b/Package.swift index 64e215b67..02fcb31f1 100644 --- a/Package.swift +++ b/Package.swift @@ -55,7 +55,7 @@ let package = Package( .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"), - .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.0.1") + .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.1.0") ], targets: [ .target( From 63d57559da8f1d42b56cdbf2d90df82b57044f4c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Nov 2024 18:43:02 +0100 Subject: [PATCH 10/14] Bump github.com/duckduckgo/sync_crypto from 0.2.0 to 0.3.0 (#1048) Bumps [github.com/duckduckgo/sync_crypto](https://github.com/duckduckgo/sync_crypto) from 0.2.0 to 0.3.0. --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 78a4f7818..70b4732a4 100644 --- a/Package.resolved +++ b/Package.resolved @@ -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 index 02fcb31f1..866a79e00 100644 --- a/Package.swift +++ b/Package.swift @@ -49,7 +49,7 @@ 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/content-scope-scripts", exact: "6.28.0"), From 0bb673de345fc47693ef5218bced6acd5b27c7e0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Nov 2024 18:43:22 +0100 Subject: [PATCH 11/14] Bump github.com/duckduckgo/privacy-dashboard from 5.3.0 to 7.1.1 (#1046) Bumps [github.com/duckduckgo/privacy-dashboard](https://github.com/duckduckgo/privacy-dashboard) from 5.3.0 to 7.1.1. --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 70b4732a4..98e4967bf 100644 --- 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" } }, { diff --git a/Package.swift b/Package.swift index 866a79e00..9125e09b3 100644 --- a/Package.swift +++ b/Package.swift @@ -51,7 +51,7 @@ let package = Package( .package(url: "https://github.com/duckduckgo/TrackerRadarKit", exact: "3.0.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"), From 393d9551907d7293964e44527a9ca9eb8c9966a6 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Fri, 1 Nov 2024 16:31:29 -0700 Subject: [PATCH 12/14] Revert "Bump github.com/1024jp/gzipswift from 6.0.1 to 6.1.0" (#1055) Reverts #1050 This change is being reverted as gzipswift breaks compilation on 6.1.0. This issue is reported in 1024jp/GzipSwift#69 and is due to a malformed Package.swift file. To test this, just check that CI is green. --- Package.resolved | 4 ++-- Package.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index 98e4967bf..3e0793e3d 100644 --- a/Package.resolved +++ b/Package.resolved @@ -41,8 +41,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/1024jp/GzipSwift.git", "state" : { - "revision" : "56bf51fdd2fe4b2cf254b2cf34aede3d7caccc6c", - "version" : "6.1.0" + "revision" : "731037f6cc2be2ec01562f6597c1d0aa3fe6fd05", + "version" : "6.0.1" } }, { diff --git a/Package.swift b/Package.swift index 9125e09b3..c63db8d64 100644 --- a/Package.swift +++ b/Package.swift @@ -55,7 +55,7 @@ let package = Package( .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"), - .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.1.0") + .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.0.1") ], targets: [ .target( From 80894bf69fe789e41b13f3de6be97f1300ca56e5 Mon Sep 17 00:00:00 2001 From: Tom Strba <57389842+tomasstrba@users.noreply.github.com> Date: Sun, 3 Nov 2024 22:21:43 +0100 Subject: [PATCH 13/14] Allowing users to delete suggestions (#1027) Task/Issue URL: https://app.asana.com/0/1148564399326804/1208219569168397/f **Description**: Improvements of the HistoryCoordinator to allow users to delete individual suggestions --- Sources/History/HistoryCoordinator.swift | 15 ++++++++ Sources/Suggestions/Suggestion.swift | 12 ++++-- Sources/Suggestions/SuggestionResult.swift | 6 +-- .../HistoryCoordinatorTests.swift | 37 +++++++++++++++++++ 4 files changed, 64 insertions(+), 6 deletions(-) diff --git a/Sources/History/HistoryCoordinator.swift b/Sources/History/HistoryCoordinator.swift index e49280670..9017f1db8 100644 --- a/Sources/History/HistoryCoordinator.swift +++ b/Sources/History/HistoryCoordinator.swift @@ -44,6 +44,7 @@ public protocol HistoryCoordinating: AnyObject { func burnDomains(_ baseDomains: Set, tld: TLD, completion: @escaping (Set) -> 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/Suggestions/Suggestion.swift b/Sources/Suggestions/Suggestion.swift index 3a50fdc33..d96cea05e 100644 --- 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 index af0997dea..1576ed48c 100644 --- 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 index 98811e65d..c84622e00 100644 --- 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 { From 45261df2963fc89094e169f9f2d0d9aa098093f3 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Sun, 3 Nov 2024 14:57:15 -0800 Subject: [PATCH 14/14] Validate VPN errors before re-throwing them (#1054) **Required**: Task/Issue URL: https://app.asana.com/0/414709148257752/1208225499545869/f iOS PR: https://github.com/duckduckgo/iOS/pull/3513 macOS PR: https://github.com/duckduckgo/macos-browser/pull/3490 What kind of version bump will this require?: Major **Description**: This PR attempts to catch malformed errors before they get thrown to the OS. It also informs us when this happens. --- .../PacketTunnelProvider.swift | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index fb133916c..db94999bf 100644 --- 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 {