fix: Place permission middleware ahead of all method implementations #24472
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rekmarks
added
team-snaps
rekmarks
requested review from
zone-live and
a team
and removed request for
FrederikBolding and
zone-live
|
@MetaMask/snaps-devs @MetaMask/mmi tagging you guys to confirm that your RPC methods continue to work as expected. |
rekmarks
force-pushed
the
rekmarks/reorder-permission-middleware
branch
3 times, most recently
from
760c9a6
to
9fb45c4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
rekmarks
force-pushed
the
rekmarks/reorder-permission-middleware
branch
2 times, most recently
from
e39e390
to
653e514
Compare
Builds ready [653e514]
Page Load Metrics (2225 ± 753 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [611dac9]
Page Load Metrics (1833 ± 699 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #24472 +/- ##
===========================================
+ Coverage 67.37% 67.48% +0.11%
===========================================
Files 1278 1289 +11
Lines 49881 50168 +287
Branches 12944 13015 +71
===========================================
+ Hits 33605 33853 +248
- Misses 16276 16315 +39 ☔ View full report in Codecov by Sentry. |
7 tasks
rekmarks
force-pushed
the
rekmarks/reorder-permission-middleware
branch
from
9c346cd
to
d3f1a0a
Compare
rekmarks
commented
May 24, 2024
Comment on lines
+5041
to
+5096
| // If the origin is not in the selectedNetworkController's `domains` state | ||
| // when the provider engine is created, the selectedNetworkController will | ||
| // fetch the globally selected networkClient from the networkController and wrap | ||
| // it in a proxy which can be switched to use its own state if/when the origin | ||
| // is added to the `domains` state | ||
| const proxyClient = | ||
| this.selectedNetworkController.getProviderAndBlockTracker(origin); |
There was a problem hiding this comment.
I moved this declaration closer to where it is used. I do not see how it could result in a behavioral change.
There was a problem hiding this comment.
The thing one could imagine would be if something further up has been relying on specific error being thrown from getProviderAndBlockTracker under certain conditions, but sounds far-fetched and move makes sense to me.
Builds ready [91dc4ea]
Page Load Metrics (991 ± 545 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Builds ready [8f971bf]
Page Load Metrics (1231 ± 610 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
rekmarks
force-pushed
the
rekmarks/reorder-permission-middleware
branch
2 times, most recently
from
58a0b80
to
3175d64
Compare
Builds ready [3175d64]
Page Load Metrics (663 ± 491 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [782af6c]
Page Load Metrics (745 ± 493 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
adonesky1
reviewed
May 30, 2024
app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js
Outdated
Show resolved
Hide resolved
adonesky1
reviewed
May 30, 2024
| @@ -35,4 +34,4 @@ const handlers = [ | |||
| ///: END:ONLY_INCLUDE_IF | |||
| ]; | |||
|
|
|||
| export default handlers; | |||
| export const legacyHandlers = [ethAccounts]; | |||
There was a problem hiding this comment.
not a huge deal but wondering if "legacy" is the right framing here? It seems like the main reason this is separated out is that you need to be able to call this method and receive an empty array if there is no authorization? Perhaps that is a legacy pattern in that we don't want to it to be reused but legacyHandlers implies to me that this method or handler is set for deprecation
There was a problem hiding this comment.
Perhaps that is a legacy pattern in that we don't want to it to be reused
on second thought, maybe this is the proper use of legacy... 🤔
There was a problem hiding this comment.
Hm, well, our implementation of eth_accounts is using a legacy pattern that we should never have cause to return to. However, it falls under a more general category of "middleware that do stuff to requests before the permission system". We could be establishing a pattern for that, but that also includes almost every other middleware in front of the permission middleware...
No, I think legacy is our best bet. We shouldn't do this!
rekmarks
force-pushed
the
rekmarks/reorder-permission-middleware
branch
from
782af6c
to
03017e7
Compare
mcmire
approved these changes
May 31, 2024
Builds ready [7506f83]
Page Load Metrics (294 ± 347 ms)
|
gauthierpetetin
added
the
release-12.0.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds all unrestricted RPC methods to the
unrestrictedMethodsarray passed to the permission controller, and moves the permission middleware ahead of all RPC method implementations insetupProviderEngine. This forces us to add all methods that aren't permission to theunrestrictedMethodsarray, as a "method not found" error will be thrown otherwise. It's safer this way.Due the requirement that it swallows authorization errors, the implementation of
eth_accountsis moved to a new "legacy method middleware", which we by necessity place ahead of the permission middleware. In addition, the responsibility of rejecting intentionally unsupported RPC methods has been moved to a dedicated middleware in front of the legacy method middleware.Related issues
Fixes: #24331
Manual testing steps
Pre-merge author checklist
Pre-merge reviewer checklist