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

URLSearchParams behavior inconsistent with spec, browser, node.js #1777

Closed
irvinebroque opened this issue Mar 7, 2024 · 8 comments · Fixed by #1778
Closed

URLSearchParams behavior inconsistent with spec, browser, node.js #1777

irvinebroque opened this issue Mar 7, 2024 · 8 comments · Fixed by #1778
Assignees

Comments

@irvinebroque
Copy link
Collaborator

The following code:

 new URLSearchParams(new URL('https://example.com/?q1=foo&q2=foo+bar&q3=foo bar&q4=foo/bar').searchParams).toString();

...returns the following on Workers:

q1=foo&q2=foo+bar&q3=foo+bar&q4=foo/bar

...but in Node.js, browsers, etc returns:

q1=foo&q2=foo+bar&q3=foo+bar&q4=foo%2Fbar

/ instead of %2

We need to understand why this is the case and if a regression from #1273

@anonrig
Copy link
Contributor

anonrig commented Mar 7, 2024

This was indeed a bug with Ada's character sets. I've fixed it and will include this in v2.7.7

@jasnell
Copy link
Member

jasnell commented Mar 7, 2024

Fixed with #1778

@Kaelten
Copy link

Kaelten commented Mar 7, 2024

This impacted our workers in a high severity way. @jasnell any idea how long this fix will take it to reach production? We need to understand if this is somethign we need to work around in the short term or not.

@jasnell
Copy link
Member

jasnell commented Mar 7, 2024

We ought to be able to get an update out fairly quickly. Let me look into it

@jasnell
Copy link
Member

jasnell commented Mar 7, 2024

Issue closed automatically because the workerd pr change landed. Working on updating the internal repo now and will update once the fix has rolled out to production.

@jasnell
Copy link
Member

jasnell commented Mar 7, 2024

@Kaelten ... let me know if you happen to spot anything else. We updated to use ada-url now under the covers which gave us a huge boost in both performance and spec compliance but it appears this one was missed to a gap in the test coverage (the web platform tests don't seem to even cover this particular case, which is fun). Overall, however, the implementation should be even more spec compliant now as a whole.

@ldileonardo
Copy link

This was deployed to production. Any reason this did not appear in the change log?

https://developers.cloudflare.com/changelog/?product=workers

@jaswrks
Copy link

jaswrks commented Mar 16, 2024

@jasnell @ldileonardo

console.log(new URLSearchParams(new URL('https://example.com/?sort=date%3Adesc').searchParams).toString());

Returns the following in Node and in browsers:
https://jsfiddle.net/jaswrks/a0x3j8L6/1/

sort=date%3Adesc

On Cloudflare workers it returns:

sort=date:desc

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 a pull request may close this issue.

6 participants