Intercept requests and add skeleton for malicious site detection #5369
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
...st/java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegrationTest.kt
Outdated
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/malicious-site-protection/intercept-requests
branch
2 times, most recently
from
13cfb3a to
8d0f3b0
Compare
CrisBarreiro
force-pushed
the
feature/cris/malicious-site-protection/intercept-requests
branch
from
8d0f3b0 to
0b7f33d
Compare
...c/main/java/com/duckduckgo/app/browser/webview/RealMaliciousSiteBlockerWebViewIntegration.kt
Outdated
Show resolved
Hide resolved
CrisBarreiro
force-pushed
the
feature/cris/malicious-site-protection/intercept-requests
branch
from
615f371 to
7fd8100
Compare
CrisBarreiro
force-pushed
the
feature/cris/malicious-site-protection/intercept-requests
branch
from
7fd8100 to
c24dff3
Compare
There was a problem hiding this comment.
Just testing the changes (haven't started the code review yet)
Sharing something I've found:
- when clicking any link in this URL https://www.yahoo.com/?guccounter=1, the url changes but I don't see it in the logs.
- After some normal browsing on a tab, testing back and forward navigation between sites, I don't see some URLs appearing, and that I can repro. (edit: actually I think this never works, and the urls I see is just some requests the site triggers)
- I think this one is fine but just fyi, in the logs I see http://domain.com and https://domain.com (just changing schema http -> https)
Friendly reminder about updating the description of the PR to include what's included and giving some context 🙏
| @@ -158,6 +164,20 @@ class BrowserWebViewClient @Inject constructor( | |||
| try { | |||
| Timber.v("shouldOverride webViewUrl: ${webView.url} URL: $url") | |||
| webViewClientListener?.onShouldOverride() | |||
|
|
|||
| if (runBlocking { | |||
There was a problem hiding this comment.
proposal: remove the suspend for shouldOverrideUrlLoading, and move the runBlocking inside the implementation of that method. That way we can control where exactly we need it and it's an implementation detail of that method.
| @@ -63,4 +67,10 @@ class RealMaliciousSiteProtection @Inject constructor( | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| override suspend fun isMalicious(url: Uri, confirmationCallback: (isMalicious: Boolean) -> Unit): IsMaliciousResult { | |||
There was a problem hiding this comment.
Proposal: implementing MaliciousSiteProtection on a different class, and leaving this one exclusively for the feature config state (since the feature state is more than just a boolean and has state). That way we can separate responsibilities, and improve cohesion, ensuring that each class has a well-defined purpose.
| @@ -48,6 +49,7 @@ interface RequestInterceptor { | |||
| request: WebResourceRequest, | |||
| webView: WebView, | |||
| documentUri: Uri?, | |||
| maliciousSiteProtectionWebViewIntegration: MaliciousSiteBlockerWebViewIntegration, | |||
There was a problem hiding this comment.
Is there a specific reason for passing this as a method argument rather than injecting it as a class attribute via the constructor? I would prefer the later since this is a dependency rather than a value.
| webViewClientListener: WebViewClientListener?, | ||
| ): WebResourceResponse? { | ||
| val url: Uri? = request.url | ||
|
|
||
| val confirmationCallback: (isMalicious: Boolean) -> Unit = { |
There was a problem hiding this comment.
I assume this will have the same implementation as in app/src/main/java/com/duckduckgo/app/browser/BrowserWebViewClient.kt L131.
Have you considered any options to don't have them duplicated?
| return false | ||
| } | ||
|
|
||
| private fun isForIframe(request: WebResourceRequest) = request.requestHeaders["Sec-Fetch-Dest"] == "iframe" || |
There was a problem hiding this comment.
I'm not very familiar with detecting iframes, so just asking, how can we validate this is the right checks / we are not missing anything?
also, this is being called from a block which is
if(mainFrame ...)
else if (isForIframe(request) && documentUri?.host == request.requestHeaders["Referer"]?.toUri()?.host)
just checking, would it be possible to skip isForIframe and just do documentUri?.host == request.requestHeaders["Referer"]?.toUri()?.host?
seems we just want to run our logic for any mainframe OR request related to that documentUri.host, does it matter if iframe is true?
| return true | ||
| } | ||
| processedUrls.add(decodedUrl) | ||
| } |
There was a problem hiding this comment.
could be add a comment documenting why we don't check for iframes here since we do in other methods?
Task/Issue URL: https://app.asana.com/0/1205008441501016/1207151848931035/f
Description
Steps to test this PR
Feature 1
Timber.tag("MaliciousSiteProtection").d("isMalicious $url")for all mainframe and iframe requests (only for internal builds).UI changes