Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pasting an image with data URL scheme in src, if strict CSP rules are defined #8707

Merged
merged 6 commits into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter';
import env from '@ckeditor/ckeditor5-utils/src/env';

import ImageUploadCommand from '../../src/imageupload/imageuploadcommand';
import { fetchLocalImage, isLocalImage } from '../../src/imageupload/utils';
import { createImageFile, isLocalImage } from '../../src/imageupload/utils';
import { createImageTypeRegExp } from './utils';
import { getViewImgFromWidget } from '../image/utils';

Expand Down Expand Up @@ -123,7 +123,7 @@ export default class ImageUploadEditing extends Plugin {
this.listenTo( editor.plugins.get( Clipboard ), 'inputTransformation', ( evt, data ) => {
const fetchableImages = Array.from( editor.editing.view.createRangeIn( data.content ) )
.filter( value => isLocalImage( value.item ) && !value.item.getAttribute( 'uploadProcessed' ) )
.map( value => { return { promise: fetchLocalImage( value.item ), imageElement: value.item }; } );
.map( value => { return { promise: createImageFile( value.item ), imageElement: value.item }; } );

if ( !fetchableImages.length ) {
return;
Expand Down
84 changes: 53 additions & 31 deletions packages/ckeditor5-image/src/imageupload/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
* @module image/imageupload/utils
*/

/* global fetch, File */
import global from '@ckeditor/ckeditor5-utils/src/dom/global';

/* global File */

/**
* Creates a regular expression used to test for image files.
Expand All @@ -27,28 +29,22 @@ export function createImageTypeRegExp( types ) {
}

/**
* Creates a promise that fetches the image local source (Base64 or blob) and resolves with a `File` object.
* Creates a promise that converts the image local source (Base64 or blob) to a blob and resolves with a `File` object.
*
* @param {module:engine/view/element~Element} image Image whose source to fetch.
* @returns {Promise.<File>} A promise which resolves when an image source is fetched and converted to a `File` instance.
* @param {module:engine/view/element~Element} image Image whose source to convert.
* @returns {Promise.<File>} A promise which resolves when an image source is converted to a `File` instance.
* It resolves with a `File` object. If there were any errors during file processing, the promise will be rejected.
*/
export function fetchLocalImage( image ) {
return new Promise( ( resolve, reject ) => {
const imageSrc = image.getAttribute( 'src' );

// Fetch works asynchronously and so does not block browser UI when processing data.
fetch( imageSrc )
.then( resource => resource.blob() )
.then( blob => {
const mimeType = getImageMimeType( blob, imageSrc );
const ext = mimeType.replace( 'image/', '' );
const filename = `image.${ ext }`;
const file = new File( [ blob ], filename, { type: mimeType } );

resolve( file );
} )
.catch( reject );
export function createImageFile( image ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor breaking change –> should be added to the merge commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer a minor breaking change in this PR, because the original fetchLocalImage() public helper is unchanged.

const imageSrc = image.getAttribute( 'src' );

// Conversion to blob works asynchronously, so it does not block browser UI when processing data.
return getBlobFromImage( imageSrc ).then( blob => {
const mimeType = getImageMimeType( imageSrc );
const ext = mimeType.replace( 'image/', '' );
const filename = `image.${ ext }`;

return new File( [ blob ], filename, { type: mimeType } );
} );
}

Expand All @@ -67,18 +63,44 @@ export function isLocalImage( node ) {
node.getAttribute( 'src' ).match( /^blob:/g );
}

// Extracts an image type based on its blob representation or its source.
// Creates a promise that resolves with a `Blob` object converted from the image source (Base64 or blob).
//
// @param {String} imageSrc Image `src` attribute value.
// @returns {Promise.<Blob>}
function getBlobFromImage( imageSrc ) {
return new Promise( ( resolve, reject ) => {
const image = global.document.createElement( 'img' );

image.addEventListener( 'load', () => {
const canvas = global.document.createElement( 'canvas' );

canvas.width = image.width;
canvas.height = image.height;

const ctx = canvas.getContext( '2d' );

ctx.drawImage( image, 0, 0 );

canvas.toBlob(
blob => blob ? resolve( blob ) : reject(),
getImageMimeType( imageSrc ),
1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should not use a default value for qualityArgument - it's 0.92 for jpeg. What do you think @Reinmar ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what it affects. I don't know the context of this code. Could you try to explain the context and pros/cons of both approaches?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation is just fetching the image from data: URL so its quality remains the same as the pasted image, but in this PR we are using Canvas to draw the image and then toBlob is used to generate jpeg (or other image formats). While converting to jpeg, the image data is compressed. By default, toBlob is using 0.92 compression ratio so it affects the size of the generated blob but also the quality of the image. The question is whether we should let the browser to use the default quality or we should try to keep the best quality possible (the hardcoded 1 in the code above).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, pros/cons of both options are not 100% clear for me. E.g. I can only guess that using 1 will increase network traffic. Am I right?

Also, if we'll use 0.92 will it work execatly like it worked before? Or will like... compress again something that was already compressed adding another level of data loss to what's already a compressed thing?

Again, this requires analysis from you guys and explaining to me what does it actually do. Consider me dumb as a chair's leg for the purpose of this review ;)

);
} );

image.addEventListener( 'error', reject );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line triggers an alert with [object Event] in it, maybe we could replace this line with image.addEventListener( 'error', () => reject() ); to ignore those errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good hint 👍🏻


image.src = imageSrc;
} );
}

// Extracts an image type based on its source.
//
// @param {String} src Image `src` attribute value.
// @param {Blob} blob Image blob representation.
// @returns {String}
function getImageMimeType( blob, src ) {
if ( blob.type ) {
return blob.type;
} else if ( src.match( /data:(image\/\w+);base64/ ) ) {
return src.match( /data:(image\/\w+);base64/ )[ 1 ].toLowerCase();
} else {
// Fallback to 'jpeg' as common extension.
return 'image/jpeg';
}
function getImageMimeType( src ) {
const match = src.match( /data:(image\/\w+);base64/ );

// Fallback to 'jpeg' as common extension.
return match ? match[ 1 ].toLowerCase() : 'image/jpeg';
}
125 changes: 81 additions & 44 deletions packages/ckeditor5-image/tests/imageupload/imageuploadediting.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* globals window, setTimeout, atob, URL, Blob, console */
/* globals document, window, setTimeout, atob, URL, Blob, HTMLCanvasElement, console */

import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';

Expand All @@ -28,6 +28,7 @@ import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification';
describe( 'ImageUploadEditing', () => {
// eslint-disable-next-line max-len
const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=';
const base64InvalidSample = 'data:image/png;base64,INVALID-DATA';

let adapterMocks = [];
let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader;
Expand Down Expand Up @@ -671,7 +672,7 @@ describe( 'ImageUploadEditing', () => {
expectModel( done, getModelData( model ), expected );
} );

it( 'should not upload and remove image if fetch failed', done => {
it( 'should not upload and remove image if conversion to a blob failed', done => {
const notification = editor.plugins.get( Notification );

// Prevent popping up alert window.
Expand All @@ -681,17 +682,12 @@ describe( 'ImageUploadEditing', () => {

setModelData( model, '<paragraph>[]foo</paragraph>' );

const clipboardHtml = `<img src=${ base64Sample } />`;
const clipboardHtml = `<img src=${ base64InvalidSample } />`;
const dataTransfer = mockDataTransfer( clipboardHtml );

const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

// Stub `fetch` so it can be rejected.
sinon.stub( window, 'fetch' ).callsFake( () => {
return new Promise( ( res, rej ) => rej( 'could not fetch' ) );
} );

let content = null;
editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', ( evt, data ) => {
content = data.content;
Expand All @@ -709,7 +705,7 @@ describe( 'ImageUploadEditing', () => {
);
} );

it( 'should upload only images which were successfully fetched and remove failed ones', done => {
it( 'should upload only images which were successfully converted to a blob and remove failed ones', done => {
const notification = editor.plugins.get( Notification );

// Prevent popping up alert window.
Expand All @@ -729,25 +725,15 @@ describe( 'ImageUploadEditing', () => {

setModelData( model, '<paragraph>[]foo</paragraph>' );

// The first 2 images are valid ones, and the 3rd one fails.
const clipboardHtml = `<p>bar</p><img src=${ base64Sample } />` +
`<img src=${ base64ToBlobUrl( base64Sample ) } /><img src=${ base64Sample } />`;
`<img src=${ base64ToBlobUrl( base64Sample ) } />` +
`<img src=${ base64InvalidSample } />`;
const dataTransfer = mockDataTransfer( clipboardHtml );

const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

// Stub `fetch` in a way that 2 first calls are successful and 3rd fails.
let counter = 0;
const fetch = window.fetch;
sinon.stub( window, 'fetch' ).callsFake( src => {
counter++;
if ( counter < 3 ) {
return fetch( src );
} else {
return Promise.reject();
}
} );

let content = null;
editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', ( evt, data ) => {
content = data.content;
Expand Down Expand Up @@ -848,15 +834,6 @@ describe( 'ImageUploadEditing', () => {
const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

// Stub `fetch` to return custom blob without type.
sinon.stub( window, 'fetch' ).callsFake( () => {
return new Promise( res => res( {
blob() {
return new Promise( res => res( new Blob( [ 'foo', 'bar' ] ) ) );
}
} ) );
} );

viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );

tryExpect( done, () => {
Expand All @@ -873,15 +850,6 @@ describe( 'ImageUploadEditing', () => {
const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

// Stub `fetch` to return custom blob without type.
sinon.stub( window, 'fetch' ).callsFake( () => {
return new Promise( res => res( {
blob() {
return new Promise( res => res( new Blob( [ 'foo', 'bar' ] ) ) );
}
} ) );
} );

viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );

tryExpect( done, () => {
Expand All @@ -907,8 +875,8 @@ describe( 'ImageUploadEditing', () => {
const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

// Stub `fetch` in a way that it always fails.
sinon.stub( window, 'fetch' ).callsFake( () => Promise.reject() );
// Stub `HTMLCanvasElement#toBlob` to return invalid blob, so image conversion always fails.
sinon.stub( HTMLCanvasElement.prototype, 'toBlob' ).callsFake( fn => fn( null ) );

viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );

Expand All @@ -923,13 +891,82 @@ describe( 'ImageUploadEditing', () => {
} );
} );

describe( 'with Content Security Policy rules', () => {
let metaElement;
let previousMetaContent;

// Set the CSP rules before the first test in this block has been executed.
before( () => {
metaElement = document.querySelector( '[http-equiv=Content-Security-Policy]' );

if ( metaElement ) {
previousMetaContent = metaElement.getAttribute( 'content' );
} else {
metaElement = document.createElement( 'meta' );
metaElement.setAttribute( 'http-equiv', 'Content-Security-Policy' );

document.head.appendChild( metaElement );
}

metaElement.setAttribute( 'content', '' +
'default-src \'none\'; ' +
'connect-src \'self\'; ' +
'script-src \'self\'; ' +
'img-src * data: blob:;' +
'style-src \'self\' \'unsafe-inline\'; ' +
'frame-src *'
);
} );

// Remove or restore the previous CSP rules after the last test in this block has been executed.
after( () => {
if ( previousMetaContent ) {
metaElement.setAttribute( 'content', previousMetaContent );
} else {
document.head.removeChild( metaElement );
}
} );

// See https://github.com/ckeditor/ckeditor5/issues/7957.
it( 'should upload image if strict CSP rules are defined', done => {
const spy = sinon.spy();
const notification = editor.plugins.get( Notification );

notification.on( 'show:warning', evt => {
spy();
evt.stop();
}, { priority: 'high' } );

setModelData( model, '<paragraph>[]foo</paragraph>' );

const clipboardHtml = `<p>bar</p><img src=${ base64Sample } />`;
const dataTransfer = mockDataTransfer( clipboardHtml );

const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
const targetViewRange = editor.editing.mapper.toViewRange( targetRange );

viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );

adapterMocks[ 0 ].loader.file.then( () => {
setTimeout( () => {
sinon.assert.notCalled( spy );
done();
} );
} ).catch( () => {
setTimeout( () => {
expect.fail( 'Promise should be resolved.' );
} );
} );
} );
} );

// Helper for validating clipboard and model data as a result of a paste operation. This function checks both clipboard
// data and model data synchronously (`expectedClipboardData`, `expectedModel`) and then the model data after `loader.file`
// promise is resolved (so model state after successful/failed file fetch attempt).
// promise is resolved (so model state after successful/failed file conversion attempt).
//
// @param {String} expectedClipboardData Expected clipboard data on `inputTransformation` event.
// @param {String} expectedModel Expected model data on `inputTransformation` event.
// @param {String} expectedModelOnFile Expected model data after all `file.loader` promises are fetched.
// @param {String} expectedModelOnFile Expected model data after all `file.loader` promises are fulfilled.
// @param {DocumentFragment} content Content processed in inputTransformation
// @param {Function} doneFn Callback function to be called when all assertions are done or error occures.
// @param {Boolean} [onSuccess=true] If `expectedModelOnFile` data should be validated
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<head>
<meta http-equiv="Content-Security-Policy" content="
default-src 'none';
connect-src 'self' http://*.cke-cs.com;
script-src 'self' 'unsafe-eval';
img-src * data: blob:;
style-src 'self' 'unsafe-inline';
frame-src *"
>
</head>

<div id="editor">
<h2>Paste here:</h2>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* globals console, window, document */

import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';

import Strikethrough from '@ckeditor/ckeditor5-basic-styles/src/strikethrough';
import Underline from '@ckeditor/ckeditor5-basic-styles/src/underline';
import Table from '@ckeditor/ckeditor5-table/src/table';
import TableToolbar from '@ckeditor/ckeditor5-table/src/tabletoolbar';
import EasyImage from '@ckeditor/ckeditor5-easy-image/src/easyimage';
import FontColor from '@ckeditor/ckeditor5-font/src/fontcolor';
import FontBackgroundColor from '@ckeditor/ckeditor5-font/src/fontbackgroundcolor';
import PageBreak from '@ckeditor/ckeditor5-page-break/src/pagebreak';
import TableProperties from '@ckeditor/ckeditor5-table/src/tableproperties';
import TableCellProperties from '@ckeditor/ckeditor5-table/src/tablecellproperties';

import PasteFromOffice from '../../../../src/pastefromoffice';

import { CS_CONFIG } from '@ckeditor/ckeditor5-cloud-services/tests/_utils/cloud-services-config';

ClassicEditor
.create( document.querySelector( '#editor' ), {
plugins: [ ArticlePluginSet, Strikethrough, Underline, Table, TableToolbar, PageBreak,
TableProperties, TableCellProperties, EasyImage, PasteFromOffice, FontColor, FontBackgroundColor ],
toolbar: [ 'heading', '|', 'bold', 'italic', 'strikethrough', 'underline', 'link',
'bulletedList', 'numberedList', 'blockQuote', 'insertTable', 'pageBreak', 'undo', 'redo' ],
table: {
contentToolbar: [ 'tableColumn', 'tableRow', 'mergeTableCells', 'tableProperties', 'tableCellProperties' ]
},
cloudServices: CS_CONFIG
} )
.then( editor => {
window.editor = editor;
} )
.catch( err => {
console.error( err.stack );
} );
Loading