fix: Add scuttle exception for fetch because it is used by chromium image util code when creating notifications by danjm · Pull Request #24631 · MetaMask/metamask-extension

danjm · 2024-05-20T14:09:11Z

Description

MV3 bug that we needto fix: #24518

In particular, we've got:

LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)

The problematic code is in not in our codebase but in chromium, here: https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we attemtped some solutions that would allow us to provide a modified and tamed fetch to be used for this image fetching. However, there are unknown problems - related to how sentry uses fetch and how sentry is initialized - that interfere with the solutions we attempted. So for now, as discussed with @weizman, the latest commit on the PR just adds a scuttling exception for fetch (and for Offscreen Canvas, which is also needed for the browser notification feature).

We will return to the root problem later, and aim to remove these scuttling exceptions.

Related issues

Fixes: #24518

Manual testing steps

yarn dist:mv3
install, onboard
send a transaction
verify that a browser notification is shown once the transaction is confirmed

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

github-actions · 2024-05-20T14:09:26Z

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 · 2024-05-20T14:53:36Z

Builds ready [91cb685]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (954 ± 525 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	149	93	25	12
		domContentLoaded	9	33	14	7	3
		load	51	2538	954	1094	525
		domInteractive	9	33	14	7	3

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

weizman

Looks great,

Some important comments.

Also, a more general (yet very important) comment:

We must cache the APIs we're using as part of this feature in order to not trust they aren't being modified later on. SES assures that in practice, but it's still a more secure approach in case we extend this to use APIs that aren't strictly SES protected intrinsics.

So cache and use the test property of RegExp and Error and fetch and Object.hasOwn and so on.

I can help with that part 👍

weizman · 2024-05-22T16:07:52Z

app/scripts/use-snow.js

What's this for?

uh, nothing... dead code from some earlier iteration

removed with 1dd34f9af5

weizman · 2024-05-22T16:09:57Z

app/scripts/use-snow.js

I'd move this function to the upper scope (same as tamedFetch)

done in 1dd34f9af5

weizman · 2024-05-22T16:10:54Z

app/scripts/use-snow.js

Unscuttled -> tamed

Will do. Can you explain that suggestion a bit? I am still building up my understanding of how scuttiling works, and I suspect your suggestion here is meaningful and I could learn something by hearing your thoughts on this suggestion

Yes of course, gladly.
"Unscuttled" is correct, because it is in fact going to be exempt from the scuttling process.
But it's not accurate enough, because "Unscuttled" really reminds something that is being completely over looked, which isn't the case here.
fetch is going to be "Unscuttled" only because we promise to tame it - that is the only legitimate excuse for excepting it from being scuttled.
That is why I think referring to it as tamed rather than unscuttled is important here - so it's clear that we mutated it in order to justify it from not being scuttled.

done in 5467de13d6

weizman · 2024-05-22T16:12:36Z

app/scripts/use-snow.js

We must use defineProperty and set configurable and writable to false. This will instruct the scuttling process to skip fetch, which would allow you to remove the exception from development/build/index.js

done in 1dd34f9af5

weizman · 2024-05-22T16:14:22Z

app/scripts/use-snow.js

What's the importance of the chromeExtensionId below really? I think we can allow fetching images from the chrome-extension scheme for all types of browser extension entities we create

addressed in b769e71328

weizman · 2024-05-22T16:18:29Z

development/build/index.js

remove this line once #24631 (comment) is fixed

ACTUALLY - applying the idea behind this PR to the Image exception as well could be very beneficial, as Image is also a very strong capability (just a thought)

…util code when creating notifications

…er entities

…ty on the global reference when setting fetch

metamaskbot · 2024-05-23T20:15:29Z

Builds ready [e48f02d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (729 ± 490 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	57	144	86	23	11
		domContentLoaded	9	22	12	4	2
		load	46	2458	729	1021	490
		domInteractive	9	22	12	4	2

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

danjm · 2024-05-23T22:59:41Z

An issue for further investigation into the root cause of the problem this PR addresses is here #24759

metamaskbot · 2024-06-04T20:07:47Z

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

weizman reviewed May 22, 2024

View reviewed changes

danjm marked this pull request as ready for review May 22, 2024 16:28

danjm requested review from a team and kumavis as code owners May 22, 2024 16:28

danjm changed the title ~~Add scuttle exception for fetch because it is used by chromium image util code when creating notifications~~ fix: Add scuttle exception for fetch because it is used by chromium image util code when creating notifications May 22, 2024

danjm requested review from a team as code owners May 23, 2024 18:06

danjm force-pushed the scuttle-fetch-browser-image-util branch 3 times, most recently from d5c47aa to dfd197a Compare May 23, 2024 18:09

danjm added 7 commits May 23, 2024 15:40

Add scuttle exception for fetch because it is used by chromium image …

9ed0184

…util code when creating notifications

Limit unscuttled fetch to only fetching images from the extension itself

ca09a97

allow fetching images from the chrome-extension scheme for all brows…

388e028

…er entities

Improve wording of tamed fetch error message

ac81435

Move scuttleWithRestrictedException to upper scope; use definedProper…

2d6fff3

…ty on the global reference when setting fetch

small code re-org

b31e0af

Got it working, at least

3a4a55b

danjm force-pushed the scuttle-fetch-browser-image-util branch from dfd197a to 08ab433 Compare May 23, 2024 18:11

Revert all changes

3d678d7

danjm force-pushed the scuttle-fetch-browser-image-util branch from 08ab433 to 3d678d7 Compare May 23, 2024 18:12

Add scuttleGlobalThisException for fetch and OffscreenCanvas

e48f02d

DDDDDanica approved these changes May 23, 2024

View reviewed changes

vthomas13 approved these changes May 23, 2024

View reviewed changes

danjm added the team-extension-platform Extension Platform team label May 23, 2024

legobeat requested a review from weizman May 23, 2024 21:11

danjm mentioned this pull request May 23, 2024

Research and resolve errors in reconfiguring fetch in use-snow.js #24759

Open

weizman approved these changes May 23, 2024

View reviewed changes

danjm merged commit ef2f2f5 into develop May 23, 2024

danjm deleted the scuttle-fetch-browser-image-util branch May 23, 2024 23:15

github-actions bot locked and limited conversation to collaborators May 23, 2024

metamaskbot added the release-11.18.0 label May 23, 2024

metamaskbot added release-11.16.6 Issue or pull request that will be included in release 11.16.6 and removed release-11.18.0 labels Jun 4, 2024

Conversation

danjm commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented May 20, 2024

Uh oh!

metamaskbot commented May 20, 2024

Uh oh!

weizman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizman May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented May 23, 2024

Uh oh!

danjm commented May 23, 2024

Uh oh!

metamaskbot commented Jun 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danjm commented May 20, 2024 •

edited

Loading

danjm May 22, 2024 •

edited

Loading

weizman May 22, 2024 •

edited

Loading