-
Notifications
You must be signed in to change notification settings - Fork 642
fix: Cleanup endowment handling of maxRequestTime
#3826
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
Conversation
| @@ -100,12 +100,11 @@ export function getCronjobCaveatJobs( | |||
| return null; | |||
| } | |||
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.
Wondering if this check (or assertion in some other endowments) is still necessary. We can maybe just rewrite to permission?.caveats?.find?.(...), right?
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.
Yeah, good point!
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.
For assertions, I've kept those around when the caveat is required.
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.
Yeah, but we assert after permission.caveats.find() in those cases as well.
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.
Ah, yes, we can dedupe
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3826 +/- ##
=======================================
Coverage 98.39% 98.40%
=======================================
Files 430 430
Lines 12454 12448 -6
Branches 1936 1933 -3
=======================================
- Hits 12254 12249 -5
+ Misses 200 199 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Several endowments had half-baked support for
maxRequestTimethat would then fail their getters or had validation setup but didn't allow the caveat at the permission level. This PR standardizes and allows the caveat everywhere as intended.Note
Medium Risk
Touches snap permission specifications and caveat getters, which can affect runtime permission validation and how endowment requests are interpreted. Changes are straightforward but broad across multiple endowments and tests.
Overview
Standardizes endowment caveat support for
maxRequestTime. Updates multiple endowment permission specs (assets,cronjob,signature-insight, plus previously caveat-lesspage-home,page-settings, andlifecycle-hooks) to explicitly allowmaxRequestTimeand wire up a generic validator where missing.Fixes caveat getters to handle multiple caveats.
getCronjobCaveatJobs,getRpcCaveatOrigins,getKeyringCaveatOrigins,getSignatureOriginCaveat, andgetTransactionOriginCaveatnow search for the relevant caveat type instead of asserting there is exactly one caveat, with tests updated accordingly.Updates inline snapshots in
permissionsandsnaps-simulationto reflect the new allowed caveats/validators, and tweaks Jest coverage thresholds.Written by Cursor Bugbot for commit 5fb24d5. This will update automatically on new commits. Configure here.