Skip to content

Commit 3910261

Browse files
CrisBarreiroCrisBarreiro
committed
Fix duplicated pixels
1 parent acb2c6a commit 3910261

File tree

4 files changed

+111
-32
lines changed

4 files changed

+111
-32
lines changed

‎app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

+4
Original file line numberDiff line numberDiff line change
@@ -3210,12 +3210,16 @@ class BrowserTabViewModel @Inject constructor(
32103210
}
32113211

32123212
override fun onReceivedMaliciousSiteWarning(url: Uri, feed: Feed, exempted: Boolean, clientSideHit: Boolean) {
3213+
val previousSite = site
32133214
site = siteFactory.buildSite(url = url.toString(), tabId = tabId)
32143215
site?.maliciousSiteStatus = when (feed) {
32153216
MALWARE -> MaliciousSiteStatus.MALWARE
32163217
PHISHING -> MaliciousSiteStatus.PHISHING
32173218
}
3219+
32183220
if (!exempted) {
3221+
if (currentBrowserViewState().maliciousSiteDetected && previousSite?.url == url.toString()) return
3222+
Timber.tag("Cris").d("Received MaliciousSiteWarning for $url, feed: $feed, exempted: false, clientSideHit: $clientSideHit")
32193223
val params = mapOf(CATEGORY_KEY to feed.name.lowercase(), CLIENT_SIDE_HIT_KEY to clientSideHit.toString())
32203224
pixel.fire(AppPixelName.MALICIOUS_SITE_PROTECTION_ERROR_SHOWN, params)
32213225
loadingViewState.postValue(

‎app/src/main/java/com/duckduckgo/app/browser/WebViewRequestInterceptor.kt

+1-1
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ class WebViewRequestInterceptor(
9494

9595
override fun onPageStarted(url: String) {
9696
requestFilterer.registerOnPageCreated(url)
97-
maliciousSiteBlockerWebViewIntegration.onPageLoadStarted()
97+
maliciousSiteBlockerWebViewIntegration.onPageLoadStarted(url)
9898
}
9999

100100
/**

‎app/src/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt

+39-26
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ interface MaliciousSiteBlockerWebViewIntegration {
6363
confirmationCallback: (maliciousStatus: MaliciousStatus) -> Unit,
6464
): IsMaliciousViewData
6565

66-
fun onPageLoadStarted()
66+
fun onPageLoadStarted(url: String)
6767

6868
fun onSiteExempted(
6969
url: Uri,
@@ -103,7 +103,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
103103
) : MaliciousSiteBlockerWebViewIntegration, PrivacyConfigCallbackPlugin {
104104

105105
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
106-
val processedUrls = mutableListOf<String>()
106+
val processedUrls = mutableMapOf<String, MaliciousStatus>()
107107

108108
private var isFeatureEnabled = false
109109
private val isSettingEnabled: Boolean
@@ -140,6 +140,9 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
140140
if (!isEnabled()) {
141141
return IsMaliciousViewData.Safe
142142
}
143+
144+
Timber.d("shouldIntercept ${request.url}")
145+
143146
val url = request.url.let {
144147
if (it.fragment != null) {
145148
it.buildUpon().fragment(null).build()
@@ -150,19 +153,22 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
150153

151154
val decodedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
152155

153-
if (processedUrls.contains(decodedUrl)) {
154-
processedUrls.remove(decodedUrl)
155-
Timber.tag("PhishingAndMalwareDetector").d("Already intercepted, skipping $decodedUrl")
156-
return IsMaliciousViewData.Safe
157-
}
158-
159156
val exemptedUrl = exemptedUrlsHolder.exemptedMaliciousUrls.firstOrNull { it.url.toString() == decodedUrl }
160157

161158
if (exemptedUrl != null) {
162-
Timber.tag("MaliciousSiteDetector").d("Previously exempted, skipping $decodedUrl as ${exemptedUrl.feed}")
159+
Timber.d("Previously exempted, skipping $decodedUrl as ${exemptedUrl.feed}")
163160
return IsMaliciousViewData.MaliciousSite(url, exemptedUrl.feed, true)
164161
}
165162

163+
processedUrls[decodedUrl]?.let {
164+
processedUrls.remove(decodedUrl)
165+
Timber.d("Already intercepted, skipping $decodedUrl, status: $it")
166+
return when (it) {
167+
is Safe -> IsMaliciousViewData.Safe
168+
is Malicious -> IsMaliciousViewData.MaliciousSite(url, it.feed, false)
169+
}
170+
}
171+
166172
val belongsToCurrentPage = documentUri?.host == request.requestHeaders["Referer"]?.toUri()?.host
167173
val isForIframe = isForIframe(request) && belongsToCurrentPage
168174
if (request.isForMainFrame || isForIframe) {
@@ -174,6 +180,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
174180
}
175181
when (result) {
176182
is ConfirmedResult -> {
183+
processedUrls[decodedUrl] = result.status
177184
when (val status = result.status) {
178185
is Malicious -> {
179186
if (isForIframe) {
@@ -183,14 +190,12 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
183190
}
184191

185192
is Safe -> {
186-
processedUrls.add(decodedUrl)
187193
return IsMaliciousViewData.Safe
188194
}
189195
}
190196
}
191197

192198
is WaitForConfirmation -> {
193-
processedUrls.add(decodedUrl)
194199
return IsMaliciousViewData.WaitForConfirmation
195200
}
196201
}
@@ -207,21 +212,25 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
207212
if (!isEnabled()) {
208213
return@runBlocking IsMaliciousViewData.Safe
209214
}
215+
Timber.d("shouldOverrideUrlLoading $url")
210216
val decodedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
211217

212-
if (processedUrls.contains(decodedUrl)) {
213-
processedUrls.remove(decodedUrl)
214-
Timber.tag("PhishingAndMalwareDetector").d("Already intercepted, skipping $decodedUrl")
215-
return@runBlocking IsMaliciousViewData.Safe
216-
}
217-
218218
val exemptedUrl = exemptedUrlsHolder.exemptedMaliciousUrls.firstOrNull { it.url.toString() == decodedUrl }
219219

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

225+
processedUrls[decodedUrl]?.let {
226+
processedUrls.remove(decodedUrl)
227+
Timber.d("Already intercepted, skipping $decodedUrl, status: $it")
228+
return@runBlocking when (it) {
229+
is Safe -> IsMaliciousViewData.Safe
230+
is Malicious -> IsMaliciousViewData.MaliciousSite(url, it.feed, false)
231+
}
232+
}
233+
225234
// iframes always go through the shouldIntercept method, so we only need to check the main frame here
226235
if (isForMainFrame) {
227236
when (val result = checkMaliciousUrl(decodedUrl, confirmationCallback)) {
@@ -231,13 +240,12 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
231240
return@runBlocking IsMaliciousViewData.MaliciousSite(url, status.feed, false)
232241
}
233242
is Safe -> {
234-
processedUrls.add(decodedUrl)
243+
processedUrls[decodedUrl] = Safe
235244
return@runBlocking IsMaliciousViewData.Safe
236245
}
237246
}
238247
}
239248
is WaitForConfirmation -> {
240-
processedUrls.add(decodedUrl)
241249
return@runBlocking IsMaliciousViewData.WaitForConfirmation
242250
}
243251
}
@@ -262,7 +270,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
262270
} else {
263271
Safe
264272
}
265-
processedUrls.clear()
273+
processedUrls[url] = isMalicious
266274
confirmationCallback(isMalicious)
267275
}
268276
}
@@ -276,8 +284,15 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
276284
return isFeatureEnabled && isSettingEnabled
277285
}
278286

279-
override fun onPageLoadStarted() {
280-
processedUrls.clear()
287+
override fun onPageLoadStarted(url: String) {
288+
val convertedUrl = URLDecoder.decode(url, "UTF-8").lowercase()
289+
/* onPageLoadStarted is often called after shouldOverride/shouldIntercept, therefore, if the URL
290+
* is already stored, we don't clear the processedUrls map to avoid re-checking the URL for the same
291+
* page load.
292+
*/
293+
if (!processedUrls.contains(convertedUrl)) {
294+
processedUrls.clear()
295+
}
281296
}
282297

283298
override fun onSiteExempted(
@@ -286,8 +301,6 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
286301
) {
287302
val convertedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
288303
exemptedUrlsHolder.addExemptedMaliciousUrl(ExemptedUrl(convertedUrl.toUri(), feed))
289-
Timber.tag("MaliciousSiteDetector").d(
290-
"Added $url to exemptedUrls, contents: ${exemptedUrlsHolder.exemptedMaliciousUrls}",
291-
)
304+
Timber.d("Added $url to exemptedUrls")
292305
}
293306
}

‎app/src/test/java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegrationTest.kt

+67-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.IsMali
2020
import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.MaliciousStatus
2121
import com.duckduckgo.malicioussiteprotection.api.MaliciousSiteProtection.MaliciousStatus.Malicious
2222
import junit.framework.TestCase.assertEquals
23+
import junit.framework.TestCase.assertFalse
2324
import junit.framework.TestCase.assertTrue
2425
import kotlinx.coroutines.CompletableDeferred
2526
import kotlinx.coroutines.channels.Channel
@@ -31,6 +32,8 @@ import org.junit.Test
3132
import org.junit.runner.RunWith
3233
import org.mockito.Mockito.mock
3334
import org.mockito.kotlin.any
35+
import org.mockito.kotlin.eq
36+
import org.mockito.kotlin.times
3437
import org.mockito.kotlin.verify
3538
import org.mockito.kotlin.whenever
3639

@@ -102,13 +105,21 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
102105
}
103106

104107
@Test
105-
fun `shouldOverrideUrlLoading returns safe when url is already processed`() = runTest {
106-
testee.processedUrls.add(exampleUri.toString())
108+
fun `shouldOverrideUrlLoading returns safe when url is already processed and safe`() = runTest {
109+
testee.processedUrls[exampleUri.toString()] = MaliciousStatus.Safe
107110

108111
val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
109112
assertEquals(Safe, result)
110113
}
111114

115+
@Test
116+
fun `shouldOverrideUrlLoading returns malicious when url is already processed and malicious`() = runTest {
117+
testee.processedUrls[exampleUri.toString()] = Malicious(MALWARE)
118+
119+
val result = testee.shouldOverrideUrlLoading(exampleUri, true) {}
120+
assertEquals(MaliciousSite(exampleUri, MALWARE, false), result)
121+
}
122+
112123
@Test
113124
fun `shouldInterceptRequest returns result when feature is enabled, setting is enabled, is malicious, and is mainframe`() = runTest {
114125
val request = mock(WebResourceRequest::class.java)
@@ -120,6 +131,32 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
120131
assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result)
121132
}
122133

134+
@Test
135+
fun `shouldInterceptRequest returns safe when url is already processed and safe`() = runTest {
136+
testee.processedUrls[exampleUri.toString()] = MaliciousStatus.Safe
137+
val request = mock(WebResourceRequest::class.java)
138+
whenever(request.url).thenReturn(exampleUri)
139+
whenever(request.isForMainFrame).thenReturn(true)
140+
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))
141+
142+
val result = testee.shouldIntercept(request, maliciousUri) {}
143+
144+
assertEquals(Safe, result)
145+
}
146+
147+
@Test
148+
fun `shouldInterceptRequest returns malicious when url is already processed and malicious`() = runTest {
149+
testee.processedUrls[exampleUri.toString()] = Malicious(MALWARE)
150+
val request = mock(WebResourceRequest::class.java)
151+
whenever(request.url).thenReturn(exampleUri)
152+
whenever(request.isForMainFrame).thenReturn(true)
153+
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))
154+
155+
val result = testee.shouldIntercept(request, maliciousUri) {}
156+
157+
assertEquals(MaliciousSite(exampleUri, MALWARE, false), result)
158+
}
159+
123160
@Test
124161
fun `shouldInterceptRequest returns result when feature is enabled, setting is enabled, is malicious, and is iframe`() = runTest {
125162
val request = mock(WebResourceRequest::class.java)
@@ -180,12 +217,19 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
180217
}
181218

182219
@Test
183-
fun `onPageLoadStarted clears processedUrls`() = runTest {
184-
testee.processedUrls.add(exampleUri.toString())
185-
testee.onPageLoadStarted()
220+
fun `onPageLoadStarted with different URL clears processedUrls`() = runTest {
221+
testee.processedUrls.put(exampleUri.toString(), MaliciousStatus.Safe)
222+
testee.onPageLoadStarted("http://another.com")
186223
assertTrue(testee.processedUrls.isEmpty())
187224
}
188225

226+
@Test
227+
fun `onPageLoadStarted with same URL does not clear processedUrls`() = runTest {
228+
testee.processedUrls.put(exampleUri.toString(), MaliciousStatus.Safe)
229+
testee.onPageLoadStarted(exampleUri.toString())
230+
assertFalse(testee.processedUrls.isEmpty())
231+
}
232+
189233
@Test
190234
fun `if a new page load triggering is malicious is started, isMalicious callback result should be ignored for the first page`() = runTest {
191235
val request = mock(WebResourceRequest::class.java)
@@ -294,6 +338,24 @@ class RealMaliciousSiteBlockerWebViewIntegrationTest {
294338
assertEquals(MaliciousSite(maliciousUri, MALWARE, true), result)
295339
}
296340

341+
@Test
342+
fun `shouldOverride called several times with same iframe URL without onPageStarted only fires pixel once`() = runTest {
343+
val request = mock(WebResourceRequest::class.java)
344+
whenever(request.url).thenReturn(maliciousUri)
345+
whenever(request.isForMainFrame).thenReturn(false)
346+
whenever(request.requestHeaders).thenReturn(mapOf("Sec-Fetch-Dest" to "iframe", "Referer" to maliciousUri.toString()))
347+
whenever(maliciousSiteProtection.isMalicious(any(), any())).thenReturn(ConfirmedResult(Malicious(MALWARE)))
348+
349+
val result = testee.shouldIntercept(request, maliciousUri) {}
350+
assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result)
351+
352+
val result2 = testee.shouldIntercept(request, maliciousUri) {}
353+
verify(mockPixel, times(1)).fire(AppPixelName.MALICIOUS_SITE_DETECTED_IN_IFRAME, mapOf("category" to MALWARE.name.lowercase()))
354+
verify(maliciousSiteProtection, times(1)).isMalicious(eq(maliciousUri), any())
355+
356+
assertEquals(MaliciousSite(maliciousUri, MALWARE, false), result2)
357+
}
358+
297359
private fun updateFeatureEnabled(enabled: Boolean) {
298360
fakeAndroidBrowserConfigFeature.enableMaliciousSiteProtection().setRawStoredState(State(enabled))
299361
testee.onPrivacyConfigDownloaded()

0 commit comments

Comments
 (0)