Skip to content
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

feat: allow wildcards in allowedOrigins #2458

Merged
merged 6 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/snaps-utils/coverage.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 96.7,
"functions": 98.72,
"lines": 98.81,
"statements": 94.79
"branches": 96.72,
"functions": 98.75,
"lines": 98.82,
"statements": 94.84
}
83 changes: 83 additions & 0 deletions packages/snaps-utils/src/json-rpc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ describe('assertIsRpcOrigins', () => {
'Invalid JSON-RPC origins: Must specify at least one JSON-RPC origin.',
);
});

it('throws if allowedOrigins contains too many wildcards', () => {
expect(() =>
assertIsRpcOrigins({ allowedOrigins: ['*.*.metamask.***'] }),
).toThrow(
'Invalid JSON-RPC origins: At path: allowedOrigins.0 -- No more than two wildcards (*) are allowed in "allowedOrigins"',
);
});
});

describe('assertIsKeyringOrigin', () => {
Expand Down Expand Up @@ -152,6 +160,81 @@ describe('isOriginAllowed', () => {
expect(isOriginAllowed(origins, SubjectType.Snap, 'foo')).toBe(false);
expect(isOriginAllowed(origins, SubjectType.Website, 'bar')).toBe(false);
});

it('supports wildcards', () => {
const origins: RpcOrigins = {
allowedOrigins: ['*'],
};

expect(isOriginAllowed(origins, SubjectType.Snap, 'foo')).toBe(true);
expect(isOriginAllowed(origins, SubjectType.Website, 'bar')).toBe(true);
});

it('supports prefixes with wildcards', () => {
const origins: RpcOrigins = {
allowedOrigins: ['https://*', 'npm:*'],
};

expect(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are maybe a bit more readable with it.each.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find it.each that readable personally tbh 😅

isOriginAllowed(
origins,
SubjectType.Website,
'https://snaps.metamask.io',
),
).toBe(true);
expect(isOriginAllowed(origins, SubjectType.Snap, 'npm:filsnap')).toBe(
true,
);
expect(
isOriginAllowed(origins, SubjectType.Website, 'http://snaps.metamask.io'),
).toBe(false);
expect(
isOriginAllowed(origins, SubjectType.Snap, 'local:http://localhost:8080'),
).toBe(false);
});

it('supports partial strings with wildcards', () => {
const origins: RpcOrigins = {
allowedOrigins: ['*.metamask.io'],
};

expect(
isOriginAllowed(
origins,
SubjectType.Website,
'https://snaps.metamask.io',
),
).toBe(true);
expect(
isOriginAllowed(origins, SubjectType.Website, 'https://foo.metamask.io'),
).toBe(true);
});

it('supports multiple wildcards', () => {
const origins: RpcOrigins = {
allowedOrigins: ['*.metamask.*'],
};

expect(
isOriginAllowed(
origins,
SubjectType.Website,
'https://snaps.metamask.io',
),
).toBe(true);
expect(
isOriginAllowed(origins, SubjectType.Website, 'https://foo.metamask.io'),
).toBe(true);
expect(
isOriginAllowed(origins, SubjectType.Website, 'https://foo.metamask.dk'),
).toBe(true);
expect(
isOriginAllowed(origins, SubjectType.Website, 'https://foo.metamask2.io'),
).toBe(false);
expect(
isOriginAllowed(origins, SubjectType.Website, 'https://ametamask2.io'),
).toBe(false);
});
});

describe('assertIsJsonRpcSuccess', () => {
Expand Down
41 changes: 38 additions & 3 deletions packages/snaps-utils/src/json-rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,28 @@ import {
import type { Infer } from 'superstruct';
import { array, boolean, object, optional, refine, string } from 'superstruct';

const AllowedOriginsStruct = array(
refine(string(), 'Allowed origin', (value) => {
const wildcards = value.split('').reduce((accumulator, character) => {
if (character === '*') {
return accumulator + 1;
}
return accumulator;
}, 0);
FrederikBolding marked this conversation as resolved.
Show resolved Hide resolved

if (wildcards > 2) {
return 'No more than two wildcards (*) are allowed in "allowedOrigins".';
FrederikBolding marked this conversation as resolved.
Show resolved Hide resolved
}

return true;
}),
);

export const RpcOriginsStruct = refine(
object({
dapps: optional(boolean()),
snaps: optional(boolean()),
allowedOrigins: optional(array(string())),
allowedOrigins: optional(AllowedOriginsStruct),
}),
'RPC origins',
(value) => {
Expand Down Expand Up @@ -58,7 +75,7 @@ export function assertIsRpcOrigins(
}

export const KeyringOriginsStruct = object({
allowedOrigins: optional(array(string())),
allowedOrigins: optional(AllowedOriginsStruct),
});

export type KeyringOrigins = Infer<typeof KeyringOriginsStruct>;
Expand All @@ -84,6 +101,20 @@ export function assertIsKeyringOrigins(
);
}

/**
* Create regular expression for matching against an origin while allowing wildcards.
*
* @param matcher - The string to create the regular expression with.
* @returns The regular expression.
*/
function createOriginRegExp(matcher: string) {
// Escape potential Regex characters
const escaped = matcher.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&');
// Support wildcards
const regex = escaped.replace(/\*/gu, '.*');
return RegExp(regex, 'u');
}

/**
* Check if the given origin is allowed by the given JSON-RPC origins object.
*
Expand All @@ -103,7 +134,11 @@ export function isOriginAllowed(
}

// If the origin is in the `allowedOrigins` list, it is allowed.
if (origins.allowedOrigins?.includes(origin)) {
if (
origins.allowedOrigins
?.map(createOriginRegExp)
.some((regex) => regex.test(origin))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid creating regex unnecessarily, what do you think of

function checkOrigin(origin: string, originSpecifier: string) {
  if (originSpecifier === '*') {
    return true;
  }

  // Create regex and test.
}


// ...
origins.allowedOrigins?.some((specifier) => checkOrigin(origin, specifier))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added one more optimization to this

) {
return true;
}

Expand Down
Loading