Security changes from upstream 2.4.7-p3

[rhoerr]

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 magento/magento2@2.4.7-p2...2.4.7-p3 with all composer.json changes removed.

[fballiano]

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

[rhoerr]

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

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

• The upstream working branch is 2.4-develop. However, this always correlates to the latest development work on the next feature release.
• So, the 2.4-develop branch contains the code for 2.4.7, plus all development work for the upcoming 2.4.8 release.
• The 2.4-develop branch does not contain any of the security changes released in patch updates 2.4.7-p1 or -p2. Adobe’s position is those patch changes are on a separate internal branch, and that internal branch won’t be merged into 2.4-develop until they make a 2.4.8 release.
• That means 2.4-develop is (1) unstable, and (2) has known unresolved security vulnerabilities. (*I wrote this a few weeks ago -- this might have changed since with 2.4.8-beta1's release, but it still can't be considered fully stable.)
• That also means that in order to release the security patch updates ourselves, we’ve had to create separate branches like release/1.x and manually PR the changes from each -p# release (which are tagged upstream, but are not in the 2.4-develop branch we sync).

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]

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

[fballiano]

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]

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]

right, ok then this one is easy to merge after #106

[rhoerr]

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

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

[rhoerr]

@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]

@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]

@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]

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

[rhoerr]

@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]

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]

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]

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]

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]

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 rhoerr#2

[fballiano]

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]

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: rhoerr@dba4520

[fballiano]

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