TINY-7998: Ensure urls are cleaned when updating

modules/tinymce/CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+- Added a new `URI.isDomSafe(uri)` API to be able to check if a URI is considered safe to be inserted into the DOM #TINY-7998
+
 ### Improved
 - The `element` argument of the `editor.selection.scrollIntoView()` API is now optional, and if it is not provided the current selection will be scrolled into view #TINY-7291
 
@@ -27,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed notifications rendering in the wrong place initially and when the page was scrolled #TINY-7894
 - Fixed an exception getting thrown when the number of `col` elements didn't match the number of columns in a table #TINY-7041 #TINY-8011
 - As of Mozilla Firefox 91, toggling fullscreen mode with `toolbar_sticky` enabled would cause the toolbar to disappear #TINY-7873
+- Fixed urls not cleaned correctly in some cases in the `link` and `image` plugin #TINY-7998
 - Rounding errors were causing the line height dropdowns and the `LineHeight` query command to be inaccurate on Safari #TINY-7895
 - Fixed the `image` and `media` toolbar buttons showing the incorrect active state in some cases #TINY-3463
 - Inserting content into a `contenteditable="true"` element that was contained within a `contenteditable="false"` element would move the selection to an incorrect location #TINY-7842

modules/tinymce/src/core/main/ts/api/dom/DOMUtils.ts

@@ -67,8 +67,8 @@ interface AttrHooks {
 const setupAttrHooks = (styles: Styles, settings: Partial<DOMUtilsSettings>, getContext): AttrHooks => {
   const keepValues: boolean = settings.keep_values;
   const keepUrlHook = {
-    set: ($elm, value: string, name: string) => {
-      if (settings.url_converter) {
+    set: ($elm, value: string | null, name: string) => {
+      if (settings.url_converter && value !== null) {
         value = settings.url_converter.call(settings.url_converter_scope || getContext(), value, name, $elm[0]);
       }
 
@@ -82,7 +82,7 @@ const setupAttrHooks = (styles: Styles, settings: Partial<DOMUtilsSettings>, get
 
   const attrHooks: AttrHooks = {
     style: {
-      set: ($elm, value: string | {}) => {
+      set: ($elm, value: string | {} | null) => {
         if (value !== null && typeof value === 'object') {
           $elm.css(value);
           return;

modules/tinymce/src/core/main/ts/api/html/SaxParser.ts

@@ -5,10 +5,11 @@
  * For commercial licenses see https://www.tiny.cloud/
  */
 
-import { Arr, Fun, Obj, Strings, Type } from '@ephox/katamari';
+import { Fun, Obj, Strings, Type } from '@ephox/katamari';
 
 import { Base64Extract, extractBase64DataUris, restoreDataUris } from '../../html/Base64Uris';
 import Tools from '../util/Tools';
+import URI from '../util/URI';
 import Entities from './Entities';
 import Schema from './Schema';
 
@@ -99,31 +100,13 @@ const enum MatchType {
   Attribute = 9
 }
 
-const safeSvgDataUrlElements = [ 'img', 'video' ];
-
 // A list of form control or other elements whereby a name/id would override a form or document property
 // https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements#value
 // https://portswigger.net/research/dom-clobbering-strikes-back
 const filteredClobberElements = Tools.makeMap('button,fieldset,form,iframe,img,image,input,object,output,select,textarea');
 
 const isValidPrefixAttrName = (name: string): boolean => name.indexOf('data-') === 0 || name.indexOf('aria-') === 0;
 
-const blockSvgDataUris = (allowSvgDataUrls: boolean | undefined, tagName: string) => {
-  // Only allow SVGs by default on images/videos since the browser won't execute scripts on those elements
-  const allowed = Type.isNullable(allowSvgDataUrls) ? Arr.contains(safeSvgDataUrlElements, tagName) : allowSvgDataUrls;
-  return !allowed;
-};
-
-const isInvalidUri = (settings: SaxParserSettings, uri: string, tagName: string) => {
-  if (settings.allow_html_data_urls) {
-    return false;
-  } else if (/^data:image\//i.test(uri)) {
-    return blockSvgDataUris(settings.allow_svg_data_urls, tagName) && /^data:image\/svg\+xml/i.test(uri);
-  } else {
-    return /^data:/i.test(uri);
-  }
-};
-
 /**
  * Returns the index of the matching end tag for a specific start tag. This can
  * be used to skip all children of a parent element from being processed.
@@ -254,7 +237,6 @@ const SaxParser = (settings?: SaxParserSettings, schema = Schema()): SaxParser =
     let anyAttributesRequired, attrValue, idCount = 0;
     const decode = Entities.decode;
     const filteredUrlAttrs = Tools.makeMap('src,href,data,background,action,formaction,poster,xlink:href');
-    const scriptUriRegExp = /((java|vb)script|mhtml):/i;
     const parsingMode = format === 'html' ? ParsingMode.Html : ParsingMode.Xml;
 
     const processEndTag = (name: { name: string; valid: boolean }) => {
@@ -320,19 +302,16 @@ const SaxParser = (settings?: SaxParserSettings, schema = Schema()): SaxParser =
     };
 
     const parseAttribute = (tagName: string, name: string, value?: string, val2?: string, val3?: string) => {
-      let attrRule, i;
-      const trimRegExp = /[\s\u0000-\u001F]+/g;
-
       name = name.toLowerCase();
       value = processAttr(name in fillAttrsMap ? name : decode(value || val2 || val3 || '')); // Handle boolean attribute than value attribute
 
       // Validate name and value pass through all data- attributes
       if (validate && !isInternalElement && isValidPrefixAttrName(name) === false) {
-        attrRule = validAttributesMap[name];
+        let attrRule = validAttributesMap[name];
 
         // Find rule by pattern matching
         if (!attrRule && validAttributePatterns) {
-          i = validAttributePatterns.length;
+          let i = validAttributePatterns.length;
           while (i--) {
             attrRule = validAttributePatterns[i];
             if (attrRule.pattern.test(name)) {
@@ -365,24 +344,8 @@ const SaxParser = (settings?: SaxParserSettings, schema = Schema()): SaxParser =
       }
 
       // Block any javascript: urls or non image data uris
-      if (filteredUrlAttrs[name] && !settings.allow_script_urls) {
-        let uri = value.replace(trimRegExp, '');
-
-        try {
-          // Might throw malformed URI sequence
-          uri = decodeURIComponent(uri);
-        } catch (ex) {
-          // Fallback to non UTF-8 decoder
-          uri = unescape(uri);
-        }
-
-        if (scriptUriRegExp.test(uri)) {
-          return;
-        }
-
-        if (isInvalidUri(settings, uri, tagName)) {
-          return;
-        }
+      if (filteredUrlAttrs[name] && !URI.isDomSafe(value, tagName, settings)) {
+        return;
       }
 
       // Block data or event attributes on elements marked as internal

modules/tinymce/src/core/main/ts/api/util/URI.ts

@@ -5,8 +5,9 @@
  * For commercial licenses see https://www.tiny.cloud/
  */
 
-import { Arr } from '@ephox/katamari';
+import { Arr, Type } from '@ephox/katamari';
 
+import Entities from '../html/Entities';
 import Tools from './Tools';
 
 /**
@@ -36,6 +37,34 @@ export interface URIConstructor {
   parseDataUri: (uri: string) => { type: string; data: string };
 }
 
+interface SafeUriOptions {
+  readonly allow_html_data_urls?: boolean;
+  readonly allow_script_urls?: boolean;
+  readonly allow_svg_data_urls?: boolean;
+}
+
+const safeSvgDataUrlElements = [ 'img', 'video' ];
+
+const blockSvgDataUris = (allowSvgDataUrls: boolean | undefined, tagName?: string) => {
+  // Only allow SVGs by default on images/videos since the browser won't execute scripts on those elements
+  if (Type.isNullable(tagName)) {
+    return true;
+  } else {
+    const allowed = Type.isNullable(allowSvgDataUrls) ? Arr.contains(safeSvgDataUrlElements, tagName) : allowSvgDataUrls;
+    return !allowed;
+  }
+};
+
+const isInvalidUri = (settings: SafeUriOptions, uri: string, tagName?: string) => {
+  if (settings.allow_html_data_urls) {
+    return false;
+  } else if (/^data:image\//i.test(uri)) {
+    return blockSvgDataUris(settings.allow_svg_data_urls, tagName) && /^data:image\/svg\+xml/i.test(uri);
+  } else {
+    return /^data:/i.test(uri);
+  }
+};
+
 class URI {
 
   public static parseDataUri(uri: string): { type: string; data: string} {
@@ -54,6 +83,40 @@ class URI {
     };
   }
 
+  /**
+   * Check to see if a URI is safe to use in the Document Object Model (DOM). This will return
+   * true if the URI can be used in the DOM without potentially triggering a security issue.
+   *
+   * @method isDomSafe
+   * @static
+   * @param {String} uri The URI to be validated.
+   * @param {Object} context An optional HTML tag name where the element is being used.
+   * @param {Object} options An optional set of options to use when determining if the URI is safe.
+   * @return {Boolean} True if the URI is safe, otherwise false.
+   */
+  public static isDomSafe(uri: string, context?: string, options: SafeUriOptions = {}): boolean {
+    if (options.allow_script_urls) {
+      return true;
+    } else {
+      let decodedUri = Entities.decode(uri).replace(/[\s\u0000-\u001F]+/g, '');
+
+      try {
+        // Might throw malformed URI sequence
+        decodedUri = decodeURIComponent(decodedUri);
+      } catch (ex) {
+        // Fallback to non UTF-8 decoder
+        decodedUri = unescape(decodedUri);
+      }
+
+      // Ensure we don't have a javascript URI, as that is not safe since it allows arbitrary JavaScript execution
+      if (/((java|vb)script|mhtml):/i.test(decodedUri)) {
+        return false;
+      }
+
+      return !isInvalidUri(options, decodedUri, context);
+    }
+  }
+
   public static getDocumentBaseUrl(loc: { protocol: string; host?: string; href?: string; pathname?: string }): string {
     let baseUrl;
 

modules/tinymce/src/core/test/ts/browser/util/UriTest.ts

@@ -1,4 +1,4 @@
-import { describe, it } from '@ephox/bedrock-client';
+import { context, describe, it } from '@ephox/bedrock-client';
 import { assert } from 'chai';
 
 import URI from 'tinymce/core/api/util/URI';
@@ -132,4 +132,44 @@ describe('browser.tinymce.core.util.UriTest', () => {
     assert.deepEqual(getDocumentBaseUrl({ protocol: 'https:', host: '[::1]', pathname: '/dir/path1/path2/' }), 'https://[::1]/dir/path1/path2/');
     assert.deepEqual(getDocumentBaseUrl({ protocol: 'http:', host: '[::1]:8080', pathname: '/dir/path1/path2' }), 'http://[::1]:8080/dir/path1/');
   });
+
+  context('isDomSafe', () => {
+    it('simple cases', () => {
+      assert.isTrue(URI.isDomSafe('http://www.site.com/'));
+      assert.isTrue(URI.isDomSafe('https://www.site.com/'));
+      assert.isTrue(URI.isDomSafe('https://www.site.com:8000/'));
+      assert.isTrue(URI.isDomSafe('ftp://www.site.com:21'));
+      assert.isTrue(URI.isDomSafe('mailto:test@test.com'));
+      assert.isTrue(URI.isDomSafe('//www.site.com/dir1/file2.html'));
+      assert.isTrue(URI.isDomSafe('/path/file.txt'));
+      assert.isTrue(URI.isDomSafe('data:image/png;base64,R3/yw=='));
+      assert.isFalse(URI.isDomSafe('javascript:alert(1)'));
+      assert.isFalse(URI.isDomSafe('jav	ascript:alert(1)'));
+      assert.isFalse(URI.isDomSafe('data:image/svg+xml;base64,R3/yw=='));
+      assert.isFalse(URI.isDomSafe('data:text/html;%3Ch1%3EHello%2C%20World%21%3C%2Fh1%3E'));
+    });
+
+    it('should be safe for SVG data URIs with allow_svg_data_urls', () => {
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'img'));
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'video'));
+      assert.isFalse(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'a'));
+
+      const options = { allow_svg_data_urls: true };
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'img', options));
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'video', options));
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'a', options));
+    });
+
+    it('should always be safe with allow_script_urls', () => {
+      const options = { allow_script_urls: true };
+      assert.isTrue(URI.isDomSafe('javascript:alert(1)', 'p', options));
+      assert.isTrue(URI.isDomSafe('data:image/svg+xml;base64,R3/yw==', 'a', options));
+      assert.isTrue(URI.isDomSafe('data:text/html;%3Ch1%3EHello%2C%20World%21%3C%2Fh1%3E', 'a', options));
+    });
+
+    it('should be safe for HTML data URIs with allow_html_data_urls', () => {
+      const options = { allow_html_data_urls: true };
+      assert.isTrue(URI.isDomSafe('data:text/html;%3Ch1%3EHello%2C%20World%21%3C%2Fh1%3E', 'a', options));
+    });
+  });
 });

