Add WebHID endowment #1219
Add WebHID endowment #1219
Conversation
| @@ -29,6 +29,10 @@ export async function createWindow( | |||
| // MDN article for `load` event: https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event | |||
| // Re: `load` firing twice: https://stackoverflow.com/questions/10781880/dynamically-created-iframe-triggers-onload-event-twice/15880489#15880489 | |||
| iframe.setAttribute('src', uri); | |||
|
|
|||
| // Enable WebHID to be executed in the iframe. | |||
| iframe.setAttribute('allow', 'hid'); | |||
There was a problem hiding this comment.
Would be interesting if we could set this conditionally based on the permission 🤔
There was a problem hiding this comment.
The snaps iframe is created before the snaps controller, so i'm not sure if we can conditionally set that.
There was a problem hiding this comment.
Hmm, it would be better if we didn't give HID to any iframes that don't need it, hmm 🤔
There was a problem hiding this comment.
The iframe is created by the execution service, after the snaps controller starts a snap, right? We could pass the endowments to the execution service when creating the iframe, and only set this attribute based on that.
There was a problem hiding this comment.
the iframe is created before the snaps controller. https://github.com/MetaMask/metamask-extension/blob/5468c2c14219420eba4643df577d8f6be0d11847/app/scripts/metamask-controller.js#L755
There was a problem hiding this comment.
Maarten is right, the iframe is created when executeSnap is called on an execution service. The IframeExecutionService does not create the iframe before it is needed to execute snap code. So we can pass along the endowments from snapData and use that.
For reference:
https://github.com/MetaMask/snaps-monorepo/blob/main/packages/snaps-controllers/src/services/AbstractExecutionService.ts#L340
|
Also this needs a rebase for CI to run @montelaidev |
montelaidev
force-pushed
the
feat/webhid-endowment
branch
from
173b75e
to
b21c54b
Compare
Socket Security Pull Request Report👍 No new dependency issues detected in pull request Pull request report summary
Bot CommandsTo ignore an alert, reply with a comment starting with Powered by socket.dev |
montelaidev
force-pushed
the
feat/webhid-endowment
branch
from
73b9ed4
to
2472067
Compare
Codecov Report
@@ Coverage Diff @@
## main #1219 +/- ##
==========================================
+ Coverage 95.27% 95.29% +0.01%
==========================================
Files 141 143 +2
Lines 4361 4374 +13
Branches 718 718
==========================================
+ Hits 4155 4168 +13
Misses 206 206
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| navigator: { | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/Navigator/hid | ||
| // https://wicg.github.io/webhid/#dom-navigator-hid | ||
| hid: rootRealmGlobal.navigator?.hid, |
There was a problem hiding this comment.
This should probably be hardened using harden. Can you verify that it still works after doing that?
Also, I wonder if there is more locking down of the object required since it is an EventTarget 🤔
Thoughts @weizman
| @@ -29,6 +29,10 @@ export async function createWindow( | |||
| // MDN article for `load` event: https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event | |||
| // Re: `load` firing twice: https://stackoverflow.com/questions/10781880/dynamically-created-iframe-triggers-onload-event-twice/15880489#15880489 | |||
| iframe.setAttribute('src', uri); | |||
|
|
|||
| // Enable WebHID to be executed in the iframe. | |||
| iframe.setAttribute('allow', 'hid'); | |||
There was a problem hiding this comment.
Hmm, it would be better if we didn't give HID to any iframes that don't need it, hmm 🤔
| * | ||
| * @returns The {@link navigator} object with only access to hid. | ||
| */ | ||
| function createHID() { |
There was a problem hiding this comment.
We need to have a discussion about this endowment in general since it is not available in Firefox. Any snap that tries to leverage it will not work in FF. In the past we have not added endowments that aren't platform agnostic.
There was a problem hiding this comment.
Perhaps the snap manifest can have a target field?
{
"targets": {
"chrome": ">=89",
},
}
Creates a new endowment to allow for webhid usage in snaps.