Skip to content

Add Function isEncoded to Base64 and check ids of image in tiny mce #11025

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

Closed
wants to merge 3 commits into from

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Sep 25, 2017

Added function to Base64 js class to avoid error with window.atob

Description

Added function isEncoded to Base64 js class
Added check in setup.js in decodeWidgets function to check if id is Base64Encoded String
Added check in editor_plugin.js in tinyMCE.init function to check if id is Base64Encoded String

Fixed Issues (if relevant)

  1. Cant put id or data attribute on <img> tags in the M2 wysiwyg. #10353
  2. Failed to Execute 'atob' on 'Window' The string to be decoded is not correctly encoded. #7079

Manual testing scenarios

  1. Create new page:Content -> Pages -> New Page
  2. Add image with id <img src='test.jpg' id='test /> or <img src='test.jpg' data-id='test />
  3. the modal shows an error: error: error in [unknown object].fireEvent(): event name: tinymceSetContent error message: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@osrecio osrecio changed the title Add Function isDecoded to Base64 and check ids of image in tiny mce Add Function isEncoded to Base64 and check ids of image in tiny mce Sep 25, 2017
@orlangur orlangur self-assigned this Sep 26, 2017
@orlangur orlangur added this to the September 2017 milestone Sep 30, 2017
@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33
@osrecio
Copy link
Member Author

osrecio commented Oct 27, 2017

Can I help with something @okorshenko @orlangur ?

@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@korostii
Copy link
Contributor

korostii commented Nov 3, 2017

Please do correct me if I'm wrong, but it seems that it's not possible to guarantee that a string was base64-encoded.
Isn't that right? Why is it even possible to have both base64-encoded and not base64-encoded strings here in the first place?
Shouldn't we rather identify and fix that cause rather than mask the consequences with a partial fix?

@osrecio
Copy link
Member Author

osrecio commented Nov 3, 2017

The problem it's assume that you can have a widget and this is encoded in base64. In this case if string is encoded in base64 proceed to decode. If not base64 because is a normal string not try to decode.

@korostii
Copy link
Contributor

korostii commented Nov 3, 2017

I'm not sure whether you understand my point.

Let me try to illustrate it with an example: would you say that bAsE6464 is a base64-encoded string?
It might be. But also, it might be a might be a piece of text which just happens to consist of valid base64 characters, thus it'll pass your test and get "decoded" which'll transform it into some unreadable trash.

I'm pretty sure that the kind of solution you suggest here doesn't cover this case. It just can't do that and I'm pretty sure you'll see that if you perform some tests on it.

The only way to reliably fix this issue IMHO is to prefix any base64 data with an explicit header when it's encoded , similar to the data-uri prefix used when embedding base64 images into img's src: https://stackoverflow.com/questions/1207190/embedding-base64-images

@osrecio
Copy link
Member Author

osrecio commented Nov 3, 2017

Oh sorry, Now I understand you. I Can make this change with prefix, but I don't know if can be accepted by core team.

@magento-engcom-team magento-engcom-team added 2.2.x Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017
@@ -364,6 +364,7 @@ var Fieldset = {
var Base64 = {
// private property
_keyStr: 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=',
_notBase64 : /[^A-Z0-9+\/=]/i,

Choose a reason for hiding this comment

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

More precise regexp may be used to verify is string looks like base64 encoded or not:

(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)

@vkublytskyi
Copy link

@osrecio, @korostii In my opinion placing some prefix would not be correct as this would be implicit knowledge about how this string is used and will lead to code coupling and bugs in future. Instead, we may use one of the following approaches:

  • introduce some object/data structure that will provide more information about content (e.g. format, mime type, etc.)
  • implement parser for some format (e.g. Data URLs data:[<mediatype>][;base64],<data>) and use it instead of only data part where it is appropriate.

In the same time I think that possibility to validate that some data is in Base64 format is a good feature to have (it is necesary condition but not sufficient).

@korostii
Copy link
Contributor

@vkublytskyi,

In my opinion placing some prefix would not be correct as this would be implicit knowledge about how this string is used and will lead to code coupling and bugs in future.

With all due respect, it's actually the exact opposite. Right now the knowledge is absent (there might be base64, or might be not). If you add an explicit prefix, then you know for sure whether it's base64 or not.

possibility to validate that some data is in Base64 format is a good feature to have

It's not possible to validate whether a string is base64 with 100% accuracy, there will always be false positives. Please carefully see my comment above for a counter-example.
Such a "validation" may fix the obvious problematic cases, but not all, for sure. Not unless you add explicit knowledge of whether the string was base64-encoded.

Sure, the Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded. error will disappear, but the base64 decoding will continue to silently corrupt strings which weren't actually base64-encoded, yet just happen not to have any symbols which would be invalid in base64.

introduce some object/data structure

Data URLs seem like the perfect choice here, no need to invent a bicycle.

Please also note that in either case adding an additional format would be backwards-incompatible, just like the recent change to use JSON instead of PHP-serialized data in 2.2.

@vkublytskyi
Copy link

vkublytskyi commented Nov 27, 2017

@korostii

With all due respect, it's actually the exact opposite. Right now the knowledge is absent (there might be base64, or might be not). If you add an explicit prefix, then you know for sure whether it's base64 or not.

Agree now knowledge is absent. And this should be fixed. What I'm trying to say that if we will add just prefix then we will need to add this prefix handling in a lot of places. A risk to forget correctly handle this prefix may create new bugs in unpredictable places (we can not control how this string is used ion 3rd party extensions). So we need to join data (let's say some string format) and knowledge how they should be processed in one place. This knowledge should not belong to Base64 but to some another abstraction.

It's not possible to validate whether a string is base64 with 100% accuracy

But we may say with 100% accuracy that something is not base64 encoded string. So it may be useful to "fail early".

Data URLs seem like the perfect choice here, no need to invent a bicycle.

Please also note that in either case adding an additional format would be backwards-incompatible, just like the recent change to use JSON instead of PHP-serialized data in 2.2.

Totally agree. I also would prefer Data URLs. But JavaScript objects with toString method may allow as create some structure with additional info and provide a possibility to use it as a string to not introduce BiC.

Actually, BiC is also blocker for implementing simple prefix addition as this would be format change. Usage of prefix is not making possible to detect base64 strings but introduces own custom format on top of base64

@orlangur
Copy link
Contributor

Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on this pull request and it will be reopened.

@orlangur orlangur closed this Jan 24, 2018
@pmsteil
Copy link

pmsteil commented Nov 1, 2018

BTW: This issue still exists on Magento CE 2.2.5. I got the same error when pasting in this line:

<img id="LPMimage-15386411587091">

The '-' in the id causes the error.

@jothikavitha
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix improvement Progress: needs update Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants