Change validateTextLinks to only get URL in markdown links (#1914)

Co-authored-by: Frederik Bolding <[email protected]>
MetaMask · Nov 15, 2023 · f90233e · f90233e
1 parent 8d3710b
commit f90233e
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 47 deletions.
diff --git a/packages/snaps-rpc-methods/src/restricted/notify.test.ts b/packages/snaps-rpc-methods/src/restricted/notify.test.ts
@@ -176,7 +176,7 @@ describe('snap_notify', () => {
           method: 'snap_notify',
           params: {
             type: 'native',
-            message: 'https://foo.bar',
+            message: '[test link](https://foo.bar)',
           },
         }),
       ).rejects.toThrow('Invalid URL: The specified URL is not allowed.');
@@ -203,7 +203,7 @@ describe('snap_notify', () => {
           method: 'snap_notify',
           params: {
             type: 'native',
-            message: 'http://foo.bar',
+            message: '[test](http://foo.bar)',
           },
         }),
       ).rejects.toThrow(

diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json
@@ -1,5 +1,5 @@
 {
-  "branches": 96.02,
+  "branches": 95.98,
   "functions": 99.01,
   "lines": 98.69,
   "statements": 95.61

diff --git a/packages/snaps-utils/src/ui.test.ts b/packages/snaps-utils/src/ui.test.ts
@@ -5,20 +5,24 @@ import { validateTextLinks, validateComponentLinks } from './ui';
 describe('validateTextLinks', () => {
   it('passes for valid links', () => {
     expect(() =>
-      validateTextLinks('https://foo.bar', () => false),
+      validateTextLinks('[test](https://foo.bar)', () => false),
     ).not.toThrow();
 
     expect(() =>
-      validateTextLinks('mailto:[email protected]', () => false),
+      validateTextLinks('[test](mailto:[email protected])', () => false),
+    ).not.toThrow();
+
+    expect(() =>
+      validateTextLinks('[](https://foo.bar)', () => false),
     ).not.toThrow();
   });
 
   it('throws an error if an invalid link is found in text', () => {
-    expect(() => validateTextLinks('http://foo.bar', () => false)).toThrow(
-      'Invalid URL: Protocol must be one of: https:, mailto:.',
-    );
+    expect(() =>
+      validateTextLinks('[test](http://foo.bar)', () => false),
+    ).toThrow('Invalid URL: Protocol must be one of: https:, mailto:.');
 
-    expect(() => validateTextLinks('https://foo^bar.bar', () => false)).toThrow(
+    expect(() => validateTextLinks('[test](foo.bar)', () => false)).toThrow(
       'Invalid URL: Unable to parse URL.',
     );
   });
@@ -46,13 +50,6 @@ describe('validateComponentLinks', () => {
   it('throws for an unsafe text component', async () => {
     const isOnPhishingList = () => true;
 
-    expect(() =>
-      validateComponentLinks(
-        text('This tests a link: https://foo.bar'),
-        isOnPhishingList,
-      ),
-    ).toThrow('Invalid URL: The specified URL is not allowed.');
-
     expect(() =>
       validateComponentLinks(
         text('This tests a [link](https://foo.bar)'),

diff --git a/packages/snaps-utils/src/ui.ts b/packages/snaps-utils/src/ui.ts
@@ -1,12 +1,13 @@
 import type { Component } from '@metamask/snaps-sdk';
 import { NodeType } from '@metamask/snaps-sdk';
-import { assert, AssertionError, hasProperty } from '@metamask/utils';
+import { assert, AssertionError } from '@metamask/utils';
+
+const MARKDOWN_LINK_REGEX = /\[(?<name>[^\]]*)\]\((?<url>[^)]+)\)/giu;
 
-const LINK_REGEX = /(?<protocol>[a-z]+:\/?\/?)(?<host>\S+?(?:\.[a-z]+)+)/giu;
 const ALLOWED_PROTOCOLS = ['https:', 'mailto:'];
 
 /**
- * Search for links in a sting and check them against the phishing list.
+ * Searches for markdown links in a string and checks them against the phishing list.
  *
  * @param text - The text to verify.
  * @param isOnPhishingList - The function that checks the link against the
@@ -17,35 +18,37 @@ export function validateTextLinks(
   text: string,
   isOnPhishingList: (url: string) => boolean,
 ) {
-  const links = text.match(LINK_REGEX);
-  if (links) {
-    links.forEach((link) => {
-      try {
-        const url = new URL(link);
-        assert(
-          ALLOWED_PROTOCOLS.includes(url.protocol),
-          `Protocol must be one of: ${ALLOWED_PROTOCOLS.join(', ')}.`,
-        );
+  const matches = String.prototype.matchAll.call(text, MARKDOWN_LINK_REGEX);
+
+  for (const { groups } of matches) {
+    const link = groups?.url;
+
+    /* This case should never happen with the regex but the TS type allows for undefined */
+    /* istanbul ignore next */
+    if (!link) {
+      continue;
+    }
+
+    try {
+      const url = new URL(link);
+      assert(
+        ALLOWED_PROTOCOLS.includes(url.protocol),
+        `Protocol must be one of: ${ALLOWED_PROTOCOLS.join(', ')}.`,
+      );
 
-        const hostname =
-          url.protocol === 'mailto:'
-            ? url.pathname.split('@')[1]
-            : url.hostname;
+      const hostname =
+        url.protocol === 'mailto:' ? url.pathname.split('@')[1] : url.hostname;
 
-        assert(
-          !isOnPhishingList(hostname),
-          'The specified URL is not allowed.',
-        );
-      } catch (error) {
-        throw new Error(
-          `Invalid URL: ${
-            error instanceof AssertionError
-              ? error.message
-              : 'Unable to parse URL.'
-          }`,
-        );
-      }
-    });
+      assert(!isOnPhishingList(hostname), 'The specified URL is not allowed.');
+    } catch (error) {
+      throw new Error(
+        `Invalid URL: ${
+          error instanceof AssertionError
+            ? error.message
+            : 'Unable to parse URL.'
+        }`,
+      );
+    }
   }
 }
 
@@ -69,7 +72,7 @@ export function validateComponentLinks(
     );
   }
 
-  if (hasProperty(component, 'value') && typeof component.value === 'string') {
+  if (type === NodeType.Text) {
     validateTextLinks(component.value, isOnPhishingList);
   }
 }