Skip to content

Commit

Permalink
Fix duplicated pixels
Browse files Browse the repository at this point in the history
  • Loading branch information
@CrisBarreiro
CrisBarreiro committed Feb 21, 2025
1 parent acb2c6a commit d3e8362
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 33 deletions.
2 changes: 1 addition & 1 deletion ...c/androidTest/java/com/duckduckgo/app/fakes/FakeMaliciousSiteBlockerWebViewIntegration.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class FakeMaliciousSiteBlockerWebViewIntegration : MaliciousSiteBlockerWebViewIn
return Safe
}

override fun onPageLoadStarted() {
override fun onPageLoadStarted(url: String) {
// no-op
}

Expand Down
4 changes: 4 additions & 0 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3210,12 +3210,16 @@ class BrowserTabViewModel @Inject constructor(
}

override fun onReceivedMaliciousSiteWarning(url: Uri, feed: Feed, exempted: Boolean, clientSideHit: Boolean) {
val previousSite = site
site = siteFactory.buildSite(url = url.toString(), tabId = tabId)
site?.maliciousSiteStatus = when (feed) {
MALWARE -> MaliciousSiteStatus.MALWARE
PHISHING -> MaliciousSiteStatus.PHISHING
}

if (!exempted) {
if (currentBrowserViewState().maliciousSiteDetected && previousSite?.url == url.toString()) return
Timber.tag("Cris").d("Received MaliciousSiteWarning for $url, feed: $feed, exempted: false, clientSideHit: $clientSideHit")
val params = mapOf(CATEGORY_KEY to feed.name.lowercase(), CLIENT_SIDE_HIT_KEY to clientSideHit.toString())
pixel.fire(AppPixelName.MALICIOUS_SITE_PROTECTION_ERROR_SHOWN, params)
loadingViewState.postValue(
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/browser/WebViewRequestInterceptor.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class WebViewRequestInterceptor(

override fun onPageStarted(url: String) {
requestFilterer.registerOnPageCreated(url)
maliciousSiteBlockerWebViewIntegration.onPageLoadStarted()
maliciousSiteBlockerWebViewIntegration.onPageLoadStarted(url)
}

/**
Expand Down
65 changes: 39 additions & 26 deletions ...rc/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ interface MaliciousSiteBlockerWebViewIntegration {
confirmationCallback: (maliciousStatus: MaliciousStatus) -> Unit,
): IsMaliciousViewData

fun onPageLoadStarted()
fun onPageLoadStarted(url: String)

fun onSiteExempted(
url: Uri,
Expand Down Expand Up @@ -103,7 +103,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
) : MaliciousSiteBlockerWebViewIntegration, PrivacyConfigCallbackPlugin {

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
val processedUrls = mutableListOf<String>()
val processedUrls = mutableMapOf<String, MaliciousStatus>()

private var isFeatureEnabled = false
private val isSettingEnabled: Boolean
Expand Down Expand Up @@ -140,6 +140,9 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
if (!isEnabled()) {
return IsMaliciousViewData.Safe
}

Timber.d("shouldIntercept ${request.url}")

val url = request.url.let {
if (it.fragment != null) {
it.buildUpon().fragment(null).build()
Expand All @@ -150,19 +153,22 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(

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 IsMaliciousViewData.Safe
}

val exemptedUrl = exemptedUrlsHolder.exemptedMaliciousUrls.firstOrNull { it.url.toString() == decodedUrl }

if (exemptedUrl != null) {
Timber.tag("MaliciousSiteDetector").d("Previously exempted, skipping $decodedUrl as ${exemptedUrl.feed}")
Timber.d("Previously exempted, skipping $decodedUrl as ${exemptedUrl.feed}")
return IsMaliciousViewData.MaliciousSite(url, exemptedUrl.feed, true)
}

processedUrls[decodedUrl]?.let {
processedUrls.remove(decodedUrl)
Timber.d("Already intercepted, skipping $decodedUrl, status: $it")
return when (it) {
is Safe -> IsMaliciousViewData.Safe
is Malicious -> IsMaliciousViewData.MaliciousSite(url, it.feed, false)
}
}

val belongsToCurrentPage = documentUri?.host == request.requestHeaders["Referer"]?.toUri()?.host
val isForIframe = isForIframe(request) && belongsToCurrentPage
if (request.isForMainFrame || isForIframe) {
Expand All @@ -174,6 +180,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
}
when (result) {
is ConfirmedResult -> {
processedUrls[decodedUrl] = result.status
when (val status = result.status) {
is Malicious -> {
if (isForIframe) {
Expand All @@ -183,14 +190,12 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
}

is Safe -> {
processedUrls.add(decodedUrl)
return IsMaliciousViewData.Safe
}
}
}

is WaitForConfirmation -> {
processedUrls.add(decodedUrl)
return IsMaliciousViewData.WaitForConfirmation
}
}
Expand All @@ -207,21 +212,25 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
if (!isEnabled()) {
return@runBlocking IsMaliciousViewData.Safe
}
Timber.d("shouldOverrideUrlLoading $url")
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@runBlocking IsMaliciousViewData.Safe
}

val exemptedUrl = exemptedUrlsHolder.exemptedMaliciousUrls.firstOrNull { it.url.toString() == decodedUrl }

if (exemptedUrl != null) {
Timber.tag("MaliciousSiteDetector").d("Previously exempted, skipping $decodedUrl")
Timber.d("Previously exempted, skipping $decodedUrl")
return@runBlocking IsMaliciousViewData.MaliciousSite(url, exemptedUrl.feed, true)
}

processedUrls[decodedUrl]?.let {
processedUrls.remove(decodedUrl)
Timber.d("Already intercepted, skipping $decodedUrl, status: $it")
return@runBlocking when (it) {
is Safe -> IsMaliciousViewData.Safe
is Malicious -> IsMaliciousViewData.MaliciousSite(url, it.feed, false)
}
}

// iframes always go through the shouldIntercept method, so we only need to check the main frame here
if (isForMainFrame) {
when (val result = checkMaliciousUrl(decodedUrl, confirmationCallback)) {
Expand All @@ -231,13 +240,12 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
return@runBlocking IsMaliciousViewData.MaliciousSite(url, status.feed, false)
}
is Safe -> {
processedUrls.add(decodedUrl)
processedUrls[decodedUrl] = Safe
return@runBlocking IsMaliciousViewData.Safe
}
}
}
is WaitForConfirmation -> {
processedUrls.add(decodedUrl)
return@runBlocking IsMaliciousViewData.WaitForConfirmation
}
}
Expand All @@ -262,7 +270,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
} else {
Safe
}
processedUrls.clear()
processedUrls[url] = isMalicious
confirmationCallback(isMalicious)
}
}
Expand All @@ -276,8 +284,15 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
return isFeatureEnabled && isSettingEnabled
}

override fun onPageLoadStarted() {
processedUrls.clear()
override fun onPageLoadStarted(url: String) {
val convertedUrl = URLDecoder.decode(url, "UTF-8").lowercase()
/* onPageLoadStarted is often called after shouldOverride/shouldIntercept, therefore, if the URL
* is already stored, we don't clear the processedUrls map to avoid re-checking the URL for the same
* page load.
*/
if (!processedUrls.contains(convertedUrl)) {
processedUrls.clear()
}
}

override fun onSiteExempted(
Expand All @@ -286,8 +301,6 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
) {
val convertedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
exemptedUrlsHolder.addExemptedMaliciousUrl(ExemptedUrl(convertedUrl.toUri(), feed))
Timber.tag("MaliciousSiteDetector").d(
"Added $url to exemptedUrls, contents: ${exemptedUrlsHolder.exemptedMaliciousUrls}",
)
Timber.d("Added $url to exemptedUrls")
}
}
72 changes: 67 additions & 5 deletions ...java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegrationTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.IsMali
import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.MaliciousStatus
import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.MaliciousStatus.Malicious
import junit.framework.TestCase.assertEquals
import junit.framework.TestCase.assertFalse
import junit.framework.TestCase.assertTrue
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.channels.Channel
Expand All @@ -31,6 +32,8 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.mock
import org.mockito.kotlin.any
import org.mockito.kotlin.eq
import org.mockito.kotlin.times
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever

Expand Down Expand Up @@ -102,13 +105,21 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
}

@Test
fun `shouldOverrideUrlLoading returns safe when url is already processed`() = runTest {
testee.processedUrls.add(exampleUri.toString())
fun `shouldOverrideUrlLoading returns safe when url is already processed and safe`() = runTest {
testee.processedUrls[exampleUri.toString()] = MaliciousStatus.Safe

val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
assertEquals(Safe, result)
}

@Test
fun `shouldOverrideUrlLoading returns malicious when url is already processed and malicious`() = runTest {
testee.processedUrls[exampleUri.toString()] = Malicious(MALWARE)

val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
assertEquals(MaliciousSite(exampleUri, MALWARE, false), result)
}

@Test
fun `shouldInterceptRequest returns result when feature is enabled, setting is enabled, is malicious, and is mainframe`() = runTest {
val request = mock(WebResourceRequest::class.java)
Expand All @@ -120,6 +131,32 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result)
}

@Test
fun `shouldInterceptRequest returns safe when url is already processed and safe`() = runTest {
testee.processedUrls[exampleUri.toString()] = MaliciousStatus.Safe
val request = mock(WebResourceRequest::class.java)
whenever(request.url).thenReturn(exampleUri)
whenever(request.isForMainFrame).thenReturn(true)
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))

val result = testee.shouldIntercept(request, maliciousUri) {}

assertEquals(Safe, result)
}

