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: support security alerts API #25544

Merged
merged 17 commits into from
Jul 5, 2024
Merged

Conversation

vinistevam
Copy link
Contributor

@vinistevam vinistevam commented Jun 27, 2024

Description

This PR enables the use of the Security Alerts API to validate dApp requests, with a fallback to local PPOM validation if the API request fails.

Environment Variables

Add the following variables to .metamaskrc:

SECURITY_ALERTS_API_URL='http://localhost:3000'
SECURITY_ALERTS_API_ENABLED='true'

Additional Changes

Introduces the security_alert_source property to transaction and signature events, indicating api or local as the source.

Related Repository

Refer to the Security Alerts API repository for more details.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2514 https://github.com/MetaMask/MetaMask-planning/issues/2515

Manual testing steps

  1. Test blockaid regression

  2. add the envs

SECURITY_ALERTS_API_URL='https://security-alerts.dev-api.cx.metamask.io'
SECURITY_ALERTS_API_ENABLED='true'
  • Go to test dapp and trigger on of the malicious signatures
  • To verify in chrome go to dev tools > network. Search for security-alerts and find the call to the API service.

Existing PPOM logic should function as before, even with the above environment variables added, due to the fallback to the controller in the event of an error.

Screenshots/Recordings

image

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • 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.

@vinistevam vinistevam added the team-confirmations Push issues to confirmations team label Jun 27, 2024
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.79%. Comparing base (a935093) to head (1d534ff).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25544      +/-   ##
===========================================
+ Coverage    69.78%   69.79%   +0.02%     
===========================================
  Files         1376     1377       +1     
  Lines        48409    48435      +26     
  Branches     13350    13354       +4     
===========================================
+ Hits         33779    33805      +26     
  Misses       14630    14630              

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

@metamaskbot
Copy link
Collaborator

Builds ready [0fbd0e9]
Page Load Metrics (140 ± 174 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7311693126
domContentLoaded9321352
load431720140363174
domInteractive9321352
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.62 KiB (0.05%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@vinistevam vinistevam marked this pull request as ready for review June 28, 2024 06:05
@vinistevam vinistevam requested review from a team as code owners June 28, 2024 06:05
app/scripts/lib/ppom/ppom-util.ts Outdated Show resolved Hide resolved
builds.yml Outdated Show resolved Hide resolved
app/scripts/lib/ppom/ppom-util.ts Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [8fa55b3]
Page Load Metrics (184 ± 187 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint733941436632
domContentLoaded1196352311
load501872184389187
domInteractive1196352311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.17 KiB (0.04%)
  • ui: 0 Bytes (0.00%)
  • common: 133 Bytes (0.00%)

security_alert_reason: BlockaidReason.setApprovalForAll,
security_alert_response: BlockaidResultType.Malicious,
security_alert_source: SecurityAlertSource.Local,
ui_customizations: ['flagged_as_malicious'],
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] is it worth saving this string as a constant to prevent typos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, I changed to use the enum MetaMetricsEventUiCustomization.

Copy link
Contributor

@pedronfigueiredo pedronfigueiredo left a comment

Choose a reason for hiding this comment

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

LGTM @vinistevam, I just left one comment

security_alert_response: BlockaidResultType.Malicious,
ui_customizations: ['flagged_as_malicious'],
});
expect(result).toStrictEqual(expectedMetricsPropsBase);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

jpuri
jpuri previously approved these changes Jul 3, 2024
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

matthewwalsh0
matthewwalsh0 previously approved these changes Jul 3, 2024
@vinistevam vinistevam dismissed stale reviews from matthewwalsh0 and jpuri via f74b5cc July 3, 2024 12:12
@metamaskbot
Copy link
Collaborator

Builds ready [ba81efe]
Page Load Metrics (222 ± 211 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint61142107209
domContentLoaded106634189
load411675222439211
domInteractive106634189
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.17 KiB (0.04%)
  • ui: 0 Bytes (0.00%)
  • common: 133 Bytes (0.00%)

Copy link

sonarcloud bot commented Jul 4, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [1d534ff]
Page Load Metrics (137 ± 148 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint63131972110
domContentLoaded86328178
load391474137307148
domInteractive86328178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.17 KiB (0.04%)
  • ui: 0 Bytes (0.00%)
  • common: 133 Bytes (0.00%)

@vinistevam vinistevam merged commit fe23ae0 into develop Jul 5, 2024
74 checks passed
@vinistevam vinistevam deleted the feat/support-security-alerts-api branch July 5, 2024 05:35
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
@metamaskbot metamaskbot added the release-12.2.0 Issue or pull request that will be included in release 12.2.0 label Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.2.0 Issue or pull request that will be included in release 12.2.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants