Remove magento validator jpeg force quality

[bastienlm]

I just realized, since the 1.9.3.3 update, magento forces the quality of the jpeg images to 100, which increases the weight of the image.

For me Magento should not perform this treatment.

[seansan]

​does the 100% in this case not read: do not change what is encoded? suppose an image is 100kb and we re-encode in photoshop to 80% and the size becomes 50kb the image is then 50kb and 50kb is considered the 100% from now on so by adjusting the quality in Magento you are re-encoding *again* and the image could be anything between 60% and 80% Not using Gmelius <https://gmelius.com?ref=mail> yet?

…

On Thu, Mar 29, 2018 at 1:06 PM, Bastien Lamamy ***@***.***> wrote: I just realized, since the 1.9.3.6 update, magento forces the quality of the jpeg images to 100, which increases the weight of the image. For me Magento should not perform this treatment. ------------------------------ You can view, comment on, or merge this pull request online at: #475 Commit Summary - Remove magento validator jpeg force quality File Changes - *M* app/code/core/Mage/Core/Model/File/Validator/Image.php <https://github.com/OpenMage/magento-lts/pull/475/files#diff-0> (3) Patch Links: - https://github.com/OpenMage/magento-lts/pull/475.patch - https://github.com/OpenMage/magento-lts/pull/475.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#475>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAn0a8mFUwmdzTMC7Q1t2JsFWNOqQYDfks5tjMAagaJpZM4TAJnd> .

[bastienlm]

@seansan I don't understand everything :)
but for the story about the weight, for me imagejpeg() increases the weight of the picture every time
https://stackoverflow.com/questions/11629768/image-resize-increment-file-size

I can't test this with photoshop right now, but I try this when I can.

[seansan]

​I added some links https://magento.stackexchange.com/questions/186405/supee-9767-patch-increasing-jpg-image-sizes https://stackoverflow.com/questions/16657540/magento-increases-image-sizes-on-the-front-end ​ Problem is I see no real proof anywhere. So would be really great to have a small test Not using Gmelius <https://gmelius.com?ref=mail> yet?

…

On Thu, Mar 29, 2018 at 5:16 PM, Bastien Lamamy ***@***.***> wrote: @seansan <https://github.com/seansan> I don't understand everything :) but for the story about the weight, for me imagejpeg() increases the weight of the picture every time https://stackoverflow.com/questions/11629768/image- resize-increment-file-size I can't test this with photoshop right now, but I try this when I can. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAn0a7TENU7c5egLUHH2IRxbAwtZcv4Tks5tjPrIgaJpZM4TAJnd> .

[colinmollenhour]

Recompressing an image with 100 quality will definitely result in a larger image if it was previously less than 100 so I can see how this would do what is claimed, I don't even think a test is necessary.

The question is, is it necessary? It would seem it was done for a good security reason so just removing it from core may not be a good option, but I don't know enough about what vulnerabilities can be put inside a JPG file (very brief Google search did not expose much). From my perspective uploading a catalog image is an admin action and there is only so much you can do to protect against malicious admins.. E.g. they could just insert an <img> tag somewhere in product description linking to a malicious jpg if one existed rather than uploading it to Magento to bypass this validation. Is it really Magento's responsibility to now be a virus scanner?

Without using ImageMagick to get the original quality I don't know of a good general purpose solution which keeps the recompression and does not assume a quality setting. Perhaps the "100" should be configurable? E.g. 85 is good for most users and saves a lot of space, but some people may want 90 or higher (even though above 90 there is almost no difference whatsoever).

Quickest fix: change to 85 or 90 to improve the situation for most users.
Best fix: I don't know...

[seansan]

Maybe the re code to ensure the data is an image afterwards.

…

On Thu, 29 Mar 2018 at 18:25, Colin Mollenhour ***@***.***> wrote: Recompressing an image with 100 quality will definitely result in a larger image if it was previously less than 100 so I can see how this would do what is claimed, I don't even think a test is necessary. The question is, is it necessary? It would seem it was done for a good security reason so just removing it from core may not be a good option, but I don't know enough about what vulnerabilities can be put inside a JPG file (very brief Google search did not expose much). From my perspective uploading a catalog image is an admin action and there is only so much you can do to protect against malicious admins.. E.g. they could just insert an <img> tag somewhere in product description linking to a malicious jpg if one existed rather than uploading it to Magento to bypass this validation. Is it really Magento's responsibility to now be a virus scanner? Without using ImageMagick to get the original quality I don't know of a good general purpose solution which keeps the recompression and does not assume a quality setting. Perhaps the "100" should be configurable? E.g. 85 is good for most users and saves a lot of space, but some people may want 90 or higher (even though above 90 there is almost no difference whatsoever). Quickest fix: change to 85 or 90 to improve the situation for most users. Best fix: I don't know... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAn0axJoCmSRnVXKSDSa8lFT6IodZofaks5tjQr4gaJpZM4TAJnd> .

