unjs / ipx

🖼️ High performance, secure and easy-to-use image optimizer.
MIT License
1.2k stars 59 forks source link

Query parameter falsifies the image id #205

Open Ayax0 opened 6 months ago

Ayax0 commented 6 months ago

Environment

Node: 21.5.0 ipx: 2.0.2

Reproduction

I have built my own image provider with NuxtImage which provides the images via an API route. The reason for this procedure is that I add the access_token as a query parameter to the image url so that the images can only be accessed with authentication. If I now create a normal IPXH3 handler, I get the error message:

I have solved the problem by copying the createIPXH3Handler function from "/src/server.ts" and changing line 28 event.path to event.path.split("?")[0]. I don't know if it would be possible to add this in general or if there is a better solution.

Describe the bug

Query parameters are included in the image id

Additional context

No response

Logs

{"error":{"message":"[403] [IPX_FORBIDDEN_PATH] Forbidden path: /TestImage.png?test=123"}}
pi0 commented 5 months ago

Hi dear @Ayax0. Thanks for explaining your usecase.

From a quick look, I would say it probably worth to move authentication headers out of query parameters to the headers. query params are an unsafe place for it and can cause cache leak issues. (if you can share your project or a reproduction similar to it I can take a look)

Also omiting ? part of URL, can cause so many side cases that actually benefit from it being as part of source URL. So this is not a fix we can just make by default (but we can add behind an option, if makes sense)

As a simpler workaround you can make a wrapper handler like this:

const handler = eventHandler((event) => {
  event.path = event.path.split('?')[0]
  return ipxHandler(event)
})
Ayax0 commented 5 months ago

Hi @pi0 first of all, thank you for your great work in the whole nuxt / unjs ecosystem. I have now tested a little further and have moved the authentication to the header as explained here: https://github.com/nuxt/image/issues/984#issuecomment-1885713766

As you mentioned it might actually make more sense to make this function optional.

Thanks for your answer!