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(lavamoat/lavadome): update integration to improve security #25653

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

Conversation

weizman
Copy link
Member

@weizman weizman commented Jul 3, 2024

Address concerns under Safe Usage:

  • #csp - do not allow font to be fetched from just about anywhere
  • #execution-order - make sure LavaDome is imported right away

@metamaskbot
Copy link
Collaborator

Builds ready [4f4ac15]
Page Load Metrics (525 ± 368 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint743301205426
domContentLoaded9184343718
load442189525767368
domInteractive9184343718
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 134 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.02%. Comparing base (eadc707) to head (6338a91).
Report is 10 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #25653   +/-   ##
========================================
  Coverage    70.02%   70.02%           
========================================
  Files         1443     1443           
  Lines        50164    50164           
  Branches     14039    14039           
========================================
  Hits         35126    35126           
  Misses       15038    15038           

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

@weizman weizman changed the title draft - require lavadome early enough to win security feat(lavamoat/lavadome): require early enough to win security Jul 3, 2024
@weizman weizman changed the title feat(lavamoat/lavadome): require early enough to win security Improve LavaDome integration to improve security Jul 3, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jul 3, 2024
@weizman weizman changed the title Improve LavaDome integration to improve security feat(lavamoat/lavadome): update integration to improve security Jul 3, 2024
@weizman weizman marked this pull request as ready for review July 3, 2024 17:05
@weizman weizman requested a review from a team as a code owner July 3, 2024 17:05
@metamaskbot
Copy link
Collaborator

Builds ready [a5260d2]
Page Load Metrics (72 ± 10 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint72144102178
domContentLoaded106730147
load45121722010
domInteractive106730147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 134 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Updated integration with LavaDome to enhance security by refining CSP and ensuring correct import order.

  • Updated app/manifest/v2/chrome.json to restrict font sources to 'self'.
  • Updated app/manifest/v3/chrome.json to align CSP with LavaDome's recommendations, restricting font sources to 'self'.
  • Added import for @lavamoat/lavadome-react at the top of app/scripts/ui.js to ensure secure execution order.

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@metamaskbot
Copy link
Collaborator

Builds ready [8c8e360]
Page Load Metrics (632 ± 480 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint632311244421
domContentLoaded10117342412
load412964632999480
domInteractive10117342412
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 134 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The pull request focuses on enhancing security through improved LavaDome integration and various other updates.

  • Removed test-e2e-swap-playwright job in .circleci/config.yml, potentially streamlining CI/CD but removing swap functionality tests.
  • Added trim() method to diffOutput in .circleci/scripts/git-diff-develop.ts for cleaner diff results.
  • Removed 'sendAToken' message key across multiple localization files, ensure no references remain to avoid runtime errors.
  • Updated background.js to improve LavaDome integration and add test-specific functionality.
  • Introduced FakeKeyringBridge in app/scripts/lib/hardware-keyring-builder-factory.ts for testing purposes, ensure it doesn't affect production code.

75 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The pull request focuses on enhancing security through improved LavaDome integration and various TypeScript conversions for better maintainability and type safety.

  • Updated lavamoat/browserify/mmi/policy.json: Modified policy to directly allow @metamask-institutional/types package.
  • Added @metamask-institutional/types dependency: Updated package.json to include this new dependency.
  • Converted multiple files to TypeScript: Files under ui/components/institutional and ui/pages/confirmations were converted to TypeScript for improved type safety.
  • Removed interactive-replacement-token-modal.js: This file was removed, indicating its functionality is no longer needed or has been moved.
  • Introduced wrong-network-notification component: Added new files for this component, including stories and tests, to ensure proper integration and functionality.

46 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

sonarcloud bot commented Jul 30, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [6bdd808]
Page Load Metrics (509 ± 399 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint623181176833
domContentLoaded991252010
load403054509831399
domInteractive991252010
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 134 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@weizman
Copy link
Member Author

weizman commented Jul 31, 2024

6bdd808 passed successfully

Copy link

sonarcloud bot commented Sep 18, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [6338a91]
Page Load Metrics (1744 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24524371686399192
domContentLoaded14162142171218288
load14652262174419091
domInteractive149335209
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 134 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-lavamoat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants