Skip to content

feat: Use sandboxed pages for Snaps execution in MV3 #25171

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

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Jun 10, 2024

Description

Moves the execution of Snaps to a sandboxed page referencing a local HTML file inside the offscreen document when using the MV3 build. This effectively gives us access to eval without hitting the network and should reduce the overhead when booting a Snap (+ allow for Snaps to execute when the user is offline).

To support this, this PR introduces some new changes to the manifest as well as the build process. For the build process we simply copy the same iframe bundle currently used in the hosted version to /dist/chrome/snaps. For the manifest, we add a reference to the sandboxed page and tweak the CSP of the sandbox to be as restrictive as possible (the default is not very strict). Then we can simply point the existing offscreen executor to use the local iframe instead of the remote one.

Closes #25250

Open in GitHub Codespaces

@FrederikBolding FrederikBolding added the team-snaps-platform Snaps Platform team label Jun 10, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jun 10, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [2b67003]
Page Load Metrics (55 ± 17 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65294904823
domContentLoaded98914178
load40203553617
domInteractive98914178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.64%. Comparing base (9a5aeae) to head (89efe5d).
Report is 7 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #25171   +/-   ##
========================================
  Coverage    65.64%   65.64%           
========================================
  Files         1367     1367           
  Lines        54257    54257           
  Branches     14189    14189           
========================================
  Hits         35616    35616           
  Misses       18641    18641           

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

@@ -1,6 +1,7 @@
{
"content_security_policy": {
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none';"
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none';",
"sandbox": "sandbox allow-scripts; script-src 'self' 'unsafe-inline' 'unsafe-eval'; object-src 'none';"
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

we could experiment with adding default-src 'none' to these, but I think that could potentially block fetch. Maybe other things. But it'd also block some exfiltration techniques like image URLs

Copy link
Member Author

Choose a reason for hiding this comment

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

If we add default-src 'none', we would need something for connect-src * to allow for fetch at least

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@FrederikBolding FrederikBolding changed the title Experiment with sandboxed pages in MV3 feat: Use sandboxed pages for Snaps execution in MV3 Jun 11, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [b0ad7b6]
Page Load Metrics (52 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6110084126
domContentLoaded9301242
load407652105
domInteractive9301242
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@naugtur
Copy link
Contributor

naugtur commented Jun 11, 2024

Looks great.
It's worth red teaming the solution to see if the sandbox itself is working as expected or whether it has access to resources we didn't want it to access, but that's chrome bugs if any.
Assuming chrome does its work, this is awesome.

@FrederikBolding FrederikBolding force-pushed the fb/run-snaps-locally-in-mv3 branch from 98f0439 to 9c51502 Compare June 12, 2024 09:08
@FrederikBolding FrederikBolding marked this pull request as ready for review June 12, 2024 09:54
@FrederikBolding FrederikBolding requested review from a team and kumavis as code owners June 12, 2024 09:54
@metamaskbot
Copy link
Collaborator

Builds ready [89efe5d]
Page Load Metrics (53 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7112387115
domContentLoaded9251232
load438753115
domInteractive9251232
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

This looks good to me, as long as the current state of the content_security_policy changes are approved by @naugtur

@@ -1,6 +1,7 @@
{
"content_security_policy": {
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none';"
"extension_pages": "script-src 'self' 'wasm-unsafe-eval'; object-src 'none'; frame-ancestors 'none';",
"sandbox": "sandbox allow-scripts; script-src 'self' 'unsafe-inline' 'unsafe-eval'; object-src 'none'; default-src 'none'; connect-src *;"
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the default-src!

Copy link
Member Author

Choose a reason for hiding this comment

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

🫡

@FrederikBolding FrederikBolding merged commit e3071f2 into develop Jun 13, 2024
77 checks passed
@FrederikBolding FrederikBolding deleted the fb/run-snaps-locally-in-mv3 branch June 13, 2024 16:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 13, 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.1.0 Issue or pull request that will be included in release 12.1.0 team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use sandboxed pages for Snaps execution in MV3
6 participants