feat: enforce async request redirect URL is in the snaps 'allowedOrig…

…ins' (#228)

* Feat: added check for async requests to enforce redirect url is in the snap's allowed origins

* Feat: created 'getSnapAllowedOrigins' private utility function and updated optional chaining logic

* Feat: updated tests for validating async redirect urls and added checks for undefined

* Fix: no allowed origin error message check in tests

* Feat: updated allowed origins check to return empty [] instead of throw an error if undefined

* Feat: updated not allowed origins redirect test to use helper function

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,76 @@ describe('SnapKeyring', () => {
       spy.mockRestore();
     });
 
+    describe('async request redirect url', () => {
+      const isNotAllowedOrigin = async (
+        allowedOrigins: string[],
+        redirectUrl: string,
+      ) => {
+        const { origin } = new URL(redirectUrl);
+        const snapObject = {
+          id: snapId,
+          manifest: {
+            initialPermissions:
+              allowedOrigins.length > 0
+                ? { 'endowment:keyring': { allowedOrigins } }
+                : {},
+          },
+          enabled: true,
+        };
+        mockSnapController.get.mockReturnValue(snapObject);
+        mockSnapController.handleRequest.mockResolvedValue({
+          pending: true,
+          redirect: {
+            message: 'Go to dapp to continue.',
+            url: redirectUrl,
+          },
+        });
+        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 async request redirect url is not an allowed origin', async () => {
+        expect.hasAssertions();
+        await isNotAllowedOrigin(
+          ['https://allowed.com'],
+          'https://notallowed.com/sign?tx=1234',
+        );
+      });
+
+      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 () => {
+        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 () => {
       mockSnapController.handleRequest.mockResolvedValue({
         pending: true,

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';
@@ -436,6 +437,22 @@ 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);
+        if (!snap) {
+          throw new Error(`Snap '${snapId}' not found.`);
+        }
+        const allowedOrigins = this.#getSnapAllowedOrigins(snap);
+        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);
     }
 
@@ -695,6 +712,19 @@ 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[] {
+    return (
+      snap.manifest.initialPermissions['endowment:keyring']?.allowedOrigins ??
+      []
+    );
+  }
+
   /**
    * Return an internal account object for a given address.
    *