@Test
fun `shouldInterceptRequest returns malicious when url is already processed and malicious`() = runTest {
testee.processedUrls[exampleUri.toString()] = Malicious(MALWARE)
val request = mock(WebResourceRequest::class.java)
whenever(request.url).thenReturn(exampleUri)
whenever(request.isForMainFrame).thenReturn(true)
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))

val result = testee.shouldIntercept(request, maliciousUri) {}

assertEquals(MaliciousSite(exampleUri, MALWARE, false), result)
}

@Test
fun `shouldInterceptRequest returns result when feature is enabled, setting is enabled, is malicious, and is iframe`() = runTest {
val request = mock(WebResourceRequest::class.java)
Expand Down Expand Up @@ -180,12 +217,19 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
}

@Test
fun `onPageLoadStarted clears processedUrls`() = runTest {
testee.processedUrls.add(exampleUri.toString())
testee.onPageLoadStarted()
fun `onPageLoadStarted with different URL clears processedUrls`() = runTest {
testee.processedUrls.put(exampleUri.toString(), MaliciousStatus.Safe)
testee.onPageLoadStarted("http://another.com")
assertTrue(testee.processedUrls.isEmpty())
}

@Test
fun `onPageLoadStarted with same URL does not clear processedUrls`() = runTest {
testee.processedUrls.put(exampleUri.toString(), MaliciousStatus.Safe)
testee.onPageLoadStarted(exampleUri.toString())
assertFalse(testee.processedUrls.isEmpty())
}

@Test
fun `if a new page load triggering is malicious is started, isMalicious callback result should be ignored for the first page`() = runTest {
val request = mock(WebResourceRequest::class.java)
Expand Down Expand Up @@ -294,6 +338,24 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
assertEquals(MaliciousSite(maliciousUri, MALWARE, true), result)
}

@Test
fun `shouldOverride called several times with same iframe URL without onPageStarted only fires pixel once`() = runTest {
val request = mock(WebResourceRequest::class.java)
whenever(request.url).thenReturn(maliciousUri)
whenever(request.isForMainFrame).thenReturn(false)
whenever(request.requestHeaders).thenReturn(mapOf("Sec-Fetch-Dest" to "iframe", "Referer" to maliciousUri.toString()))
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))

val result = testee.shouldIntercept(request, maliciousUri) {}
assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result)

val result2 = testee.shouldIntercept(request, maliciousUri) {}
verify(mockPixel, times(1)).fire(AppPixelName.MALICIOUS_SITE_DETECTED_IN_IFRAME, mapOf("category" to MALWARE.name.lowercase()))
verify(maliciousSiteProtection, times(1)).isMalicious(eq(maliciousUri), any())

assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result2)
}

private fun updateFeatureEnabled(enabled: Boolean) {
fakeAndroidBrowserConfigFeature.enableMaliciousSiteProtection().setRawStoredState(State(enabled))
testee.onPrivacyConfigDownloaded()
Expand Down

0 comments on commit d3e8362

Please sign in to comment.