Skip to content

fix(preview): check mime type before processing with Imagick #44710

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.

Sign up for GitHub

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

Jump to bottom
Merged
nickvergessen merged 1 commit into from
May 15, 2024
Merged

fix(preview): check mime type before processing with Imagick #44710

nickvergessen merged 1 commit into from
May 15, 2024

Conversation

@pulsejet
Copy link
Member

@pulsejet pulsejet commented Apr 8, 2024 •
edited by nickvergessen
Loading

No description provided.

@pulsejet pulsejet added 3. to review Waiting for reviews security php Pull requests that update Php code labels Apr 8, 2024
@pulsejet pulsejet added this to the Nextcloud 30 milestone Apr 8, 2024
@pulsejet pulsejet requested review from a team, come-nc, nickvergessen and skjnldsv April 8, 2024 02:37
@nickvergessen nickvergessen removed the request for review from a team April 8, 2024 03:05
nickvergessen
nickvergessen reviewed Apr 8, 2024
View reviewed changes
@pulsejet pulsejet force-pushed the pulsejet/imagick-check-type branch from fb5711d to ffe31a5 Compare April 8, 2024 03:18
nickvergessen
nickvergessen reviewed Apr 8, 2024
View reviewed changes
@pulsejet pulsejet force-pushed the pulsejet/imagick-check-type branch from ffe31a5 to 0e612ae Compare April 8, 2024 07:55
@pulsejet
Copy link
Member Author

pulsejet commented Apr 8, 2024

I made one change: the default behavior of the Bitmap class is now to match nothing rather than everything. As a result, any derived classes will not match anything unless getAllowedMimeTypes is defined (the breaking change is intentional). Let me know if this isn't desirable for any strong reason.

come-nc
come-nc reviewed Apr 8, 2024
View reviewed changes
@pulsejet pulsejet force-pushed the pulsejet/imagick-check-type branch 2 times, most recently from c46ecd6 to 31fc099 Compare April 8, 2024 08:30
@pulsejet pulsejet requested a review from nickvergessen April 8, 2024 08:33
@pulsejet pulsejet force-pushed the pulsejet/imagick-check-type branch from 31fc099 to 5d4d84b Compare April 8, 2024 09:17
come-nc
come-nc reviewed Apr 8, 2024
View reviewed changes
@pulsejet pulsejet force-pushed the pulsejet/imagick-check-type branch from 5d4d84b to 4ab40e3 Compare April 8, 2024 17:22
@pulsejet
Copy link
Member Author

pulsejet commented Apr 19, 2024

Bump

@pulsejet
Copy link
Member Author

pulsejet commented Apr 25, 2024

Bump (2)

@nickvergessen
Copy link
Member

nickvergessen commented Apr 25, 2024

yeah, there was a freeze recently for updates, so this could not proceed with all necessary energy

Altahrim
Altahrim reviewed Apr 25, 2024
View reviewed changes
susnux
susnux reviewed Apr 25, 2024
View reviewed changes
@nextcloud nextcloud deleted a comment from github-actions bot Apr 30, 2024
@nickvergessen nickvergessen self-assigned this May 2, 2024
@nickvergessen nickvergessen merged commit bd6989d into master May 15, 2024
@nickvergessen nickvergessen deleted the pulsejet/imagick-check-type branch May 15, 2024 20:13
@nickvergessen
Copy link
Member

nickvergessen commented May 15, 2024

/backport to stable29

@nickvergessen
Copy link
Member

nickvergessen commented May 15, 2024

/backport to stable28

@nickvergessen
Copy link
Member

nickvergessen commented May 15, 2024

/backport to stable27

@backportbot backportbot bot mentioned this pull request May 15, 2024
Merged
This was referenced May 15, 2024
Merged
Merged
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
@vermeeren
Copy link

vermeeren commented Nov 1, 2024

Hi @nickvergessen,

Recently I'm starting to use the HEIC format and saw the image previews are not working by default. I did some searching and found a nasty CVE from 2021 https://hackerone.com/reports/1261413 which was fixed by disabling the HEIC preview in #28077. With this change and checking the mime type it does seem safe to me to enable HEIC, but I do notice the default is still to keep it disabled.

Could you confirm whether it is considered secure to enable HEIC previews in the latest Nextcloud versions? I would love to have previews but of course not at the cost of a potentially critical security issue.

Many thanks!

@nickvergessen
Copy link
Member

nickvergessen commented Nov 6, 2024

Could you confirm whether it is considered secure to enable HEIC previews in the latest Nextcloud versions? I would love to have previews but of course not at the cost of a potentially critical security issue.

I'm not too much into previews and Imagick/HEIC things, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@susnux susnux susnux left review comments

@come-nc come-nc come-nc left review comments

@Altahrim Altahrim Altahrim approved these changes

@artonge artonge artonge approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

@nickvergessen nickvergessen

Labels

3. to review Waiting for reviews feature: previews and thumbnails feedback-requested php Pull requests that update Php code

Projects

None yet

Milestone

Nextcloud 30

Development

Successfully merging this pull request may close these issues.

8 participants

@pulsejet @nickvergessen @vermeeren @susnux @Altahrim @artonge @come-nc