Skip to content

Commit a3872c4

Browse files
fix: Make KeyringOrigin caveat explicitly optional (#3955)
Unintentional or not, `endowment:keyring` has since its implementation allowed no caveats to be passed until 79e8b90. This was caused by the caveat mapper constructing the caveat with an empty object as the `value`, which is valid according to our validation: https://github.com/MetaMask/snaps-skunkworks/blob/fb/keyring-origins-optional/packages/snaps-utils/src/json-rpc.ts#L77 The Bitcoin Snap used in production specifies `endowment:keyring: {}` in its manifest. This PR makes the permission validation explicit in making `KeyringOrigin` optional, including adding a fallback in `getKeyringCaveatOrigins`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adjusts permission validation and defaults for the security-adjacent `endowment:keyring` caveats; incorrect optionality or fallback behavior could inadvertently widen allowed origins if downstream assumes the caveat is always present. > > **Overview** > Makes `endowment:keyring` permission validation explicitly treat the `keyringOrigin` caveat as *optional* (matching other keyring caveats), rather than requiring it to be present. > > Updates `getKeyringCaveatOrigins` to **gracefully default** to `{ allowedOrigins: [] }` when the caveat (or caveats list) is missing, and aligns tests/coverage thresholds with the new behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5ca964e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent c339e31 commit a3872c4

3 files changed

Lines changed: 9 additions & 19 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ module.exports = deepmerge(baseConfig, {
1010
],
1111
coverageThreshold: {
1212
global: {
13-
branches: 96.66,
13+
branches: 96.68,
1414
functions: 99.2,
1515
lines: 99.06,
1616
statements: 98.78,

packages/snaps-rpc-methods/src/endowments/keyring.test.ts

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,9 @@ describe('endowment:keyring', () => {
3030
});
3131

3232
describe('validator', () => {
33-
it('throws if the caveat is not a single "keyringOrigin"', () => {
33+
it('throws if the caveats are invalid', () => {
3434
const specification = keyringEndowmentBuilder.specificationBuilder({});
3535

36-
expect(() =>
37-
specification.validator({
38-
// @ts-expect-error Missing other required permission types.
39-
caveats: undefined,
40-
}),
41-
).toThrow('Expected the following caveats: "keyringOrigin".');
42-
4336
expect(() =>
4437
// @ts-expect-error Missing other required permission types.
4538
specification.validator({
@@ -147,13 +140,13 @@ describe('getKeyringCaveatOrigins', () => {
147140
).toStrictEqual({ allowedOrigins: ['foo.com'] });
148141
});
149142

150-
it('throws if there is no "keyringOrigin" caveat', () => {
151-
expect(() =>
143+
it('returns an empty array when no origins are provided', () => {
144+
expect(
152145
// @ts-expect-error Missing other required permission types.
153146
getKeyringCaveatOrigins({
154-
caveats: [{ type: 'foo', value: 'bar' }],
147+
caveats: null,
155148
}),
156-
).toThrow('Assertion failed.');
149+
).toStrictEqual({ allowedOrigins: [] });
157150
});
158151
});
159152

packages/snaps-rpc-methods/src/endowments/keyring.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
SnapCaveatType,
2121
} from '@metamask/snaps-utils';
2222
import type { Json, NonEmptyArray } from '@metamask/utils';
23-
import { assert, hasProperty, isObject, isPlainObject } from '@metamask/utils';
23+
import { hasProperty, isObject, isPlainObject } from '@metamask/utils';
2424

2525
import { createGenericPermissionValidator } from './caveats';
2626
import { SnapEndowments } from './enum';
@@ -58,7 +58,7 @@ const specificationBuilder: PermissionSpecificationBuilder<
5858
],
5959
endowmentGetter: (_getterOptions?: EndowmentGetterParams) => null,
6060
validator: createGenericPermissionValidator([
61-
{ type: SnapCaveatType.KeyringOrigin },
61+
{ type: SnapCaveatType.KeyringOrigin, optional: true },
6262
{ type: SnapCaveatType.KeyringCapabilities, optional: true },
6363
{ type: SnapCaveatType.MaxRequestTime, optional: true },
6464
]),
@@ -145,8 +145,6 @@ export function getKeyringCaveatMapper(
145145
*
146146
* @param permission - The permission to get the caveat value from.
147147
* @returns The caveat value.
148-
* @throws If the permission does not have a valid {@link KeyringOrigins}
149-
* caveat.
150148
*/
151149
export function getKeyringCaveatOrigins(
152150
permission?: PermissionConstraint,
@@ -155,8 +153,7 @@ export function getKeyringCaveatOrigins(
155153
(permCaveat) => permCaveat.type === SnapCaveatType.KeyringOrigin,
156154
) as Caveat<string, KeyringOrigins> | undefined;
157155

158-
assert(caveat);
159-
return caveat.value;
156+
return caveat?.value ?? { allowedOrigins: [] };
160157
}
161158

162159
/**

0 commit comments

Comments
 (0)