Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make libvips opt-in #30504

Merged
merged 9 commits into from
Jun 3, 2024
Merged

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented May 31, 2024

#30090 removes ImageMagick in favor of libvips, which has many benefits.

This PR fixes a few issue with it and requires libvips 8.13+ in order to disable unsafe and unused format support.

However, that requirement being pretty steep, with a lot of major distributions shipping earlier version, it changes libvips to be opt-in, behind the MASTODON_USE_LIBVIPS environment variable.

The idea being to deprecate ImageMagick in favor of libvips in 4.3, and drop support for ImageMagick in 4.4 or 5.0.

app/models/concerns/attachmentable.rb Outdated Show resolved Hide resolved
app/models/preview_card.rb Outdated Show resolved Hide resolved
config/application.rb Outdated Show resolved Hide resolved
@@ -1,3 +1,27 @@
# frozen_string_literal: true

Vips.block_untrusted(true) if Vips.at_least_libvips?(8, 13)
if ENV['MASTODON_USE_LIBVIPS'] == 'true'
ENV['VIPS_BLOCK_UNTRUSTED'] = 'true'
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same effect as the block_untrusted below I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes. My reasoning to go with the env var approach was to disable those as soon as possible.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this has an effect if we call block_untrusted below? I dont think that vips will run any foreign code if it is not called on an image.

@vmstan
Copy link
Sponsor Contributor

vmstan commented May 31, 2024

This is probably the right path at least initially for upgrading to 4.3 from previous versions. Would give folks time to figure out how to adopt it if we advertise the performance benefits. The container builds would be fine to switch to libvips.

Would we mark IM support as deprecated and then maybe remove it in 4.4 or 4.5?

@ClearlyClaire ClearlyClaire changed the title [WiP] Make libvips optional [WiP] Make libvips opt-in Jun 3, 2024
@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Jun 3, 2024

I still need to refactor it a bit, but I added a couple CI jobs running on Ubuntu 24.04 (ubuntu-latest still being 22.04 and shipping an older version of libvips that I decided to reject because we can't properly harden it) and run the paperclip_processing jobs. I think that's all the use cases of libvips, but maybe I'm missing something?

I also added an configuration.media_attachments.image_processor attribute to /api/v2/instance in order to help with troubleshooting. Added libvips version to the admin dashboard, when used.

I think all that remains is refactoring the ENV['MASTODON_USE_LIBVIPS'] == 'true' check, as well as ImageMagick version.

@ClearlyClaire ClearlyClaire marked this pull request as ready for review June 3, 2024 12:56
@ClearlyClaire ClearlyClaire changed the title [WiP] Make libvips opt-in Make libvips opt-in Jun 3, 2024
@ClearlyClaire ClearlyClaire requested a review from a team June 3, 2024 12:59
@@ -148,6 +148,93 @@ jobs:
env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

test-libvips:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use a re-usable workflow for this to deduplicate the code, but this can be done in a further PR

Dockerfile Outdated Show resolved Hide resolved
@@ -1,3 +1,27 @@
# frozen_string_literal: true

Vips.block_untrusted(true) if Vips.at_least_libvips?(8, 13)
if ENV['MASTODON_USE_LIBVIPS'] == 'true'
ENV['VIPS_BLOCK_UNTRUSTED'] = 'true'
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this has an effect if we call block_untrusted below? I dont think that vips will run any foreign code if it is not called on an image.

config/application.rb Outdated Show resolved Hide resolved
@ClearlyClaire ClearlyClaire merged commit 196f54a into feature-libvips Jun 3, 2024
57 checks passed
@ClearlyClaire ClearlyClaire deleted the features/optional-libvips branch June 3, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants