fix: encode link url unnecessarily (nhn#1641)

* fix: decode url when executing addImage, addLink command * chore: add test case(addImage, addLink) * chore: apply code review * refactor: assertToContainHTML function * chore: add test case(converting the html block which has "=" attr value)
ahamelers · Jul 6, 2021 · 3583e33 · 3583e33
1 parent 1473167
commit 3583e33
Show file tree

Hide file tree

Showing 9 changed files with 106 additions and 63 deletions.
diff --git a/src/__test__/unit/convertor.spec.ts b/src/__test__/unit/convertor.spec.ts
@@ -828,6 +828,13 @@ describe('Convertor', () => {
       assertConverting(markdown, markdown);
     });
 
+    it('should convert html block element which has "=" character as the attribute value', () => {
+      const markdown =
+        '<iframe width="420" height="315" src="//player.bilibili.com/player.html?aid=588782532&bvid=BV1hB4y1K7ro&cid=360826679&page=1"></iframe>';
+
+      assertConverting(markdown, markdown);
+    });
+
     it('should convert html inline node', () => {
       const markdown = '<big class="my-big">content</big>';
 

diff --git a/src/__test__/unit/markdown/mdCommand.spec.ts b/src/__test__/unit/markdown/mdCommand.spec.ts
@@ -292,6 +292,17 @@ describe('addImage command', () => {
 
     expect(getTextContent(mde)).toBe('![image](myurl%20%28%29%5B%5D%3C%3E)');
   });
+
+  it('should not decode url which is already encoded', () => {
+    cmd.exec('addImage', {
+      altText: 'image',
+      imageUrl: 'https://firebasestorage.googleapis.com/images%2Fimage.png?alt=media',
+    });
+
+    expect(getTextContent(mde)).toBe(
+      '![image](https://firebasestorage.googleapis.com/images%2Fimage.png?alt=media)'
+    );
+  });
 });
 
 describe('addLink command', () => {
@@ -318,6 +329,17 @@ describe('addLink command', () => {
 
     expect(getTextContent(mde)).toBe('[TOAST UI](myurl%20%28%29%5B%5D%3C%3E)');
   });
+
+  it('should not decode url which is already encoded', () => {
+    cmd.exec('addLink', {
+      linkText: 'TOAST UI',
+      linkUrl: 'https://firebasestorage.googleapis.com/links%2Fimage.png?alt=media',
+    });
+
+    expect(getTextContent(mde)).toBe(
+      '[TOAST UI](https://firebasestorage.googleapis.com/links%2Fimage.png?alt=media)'
+    );
+  });
 });
 
 describe('heading command', () => {

diff --git a/src/__test__/unit/wysiwyg/wwCommand.spec.ts b/src/__test__/unit/wysiwyg/wwCommand.spec.ts
@@ -484,14 +484,14 @@ describe('wysiwyg commands', () => {
       expect(wwe.getHTML()).toBe('<p><br></p>');
     });
 
-    it('should decode attribute and encode wrong markdown charactors', () => {
+    it('should not decode url which is already encoded', () => {
       cmd.exec('addImage', {
-        imageUrl: 'foo %D1%88%D0%B5%D0%BB%D0%BB%D1%8B ()[]<>',
-        altText: 'foo ()[]<>',
+        imageUrl: 'https://firebasestorage.googleapis.com/images%2Fimage.png?alt=media',
+        altText: 'foo',
       });
 
       expect(wwe.getHTML()).toBe(
-        '<p><img src="foo%20шеллы%20%28%29%5B%5D%3C%3E" alt="foo \\(\\)\\[\\]\\<\\>"><br></p>'
+        '<p><img src="https://firebasestorage.googleapis.com/images%2Fimage.png?alt=media" alt="foo"><br></p>'
       );
     });
   });
@@ -520,17 +520,6 @@ describe('wysiwyg commands', () => {
       expect(wwe.getHTML()).toBe('<p><br></p>');
     });
 
-    it('should decode attribute and encode wrong markdown charactors', () => {
-      cmd.exec('addLink', {
-        linkUrl: 'foo %D1%88%D0%B5%D0%BB%D0%BB%D1%8B ()[]<>',
-        linkText: 'foo ()[]<>',
-      });
-
-      expect(wwe.getHTML()).toBe(
-        '<p><a href="foo%20шеллы%20%28%29%5B%5D%3C%3E">foo ()[]&lt;&gt;</a></p>'
-      );
-    });
-
     it('should change link url in selection', () => {
       cmd.exec('addLink', {
         linkUrl: '#',
@@ -554,6 +543,17 @@ describe('wysiwyg commands', () => {
 
       expect(wwe.getHTML()).toBe(expected);
     });
+
+    it('should not decode url which is already encoded', () => {
+      cmd.exec('addLink', {
+        linkUrl: 'https://firebasestorage.googleapis.com/links%2Fimage.png?alt=media',
+        linkText: 'foo',
+      });
+
+      expect(wwe.getHTML()).toBe(
+        '<p><a href="https://firebasestorage.googleapis.com/links%2Fimage.png?alt=media">foo</a></p>'
+      );
+    });
   });
 
   describe(`addLink command with 'linkAttributes' option`, () => {

diff --git a/src/__test__/unit/wysiwyg/wwEditor.spec.ts b/src/__test__/unit/wysiwyg/wwEditor.spec.ts
@@ -14,6 +14,10 @@ jest.useFakeTimers();
 describe('WysiwygEditor', () => {
   let wwe: WysiwygEditor, em: EventEmitter, el: HTMLElement;
 
+  function assertToContainHTML(html: string) {
+    expect(wwe.view.dom).toContainHTML(html);
+  }
+
   function setContent(content: string) {
     const wrapper = document.createElement('div');
 
@@ -81,7 +85,7 @@ describe('WysiwygEditor', () => {
     it('setPlaceholder() attach placeholder element', () => {
       wwe.setPlaceholder('placeholder text');
 
-      expect(wwe.getHTML()).toBe(oneLineTrim`
+      assertToContainHTML(oneLineTrim`
         <p>
           <span class="placeholder ProseMirror-widget">placeholder text</span>
           <br>
@@ -128,7 +132,7 @@ describe('WysiwygEditor', () => {
       wwe.setSelection(3, 7);
       wwe.replaceSelection('new foo\nnew bar');
 
-      expect(wwe.getHTML()).toBe(oneLineTrim`
+      assertToContainHTML(oneLineTrim`
         <p>fonew foo</p>
         <p>new barar</p>
       `);
@@ -172,21 +176,21 @@ describe('WysiwygEditor', () => {
       '<iframe width="420" height="315" src="https://www.youtube.com/embed/XyenY12fzAk"></iframe>'
     );
 
-    expect(wwe.getHTML()).toBe(
+    assertToContainHTML(
       '<iframe width="420" height="315" src="https://www.youtube.com/embed/XyenY12fzAk" class="html-block ProseMirror-selectednode" draggable="true"></iframe>'
     );
   });
 
   it('should display html inline element properly', () => {
     setContent('<big class="my-inline">text</big>');
 
-    expect(wwe.getHTML()).toBe('<p><big class="my-inline">text</big></p>');
+    assertToContainHTML('<p><big class="my-inline">text</big></p>');
   });
 
   it('should sanitize html element', () => {
     setContent('<iframe width="420" height="315" src="javascript: alert(1);"></iframe>');
 
-    expect(wwe.getHTML()).toBe(
+    assertToContainHTML(
       '<iframe width="420" height="315" class="html-block ProseMirror-selectednode" draggable="true"></iframe>'
     );
   });

diff --git a/src/markdown/marks/link.ts b/src/markdown/marks/link.ts
@@ -2,7 +2,7 @@ import { DOMOutputSpecArray, Mark as ProsemirrorMark } from 'prosemirror-model';
 import { EditorCommand } from '@t/spec';
 import { clsWithMdPrefix } from '@/utils/dom';
 import Mark from '@/spec/mark';
-import { decodeURIGraceful, encodeMarkdownText } from '@/utils/encoder';
+import { encodeMarkdownText, escapeMarkdownText } from '@/utils/encoder';
 import { createTextNode } from '@/helper/manipulation';
 import { resolveSelectionPos } from '../helper/pos';
 
@@ -56,8 +56,8 @@ export class Link extends Mark {
         syntax = '!';
       }
 
-      text = encodeMarkdownText(text, false);
-      url = encodeMarkdownText(decodeURIGraceful(url), true);
+      text = escapeMarkdownText(text);
+      url = encodeMarkdownText(url);
       syntax += `[${text}](${url})`;
 
       dispatch!(tr.replaceWith(from, to, createTextNode(schema, syntax)));

diff --git a/src/utils/encoder.ts b/src/utils/encoder.ts
@@ -1,35 +1,45 @@
-const reURL = /^(https?:\/\/)?([\da-z.-]+)\.([a-z.]{2,6})(\/([^\s]*))?$/g;
-const encodingRegExps = [/\(/g, /\)/g, /\[/g, /\]/g, /</g, />/g];
-const encodedList = ['%28', '%29', '%5B', '%5D', '%3C', '%3E'];
-const escapedList = ['\\(', '\\)', '\\[', '\\]', '\\<', '\\>'];
+const encoderList = [
+  {
+    regExp: /\(/g,
+    encoded: '%28',
+    escaped: '\\(',
+  },
+  {
+    regExp: /\)/g,
+    encoded: '%29',
+    escaped: '\\)',
+  },
+  {
+    regExp: /\[/g,
+    encoded: '%5B',
+    escaped: '\\[',
+  },
+  {
+    regExp: /\]/g,
+    encoded: '%5D',
+    escaped: '\\]',
+  },
+  {
+    regExp: /</g,
+    encoded: '%3C',
+    escaped: '\\<',
+  },
+  {
+    regExp: />/g,
+    encoded: '%3E',
+    escaped: '\\>',
+  },
+  {
+    regExp: / /g,
+    encoded: '%20',
+    escaped: ' ',
+  },
+];
 
-export function decodeURIGraceful(uri: string) {
-  const uriList = uri.split(' ');
-
-  return uriList
-    .reduce<string[]>((decodedURIList, targetUri) => {
-      let decodedURI = '';
-
-      try {
-        decodedURI = decodeURIComponent(targetUri);
-      } catch (e) {
-        decodedURI = targetUri;
-      }
-
-      return decodedURIList.concat(decodedURI);
-    }, [])
-    .join('%20');
-}
-
-export function encodeMarkdownText(text: string, encode: boolean) {
-  const expectedValues = encode ? encodedList : escapedList;
-
-  return encodingRegExps.reduce(
-    (result, regExp, index) => result.replace(regExp, expectedValues[index]),
-    text
-  );
+export function escapeMarkdownText(text: string) {
+  return encoderList.reduce((result, { regExp, escaped }) => result.replace(regExp, escaped), text);
 }
 
-export function decodeURL(text: string) {
-  return text.replace(reURL, (matched) => encodeMarkdownText(decodeURIGraceful(matched), true));
+export function encodeMarkdownText(text: string) {
+  return encoderList.reduce((result, { regExp, encoded }) => result.replace(regExp, encoded), text);
 }
diff --git a/src/wysiwyg/marks/link.ts b/src/wysiwyg/marks/link.ts
@@ -2,7 +2,7 @@ import { Mark as ProsemirrorMark, DOMOutputSpecArray } from 'prosemirror-model';
 import { toggleMark } from 'prosemirror-commands';
 
 import Mark from '@/spec/mark';
-import { decodeURIGraceful, encodeMarkdownText } from '@/utils/encoder';
+import { encodeMarkdownText, escapeMarkdownText } from '@/utils/encoder';
 import { sanitizeXSSAttributeValue } from '@/sanitizer/htmlSanitizer';
 import { createTextNode } from '@/helper/manipulation';
 import { getCustomAttrs, getDefaultCustomAttrs } from '@/wysiwyg/helper/node';
@@ -65,8 +65,8 @@ export class Link extends Mark {
 
       if (from && to && linkUrl) {
         const attrs = {
-          linkUrl: encodeMarkdownText(decodeURIGraceful(linkUrl), true),
-          linkText: encodeMarkdownText(linkText, false),
+          linkUrl: encodeMarkdownText(linkUrl),
+          linkText: escapeMarkdownText(linkText),
         };
         const mark = schema.mark('link', attrs);
 

diff --git a/src/wysiwyg/nodes/html.ts b/src/wysiwyg/nodes/html.ts
@@ -24,10 +24,10 @@ export function getHTMLAttrsByHTMLString(html: string) {
 
   return attrs
     ? attrs.reduce<Record<string, string | null>>((acc, attr) => {
-        const [name, ...value] = attr.trim().split('=');
+        const [name, ...values] = attr.trim().split('=');
 
-        if (value.length > 0) {
-          acc[name] = value.join('=').replace(/'|"/g, '').trim();
+        if (values.length) {
+          acc[name] = values.join('=').replace(/'|"/g, '').trim();
         }
 
         return acc;

diff --git a/src/wysiwyg/nodes/image.ts b/src/wysiwyg/nodes/image.ts
@@ -1,7 +1,7 @@
 import { Node as ProsemirrorNode, DOMOutputSpecArray } from 'prosemirror-model';
 
 import NodeSchema from '@/spec/node';
-import { decodeURIGraceful, encodeMarkdownText } from '@/utils/encoder';
+import { encodeMarkdownText } from '@/utils/encoder';
 import { sanitizeXSSAttributeValue } from '@/sanitizer/htmlSanitizer';
 
 import { EditorCommand } from '@t/spec';
@@ -60,8 +60,8 @@ export class Image extends NodeSchema {
       }
 
       const node = schema.nodes.image.createAndFill({
-        imageUrl: encodeMarkdownText(decodeURIGraceful(imageUrl), true),
-        ...(altText && { altText: encodeMarkdownText(altText, false) }),
+        imageUrl: encodeMarkdownText(imageUrl),
+        ...(altText && { altText }),
       });
 
       dispatch!(tr.replaceSelectionWith(node!).scrollIntoView());