CVE-2018-11093

@@ -14,7 +14,7 @@ import {
 import { upcastElementToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
 import LinkCommand from './linkcommand';
 import UnlinkCommand from './unlinkcommand';
-import { createLinkElement } from './utils';
+import { createLinkElement, ensureSafeUrl } from './utils';
 import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute';
 import findLinkRange from './findlinkrange';
 import '../theme/link.css';
@@ -38,9 +38,14 @@ export default class LinkEditing extends Plugin {
 		// Allow link attribute on all inline nodes.
 		editor.model.schema.extend( '$text', { allowAttributes: 'linkHref' } );
 
-		editor.conversion.for( 'downcast' )
+		editor.conversion.for( 'dataDowncast' )
 			.add( downcastAttributeToElement( { model: 'linkHref', view: createLinkElement } ) );
 
+		editor.conversion.for( 'editingDowncast' )
+			.add( downcastAttributeToElement( { model: 'linkHref', view: ( href, writer ) => {
+				return createLinkElement( ensureSafeUrl( href ), writer );
+			} } ) );
+
 		editor.conversion.for( 'upcast' )
 			.add( upcastElementToAttribute( {
 				view: {

@@ -16,6 +16,8 @@ import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker';
 import FocusCycler from '@ckeditor/ckeditor5-ui/src/focuscycler';
 import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
 
+import { ensureSafeUrl } from '../utils';
+
 import unlinkIcon from '../../theme/icons/unlink.svg';
 import pencilIcon from '@ckeditor/ckeditor5-core/theme/icons/pencil.svg';
 import '../../theme/linkactions.css';
@@ -206,7 +208,7 @@ export default class LinkActionsView extends View {
 					'ck',
 					'ck-link-actions__preview'
 				],
-				href: bind.to( 'href' ),
+				href: bind.to( 'href', href => href && ensureSafeUrl( href ) ),
 				target: '_blank'
 			}
 		} );

@@ -9,6 +9,9 @@
 
 const linkElementSymbol = Symbol( 'linkElement' );
 
+const ATTRIBUTE_WHITESPACES = /[\u0000-\u0020\u00A0\u1680\u180E\u2000-\u2029\u205f\u3000]/g; // eslint-disable-line no-control-regex
+const SAFE_URL = /^(?:(?:https?|ftps?|mailto):|[^a-z]|[a-z+.-]+(?:[^a-z+.:-]|$))/i;
+
 /**
  * Returns `true` if a given view node is the link element.
  *
@@ -32,3 +35,29 @@ export function createLinkElement( href, writer ) {
 
 	return linkElement;
 }
+
+/**
+ * Returns a safe URL based on a given value.
+ *
+ * An URL is considered safe if it is safe for the user (does not contain any malicious code).
+ *
+ * If URL is considered unsafe, a simple `"#"` is returned.
+ *
+ * @protected
+ * @param {*} url
+ * @returns {String} Safe URL.
+ */
+export function ensureSafeUrl( url ) {
+	url = String( url );
+
+	return isSafeUrl( url ) ? url : '#';
+}
+
+// Checks whether the given URL is safe for the user (does not contain any malicious code).
+//
+// @param {String} url URL to check.
+function isSafeUrl( url ) {
+	const normalizedUrl = url.replace( ATTRIBUTE_WHITESPACES, '' );
+
+	return normalizedUrl.match( SAFE_URL );
+}

@@ -118,6 +118,25 @@ describe( 'LinkEditing', () => {
 
 			expect( editor.getData() ).to.equal( '<p><a href="">foo</a>bar</p>' );
 		} );
+
+		// The editor's role is not to filter out potentially malicious data.
+		// Its job is to not let this code be executed inside the editor (see the test in "editing pipeline conversion").
+		it( 'should output a link with a potential XSS code', () => {
+			setModelData( model, '<paragraph>[]</paragraph>' );
+
+			model.change( writer => {
+				writer.insertText( 'foo', { linkHref: 'javascript:alert(1)' }, model.document.selection.getFirstPosition() );
+			} );
+
+			expect( editor.getData() ).to.equal( '<p><a href="javascript:alert(1)">foo</a></p>' );
+		} );
+
+		it( 'should load a link with a potential XSS code', () => {
+			editor.setData( '<p><a href="javascript:alert(1)">foo</a></p>' );
+
+			expect( getModelData( model, { withoutSelection: true } ) )
+				.to.equal( '<paragraph><$text linkHref="javascript:alert(1)">foo</$text></paragraph>' );
+		} );
 	} );
 
 	describe( 'editing pipeline conversion', () => {
@@ -147,6 +166,13 @@ describe( 'LinkEditing', () => {
 
 			expect( editor.getData() ).to.equal( '<p><a href="url">a<f>b</f>c</a></p>' );
 		} );
+
+		it( 'must not render a link with a potential XSS code', () => {
+			setModelData( model, '<paragraph><$text linkHref="javascript:alert(1)">[]foo</$text>bar[]</paragraph>' );
+
+			expect( getViewData( editor.editing.view, { withoutSelection: true } ) )
+				.to.equal( '<p><a href="#">foo</a>bar</p>' );
+		} );
 	} );
 
 	describe( 'link highlighting', () => {

@@ -88,7 +88,7 @@ describe( 'LinkActionsView', () => {
 				expect( view.previewButtonView.element.getAttribute( 'target' ) ).to.equal( '_blank' );
 			} );
 
-			describe( '<button> bindings', () => {
+			describe( '<a> bindings', () => {
 				it( 'binds href DOM attribute to view#href', () => {
 					expect( view.previewButtonView.element.getAttribute( 'href' ) ).to.be.null;
 
@@ -97,6 +97,12 @@ describe( 'LinkActionsView', () => {
 					expect( view.previewButtonView.element.getAttribute( 'href' ) ).to.equal( 'foo' );
 				} );
 
+				it( 'does not render unsafe view#href', () => {
+					view.href = 'javascript:alert(1)';
+
+					expect( view.previewButtonView.element.getAttribute( 'href' ) ).to.equal( '#' );
+				} );
+
 				it( 'binds #isEnabled to view#href', () => {
 					expect( view.previewButtonView.isEnabled ).to.be.false;
 

@@ -9,10 +9,10 @@ import AttributeElement from '@ckeditor/ckeditor5-engine/src/view/attributeeleme
 import ContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement';
 import Text from '@ckeditor/ckeditor5-engine/src/view/text';
 
-import { createLinkElement, isLinkElement } from '../src/utils';
+import { createLinkElement, isLinkElement, ensureSafeUrl } from '../src/utils';
 
 describe( 'utils', () => {
-	describe( 'isLinkElement', () => {
+	describe( 'isLinkElement()', () => {
 		it( 'should return true for elements created by createLinkElement', () => {
 			const element = createLinkElement( 'http://ckeditor.com', new ViewWriter( new ViewDocument() ) );
 
@@ -32,7 +32,7 @@ describe( 'utils', () => {
 		} );
 	} );
 
-	describe( 'createLinkElement', () => {
+	describe( 'createLinkElement()', () => {
 		it( 'should create link AttributeElement', () => {
 			const element = createLinkElement( 'http://cksource.com', new ViewWriter( new ViewDocument() ) );
 
@@ -42,4 +42,119 @@ describe( 'utils', () => {
 			expect( element.name ).to.equal( 'a' );
 		} );
 	} );
+
+	describe( 'ensureSafeUrl()', () => {
+		it( 'returns the same absolute http URL', () => {
+			const url = 'http://xx.yy/zz#foo';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same absolute https URL', () => {
+			const url = 'https://xx.yy/zz';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same absolute ftp URL', () => {
+			const url = 'ftp://xx.yy/zz';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same absolute ftps URL', () => {
+			const url = 'ftps://xx.yy/zz';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same absolute mailto URL', () => {
+			const url = 'mailto://foo@bar.com';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same relative URL (starting with a dot)', () => {
+			const url = './xx/yyy';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same relative URL (starting with two dots)', () => {
+			const url = '../../xx/yyy';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same relative URL (starting with a slash)', () => {
+			const url = '/xx/yyy';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same relative URL (starting with a backslash)', () => {
+			const url = '\\xx\\yyy';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same relative URL (starting with a letter)', () => {
+			const url = 'xx/yyy';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same URL even if it contains whitespaces', () => {
+			const url = '  ./xx/ yyy\t';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'returns the same URL even if it contains non ASCII characters', () => {
+			const url = 'https://kłącze.yy/źdźbło';
+
+			expect( ensureSafeUrl( url ) ).to.equal( url );
+		} );
+
+		it( 'accepts non string values', () => {
+			expect( ensureSafeUrl( undefined ) ).to.equal( 'undefined' );
+			expect( ensureSafeUrl( null ) ).to.equal( 'null' );
+		} );
+
+		it( 'returns safe URL when a malicious URL starts with javascript:', () => {
+			const url = 'javascript:alert(1)';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+
+		it( 'returns safe URL when a malicious URL starts with an unknown protocol', () => {
+			const url = 'foo:alert(1)';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+
+		it( 'returns safe URL when a malicious URL contains spaces', () => {
+			const url = 'java\u0000script:\talert(1)';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+
+		it( 'returns safe URL when a malicious URL contains spaces (2)', () => {
+			const url = '\u0000 javascript:alert(1)';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+
+		it( 'returns safe URL when a malicious URL contains a safe part', () => {
+			const url = 'javascript:alert(1)\nhttp://xxx';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+
+		it( 'returns safe URL when a malicious URL contains a safe part (2)', () => {
+			const url = 'javascript:alert(1);http://xxx';
+
+			expect( ensureSafeUrl( url ) ).to.equal( '#' );
+		} );
+	} );
 } );