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

Use IPX as high performance, secure and easy-to-use image optimizer. #2

Open
4 tasks done
Murderlon opened this issue Dec 19, 2024 · 8 comments
Open
4 tasks done
Labels
👍 phase/yes Post is accepted and can be worked on

Comments

@Murderlon
Copy link
Member

Initial checklist

Problem

One thing that could be expected/wanted from an image proxy is also optimizing images and/or changing the image format.

Current solutions

This was already on our minds but building it yourself from scratch is a lot of work.

Proposed solutions

unjs is a great ecosystem focussed on universal tooling, much like unified's philosophy. They have an image optimizer which can be easily integrated into Node.js servers.

https://github.com/unjs/ipx

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Dec 26, 2024

Sounds interesting!
Worth looking into.

One consideration unpacking images opens up a resource exhaustion risk.
For example https://www.bamsoftware.com/hacks/deflate.html

It could be good to have defaults that mitigate these risks.

@wooorm
Copy link
Member

wooorm commented Jan 2, 2025

I worry a bit about using a tool that wraps other tools.
Especially but not solely for security stuff like what Christian mentions.

If the proposed solution is already very simple to integrate into a server, then what is missing?
Would an example showing how to use 2 tools together solve this issue?

@Murderlon
Copy link
Member Author

If the proposed solution is already very simple to integrate into a server, then what is missing?

Briefly looking at it again, ipx seems to be mostly designed to be used as req/res middleware. However that doesn't work in our case, even if we introduce camomile.use(), as we need it to operate on the result of the fetch, not the original request.

So we'd have to add something like options.onBufferLoaded(buffer: Buffer): Buffer and add an example with a lower level thing from ipx, not the request handler, to transform the image. This works because we actually buffer the whole image into memory before moving on.

@wooorm
Copy link
Member

wooorm commented Jan 8, 2025

sharp allows many manipulations, and ipx seems to allow that too. So that’s a bit more complex.
But svgo is more simple (although: svg is also unsafe, not sure we should support it? I don’t think GH does normally either).
But, we could have examples such as this in the readme here?https://github.com/unjs/ipx/blob/f8455cd466aef067f8645ebfa592f57449811532/src/ipx.ts#L170-L191

@Murderlon
Copy link
Member Author

So that’s a bit more complex

Yeah in which case I would say building it inside camomile with an opt-in option still isn't a bad idea, especially since we're tightly coupled to formats we support. If we decide we want it, and we determine the code is non-trivial (too big for an example), then it's better to figure it out with security in mind once instead of pushing the risk to implementers.

@wooorm
Copy link
Member

wooorm commented Jan 9, 2025

What is the it? sharp? Or ipx, or a hook?

The it I’d lean to would be an example for sharp, plus hooks in the API where needed to make the example work. From what I understand sharp + 20 lines of example code + 1 hook could solve this issue?

Indeed, if it is non-trivial, and we can make it secure, optionally with ipx, then we could expose an implementation of that hook in the future?

The hook would be options.onBufferLoaded you talk about. From what I see in the code, and given the use case, I’m thinking it would be called here. For a name, perhaps options.transform or transformImage or transformResponse might be better? That hook might also want to get the mime type we already inferred and checked? Perhaps other headers might be useful for sharp/svgo?

@Murderlon
Copy link
Member Author

The it I’d lean to would be an example for sharp, plus hooks in the API where needed to make the example work. From what I understand sharp + 20 lines of example code + 1 hook could solve this issue?

Yes that sounds good.

@wooorm wooorm added the 👍 phase/yes Post is accepted and can be worked on label Feb 21, 2025
Copy link

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👍 phase/yes Post is accepted and can be worked on
Development

No branches or pull requests

3 participants