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

feat: add limits for modifiers and dimensions #152

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ausir0726
Copy link

πŸ”— Linked issue

#45

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Based on the issues mentioned, using encrypted URLs as a deterrent against abuse should be considered. However, inspired by Cloudinary's approach (where the server stops serving images beyond 8000px, which likely meets current screen usage), and taking into account the limitations on images in the ImageKit.io documentation, we have decided to impose restrictions on the usage of modifiers. Additionally, requests for width or height must conform to the maximum limits. This configuration serves as the minimum guarantee to prevent server abuse when URL encryption is not applied.

Furthermore, an example of using IPX_DOMAINS is added because it defaults to an empty array, but the input value is in the form of a comma-separated string, leading to confusion due to the different data types in use.

ImageKit.io doc about limits image : https://docs.imagekit.io/limits-and-troubleshooting/limits

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@ausir0726
Copy link
Author

Taking into consideration the deployment issues of existing users and to avoid any breaking changes, the default values will retain all settings. By limiting the maximum pixels, potential damage caused by attacks can be minimized.

README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #152 (5af59c2) into main (606400a) will decrease coverage by 0.52%.
The diff coverage is 43.24%.

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
- Coverage   56.91%   56.39%   -0.52%     
==========================================
  Files          10       10              
  Lines         940      977      +37     
  Branches       41       42       +1     
==========================================
+ Hits          535      551      +16     
- Misses        405      426      +21     
Files Changed Coverage Ξ”
src/ipx.ts 79.65% <43.24%> (-6.95%) ⬇️

@ausir0726
Copy link
Author

Is there any way I can continue to promote this PR? Because we're suffering from memory crashes due to mass production of images.

Copy link
Member

atinux commented Aug 8, 2023

I guess until @pi0 can have a look at it, you can use your fork version @ausir0726

@pi0 pi0 added the v1 label Sep 5, 2023
@jameswragg
Copy link
Contributor

I wonder if validating modifiers via a Joi, zod or typebox schema would be the better approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants