Skip to content

fix: Make KeyringOrigin caveat explicitly optional#3955

Merged
FrederikBolding merged 3 commits intomainfrom
fb/keyring-origins-optional
Apr 14, 2026
Merged

fix: Make KeyringOrigin caveat explicitly optional#3955
FrederikBolding merged 3 commits intomainfrom
fb/keyring-origins-optional

Conversation

@FrederikBolding
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding commented Apr 14, 2026

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.


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.

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.14%. Comparing base (c339e31) to head (5ca964e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3955      +/-   ##
==========================================
+ Coverage   98.56%   99.14%   +0.57%     
==========================================
  Files         428      346      -82     
  Lines       12348     9314    -3034     
  Branches     1939     1539     -400     
==========================================
- Hits        12171     9234    -2937     
+ Misses        177       80      -97     

☔ 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.

@FrederikBolding FrederikBolding marked this pull request as ready for review April 14, 2026 14:32
@FrederikBolding FrederikBolding requested a review from a team as a code owner April 14, 2026 14:32
@FrederikBolding FrederikBolding requested a review from Mrtenz April 14, 2026 14:36
@FrederikBolding FrederikBolding added this pull request to the merge queue Apr 14, 2026
Merged via the queue into main with commit a3872c4 Apr 14, 2026
131 checks passed
@FrederikBolding FrederikBolding deleted the fb/keyring-origins-optional branch April 14, 2026 14:46
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.

2 participants