Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% #2629

kiatng · 2022-09-30T12:50:51Z

Description (*)

Config general/reprocess_images/active is removed and replaced with admin/security/reprocess_image_quality, where it is default to 85. ~~For BC, checking of general/reprocess_images/active takes precedent.~~

Fixed Issues (if relevant)

Fixes Remove magento validator jpeg force quality #475

app/code/core/Mage/Core/Model/File/Validator/Image.php

app/code/core/Mage/Core/etc/system.xml

ADDISON74 · 2022-09-30T15:03:59Z

This change must be accompanied by an explanation in the README. First of all, why was it introduced in Magento 1.9.3.3 and was it hardcoded until now. Strictly on security aspects. As has been discussed, it is not just a problem of not compressing an already compressed file, but a more complex, malicious one. If no suitable explanation can be found, the PRs/Issues where they were discussed can be indicated for the beginning.

kiatng · 2022-09-30T15:29:55Z

@ADDISON74 See the original comment here

magento-lts/app/code/core/Mage/Core/etc/config.xml

Lines 500 to 503 in fb12d0f

    
           <!-- NOTE: If you turn off images reprocessing, then your upload images process may cause security risks. --> 
        
           <reprocess_images> 
        
               <active>1</active> 
        
           </reprocess_images>

<!-- NOTE: If you turn off images reprocessing, then your upload images process may cause security risks. -->

This PR doesn't change what was introduced, if someone uses it in some custom config.xml, it will still work for them: either it is turn off or the image quality is set to 100. This takes precedent over the new feature.

The new feature is the ability to set the image quality in backend, which should satisfy everyone . Ref the discussion thread in issue #475, some want to turn it off, some wants to have another value for the image quality. With this PR, you can turn it off by setting the image quality to zero, or set any image quality you want.

kiatng · 2022-09-30T15:33:08Z

I think I found the reason: https://web.archive.org/web/20180406222439/https://magento.com/security/patches/supee-9767

elidrissidev · 2022-09-30T16:15:50Z

I think I found the reason: https://web.archive.org/web/20180406222439/https://magento.com/security/patches/supee-9767

Good find!

kiatng · 2022-09-30T16:20:21Z

This makes sense now:

magento-lts/app/code/core/Mage/Core/Model/File/Validator/Image.php

Line 92 in fb12d0f

//replace tmp image with re-sampled copy to exclude images with malicious data

elidrissidev

Looks solid. Suggestion to precise the commit message upon merge.

ADDISON74 · 2022-09-30T17:38:52Z

Let's wait a few more opinions before merging.

app/code/core/Mage/Core/Model/File/Validator/Image.php

colinmollenhour · 2022-09-30T19:36:04Z

Just FYI, OpenMage is much less susceptible to attacks using media uploads than Magento because we addressed the "allow_symlinks" option by removing it altogether and adding extra checks to prevent use of "../" in block template paths in #442 . So even if a malicious file was uploaded one would not be able to execute it with a template hack as far as we know. Still I think it is a good idea to reprocess these images to be safe but the CVEs referenced are completely irrelevant to OpenMage since it no longer even has an "AllowSymlinks" option and is probably more secure than even when it was on.

colinmollenhour · 2022-09-30T21:51:47Z

So for existing installations:

XML config present to set active=1 - continue using 100 (I added suggestion to change to 85 just because 100 is so inefficient I can't see anyone really wanting it to be 100)
XML config present to set active=0 - reprocessing is still disabled
No XML config present - use new default of 85, user can change via Admin > Security config

Only problem is users that changed their XML will not be able to modify via the UI without removing their XML update so this could be confusing/unexpected..

I propose changing the logic so the default value is empty and if there is a non-empty value for the new config always use it, then check for the legacy config and use it and otherwise default to 85.

sreichel · 2022-09-30T22:28:04Z

How about a RSS notification to re-set this value in admin config?

…recated config exists.

kiatng · 2022-10-01T12:25:11Z

@colinmollenhour actually described a bug (users not able to modify image quality in backend if deprecated config exists), and I like his fix, which is implemented:

The new config is no longer a required entry, image quality is null|'', which is default to 85. Most users, including me, would not have known what value to set, so the hard-coded default value serves us well. For other users, they can set it how they want and the value will be used.

In edge cases where there is a custom XML with the deprecated config, it means it wants to skip the image reprocessing. Otherwise, there is no reason to override the config. This PR will comply with the directive. Now, the same users come across the config in backend, they can set it to zero, to continue skipping image reprocessing, or they can set a value, which will take effect.

Thanks @colinmollenhour for the neat solution.

sreichel · 2022-10-02T22:04:36Z

Mhhh ... i'm not so happy to check for an orphan config entry just for BC.

How about an update script that move value from old to new config path?

if someone has set it manually in config.xml and saved config, it is stored to DB ...
optional ... if old value is above 85 (in case of not knowing the bad effects) it could/should reset to 85
still add RSS notification

kiatng · 2022-10-03T06:09:58Z

How about an update script that move value from old to new config path?

By "update script", do you mean add a file in app/code/core/Mage/Core/data/core_setup/data-upgrade-1.6.0.10-1.6.0.11.php? I'll give it a try.

still add RSS notification

That I do not know how to do.

colinmollenhour

Looks good to me.

@sreichel The code is only executed when files are uploaded so the overhead of checking a deprecated config value is nil and it's just a few lines of code. This seems like a sufficient solution to me and a database migration is extra coding time but for what added value? Probably 99% of users don't even know about this config option. The ones that disabled it will still get the same behavior but now have the option to set it in the GUI so win-win IMHO.

sreichel · 2022-10-03T23:30:52Z

@colinmollenhour it's not about "overhead". Why to deal with a non-existing config node?

Simple update script ....

<?php
$installer = $this;
$installer->startSetup();

# change config values
$installer->run("
    UPDATE {$this->getTable('core_config_data')}
    SET path = 'admin/security/reprocess_image_quality'
    WHERE path = 'general/reprocess_images/active';
");

$installer->endSetup();

Comment/note in config sections seems to be clear ... no need for special rss update ...

colinmollenhour · 2022-10-04T02:50:55Z

it's not about "overhead"

To me it's mostly a matter of time being well spent including/especially discussion time.. we have a working PR now.. and although it isn't complex, it is not quite that simple. The setup script you put in the comment isn't a commit so it can't be approved yet, it doesn't account for the value being set in the XML (XML values don't get saved to the database unless they are also in system.xml, I am sure of this) and the old value is boolean while the new one is 0-100.

kiatng · 2022-10-04T03:28:21Z

This statement

if someone has set it manually in config.xml and saved config, it is stored to DB ...

can be true or false, but in our case, it is false for general/reprocess_images/active.

Whereas this one

XML values don't get saved to the database unless they are also in system.xml

is true, I tested it.

It means the upgrade script needs to do a scan for general/reprocess_images/active in all the config.xml files, examine the value and do something about it. Let's not go this route.

ADDISON74 · 2022-10-04T07:47:58Z

This is the second PR in which I make comments about the wording of some phrases. They are not discussed but simply ignored. More attention should be paid in this regard because it is what the OM user read.

kiatng · 2022-10-04T12:57:09Z

@ADDISON74 Please comment again the ignored "wording of some phrases". If you can help to review again, I will look into it and incorporate them in this PR.

Regarding "an explanation in the README", can you help to make a PR after this is merged? Basically, the original code make jpeg image files too large due to saving it at 100% quality. This PR reduces it to 85%. The backend config is really cater for edge cases where the users can play with the quality setting. I do not really see a need to explain it in README, I think a note in release commit is sufficient.

ADDISON74 · 2022-10-04T13:05:38Z

@kiatng - If you opened this page in a browser you can see my comment above. It is also in "File changed" section.

elidrissidev · 2022-10-04T13:43:27Z

@ADDISON74 I can't see any review of yours here. Perhaps you forgot to submit it?

ADDISON74 · 2022-10-04T14:25:08Z

In Files changed tab, [Review changes] button has a badge. Here is a screenshot:

On the same page bellow you may find that review:

In Conversation tab, my comment can be found too. See bellow

My comment is 3 days old. I don't post to GitHub from Phpstorm, I just use the browser.

In other two merged PR's the same situation. I am starting to wonder if it is still necessary to make comments since they are being ignored.

fballiano · 2022-10-04T14:34:29Z

I learned that when it says “pending” you’re the only one that can see it. Tou have to “submit the review” or “add single comment” 😉

elidrissidev · 2022-10-04T14:36:21Z

Your comments are not being ignored, no one can see them yet because they are still pending, you have to actually submit the review ("Review changes" -> "Submit review") for it to be public.

This is a common confusion with the GitHub UI, it happened with Fabrizio before too.

app/locale/en_US/Mage_Core.csv

ADDISON74 · 2022-10-04T14:46:45Z

@fballiano - GitHub has an incomprehensible behavior, if I leave a comment it doesn't appear for the others (I logged out to check before). It is only displayed if I request changes. It should have been displayed as long as GitHub explicitly mentions that choosing Comment means "Submit general feedback without explicit approval". Is it a general feedback but it is displayed only for me? Anyway, I appreciate for pointing me out, I will pay more attention from now on to what is displayed when I am not logged in.

elidrissidev · 2022-10-04T14:53:06Z

It's for batching multiple comments in a single review. But you can always just choose "Add single comment".

ADDISON74 · 2022-10-04T15:02:55Z

I used "Add single comment" button but you may see the result above, it was hidden from everyone else. I had to start a review or request for a change. Of the two options, the change request has priority and cancels all previous approvals.

github-actions · 2022-10-05T08:22:22Z

Unit Test Results

1 files 1 suites 0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
7 runs 5 ✔️ 2 💤 0 ❌

Results for commit 6efea4b.

* Fixed "should return string but returns false" * Fixed "should return XYZ but returns false" * Fixed "should return array but returns null" * Fixed "should return string but returns null" * Fixed "should return int but returns null" * Fixed "should return bool but returns" * Fixed docs (see comments) * Fixed "should return array" * Update app/code/core/Mage/Adminhtml/Block/Widget/Grid/Massaction/Abstract.php Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com> * Update app/code/core/Mage/Catalog/Model/Product/Attribute/Tierprice/Api.php Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com> * Update app/code/core/Mage/Tag/Model/Resource/Tag.php Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com> * Update app/code/core/Mage/Customer/Block/Form/Register.php Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com> * Added module names to helper(#2617) * Get catalog search result collection from engine (#2634) * Add PHP dependencies security check workflow (#2639) * [security-workflow] Fixed cron syntax (#2640) * Added OpenMage Contributors Copyright (#2295) * Added ddev snippets to docs (#2575) * Improve dev/openmage/install.sh script for newer versions of Docker - no longer requires separate compose. * Only run workflows when relevant files change (#2641) * Add back notification popup severity icons URL (#2633) * Reduce reprocessed jpeg file size by defaulting quality to 85% (#2629) * Fixed issue #475. * Removed <frontend_type>text</frontend_type> as it is the default. * Fixed bug on users not able to modify image quality in backend if deprecated config exists. * Fixed bug on incorrect check if image quality was not set in backend. * Improved note in system.xml. * Prevented editing of a non-editable order (#2632) * Fix dev/openmage/install.sh script setting permissions on var directory. * Allowed automatic full invoice from API (#2393) * Add check if array key isset before using it (#2649) * Fixed phpstan-baseline.neon * Fixed phpstan-baseline.neon (updated dev tools) * Revert "Fixed phpstan-baseline.neon" This reverts commit 3c82e76. * Fixed getRegion() Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com> Co-authored-by: Mohamed ELIDRISSI <67818913+elidrissidev@users.noreply.github.com> Co-authored-by: Justin Beaty <51970393+justinbeaty@users.noreply.github.com> Co-authored-by: Colin Mollenhour <colin@mollenhour.com> Co-authored-by: luigifab <31816829+luigifab@users.noreply.github.com> Co-authored-by: Przemysław Piotrowski <przemyslaw.p@deligo.pl>

Fixed issue OpenMage#475.

eff61f9

github-actions bot added Component: Core Relates to Mage_Core translations Relates to app/locale labels Sep 30, 2022

kiatng requested review from fballiano, elidrissidev, ADDISON74, colinmollenhour, sreichel and tmotyl and removed request for fballiano September 30, 2022 12:53

elidrissidev reviewed Sep 30, 2022

View reviewed changes

app/code/core/Mage/Core/Model/File/Validator/Image.php Outdated Show resolved Hide resolved

elidrissidev reviewed Sep 30, 2022

View reviewed changes

app/code/core/Mage/Core/etc/system.xml Outdated Show resolved Hide resolved

Removed <frontend_type>text</frontend_type> as it is the default.

2df1069

elidrissidev previously approved these changes Sep 30, 2022

View reviewed changes

ADDISON74 previously approved these changes Sep 30, 2022

View reviewed changes

colinmollenhour reviewed Sep 30, 2022

View reviewed changes

app/code/core/Mage/Core/Model/File/Validator/Image.php Outdated Show resolved Hide resolved

Fixed bug on users not able to modify image quality in backend if dep…

12071f7

…recated config exists.

kiatng dismissed stale reviews from ADDISON74 and elidrissidev via 12071f7 October 1, 2022 11:54

Fixed bug on incorrect check if image quality was not set in backend.

e795d54

colinmollenhour previously approved these changes Oct 3, 2022

View reviewed changes

sreichel previously approved these changes Oct 4, 2022

View reviewed changes

kiatng changed the title ~~Fixed issue #475.~~ Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% Oct 4, 2022

ADDISON74 requested changes Oct 4, 2022

View reviewed changes

app/locale/en_US/Mage_Core.csv Outdated Show resolved Hide resolved

Improved note in system.xml.

25dd956

kiatng dismissed stale reviews from sreichel and colinmollenhour via 25dd956 October 5, 2022 01:25

ADDISON74 approved these changes Oct 5, 2022

View reviewed changes

elidrissidev approved these changes Oct 5, 2022

View reviewed changes

elidrissidev merged commit 6efea4b into OpenMage:1.9.4.x Oct 5, 2022

elidrissidev mentioned this pull request Jan 31, 2023

Cannot upload favicon in Windows ICO format (.ico) #3001

Closed

kiatng deleted the issue475_image_reprocessing branch February 27, 2023 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% #2629

Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% #2629

kiatng commented Sep 30, 2022 •

edited

Loading

ADDISON74 commented Sep 30, 2022

kiatng commented Sep 30, 2022

kiatng commented Sep 30, 2022 •

edited

Loading

elidrissidev commented Sep 30, 2022

kiatng commented Sep 30, 2022 •

edited

Loading

elidrissidev left a comment •

edited

Loading

ADDISON74 commented Sep 30, 2022

colinmollenhour commented Sep 30, 2022

colinmollenhour commented Sep 30, 2022

sreichel commented Sep 30, 2022 •

edited

Loading

kiatng commented Oct 1, 2022 •

edited

Loading

sreichel commented Oct 2, 2022

kiatng commented Oct 3, 2022

colinmollenhour left a comment

sreichel commented Oct 3, 2022

colinmollenhour commented Oct 4, 2022

kiatng commented Oct 4, 2022

ADDISON74 commented Oct 4, 2022

kiatng commented Oct 4, 2022 •

edited

Loading

ADDISON74 commented Oct 4, 2022

elidrissidev commented Oct 4, 2022 •

edited

Loading

ADDISON74 commented Oct 4, 2022 •

edited

Loading

fballiano commented Oct 4, 2022

elidrissidev commented Oct 4, 2022 •

edited

Loading

ADDISON74 commented Oct 4, 2022

elidrissidev commented Oct 4, 2022

ADDISON74 commented Oct 4, 2022

github-actions bot commented Oct 5, 2022

Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% #2629

Fixes issue #475, reduce reprocessed jpeg image file size by defaulting image quality to 85% #2629

Conversation

kiatng commented Sep 30, 2022 • edited Loading

Description (*)

Fixed Issues (if relevant)

ADDISON74 commented Sep 30, 2022

kiatng commented Sep 30, 2022

kiatng commented Sep 30, 2022 • edited Loading

elidrissidev commented Sep 30, 2022

kiatng commented Sep 30, 2022 • edited Loading

elidrissidev left a comment • edited Loading

Choose a reason for hiding this comment

ADDISON74 commented Sep 30, 2022

colinmollenhour commented Sep 30, 2022

colinmollenhour commented Sep 30, 2022

sreichel commented Sep 30, 2022 • edited Loading

kiatng commented Oct 1, 2022 • edited Loading

sreichel commented Oct 2, 2022

kiatng commented Oct 3, 2022

colinmollenhour left a comment

Choose a reason for hiding this comment

sreichel commented Oct 3, 2022

colinmollenhour commented Oct 4, 2022

kiatng commented Oct 4, 2022

ADDISON74 commented Oct 4, 2022

kiatng commented Oct 4, 2022 • edited Loading

ADDISON74 commented Oct 4, 2022

elidrissidev commented Oct 4, 2022 • edited Loading

ADDISON74 commented Oct 4, 2022 • edited Loading

fballiano commented Oct 4, 2022

elidrissidev commented Oct 4, 2022 • edited Loading

ADDISON74 commented Oct 4, 2022

elidrissidev commented Oct 4, 2022

ADDISON74 commented Oct 4, 2022

github-actions bot commented Oct 5, 2022

Unit Test Results

kiatng commented Sep 30, 2022 •

edited

Loading

kiatng commented Sep 30, 2022 •

edited

Loading

kiatng commented Sep 30, 2022 •

edited

Loading

elidrissidev left a comment •

edited

Loading

sreichel commented Sep 30, 2022 •

edited

Loading

kiatng commented Oct 1, 2022 •

edited

Loading

kiatng commented Oct 4, 2022 •

edited

Loading

elidrissidev commented Oct 4, 2022 •

edited

Loading

ADDISON74 commented Oct 4, 2022 •

edited

Loading

elidrissidev commented Oct 4, 2022 •

edited

Loading