-
-
Notifications
You must be signed in to change notification settings - Fork 764
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
Plugin crop/fix safety checks #743
Plugin crop/fix safety checks #743
Conversation
is this repo dead? 😢 |
@jagaSto please rebase so the CI runs. Getting this project set up with good CI/CD has been an issue. but now we have circleci setup (I think) so it should be easier to mange the project! |
thx for the hint @hipstersmoothie, will fix the tests once I find the time. |
@hipstersmoothie this is fixed now! |
🚀 PR was released in v0.6.7 🚀 |
What's Changing and Why
width
andheight
ofremainingPixels
are moved behind thepixelsToCrop
sanitization, otherwise they lead to wrong values if thepixelsToCrop
are negativeI encountered this in the following use case:
autocrop
function on an image, I specified aleaveBorder
value that was larger than the number of pixels that were cropped from the image. This lead to incorrect values forwidthOfRemainingPixels
andheightOfRemainingPixels
, as they use the negative values ofpixelsToCrop
instead of the sanitized ones.leaveBorder
is applied here:https://github.com/oliver-moran/jimp/blob/736054ef1bea6b6fb03db3e75f05cd922a9c104f/packages/plugin-crop/src/index.js#L209-L213
safety checks
are applied here:https://github.com/oliver-moran/jimp/blob/736054ef1bea6b6fb03db3e75f05cd922a9c104f/packages/plugin-crop/src/index.js#L224-L234
Moving the safety checks behind the sanitization leads to correct values.
Note: I was not able to build and test locally, due to this issue.
What else might be affected
crop
plugin is affectedTasks
jimp.d.ts