[colinmollenhour]

Yes but there are two problems with it:

• It isn't just a check, it modifies the image making it take a lot more space. Perhaps the user took great care to save it with proper color profiles, squash the size losslessly, etc.. Now Magento just blindly recompresses it with the crappy GD library using 100 quality.
• This assumes that GD doesn't have vulnerabilities from reading jpeg files (like imagemagick did for PNG files not too long ago) because if it does then the validation itself will expose a vulnerability. Not likely, but then it seems whatever vulnerability they are trying to protect from is about equally unlikely.

Perhaps this recompression should be a configurable option for those that are more concerned with photos than this particular security vulnerability or have sufficient workarounds in place?

[bastienlm]

Magento already leave the choose to make the reprocess image with the config
'general/reprocess_images/active'

/** if 'general/reprocess_images/active' false then skip image reprocessing. */
if (!Mage::getStoreConfigFlag('general/reprocess_images/active')) {
      return null;
}

So, I think, and colin already say that, this upload is only available for admin users and if you trust your admin user, you can disable reprocess images.
For me this config is maybe too generic and it's maybe interesting to disable only the jpeg reprocess.

[Flyingmana]

There can be so many reasons why this was added, but there is a config to disable it, so I dont see a need to change the code.

[seansan]

There does not seem to be any backend gui config for it, just a flag in app/code/core/Mage/Core/etc/config.xml

https://github.com/OpenMage/magento-lts/search?utf8=%E2%9C%93&q=reprocess&type=

So adding a backend config flag to enable/disable would be a good idea

[seansan]

After looking into this (and running into this exact problem: result was bloated bloated 200-300kb images uploaded as 30-40kb) ... terrible

I came to this conclusion. If we can detect the CompressionQuality then just store it like it was

If we cant detect it then keep the default (which is 75)

I think the wrong here is that it overwrites the default behavior of imagejpeg which has a quality of 75 to 100

Leading to the unexpected behavior

                            if( class_exists("Imagick") ) {
                                $img = new Imagick($filename);
                                $quality = $img->getImageCompressionQuality();
                                if ($quality > 0) {
                                    imagejpeg($img, $filePath, $quality);
                                }
                            } else {
                                imagejpeg($img, $filePath);
                            }

[partounian]

Bump

[bastienlm]

Reprocessing cause other problem with gif format. Dynamic gif media become static after reprocessing...

[sreichel]

After looking into this (and running into this exact problem: result was bloated bloated 200-300kb images uploaded as 30-40kb) ... terrible

I came to this conclusion. If we can detect the CompressionQuality then just store it like it was

If we cant detect it then keep the default (which is 75)

I think the wrong here is that it overwrites the default behavior of imagejpeg which has a quality of 75 to 100

Leading to the unexpected behavior

                            if( class_exists("Imagick") ) {
                                $img = new Imagick($filename);
                                $quality = $img->getImageCompressionQuality();
                                if ($quality > 0) {
                                    imagejpeg($img, $filePath, $quality);
                                }
                            } else {
                                imagejpeg($img, $filePath);
                            }

Where is this code from?

[tmotyl]

A little context about potential security issues.
The problem is that its not enough to just check if the image has valid image headers/content inside.
There were several attacts to PHP applications which uses files which at the same time balid images and contained malicious code.
E.g. a valid jpeg file being at the same time a valid phar archive (and using some unserialize as attac vector). You can read more about this example (and how to protect yourself) here: https://github.com/TYPO3/phar-stream-wrapper

Is this validator used only in admin context? Or is /can be also used when Magento allows to upload files by customers?

[colinmollenhour]

Thanks for the info @tmotyl, that's kinda along the lines of what I assumed the issue was.

In a properly configured web server it should not be possible to execute any files in the media directory so it is a risk that should already be fully mitigated. That's not to say that allowing phar files to be uploaded to your media directory is a good practice so the more layers of security the better. :)

