fix: always encode `=` in query key and values by undermoonn · Pull Request #164 · unjs/ufo

undermoonn · 2023-08-16T08:15:54Z

🔗 Linked issue

resolves #162

❓ 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

URL ponyfill output is different from encoding query value = with new URL

new URL

const url = new URL('https://unjs.io/')
url.searchParams.set('foo', 'a=1&b=1')

console.log(url.toString()) // output -> `https://unjs.io/?foo=a%3D1%26b%3D1`

withQuery

import { withQuery } from 'ufo'

const url = withQuery('https://unjs.io/', { foo: 'a=1&b=1' })

console.log(url) // output -> `https://unjs.io/?foo=a=1%26b=1`

https://url.spec.whatwg.org/#concept-urlencoded-serializer

Let name be the result of running percent-encode after encoding with encoding, tuple’s name, the application/x-www-form-urlencoded percent-encode set, and true.

Let value be the result of running percent-encode after encoding with encoding, tuple’s value, the application/x-www-form-urlencoded percent-encode set, and true.

From the URL serializing document, encodeQueryValue should be same as encodeQueryKey

📝 Checklist

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

pi0 · 2023-08-24T16:09:18Z

src/encoding.ts

 */
 export function encodeQueryKey(text: string | number): string {
-  return encodeQueryValue(text).replace(EQUAL_RE, "%3D");
+  return encodeQueryValue(text);


/cc @posva was there any specific reason = wasn't being encoded before? (This was taken from vue-router utils i guess)

https://github.com/vuejs/router/blob/c396d14b57be0da1e3504856e89d282f0666242f/packages/router/src/encoding.ts#L107

I don't remember exactly but I think that it's because it wasn't necessary to encode the = within a value since it already appeared after a key and the parser won't be expecting one until it gets into the state of reading a key. In other words, ?q=value=with=&b=other is parsable as the = only has a meaning when parsing a key

Indeed it is parsable and valid without encoding.. However URL sppec encoded like this. I think it would be nice if we could preserve a consistent behavior between ufo, vue-router (and URL) spec if you are okay I can make a PR to vue-router as well to keep them in sync and avoid mistmatching behavior between different places (as we use ufo in nuxt)

I will give it a look in 2-3 weeks. I did a lot of testing in different browsers when writing that code and I imagine that if I added 2 functions instead of one, there must be a reason 😆

Here is the discrepancy:

When directly entering some characters in a URL, the browser encodes some of them. The = within a query value, isn't encoded e.g. reads as = in location.href.

URLSearchParams() encodes =

Depending on the browsers, they display the value on the URL as encoded or not, therefore I tried, as much as possible, to display the original characters across browsers when they didn't need encoding. Also, Vue Router supports some browsers that do not have full URLSearchParams() support.

BTW I don't think the URL spec specifies that = must be encoded in query values.

When I did the research on what was supported and what the spec said, it turned out encodeURIComponent() was doing too much in some scenarios and that's why it's not used in vue router. Unless there is a bug, changing this behavior in vue router would be tiny breaking change, so it's better to keep it the way it is, being more readable across browsers.

Cheers

Thanks for detailed feedback. Is there a list of officially supported browsers by vue-router btw? And do you think it would be an issue if ufo (in a major version) choose to always encode things like = with URLSearchParams compatibility? (basically: does vue-router guarantees to decode encoded values like = in query even if it doesn't encodes it?)

We don't have a list, it's the same as Vue 3 😅

It's better to try by yourself, there might be problems with redirects and reloads. However, if I were you, since there is currently no problem with the encoding, I wouldn't change it in ufo either. The opened issue is fundamentally wrong, specifically, this statement is not true:

When we encode a query not a url. we should use encodeURIComponent not encodeURI.

The spec specifies encode sets, not functions to use. Since the issue lacks a real-world scenario with a problem, I would personally wait to see if this fixes anything, if not, it's better to keep the current behavior for the reasons I mentioned in my previous comment

The thing is, for the next major version of UFO I am thinking of relying on native URL and URLSearchParams for encoding and parsing since it has good coverage in platforms (I guess at least all Vue 3 supported targets -- have to double check) and as a result, we might end up with compatibility issues if didn't anticipate edge cases.

Thanks for feedbacks anyway, if we want to do this, i will certainly reach you out for more through tests across ecosystem packages.

FYI I wanted to rely on URL and URLSearchParams but there were small bugs in some old versions of browsers that forced me to not use them yet. I do hope to use them in the future major version of Vue Router though!

codecov · 2023-08-24T16:12:31Z

Codecov Report

Merging #164 (9442bab) into main (94347b0) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage   94.91%   94.92%           
=======================================
  Files           7        7           
  Lines         846      847    +1     
  Branches      177      177           
=======================================
+ Hits          803      804    +1     
  Misses         43       43

Files Changed	Coverage Δ
src/encoding.ts	`100.00% <100.00%> (ø)`

pi0 · 2024-02-05T16:12:31Z

Following up discussion, it seems we are not going to change behavior at least until next major version that likely will use URL and this behavior change by default.

If you spot any bugs with this current behavior we can certainly reconsider. and thanks for PR @undermoonn anyway ❤️

fix: URL ponyfill of query value encoding

9442bab

pi0 changed the title ~~fix: URL ponyfill of query value encoding~~ fix: always encode = in query key and values Aug 24, 2023

pi0 reviewed Aug 24, 2023

View reviewed changes

pi0 mentioned this pull request Aug 24, 2023

fix: query value encode #163

Closed

8 tasks

pi0 closed this Feb 5, 2024

This was referenced Feb 5, 2024

wrong = encoding in query values #162

Closed

normalizeURL encodes url incorrectly #165

Closed

Migrate to native URL #208

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: always encode `=` in query key and values#164

fix: always encode `=` in query key and values#164
undermoonn wants to merge 1 commit intounjs:mainfrom
undermoonn:fix/encoding

undermoonn commented Aug 16, 2023 •

edited by pi0

Loading

Uh oh!

pi0 Aug 24, 2023

Uh oh!

pi0 Aug 24, 2023

Uh oh!

posva Aug 24, 2023

Uh oh!

pi0 Aug 24, 2023 •

edited

Loading

Uh oh!

posva Aug 25, 2023

Uh oh!

posva Sep 13, 2023

Uh oh!

pi0 Sep 13, 2023

Uh oh!

posva Sep 13, 2023

Uh oh!

pi0 Sep 13, 2023 •

edited

Loading

Uh oh!

posva Sep 13, 2023

Uh oh!

codecov bot commented Aug 24, 2023

Uh oh!

pi0 commented Feb 5, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

undermoonn commented Aug 16, 2023 • edited by pi0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pi0 Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pi0 Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 24, 2023

Codecov Report

Uh oh!

pi0 commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

undermoonn commented Aug 16, 2023 •

edited by pi0

Loading

pi0 Aug 24, 2023 •

edited

Loading

pi0 Sep 13, 2023 •

edited

Loading

pi0 commented Feb 5, 2024 •

edited

Loading