Address PR comments

duckduckgo · Dec 16, 2024 · 3fb96e8 · 3fb96e8
1 parent bba00aa
commit 3fb96e8
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 40 deletions.
diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt
@@ -199,7 +199,7 @@ class BrowserWebViewClientTest {
         whenever(currentTimeProvider.elapsedRealtime()).thenReturn(0)
         whenever(webViewVersionProvider.getMajorVersion()).thenReturn("1")
         whenever(deviceInfo.appVersion).thenReturn("1")
-        whenever(mockMaliciousSiteProtection.shouldOverrideUrlLoading(any(), any(), any(), any())).thenReturn(false)
+        whenever(mockMaliciousSiteProtection.shouldOverrideUrlLoading(any(), any(), any())).thenReturn(false)
     }
 
     @UiThreadTest

diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt
@@ -165,14 +165,11 @@ class BrowserWebViewClient @Inject constructor(
             Timber.v("shouldOverride webViewUrl: ${webView.url} URL: $url")
             webViewClientListener?.onShouldOverride()
 
-            if (runBlocking {
-                maliciousSiteProtectionWebViewIntegration.shouldOverrideUrlLoading(
-                        url,
-                        webView.url?.toUri(),
-                        isForMainFrame,
-                        confirmationCallback,
-                    )
-            }
+            if (maliciousSiteProtectionWebViewIntegration.shouldOverrideUrlLoading(
+                    url,
+                    isForMainFrame,
+                    confirmationCallback,
+                )
             ) {
                 // TODO (cbarreiro): Handle site blocked synchronously
                 return true

diff --git a/...rc/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt b/...rc/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt
@@ -35,6 +35,7 @@ import java.net.URLDecoder
 import javax.inject.Inject
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
+import kotlinx.coroutines.runBlocking
 import timber.log.Timber
 
 interface MaliciousSiteBlockerWebViewIntegration {
@@ -45,9 +46,8 @@ interface MaliciousSiteBlockerWebViewIntegration {
         confirmationCallback: (isMalicious: Boolean) -> Unit,
     ): WebResourceResponse?
 
-    suspend fun shouldOverrideUrlLoading(
+    fun shouldOverrideUrlLoading(
         url: Uri,
-        webViewUrl: Uri?,
         isForMainFrame: Boolean,
         confirmationCallback: (isMalicious: Boolean) -> Unit,
     ): Boolean
@@ -123,30 +123,32 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
         return null
     }
 
-    override suspend fun shouldOverrideUrlLoading(
+    override fun shouldOverrideUrlLoading(
         url: Uri,
-        webViewUrl: Uri?,
         isForMainFrame: Boolean,
         confirmationCallback: (isMalicious: Boolean) -> Unit,
     ): Boolean {
-        if (!isFeatureEnabled) {
-            return false
-        }
-        val decodedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
+        return runBlocking {
+            if (!isFeatureEnabled) {
+                return@runBlocking false
+            }
+            val decodedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
 
-        if (processedUrls.contains(decodedUrl)) {
-            processedUrls.remove(decodedUrl)
-            Timber.tag("PhishingAndMalwareDetector").d("Already intercepted, skipping $decodedUrl")
-            return false
-        }
+            if (processedUrls.contains(decodedUrl)) {
+                processedUrls.remove(decodedUrl)
+                Timber.tag("PhishingAndMalwareDetector").d("Already intercepted, skipping $decodedUrl")
+                return@runBlocking false
+            }
 
-        if (isForMainFrame && decodedUrl.toUri() == webViewUrl) {
-            if (maliciousSiteProtection.isMalicious(decodedUrl.toUri(), confirmationCallback) == MALICIOUS) {
-                return true
+            // iframes always go through the shouldIntercept method, so we only need to check the main frame here
+            if (isForMainFrame) {
+                if (maliciousSiteProtection.isMalicious(decodedUrl.toUri(), confirmationCallback) == MALICIOUS) {
+                    return@runBlocking true
+                }
+                processedUrls.add(decodedUrl)
             }
-            processedUrls.add(decodedUrl)
+            false
         }
-        return false
     }
 
     private fun isForIframe(request: WebResourceRequest) = request.requestHeaders["Sec-Fetch-Dest"] == "iframe" ||

diff --git a/...java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegrationTest.kt b/...java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegrationTest.kt
@@ -49,7 +49,7 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
     fun `shouldOverrideUrlLoading returns false when feature is disabled`() = runTest {
         updateFeatureEnabled(false)
 
-        val result = testee.shouldOverrideUrlLoading(exampleUri, null, true) {}
+        val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
         assertFalse(result)
     }
 
@@ -67,7 +67,7 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
     fun `shouldOverrideUrlLoading returns false when url is already processed`() = runTest {
         testee.processedUrls.add(exampleUri.toString())
 
-        val result = testee.shouldOverrideUrlLoading(exampleUri, exampleUri, true) {}
+        val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
         assertFalse(result)
     }
 
@@ -109,31 +109,23 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
     fun `shouldOverride returns false when feature is enabled, is malicious, and is not mainframe`() = runTest {
         whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(MALICIOUS)
 
-        val result = testee.shouldOverrideUrlLoading(maliciousUri, maliciousUri, false) {}
+        val result = testee.shouldOverrideUrlLoading(maliciousUri, false) {}
         assertFalse(result)
     }
 
     @Test
     fun `shouldOverride returns true when feature is enabled, is malicious, and is mainframe`() = runTest {
         whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(MALICIOUS)
 
-        val result = testee.shouldOverrideUrlLoading(maliciousUri, maliciousUri, true) {}
+        val result = testee.shouldOverrideUrlLoading(maliciousUri, true) {}
         assertTrue(result)
     }
 
     @Test
     fun `shouldOverride returns false when feature is enabled, is malicious, and not mainframe nor iframe`() = runTest {
         whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(MALICIOUS)
 
-        val result = testee.shouldOverrideUrlLoading(maliciousUri, maliciousUri, false) {}
-        assertFalse(result)
-    }
-
-    @Test
-    fun `shouldOverride returns true when feature is enabled, is malicious, and is mainframe but webView has different host`() = runTest {
-        whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(MALICIOUS)
-
-        val result = testee.shouldOverrideUrlLoading(maliciousUri, exampleUri, true) {}
+        val result = testee.shouldOverrideUrlLoading(maliciousUri, false) {}
         assertFalse(result)
     }