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.

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

Intercept requests and add skeleton for malicious site detection #5369

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Dec 9, 2024

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

Description

Adds logic to intercept requests using shouldOverrideUrlLoading and shouldInterceptRequest, when the request matches the following conditions

  • Is for mainframe
  • Is for iframe and the host loading it matches the webview host
  • The same URL hasn't been already intercepted during the same page load (used to prevent the same URL being intercepted by the 2 webview callbacks)

Steps to test this PR

Feature 1

  • Load a site and check you get Timber.tag("MaliciousSiteProtection").d("isMalicious $url") for all mainframe and iframe requests (only for internal builds).
  • Check you should never get 2 instances of those logs for the exact same URL

UI changes

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

Copy link
Contributor Author

CrisBarreiro commented Dec 9, 2024

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch 3 times, most recently from 8d0f3b0 to 0b7f33d Compare December 10, 2024 13:37
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch 2 times, most recently from 7fd8100 to c24dff3 Compare December 11, 2024 14:12
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.

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 🙏

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch 2 times, most recently from 3fb96e8 to 601db6b Compare December 16, 2024 17:31
@CrisBarreiro
Copy link
Contributor Author

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 🙏

Discussed over zoom, there are no requests to such pages when navigating to an article. Documented here

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 2c3627c to cfc8cea Compare December 18, 2024 15:41
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

Suggested to remove the state from the singleton if we can do that.

@@ -424,12 +415,11 @@ class BrowserWebViewClient @Inject constructor(
// See https://app.asana.com/0/0/1206159443951489/f (WebView limitations)
if (it != ABOUT_BLANK && start == null) {
start = currentTimeProvider.elapsedRealtime()
maliciousSiteProtectionWebViewIntegration.onPageLoadStarted()
requestInterceptor.onPageStarted(url)
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 need to check if moving requestInterceptor.onPageStarted(url) here has any unintended side-effect. This is because onPageStarted can be called several times when a page is loaded

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from c418e0f to db4d9bc Compare December 19, 2024 10:15
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from db4d9bc to bad45a7 Compare December 19, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants