Add Malicious Site Protection algorithm pixels #5604

CrisBarreiro · 2025-02-06T09:40:56Z

Task/Issue URL: https://app.asana.com/0/488551667048375/1209304069660515

Description

Add Malicious Site Protection algorithm pixels

Steps to test this PR

Feature 1

Load https://privacy-test-pages.site/security/badware/phishing-legit-iframe-loader.html
Check m_malicious-site-protection_iframe-loaded with params: {category=phishing} is fired
Check m_malicious-site-protection_error-page-shown is fired

Feature 2

Set timeout to 10ms in override suspend fun matches(hashPrefix: String): List
Load https://privacy-test-pages.site/security/badware/phishing-legit-iframe-loader.html
Check no error page is shown and m_malicious-site-protection_client-timeout is fired

Feature 3

Load https://privacy-test-pages.site/security/badware/
Click on "Phishing iFrame Loader"
Check m_malicious-site-protection_iframe-loaded and m_malicious-site-protection_error-page-shown are fired only once

UI changes

Before	After
!(Upload before screenshot)	(Upload after screenshot)

CrisBarreiro · 2025-02-06T09:41:17Z

Add Malicious Site Protection algorithm pixels #5604 👈 (View in Graphite)
Integrate privacy dashboard #5624 : 1 other dependent PR (#5626 )
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

cmonfortep

Code looks good, left a note in Asana about testing.

cmonfortep

LGTM

Few comments, and I see also a bunch of logs that not sure if we want to keep, but we should use the right tag for consistency. (I've commented in some of them, but there are a more).

cmonfortep · 2025-02-28T10:37:33Z

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

@@ -243,7 +270,7 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
            } else {
                Safe
            }
-            processedUrls.clear()
+            processedUrls[url] = isMalicious


We have a previous check if (checkId == currentCheckId.get()) that validates this is related to the latest site trying to load. If we detect another site has started loading, we return safe to avoid showing the phishing page in the wrong context. However, we are assigning Safe to possible phishing pages here. I believe the right value here is processedUrls[url] = it.

That makes a lot of sense, thanks!

cmonfortep · 2025-02-28T10:40:44Z

app/src/main/java/com/duckduckgo/app/pixels/AppPixelName.kt

