From 62d336dc7435669253c7ccdbbd643a73901b4845 Mon Sep 17 00:00:00 2001 From: Eric Han Date: Fri, 21 Jul 2023 14:33:12 +0800 Subject: [PATCH 1/6] try/catch error from executing url regex --- lib/ExpensiMark.d.ts | 2 +- lib/ExpensiMark.js | 56 ++++++++++++++++++++++++++------------------ 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/lib/ExpensiMark.d.ts b/lib/ExpensiMark.d.ts index 75de4525..529e7b52 100644 --- 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 index 33149c00..590d54a1 100644 --- a/lib/ExpensiMark.js +++ b/lib/ExpensiMark.js @@ -391,23 +391,29 @@ 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) { + // We want to return text without applying rules if exception occurs during replacing + return shouldEscapeText ? _.escape(text) : text; + } return replacedText; } @@ -731,15 +737,19 @@ 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) { + return undefined; + } } /** @@ -752,6 +762,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); } } From 6c4a3d15a93da160d3d94f98fca8d3dec4314931 Mon Sep 17 00:00:00 2001 From: Eric Han Date: Sun, 23 Jul 2023 23:38:43 +0800 Subject: [PATCH 2/6] add test for try/catch change for methods --- __tests__/ExpensiMark-test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/__tests__/ExpensiMark-test.js b/__tests__/ExpensiMark-test.js index 436f4b1d..80ec6a11 100644 --- a/__tests__/ExpensiMark-test.js +++ b/__tests__/ExpensiMark-test.js @@ -1,5 +1,7 @@ /* eslint-disable max-len */ import ExpensiMark from '../lib/ExpensiMark'; +import Str from '../lib/str'; +import _ from 'underscore'; const parser = new ExpensiMark(); @@ -14,3 +16,20 @@ test('Test text is unescaped', () => { const resultString = '& & < <'; expect(parser.htmlToText(htmlString)).toBe(resultString); }); + +test('Test with regex Maximum regex stack depth reached error', () => { + const testString = '

heading

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); +}); From 9ac15b69c0d516d8f1dc6be89a8468dd1c03d879 Mon Sep 17 00:00:00 2001 From: Eric Han Date: Tue, 25 Jul 2023 23:19:57 +0800 Subject: [PATCH 3/6] Log warning message for regex execution errors --- lib/ExpensiMark.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js index 590d54a1..0ad02104 100644 --- a/lib/ExpensiMark.js +++ b/lib/ExpensiMark.js @@ -1,4 +1,5 @@ import _ from 'underscore'; +import Log from './Log'; import Str from './str'; import {MARKDOWN_URL_REGEX, LOOSE_URL_REGEX, URL_REGEX} from './Url'; import {CONST} from './CONST'; @@ -411,6 +412,7 @@ export default class ExpensiMark { } }); } catch (e) { + Log.warn('Exception occurs in method ExpensiMark.replace', {error: e}); // We want to return text without applying rules if exception occurs during replacing return shouldEscapeText ? _.escape(text) : text; } @@ -748,6 +750,7 @@ export default class ExpensiMark { const links = _.map(matches, match => Str.sanitizeURL(match[2])); return links; } catch (e) { + Log.warn('Exception occurs in method ExpensiMark.extractLinksInMarkdownComment', {error: e}); return undefined; } } From 42c18424fc76d612ae5cd19ff2efd9c921ee9a15 Mon Sep 17 00:00:00 2001 From: Eric Han Date: Tue, 25 Jul 2023 23:21:17 +0800 Subject: [PATCH 4/6] fix lint --- lib/ExpensiMark.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js index 0ad02104..62a12b29 100644 --- a/lib/ExpensiMark.js +++ b/lib/ExpensiMark.js @@ -413,6 +413,7 @@ export default class ExpensiMark { }); } catch (e) { Log.warn('Exception occurs in method ExpensiMark.replace', {error: e}); + // We want to return text without applying rules if exception occurs during replacing return shouldEscapeText ? _.escape(text) : text; } From fbe2ee8cee433f79a8dd9ad63579460aea1b84de Mon Sep 17 00:00:00 2001 From: Eric Han Date: Wed, 26 Jul 2023 10:08:50 +0800 Subject: [PATCH 5/6] improve error log message --- lib/ExpensiMark.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js index 62a12b29..36890a4c 100644 --- a/lib/ExpensiMark.js +++ b/lib/ExpensiMark.js @@ -412,7 +412,7 @@ export default class ExpensiMark { } }); } catch (e) { - Log.warn('Exception occurs in method ExpensiMark.replace', {error: e}); + Log.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; @@ -751,7 +751,7 @@ export default class ExpensiMark { const links = _.map(matches, match => Str.sanitizeURL(match[2])); return links; } catch (e) { - Log.warn('Exception occurs in method ExpensiMark.extractLinksInMarkdownComment', {error: e}); + Log.warn('Error parsing url in ExpensiMark.extractLinksInMarkdownComment', {error: e}); return undefined; } } From 6efa0193dcbcf91a7f889e5d12bfe2147c39532d Mon Sep 17 00:00:00 2001 From: Eric Han Date: Thu, 27 Jul 2023 14:51:14 +0800 Subject: [PATCH 6/6] use console.warn to log error message --- __tests__/ExpensiMark-test.js | 1 - lib/ExpensiMark.js | 7 ++++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/__tests__/ExpensiMark-test.js b/__tests__/ExpensiMark-test.js index 80ec6a11..c54f8bf4 100644 --- a/__tests__/ExpensiMark-test.js +++ b/__tests__/ExpensiMark-test.js @@ -1,6 +1,5 @@ /* eslint-disable max-len */ import ExpensiMark from '../lib/ExpensiMark'; -import Str from '../lib/str'; import _ from 'underscore'; const parser = new ExpensiMark(); diff --git a/lib/ExpensiMark.js b/lib/ExpensiMark.js index 36890a4c..aad82fa1 100644 --- a/lib/ExpensiMark.js +++ b/lib/ExpensiMark.js @@ -1,5 +1,4 @@ import _ from 'underscore'; -import Log from './Log'; import Str from './str'; import {MARKDOWN_URL_REGEX, LOOSE_URL_REGEX, URL_REGEX} from './Url'; import {CONST} from './CONST'; @@ -412,7 +411,8 @@ export default class ExpensiMark { } }); } catch (e) { - Log.warn('Error replacing text with html in ExpensiMark.replace', {error: 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; @@ -751,7 +751,8 @@ export default class ExpensiMark { const links = _.map(matches, match => Str.sanitizeURL(match[2])); return links; } catch (e) { - Log.warn('Error parsing url in ExpensiMark.extractLinksInMarkdownComment', {error: e}); + // eslint-disable-next-line no-console + console.warn('Error parsing url in ExpensiMark.extractLinksInMarkdownComment', {error: e}); return undefined; } }