-
Notifications
You must be signed in to change notification settings - Fork 551
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
Add WebHID endowment #1219
Add WebHID endowment #1219
Changes from all commits
b63302f
ab6e178
3306f08
6d62961
e7e15ec
fed6f48
2472067
0e1dcfa
7bccc12
0321c34
b6c8e77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
{ | ||
"branches": 89.19, | ||
"functions": 95.01, | ||
"lines": 96.45, | ||
"statements": 96.35 | ||
"functions": 95.04, | ||
"lines": 96.47, | ||
"statements": 96.37 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { PermissionType } from '@metamask/permission-controller'; | ||
|
||
import { SnapEndowments } from './enum'; | ||
import { webhidEndowmentBuilder } from './webhid'; | ||
|
||
describe('endowment:webhid', () => { | ||
it('builds the expected permission specification', () => { | ||
const specification = webhidEndowmentBuilder.specificationBuilder({}); | ||
expect(specification).toStrictEqual({ | ||
permissionType: PermissionType.Endowment, | ||
targetKey: SnapEndowments.WebHID, | ||
endowmentGetter: expect.any(Function), | ||
allowedCaveats: null, | ||
}); | ||
|
||
expect(specification.endowmentGetter()).toStrictEqual(['navigator']); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { | ||
EndowmentGetterParams, | ||
PermissionSpecificationBuilder, | ||
PermissionType, | ||
ValidPermissionSpecification, | ||
} from '@metamask/permission-controller'; | ||
|
||
import { SnapEndowments } from './enum'; | ||
|
||
const permissionName = SnapEndowments.WebHID; | ||
|
||
type WebHIDEndowmentSpecification = ValidPermissionSpecification<{ | ||
permissionType: PermissionType.Endowment; | ||
targetKey: typeof permissionName; | ||
endowmentGetter: (_options?: any) => ['navigator']; | ||
allowedCaveats: null; | ||
}>; | ||
|
||
/** | ||
* `endowment:webhid` returns the name of global browser API(s) that | ||
* enable the usage of navigator.webhid. This enables the connection of devices that uses webhid such as Ledger. | ||
* | ||
* @param _builderOptions - Optional specification builder options. | ||
* @returns The specification for the webhid endowment. | ||
*/ | ||
const specificationBuilder: PermissionSpecificationBuilder< | ||
PermissionType.Endowment, | ||
any, | ||
WebHIDEndowmentSpecification | ||
> = (_builderOptions?: any) => { | ||
return { | ||
permissionType: PermissionType.Endowment, | ||
targetKey: permissionName, | ||
allowedCaveats: null, | ||
endowmentGetter: (_getterOptions?: EndowmentGetterParams) => { | ||
return ['navigator']; | ||
}, | ||
}; | ||
}; | ||
|
||
export const webhidEndowmentBuilder = Object.freeze({ | ||
targetKey: permissionName, | ||
specificationBuilder, | ||
} as const); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
{ | ||
"branches": 77.41, | ||
"functions": 91.26, | ||
"lines": 85.23, | ||
"statements": 85.32 | ||
"functions": 91.33, | ||
"lines": 85.29, | ||
"statements": 85.37 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { rootRealmGlobal } from '../globalObject'; | ||
import Webhid from './webhid'; | ||
|
||
describe('Webhid endowment', () => { | ||
it('has expected properties', () => { | ||
expect(Webhid).toMatchObject({ | ||
names: ['navigator'], | ||
factory: expect.any(Function), | ||
}); | ||
}); | ||
|
||
it('does not return the navigator from rootRealmGlobal', () => { | ||
const { navigator } = Webhid.factory(); | ||
expect(navigator).not.toStrictEqual(rootRealmGlobal.navigator); | ||
}); | ||
|
||
it('contains the hid key only', () => { | ||
const { navigator } = Webhid.factory(); | ||
expect(Object.keys(navigator)).toContain('hid'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { rootRealmGlobal } from '../globalObject'; | ||
|
||
/** | ||
* Create a {@link navigator} object, with the same properties as the global | ||
* {@link navigator} object, but only with access to hid. | ||
* | ||
* @returns The {@link navigator} object with only access to hid. | ||
*/ | ||
function createHID() { | ||
return { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be hardened using Also, I wonder if there is more locking down of the object required since it is an Thoughts @weizman |
||
}, | ||
}; | ||
} | ||
|
||
const endowmentModule = { | ||
names: ['navigator'] as const, | ||
factory: createHID, | ||
}; | ||
|
||
export default endowmentModule; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,5 +2,5 @@ | |
"branches": 92.92, | ||
"functions": 100, | ||
"lines": 98.94, | ||
"statements": 98.95 | ||
"statements": 98.96 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be interesting if we could set this conditionally based on the permission 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maarten is right, the iframe is created when For reference: |
||
|
||
document.body.appendChild(iframe); | ||
|
||
iframe.addEventListener('load', () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the snap manifest can have a target field?