Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Feb 4, 2025
Merged

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

merged 2 commits into from
Feb 4, 2025
+480 −204

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Jan 30, 2025 •
edited by cmonfortep
Loading

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

Feature 2

UI changes

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

@CrisBarreiro CrisBarreiro mentioned this pull request Jan 30, 2025
Merged
50 tasks
@CrisBarreiro Graphite App
Copy link
Contributor Author

CrisBarreiro commented Jan 30, 2025 •
edited
Loading

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

@CrisBarreiro 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 CrisBarreiro marked this pull request as ready for review January 31, 2025 10:04
@CrisBarreiro 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 CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch 2 times, most recently from 3948aaa to a2affdd Compare February 3, 2025 15:29
@CrisBarreiro CrisBarreiro mentioned this pull request Feb 3, 2025
Merged
5 tasks
cmonfortep
cmonfortep approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/com/duckduckgo/app/browser/WebViewRequestInterceptor.kt Show resolved Hide resolved
...ain/kotlin/com/duckduckgo/malicioussiteprotection/impl/domain/RealMaliciousSiteProtection.kt Outdated Show resolved Hide resolved
...ain/kotlin/com/duckduckgo/malicioussiteprotection/impl/domain/RealMaliciousSiteProtection.kt Outdated Show resolved Hide resolved
...main/kotlin/com/duckduckgo/malicioussiteprotection/impl/data/network/MaliciousSiteService.kt Outdated Show resolved Hide resolved
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add some tests, but we don't do any validation. If the Uri is invalid, nothing will break, but it'll never get matched either. All URLs used in this case come from the WebView though, so they won't be invalid, and I don't think we should do validation in every single step of the way, as it would be terrible for performance.

app/src/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt Outdated
val convertedUrl = URLDecoder.decode(url.toString(), "UTF-8").lowercase()
exemptedUrlsHolder.exemptedMaliciousUrls.add(convertedUrl)
exemptedUrlsHolder.exemptedMaliciousUrls.add(ExemptedUrl(convertedUrl.toUri(), feed))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to decode so we can match 2 URLs that are technically the same, but encoded differently. We could technically then store it as String, but after my experience working in performance, I prefer to use Uri whenever possible, and only rely on strings when necessary, as it's the case here

app/src/main/java/com/duckduckgo/app/browser/webview/MaliciousSiteBlockerWebViewIntegration.kt Outdated
@SingleInstanceIn(AppScope::class)
class ExemptedUrlsHolder @Inject constructor() {
val exemptedMaliciousUrls = mutableSetOf<String>()
val exemptedMaliciousUrls = mutableSetOf<ExemptedUrl>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't check it here, but we need to be able to return both the URL and the feed, so that we can then setup the privacy dashboard for exempted sites, as the message shown is different depending on the feed that flagged the site

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch 2 times, most recently from 0e7fda2 to 7734cc1 Compare February 4, 2025 12:57
@CrisBarreiro
Add feed to isMalicious return type and integrate with error page (+9…
dc38d8c 
… squashed commits)

Squashed commits:
[7734cc1] Scope exemptions to tab
[7590760] Rename category to feed in network model
[c37e012] Address PR comments
[e21ffe9] Add missing progress
[d4bee55] Scope exemptions to tab
[0e07684] Fix failing tests
[dd3e527] Fix tests
[e78a107] Allow matches to recover from faulty category
[ceafb2d] Add feed to isMalicious return type and integrate with error page
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/add-feed-to-return-type branch from 0e667cd to dc38d8c Compare February 4, 2025 15:55
@CrisBarreiro CrisBarreiro enabled auto-merge (squash) February 4, 2025 16:13
@CrisBarreiro CrisBarreiro mentioned this pull request Feb 4, 2025
Open
4 tasks
@CrisBarreiro CrisBarreiro merged commit 397271f into develop Feb 4, 2025
6 checks passed
@CrisBarreiro CrisBarreiro deleted the feature/cris/malicious-site-protection/add-feed-to-return-type branch February 4, 2025 16:26
@CrisBarreiro CrisBarreiro mentioned this pull request Feb 4, 2025
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonfortep cmonfortep cmonfortep approved these changes

Assignees

@cmonfortep cmonfortep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CrisBarreiro @cmonfortep