Skip to content

Commit

Permalink
WIP: Stop recommending imagemagick in the admin pages
Browse files Browse the repository at this point in the history
This tempts admins into installing insecure packages to make the
warning go away, even going so far as to [impurely modify running
containers](nextcloud/docker#1414 (comment)).

Changing the warning to trigger if the php-imagick module is loaded is
more in line with the actual upstream recommendation, and hopefully
helps unsuspecting users who have hacked around the warning in the
past realize this.

Draft because:

- [ ] Untested (will try building this all this evening)
- [ ] Translations are missing

Band-aid for nextcloud#1414, while we wait for a proper solution with libvips
or somesuch.
  • Loading branch information
TLATER committed Feb 25, 2022
1 parent 22a9c54 commit 11f9a71
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
4 changes: 2 additions & 2 deletions apps/settings/lib/Controller/CheckSetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -815,8 +815,8 @@ protected function isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(): bool {
return false;
}

protected function imageMagickLacksSVGSupport(): bool {
return extension_loaded('imagick') && count(\Imagick::queryFormats('SVG')) === 0;
protected function imageMagickLoaded(): bool {
return extension_loaded('imagick');
}

/**
Expand Down
8 changes: 4 additions & 4 deletions core/js/setupchecks.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,13 +424,13 @@
type: OC.SetupChecks.MESSAGE_TYPE_INFO
})
}
if (data.imageMagickLacksSVGSupport) {
if (data.imageMagickLoaded) {
messages.push({
msg: t(
'core',
'Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it.'
),
type: OC.SetupChecks.MESSAGE_TYPE_INFO
'Imagemagick is in use for document preview. This is not recommended out of security concerns, see the document preview documentation here:'

This comment has been minimized.

Copy link
@EngelPika32

EngelPika32 Feb 25, 2022

I'd recommend clarifying the document types Imagemagick affects. Otherwise, some not familiar with the topic might think this affects all image previews in general.
Something along the lines "Imagemagick is in use for SVG document previews. […]" might work.
(Though, I dunno if there are any other types/mime-types affected.)

And if we can say for certain, that it's a PHP module: "PHP-module imagemagick […]" (provide as much clear information as possible) =)

This comment has been minimized.

Copy link
@TLATER

TLATER Feb 25, 2022

Author Owner

I considered listing all the file types it's used for, but there are a variety and I haven't been able to pinpoint exactly how to list what Nextcloud will forward to imagemagick. If you can find the full list, I'd be happy to include a <ul> for that :)

The problem is with imagemagick itself, not just the PHP module: https://www.cvedetails.com/vulnerability-list/vendor_id-1749/Imagemagick.html. If you'd like to know more about the actual issue, the discussion that originally spawned this is here: nextcloud-snap/nextcloud-snap#629 (comment)

This comment has been minimized.

Copy link
@TLATER

TLATER Feb 25, 2022

Author Owner

Also, please hang on until I actually open a PR for this, makes the discussion much easier to follow ;)

) + " <a href='https://docs.nextcloud.com/server/stable/admin_manual/configuration_files/previews_configuration.html'>https://docs.nextcloud.com/server/stable/admin_manual/configuration_files/previews_configuration.html</a>",
type: OC.SetupChecks.MESSAGE_TYPE_WARNING
})
}
if (data.pendingBigIntConversionColumns.length > 0) {
Expand Down

0 comments on commit 11f9a71

Please sign in to comment.