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

Updated TinyMCE to 6.8 via composer 🚀 #3658

Merged
merged 17 commits into from
Dec 19, 2023
Merged

Updated TinyMCE to 6.8 via composer 🚀 #3658

merged 17 commits into from
Dec 19, 2023

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Nov 16, 2023

Update TinyMCE via composer/dependabot.

(something went wrong in #3643, so new clean (?) PR)

Related Pull Requests


From composer audit:

Package tinymce/tinymce
CVE CVE-2023-48219
Title TinyMCE vulnerable to mutation Cross-site Scripting via special characters in unescaped text nodes
URL GHSA-v626-r774-j7f8
Affected versions >=6.0.0,<6.7.3
Reported at 2023-11-15T18:32:34+00:00

@github-actions github-actions bot added composer Relates to composer.json JavaScript Relates to js/* labels Nov 16, 2023
@fballiano
Copy link
Contributor

tested and it works, notes:

  • I still think that the new plugin should be under the openmage organization (but that can be done a a later stage)
  • it seems to me that js/tinymce/langs folder is missing (it contains the translations for the editor)

@sreichel
Copy link
Contributor Author

js/tinymce/langs is not part of tinymce repo. Where does it comes from?

@fballiano
Copy link
Contributor

@sreichel
Copy link
Contributor Author

Added https://github.com/mklkj/tinymce-i18n that uses that :)

@fballiano
Copy link
Contributor

nice find

fballiano
fballiano previously approved these changes Nov 16, 2023
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

tested, works for me and it is a good improvement

@fballiano
Copy link
Contributor

because of GHSA-v626-r774-j7f8 it would be good if somebody could review this PR, we'd need to release it

@kiatng @addison74 @elidrissidev @Flyingmana @colinmollenhour

@sreichel
Copy link
Contributor Author

Only tested with git install!

If it does not work inside composer, i'v prepared a wrapper inside a magento-module. (or change plugin?)

Cons:

It needs to be updated hisself.

@fballiano
Copy link
Contributor

do you know for sure that it doesn't work with composer based projects? I can't find a way to test that case before it's merged

@sreichel
Copy link
Contributor Author

do you know for sure that it doesn't work with composer based projects?

No, just an idea - i also could not test it.

May setup a test-scenario, but this would take me some days.

@fballiano
Copy link
Contributor

better to merge and test on dev-main, extremely easier

composer.json Outdated Show resolved Hide resolved
Co-authored-by: Fabrizio Balliano <[email protected]>
@sreichel
Copy link
Contributor Author

That vulnerability requires us to speed up how TinyMCE is updated.

Appove?

If composer install does not work, revert it.

@fballiano fballiano merged commit 0e79abc into OpenMage:main Dec 19, 2023
18 checks passed
@fballiano
Copy link
Contributor

tested on a composer based project (using "openmage/magento-lts": "dev-main") but it doesn't work, @sreichel maybe it's just the "copy" plugin that doesn't work? could you check it?

@sreichel sreichel deleted the om-tinymce branch December 19, 2023 16:28
@sreichel sreichel restored the om-tinymce branch December 19, 2023 16:28
@sreichel
Copy link
Contributor Author

sreichel commented Dec 19, 2023

Thanks for testing.

Mhh. Not 100% about this, I guess plugins work only on top-level composer. Maybe its easier to create a magento-module that updates tinymce via dependabot?

(or add install intructions ... ?)

@fballiano
Copy link
Contributor

also if I add the plugin in my main composer.json (the project's one) it doesn't work :-(

@sreichel
Copy link
Contributor Author

@fballiano will test it later. Until ... can you give it a last try with verbose vvv and see if the files maby copied to wrong location?

@fballiano
Copy link
Contributor

ok, from what I see with -vvv

> post-update-cmd: Sreichel\Composer\Plugin\FileCopy\ScriptHandler->onPostCmd
The parameter handler needs to be configured through the extra.file-copy setting.

we'd need to have the extra.file-copy in the top-level composer.json? that's a real bummer

@sreichel
Copy link
Contributor Author

we'd need to have the extra.file-copy in the top-level composer.json?

Seems so :(

Actually extra. config is read from root-composer only. No time to debug yet, but found this ...

https://github.com/wikimedia/composer-merge-plugin/blob/master/README.md#merge-extra ....

@fballiano
Copy link
Contributor

but it's another plugin, nah at this point it's kinda too many plugins IMHO.

@sreichel
Copy link
Contributor Author

Everything is better then manual file updates :(

Better to work on a Mage_TineyMce module?

@sreichel
Copy link
Contributor Author

Lets revert for now. :(

@fballiano
Copy link
Contributor

I think it should be implemented in the magento composer plugin but... ye...

fballiano added a commit that referenced this pull request Dec 19, 2023
@sreichel sreichel deleted the om-tinymce branch December 19, 2023 19:30
@sreichel sreichel restored the om-tinymce branch December 19, 2023 19:35
@sreichel
Copy link
Contributor Author

sreichel commented Dec 19, 2023

If that plugin works, why not ... ? Same logic to merge extra.config had to be adapted to magento-composer-plugin.

@fballiano
Copy link
Contributor

because everybody would automatically (kinda) update to a new version of the magento-composer-plugin, but it's more difficult to make them install a new one (in their composer file)

fballiano added a commit that referenced this pull request Dec 20, 2023
@sreichel sreichel deleted the om-tinymce branch January 5, 2024 21:43
@addison74
Copy link
Contributor

I can't help but notice what a nice collaboration there was between contributors. It happened 9 months ago...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composer Relates to composer.json JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants