From a9db733a80b0942fe26016b149d7e49f39c1a59c Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Wed, 28 Feb 2024 14:39:03 -0500 Subject: [PATCH 1/6] Feat: added check for async requests to enforce redirect url is in the snap's allowed origins --- src/SnapKeyring.test.ts | 77 +++++++++++++++++++++++++++++++++++++++++ src/SnapKeyring.ts | 15 ++++++++ 2 files changed, 92 insertions(+) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index febd48fc..43c5c5c4 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -316,6 +316,19 @@ describe('SnapKeyring', () => { ])('returns a redirect %s', async (redirect) => { const spy = jest.spyOn(console, 'log').mockImplementation(); + const snapObject = { + id: snapId, + manifest: { + initialPermissions: { + 'endowment:keyring': { + allowedOrigins: ['https://example.com'], + }, + }, + }, + enabled: true, + }; + mockSnapController.get.mockReturnValue(snapObject); + mockSnapController.handleRequest.mockResolvedValue({ pending: true, redirect, @@ -353,6 +366,70 @@ describe('SnapKeyring', () => { spy.mockRestore(); }); + it('throws an error if async request redirect url is not an allowed origin', async () => { + const redirect = { + message: 'Go to dapp to continue.', + url: 'https://notallowed.com/sign?tx=1234', + }; + const { origin } = new URL(redirect.url); + + const snapObject = { + id: snapId, + manifest: { + initialPermissions: { + 'endowment:keyring': { + allowedOrigins: ['https://allowed.com'], + }, + }, + }, + enabled: true, + }; + mockSnapController.get.mockReturnValue(snapObject); + + mockSnapController.handleRequest.mockResolvedValue({ + pending: true, + redirect, + }); + const requestPromise = keyring.signPersonalMessage( + accounts[0].address, + 'hello', + ); + + await expect(requestPromise).rejects.toThrow( + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + ); + }); + + it('throws an error if no allowed origins and async request redirect url', async () => { + const redirect = { + message: 'Go to dapp to continue.', + url: 'https://notallowed.com/sign?tx=1234', + }; + const { origin } = new URL(redirect.url); + + const snapObject = { + id: snapId, + manifest: { + initialPermissions: {}, + }, + enabled: true, + }; + mockSnapController.get.mockReturnValue(snapObject); + + mockSnapController.handleRequest.mockResolvedValue({ + pending: true, + redirect, + }); + const requestPromise = keyring.signPersonalMessage( + accounts[0].address, + 'hello', + ); + + await expect(requestPromise).rejects.toThrow( + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + ); + }); + it('rejects an async request', async () => { mockSnapController.handleRequest.mockResolvedValue({ pending: true, diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts index 5e72c60d..ff0cd39c 100644 --- a/src/SnapKeyring.ts +++ b/src/SnapKeyring.ts @@ -436,6 +436,21 @@ export class SnapKeyring extends EventEmitter { // If the snap answers asynchronously, we will inform the user with a redirect if (response.redirect?.message || response.redirect?.url) { const { message = '', url = '' } = response.redirect; + + // Check redirect url domain is in the snap allowed origins + if (url) { + const { origin } = new URL(url); + const snap = this.#snapClient.getController().get(snapId); + const allowedOrigins = + snap?.manifest?.initialPermissions['endowment:keyring'] + ?.allowedOrigins ?? []; + if (!allowedOrigins.includes(origin)) { + throw new Error( + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + ); + } + } + await this.#callbacks.redirectUser(snapId, url, message); } From 43d7d617b3583a3578843d626d3efb21fcfdb441 Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Thu, 29 Feb 2024 12:16:21 -0500 Subject: [PATCH 2/6] Feat: created 'getSnapAllowedOrigins' private utility function and updated optional chaining logic --- src/SnapKeyring.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts index ff0cd39c..011619b5 100644 --- a/src/SnapKeyring.ts +++ b/src/SnapKeyring.ts @@ -24,6 +24,7 @@ import { } from '@metamask/keyring-api'; import type { SnapController } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; +import type { Snap } from '@metamask/snaps-utils'; import type { Json } from '@metamask/utils'; import { bigIntToHex } from '@metamask/utils'; import { EventEmitter } from 'events'; @@ -441,9 +442,7 @@ export class SnapKeyring extends EventEmitter { if (url) { const { origin } = new URL(url); const snap = this.#snapClient.getController().get(snapId); - const allowedOrigins = - snap?.manifest?.initialPermissions['endowment:keyring'] - ?.allowedOrigins ?? []; + const allowedOrigins = this.#getSnapAllowedOrigins(snap); if (!allowedOrigins.includes(origin)) { throw new Error( `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, @@ -710,6 +709,20 @@ export class SnapKeyring extends EventEmitter { : undefined; } + /** + * Get the allowed origins of a snap. + * + * @param snap - Snap. + * @returns The allowed origins of the snap. + */ + #getSnapAllowedOrigins(snap: Snap): string[] { + if (snap.manifest.initialPermissions['endowment:keyring']?.allowedOrigins) { + return snap.manifest.initialPermissions['endowment:keyring'] + .allowedOrigins; + } + throw new Error(`Snap '${snap.id}' does not have allowed origins`); + } + /** * Return an internal account object for a given address. * From 61302ba462facb9c4ccdf268e4579549579bc6e3 Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Thu, 29 Feb 2024 12:30:36 -0500 Subject: [PATCH 3/6] Feat: updated tests for validating async redirect urls and added checks for undefined --- src/SnapKeyring.test.ts | 136 +++++++++++++++++++++++----------------- src/SnapKeyring.ts | 3 + 2 files changed, 83 insertions(+), 56 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index 43c5c5c4..bb9ebd00 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -366,68 +366,92 @@ describe('SnapKeyring', () => { spy.mockRestore(); }); - it('throws an error if async request redirect url is not an allowed origin', async () => { - const redirect = { - message: 'Go to dapp to continue.', - url: 'https://notallowed.com/sign?tx=1234', - }; - const { origin } = new URL(redirect.url); - - const snapObject = { - id: snapId, - manifest: { - initialPermissions: { - 'endowment:keyring': { - allowedOrigins: ['https://allowed.com'], + describe('async request redirect url', () => { + it('throws an error if async request redirect url is not an allowed origin', async () => { + const redirect = { + message: 'Go to dapp to continue.', + url: 'https://notallowed.com/sign?tx=1234', + }; + const { origin } = new URL(redirect.url); + + const snapObject = { + id: snapId, + manifest: { + initialPermissions: { + 'endowment:keyring': { + allowedOrigins: ['https://allowed.com'], + }, }, }, - }, - enabled: true, - }; - mockSnapController.get.mockReturnValue(snapObject); - - mockSnapController.handleRequest.mockResolvedValue({ - pending: true, - redirect, + enabled: true, + }; + mockSnapController.get.mockReturnValue(snapObject); + + mockSnapController.handleRequest.mockResolvedValue({ + pending: true, + redirect, + }); + const requestPromise = keyring.signPersonalMessage( + accounts[0].address, + 'hello', + ); + + await expect(requestPromise).rejects.toThrow( + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + ); }); - const requestPromise = keyring.signPersonalMessage( - accounts[0].address, - 'hello', - ); - - await expect(requestPromise).rejects.toThrow( - `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, - ); - }); - - it('throws an error if no allowed origins and async request redirect url', async () => { - const redirect = { - message: 'Go to dapp to continue.', - url: 'https://notallowed.com/sign?tx=1234', - }; - const { origin } = new URL(redirect.url); - - const snapObject = { - id: snapId, - manifest: { - initialPermissions: {}, - }, - enabled: true, - }; - mockSnapController.get.mockReturnValue(snapObject); - mockSnapController.handleRequest.mockResolvedValue({ - pending: true, - redirect, + it('throws an error if no allowed origins', async () => { + const redirect = { + message: 'Go to dapp to continue.', + url: 'https://notallowed.com/sign?tx=1234', + }; + const { origin } = new URL(redirect.url); + + const snapObject = { + id: snapId, + manifest: { + initialPermissions: {}, + }, + enabled: true, + }; + mockSnapController.get.mockReturnValue(snapObject); + + mockSnapController.handleRequest.mockResolvedValue({ + pending: true, + redirect, + }); + const requestPromise = keyring.signPersonalMessage( + accounts[0].address, + 'hello', + ); + + await expect(requestPromise).rejects.toThrow( + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + ); }); - const requestPromise = keyring.signPersonalMessage( - accounts[0].address, - 'hello', - ); - await expect(requestPromise).rejects.toThrow( - `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, - ); + it('throws an error if the snap is undefined', async () => { + const redirect = { + message: 'Go to dapp to continue.', + url: 'https://example.com/sign?tx=1234', + }; + + mockSnapController.get.mockReturnValue(undefined); + + mockSnapController.handleRequest.mockResolvedValue({ + pending: true, + redirect, + }); + const requestPromise = keyring.signPersonalMessage( + accounts[0].address, + 'hello', + ); + + await expect(requestPromise).rejects.toThrow( + `Snap '${snapId}' not found.`, + ); + }); }); it('rejects an async request', async () => { diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts index 011619b5..9bbd0bfb 100644 --- a/src/SnapKeyring.ts +++ b/src/SnapKeyring.ts @@ -442,6 +442,9 @@ export class SnapKeyring extends EventEmitter { if (url) { const { origin } = new URL(url); const snap = this.#snapClient.getController().get(snapId); + if (!snap) { + throw new Error(`Snap '${snapId}' not found.`); + } const allowedOrigins = this.#getSnapAllowedOrigins(snap); if (!allowedOrigins.includes(origin)) { throw new Error( From 0eab89f28c9abfd7d31495123bf3a8d7ddae6eb6 Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Thu, 29 Feb 2024 12:37:26 -0500 Subject: [PATCH 4/6] Fix: no allowed origin error message check in tests --- src/SnapKeyring.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index bb9ebd00..fe3a91e5 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -406,7 +406,6 @@ describe('SnapKeyring', () => { message: 'Go to dapp to continue.', url: 'https://notallowed.com/sign?tx=1234', }; - const { origin } = new URL(redirect.url); const snapObject = { id: snapId, @@ -427,7 +426,7 @@ describe('SnapKeyring', () => { ); await expect(requestPromise).rejects.toThrow( - `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, + `Snap '${snapId}' does not have allowed origins`, ); }); From 29a1904a3b9c918781f3c09513e44fe82dcbab38 Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Thu, 29 Feb 2024 13:11:11 -0500 Subject: [PATCH 5/6] Feat: updated allowed origins check to return empty [] instead of throw an error if undefined --- src/SnapKeyring.test.ts | 5 +++-- src/SnapKeyring.ts | 9 ++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index fe3a91e5..38cbdae1 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -404,8 +404,9 @@ describe('SnapKeyring', () => { it('throws an error if no allowed origins', async () => { const redirect = { message: 'Go to dapp to continue.', - url: 'https://notallowed.com/sign?tx=1234', + url: 'https://example.com/sign?tx=1234', }; + const { origin } = new URL(redirect.url); const snapObject = { id: snapId, @@ -426,7 +427,7 @@ describe('SnapKeyring', () => { ); await expect(requestPromise).rejects.toThrow( - `Snap '${snapId}' does not have allowed origins`, + `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, ); }); diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts index 9bbd0bfb..233d3143 100644 --- a/src/SnapKeyring.ts +++ b/src/SnapKeyring.ts @@ -719,11 +719,10 @@ export class SnapKeyring extends EventEmitter { * @returns The allowed origins of the snap. */ #getSnapAllowedOrigins(snap: Snap): string[] { - if (snap.manifest.initialPermissions['endowment:keyring']?.allowedOrigins) { - return snap.manifest.initialPermissions['endowment:keyring'] - .allowedOrigins; - } - throw new Error(`Snap '${snap.id}' does not have allowed origins`); + return ( + snap.manifest.initialPermissions['endowment:keyring']?.allowedOrigins ?? + [] + ); } /** From f1bc7ff5e2a4b2487e5b5d9c6ad1c0c2ef09d40c Mon Sep 17 00:00:00 2001 From: Kate Johnson Date: Mon, 4 Mar 2024 16:13:11 -0500 Subject: [PATCH 6/6] Feat: updated not allowed origins redirect test to use helper function --- src/SnapKeyring.test.ts | 64 +++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 41 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index 38cbdae1..a2c1a5f0 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -367,29 +367,28 @@ describe('SnapKeyring', () => { }); describe('async request redirect url', () => { - it('throws an error if async request redirect url is not an allowed origin', async () => { - const redirect = { - message: 'Go to dapp to continue.', - url: 'https://notallowed.com/sign?tx=1234', - }; - const { origin } = new URL(redirect.url); - + const isNotAllowedOrigin = async ( + allowedOrigins: string[], + redirectUrl: string, + ) => { + const { origin } = new URL(redirectUrl); const snapObject = { id: snapId, manifest: { - initialPermissions: { - 'endowment:keyring': { - allowedOrigins: ['https://allowed.com'], - }, - }, + initialPermissions: + allowedOrigins.length > 0 + ? { 'endowment:keyring': { allowedOrigins } } + : {}, }, enabled: true, }; mockSnapController.get.mockReturnValue(snapObject); - mockSnapController.handleRequest.mockResolvedValue({ pending: true, - redirect, + redirect: { + message: 'Go to dapp to continue.', + url: redirectUrl, + }, }); const requestPromise = keyring.signPersonalMessage( accounts[0].address, @@ -399,36 +398,19 @@ describe('SnapKeyring', () => { await expect(requestPromise).rejects.toThrow( `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, ); - }); - - it('throws an error if no allowed origins', async () => { - const redirect = { - message: 'Go to dapp to continue.', - url: 'https://example.com/sign?tx=1234', - }; - const { origin } = new URL(redirect.url); - - const snapObject = { - id: snapId, - manifest: { - initialPermissions: {}, - }, - enabled: true, - }; - mockSnapController.get.mockReturnValue(snapObject); + }; - mockSnapController.handleRequest.mockResolvedValue({ - pending: true, - redirect, - }); - const requestPromise = keyring.signPersonalMessage( - accounts[0].address, - 'hello', + it('throws an error if async request redirect url is not an allowed origin', async () => { + expect.hasAssertions(); + await isNotAllowedOrigin( + ['https://allowed.com'], + 'https://notallowed.com/sign?tx=1234', ); + }); - await expect(requestPromise).rejects.toThrow( - `Redirect URL domain '${origin}' is not an allowed origin by snap '${snapId}'`, - ); + it('throws an error if no allowed origins', async () => { + expect.hasAssertions(); + await isNotAllowedOrigin([], 'https://example.com/sign?tx=1234'); }); it('throws an error if the snap is undefined', async () => {