Add feed to isMalicious return type and update error page with feed info #5564

CrisBarreiro · 2025-01-30T17:17:14Z

Task/Issue URL: https://app.asana.com/0/1205008441501016/1209270355529418/f

Description

Add feed to isMalicious return type and update error page with feed info

Steps to test this PR

Feature 1

Load https://privacy-test-pages.site/security/badware/phishing.html
Check the error message is "This website may be impersonating a legitimate site in order to trick you into providing personal information, such as passwords or credit card numbers."

Feature 2

Load https://privacy-test-pages.site/security/badware/malware.html
Check the error message is "DuckDuckGo blocked this page because it may be distributing malware designed to compromise your device or steal your personal information."

UI changes

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

CrisBarreiro · 2025-01-30T17:17:37Z

Scope exemptions to tab #5585
Add feed to isMalicious return type and update error page with feed info #5564 👈 (View in Graphite)
Display warning page for malicious sites #5416
Add blocking algorithm #5370 : 2 other dependent PRs (#5376 , #5510 )
Intercept requests and add skeleton for malicious site detection #5369
develop

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

cmonfortep

LGTM
just few comments, nothin blocking.
Test cases worked fine. I've found an issue but unsure if related to changes here, so will share directly in Asana to triage.

cmonfortep · 2025-02-04T09:39:51Z

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

+            PHISHING -> MaliciousSiteStatus.PHISHING
+        }
+        if (!exempted) {
+            loadingViewState.postValue(currentLoadingViewState().copy(isLoading = false, url = url.toString()))


I've noticed we dropped progress = 100 in the diff. Intended?

cmonfortep · 2025-02-04T10:00:29Z

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

+        url: Uri?,
+    ) {
+        if (isMalicious is Malicious) {
+            handleSiteBlocked(webViewClientListener, url, isMalicious.feed, false)


So, if I get this right, we have false here as exempted because this is the async callback. And we should assume the synchronous logic already checks if it's exempted, so any exempted url will never land in the async callback. Can we leave a comment about that in this line?

The second question, it's to validate why we need to pass it down. I assume if a site is exempted, we don't block, but we want to update the omnibar logo, right?

cmonfortep · 2025-02-04T11:15:36Z

...ain/kotlin/com/duckduckgo/malicioussiteprotection/impl/domain/RealMaliciousSiteProtection.kt

+            filterSet.filters.firstOrNull {
+                Pattern.compile(it.regex).matcher(url.toString()).find()
+            }?.let {
+                Timber.d("\uD83D\uDFE2 Cris: shouldBlock $url")


NIT: do we have a Tag/Prefix you want to use from now on for logs? Probably better to start using it now, in case we forget to clean the logs with Cris.

Maybe just use MaliciousSiteDetector or MaliciousSite`?

cmonfortep · 2025-02-04T11:15:41Z

...ain/kotlin/com/duckduckgo/malicioussiteprotection/impl/domain/RealMaliciousSiteProtection.kt

+                val result = matches(hashPrefix, url, hostname, hash)?.let { feed: Feed ->
+                    Malicious(feed)
+                } ?: Safe
+                confirmationCallback(result)
            } catch (e: Exception) {
                Timber.e(e, "\uD83D\uDD34 Cris: shouldBlock $url")


cmonfortep · 2025-02-04T11:17:09Z

...main/kotlin/com/duckduckgo/malicioussiteprotection/impl/data/network/MaliciousSiteService.kt

@@ -78,6 +78,7 @@ data class MatchResponse(
    val url: String,
    val regex: String,
    val hash: String,
+    val category: String,


Why do we call them Feed but category here? which one is how we refer to these in the objective?

cmonfortep · 2025-02-04T11:24:30Z

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

@@ -197,9 +242,12 @@ class RealMaliciousSiteBlockerWebViewIntegration @Inject constructor(
        processedUrls.clear()
    }

-    override fun onSiteExempted(url: Uri) {
+    override fun onSiteExempted(


Can we add some test to this method? it seems they are missing. I was wondering about for example: what if url is not a valid URI?

cmonfortep · 2025-02-04T11:26:40Z

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

        val convertedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
-        exemptedUrlsHolder.exemptedMaliciousUrls.add(convertedUrl)
+        exemptedUrlsHolder.exemptedMaliciousUrls.add(ExemptedUrl(convertedUrl.toUri(), feed))


why receiving an url: Uri -> decoding to Url -> and converting to Uri again?

cmonfortep · 2025-02-04T11:28:38Z

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

 @SingleInstanceIn(AppScope::class)
 class ExemptedUrlsHolder @Inject constructor() {
-    val exemptedMaliciousUrls = mutableSetOf<String>()
+    val exemptedMaliciousUrls = mutableSetOf<ExemptedUrl>()


When checking how we use this, I don't see we ever check feed, only url. Do we have plans to differentiate exemptedUrls per feed?

CrisBarreiro mentioned this pull request Jan 30, 2025

Display warning page for malicious sites #5416

Merged

50 tasks

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch from b6d3050 to 31ef2ac Compare January 31, 2025 09:50

CrisBarreiro marked this pull request as ready for review January 31, 2025 10:04

CrisBarreiro requested a review from cmonfortep January 31, 2025 10:05

CrisBarreiro assigned cmonfortep Jan 31, 2025

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch 3 times, most recently from 37a2b10 to d297727 Compare January 31, 2025 11:29

Base automatically changed from feature/cris/malicious-site-protection/error-page to develop January 31, 2025 11:39

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch from d297727 to 31c9560 Compare January 31, 2025 12:02

CrisBarreiro added 4 commits February 3, 2025 15:20

Add feed to isMalicious return type and integrate with error page

f25f3e9

Allow matches to recover from faulty category

175738b

Fix tests

e6791ee

Fix failing tests

a2affdd

CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch from 3948aaa to a2affdd Compare February 3, 2025 15:29

CrisBarreiro mentioned this pull request Feb 3, 2025

Scope exemptions to tab #5585

Open

5 tasks

cmonfortep approved these changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feed to isMalicious return type and update error page with feed info #5564

Add feed to isMalicious return type and update error page with feed info #5564

CrisBarreiro commented Jan 30, 2025 •

edited by cmonfortep

Loading

CrisBarreiro commented Jan 30, 2025 •

edited

Loading

cmonfortep left a comment

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

cmonfortep Feb 4, 2025

Add feed to isMalicious return type and update error page with feed info #5564

Are you sure you want to change the base?

Add feed to isMalicious return type and update error page with feed info #5564

Conversation

CrisBarreiro commented Jan 30, 2025 • edited by cmonfortep Loading

Description

Steps to test this PR

UI changes

CrisBarreiro commented Jan 30, 2025 • edited Loading

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

CrisBarreiro commented Jan 30, 2025 •

edited by cmonfortep

Loading

CrisBarreiro commented Jan 30, 2025 •

edited

Loading