Skip to content

feat: add capabilities to keyring endowment#3903

Open
hmalik88 wants to merge 24 commits intomainfrom
hm/add-keyring-capabilities
Open

feat: add capabilities to keyring endowment#3903
hmalik88 wants to merge 24 commits intomainfrom
hm/add-keyring-capabilities

Conversation

@hmalik88
Copy link
Copy Markdown
Contributor

@hmalik88 hmalik88 commented Mar 19, 2026

Adding capabilities to the keyring endowment as part of keyring v2 work (https://github.com/MetaMask/decisions/blob/main/decisions/core/0006-keyring-interface.md)


Note

Medium Risk
Extends the endowment:keyring permission model and manifest validation with a new optional caveat; mistakes could cause snaps’ permissions/manifests to be rejected or misinterpreted across clients.

Overview
Adds a new keyringCapabilities caveat to endowment:keyring, allowing snaps to declare supported keyring capabilities (e.g., scopes and optional BIP-44/private key/custom flags) in initialPermissions/manifests.

Updates the keyring endowment permission spec to accept/validate this optional caveat, adds getKeyringCaveatCapabilities, and adjusts the caveat mapper to emit origin + capabilities caveats (or null when empty). Propagates the new caveat through SDK permission types, snaps-utils caveat enum + runtime struct/assertion + manifest validation, and updates tests/snapshots/coverage thresholds accordingly.

Reviewed by Cursor Bugbot for commit e629c04. Bugbot is set up for automated code reviews on this repo. Configure here.

@hmalik88 hmalik88 changed the title feature: add capabilities to keyring endowment feat: add capabilities to keyring endowment Mar 19, 2026
@socket-security
Copy link
Copy Markdown

socket-security bot commented Mar 19, 2026

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

@hmalik88 hmalik88 marked this pull request as ready for review March 20, 2026 15:07
@hmalik88 hmalik88 requested a review from a team as a code owner March 20, 2026 15:07
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.56%. Comparing base (26ff2aa) to head (e629c04).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3903   +/-   ##
=======================================
  Coverage   98.56%   98.56%           
=======================================
  Files         426      427    +1     
  Lines       12316    12343   +27     
  Branches     1935     1939    +4     
=======================================
+ Hits        12139    12166   +27     
  Misses        177      177           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

endowmentGetter: (_getterOptions?: EndowmentGetterParams) => null,
validator: createGenericPermissionValidator([
{ type: SnapCaveatType.KeyringOrigin },
{ type: SnapCaveatType.KeyringCapabilities, optional: true },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we decide on behaviour if this is not defined? Since we are making it optional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, omission of the capabilities is an implicit indication of using keyring v1

Comment on lines +125 to +139
const caveats = [];

caveats.push({
type: SnapCaveatType.KeyringOrigin,
value: { allowedOrigins: value.allowedOrigins },
});

if (hasProperty(value, 'capabilities')) {
caveats.push({
type: SnapCaveatType.KeyringCapabilities,
value: { capabilities: value.capabilities },
});
}

return { caveats: caveats as unknown as NonEmptyArray<CaveatConstraint> };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a better way of doing this that doesn't require the ugly type cast at the end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I followed the pattern Frederik suggested: #3903 (comment)

Copy link
Copy Markdown
Contributor

@GuillaumeRx GuillaumeRx Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do that to remove the need to cast to unkown :

Suggested change
const caveats = [];
caveats.push({
type: SnapCaveatType.KeyringOrigin,
value: { allowedOrigins: value.allowedOrigins },
});
if (hasProperty(value, 'capabilities')) {
caveats.push({
type: SnapCaveatType.KeyringCapabilities,
value: { capabilities: value.capabilities },
});
}
return { caveats: caveats as unknown as NonEmptyArray<CaveatConstraint> };
const caveats = [];
caveats.push({
type: SnapCaveatType.KeyringOrigin,
value,
});
if (value.capabilities) {
caveats.push({
type: SnapCaveatType.KeyringCapabilities,
value,
});
}
return { caveats: caveats as NonEmptyArray<CaveatConstraint> };

Also I've noticed in Frederik's example that the value is extracted so I don't know if we should do that instead :

Suggested change
const caveats = [];
caveats.push({
type: SnapCaveatType.KeyringOrigin,
value: { allowedOrigins: value.allowedOrigins },
});
if (hasProperty(value, 'capabilities')) {
caveats.push({
type: SnapCaveatType.KeyringCapabilities,
value: { capabilities: value.capabilities },
});
}
return { caveats: caveats as unknown as NonEmptyArray<CaveatConstraint> };
const caveats = [];
caveats.push({
type: SnapCaveatType.KeyringOrigin,
value: value.allowedOrigins,
});
if (value.capabilities) {
caveats.push({
type: SnapCaveatType.KeyringCapabilities,
value: value.capabilities,
});
}
return { caveats: caveats as NonEmptyArray<CaveatConstraint> };

Both works TBH, I'm just wondering why there's a difference between the two

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second one seems to be the standard btw

Copy link
Copy Markdown
Contributor Author

@hmalik88 hmalik88 Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undid extracting the value since it seems we weren't doing that with allowedOrigins before so I'll still follow that pattern. I did however get the unknown cast removed.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants