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

Enabling the MetaMask Security Code Scanner #22916

Merged
merged 20 commits into from
Feb 27, 2024
Merged

Conversation

witmicko
Copy link
Contributor

@witmicko witmicko commented Feb 13, 2024

Required Action

Prior to merging this pull request, please ensure the following has been completed:

  • The lines specifying branches correctly specifies this repository's default branch (usually main).
  • Any paths you would like to ignore have been added to the paths_ignored configuration option (see setup)
  • Any existing CodeQL configuration has been disabled.

What is the Security Code Scanner?

This pull request enables the MetaMask Security Code Scanner GitHub Action. This action runs on each pull request, and will flag potential vulnerabilities as a review comment. It will also scan this repository's default branch, and log any findings in this repository's Code Scanning Alerts Tab.

Screenshot 2024-02-12 at 9 19 05 PM

The action itself runs various static analysis engines behind the scenes. Currently, it is only running GitHub's CodeQL engine. For this reason, we recommend disabling any existing CodeQL configuration your repository may have.

How do I interact with the tool?

Every finding raised by the Security Code Scanner will present context behind the potential vulnerability identified, and allow the developer to fix, or dismiss it.

The finding will automatically be dismissed by pushing a commit that fixes the identified issue, or by manually dismissing the alert using the button in GitHub's UI. If dismissing an alert manually, please add any additional context surrounding the reason for dismissal, as this informs our decision to disable, or improve any poor performing rules.

Screenshot 2024-02-12 at 8 41 46 PM

For more configuration options, please review the tool's README.
For any additional questions, please reach out to @mm-application-security slack.

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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 13, 2024
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@metamaskbot
Copy link
Collaborator

Builds ready [68db3ae]
Page Load Metrics (961 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1073111885426
domContentLoaded999322713
load755130396111555
domInteractive999322713
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [1e58a01]
Page Load Metrics (1028 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1142501923818
domContentLoaded1287352612
load892115810286933
domInteractive1187352612
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@witmicko witmicko marked this pull request as ready for review February 13, 2024 18:22
@witmicko witmicko requested a review from a team as a code owner February 13, 2024 18:22
@metamaskbot
Copy link
Collaborator

Builds ready [be81bb9]
Page Load Metrics (1065 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1313201914421
domContentLoaded10130383015
load8181413106512660
domInteractive10130383015
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.60%. Comparing base (3ca675f) to head (97d6827).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #22916   +/-   ##
========================================
  Coverage    68.60%   68.60%           
========================================
  Files         1097     1097           
  Lines        43309    43309           
  Branches     11544    11544           
========================================
  Hits         29711    29711           
  Misses       13598    13598           

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

@metamaskbot
Copy link
Collaborator

Builds ready [13f8900]
Page Load Metrics (1087 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1195352148742
domContentLoaded993502713
load9321298108710752
domInteractive993502713
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [c477d72]
Page Load Metrics (1118 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1275902119445
domContentLoaded994402613
load8971629111817986
domInteractive994402613
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [4333e8e]
Page Load Metrics (1063 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1334802249144
domContentLoaded999362512
load9231384106312761
domInteractive999362512
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [1b687ca]
Page Load Metrics (1204 ± 111 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1205782188742
domContentLoaded13103483015
load91117861204232111
domInteractive13103483015
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [55c5572]
Page Load Metrics (1117 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint992861964823
domContentLoaded1196342512
load8401302111710852
domInteractive1196342512
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

NicholasEllul
NicholasEllul previously approved these changes Feb 15, 2024
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

Looks good from the security side of things. Will need someone from the @MetaMask/extension-devs team to push a commit highlighting the paths they would like to avoid scanning.

@metamaskbot
Copy link
Collaborator

Builds ready [048aabb]
Page Load Metrics (1110 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1063512074924
domContentLoaded1073352210
load9541526111013364
domInteractive1072352210
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt
Copy link
Member

Gudahtt commented Feb 15, 2024

The examples of third-party/vendored code that I'm aware of are:

  • node_modules
  • development/chromereload.js

Definitely seems appropriate to ignore those two at least.

@metamaskbot
Copy link
Collaborator

Builds ready [1d87657]
Page Load Metrics (1008 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1332572003316
domContentLoaded983272010
load808114210088239
domInteractive983272010
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@witmicko
Copy link
Contributor Author

The examples of third-party/vendored code that I'm aware of are:

  • node_modules
  • development/chromereload.js

Definitely seems appropriate to ignore those two at least.

I've added these two paths. CodeQL scans on just the repo- node_modules are not scanned by the default, Im ok leaving it in so it is explicit.

@metamaskbot
Copy link
Collaborator

Builds ready [64f30dc]
Page Load Metrics (1015 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1293281844823
domContentLoaded1078342412
load889121810157938
domInteractive1078342412
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [dbf5034]
Page Load Metrics (1065 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1094211896933
domContentLoaded9103342914
load7921513106516981
domInteractive9103342914
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM but would it be not too tricky to have a preview available of what it would look like on current state of develop to get a sense of the amount of signal and noise? Like running this change on a fork? :)

@witmicko
Copy link
Contributor Author

LGTM but would it be not too tricky to have a preview available of what it would look like on current state of develop to get a sense of the amount of signal and noise? Like running this change on a fork? :)

thank you @legobeat
good suggestion, ran it on my fork
https://github.com/witmicko/metamask-extension/security/code-scanning
got same results as the ones in extension already
https://github.com/MetaMask/metamask-extension/security/code-scanning?query=is%3Aclosed+branch%3Adevelop

@metamaskbot
Copy link
Collaborator

Builds ready [1dbcbda]
Page Load Metrics (1769 ± 83 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1113381715225
domContentLoaded115626147
load13822242176917383
domInteractive115626147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

Needs to accommodate to not make branches from external repos have failing CI.

@witmicko
Copy link
Contributor Author

Needs to accommodate to not make branches from external repos have failing CI.

This is fixed now

@metamaskbot
Copy link
Collaborator

Builds ready [f5ba74f]
Page Load Metrics (1509 ± 338 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint882831474823
domContentLoaded1075372210
load7420471509704338
domInteractive1075372210
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [97d6827]
Page Load Metrics (1045 ± 377 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint612751354923
domContentLoaded1093332311
load4920471045785377
domInteractive1093332311
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@witmicko witmicko merged commit 3096404 into develop Feb 27, 2024
67 checks passed
@witmicko witmicko deleted the mm-security-code-scanner branch February 27, 2024 15:15
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2024
@metamaskbot metamaskbot added the release-11.13.0 Issue or pull request that will be included in release 11.13.0 label Feb 27, 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-11.13.0 Issue or pull request that will be included in release 11.13.0 team-extension-platform team-security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants