SemVer handling is now through opaque types #961

ritave · 2022-11-14T19:42:56Z

Manifest handling is very sensitive part of Snaps. It's loaded inside the main extension process and contains a list of permissions.

Any tools that can help us make handling manifest related operations more secure are deeply appreciated.

This PR introduces two new types SemVerRange and SemVerVersion, both of which are strings, but behave as opaque types. Meaning that any type casting between those types needs to be explicit and disallows duck typing from and to string.

This provides not only better intention handling in the code, it increases security by requiring validation before casting.

Mrtenz

I'm not sure if I agree with this in general. I'd prefer if we could implement this in a way with template literal types, i.e., ${string}.${string}.${string} for proper version types. This avoids the need for type casts as well.

Mrtenz · 2022-11-14T19:54:39Z

packages/snaps-controllers/src/cronjob/CronjobController.test.ts

@@ -92,7 +93,7 @@ describe('CronjobController', () => {

    const callActionMock = jest
      .spyOn(controllerMessenger, 'call')
-      .mockImplementation((method) => {
+      .mockImplementation((method, ..._) => {


What's this for?

Visual Studio Code is using new version of TS, and it's complaining that we're not accepting all the parameters of the call. This silences TypeScript in VSC, and will be useful when we're going to upgrade to newer version.

packages/snaps-utils/src/versions.test.ts

ritave · 2022-11-14T20:04:03Z

I'm not sure if I agree with this in general. I'd prefer if we could implement this in a way with template literal types, i.e., ${string}.${string}.${string} for proper version types. This avoids the need for type casts as well.

The whole point is to have explicit type casts. Otherwise TypeScript will happily duck-type a version between range and we'll not find it during testing.

For example 1.2.3 is both a valid version and a range. Without opaque types it'll be accepted into wrong function, breaking the assumptions our code has.

Additionally ${string}.${string}.${string} is invalid, as it doesn't validate pre-releases properly. Having the type follow the full logic of semver package might not be reasonable.

I've worked with Opaque Types before in TypeScript, and believe me, it's the only way to properly disable duck typing.

ritave · 2022-11-15T20:30:55Z

packages/snaps-utils/src/versions.ts

+ * @see {@link assertIsSemVerRange}
+ * @see {@link isValidSemVerRange}
+ */
+export type SemVerRange = Opaque<string, typeof semVerRange>;


TIL - interfaces can be self recursive but types can't.

// works interface A { b: Opaque<string, A>; } // errors type B = Opaque<string, B>;

hmalik88 · 2022-11-16T05:57:36Z

packages/snaps-utils/src/test-utils/snap.ts

@@ -27,6 +28,16 @@ export const MOCK_SNAP_ID = 'npm:@metamask/example-snap';
 export const MOCK_LOCAL_SNAP_ID = 'local:@metamask/example-snap';
 export const MOCK_ORIGIN = 'example.com';

+type GetPersistedSnapObjectOptions = Partial<Omit<PersistedSnap, 'version'>> & {
+  version?: string | SemVerVersion;


Why use omit here (and in the other instances that you do this) when you can just change the original type to be SemVerVersion? Also I don't understand why we would want it to be union type w/ string? I thought the whole point of the opaque types was to narrow types?

If you need to not change original types for some reason, here's a utility type to make things a tiny bit less redundant and clearer to understand:

type MakeSemVer<T> = { [K in keyof T]: K extends 'version' ? SemVerVersion | string : T[K]; } // then you can do something like below type GetPersistedSnapObjectOptions = Partial<MakeSemVer<PersistedSnap>>

There's tens if not hundreds of tests that use those functions, and we would have to update all of them to have a type cast.

This is for backwards compatibility because of that.

The whole source code (not tests) is type cast though.

packages/snaps-utils/src/versions.ts

packages/snaps-controllers/src/snaps/SnapController.test.ts

packages/snaps-controllers/src/cronjob/CronjobController.test.ts

Mrtenz · 2022-11-16T09:16:08Z

packages/snaps-controllers/src/snaps/SnapController.test.ts

@@ -2593,7 +2595,7 @@ describe('SnapController', () => {
        .mockImplementationOnce(async () => {
          const manifest: SnapManifest = {
            ...getSnapManifest(),
-            version: '1.1.0',
+            version: '1.1.0' as SemVerVersion,


This could be moved to a constant on top of the file.

Not possible in this case. Tests that use specific versions instead of creation utils, do it because they're testing version/updates functionality.

Mrtenz · 2022-11-16T09:16:49Z

packages/snaps-utils/src/test-utils/snap.ts

@@ -27,6 +28,16 @@ export const MOCK_SNAP_ID = 'npm:@metamask/example-snap';
 export const MOCK_LOCAL_SNAP_ID = 'local:@metamask/example-snap';
 export const MOCK_ORIGIN = 'example.com';

+type GetPersistedSnapObjectOptions = Partial<Omit<PersistedSnap, 'version'>> & {
+  version?: string | SemVerVersion;


Why do we accept string here?

#961 (comment)

Mrtenz

Just some small comments.

Mrtenz · 2022-11-18T13:59:29Z

packages/snaps-utils/package.json

@@ -62,7 +62,8 @@
    "rfdc": "^1.3.0",
    "semver": "^7.3.7",
    "ses": "^0.17.0",
-    "superstruct": "^0.16.7"
+    "superstruct": "^0.16.7",
+    "ts-opaque": "^3.0.1"


Please remove.

Mrtenz · 2022-11-18T14:02:58Z

packages/snaps-utils/src/versions.ts

-export function resolveVersion(version?: Json): Json {
+export function resolveVersionRange(
+  version?: Json,
+): [undefined, SemVerRange] | [Error, undefined] {


Suggested change

): [undefined, SemVerRange] | [Error, undefined] {

): [error: undefined, range: SemVerRange] | [error: Error, range: undefined] {

codecov-commenter · 2022-11-18T14:19:28Z

Codecov Report

Merging #961 (07d0650) into main (fb6989c) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
+ Coverage   92.58%   92.67%   +0.08%     
==========================================
  Files          93       93              
  Lines        9116     9226     +110     
  Branches      890      894       +4     
==========================================
+ Hits         8440     8550     +110     
  Misses        676      676

Impacted Files	Coverage Δ
packages/snaps-cli/src/cmds/init/initHandler.ts	`100.00% <100.00%> (ø)`
packages/snaps-utils/src/manifest/validation.ts	`100.00% <100.00%> (ø)`
packages/snaps-utils/src/snaps.ts	`98.84% <100.00%> (+<0.01%)`	⬆️
packages/snaps-utils/src/types.ts	`100.00% <100.00%> (ø)`
packages/snaps-utils/src/versions.ts	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ritave requested a review from a team as a code owner November 14, 2022 19:42

Mrtenz requested changes Nov 14, 2022

View reviewed changes

ritave force-pushed the ritave/opaque branch 2 times, most recently from b2f5207 to 90a8f1f Compare November 15, 2022 19:58

ritave commented Nov 15, 2022

View reviewed changes

hmalik88 reviewed Nov 16, 2022

View reviewed changes

Mrtenz requested changes Nov 16, 2022

View reviewed changes

ritave force-pushed the ritave/opaque branch from 90a8f1f to c20ab47 Compare November 16, 2022 16:22

ritave added 10 commits November 18, 2022 14:24

[TMP] Added SemVer opaque types

70ca4bf

Added opaque types for SemVers

1bf305b

Better documentation

c97d2a0

Removed versions.test.ts utillity functions

a7a11f0

Used ts-opaque

f969ada

Removed debug comments

1207f4d

Coverage

b8d3f94

PR review

08b53e5

Coverage

1ab518a

Lint

eb04a9b

ritave force-pushed the ritave/opaque branch from c20ab47 to eb04a9b Compare November 18, 2022 13:40

Removed ts-opaque dependency

338856d

Mrtenz requested changes Nov 18, 2022

View reviewed changes

ritave added 2 commits November 18, 2022 15:08

PR review

b0c909b

Yarn.lock

07d0650

Mrtenz approved these changes Nov 18, 2022

View reviewed changes

ritave merged commit b6471f6 into main Nov 18, 2022

ritave deleted the ritave/opaque branch November 18, 2022 14:27

ritave mentioned this pull request Jan 10, 2023

Add tests for opaque types MetaMask/utils#73

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SemVer handling is now through opaque types #961

SemVer handling is now through opaque types #961

ritave commented Nov 14, 2022

Mrtenz left a comment

Mrtenz Nov 14, 2022

ritave Nov 14, 2022 •

edited

ritave commented Nov 14, 2022 •

edited

ritave Nov 15, 2022 •

edited

hmalik88 Nov 16, 2022 •

edited

hmalik88 Nov 16, 2022 •

edited

ritave Nov 16, 2022

Mrtenz Nov 16, 2022

ritave Nov 16, 2022

Mrtenz Nov 16, 2022

ritave Nov 16, 2022

Mrtenz left a comment

Mrtenz Nov 18, 2022

Mrtenz Nov 18, 2022

codecov-commenter commented Nov 18, 2022 •

edited

	): [undefined, SemVerRange] \| [Error, undefined] {
	): [error: undefined, range: SemVerRange] \| [error: Error, range: undefined] {

SemVer handling is now through opaque types #961

SemVer handling is now through opaque types #961

Conversation

ritave commented Nov 14, 2022

Mrtenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritave Nov 14, 2022 • edited

Choose a reason for hiding this comment

ritave commented Nov 14, 2022 • edited

ritave Nov 15, 2022 • edited

Choose a reason for hiding this comment

hmalik88 Nov 16, 2022 • edited

Choose a reason for hiding this comment

hmalik88 Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mrtenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 18, 2022 • edited

Codecov Report

ritave Nov 14, 2022 •

edited

ritave commented Nov 14, 2022 •

edited

ritave Nov 15, 2022 •

edited

hmalik88 Nov 16, 2022 •

edited

hmalik88 Nov 16, 2022 •

edited

codecov-commenter commented Nov 18, 2022 •

edited