Merge pull request #559 from eh2077/21266-try-catch-regex-execution-e…

…rror Fix: error from executing url regex
Expensify · Jul 27, 2023 · 9940dd1 · 9940dd1
2 parents 98d8fea + 6efa019
commit 9940dd1
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 24 deletions.
diff --git a/__tests__/ExpensiMark-test.js b/__tests__/ExpensiMark-test.js
@@ -1,5 +1,6 @@
 /* eslint-disable max-len */
 import ExpensiMark from '../lib/ExpensiMark';
+import _ from 'underscore';
 
 const parser = new ExpensiMark();
 
@@ -14,3 +15,20 @@ test('Test text is unescaped', () => {
     const resultString = '& &amp; &lt; <';
     expect(parser.htmlToText(htmlString)).toBe(resultString);
 });
+
+test('Test with regex Maximum regex stack depth reached error', () => {
+    const testString = '<h1>heading</h1> asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfisjksksjsjssskssjskskssksksksksskdkddkddkdksskskdkdkdksskskskdkdkdkdkekeekdkddenejeodxkdndekkdjddkeemdjxkdenendkdjddekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdi.cdjd';
+    const parser = new ExpensiMark();
+    // Mock method modifyTextForUrlLinks to let it throw an error to test try/catch of method ExpensiMark.replace
+    const modifyTextForUrlLinksMock = jest.fn((a, b, c) => {throw new Error('Maximum regex stack depth reached')});
+    parser.modifyTextForUrlLinks = modifyTextForUrlLinksMock;
+    expect(parser.replace(testString)).toBe(_.escape(testString));
+    expect(modifyTextForUrlLinksMock).toHaveBeenCalledTimes(1);
+
+    // Mock method extractLinksInMarkdownComment to let it return undefined to test try/catch of method ExpensiMark.extractLinksInMarkdownComment
+    const extractLinksInMarkdownCommentMock = jest.fn((a) => undefined);
+    parser.extractLinksInMarkdownComment = extractLinksInMarkdownCommentMock;
+    expect(parser.extractLinksInMarkdownComment(testString)).toBe(undefined);
+    expect(parser.getRemovedMarkdownLinks(testString, 'google.com')).toStrictEqual([]);
+    expect(extractLinksInMarkdownCommentMock).toHaveBeenCalledTimes(3);
+});
diff --git a/lib/ExpensiMark.d.ts b/lib/ExpensiMark.d.ts
@@ -86,7 +86,7 @@ export default class ExpensiMark {
      * @param textToCheck - Text to check
      */
     containsNonPairTag(textToCheck: string): boolean;
-    extractLinksInMarkdownComment(comment: string): string[];
+    extractLinksInMarkdownComment(comment: string): string[] | undefined;
     /**
      * Compares two markdown comments and returns a list of the links removed in a new comment.
      *

diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js
@@ -391,23 +391,32 @@ export default class ExpensiMark {
         let replacedText = shouldEscapeText ? _.escape(text) : text;
 
         const rules = _.isEmpty(filterRules) ? this.rules : _.filter(this.rules, rule => _.contains(filterRules, rule.name));
-        rules.forEach((rule) => {
-            // Pre-process text before applying regex
-            if (rule.pre) {
-                replacedText = rule.pre(replacedText);
-            }
 
-            if (rule.process) {
-                replacedText = rule.process(replacedText, rule.replacement);
-            } else {
-                replacedText = replacedText.replace(rule.regex, rule.replacement);
-            }
+        try {
+            rules.forEach((rule) => {
+                // Pre-process text before applying regex
+                if (rule.pre) {
+                    replacedText = rule.pre(replacedText);
+                }
 
-            // Post-process text after applying regex
-            if (rule.post) {
-                replacedText = rule.post(replacedText);
-            }
-        });
+                if (rule.process) {
+                    replacedText = rule.process(replacedText, rule.replacement);
+                } else {
+                    replacedText = replacedText.replace(rule.regex, rule.replacement);
+                }
+
+                // Post-process text after applying regex
+                if (rule.post) {
+                    replacedText = rule.post(replacedText);
+                }
+            });
+        } catch (e) {
+            // eslint-disable-next-line no-console
+            console.warn('Error replacing text with html in ExpensiMark.replace', {error: e});
+
+            // We want to return text without applying rules if exception occurs during replacing
+            return shouldEscapeText ? _.escape(text) : text;
+        }
 
         return replacedText;
     }
@@ -731,15 +740,21 @@ export default class ExpensiMark {
 
     /**
      * @param {String} comment
-     * @returns {Array}
+     * @returns {Array} or undefined if exception occurs when executing regex matching
      */
     extractLinksInMarkdownComment(comment) {
-        const escapedComment = _.escape(comment);
-        const matches = [...escapedComment.matchAll(MARKDOWN_LINK_REGEX)];
-
-        // Element 1 from match is the regex group if it exists which contains the link URLs
-        const links = _.map(matches, match => Str.sanitizeURL(match[2]));
-        return links;
+        try {
+            const escapedComment = _.escape(comment);
+            const matches = [...escapedComment.matchAll(MARKDOWN_LINK_REGEX)];
+
+            // Element 1 from match is the regex group if it exists which contains the link URLs
+            const links = _.map(matches, match => Str.sanitizeURL(match[2]));
+            return links;
+        } catch (e) {
+            // eslint-disable-next-line no-console
+            console.warn('Error parsing url in ExpensiMark.extractLinksInMarkdownComment', {error: e});
+            return undefined;
+        }
     }
 
     /**
@@ -752,6 +767,6 @@ export default class ExpensiMark {
     getRemovedMarkdownLinks(oldComment, newComment) {
         const linksInOld = this.extractLinksInMarkdownComment(oldComment);
         const linksInNew = this.extractLinksInMarkdownComment(newComment);
-        return _.difference(linksInOld, linksInNew);
+        return linksInOld === undefined || linksInNew === undefined ? [] : _.difference(linksInOld, linksInNew);
     }
 }