What about adding some basic detection of a hidden phar file when the auto re-encoding is disabled? E.g. searching for __HALT_COMPILER() in the contents of the jpg (apparently this is a requirement for phar files, based on my brief research - https://www.php.net/manual/en/phar.fileformat.stub.php).

[fballiano]

What should we do with this? Add a configuration for general/reprocess_images/active? Change the logic to detect the compression level? Close the issue and leave everything as it is?

[addison74]

I read all the posts from the links mentioned by @seansan. As far as I understand the quality of a new image when it is resized from another is questioned. Opinions are divided, some say that by resizing the new saved image has a larger size, others say that the issue is not confirmed.

I did a test by processing a high resolution TIFF image in Photoshop where I export it as a JPEG at a (width/height of 1600px, resolution 72, quality 100). The file size that I call it master was 255 kB. I opened this image in Photoshop where I saved it as JPEG at a (width/height of 800px, resolution 72, quality 100). This time the file size was 233 kB. Then I opened this image and saved it again at a (width/height of 400px, resolution 72, quality 100). its size was 89 KB.

Extrapolating this test that I did to the situation in Magento when saving a master image to a smaller size it should not increase its size on disk. Well, I looked in all directories in /catalog/products/cache and I did not find an larger than the master. All are smaller as file size.

The quality of a resized image should be configured in the Backend in no case eliminated as I saw that it is proposed by this PR. Any administrator can decide what quality fits his needs. But if he chooses for a quality less than 100 as it is now hardcoded maybe he will get a better file size, but he will lose the visual quality. I didn't confirm that the image size increases in disk size and I searched a lot on Internet related to php imagejpeg(). Maybe we should look into the Magento 2 code and see if there have been any changes on this topic.

My opinion asking to @fballiano we need that configuration in Backend. I am evaluating if detecting compression/quality level brings any benefits.

[colinmollenhour]

Extrapolating this test that I did to the situation in Magento when saving a master image to a smaller size it should not increase its size on disk.

Your test does not use the same methodology that Magento uses, it may be true for Photoshop but it is not for Magento/PHP.

Here is a real test done with two images, one is a high quality wallpaper weighing in a 4.3MB, and the other is the same image that was saved at 75% quality that is almost half the size.

Test source, copied from the Magento code to have the exact same resampling strategy:

<?php

function recompress($filePath) {
  var_dump(filesize($filePath));
  list($imageWidth, $imageHeight, $fileType) = getimagesize($filePath);
  $image = imagecreatefromstring(file_get_contents($filePath));
  $img = imagecreatetruecolor($imageWidth, $imageHeight);
  imagealphablending($img, false);
  imagecopyresampled($img, $image, 0, 0, 0, 0, $imageWidth, $imageHeight, $imageWidth, $imageHeight);
  imagesavealpha($img, true);
  imagejpeg($img, $filePath.'X', 100);
  var_dump(filesize($filePath.'X'));
}
recompress('./original.jpg');
recompress('./quality-75.jpg');

Results:

/var/www/html/test.php:4:
int(4330074)
/var/www/html/test.php:12:
int(9176579)
/var/www/html/test.php:4:
int(2423079)
/var/www/html/test.php:12:
int(5843514)

So the Magento strategy is extremely inefficient and doubles the image size in both cases.

Changing the hard-coded 100 to 90 has much better results, the original image is slightly smaller and the 75% image is slightly larger:

/var/www/html/test.php:4:
int(4330074)
/var/www/html/test.php:12:
int(3701318)
/var/www/html/test.php:4:
int(2423079)
/var/www/html/test.php:12:
int(2761400)

At 85 the smaller version stays almost the same size:

/var/www/html/test.php:4:
int(4330074)
/var/www/html/test.php:12:
int(3232244)
/var/www/html/test.php:4:
int(2423079)
/var/www/html/test.php:12:
int(2457552)

So a quick fix would be change 100 to 85, but it is possible that someone who hates bandwidth and load times could not like this change. For everyone else it would be a huge win.

[addison74]

I appreciate your reply Colin. PHP testing highlights the issue.

In this case my opinion would be to have that input box in the Backend where the value can be set and we should use 85, making a brief clarification to consider what happens if it is 100. Also, we can explain more in detail in another file that we use for documentation.

My proposal is to close this PR and make a new one with the necessary clarifications.

[addison74]

Here are some suggestions on disabling image quality https://magento.stackexchange.com/questions/186405/supee-9767-patch-increasing-jpg-image-sizes.

insert into core_config_data (scope, scope_id, path, value) values ('default', '0', 'general/reprocess_images/active', '0')

case IMAGETYPE_JPEG:
   imagejpeg ($img, $filePath);

Both options are obscure in my opinion and I think the input box version in Backend is the way to go. If the value is set to zero covers both suggestions, but there is an opportunity to allow the administrator to add whatever quality he wants, as you suggested 85-90 should be ideal.

[tmotyl]

I'm ok with allowing admin to enable/disable image processing on upload.
It should be marked as potentially dangerous with link to security notice.
However if a shop owner has different means of ensuring security (e.g. by not allowing customers to upload data, and haveing means to validate/trust administrators) then its good to go.

[fballiano]

I wanted to create a backend configuration for this, using the general/reprocess_images/active path but it doesn't make sense to have it in the general section, it should be in the admin section. so I don't know.

[kiatng]

+1 To move general/reprocess_images/active to admin/security/..... After all, it is a security setting.

[fballiano]

But if somebody tweaked this setting in their config.xml? :-\