-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(server): bump sharp to 0.34.3 + add unlimited
flag
#20327
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
base: main
Are you sure you want to change the base?
Conversation
@@ -141,6 +141,7 @@ export class MediaRepository { | |||
failOn: options.processInvalidImages ? 'none' : 'error', | |||
limitInputPixels: false, | |||
raw: options.raw, | |||
unlimited: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should presumably be guarded behind some sort of configuration option (e.g. an environment variable).
Enabling this should be carefully considered in deployments where trust in users is weaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on the potential issues or security concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could presumably create a malicious image which consumes enough memory to crash the server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it'd be better to make limitInputPixels
configurable? That sounds like it might also solve this and be a safer approach. Potentially it could be set based on available system memory, which might also prevent people from hitting this as often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could presumably create a malicious image which consumes enough memory to crash the server
Thank you, this is what I meant
the base-image is merged |
e9d4a34
to
a135c22
Compare
Description
This bumps
sharp
to0.34.3
and adds theunlimited
flag to disable all memory-related restrictions.Fixes #20028
To Do
I applied the changes locally in order to test the changes in this PR.
How Has This Been Tested?
Checklist:
src/services/
uses repositories implementations for database calls, filesystem operations, etc.src/repositories/
is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/
)