Add `PermittedHandlerExport` to `permission-controller` #1184

Mrtenz · 2023-04-14T11:19:33Z

Description

The primary intent of this change is to move PermittedHandlerExport from @metamask/types to the permission-controller package. This required some additional changes however.

@metamask/json-rpc-engine was updated to the latest version, so we can use the latest types for the PermittedHandlerExport. This required updating @metamask/rpc-errors (previously eth-rpc-errors) and @metamask/utils too.

See the changelog below for a more detailed explanation.

Changes

ADDED: Add PermittedHandlerExport type.
CHANGED: Bump @metamask/json-rpc-engine, @metamask/rpc-errors, and @metamask/utils in permission-controller to the latest version.
- Usage of eth-rpc-errors has been changed to use @metamask/rpc-errors.
CHANGED: RestrictedMethodParameters now accepts undefined instead of void, as void is not compatible with our JsonRpcParams type.
- I'm unsure how this is used externally, but this may be a breaking change.
FIXED: Dedupe yarn.lock.
- There were a lot of duplicate package versions before, which have been deduped by running yarn dedupe.

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
I've highlighted breaking changes using the "BREAKING" category above as appropriate

socket-security · 2023-04-14T11:21:14Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore @metamask/json-rpc-engine@7.0.0

⚠️ No contributors or author data

Package does not specify a list of contributors or an author in package.json.

Add a author field or contributors array to package.json.

Package	Location	Source
@metamask/json-rpc-engine@7.0.0 (added)	package.json	`packages/permission-controller/package.json`

Pull request alert summary

Issue	Status
Critical CVE	✅ 0 issues
CVE	✅ 0 issues
Mild CVE	✅ 0 issues
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script confusion	✅ 0 issues
Bin script shell injection	✅ 0 issues
Filesystem access	✅ 0 issues
Network access	✅ 0 issues
Shell access	✅ 0 issues
Debug access	✅ 0 issues
Long strings	✅ 0 issues
High entropy strings	✅ 0 issues
URL strings	✅ 0 issues
Uses eval	✅ 0 issues
Dynamic require	✅ 0 issues
Environment variable access	✅ 0 issues
Missing dependency	✅ 0 issues
Unused dependency	✅ 0 issues
Peer dependency	✅ 0 issues
Uncaught optional dependency	✅ 0 issues
Unresolved require	✅ 0 issues
Extraneous dependency	✅ 0 issues
Obfuscated require	✅ 0 issues
Obfuscated code	✅ 0 issues
Minified code	✅ 0 issues
Bidirectional unicode control characters	✅ 0 issues
Zero width unicode chars	✅ 0 issues
Bad text encoding	✅ 0 issues
Unicode homoglyphs	✅ 0 issues
Invisible chars	✅ 0 issues
Suspicious strings	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
GitHub dependency	✅ 0 issues
File dependency	✅ 0 issues
No tests	✅ 0 issues
No repository	✅ 0 issues
Bad semver	✅ 0 issues
Bad dependency semver	✅ 0 issues
No v1	✅ 0 issues
No website	✅ 0 issues
No bug tracker	✅ 0 issues
No contributors or author data	⚠️ 1 issue
CommonJS depending on ESModule	✅ 0 issues
Empty package	✅ 0 issues
Trivial Package	✅ 0 issues
No README	✅ 0 issues
Deprecated	✅ 0 issues
Chronological version anomaly	✅ 0 issues
Semver anomaly	✅ 0 issues
New author	✅ 0 issues
Unstable ownership	✅ 0 issues
Non-existent author	✅ 0 issues
Unmaintained	✅ 0 issues
Unpublished package	✅ 0 issues
Major refactor	✅ 0 issues
Missing package tarball	✅ 0 issues
Unsafe copyright	✅ 0 issues
License change	✅ 0 issues
Non OSI license	✅ 0 issues
Deprecated license	✅ 0 issues
Missing license	✅ 0 issues
Non SPDX license	✅ 0 issues
Unclear license	✅ 0 issues
Mixed license	✅ 0 issues
Legal notice	✅ 0 issues
Modified license	✅ 0 issues
Modified license exception	✅ 0 issues
License exception	✅ 0 issues
Deprecated SPDX exception	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues
AI detected security risk	✅ 0 issues
AI warning	✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	`+/-` Transitive Count	Publisher
@metamask/json-rpc-engine@7.0.0	None	`+1`	metamaskbot
@metamask/rpc-errors@5.1.1	None	`+0`	metamaskbot

🚮 Removed packages: @metamask/types@1.1.0

packages/permission-controller/package.json

mcmire

Might be good to get a second review on this, but seems like a reasonable change to me.

packages/permission-controller/src/permitted-handler.ts

legobeat · 2023-04-19T03:12:30Z

@Mrtenz Changes look reasonable to me but especially given that effectively used versions of ethereumjs and babel packages and the types are nontrivial combined with that these commits will be squashed:

could we break out all the dependency updates into a separate PR, which would precede the permission-controller changes in this PR?

legobeat · 2023-04-19T03:32:21Z

55e256b fixes this error (but as noted above IMO this should be broken out with the upgrades)

legobeat · 2023-04-19T04:48:16Z

@metamask/utils upgrade and part of the dedupes (including the parts triggering the hexToBN regression) broken out into #1211.

Mrtenz · 2023-04-19T09:02:31Z

Thanks @legobeat! Will update this PR once that's merged.

Mrtenz · 2023-04-19T09:32:34Z

Blocked by MetaMask/rpc-errors#91.

packages/permission-controller/src/rpc-methods/requestPermissions.test.ts

legobeat · 2023-04-19T12:29:20Z

Getting there! https://github.com/MetaMask/core/actions/runs/4743019748/jobs/8422056659?pr=1184#step:5:86

FrederikBolding · 2023-04-19T14:12:29Z

packages/permission-controller/src/PermissionController.test.ts

@@ -1,5 +1,5 @@
 import assert from 'assert';
-import { JsonRpcEngine, PendingJsonRpcResponse } from 'json-rpc-engine';
+import { JsonRpcEngine } from '@metamask/json-rpc-engine';


I wonder if we will get type issues by just updating json-rpc-engine in this package alone 🤔

Also this will make the PermissionController minimum Node 16. Are we ready for that in this repo?

Good catch @FrederikBolding.

We don't currently supporting bumping the minimum Node version for select packages in this monorepo. The Node version would have to be bumped across the board.

~~I'm not sure if we are ready to do this~~ Update: It looks like we are getting pretty close to bumping to Node 16 on mobile, so this may not be a problem, but either way, it might be good to bump @metamask/json-rpc-engine in another PR for visibility, if that's possible?

Bumping @metamask/json-rpc-engine requires bumping @metamask/utils and @metamask/rpc-errors, due to type conflicts with @metamask/types otherwise. I could cherry-pick the first commit into a separate PR, if that makes it more clear?

As of today bumping this library to 16 should not be a problem as mobile is now running Node 16.

@Mrtenz Cherry-picking the first commit in a separate PR makes sense. We should also make a new PR that bumps the minimum version of Node to 16 across the board so we can codify that in the next release.

I tried cherry-picking without including the new PermittedHandlerExport, but the types are not compatible between @metamask/types and @metamask/utils, so that doesn't really work.

All packages in this repo now require Node 16, so that part is at least done.

mcmire · 2023-04-27T17:55:53Z

Happy to re-review this when the conflicts are resolved.

…sk/utils` in `permission-controller` In order to remove the dependency on `@metamask/types` inside `snaps-monorepo`, we need the `PermittedHandlerExport` type. This type depends on types from the `@metamask/json-rpc-engine` package, which were previously duplicated inside `@metamask/types`. In this commit, I have moved `PermittedHandlerExport` to this package, which required bumping `@metamask/json-rpc-engine`, `@metamask/rpc-errors`, and `@metamask/utils`.

This commit updates "@metamask/rpc-errors" package to version "^5.1.1" in package.json and yarn.lock files. This will resolve an issue related to an internal error in PermissionController caused by this dependency.

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>

Some error objects should have a 'cause' field that will capture inner causes of particular errors.

Mrtenz requested a review from a team as a code owner April 14, 2023 11:19

Mrtenz mentioned this pull request Apr 14, 2023

Bring json-rpc-engine types up to date MetaMask/types#19

Closed

legobeat reviewed Apr 15, 2023

View reviewed changes

packages/permission-controller/package.json Outdated Show resolved Hide resolved

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from 49be581 to 9d183f3 Compare April 17, 2023 10:02

mcmire previously approved these changes Apr 18, 2023

View reviewed changes

legobeat reviewed Apr 19, 2023

View reviewed changes

packages/permission-controller/src/permitted-handler.ts Outdated Show resolved Hide resolved

legobeat dismissed mcmire’s stale review via 7814e36 April 19, 2023 03:30

legobeat force-pushed the mrtenz/permitted-handler-type branch 2 times, most recently from 4eff32a to 55e256b Compare April 19, 2023 03:43

legobeat mentioned this pull request Apr 19, 2023

deps: bump @metamask/utils to 5.0.1 #1211

Merged

3 tasks

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from 55e256b to 8e80463 Compare April 19, 2023 09:06

legobeat mentioned this pull request Apr 19, 2023

deps: eth-rpc-errors@4.0.0->4.0.2 #1215

Merged

3 tasks

legobeat reviewed Apr 19, 2023

View reviewed changes

packages/permission-controller/src/rpc-methods/requestPermissions.test.ts Outdated Show resolved Hide resolved

legobeat mentioned this pull request Apr 19, 2023

deps: dedupe dependencies #1216

Merged

3 tasks

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from bc01c35 to 394834f Compare April 19, 2023 11:59

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from 8ccde14 to 2c37d50 Compare April 19, 2023 12:54

FrederikBolding reviewed Apr 19, 2023

View reviewed changes

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from a785e17 to 6003a97 Compare April 20, 2023 12:04

FrederikBolding mentioned this pull request Apr 25, 2023

BREAKING: Bump to Node 16 #1262

Merged

Mrtenz and others added 3 commits April 28, 2023 10:43

Update "@metamask/rpc-errors" to version "^5.1.1"

ecd9c6a

This commit updates "@metamask/rpc-errors" package to version "^5.1.1" in package.json and yarn.lock files. This will resolve an issue related to an internal error in PermissionController caused by this dependency.

Update packages/permission-controller/src/permitted-handler.ts

327803b

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>

Add a 'cause' field to error objects in tests

f05f318

Some error objects should have a 'cause' field that will capture inner causes of particular errors.

Mrtenz force-pushed the mrtenz/permitted-handler-type branch from 6003a97 to f05f318 Compare April 28, 2023 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `PermittedHandlerExport` to `permission-controller` #1184

Add `PermittedHandlerExport` to `permission-controller` #1184

Mrtenz commented Apr 14, 2023

socket-security bot commented Apr 14, 2023 •

edited

Loading

mcmire left a comment

legobeat commented Apr 19, 2023 •

edited

Loading

legobeat commented Apr 19, 2023 •

edited

Loading

legobeat commented Apr 19, 2023 •

edited

Loading

Mrtenz commented Apr 19, 2023 •

edited

Loading

Mrtenz commented Apr 19, 2023

legobeat commented Apr 19, 2023

FrederikBolding Apr 19, 2023

FrederikBolding Apr 19, 2023

mcmire Apr 19, 2023 •

edited

Loading

Mrtenz Apr 20, 2023

FrederikBolding Apr 20, 2023

mcmire Apr 20, 2023 •

edited

Loading

Mrtenz Apr 25, 2023

FrederikBolding Apr 26, 2023

mcmire commented Apr 27, 2023

Add PermittedHandlerExport to permission-controller #1184

Are you sure you want to change the base?

Add PermittedHandlerExport to permission-controller #1184

Conversation

Mrtenz commented Apr 14, 2023

Description

Changes

Checklist

socket-security bot commented Apr 14, 2023 • edited Loading

mcmire left a comment

Choose a reason for hiding this comment

legobeat commented Apr 19, 2023 • edited Loading

legobeat commented Apr 19, 2023 • edited Loading

legobeat commented Apr 19, 2023 • edited Loading

Mrtenz commented Apr 19, 2023 • edited Loading

Mrtenz commented Apr 19, 2023

legobeat commented Apr 19, 2023

FrederikBolding Apr 19, 2023

Choose a reason for hiding this comment

FrederikBolding Apr 19, 2023

Choose a reason for hiding this comment

mcmire Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Mrtenz Apr 20, 2023

Choose a reason for hiding this comment

FrederikBolding Apr 20, 2023

Choose a reason for hiding this comment

mcmire Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Mrtenz Apr 25, 2023

Choose a reason for hiding this comment

FrederikBolding Apr 26, 2023

Choose a reason for hiding this comment

mcmire commented Apr 27, 2023

Add `PermittedHandlerExport` to `permission-controller` #1184

Add `PermittedHandlerExport` to `permission-controller` #1184

socket-security bot commented Apr 14, 2023 •

edited

Loading

legobeat commented Apr 19, 2023 •

edited

Loading

legobeat commented Apr 19, 2023 •

edited

Loading

legobeat commented Apr 19, 2023 •

edited

Loading

Mrtenz commented Apr 19, 2023 •

edited

Loading

mcmire Apr 19, 2023 •

edited

Loading

mcmire Apr 20, 2023 •

edited

Loading