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

psmyrek · 2020-12-18T13:07:36Z

Suggested merge commit message (convention)

Fix (image): Allow pasting an image with data URL scheme in src, if strict CSP rules are defined. Closes #7957.

Reinmar · 2020-12-21T14:23:16Z

packages/ckeditor5-image/src/imageupload/utils.js

-				resolve( file );
-			} )
-			.catch( reject );
+export function createImageFile( image ) {


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

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

packages/ckeditor5-image/src/imageupload/utils.js

niegowski · 2020-12-21T14:57:17Z

packages/ckeditor5-image/src/imageupload/utils.js

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


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 ?

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?

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).

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 ;)

niegowski · 2020-12-21T18:46:49Z

Ok, as the discussion in #8707 (comment) seems to be more product specific and not the code itself I'm extracting it here:

The current implementation is just fetching the image binary data that was provided from the clipboard (compressed image) and uploads it to the server (without any change).

In this PR this is changed, the image is no longer fetched, it's now loaded as a standard image element, then it's drawn onto an internal canvas element, so the image is decompressed to the pixels data, and then it's compressed to the same file format. So the original image is once again compressed and if it was using lossy compression, then another round of compression is increasing losses of the quality of the image.

During recompression of the pixel data to the image that will be uploaded to the server, we can specify the quality argument, a number between 0 and 1 indicating image quality. The default value for JPEG is 0.92.

And here comes the question - what compression ratio should we use to recompress the image:

1 - best quality
- 👍 because it minimizes the new artifacts
- 👎 because it increases the image file size
0.92 - default quality
- 👍 because the image size would be reasonable (but still might be different from the original one)
- 👎 because it can introduce more artifacts to the image

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?

Currently, the image binary data is passed to the server without any changes, after the change it would recompress the original image.

psmyrek · 2020-12-22T08:07:36Z

Maybe changes from this PR should be provided as a fallback to the original code?

I see 2 options:

If the original fetch() call has failed due to strict CSP rules, then try with this fallback, but:
- failed fetch() call will log an error in the console, which is not nice,
- I'm not sure, if it is possible to detect the fetch() failure reason.
Detect CSP rules and then choose, which method should be used: original one with fetch(), or this fallback with canvas, but:
- I'm not sure, how difficult it will be to find all cases, where fetch() method fails, if strict CSP rules are defined.

Reinmar · 2020-12-22T09:17:43Z

Maybe changes from this PR should be provided as a fallback to the original code?

That'd make sense. Both options (1 and 0.92) have a negative impact. The original behavior seems superior, so if we can keep it for as many cases as possible, that'd be great.

psmyrek · 2020-12-22T12:28:06Z

In next days I'll try to change my solution with canvas to a fallback procedure, which will be called in case fetch() has failed.

psmyrek · 2020-12-28T14:00:21Z

I've re-worked the whole solution. The original behaviour with fetch() is used in the same way as previously, and the fallback solution using canvas is called only, if fetch() has failed. It works fine, but:

If the fetch() call has failed due to strict CSP rules, then it logs an error in the console, which is not nice.
It is not possible to detect the exact reason why fetch() has failed - it just rejects the promise with either AbortError or TypeError:
- The AbortError is returned when the request was aborted due to a call to the AbortController#abort() method, which is not the case in this issue.
- The TypeError is returned when it can't make a request due to a network failure or if anything prevented the request from completing: URL can't be parsed, or URL does not exist, or there is no internet connection, or request was prevented by CORS, or finally the request was prevented by strict CSP rules, which is our case in this issue.

niegowski · 2020-12-29T09:41:15Z

I've re-worked the whole solution. The original behaviour with fetch() is used in the same way as previously, and the fallback solution using canvas is called only, if fetch() has failed. It works fine, but:

If the fetch() call has failed due to strict CSP rules, then it logs an error in the console, which is not nice.

It is not possible to detect the exact reason why fetch() has failed - it just rejects the promise with either AbortError or TypeError:

The AbortError is returned when the request was aborted due to a call to the AbortController#abort() method, which is not the case in this issue.

The TypeError is returned when it can't make a request due to a network failure or if anything prevented the request from completing: URL can't be parsed, or URL does not exist, or there is no internet connection, or request was prevented by CORS, or finally the request was prevented by strict CSP rules, which is our case in this issue.

I also don't like the error logged in the console. I was thinking about how could we avoid it. I can see two options:

checking CSP rules - but this seems redundant for such a rare issue
exposing a configuration option for the image plugin that would indicate that the editor should use fallback without trying to fetch the image - this would be a direct indication that CSP rules are strict. So by default, I would use fetch + canvas fallback, but if the configuration option is set, then it would use canvas. WDYT @Reinmar ?

Reinmar · 2021-01-04T10:46:58Z

I also don't like the error logged in the console. I was thinking about how could we avoid it. I can see two options:

I can see that @psmyrek used a try-catch there. Does it prevent logging any error, @psmyrek?

Also, cc @ckeditor/qa-team – could you retest this PR?

psmyrek · 2021-01-04T12:01:23Z

I can see that @psmyrek used a try-catch there. Does it prevent logging any error, @psmyrek?

Nope, error is logged in the console.

LukaszGudel · 2021-01-05T11:26:55Z

I've tested this PR and it is working on manual tests. I did not find any regression.

When copying and pasting some content with an images console returns error for each image:

utils.js:43 
Refused to connect to 'data:image/jpeg;base64,/9j/4AA.....ChQo6RH//Z' because it violates the document's Content Security Policy.

but image is added and editor is still working correctly.

niegowski · 2021-01-07T12:19:31Z

2. exposing a configuration option for the image plugin that would indicate that the editor should use fallback without trying to fetch the image - this would be a direct indication that CSP rules are strict. So by default, I would use fetch + canvas fallback, but if the configuration option is set, then it would use canvas.

What do you think about such a configuration option?

Reinmar · 2021-01-11T09:30:28Z

What do you think about such a configuration option?

Just to be sure – if someone has strict CSP rules, how often will they see the error? Is this for all content coming from Word or in specific cases only?

niegowski · 2021-01-11T10:03:10Z

Just to be sure – if someone has strict CSP rules, how often will they see the error? Is this for all content coming from Word or in specific cases only?

AFAIK every content pasted from Word that contains an image will trigger that error.

Reinmar · 2021-01-11T10:43:27Z

So it seems that the option would be justified.

Alternatively, we can release this fix without the option and see if anyone complains. There's a high chance no one will, as long as it works.

niegowski · 2021-01-11T14:08:52Z

packages/ckeditor5-image/src/imageupload/utils.js

+			canvas.toBlob( blob => blob ? resolve( blob ) : reject() );
+		} );
+
+		image.addEventListener( 'error', reject );


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?

Thanks, good hint 👍🏻

Use canvas.toBlob() instead of fetch() to create a blob from image src

8737a48

psmyrek marked this pull request as draft December 18, 2020 13:33

Fixed code coverage

a0af560

psmyrek marked this pull request as ready for review December 18, 2020 14:50

niegowski self-assigned this Dec 21, 2020

niegowski self-requested a review December 21, 2020 09:05

Reinmar reviewed Dec 21, 2020

View reviewed changes

niegowski reviewed Dec 21, 2020

View reviewed changes

Call getImageMimeType() once

8c6c7a1

psmyrek added 2 commits December 28, 2020 14:13

Use canvas conversion as a fallback only if fetch() has failed

9079fcd

Cosmetic clean-up

ac23816

psmyrek requested a review from niegowski December 28, 2020 14:03

LukaszGudel mentioned this pull request Jan 5, 2021

fetch doesn't support blob images which are CORS. #8422

Closed

niegowski reviewed Jan 11, 2021

View reviewed changes

Prevent showing an alert() with event object

402e793

niegowski merged commit f7a3948 into master Jan 12, 2021

niegowski deleted the i/7957 branch January 12, 2021 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

psmyrek commented Dec 18, 2020 •

edited

Loading

Reinmar Dec 21, 2020

psmyrek Dec 22, 2020

psmyrek Dec 28, 2020

niegowski Dec 21, 2020

Reinmar Dec 21, 2020

niegowski Dec 21, 2020

Reinmar Dec 21, 2020

niegowski commented Dec 21, 2020

psmyrek commented Dec 22, 2020

Reinmar commented Dec 22, 2020

psmyrek commented Dec 22, 2020

psmyrek commented Dec 28, 2020

niegowski commented Dec 29, 2020

Reinmar commented Jan 4, 2021

psmyrek commented Jan 4, 2021

LukaszGudel commented Jan 5, 2021 •

edited

Loading

niegowski commented Jan 7, 2021

Reinmar commented Jan 11, 2021

niegowski commented Jan 11, 2021

Reinmar commented Jan 11, 2021

niegowski Jan 11, 2021

psmyrek Jan 12, 2021

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

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

Conversation

psmyrek commented Dec 18, 2020 • edited Loading

Suggested merge commit message (convention)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niegowski commented Dec 21, 2020

psmyrek commented Dec 22, 2020

Reinmar commented Dec 22, 2020

psmyrek commented Dec 22, 2020

psmyrek commented Dec 28, 2020

niegowski commented Dec 29, 2020

Reinmar commented Jan 4, 2021

psmyrek commented Jan 4, 2021

LukaszGudel commented Jan 5, 2021 • edited Loading

niegowski commented Jan 7, 2021

Reinmar commented Jan 11, 2021

niegowski commented Jan 11, 2021

Reinmar commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psmyrek commented Dec 18, 2020 •

edited

Loading

LukaszGudel commented Jan 5, 2021 •

edited

Loading