search

mage-os / mageos-magento2

Work in progress.
Open Software License 3.0
213 stars 45 forks source link

Security changes from upstream 2.4.7-p3 #107

Closed rhoerr closed 3 weeks ago

rhoerr commented 1 month ago

Description (*)

This PR pulls the changes from 2.4.7-p3 vs 2.4.7-p2 onto release/1.x. This brings it in line with 2.4.7-p3 for our next release.

I sourced the change list from https://github.com/magento/magento2/compare/2.4.7-p2...2.4.7-p3 with all composer.json changes removed.

fballiano commented 1 month ago

@rhoerr shouldn't this kind of upstream-sync happen automatically?

rhoerr commented 1 month ago

@rhoerr shouldn't this kind of upstream-sync happen automatically?

Unfortunately not in this case, because of how upstream handles security patch releases.

I hope that's clear, feel free to ask further if not.

Something to consider: At what point do we pull the next feature release (Magento 2.4.8) into a Mage-OS release? Now, with 2.4.8-beta1? Only upon the final upstream release, in April 2025? We do want a more rapid development and release cycle than Magento, but pulling in 2.4.8 in alpha or beta stage would risk the release of stability problems and incomplete development.

We released Mage-OS 1.0.0 shortly after Magento 2.4.7, so we have not had to seriously consider this before.

(cc @Vinai )

hostep commented 1 month ago

@rhoerr: yesterday all security fixes from the past 3 security patch releases got merged upstream in 2.4-develop (finally 😅), see: https://github.com/magento/magento2/commit/ba1598a6075af931b644e1834fce6725de2bbdc3, just FYI

fballiano commented 1 month ago

Something to consider: At what point do we pull the next feature release (Magento 2.4.8) into a Mage-OS release? Now, with 2.4.8-beta1? Only upon the final upstream release, in April 2025? We do want a more rapid development and release cycle than Magento, but pulling in 2.4.8 in alpha or beta stage would risk the release of stability problems and incomplete development.

IMHO i wouldn't merge untile 2.4.8 final

@rhoerr if what @hostep says it's true then we should get an automerge and we could close this PR right?

rhoerr commented 1 month ago

Thanks @hostep! Finally. 🙂

@rhoerr if what @hostep says it's true then we should get an automerge and we could close this PR right?

No, because the automerge is for 2.4-develop, which is (currently) 2.4.8-beta1, which we aren't releasing yet. We need these changes in Mage-OS 1.0.5 (which tracks release/1.x, branched from 2.4-develop after the last stable feature release 2.4.7).

release/1.x doesn't get any automerge changes, so it still needs the 2.4.7-p3 release to be manually applied or cherry picked. Unless I'm missing something.

Thanks

fballiano commented 1 month ago

right, ok then this one is easy to merge after https://github.com/mage-os/mageos-magento2/pull/106

rhoerr commented 1 month ago

No dependency on #106 because that's a separate branch.

We'll need this processed/approved before we can build 1.0.5.

rhoerr commented 1 month ago

@fballiano If you're able to help review this:

I got the changes from upstream tags 2.4.7-p2 .. 2.4.7-p3 -- which should take us right to equivalence with 2.4.7-p3. I stripped out release changes (build versions and dependency constraints) to composer.json files. https://github.com/magento/magento2/compare/2.4.7-p2...2.4.7-p3.patch

vs this PR: https://patch-diff.githubusercontent.com/raw/mage-os/mageos-magento2/pull/107.patch

fballiano commented 1 month ago

@rhoerr this PR brings in tinymce7, GPL code IMHO should not be part of our repos because of incompatibility with OSL3. Yes, this PR only replicates the security patches from upstream, but...

@Vinai what's you stand on this?

fballiano commented 1 month ago

@rhoerr PHPCS is complaining about

FILE: ...testsuite/Magento/Sales/Controller/Adminhtml/Order/Creditmemo/SaveTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 32 | ERROR | PHP syntax error: Cannot redeclare
    |       | Magento\Sales\Controller\Adminhtml\Order\Creditmemo\SaveTest::$resource

should we address it ourselves or we keep the error in our codebase?

fballiano commented 1 month ago

@rhoerr I've applied https://github.com/magento/magento2/compare/2.4.7-p2...2.4.7-p3 on top of release/1.x but I get a much bigger change, do you manually review all the changes?

EG: https://github.com/magento/magento2/compare/2.4.7-p2...2.4.7-p3.diff I get a change to app/i18n/Magento/en_US/composer.json but it's not a part of this PR and it seems to me our file on release/1.x should be updated.

rhoerr commented 1 month ago