@@ -397,4 +397,6 @@ enum class AppPixelName(override val pixelName: String) : Pixel.PixelName {
    SET_AS_DEFAULT_PROMPT_CLICK("m_set-as-default_prompt_click"),
    SET_AS_DEFAULT_PROMPT_DISMISSED("m_set-as-default_prompt_dismissed"),
    SET_AS_DEFAULT_IN_MENU_CLICK("m_set-as-default_in-menu_click"),
+
+    MALICIOUS_SITE_DETECTED_IN_IFRAME("m_malicious-site-protection_iframe-loaded"),


why is this one here if we have MaliciousSitePixelName? this should be inside malicious module.

Because MaliciousSitePixelName belongs to the MSP implementation module, and the MSP module knows nothing about iframes, that happens on the browser level

cmonfortep · 2025-02-28T10:44:14Z

.../src/main/kotlin/com/duckduckgo/malicioussiteprotection/impl/data/MaliciousSiteRepository.kt

-                    Match(it.hostname, it.url, it.regex, it.hash, feed)
-                } else {
-                    null
+            withTimeout(1000) {


I haven't found why 1000 is the timeout here in the pixel definition, or in Asana. Do we have it somewhere? would be good to move it into a const, and leave a comment or link with context.

@cmonfortep, I believe this was discussed on a meeting. We recently created a task to document timeouts and it looks like all other platforms are using a different value, so I'll probably need to update this one. We also want to update the timeout for dataset downloads, and I've created a task for it, so for now I'm just extracting it to a constant. I've asked whether we want to update this timeout in particular to 5s, so in case the answer is yes, I'll use the task for dataset downloads timeout to also update this one

cmonfortep · 2025-02-28T10:45:15Z

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

        if (!exempted) {
+            if (currentBrowserViewState().maliciousSiteDetected && previousSite?.url == url.toString()) return
+            Timber.tag("Cris").d("Received MaliciousSiteWarning for $url, feed: $feed, exempted: false, clientSideHit: $clientSideHit")


if you want to keep this log, we should use a the right tag.

I removed the tag, since we don't seem to be setting any specific one for this class

cmonfortep · 2025-02-28T10:45:31Z

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

@@ -137,6 +140,9 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
        if (!isEnabled()) {
            return IsMaliciousViewData.Safe
        }
+
+        Timber.d("shouldIntercept ${request.url}")


cmonfortep · 2025-02-28T10:45:49Z

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

        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}")


I see we've removed tag, not sure if intentional

Yes, similar to #5604 (comment), we don't use tag for other logs in this class

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch from 2592f74 to 39aa159 Compare February 6, 2025 12:20

CrisBarreiro requested a review from cmonfortep February 6, 2025 12:23

CrisBarreiro assigned cmonfortep Feb 6, 2025

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch 3 times, most recently from 3d36dce to 79c5789 Compare February 10, 2025 16:52

CrisBarreiro changed the base branch from develop to feature/cris/malicious-site-protection/do-not-auto-focus-omnibar-on-resume February 10, 2025 16:52

This was referenced Feb 10, 2025

Do not autofocus omnibar on error page #5626

Merged

Integrate privacy dashboard #5624

Merged

Fix back navigation when site blocked before added to webview history #5633

Merged

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/do-not-auto-focus-omnibar-on-resume branch from 7001024 to 97b9ac5 Compare February 11, 2025 18:10

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch from 79c5789 to 40577d8 Compare February 11, 2025 18:11

Base automatically changed from feature/cris/malicious-site-protection/do-not-auto-focus-omnibar-on-resume to feature/cris/malicious-site-protection/integrate-privacy-dashboard-2 February 11, 2025 18:11

Base automatically changed from feature/cris/malicious-site-protection/integrate-privacy-dashboard-2 to develop February 12, 2025 07:39

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch from 40577d8 to 3cff846 Compare February 13, 2025 14:26

cmonfortep reviewed Feb 20, 2025

View reviewed changes

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch 3 times, most recently from 3910261 to d3e8362 Compare February 21, 2025 17:07

cmonfortep approved these changes Feb 28, 2025

View reviewed changes

CrisBarreiro added 3 commits March 3, 2025 09:29

Add algorithm pixels

d08d3b8

Fix duplicated pixels

0cf5b0a

Address comments from PR

991e53b

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-algorithm-pixels branch from d3e8362 to 991e53b Compare March 3, 2025 09:01

CrisBarreiro merged commit 2796ef7 into develop Mar 3, 2025
6 checks passed

CrisBarreiro deleted the feature/cris/malicious-site-protection/add-algorithm-pixels branch March 3, 2025 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Malicious Site Protection algorithm pixels #5604

Add Malicious Site Protection algorithm pixels #5604

CrisBarreiro commented Feb 6, 2025 •

edited by cmonfortep

Loading

CrisBarreiro commented Feb 6, 2025 •

edited

Loading

cmonfortep left a comment

cmonfortep left a comment

cmonfortep Feb 28, 2025

CrisBarreiro Mar 3, 2025

cmonfortep Feb 28, 2025

CrisBarreiro Mar 3, 2025

cmonfortep Feb 28, 2025

CrisBarreiro Mar 3, 2025

cmonfortep Feb 28, 2025

CrisBarreiro Mar 3, 2025

cmonfortep Feb 28, 2025

cmonfortep Feb 28, 2025

CrisBarreiro Mar 3, 2025

Add Malicious Site Protection algorithm pixels #5604

Add Malicious Site Protection algorithm pixels #5604

Conversation

CrisBarreiro commented Feb 6, 2025 • edited by cmonfortep Loading

Description

Steps to test this PR

UI changes

CrisBarreiro commented Feb 6, 2025 • edited Loading

cmonfortep left a comment

Choose a reason for hiding this comment

cmonfortep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrisBarreiro commented Feb 6, 2025 •

edited by cmonfortep

Loading

CrisBarreiro commented Feb 6, 2025 •

edited

Loading