modules/tinymce/src/plugins/image/main/ts/core/ImageSelection.ts

@@ -113,14 +113,24 @@ const writeImageDataToSelection = (editor: Editor, data: ImageData) => {
   }
 };
 
+const sanitizeImageData = (editor: Editor, data: ImageData): ImageData => {
+  // Sanitize the URL
+  const src = data.src;
+  return {
+    ...data,
+    src: Utils.isSafeImageUrl(editor, src) ? src : ''
+  };
+};
+
 const insertOrUpdateImage = (editor: Editor, partialData: Partial<ImageData>): void => {
   const image = getSelectedImage(editor);
   if (image) {
     const selectedImageData = read((css) => normalizeCss(editor, css), image);
     const data = { ...selectedImageData, ...partialData };
+    const sanitizedData = sanitizeImageData(editor, data);
 
     if (data.src) {
-      writeImageDataToSelection(editor, data);
+      writeImageDataToSelection(editor, sanitizedData);
     } else {
       deleteImage(editor, image);
     }

modules/tinymce/src/plugins/image/main/ts/core/Utils.ts

@@ -10,6 +10,7 @@ import { Type } from '@ephox/katamari';
 import Editor from 'tinymce/core/api/Editor';
 import { StyleMap } from 'tinymce/core/api/html/Styles';
 import Promise from 'tinymce/core/api/util/Promise';
+import URI from 'tinymce/core/api/util/URI';
 import XHR from 'tinymce/core/api/util/XHR';
 
 import * as Settings from '../api/Settings';
@@ -165,6 +166,9 @@ const blobToDataUri = (blob: Blob): Promise<string> => new Promise((resolve, rej
 const isPlaceholderImage = (imgElm: Element): imgElm is HTMLImageElement =>
   imgElm.nodeName === 'IMG' && (imgElm.hasAttribute('data-mce-object') || imgElm.hasAttribute('data-mce-placeholder'));
 
+const isSafeImageUrl = (editor: Editor, src: string): boolean =>
+  URI.isDomSafe(src, 'img', editor.settings);
+
 export {
   getImageSize,
   removePixelSuffix,
@@ -173,5 +177,6 @@ export {
   createImageList,
   waitLoadImage,
   blobToDataUri,
-  isPlaceholderImage
+  isPlaceholderImage,
+  isSafeImageUrl
 };

modules/tinymce/src/plugins/image/main/ts/ui/Dialog.ts

@@ -393,11 +393,17 @@ const submitHandler = (editor: Editor) => (info: ImageDialogInfo) => (api: API):
   api.close();
 };
 
-const imageSize = (editor: Editor) => (url: string): Promise<Size> =>
-  Utils.getImageSize(editor.documentBaseURI.toAbsolute(url)).then((dimensions) => ({
-    width: String(dimensions.width),
-    height: String(dimensions.height)
-  }));
+const imageSize = (editor: Editor) => (url: string): Promise<Size> => {
+  // If the URL isn't safe then don't attempt to load it to get the sizes
+  if (!Utils.isSafeImageUrl(editor, url)) {
+    return Promise.resolve({ width: '', height: '' });
+  } else {
+    return Utils.getImageSize(editor.documentBaseURI.toAbsolute(url)).then((dimensions) => ({
+      width: String(dimensions.width),
+      height: String(dimensions.height)
+    }));
+  }
+};
 
 const createBlobCache = (editor: Editor) => (file: File, blobUri: string, dataUrl: string): BlobInfo =>
   editor.editorUpload.blobCache.create({

modules/tinymce/src/plugins/image/test/ts/browser/api/CommandsTest.ts

@@ -145,4 +145,14 @@ describe('browser.tinymce.plugins.image.api.CommandsTest', () => {
     });
     TinyAssertions.assertContent(editor, '<p><img src="#1" /></p>');
   });
+
+  it('TINY-7998: Update image with dangerous URL should remove the src attribute', () => {
+    const editor = hook.editor();
+    editor.setContent('<p><img src="#1" alt="alt1" /></p>');
+    TinySelections.setSelection(editor, [ 0 ], 0, [ 0 ], 1);
+    updateImage(editor, {
+      src: 'javascript:alert(1)'
+    });
+    TinyAssertions.assertContent(editor, '<p><img alt="alt1" /></p>');
+  });
 });

modules/tinymce/src/plugins/link/main/ts/core/Utils.ts

@@ -11,6 +11,7 @@ import EditorSelection from 'tinymce/core/api/dom/Selection';
 import DomTreeWalker from 'tinymce/core/api/dom/TreeWalker';
 import Editor from 'tinymce/core/api/Editor';
 import Tools from 'tinymce/core/api/util/Tools';
+import URI from 'tinymce/core/api/util/URI';
 
 import * as Settings from '../api/Settings';
 import { AssumeExternalTargets } from '../api/Types';
@@ -245,8 +246,18 @@ const unwrapOptions = (data: LinkDialogOutput) => {
   }, (v, _k) => Type.isNull(v) === false);
 };
 
+const sanitizeData = (editor: Editor, data: LinkDialogOutput): LinkDialogOutput => {
+  // Sanitize the URL
+  const href = data.href;
+  return {
+    ...data,
+    href: URI.isDomSafe(href, 'a', editor.settings) ? href : ''
+  };
+};
+
 const link = (editor: Editor, attachState: AttachState, data: LinkDialogOutput): void => {
-  editor.hasPlugin('rtc', true) ? editor.execCommand('createlink', false, unwrapOptions(data)) : linkDomMutation(editor, attachState, data);
+  const sanitizedData = sanitizeData(editor, data);
+  editor.hasPlugin('rtc', true) ? editor.execCommand('createlink', false, unwrapOptions(sanitizedData)) : linkDomMutation(editor, attachState, sanitizedData);
 };
 
 const unlink = (editor: Editor): void => {