@rhoerr this PR brings in tinymce7, GPL code IMHO should not be part of our repos because of incompatibility with OSL3. Yes, this PR only replicates the security patches from upstream, but...

@Vinai what's you stand on this?

I'm not opposed to us changing to TinyMCE6 before we release. We could fix the 2.4.7-p3 bugs with JS minifying and missing font size in the process. That should be a separate PR though, and we would need someone to actually do that work.

@rhoerr PHPCS is complaining about ...

should we address it ourselves or we keep the error in our codebase?

We can fix that -- small enough issue.

@rhoerr I've applied magento/magento2@2.4.7-p2...2.4.7-p3 on top of release/1.x but I get a much bigger change, do you manually review all the changes?

EG: https://github.com/magento/magento2/compare/2.4.7-p2...2.4.7-p3.diff I get a change to app/i18n/Magento/en_US/composer.json but it's not a part of this PR and it seems to me our file on release/1.x should be updated.

That's intentional -- no composer.json changes are included. I reviewed each one to confirm there aren't any impacting composer.json changes missed. The reason not to include those is the 2.4.7-p3 upstream tag is the built release (with package and dependency versions all resolved), but we need the unversioned/unresolved code for contributing purposes. Our release process creates similar composer.json changes for all the modules when it tags a release.

fballiano commented 1 month ago

I'm not opposed to us changing to TinyMCE6 before we release. We could fix the 2.4.7-p3 bugs with JS minifying and missing font size in the process. That should be a separate PR though, and we would need someone to actually do that work.

the thing is, once GPL code enters the repo... it will create legal issues, I don't know how upstream will handle this but I would avoid merging it at the moment.

magento2 (before this update) still was on tinymce5, they never updated to v6

@rhoerr PHPCS is complaining about ... should we address it ourselves or we keep the error in our codebase?

We can fix that -- small enough issue.

as you wish, I just wouldn't want to have a workflow error spreading to all next PRs on this branch

rhoerr commented 1 month ago

the thing is, once GPL code enters the repo... it will create legal issues, I don't know how upstream will handle this but I would avoid merging it at the moment.

magento2 (before this update) still was on tinymce5, they never updated to v6

I don't think we can realistically avoid that overall though. The GPL code was already merged and released upstream. We can avoid releasing it in Mage-OS Distribution by changing to 6, but it already hit our 2.4-develop branch from upstream: https://github.com/mage-os/mageos-magento2/tree/2.4-develop/lib/web/tiny_mce_7 -- undoing that and preventing it from hitting the commit log again seems impractical.

One way or another, we're missing security fixes from upstream (even if relatively minor this time) -- we can't wait too long to fix that.

fballiano commented 1 month ago

I don't think we can realistically avoid that overall though. The GPL code was already merged and released upstream. We can avoid releasing it in Mage-OS Distribution by changing to 6, but it already hit our 2.4-develop branch from upstream: https://github.com/mage-os/mageos-magento2/tree/2.4-develop/lib/web/tiny_mce_7 -- undoing that and preventing it from hitting the commit log again seems impractical.

if it was me I'd never release mageOS with GPL code in it

for the mirror, yeah I guess upstream messed it up and a mirror has to mirror so...

One way or another, we're missing security fixes from upstream (even if relatively minor this time) -- we can't wait too long to fix that.

tiny6 CVE gets fixed with this simple change, that's all, more on this here

hostep commented 1 month ago

Not sure if useful to the discussion here, but Nathan (head of security in Adobe for Magento) told me about 10 days ago in private that the reason they updated TinyMCE was for CVE-2024-38357. Anyway, just FYI.

fballiano commented 1 month ago

Not sure if useful to the discussion here, but Nathan (head of security in Adobe for Magento) told me about 10 days ago in private that the reason they updated TinyMCE was for CVE-2024-38357. Anyway, just FYI.

@hostep ok thanks, I thought it was the other CVE, anyway I see that the one you linked is marked as solved in tiny6.8.5 which is the one I'm adding to this PR through https://github.com/rhoerr/mageos-magento2/pull/2

fballiano commented 1 month ago

anyway, legally, updating to v7 is not viable since the license change. maybe adobe can afford to pay a commercial license to distribute tiny7 but IMHO this should be avoided at all costs in mageOS. it is simply not possible to include GPL software in a OSL one.

rhoerr commented 4 weeks ago

The change to TinyMCE 6 is merged (thanks Fabrizio!).

Once this PR is merged, I have an additional branch to PR with toolbar improvements compared to upstream: https://github.com/rhoerr/mageos-magento2/commit/dba45203d17616d303e13104d34f8d2977fd89ff

fballiano commented 4 weeks ago

@rhoerr if we don't get other reviews soon, I'll merge in the next couple of days max