Fix #6146: Add eTLD+1 check when debouncing (#6147)

Author: cuba

Client/WebFilters/DebouncingResourceDownloader.swift

@@ -360,16 +360,18 @@ public class DebouncingResourceDownloader {
     ///
     /// The following conditions must be met to redirect:
     /// 1. Must have a `MatcherRule` that satisifes this url
-    /// 2. Extracted URL  not be the same domain as the base URL
-    /// 3. Extracted URL must be a valid URL (Have valid scheme, have an `eTLD+1` or be an IP address etc.. )
-    /// 4. Extracted URL must have a `http` or `https` scheme
+    /// 2. Extracted URL must not have the same origin as that of the base URL
+    /// 3. Extracted URL must not have the same `eTLD+1` as that of the base URL (#6146)
+    /// 4. Extracted URL must be a valid URL (Have valid scheme, have an `eTLD+1` or be an IP address etc.. )
+    /// 5. Extracted URL must have a `http` or `https` scheme
     private func redirectURLOnce(from url: URL) throws -> (url: URL, rule: RedirectRule)? {
       guard let rule = matchingRedirectRule(for: url) else {
         return nil
       }
 
       guard let extractedURL = try rule.extractRedirectURL(from: url),
-            url.host != extractedURL.host,
+            url.origin != extractedURL.origin,
+            url.baseDomain != extractedURL.baseDomain,
             extractedURL.scheme == "http" || extractedURL.scheme == "https" else {
         return nil
       }

Tests/ClientTests/Resources/debouncing.json

@@ -244,6 +244,15 @@
     "action": "regex-path",
     "param": "^/(.*)/(.*)$"
   },
+  {
+    "include": [
+      "http://*.click.example/bounce?url=*"
+    ],
+    "exclude": [
+    ],
+    "action": "redirect",
+    "param": "url"
+  },
   {
     "include": [
       "*://brave-long-regex.example/*"

Tests/ClientTests/Web Filters/DebouncingResourceDownloaderTests.swift

@@ -110,7 +110,9 @@ class DebouncingResourceDownloaderTests: XCTestCase {
     let redirectChain = downloader.redirectChain(for: startURL)
 
     // Then
-    XCTAssertEqual(redirectChain.last?.url, finalURL)
+    // While we used to go to `finalURL`, we now stop at `intermediateURL`
+    // This changed with the introduction of ticket #6146
+    XCTAssertEqual(redirectChain.last?.url, intermediateURL)
   }
 
   // Test a long redirect chain that bounces through the original URL's domain,
@@ -374,6 +376,36 @@ class DebouncingResourceDownloaderTests: XCTestCase {
     )
   }
 
+  /// Test several restrictions to debouncing
+  func testDebounceRestrictions() {
+    // Given
+    // A constructed url
+    let baseURL = URL(string: "http://a.click.example/bounce")!
+    let startURL1 = baseURL.addRedirectParam(URL(string: "http://a.click.example/dest")!)
+    let startURL2 = baseURL.addRedirectParam(URL(string: "http://b.click.example/dest")!)
+    let startURL3 = baseURL.addRedirectParam(URL(string: "ftp://example.com/dest")!)
+
+    // When
+    // A redirect is returned
+    let redirectChain1 = downloader.redirectChain(for: startURL1)
+    let redirectChain2 = downloader.redirectChain(for: startURL2)
+    let redirectChain3 = downloader.redirectChain(for: startURL3)
+
+    // Then
+    XCTAssertNil(
+      redirectChain1.last,
+      "This should be nil because origin matches"
+    )
+    XCTAssertNil(
+      redirectChain2.last,
+      "This should be nil because eTLD+1 matches"
+    )
+    XCTAssertNil(
+      redirectChain3.last,
+      "This should be nil because the scheme is not http or https"
+    )
+  }
+  
   /// Test regex rule where `prepend_scheme` is not specified and `base64` action is specified
   func testRegexRuleWithBase64DecodedURL() {
     // Given