Skip to content

Commit 92ace80

Browse files
fix: Assert expected hooks for restricted method specifications (#3993)
Assert the expected hooks as part of `buildSnapRestrictedMethodSpecifications` to indicate missing or extraneous hooks being passed in to the developer. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Medium risk because it adds strict hook validation in `buildSnapRestrictedMethodSpecifications`, which can cause runtime errors for callers passing missing or extra hooks, and updates simulation wiring accordingly. > > **Overview** > Adds hook contract enforcement to `buildSnapRestrictedMethodSpecifications` by computing the expected `methodHooks` from non-excluded restricted-method builders, calling `assertExpectedHooks`, and filtering excluded permissions before spec creation. > > Updates tests to pass the full required hook set and adds coverage ensuring excluded permissions (e.g., `snap_dialog`) are omitted. Adjusts snaps simulation permission spec assembly to pass only the required hooks (including `getSnapKeyring`) and tweaks Jest coverage thresholds. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6640a8d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 253ace1 commit 92ace80

4 files changed

Lines changed: 58 additions & 19 deletions

File tree

packages/snaps-rpc-methods/jest.config.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ module.exports = deepmerge(baseConfig, {
1010
],
1111
coverageThreshold: {
1212
global: {
13-
branches: 97.28,
14-
functions: 98.84,
13+
branches: 97.29,
14+
functions: 98.85,
1515
lines: 99.14,
1616
statements: 98.81,
1717
},

packages/snaps-rpc-methods/src/permissions.test.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,14 @@ describe('buildSnapRestrictedMethodSpecifications', () => {
197197
it('returns the expected object', () => {
198198
const specifications = buildSnapRestrictedMethodSpecifications(
199199
[],
200-
{},
200+
{
201+
getUnlockPromise: jest.fn(),
202+
getClientCryptography: jest.fn(),
203+
isOnPhishingList: jest.fn(),
204+
maybeUpdatePhishingList: jest.fn(),
205+
getSnapKeyring: jest.fn(),
206+
getPreferences: jest.fn(),
207+
},
201208
new Messenger({ namespace: 'SnapsRestrictedMethods' }),
202209
);
203210
expect(specifications).toMatchInlineSnapshot(`
@@ -316,4 +323,21 @@ describe('buildSnapRestrictedMethodSpecifications', () => {
316323
}
317324
`);
318325
});
326+
327+
it('excludes the specified permissions', () => {
328+
const specifications = buildSnapRestrictedMethodSpecifications(
329+
['snap_dialog'],
330+
{
331+
getUnlockPromise: jest.fn(),
332+
getClientCryptography: jest.fn(),
333+
isOnPhishingList: jest.fn(),
334+
maybeUpdatePhishingList: jest.fn(),
335+
getSnapKeyring: jest.fn(),
336+
getPreferences: jest.fn(),
337+
},
338+
new Messenger({ namespace: 'SnapsRestrictedMethods' }),
339+
);
340+
341+
expect(specifications.snap_dialog).toBeUndefined();
342+
});
319343
});

packages/snaps-rpc-methods/src/permissions.ts

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { selectHooks } from '@metamask/json-rpc-engine/v2';
1+
import { selectHooks, assertExpectedHooks } from '@metamask/json-rpc-engine/v2';
22
import {
33
createRestrictedMethodMessenger,
44
type PermissionConstraint,
@@ -70,27 +70,40 @@ export const buildSnapRestrictedMethodSpecifications = (
7070
excludedPermissions: string[],
7171
hooks: Record<string, unknown>,
7272
messenger: RestrictedMethodMessenger,
73-
) =>
74-
Object.values(restrictedMethodPermissionBuilders).reduce<
73+
) => {
74+
const permissionBuilders = Object.values(
75+
restrictedMethodPermissionBuilders,
76+
).filter((builder) => !excludedPermissions.includes(builder.targetName));
77+
78+
const expectedHookNames = new Set(
79+
permissionBuilders.flatMap((builder) =>
80+
builder.methodHooks
81+
? Object.getOwnPropertyNames(builder.methodHooks)
82+
: [],
83+
),
84+
);
85+
86+
assertExpectedHooks(hooks, expectedHookNames);
87+
88+
return permissionBuilders.reduce<
7589
Record<string, PermissionSpecificationConstraint>
7690
>(
7791
(
7892
specifications,
7993
{ targetName, specificationBuilder, methodHooks, actionNames },
8094
) => {
81-
if (!excludedPermissions.includes(targetName)) {
82-
specifications[targetName] = specificationBuilder({
83-
methodHooks: selectHooks(hooks, methodHooks),
84-
messenger: createRestrictedMethodMessenger({
85-
namespace: targetName,
86-
rootMessenger: messenger,
87-
actionNames: actionNames as readonly [
88-
RestrictedMethodActions['type'],
89-
],
90-
}),
91-
});
92-
}
95+
specifications[targetName] = specificationBuilder({
96+
methodHooks: selectHooks(hooks, methodHooks),
97+
messenger: createRestrictedMethodMessenger({
98+
namespace: targetName,
99+
rootMessenger: messenger,
100+
actionNames: actionNames as readonly [
101+
RestrictedMethodActions['type'],
102+
],
103+
}),
104+
});
93105
return specifications;
94106
},
95107
{},
96108
);
109+
};

packages/snaps-simulation/src/methods/specifications.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ export function getPermissionSpecifications({
7373
hooks,
7474
options,
7575
}: GetPermissionSpecificationsOptions): PermissionSpecificationMap<PermissionSpecificationConstraint> {
76+
const { getClientCryptography } = hooks;
7677
return {
7778
[caip25EndowmentBuilder.targetName]:
7879
caip25EndowmentBuilder.specificationBuilder({}),
@@ -81,11 +82,12 @@ export function getPermissionSpecifications({
8182
EXCLUDED_SNAP_PERMISSIONS,
8283
{
8384
// Shared hooks.
84-
...hooks,
85+
getClientCryptography,
8586

8687
// Snaps-specific hooks.
8788
getPreferences: getGetPreferencesMethodImplementation(options),
8889
getUnlockPromise: asyncResolve(true),
90+
getSnapKeyring: asyncResolve(null),
8991

9092
// TODO: Allow the user to specify the result of this function.
9193
isOnPhishingList: resolve(false),

0 commit comments

Comments
 (0)