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

new URL() accepts array of string #41653

Closed
7c opened this issue Jan 22, 2022 · 6 comments · Fixed by #41658
Closed

new URL() accepts array of string #41653

7c opened this issue Jan 22, 2022 · 6 comments · Fixed by #41658
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@7c
Copy link

7c commented Jan 22, 2022

Version

v14.18.2

Platform

macos && ubuntu tested

Subsystem

No response

What steps will reproduce the bug?

Based on documentation and WHATWG URL API new URL() accepts a string as input. By tests i figured out that an array with single string is also accepted as seen here:

image

i understand this might be accepted but an array with 2 strings is not accepted, more problematic, it returns a VALID URL which should NOT be valid (see screenshot). Moreover same situation applies to Chrome, Firefox (latest versions).

This behaviour might cause a security issue, since especially passing an array with 2 strings does not throw any INVALID_URL error like the 4th case in the screenshot. People might inject stuff and this validation would fail. Even though this might be minor issue on browser, at the server-side this might be major thing

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

new URL(Array) should throw

What do you see instead?

image

Additional information

No response

@Trott
Copy link
Member

Trott commented Jan 22, 2022

This is perhaps a documentation bug but not a bug in the code.

The spec indicates that new URL() accepts a USVString. Any non-string JavaScript value sent to an API that accepts a USVString is first stringified. So, for the most part, any non-string value that has a toString() function is treated as if it is the value returned by that toString() function. So sending ['https://example.com'] is the same as sending 'https://example.com'.

MDN indicates that new URL() should accept any object with a stringifier.

I tested in Chrome and it happily accepts an array.

@Trott
Copy link
Member

Trott commented Jan 22, 2022

@nodejs/url

@jasnell
Copy link
Member

jasnell commented Jan 22, 2022

@Trott's explanation is correct. There's no bug here. The behavior is correct.

Trott added a commit to Trott/io.js that referenced this issue Jan 23, 2022
Trott added a commit to Trott/io.js that referenced this issue Jan 23, 2022
@VoltrexKeyva VoltrexKeyva added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 23, 2022
@benjamingr
Copy link
Member

As this isn't up to us but the URL standard (as explained by Rich and confirmed by James above - and if you're interested you can check the conversions in the WebIDL spec) - I think it's not something Node can/should fix.

If you feel strongly about this - please open an issue in the https://github.com/whatwg/url repo

As usual - if anyone feels this should be reopened please do so.

@Trott
Copy link
Member

Trott commented Jan 24, 2022

Doc change to clarify this: #41658

@benjamingr
Copy link
Member

Let's reopen until the doc clarification at #41658 lands then I missed that

@benjamingr benjamingr reopened this Jan 24, 2022
nodejs-github-bot pushed a commit that referenced this issue Jan 25, 2022
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
Closes: nodejs#41653

PR-URL: nodejs#41658
Fixes: nodejs#41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 8, 2022
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 2, 2022
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 3, 2022
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 14, 2022
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants