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

fix(ipx): fix retrieving images with query params #997

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

matthijsch
Copy link

@matthijsch matthijsch commented Sep 19, 2023

Problem
images containing query params cannot be retrieved in IPX because the query params are encoded

issues
#815
#944

Solution
by using ufo's normalizeURL the src is split up in host, pathname, query etc before any encoding is done. This way only the path name is encoded

before
/_ipx/w_500%26%26q_80/https://host.com/uploads/image%20file.jpg%3Fv=1%26foo=bar
after
/_ipx/w_500%26%26q_80/https://host.com/uploads/image%20file.jpg?v=1&foo=bar

Remaining problem
This solves retrieving images, what remains to be fixed is writing the image to the disk as Nitro prevent saving images with routes containing "?" since 2.6.0:
nitrojs/nitro#1474

@matthijsch matthijsch changed the title fix(ipx): fix encoding of query params in src fix(ipx): fix retrieving images with query params Sep 19, 2023
@danielroe danielroe requested a review from pi0 September 19, 2023 13:45
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (cb18ed1) 89.69% compared to head (21c45bf) 89.69%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #997   +/-   ##
=======================================
  Coverage   89.69%   89.69%           
=======================================
  Files          44       44           
  Lines        2920     2920           
  Branches      328      328           
=======================================
  Hits         2619     2619           
  Misses        300      300           
  Partials        1        1           
Files Coverage Δ
src/runtime/providers/ipx.ts 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pi0
Copy link
Member

pi0 commented Oct 2, 2023

Thanks for this PR.

(forwarding from discord)

While this solution might work to solve encoding issues it seems not safer. Adding URLs to the ends of a URL is already not totally safe for parsers and UFO normalize util tends to avoid extra optional encodings (following the vue-router approach). By doing this change, we can potentially introduce new edge cases.

We might investigate the upstream ipx server to handle double encoding.

I will try to follow up on this soon.

@danielroe
Copy link
Member

@pi0 Did you end up resolving upstream in ipx? I'm aware there's been a major release since this comment so I think likely.

@danielroe danielroe marked this pull request as draft February 24, 2024 09:54
@barayuda
Copy link

Hi everyone @pi0 @danielroe any update regarding this?
I got this error as well when do SSG

@TechAkayy
Copy link

TechAkayy commented Jul 23, 2024

Hi @pi0 @danielroe 🙂,

Can this PR be merged? It inherently fixes a key bug with nuxt generate when optimizing remote images with params. The params are getting double-encoded. Currently, the only workaround seems to be falling back to using the img tag.

I recently noticed some heated debate on Reddit - https://www.reddit.com/r/Nuxt/comments/1e7t9dp/is_nuxt_fit_for_static_sites_or_astro/?utm_source=share&utm_medium=mweb3x&utm_name=mweb3xcss&utm_term=1&utm_content=share_button.

The optimization to move this double-encoding avoidance upstream to ipx could be pursued separately.

This probably fixes a few existing issues with nuxt generate. Unless I'm mistaken, please correct me if you see any regression with this normalization.

Thanks 🙏🏾 bunch!

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

Successfully merging this pull request may close these issues.

6 participants