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

feat: implement client side malicious network request detection #25839

Merged
merged 36 commits into from
Aug 29, 2024

Conversation

AugmentedMode
Copy link
Contributor

@AugmentedMode AugmentedMode commented Jul 15, 2024

Description

Open in GitHub Codespaces

This PR introduces three new updates to the MetaMask extension with the new phishing controller version 12.0.0.

  1. Removal of PhishFort List References: MetaMask no longer has a contract with PhishFort and has also been introducing false positives to the blocklist, all references to the PhishFort blocklist have been removed as we no longer use their list in the new PhishingController version.

  2. Support for Checking Malicious IPFS Domains: The phishing controller now includes support for detecting and blocking known malicious IPFS domains.

  3. Management of a C2 Domain Blocklist: The PhishingController now supports a client-side blocklist specifically for Command & Control (C2) domains. The extension now checks network requests against this blocklist and redirects users the the phishing warning page.

Manual testing steps

  1. Go to a website known to be on the C2 domain blocklist. For now we made our test website https://develop.d3bkcslj57l47p.amplifyapp.com/ have a malicious C2 Request that is on our blocklist.
  2. Attempt to interact with the site.
  3. Verify that on visiting the website you get redirected to the red Metamask phishing page.
  4. Repeat with a site that is not on the blocklist to confirm normal operation.

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g., pulled and built the branch, run the app, tested the code changes).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and/or screenshots.

@AugmentedMode AugmentedMode requested a review from a team as a code owner July 15, 2024 19:13
Copy link
Contributor

github-actions bot commented Jul 15, 2024

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

1 out of 2 committers have signed the CLA.
@AugmentedMode
@jacob Lebowitz

GitHub can't find an account for Jacob Lebowitz.
You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Jacob Lebowitz added 2 commits July 15, 2024 15:26
…they are on has a malicous c2 network request - updated meta metrics to track the type of malicous request
@AugmentedMode AugmentedMode changed the title Feat/client side detection feat: implement client side malicious network request detection Jul 16, 2024
@AugmentedMode
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@AugmentedMode AugmentedMode requested a review from a team as a code owner July 29, 2024 14:27
@AugmentedMode AugmentedMode self-assigned this Aug 28, 2024
@AugmentedMode AugmentedMode marked this pull request as draft August 28, 2024 03:46
@AugmentedMode AugmentedMode added the team-product-safety Push issues to Product Safety team label Aug 28, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Aug 28, 2024
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -10,6 +10,7 @@ describe('isBlockedUrl', () => {
allowedEvents: [],
});
const phishingController = new PhishingController({
// @ts-expect-error TODO: Resolve/patch mismatch between messenger types
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This is caused by the major version difference in the base-controller versions used in phishing-controller (v6) vs. extension (v5).

The only reason we're not seeing the same error in metamask-controller.js is because the file hasn't been converted to TypeScript.

@AugmentedMode
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [0b5e4ee]
Page Load Metrics (2443 ± 357 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21340732058958460
domContentLoaded166440132401742356
load167740712443743357
domInteractive13139533617
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 3.71 KiB (0.11%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 70.07%. Comparing base (b9173da) to head (cebf9f0).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
app/scripts/background.js 0.00% 9 Missing ⚠️
app/scripts/metamask-controller.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25839      +/-   ##
===========================================
- Coverage    70.08%   70.07%   -0.01%     
===========================================
  Files         1414     1414              
  Lines        49330    49337       +7     
  Branches     13782    13785       +3     
===========================================
  Hits         34570    34570              
- Misses       14760    14767       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AugmentedMode
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

app/scripts/background.js Show resolved Hide resolved
test/e2e/tests/phishing-controller/mocks.js Outdated Show resolved Hide resolved
Mrtenz
Mrtenz previously approved these changes Aug 29, 2024
Copy link

sonarcloud bot commented Aug 29, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [cebf9f0]
Page Load Metrics (1660 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23518111513431207
domContentLoaded1466180416428039
load1473181316608541
domInteractive146730147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 3.55 KiB (0.10%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt
Copy link
Member

Gudahtt commented Aug 29, 2024

Looks like the commit author info is wrong for the first few commits, resulting in the CLA bot error. That will be resolved when the PR is squashed though. I can override that check when this PR is ready to be merged.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM for the privacy reviews check, I have confirmed that the new API in the privacy snapshot is not accessed when basic functionalities (or the phishing detection setting) is disabled.

@AugmentedMode AugmentedMode merged commit be04ca8 into develop Aug 29, 2024
77 of 78 checks passed
@AugmentedMode AugmentedMode deleted the feat/client-side-detection branch August 29, 2024 20:52
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 29, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-product-safety Push issues to Product Safety team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants