cloudinary / js-url-gen

Cloudinary's base javascript library, including URL generation.
https://www.cloudinary.com
MIT License
47 stars 9 forks source link

toURL() function encodes already encoded URLs #575

Closed kyle-jorve closed 10 months ago

kyle-jorve commented 1 year ago

There's a similar issue in the next-cloudinary repo, here. After running transforms on an image using cloudinary/url-gen, then running toURL() on the result, if the original image URL had encoded characters (like %20 for a space, for example), the toURL() function will double-encode them (writing %2520 in place of &20, "%25" being the character code for a percent sign).

dannyv-cloudinary commented 1 year ago

Hi @kyle-jorve. Thanks for flagging this with us. I'll raise a ticket internally to get this addressed. I can't provide any ETA at this time, but looking at the modifications made in the next-cloudinary repo, I don't imagine it would be a particularly difficult change.

We'll provide you with further updates as we can.

dannyv-cloudinary commented 1 year ago

Hi @kyle-jorve. I was just putting together an example for the development team and testing some things out. As you mention, URL encoded public ID's throw errors, but unescaped are fine. I wonder if simply URL decoding your public ID's with decodeURI() before passing them to cld.image would be a suitable workaround in the meantime?

Please see this example CodeSandbox

https://codesandbox.io/s/beautiful-lovelace-kjyvpd?file=/src/index.js

kyle-jorve commented 1 year ago

@dannyv-cloudinary thanks for the suggestion. I'll give that a try.

stayko-chalakov commented 1 year ago

Hi @dannyv-cloudinary, we are hitting the same issue when using the next-cloudinary package and while decoding the image URLs in advance with decodeURIComponent fixes some of them, it breaks others e.g.

Here are 2 images: Working: https://res.cloudinary.com/dtljonz0f/image/upload/f_auto/q_auto/v1/gc-v1/cancun/Lucha%2520libre%252C%2520Tacos%2520and%2520Beer4?_a=BAVAdwE40 Broken: https://res.cloudinary.com/dtljonz0f/image/upload/f_auto/q_auto/v1/gc-v1/cancun/Sunrise%2520Wildlife%2520Stand-Up%2520Paddleboard%2520or%2520Kayak%2520Tour1-3-2-2?_a=BAVAdwE40

After decoding, CldImage generates: Broken: https://res.cloudinary.com/dtljonz0f/image/upload/f_auto/q_auto/v1/gc-v1/cancun/Lucha%20libre%252C%20Tacos%20and%20Beer4?_a=BAVAdwE40 Working: https://res.cloudinary.com/dtljonz0f/image/upload/f_auto/q_auto/v1/gc-v1/cancun/Sunrise%20Wildlife%20Stand-Up%20Paddleboard%20or%20Kayak%20Tour1-3-2-2?_a=BAVAdwE40

momoip commented 1 year ago

Hi @stayko-chalakov I've forwarded your question to @dannyv-cloudinary who is offline at the moment.

Vdeub-cloudinary commented 1 year ago

@stayko-chalakov Taking a step back, could you share how you are uploading the assets to your account? Some are encoded, and some are not. For example, for the Lucha Libre one, you have 2 images in your account:

You can see both in the screenshot below.

Screenshot 2023-09-27 at 11 52 10

For me the issue here is on the 2nd one, you are decoding all % but the one for the comma but the 1st URL is only working because you have the encoded one in your account.

ShaunMendhamLPG commented 1 year ago

@Vdeub-cloudinary cc: @stayko-chalakov

Taking a step back, could you share how you are uploading the assets to your account? Some are encoded, and some are not. For example, for the Lucha Libre one, you have 2 images in your account

I can add some detail to this one. We are using the auto-upload functionality to progressively migrate only our used assets.

Using the Lucha Libre image as an example:

Just validated the above by removing the two assets above and re-migrating it via the above mechanism. I assume that the other was generated as part of experimentation with decoding/encoding, so some cleanup for us to do there.

dannyv-cloudinary commented 1 year ago

Hi @ShaunMendhamLPG, @stayko-chalakov .

Thanks for the additional context. The internal ticket we have for this has been picked up by a member of the development team, though it's still in a to-do state. Hopefully it gets scheduled in for an upcoming sprint soon.

In the meantime, where possible, I would recommend either passing URLs that are already decoded, or continuing to use auto-upload.

tommyg-cld commented 10 months ago

Hi @ShaunMendhamLPG, @stayko-chalakov ,

So there were updates to toURL encoding on version 1.12.1 so please update to the latest version and let us know if you still experience the issue.

Thanks, Thomas

tommyg-cld commented 10 months ago

also another point I want to make is that you don't need to pre-encode special characters as the package will do it automatically for you.